Skip to content

Commit

Permalink
Retain horizontal viewport offset when moving to bottom (#8434)
Browse files Browse the repository at this point in the history
When the viewport is moved to the "virtual bottom" of the buffer (via
the `MoveToBottom` method), it is important that the horizontal viewport
offset be left as it is, otherwise that can result in some undesirable
side effects.

Since the VT coordinate system is relative to the top of the viewport,
many VT operations call the `MoveToBottom` method to make sure the
viewport is correctly positioned before proceeding. There is no need for
the horizontal position to be adjusted, though, since the X coordinates
are not constrained by the viewport, but are instead relative to the
underlying buffer.

Setting the viewport X coordinate to 0 in `MoveToBottom` (as we were
previously doing) could result in the cursor being pushed off screen.
And if the operation itself was moving the cursor, that would then
trigger another viewport move to bring the cursor back into view. These
conflicting movements meant the viewport was always forced as far left
as possible, and could also result in cursor "droppings" as the cursor
lost track of where it had been.

I've now fixed this by updating the `GetVirtualViewport` method to match
the horizontal offset of the active viewport, instead of having the X
coordinate hardcoded to 0. 

## Validation Steps Performed

I've manually confirmed that this fixes the cursor "droppings" test case
reported in issue #8213.

I've also added a screen buffer test that makes sure the `MoveToBottom`
method is working as expected, and not changing the horizontal viewport
offset when it moves down.

Closes #8213
  • Loading branch information
j4james authored Nov 30, 2020
1 parent 91aafe8 commit 90316be
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2573,7 +2573,7 @@ void SCREEN_INFORMATION::MoveToBottom()
Viewport SCREEN_INFORMATION::GetVirtualViewport() const noexcept
{
const short newTop = _virtualBottom - _viewport.Height() + 1;
return Viewport::FromDimensions({ 0, newTop }, _viewport.Dimensions());
return Viewport::FromDimensions({ _viewport.Left(), newTop }, _viewport.Dimensions());
}

// Method Description:
Expand Down
39 changes: 39 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ class ScreenBufferTests
TEST_METHOD(TestAddHyperlinkCustomIdDifferentUri);

TEST_METHOD(UpdateVirtualBottomWhenCursorMovesBelowIt);
TEST_METHOD(RetainHorizontalOffsetWhenMovingToBottom);

TEST_METHOD(TestWriteConsoleVTQuirkMode);
};
Expand Down Expand Up @@ -6020,6 +6021,44 @@ void ScreenBufferTests::UpdateVirtualBottomWhenCursorMovesBelowIt()
VERIFY_ARE_EQUAL(newVirtualBottom, si.GetViewport().BottomInclusive());
}

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

Log::Comment(L"Make the viewport half the default width");
auto initialSize = COORD{ CommonState::s_csWindowWidth / 2, CommonState::s_csWindowHeight };
si.SetViewportSize(&initialSize);

Log::Comment(L"Offset the viewport both vertically and horizontally");
auto initialOrigin = COORD{ 10, 20 };
VERIFY_SUCCEEDED(si.SetViewportOrigin(true, initialOrigin, true));

Log::Comment(L"Verify that the virtual viewport is where it's expected to be");
VERIFY_ARE_EQUAL(initialSize, si.GetVirtualViewport().Dimensions());
VERIFY_ARE_EQUAL(initialOrigin, si.GetVirtualViewport().Origin());

Log::Comment(L"Set the cursor position at the viewport origin");
cursor.SetPosition(initialOrigin);
VERIFY_ARE_EQUAL(initialOrigin, cursor.GetPosition());

Log::Comment(L"Pan the viewport up by 10 lines");
VERIFY_SUCCEEDED(si.SetViewportOrigin(false, { 0, -10 }, false));

Log::Comment(L"Verify Y offset has moved up and X is unchanged");
VERIFY_ARE_EQUAL(initialOrigin.Y - 10, si.GetViewport().Top());
VERIFY_ARE_EQUAL(initialOrigin.X, si.GetViewport().Left());

Log::Comment(L"Move the viewport back to the virtual bottom");
si.MoveToBottom();

Log::Comment(L"Verify Y offset has moved back and X is unchanged");
VERIFY_ARE_EQUAL(initialOrigin.Y, si.GetViewport().Top());
VERIFY_ARE_EQUAL(initialOrigin.X, si.GetViewport().Left());
}

void ScreenBufferTests::TestWriteConsoleVTQuirkMode()
{
BEGIN_TEST_METHOD_PROPERTIES()
Expand Down

0 comments on commit 90316be

Please sign in to comment.