Prevent the virtual viewport bottom being updated incorrectly #12972
Conversation
ghost
added
Area-VT
j4james
force-pushed
the
fix-update-bottom-2
branch
from
c62ea78
to
b0d609b
Compare
|
I know these fixes are a bit hacky, and it feels like we're just papering over the cracks, but this is the best I could come up for now. I'm a little concerned that the changes in PR #12703 might expose these virtual buffer problems more frequently (since we're no longer calling |
| // Make sure the new virtual bottom is far enough down to include both | ||
| // the cursor row and the last non-space row. | ||
| const auto cursorRow = newTextBuffer->GetCursor().GetPosition().Y; | ||
| const auto lastNonSpaceRow = newTextBuffer->GetLastNonSpaceCharacter().Y; |
There was a problem hiding this comment.
GetLastNonSpaceCharacter is very very slow if the text buffer is mostly empty.
Will this be a problem? Is there some other way we can extract this information?
There was a problem hiding this comment.
Yeah. I would have thought the TextBuffer::Reflow should have been able to provide that information for us, since it's just created that new buffer. But then that made me ask why the TextBuffer isn't tracking the virtual bottom in general? And why we've rewritten essentially the same resizing code in the Terminal class but in a completely different way? (Although it's worth noting that the Terminal implementation also uses GetLastNonSpaceCharacter.) And my conclusion was that all of this buffer management code really needs a unified rewrite, but that was way more effort than I wanted to get into now, if ever. Hence the quick fix hack.
There was a problem hiding this comment.
That's mostly on me. SCREEN_INFO is pretty tightly bound to the rest of the console itself. It was a lot harder to separate out from... everything else that makes the console the console. I was trying to create a version with a cleaner interface in Terminal.
In retrospect, yes, a unified implementation would have been smart. I think I would have liked to create a cleaner boundary around SCREEN_INFO so that terminal could have reused it too.
There was a problem hiding this comment.
Yeah, I realise it won't be easy decoupling that code from the console, but I think it'll be worth revisiting at some point, possibly when/if we get around to implementing VT paging. Because ideally I'd like for us to have a unified buffer management class that can handle both the console buffers and the VT pages in the same way (along with XTerm's alt/main weirdness). And that class could hopefully then be shared between conhost and Terminal and reduce some of the duplication. But it's not urgent.
|
@msftbot make sure @zadjii-msft signs off |
ghost
added
the
AutoMerge
|
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
DHowett
removed
the
AutoMerge
DHowett
added
the
AutoMerge
|
Hello @DHowett! 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 (
|
ghost
merged commit ## Summary of the Pull Request When calculating the position of the virtual bottom after a resize with reflow, it was possible for it to end up less than the height of the viewport. This meant that the top of the virtual viewport would be negative, which resulted in other operations failing further down the line. This PR updates the virtual bottom calculation to fix that scenario. ## References This was probably a regression introduced in PR #12972. ## PR Checklist * [x] Closes #13034 * [x] CLA signed. * [x] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [x] I've discussed this with core contributors already. Issue number where discussion took place: #13034 ## Validation Steps Performed I wasn't able to replicate the exact case described in issue #13034, because I don't have Windows 11, so can't configure the default terminal. However, I was able to reproduce a similar failure using a `SetConsoleScreenBufferInfoEx` call, and I've confirmed that this PR has fixed that. I've also added another screen buffer test to make sure the `ResizeWithReflow` method doesn't shrink the virtual bottom when resizing at the top of the buffer.
When the buffer is resized with a reflow, we were previously calculating the new virtual bottom based on the position of last non-space character. If the viewport was largely blank when resized, this could result in the new virtual bottom being higher than it should be. This PR attempts to address that problem by restoring the virtual bottom to a position that is the same distance from the cursor row as it was prior to the resize. This was a regression introduced in PR #12972. We still take the last non-space row into account when determining the virtual bottom, because if the content of the screen is forced to wrap, the virtual bottom will need to be lower (relative to the cursor) than it was before. We also need to check that we don't overflow the bottom of the buffer, which can occur when the viewport is at the bottom of the buffer, and the cursor position is pushed down as a result of content wrapping above it. I've manually confirmed that this fixes the problem reported in issue #13078, and I've also extended the existing `RefreshWithReflow` unit test to cover that particular scenario. Closes #13078
When the buffer is resized with a reflow, we were previously calculating the new virtual bottom based on the position of last non-space character. If the viewport was largely blank when resized, this could result in the new virtual bottom being higher than it should be. This PR attempts to address that problem by restoring the virtual bottom to a position that is the same distance from the cursor row as it was prior to the resize. This was a regression introduced in PR #12972. We still take the last non-space row into account when determining the virtual bottom, because if the content of the screen is forced to wrap, the virtual bottom will need to be lower (relative to the cursor) than it was before. We also need to check that we don't overflow the bottom of the buffer, which can occur when the viewport is at the bottom of the buffer, and the cursor position is pushed down as a result of content wrapping above it. I've manually confirmed that this fixes the problem reported in issue #13078, and I've also extended the existing `RefreshWithReflow` unit test to cover that particular scenario. Closes #13078 (cherry picked from commit c2f8308) Service-Card-Id: 81864034 Service-Version: 1.14
|
🎉 Handy links: |
ghost
mentioned this 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, or moved down further than necessary. This PR attempts
to fix that.
There was an earlier, unsuccessful attempt to fix this in PR #9770 which
was later reverted (issue #9872 was the reason it had to be reverted).
PRs #2666, #2705, and #5317 were fixes for related virtual viewport
problems, some of which have either been extended or superseded by this
PR.
SetConsoleCursorPositionImplis one of the cases that actually doesneed to move the virtual viewport upwards sometimes, in particular when
the cmd shell resets the buffer with a
CLScommand. But when thisoperation "snaps" the viewport to the location of the cursor, it needs
to use the virtual viewport as the frame of reference. This was
partially addressed by PR #2705, but that only applied in
terminal-scrolling mode, so I've now applied that fix regardless of the
mode.
SetViewportOrigintakes a flag which determines whether it will alsomove the virtual bottom to match the visible viewport. In some case this
is appropriate (
SetConsoleCursorPositionImplbeing one example), butin other cases (e.g. when panning the viewport downwards in the
AdjustCursorPositionfunction), it should only be allowed to movedownwards. We can't just not set the update flag in those cases, because
that also determines whether or not the viewport would be clamped, and
we don't want change that. So what I've done is limit
SetViewportOriginto only move the virtual bottom downwards, and addedan explicit
UpdateBottomcall in those places that may also requireupward movement.
ResizeWindowin theConhostInternalGetSetclass has a similarproblem to
SetConsoleCursorPositionImpl, in that it's updating theviewport to account for the new size, but if that visible viewport is
scrolled back or forward, it would end up placing the virtual viewport
in the wrong place. So again the solution here was to use the virtual
viewport as the frame of reference for the position. However, if the
viewport is being shrunk, this can still result in the cursor falling
below the bottom, so we need an additional check to adjust for that.
This can't be applied in pty mode, though, because that would break the
conpty resizing operation.
_InternalSetViewportSizecomes into play when resizing the windowmanually, and again the viewport after the resize can end up truncating
the virtual bottom if not handled correctly. This was partially
addressed in the original code by clamping the new viewport above the
virtual bottom under certain conditions, and only in terminal scrolling
mode. I've replaced that with a new algorithm which links the virtual
bottom to the visible viewport bottom if the two intersect, but
otherwise leaves it unchanged. This applies regardless of the scrolling
mode.
ResizeWithReflowis another sizing operation that can affect thevirtual bottom. This occurs when a change of the window width requires
the buffer to be reflowed, and we need to reposition the viewport in the
newly generated buffer. Previously we were just setting the virtual
bottom to align with the new visible viewport, but that could easily
result in the buffer truncation if the visible viewport was scrolled
back at the time. We now set the virtual bottom to the last non-space
row, or the cursor row (whichever is larger). There'll be edge cases
where this is probably not ideal, but it should still work reasonably
well.
MakeCursorVisiblewas another case where the virtual bottom was beingupdated (when requested with a flag) via a
SetViewportOrigincall.When I checked all the places it was used, though, none of them actually
required that behavior, and doing so could result in the virtual bottom
being incorrectly positioned, even after
SetViewportOriginwas limitedto moving the virtual bottom downwards. So I've now made it so that
MakeCursorVisiblenever updates the virtual bottom.SelectAllin theSelectionclass was a similar case. It was callingSetViewportOriginwith theupdateBottomflag set when that reallywasn't necessary and could result in the virtual bottom being
incorrectly set. I've changed the flag to false now.
Validation Steps Performed
I've manually confirmed that the test cases in issue #9754 are working
now, except for the one involving margins, which is bigger problem with
AdjustCursorPositionwhich will need to be addressed separately.I've also double checked the test cases from several other virtual
bottom issues (#1206, #1222, #5302, and #9872), and confirmed that
they're still working correctly with these changes.
And I've added a few screen buffer tests in which I've tried to cover as
many of the problematic code paths as possible.
Closes #9754