Prevent the virtual viewport bottom being moved up unintentionally (#9770)

## Summary of the Pull Request

The "virtual bottom" marks the last line of the mutable viewport area, which is the part of the buffer that VT sequences can write to. This region should typically only move downwards, as new lines are added to the buffer, but there were a number of cases where it was incorrectly being moved up. This PR attempts to fix that.

## PR Checklist
* [x] Closes #9754
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #9754

## Detailed Description of the Pull Request / Additional comments

When a call is made to `UpdateBottom`, we now clamp the value so it's at least as low as the current viewport bottom (i.e. if the viewport has moved down, we want the virtual bottom to move down too), but no lower than the bottom of the buffer (we don't want it to be out of range).

There is one special case where we do actually want the virtual bottom to move up - when the scrollback has been cleared with an `ED3` escape sequence. So in that case we needed a new `ConGetSet` API (`ResetBottom`) to reset the virtual bottom to the top of the buffer (essentially one less than the viewport height, since the virtual bottom points to the last line of the viewport).

## Validation Steps Performed

I had to reset the virtual bottom manually in some parts of the `ScreenBufferTests`, since some of the tests were relying on the virtual bottom being automatically reset when the viewport was reset, which is no longer the case.

I've also added a new test to verify that the virtual bottom doesn't move upwards if an update is triggered while the visible viewport is scrolled up. This essentially reproduces the test case from issue #9754, which I've also manually confirmed is fixed.
This commit is contained in:
James Holderness 2021-04-15 18:07:59 +01:00 committed by GitHub
parent dba66da18d
commit 74909c0c65
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 96 additions and 3 deletions

View File

@ -656,6 +656,17 @@ bool ConhostInternalGetSet::InsertLines(const size_t count)
return true;
}
// Method Description:
// - Resets the internal "virtual bottom" tracker to the top of the buffer.
// Arguments:
// - <none>
// Return Value:
// - <none>
void ConhostInternalGetSet::ResetBottom()
{
_io.GetActiveOutputBuffer().ResetBottom();
}
// Method Description:
// - Connects the MoveToBottom call directly into our Driver Message servicing
// call inside Conhost.exe

View File

@ -123,6 +123,7 @@ public:
bool DeleteLines(const size_t count) override;
bool InsertLines(const size_t count) override;
void ResetBottom() override;
bool MoveToBottom() const override;
bool PrivateGetColorTableEntry(const size_t index, COLORREF& value) const noexcept override;

View File

