-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Implement preventing auto-scroll on new output #6062
Conversation
This is probably coming in way to late, but having just found this - can we call this "scrollOnOutput"? The only reason I found this PR is because it's linked through other issues that reference "scroll" in their name. "snap" feels very obscure compared to what this setting actually controls (the scroll position). |
Fair request. Copying this comment over to #2529 for discussion there. EDIT: wait, it's already there hahaha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test?
I think we don't have scrolling-related tests, but I'll double check. So nobody come along and automerge this plz! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
block for a test
I hate that you're in the right for doing this... hahaha |
Ok. I spent most of today trying to add a test to That should totally work. All I'm doing is writing lines of text to the buffer, and comparing the scroll offset to what it should be. It doesn't though because we need a callback to the scrollback in TermControl. Spoke with Dustin and this just doesn't feel right. Like, scrolling should be pretty self sustained. So, I created #6842 to track this better. Test Codevoid ScrollTest::TestConditionalAutoscrolling()
{
// When new output is generated, we should only scroll to it when...
// - no selection is active, and
// - the viewport is already at the bottom of the scroll history
Log::Comment(L"Watch out - this test takes a while to run, and won't "
L"output anything unless in encounters an error. This is expected.");
auto& termTb = *_term->_buffer;
const auto totalBufferSize = termTb.GetSize().Height();
// We're outputting like 18000 lines of text here, so emitting 18000*4 lines
// of output to the console is actually quite unnecessary
WEX::TestExecution::SetVerifyOutput settings(WEX::TestExecution::VerifyOutputSettings::LogOnlyFailures);
// Emit a bunch of newlines to test scrolling.
auto scrollAmount = 0;
for (auto currentRow = 0; currentRow < totalBufferSize * 2; currentRow++)
{
*_scrollBarNotification = std::nullopt;
_renderTarget->Reset();
_term->_WriteBuffer(L"X\r\n");
if (currentRow < TerminalViewHeight)
{
// no scrolling is expected
VERIFY_ARE_EQUAL(0, _term->_scrollOffset);
}
else if (currentRow < TerminalViewHeight * 2)
{
// keep track of scrolling
++scrollAmount;
VERIFY_ARE_EQUAL(scrollAmount, _term->_scrollOffset);
}
else if (currentRow == TerminalViewHeight * 2)
{
// create a selection
Log::Comment(L"Create a selection to pause scrolling");
_term->SelectNewRegion({ 0, static_cast<SHORT>(currentRow) }, { 5, static_cast<SHORT>(currentRow) });
VERIFY_ARE_EQUAL(scrollAmount, _term->_scrollOffset);
}
else if (currentRow < totalBufferSize)
{
// scrolling is unchanged because of selection
VERIFY_ARE_EQUAL(scrollAmount, _term->_scrollOffset);
}
else if (scrollAmount > 0)
{
// circling the buffer
// we should be scrolling up to keep the same content in the viewport
--scrollAmount;
VERIFY_ARE_EQUAL(scrollAmount, _term->_scrollOffset);
}
else
{
// still circling the buffer
// we can't scroll up anymore
VERIFY_ARE_EQUAL(0, _term->_scrollOffset);
}
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good enough for me
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
🎉 Handy links: |
) 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
) 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)
I'm using 1.17.1023 and when I'm compiling and scroll back to see an error after a few moments it jumps back down to the bottom of buffer. This happens whether I select some text or not. |
@brandon-kohn Interesting - mind filing a new issue? This should have shipped nearly 3 years ago now, so whatever you're seeing is probably unrelated to this original PR. I want to make sure we don't lose track of it. I'd also check the Thanks! |
Summary of the Pull Request
Updates the Terminal's scroll response to new output. The Terminal will not automatically scroll if...
References
#2529 - Spec
#3863 - Implementation
PR Checklist
Detailed Description of the Pull Request / Additional comments
Updates the
_scrollOffset
value properly in TerminalCore when the cursor moves. We calculate a new_scrollOffset
based on if we are circling the buffer and how far below the mutable bottom is.We specifically check for if a selection is active and if the viewport is at the bottom, then use that as a condition for deciding if we should update
_scrollOffset
to the new calculated value or 0 (the bottom of the scroll history).Validation Steps Performed
Manual testing. Though I should add automated tests.