Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent the virtual viewport bottom being moved up unintentionally #9770

Merged
4 commits merged into from
Apr 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/host/outputStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,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
Expand Down
1 change: 1 addition & 0 deletions src/host/outputStream.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal::
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;
Expand Down
24 changes: 21 additions & 3 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,15 @@ 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,
defaultAttributes,
uiCursorSize,
pScreen->_renderTarget);

pScreen->UpdateBottom();
Comment on lines -116 to +123
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This had to move down, because UpdateBottom now depends on _textBuffer to verify that the bottom hasn't become out of range.


const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
pScreen->_textBuffer->GetCursor().SetColor(gci.GetCursorColor());
pScreen->_textBuffer->GetCursor().SetType(gci.GetCursorType());
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions src/host/screenInfo.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console
void SetTerminalConnection(_In_ Microsoft::Console::ITerminalOutputConnection* const pTtyConnection);

void UpdateBottom();
void ResetBottom();
void MoveToBottom();

Microsoft::Console::Render::IRenderTarget& GetRenderTarget() noexcept;
Expand Down
53 changes: 53 additions & 0 deletions src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down Expand Up @@ -215,6 +217,7 @@ class ScreenBufferTests
TEST_METHOD(TestAddHyperlinkCustomIdDifferentUri);

TEST_METHOD(UpdateVirtualBottomWhenCursorMovesBelowIt);
TEST_METHOD(DontUpdateVirtualBottomWhenMovedUp);
TEST_METHOD(RetainHorizontalOffsetWhenMovingToBottom);

TEST_METHOD(TestWriteConsoleVTQuirkMode);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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();
Expand Down
3 changes: 3 additions & 0 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/terminal/adapter/conGetSet.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,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;
Expand Down
5 changes: 5 additions & 0 deletions src/terminal/adapter/ut_adapter/adapterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,11 @@ class TestGetSet final : public ConGetSet
return TRUE;
}

void ResetBottom() override
{
Log::Comment(L"ResetBottom MOCK called...");
}

bool MoveToBottom() const override
{
Log::Comment(L"MoveToBottom MOCK called...");
Expand Down