Skip to content

Commit

Permalink
Fix viewport moving when we've scrolled up and circled the buffer (#7247
Browse files Browse the repository at this point in the history
)

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

(cherry picked from commit bc642bb)
  • Loading branch information
zadjii-msft authored and DHowett committed Aug 24, 2020
1 parent cb8d371 commit a42bf7b
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 1 deletion.
18 changes: 17 additions & 1 deletion src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -830,15 +830,31 @@ 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
const bool scrollToOutput = !IsSelectionActive() && _scrollOffset == 0;

_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();
}

Expand Down
84 changes: 84 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

0 comments on commit a42bf7b

Please sign in to comment.