Fix snapping to the cursor in "Terminal Scrolling" mode #2705
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fixes #1222 PSReadline calls SetConsoleCursorPosition on each character they emit (go figure). When that function is called, and we set the cursor position, we'll try and "snap" the viewport to the location of the cursor, so that the cursor remains visible. However, we'd only ever do this with the visible viewport, the viewport defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in Terminal Scrolling mode, we actually need to snap the virtual viewport, so that this behavior looks more regular.
zadjii-msft
added
Product-Conhost
carlos-zamora
approved these changes
Sep 9, 2019
carlos-zamora
requested changes
Sep 9, 2019
ghost
added
the
Needs-Author-Feedback
ghost
added
the
No-Recent-Activity
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
ghost
closed this
ghost
removed
the
No-Recent-Activity
DHowett-MSFT
removed
the
Needs-Author-Feedback
# Conflicts: # src/host/getset.cpp # src/host/ut_host/ScreenBufferTests.cpp
|
I cannot determine why @carlos-zamora blocked this. |
There was a failing test. If it's good to go now, I'll approve. |
carlos-zamora
approved these changes
Sep 26, 2019
DHowett-MSFT
pushed a commit
that referenced
this pull request
Oct 17, 2019
fixes #1222 PSReadline calls SetConsoleCursorPosition on each character they emit (go figure). When that function is called, and we set the cursor position, we'll try and "snap" the viewport to the location of the cursor, so that the cursor remains visible. However, we'd only ever do this with the visible viewport, the viewport defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in Terminal Scrolling mode, we actually need to snap the virtual viewport, so that this behavior looks more regular. (cherry picked from commit 6f4b98a)
DHowett-MSFT
pushed a commit
that referenced
this pull request
Oct 17, 2019
- Fix snapping to the cursor in "Terminal Scrolling" mode (GH-2705) fixes GH-1222 PSReadline calls SetConsoleCursorPosition on each character. We try to snap. However, we'd only ever do this with the visible viewport, the viewport defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in Terminal Scrolling mode, we actually need to snap the virtual viewport, so that this behavior looks more regular. (cherry picked from commit 6f4b98a) - Passthrough BEL in conpty (GH-2990) fixes GH-2952. (cherry picked from commit 6831120) - make filling chars (and, thus, erase line/char) unset wrap (GH-2831) EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to the end of the line. Originally, we had a boolean `setWrap` that would mean... - **true**: if writing to the end of the row, SET the wrap value to true - **false**: if writing to the end of the row, DON'T CHANGE the wrap value ,- current wrap value | ,- fill last cell? | | ,- new wrap value | | | ,- comments |-|-|-| |1|0|0| THIS CASE WAS UNHANDLED To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have ~setWrap~ `wrap` mean the following: - **true**: if writing to the end of the row, SET the wrap value to TRUE - **false**: if writing to the end of the row, SET the wrap value to FALSE - **nullopt**: leave the wrap value as it is Closes GH-1126 (cherry picked from commit 4dd9f9c) - When reverse indexing, preserve RGB/256 attributes (GH-2987) (cherry picked from commit 847d6b5) - do not allow CUU and CUD to move "across" margins. (GH-2996) If we're moving the cursor up, its vertical movement should be stopped at the top margin. Similarly, this applies to moving down and the bottom margin. Fixes GH-2929. (cherry picked from commit 0ce08af) - Prevent the v1 propsheet from zeroing colors, causing black text on black background. (GH-2651) fixes GH-2319 (cherry picked from commit b97db63) - Render the cursor state to VT (GH-2829) (cherry picked from commit a9f3849) - Don't put NUL in the buffer in VT mode (GH-3015) (cherry picked from commit a82d6b8) - Return to ground when we flush the last char (GH-2823) The InputStateMachineEngine was incorrectly not returning to the ground state after flushing the last sequence. That means that something like alt+backspace would leave us in the Escape state, not the ground state. This makes sure we return to ground. Additionally removes the "Parser.UnitTests-common.vcxproj" file, which was originally used for a theoretical time when we only open-sourced the parser. It's unnecessary now, and we can get rid of it. Also includes a small patch to bcz.cmd, to make sure bx works with projects with a space in their name. (cherry picked from commit 53c81a0) - Add support for passing through extended text attributes, like… (GH-2917) ... Related work items: #23678919, #23678920, #23731910, #23731911, #23731912, #23731913, #23731914, #23731915, #23731916, #23731917, #23731918
ghost
pushed a commit
that referenced
this pull request
May 2, 2022
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. `SetConsoleCursorPositionImpl` is one of the cases that actually does need to move the virtual viewport upwards sometimes, in particular when the cmd shell resets the buffer with a `CLS` command. But when this operation "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. `SetViewportOrigin` takes a flag which determines whether it will also move the virtual bottom to match the visible viewport. In some case this is appropriate (`SetConsoleCursorPositionImpl` being one example), but in other cases (e.g. when panning the viewport downwards in the `AdjustCursorPosition` function), it should only be allowed to move downwards. 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 `SetViewportOrigin` to only move the virtual bottom downwards, and added an explicit `UpdateBottom` call in those places that may also require upward movement. `ResizeWindow` in the `ConhostInternalGetSet` class has a similar problem to `SetConsoleCursorPositionImpl`, in that it's updating the viewport 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. `_InternalSetViewportSize` comes into play when resizing the window manually, 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. `ResizeWithReflow` is another sizing operation that can affect the virtual 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. `MakeCursorVisible` was another case where the virtual bottom was being updated (when requested with a flag) via a `SetViewportOrigin` call. 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 `SetViewportOrigin` was limited to moving the virtual bottom downwards. So I've now made it so that `MakeCursorVisible` never updates the virtual bottom. `SelectAll` in the `Selection` class was a similar case. It was calling `SetViewportOrigin` with the `updateBottom` flag set when that really wasn'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 `AdjustCursorPosition` which 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
PSReadline calls SetConsoleCursorPosition on each character they emit (go
figure). When that function is called, and we set the cursor position, we'll
try and "snap" the viewport to the location of the cursor, so that the cursor
remains visible.
However, we'd only ever do this with the visible viewport, the viewport
defined by
SCREEN_INFORMATION::_viewport. When there's a virtual viewport inTerminal Scrolling mode, we actually need to snap the virtual viewport, so
that this behavior looks more regular.
PR Checklist