starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

[PR #12972] [MERGED] Prevent the virtual viewport bottom being updated incorrectly #29309

New Issue
Open
opened 2026-01-31 09:34:09 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:34:09 +00:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/12972
Author: @j4james
Created: 4/24/2022
Status: ✅ Merged
Merged: 5/2/2022
Merged by: @undefined

Base: main ← Head: fix-update-bottom-2

📝 Commits (9)

  • 3b2f4dd Reimplement virtual bottom adjustment in _InternalSetViewportSize.
  • d0d9e48 Make sure VT resize operations keep the cursor in view.
  • 325b464 Only move the virtual bottom downwards in SetViewportOrigin.
  • 60e88c1 Always use the virtual viewport for SetConsoleCursorPosition viewport snapping.
  • 8225119 Don't ever update the virtual bottom from MakeCursorVisible.
  • 47d6263 Make sure ResizeWithReflow doesn't truncate the virtual buffer.
  • a468803 Don't update the virtual bottom from SelectAll.
  • 6505dc6 Initialize virtual bottom in ScreenBufferTests.
  • b0d609b Add some screen buffer tests.

📊 Changes

7 files changed (+330 additions, -34 deletions)

View changed files

📝 src/host/getset.cpp (+9 -8)
📝 src/host/outputStream.cpp (+12 -2)
📝 src/host/screenInfo.cpp (+30 -19)
📝 src/host/screenInfo.hpp (+1 -1)
📝 src/host/selection.cpp (+3 -3)
📝 src/host/selectionInput.cpp (+1 -1)
📝 src/host/ut_host/ScreenBufferTests.cpp (+274 -0)

📄 Description

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

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/12972 **Author:** [@j4james](https://github.com/j4james) **Created:** 4/24/2022 **Status:** ✅ Merged **Merged:** 5/2/2022 **Merged by:** [@undefined](undefined) **Base:** `main` ← **Head:** `fix-update-bottom-2` --- ### 📝 Commits (9) - [`3b2f4dd`](https://github.com/microsoft/terminal/commit/3b2f4dd6bed8b9c1ac7730a771bfa02507d94e76) Reimplement virtual bottom adjustment in _InternalSetViewportSize. - [`d0d9e48`](https://github.com/microsoft/terminal/commit/d0d9e48daf78effb631e63ebf255bb14fc327eee) Make sure VT resize operations keep the cursor in view. - [`325b464`](https://github.com/microsoft/terminal/commit/325b464835ea14434eb0c9f1b7cfac3f7cbae52e) Only move the virtual bottom downwards in SetViewportOrigin. - [`60e88c1`](https://github.com/microsoft/terminal/commit/60e88c1f3301c70a26d3199c39f1e46a7b8c22a7) Always use the virtual viewport for SetConsoleCursorPosition viewport snapping. - [`8225119`](https://github.com/microsoft/terminal/commit/82251190da7f129f19f3186fb78f6704d818d8a4) Don't ever update the virtual bottom from MakeCursorVisible. - [`47d6263`](https://github.com/microsoft/terminal/commit/47d6263bce8e38c3d8f1089f22649b530bce7829) Make sure ResizeWithReflow doesn't truncate the virtual buffer. - [`a468803`](https://github.com/microsoft/terminal/commit/a46880357b2adb05661a9485a10c432964775410) Don't update the virtual bottom from SelectAll. - [`6505dc6`](https://github.com/microsoft/terminal/commit/6505dc6a3d73b79c4b186db8036b6f74237c54b6) Initialize virtual bottom in ScreenBufferTests. - [`b0d609b`](https://github.com/microsoft/terminal/commit/b0d609b5ba9aec4653a80948e5dc3c8941544897) Add some screen buffer tests. ### 📊 Changes **7 files changed** (+330 additions, -34 deletions) <details> <summary>View changed files</summary> 📝 `src/host/getset.cpp` (+9 -8) 📝 `src/host/outputStream.cpp` (+12 -2) 📝 `src/host/screenInfo.cpp` (+30 -19) 📝 `src/host/screenInfo.hpp` (+1 -1) 📝 `src/host/selection.cpp` (+3 -3) 📝 `src/host/selectionInput.cpp` (+1 -1) 📝 `src/host/ut_host/ScreenBufferTests.cpp` (+274 -0) </details> ### 📄 Description 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 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:34:09 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#29309
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?