Skip to content

Commit

Permalink
Correct scrolling invalidation region for tmux in pty w/ bitmap (#5122)
Browse files Browse the repository at this point in the history
Correct scrolling invalidation region for tmux in pty w/ bitmap

Add tracing for circling and scrolling operations. Fix improper
invalidation within AdjustCursorPosition routine in the subsection about
scrolling down at the bottom with a set of margins enabled.

## References
- Introduced with #5024 

## Detailed Description of the Pull Request / Additional comments
- This occurs when there is a scroll region restriction applied and a
  newline operation is performed to attempt to spin the contents of just
  the scroll region. This is a frequent behavior of tmux.
- Right now, the Terminal doesn't support any sort of "scroll content"
  operation, so what happens here generally speaking is that the PTY in
  the ConHost will repaint everything when this happens.
- The PTY when doing `AdjustCursorPosition` with a scroll region
  restriction would do the following things:

1. Slide literally everything in the direction it needed to go to take
   advantage of rotating the circular buffer. (This would force a
   repaint in PTY as the PTY always forces repaint when the buffer
   circles.)
2. Copy the lines that weren't supposed to move back to where they were
   supposed to go.
3. Backfill the "revealed" region that encompasses what was supposed to
   be the newline.

- The invalidations for the three operations above were:

1. Invalidate the number of rows of the delta at the top of the buffer
   (this part was wrong)
2. Invalidate the lines that got copied back into position (probably
   unnecessary, but OK)
3. Invalidate the revealed/filled-with-spaces line (this is good).

- When we were using a simple single rectangle for invalidation, the
  union of the top row of the buffer from 1 and the bottom row of the
  buffer from 2 (and 3 was irrelevant as it was already unioned it)
  resulted in repainting the entire buffer and all was good.

- When we switched to a bitmap, it dutifully only repainted the top line
  and the bottom two lines as the middle ones weren't a consequence of
  intersect.

- The logic was wrong. We shouldn't be invalidating rows-from-the-top
  for the amount of the delta. The 1 part should be invalidating
  everything BUT the lines that were invalidated in parts 2 and 3.
  (Arguably part 2 shouldn't be happening at all, but I'm not optimizing
  for that right now.)

- So this solves it by restoring an entire screen repaint for this sort
  of slide data operation by giving the correct number of invalidated
  lines to the bitmap.

## Validation Steps Performed
- Manual validation with the steps described in #5104
- Automatic test `ConptyRoundtripTests::ScrollWithMargins`.

Closes #5104
  • Loading branch information
miniksa authored Mar 27, 2020
1 parent d7123d5 commit ef80f66
Show file tree
Hide file tree
Showing 14 changed files with 505 additions and 20 deletions.
4 changes: 4 additions & 0 deletions .github/actions/spell-check/whitelist/whitelist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ CBoolean
cbt
cbuffer
CCCBB
CCCC
ccf
cch
CCHAR
Expand Down Expand Up @@ -534,6 +535,7 @@ dcommon
DCompile
dcompiler
ddb
DDDD
dde
DDESHARE
DDevice
Expand Down Expand Up @@ -710,6 +712,7 @@ edputil
edu
eeb
eee
EEEE
Efast
EHsc
EJO
Expand Down Expand Up @@ -1928,6 +1931,7 @@ pythonw
qi
QJ
qo
QQQQ
qsort
queryable
QUESTIONMARK
Expand Down
247 changes: 247 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
static const SHORT TerminalViewWidth = 80;
static const SHORT TerminalViewHeight = 32;

// This test class is for tests that are supposed to emit something in the PTY layer
// and then check that they've been staged for presentation correctly inside
// the Terminal application. Which sequences were used to get here don't matter.
TEST_CLASS(ConptyRoundtripTests);

TEST_CLASS_SETUP(ClassSetup)
Expand Down Expand Up @@ -171,6 +174,8 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final

TEST_METHOD(TestResizeHeight);

TEST_METHOD(ScrollWithMargins);

private:
bool _writeCallback(const char* const pch, size_t const cch);
void _flushFirstFrame();
Expand Down Expand Up @@ -1055,3 +1060,245 @@ void ConptyRoundtripTests::PassthroughHardReset()
TestUtils::VerifyExpectedString(termTb, std::wstring(TerminalViewWidth, L' '), { 0, y });
}
}

