Skip to content

Commit

Permalink
Update the virtual bottom location if the cursor moves below it (#5317)
Browse files Browse the repository at this point in the history
If an application writes to the screen while not in VT mode, and the
user has scrolled forward in the screen buffer, the _virtual bottom_
location is not updated to take that new content into account. As a
result, the viewport can later jump back to the previous _virtual
bottom_, making the content disappear off screen. This PR attempts to
fix that issue by updating the _virtual bottom_ location whenever the
cursor moves below that point.

## PR Checklist
* [x] CLA signed. 
* [x] Tests added/passed

## Detailed Description of the Pull Request / Additional comments

This simply adds a condition in the
`SCREEN_INFORMATION::SetCursorPosition` to check if the new _Y_
coordinate is below the current _virtual bottom_, and if so, updates the
_virtual bottom_ to that new value.

I considered trying to make it only update when something is actually
written to the screen, but this seemed like a cleaner solution, and is
less likely to miss out on a needed update.

## Validation Steps Performed

I've manually tested the case described in issue #5302, and confirmed
that it now works as expected. I've also added a unit test that checks
the virtual bottom is updated correctly under similar conditions.

Closes #5302
  • Loading branch information
j4james committed Apr 14, 2020
1 parent 9dd5f6e commit 348f468
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1665,6 +1665,12 @@ void SCREEN_INFORMATION::SetCursorDBMode(const bool DoubleCursor)

cursor.SetPosition(Position);

// If the cursor has moved below the virtual bottom, the bottom should be updated.
if (Position.Y > _virtualBottom)
{
_virtualBottom = Position.Y;
}

// if we have the focus, adjust the cursor state
if (gci.Flags & CONSOLE_HAS_FOCUS)
{
Expand Down
49 changes: 49 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ class ScreenBufferTests
TEST_METHOD(ScreenAlignmentPattern);

TEST_METHOD(TestCursorIsOn);

TEST_METHOD(UpdateVirtualBottomWhenCursorMovesBelowIt);
};

void ScreenBufferTests::SingleAlternateBufferCreationTest()
Expand Down Expand Up @@ -5870,3 +5872,50 @@ void ScreenBufferTests::TestCursorIsOn()
VERIFY_IS_FALSE(cursor.IsBlinkingAllowed());
VERIFY_IS_FALSE(cursor.IsVisible());
}

void ScreenBufferTests::UpdateVirtualBottomWhenCursorMovesBelowIt()
{
auto& g = ServiceLocator::LocateGlobals();
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& cursor = si.GetTextBuffer().GetCursor();

Log::Comment(L"Make sure the initial virtual bottom is at the bottom of the viewport");
si.UpdateBottom();
const auto initialVirtualBottom = si.GetViewport().BottomInclusive();
VERIFY_ARE_EQUAL(initialVirtualBottom, si._virtualBottom);

Log::Comment(L"Set the initial cursor position on that virtual bottom line");
const auto initialCursorPos = COORD{ 0, initialVirtualBottom };
cursor.SetPosition(initialCursorPos);
VERIFY_ARE_EQUAL(initialCursorPos, cursor.GetPosition());

Log::Comment(L"Pan down so the initial viewport has the cursor in the middle");
const auto initialOrigin = COORD{ 0, si.GetViewport().Top() + si.GetViewport().Height() / 2 };
gci.SetTerminalScrolling(false);
VERIFY_SUCCEEDED(si.SetViewportOrigin(false, initialOrigin, false));
VERIFY_ARE_EQUAL(initialOrigin, si.GetViewport().Origin());

Log::Comment(L"Confirm that the virtual bottom has not changed");
VERIFY_ARE_EQUAL(initialVirtualBottom, si._virtualBottom);

Log::Comment(L"Now write several lines of content using WriteCharsLegacy");
const auto content = L"1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n";
auto numBytes = wcslen(content) * sizeof(wchar_t);
VERIFY_SUCCESS_NTSTATUS(WriteCharsLegacy(si, content, content, content, &numBytes, nullptr, 0, 0, nullptr));

Log::Comment(L"Confirm that the cursor position has moved down 10 lines");
const auto newCursorPos = COORD{ initialCursorPos.X, initialCursorPos.Y + 10 };
VERIFY_ARE_EQUAL(newCursorPos, cursor.GetPosition());

Log::Comment(L"Confirm that the virtual bottom matches that new cursor position");
const auto newVirtualBottom = newCursorPos.Y;
VERIFY_ARE_EQUAL(newVirtualBottom, si._virtualBottom);

Log::Comment(L"The viewport itself should not have changed at this point");
VERIFY_ARE_EQUAL(initialOrigin, si.GetViewport().Origin());

Log::Comment(L"But after MoveToBottom, the viewport should align with the new virtual bottom");
si.MoveToBottom();
VERIFY_ARE_EQUAL(newVirtualBottom, si.GetViewport().BottomInclusive());
}

0 comments on commit 348f468

Please sign in to comment.