@ -113,7 +113,6 @@ SCREEN_INFORMATION::~SCREEN_INFORMATION()
// Set up viewport
pScreen->_viewport = Viewport::FromDimensions({ 0, 0 },
pScreen->_IsInPtyMode() ? coordScreenBufferSize : coordWindowSize);
pScreen->UpdateBottom();
// Set up text buffer
pScreen->_textBuffer = std::make_unique<TextBuffer>(coordScreenBufferSize,
@ -121,6 +120,8 @@ SCREEN_INFORMATION::~SCREEN_INFORMATION()
uiCursorSize,
pScreen->_renderTarget);
pScreen->UpdateBottom();
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
pScreen->_textBuffer->GetCursor().SetColor(gci.GetCursorColor());
pScreen->_textBuffer->GetCursor().SetType(gci.GetCursorType());
@ -2520,13 +2521,30 @@ TextBufferCellIterator SCREEN_INFORMATION::GetCellDataAt(const COORD at, const V
// Method Description:
// - Updates our internal "virtual bottom" tracker with wherever the viewport
// currently is.
// currently is. This is only used to move the bottom further down, unless
// the buffer has shrunk, and the viewport needs to be moved back up to
// fit within the new buffer range.
// - <none>
// Return Value:
// - <none>
void SCREEN_INFORMATION::UpdateBottom()
{
_virtualBottom = _viewport.BottomInclusive();
// We clamp it so it's at least as low as the current viewport bottom,
// but no lower than the bottom of the buffer.
_virtualBottom = std::clamp(_virtualBottom, _viewport.BottomInclusive(), GetBufferSize().BottomInclusive());
}
// Method Description:
// - Resets the internal "virtual bottom" tracker to the top of the buffer.
// Used when the scrollback buffer has been completely cleared.
// - <none>
// Return Value:
// - <none>
void SCREEN_INFORMATION::ResetBottom()
{
// The virtual bottom points to the last line of the viewport, so at the
// top of the buffer it should be one less than the viewport height.
_virtualBottom = _viewport.Height() - 1;
}
// Method Description:

View File

@ -225,6 +225,7 @@ public:
void SetTerminalConnection(_In_ Microsoft::Console::ITerminalOutputConnection* const pTtyConnection);
void UpdateBottom();
void ResetBottom();
void MoveToBottom();
Microsoft::Console::Render::IRenderTarget& GetRenderTarget() noexcept;

View File

@ -67,6 +67,8 @@ class ScreenBufferTests
auto& currentBuffer = gci.GetActiveOutputBuffer();
// Make sure a test hasn't left us in the alt buffer on accident
VERIFY_IS_FALSE(currentBuffer._IsAltBuffer());
// Make sure the virtual viewport bottom is reset.
currentBuffer._virtualBottom = 0;
VERIFY_SUCCEEDED(currentBuffer.SetViewportOrigin(true, { 0, 0 }, true));
// Make sure the viewport always starts off at the default size.
auto defaultSize = COORD{ CommonState::s_csWindowWidth, CommonState::s_csWindowHeight };
@ -215,6 +217,7 @@ class ScreenBufferTests
TEST_METHOD(TestAddHyperlinkCustomIdDifferentUri);
TEST_METHOD(UpdateVirtualBottomWhenCursorMovesBelowIt);
TEST_METHOD(DontUpdateVirtualBottomWhenMovedUp);
TEST_METHOD(RetainHorizontalOffsetWhenMovingToBottom);
TEST_METHOD(TestWriteConsoleVTQuirkMode);
@ -1542,6 +1545,7 @@ void ScreenBufferTests::VtNewlineOutsideMargins()
VERIFY_ARE_EQUAL(COORD({ 0, viewportTop + 1 }), si.GetViewport().Origin());
Log::Comment(L"Reset viewport and apply DECSTBM margins");
si._virtualBottom = 0;
VERIFY_SUCCEEDED(si.SetViewportOrigin(true, COORD({ 0, viewportTop }), true));
stateMachine.ProcessString(L"\x1b[1;5r");
// Make sure we clear the margins on exit so they can't break other tests.
@ -3341,6 +3345,7 @@ void ScreenBufferTests::ScrollOperations()
const auto bufferHeight = si.GetBufferSize().Height();
// Move the viewport down a few lines, and only cover part of the buffer width.
si._virtualBottom = 0;
si.SetViewport(Viewport::FromDimensions({ 5, 10 }, { bufferWidth - 10, 10 }), true);
const auto viewportStart = si.GetViewport().Top();
const auto viewportEnd = si.GetViewport().BottomExclusive();
@ -3865,6 +3870,7 @@ void ScreenBufferTests::EraseTests()
const auto bufferHeight = si.GetBufferSize().Height();
// Move the viewport down a few lines, and only cover part of the buffer width.
si._virtualBottom = 0;
si.SetViewport(Viewport::FromDimensions({ 5, 10 }, { bufferWidth - 10, 10 }), true);
// Fill the entire buffer with Zs. Blue on Green.
@ -3985,6 +3991,7 @@ void _CommonScrollingSetup()
const auto oldView = si.GetViewport();
const auto view = Viewport::FromDimensions({ 0, 0 }, { oldView.Width(), 6 });
si.SetViewport(view, true);
si.ResetBottom();
cursor.SetPosition({ 0, 0 });
stateMachine.ProcessString(L"A");
cursor.SetPosition({ 0, 5 });
@ -6021,6 +6028,52 @@ void ScreenBufferTests::UpdateVirtualBottomWhenCursorMovesBelowIt()
VERIFY_ARE_EQUAL(newVirtualBottom, si.GetViewport().BottomInclusive());
}
void ScreenBufferTests::DontUpdateVirtualBottomWhenMovedUp()
{
auto& g = ServiceLocator::LocateGlobals();
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& cursor = si.GetTextBuffer().GetCursor();
Log::Comment(L"Set the initial viewport one page from the top");
const auto initialOrigin = COORD{ 0, si.GetViewport().Top() + si.GetViewport().Height() };
VERIFY_SUCCEEDED(si.SetViewportOrigin(false, initialOrigin, false));
VERIFY_ARE_EQUAL(initialOrigin, si.GetViewport().Origin());
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 5 lines above virtual bottom line");
const auto initialCursorPos = COORD{ 0, initialVirtualBottom - 5 };
cursor.SetPosition(initialCursorPos);
VERIFY_ARE_EQUAL(initialCursorPos, cursor.GetPosition());
Log::Comment(L"Pan to the top of the buffer so the the cursor is out of view");
const auto topOfBufferOrigin = COORD{ 0, 0 };
VERIFY_SUCCEEDED(si.SetViewportOrigin(true, topOfBufferOrigin, false));
VERIFY_ARE_EQUAL(topOfBufferOrigin, 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 out some content using WriteCharsLegacy");
const auto content = L"Hello World";
auto numBytes = wcslen(content) * sizeof(wchar_t);
VERIFY_SUCCESS_NTSTATUS(WriteCharsLegacy(si, content, content, content, &numBytes, nullptr, 0, 0, nullptr));
Log::Comment(L"The viewport bottom should now align with the cursor pos");
VERIFY_ARE_EQUAL(initialCursorPos.Y, si.GetViewport().BottomInclusive());
Log::Comment(L"But the virtual bottom should still not have changed");
VERIFY_ARE_EQUAL(initialVirtualBottom, si._virtualBottom);
Log::Comment(L"After MoveToBottom, the viewport should align with the virtual bottom");
si.MoveToBottom();
VERIFY_ARE_EQUAL(initialVirtualBottom, si.GetViewport().BottomInclusive());
}
void ScreenBufferTests::RetainHorizontalOffsetWhenMovingToBottom()
{
auto& g = ServiceLocator::LocateGlobals();

View File

@ -2030,6 +2030,9 @@ bool AdaptDispatch::_EraseScrollback()
if (success)
{
// Reset the virtual viewport bottom to the top of the buffer.
_pConApi->ResetBottom();
// Move the viewport (CAN'T be done in one call with SetConsolescreenBufferInfoEx, because legacy)
SMALL_RECT newViewport;
newViewport.Left = screen.Left;

View File

@ -88,6 +88,7 @@ namespace Microsoft::Console::VirtualTerminal
virtual bool DeleteLines(const size_t count) = 0;
virtual bool InsertLines(const size_t count) = 0;
virtual void ResetBottom() = 0;
virtual bool MoveToBottom() const = 0;
virtual bool PrivateGetColorTableEntry(const size_t index, COLORREF& value) const = 0;

View File

@ -492,6 +492,11 @@ public:
return TRUE;
}
void ResetBottom() override
{
Log::Comment(L"ResetBottom MOCK called...");
}
bool MoveToBottom() const override
{
Log::Comment(L"MoveToBottom MOCK called...");