void ConptyRoundtripTests::ScrollWithMargins()
{
auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& hostSm = si.GetStateMachine();

auto& hostTb = si.GetTextBuffer();
auto& termTb = *term->_buffer;
const auto initialTermView = term->GetViewport();

Log::Comment(L"Flush first frame.");
_flushFirstFrame();

// Fill up the buffer with some text.
// We're going to write something like this:
// AAAA
// BBBB
// CCCC
// ........
// QQQQ
// ****************
// The letters represent the data in the TMUX pane.
// The final *** line represents the mode line which we will
// attempt to hold in place and not scroll.

Log::Comment(L"Fill host with text pattern by feeding it into VT parser.");
const auto rowsToWrite = initialTermView.Height() - 1;

// For all lines but the last one, write out a few of a letter.
for (auto i = 0; i < rowsToWrite; ++i)
{
const wchar_t wch = static_cast<wchar_t>(L'A' + i);
hostSm.ProcessCharacter(wch);
hostSm.ProcessCharacter(wch);
hostSm.ProcessCharacter(wch);
hostSm.ProcessCharacter(wch);
hostSm.ProcessCharacter('\n');
}

// For the last one, write out the asterisks for the mode line.
for (auto i = 0; i < initialTermView.Width(); ++i)
{
hostSm.ProcessCharacter('*');
}

// no newline in the bottom right corner or it will move unexpectedly.

// Now set up the verification that the buffers are full of the pattern we expect.
// This function will verify the text backing buffers.
auto verifyBuffer = [&](const TextBuffer& tb) {
auto& cursor = tb.GetCursor();
// Verify the cursor is waiting in the bottom right corner
VERIFY_ARE_EQUAL(initialTermView.Height() - 1, cursor.GetPosition().Y);
VERIFY_ARE_EQUAL(initialTermView.Width() - 1, cursor.GetPosition().X);

// For all rows except the last one, verify that we have a run of four letters.
for (auto i = 0; i < rowsToWrite; ++i)
{
const std::wstring expectedString(4, static_cast<wchar_t>(L'A' + i));
const COORD expectedPos{ 0, gsl::narrow<SHORT>(i) };
TestUtils::VerifyExpectedString(tb, expectedString, expectedPos);
}

// For the last row, verify we have an entire row of asterisks for the mode line.
const std::wstring expectedModeLine(initialTermView.Width(), L'*');
const COORD expectedPos{ 0, gsl::narrow<SHORT>(rowsToWrite) };
TestUtils::VerifyExpectedString(tb, expectedModeLine, expectedPos);
};

// This will verify the text emitted from the PTY.
for (auto i = 0; i < rowsToWrite; ++i)
{
const std::string expectedString(4, static_cast<char>('A' + i));
expectedOutput.push_back(expectedString);
expectedOutput.push_back("\r\n");
}
{
const std::string expectedString(initialTermView.Width(), '*');
expectedOutput.push_back(expectedString);

// Cursor gets reset into bottom right corner as we're writing all the way into that corner.
std::stringstream ss;
ss << "\x1b[" << initialTermView.Height() << ";" << initialTermView.Width() << "H";
expectedOutput.push_back(ss.str());
}

Log::Comment(L"Verify host buffer contains pattern.");
// Verify the host side.
verifyBuffer(hostTb);

Log::Comment(L"Emit PTY frame and validate it transmits the right data.");
// Paint the frame
VERIFY_SUCCEEDED(renderer.PaintFrame());

Log::Comment(L"Verify terminal buffer contains pattern.");
// Verify the terminal side.
verifyBuffer(termTb);

Log::Comment(L"!!! OK. Set up the scroll region and let's get scrolling!");
// This is a simulation of what TMUX does to scroll everything except the mode line.
// First build up our VT strings...
std::wstring reducedScrollRegion;
{
std::wstringstream wss;
// For 20 tall buffer...
// ESC[1;19r
// Set scroll region to lines 1-19.
wss << L"\x1b[1;" << initialTermView.Height() - 1 << L"r";
reducedScrollRegion = wss.str();
}
std::wstring completeScrollRegion;
{
std::wstringstream wss;
// For 20 tall buffer...
// ESC[1;20r
// Set scroll region to lines 1-20. (or the whole buffer)
wss << L"\x1b[1;" << initialTermView.Height() << L"r";
completeScrollRegion = wss.str();
}
std::wstring reducedCursorBottomRight;
{
std::wstringstream wss;
// For 20 tall and 100 wide buffer
// ESC[19;100H
// Put cursor on line 19 (1 before last) and the right most column 100.
// (Remember that in VT, we start counting from 1 not 0.)
wss << L"\x1b[" << initialTermView.Height() - 1 << L";" << initialTermView.Width() << "H";
reducedCursorBottomRight = wss.str();
}
std::wstring completeCursorAtPromptLine;
{
std::wstringstream wss;
// For 20 tall and 100 wide buffer
// ESC[19;1H
// Put cursor on line 19 (1 before last) and the left most column 1.
// (Remember that in VT, we start counting from 1 not 0.)
wss << L"\x1b[" << initialTermView.Height() - 1 << L";1H";
completeCursorAtPromptLine = wss.str();
}

Log::Comment(L"Perform all the operations on the buffer.");

// OK this is what TMUX does.
// 1. Mark off the scroll area as everything but the mode line.
hostSm.ProcessString(reducedScrollRegion);
// 2. Put the cursor in the bottom-right corner of the scroll region.
hostSm.ProcessString(reducedCursorBottomRight);
// 3. Send a single newline which should do the heavy lifting
// of pushing everything in the scroll region up by 1 line and
// leave everything outside the region alone.

// This entire block is subject to change in the future with optimizations.
{
// Cursor gets redrawn in the bottom right of the scroll region with the repaint that is forced
// early while the screen is rotated.
std::stringstream ss;
ss << "\x1b[" << initialTermView.Height() - 1 << ";" << initialTermView.Width() << "H";
expectedOutput.push_back(ss.str());

expectedOutput.push_back("\x1b[?25h"); // turn the cursor back on too.
}

hostSm.ProcessString(L"\n");
// 4. Remove the scroll area by setting it to the entire size of the viewport.
hostSm.ProcessString(completeScrollRegion);
// 5. Put the cursor back at the beginning of the new line that was just revealed.
hostSm.ProcessString(completeCursorAtPromptLine);

// Set up the verifications like above.
auto verifyBufferAfter = [&](const TextBuffer& tb) {
auto& cursor = tb.GetCursor();
// Verify the cursor is waiting on the freshly revealed line (1 above mode line)
// and in the left most column.
VERIFY_ARE_EQUAL(initialTermView.Height() - 2, cursor.GetPosition().Y);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().X);

// For all rows except the last two, verify that we have a run of four letters.
for (auto i = 0; i < rowsToWrite - 1; ++i)
{
// Start with B this time because the A line got scrolled off the top.
const std::wstring expectedString(4, static_cast<wchar_t>(L'B' + i));
const COORD expectedPos{ 0, gsl::narrow<SHORT>(i) };
TestUtils::VerifyExpectedString(tb, expectedString, expectedPos);
}

// For the second to last row, verify that it is blank.
{
const std::wstring expectedBlankLine(initialTermView.Width(), L' ');
const COORD blankLinePos{ 0, gsl::narrow<SHORT>(rowsToWrite - 1) };
TestUtils::VerifyExpectedString(tb, expectedBlankLine, blankLinePos);
}

// For the last row, verify we have an entire row of asterisks for the mode line.
{
const std::wstring expectedModeLine(initialTermView.Width(), L'*');
const COORD modeLinePos{ 0, gsl::narrow<SHORT>(rowsToWrite) };
TestUtils::VerifyExpectedString(tb, expectedModeLine, modeLinePos);
}
};

