From bc642bbf2a51e792b1eddb219d8ae3117bed9d3c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 11 Aug 2020 14:57:45 -0500 Subject: [PATCH] Fix viewport moving when we've scrolled up and circled the buffer (#7247) If you scroll up to view the scrollback, then we want the viewport to "stay in place", as new output comes in (see #6062). This works fine up until the buffer circles. In this case, the mutable viewport isn't actually moving, so we never set `updatedViewport` to true. This regressed in #6062 Closes #7222 --- src/cascadia/TerminalCore/Terminal.cpp | 18 +++- .../TerminalBufferTests.cpp | 84 +++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 4709eeaf0e8..f6b3950a262 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -843,8 +843,12 @@ void Terminal::_AdjustCursorPosition(const COORD proposedPosition) } } - if (updatedViewport) + // If the viewport moved, or we circled the buffer, we might need to update + // our _scrollOffset + if (updatedViewport || newRows != 0) { + const auto oldScrollOffset = _scrollOffset; + // scroll if... // - no selection is active // - viewport is already at the bottom @@ -852,6 +856,18 @@ void Terminal::_AdjustCursorPosition(const COORD proposedPosition) _scrollOffset = scrollToOutput ? 0 : _scrollOffset + scrollAmount + newRows; + // Clamp the range to make sure that we don't scroll way off the top of the buffer + _scrollOffset = std::clamp(_scrollOffset, + 0, + _buffer->GetSize().Height() - _mutableViewport.Height()); + + // If the new scroll offset is different, then we'll still want to raise a scroll event + updatedViewport = updatedViewport || (oldScrollOffset != _scrollOffset); + } + + // If the viewport moved, then send a scrolling notification. + if (updatedViewport) + { _NotifyScrollEvent(); } diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp index 5fbc2d7403d..e4636081803 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp @@ -39,6 +39,8 @@ class TerminalCoreUnitTests::TerminalBufferTests final TEST_METHOD(TestWrappingCharByChar); TEST_METHOD(TestWrappingALongString); + TEST_METHOD(DontSnapToOutputTest); + TEST_METHOD_SETUP(MethodSetup) { // STEP 1: Set up the Terminal @@ -149,3 +151,85 @@ void TerminalBufferTests::TestWrappingALongString() TestUtils::VerifyExpectedString(termTb, TestUtils::Test100CharsString, { 0, 0 }); } + +void TerminalBufferTests::DontSnapToOutputTest() +{ + auto& termTb = *term->_buffer; + auto& termSm = *term->_stateMachine; + const auto initialView = term->GetViewport(); + + VERIFY_ARE_EQUAL(0, initialView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight, initialView.BottomExclusive()); + VERIFY_ARE_EQUAL(0, term->_scrollOffset); + + // -1 so that we don't print the last \n + for (int i = 0; i < TerminalViewHeight + 8 - 1; i++) + { + termSm.ProcessString(L"x\n"); + } + + const auto secondView = term->GetViewport(); + + VERIFY_ARE_EQUAL(8, secondView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight + 8, secondView.BottomExclusive()); + VERIFY_ARE_EQUAL(0, term->_scrollOffset); + + Log::Comment(L"Scroll up one line"); + term->_scrollOffset = 1; + + const auto thirdView = term->GetViewport(); + VERIFY_ARE_EQUAL(7, thirdView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight + 7, thirdView.BottomExclusive()); + VERIFY_ARE_EQUAL(1, term->_scrollOffset); + + Log::Comment(L"Print a few lines, to see that the viewport stays where it was"); + for (int i = 0; i < 8; i++) + { + termSm.ProcessString(L"x\n"); + } + + const auto fourthView = term->GetViewport(); + VERIFY_ARE_EQUAL(7, fourthView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight + 7, fourthView.BottomExclusive()); + VERIFY_ARE_EQUAL(1 + 8, term->_scrollOffset); + + Log::Comment(L"Print enough lines to get the buffer just about ready to " + L"circle (on the next newline)"); + auto viewBottom = term->_mutableViewport.BottomInclusive(); + do + { + termSm.ProcessString(L"x\n"); + viewBottom = term->_mutableViewport.BottomInclusive(); + } while (viewBottom < termTb.GetSize().BottomInclusive()); + + const auto fifthView = term->GetViewport(); + VERIFY_ARE_EQUAL(7, fifthView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight + 7, fifthView.BottomExclusive()); + VERIFY_ARE_EQUAL(TerminalHistoryLength - 7, term->_scrollOffset); + + Log::Comment(L"Print 3 more lines, and see that we stick to where the old " + L"rows now are in the buffer (after circling)"); + for (int i = 0; i < 3; i++) + { + termSm.ProcessString(L"x\n"); + Log::Comment(NoThrowString().Format( + L"_scrollOffset: %d", term->_scrollOffset)); + } + const auto sixthView = term->GetViewport(); + VERIFY_ARE_EQUAL(4, sixthView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight + 4, sixthView.BottomExclusive()); + VERIFY_ARE_EQUAL(TerminalHistoryLength - 4, term->_scrollOffset); + + Log::Comment(L"Print 8 more lines, and see that we're now just stuck at the" + L"top of the buffer"); + for (int i = 0; i < 8; i++) + { + termSm.ProcessString(L"x\n"); + Log::Comment(NoThrowString().Format( + L"_scrollOffset: %d", term->_scrollOffset)); + } + const auto seventhView = term->GetViewport(); + VERIFY_ARE_EQUAL(0, seventhView.Top()); + VERIFY_ARE_EQUAL(TerminalViewHeight, seventhView.BottomExclusive()); + VERIFY_ARE_EQUAL(TerminalHistoryLength, term->_scrollOffset); +}