// This will verify the text emitted from the PTY.

expectedOutput.push_back("\x1b[H"); // cursor returns to top left corner.
for (auto i = 0; i < rowsToWrite - 1; ++i)
{
const std::string expectedString(4, static_cast<char>('B' + i));
expectedOutput.push_back(expectedString);
expectedOutput.push_back("\x1b[K"); // erase the rest of the line.
expectedOutput.push_back("\r\n");
}
{
expectedOutput.push_back(""); // nothing for the empty line
expectedOutput.push_back("\x1b[K"); // erase the rest of the line.
expectedOutput.push_back("\r\n");
}
{
const std::string expectedString(initialTermView.Width(), '*');
expectedOutput.push_back(expectedString);
}
{
// Cursor gets reset into second line from bottom, left most column
std::stringstream ss;
ss << "\x1b[" << initialTermView.Height() - 1 << ";1H";
expectedOutput.push_back(ss.str());
}
expectedOutput.push_back("\x1b[?25h"); // turn the cursor back on too.

Log::Comment(L"Verify host buffer contains pattern moved up one and mode line still in place.");
// Verify the host side.
verifyBufferAfter(hostTb);

Log::Comment(L"Emit PTY frame and validate it transmits the right data.");
// Paint the frame
VERIFY_SUCCEEDED(renderer.PaintFrame());

Log::Comment(L"Verify terminal buffer contains pattern moved up one and mode line still in place.");
// Verify the terminal side.
verifyBufferAfter(termTb);
}
28 changes: 27 additions & 1 deletion src/host/_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,33 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100;
// displays the correct text.
if (newViewOrigin == viewport.Origin())
{
Viewport invalid = Viewport::FromDimensions(viewport.Origin(), { viewport.Width(), delta });
// Inside this block, we're shifting down at the bottom.
// This means that we had something like this:
// AAAA
// BBBB
// CCCC
// DDDD
// EEEE
//
// Our margins were set for lines A-D, but not on line E.
// So we circled the whole buffer up by one:
// BBBB
// CCCC
// DDDD
// EEEE
// <blank, was AAAA>
//
// Then we scrolled the contents of everything OUTSIDE the margin frame down.
// BBBB
// CCCC
// DDDD
// <blank, filled during scroll down of EEEE>
// EEEE
//
// And now we need to report that only the bottom line didn't "move" as we put the EEEE
// back where it started, but everything else moved.
// In this case, delta was 1. So the amount that moved is the entire viewport height minus the delta.
Viewport invalid = Viewport::FromDimensions(viewport.Origin(), { viewport.Width(), viewport.Height() - delta });
screenInfo.GetRenderTarget().TriggerRedraw(invalid);
}

Expand Down
3 changes: 3 additions & 0 deletions src/host/ut_host/ConptyOutputTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ using namespace Microsoft::Console::Types;

class ConptyOutputTests
{
// This test class is to write some things into the PTY and then check that
// the rendering that is coming out of the VT-sequence generator is exactly
// as we expect it to be.
BEGIN_TEST_CLASS(ConptyOutputTests)
TEST_CLASS_PROPERTY(L"IsolationLevel", L"Class")
END_TEST_CLASS()
Expand Down
24 changes: 24 additions & 0 deletions src/inc/til/point.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
return point{ x, y };
}

point& operator+=(const point& other)
{
*this = *this + other;
return *this;
}

point operator-(const point& other) const
{
ptrdiff_t x;
Expand All @@ -118,6 +124,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
return point{ x, y };
}

point& operator-=(const point& other)
{
*this = *this - other;
return *this;
}

point operator*(const point& other) const
{
ptrdiff_t x;
Expand All @@ -129,6 +141,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
return point{ x, y };
}

point& operator*=(const point& other)
{
*this = *this * other;
return *this;
}

point operator/(const point& other) const
{
ptrdiff_t x;
Expand All @@ -140,6 +158,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
return point{ x, y };
}

point& operator/=(const point& other)
{
*this = *this / other;
return *this;
}

constexpr ptrdiff_t x() const noexcept
{
return _x;
Expand Down
Loading

0 comments on commit ef80f66

Please sign in to comment.