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

[PR #2764] [MERGED] Remove unwanted DECSTBM clipping #25062

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/2764
Author: @j4james
Created: 9/15/2019
Status: ✅ Merged
Merged: 9/24/2019
Merged by: @DHowett-MSFT

Base: master ← Head: fix-unwanted-margins

📝 Commits (8)

  • d94be7a Apply DECSTBM margins before scrolling for the RI, IL, and DL escape sequences.
  • e8bcb60 Apply DECSTBM margins before scrolling for the SU and SD escape sequences.
  • cb9b0c4 Remove the unwanted DECSTBM margin clipping from the ScrollRegion function.
  • 74bf34e Remove the unneeded margin modification in the AdjustCursorPosition function.
  • 1394d84 Extend the ICH and DCH screenbuffer tests, to check that they also work outside the DECSTBM margin boundaries.
  • 64f3ad9 Extend the ScrollConsoleScreenBuffer API test, to check that it is not affected by the DECSTBM margins.
  • 8f730b9 Make sure the scrolling in AdjustCursorPosition always affects the full buffer width.
  • 7ab2026 Reset the margins on exit from tests that change them, otherwise those margins can persist into following tests.

📊 Changes

6 files changed (+56 additions, -55 deletions)

View changed files

📝 src/host/_stream.cpp (+3 -27)
📝 src/host/getset.cpp (+10 -0)
📝 src/host/output.cpp (+0 -25)
📝 src/host/ut_host/ApiRoutinesTests.cpp (+10 -0)
📝 src/host/ut_host/ScreenBufferTests.cpp (+26 -0)
📝 src/terminal/adapter/adaptDispatch.cpp (+7 -3)

📄 Description

Summary of the Pull Request

The DECSTBM margins are meant to define the range of lines within which certain vertical scrolling operations take place. However, we were applying these margin restrictions in the ScrollRegion function, which is also used in a number of places that shouldn't be affected by DECSTBM.

This includes the ICH and DCH escape sequences (which are only affected by the horizontal margins, which we don't yet support), the ScrollConsoleScreenBuffer API (which is public Console API, not meant to be affected by the VT terminal emulation), and the CSI 3 J erase scrollback extension (which isn't really scrolling as such, but uses the ScrollRegion function to manipulate the scrollback buffer).

This PR moves the margin clipping out of the ScrollRegion function, so it can be applied exclusively in the places that need it.

PR Checklist

  • Closes #2543 and Bug Report - Jerky terminal (#2659)
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #2543

Detailed Description of the Pull Request / Additional comments

With the margin clipping removed from the ScrollRegion function, it now had to be applied manually in the places it was actually required. This included:

  • The DoSrvPrivateReverseLineFeed function (for the RI control): This was just a matter of updating the bottom of the scroll rect to the bottom margin (at least when the margins were actually set), since the top of the scroll rect would always be the top of the viewport.
  • The DoSrvPrivateModifyLinesImpl function (for the IL and DL commands): Again this was just a matter of updating the bottom of the scroll rect, since the cursor position would always determine the top of the scroll rect.
  • The AdaptDispatch::_ScrollMovement method (for the SU and SD commands): This required updating both the top and bottom coordinates of the scroll rect, and also a simpler destination Y coordinate (the way the ScrollRegion function worked before, the caller was expected to take the margins into account when determining the destination).

On the plus side, there was now no longer a need to override the margins when calling ScrollRegion in the AdjustCursorPosition function. In the first case, the margins had needed to be cleared, but that is now the default behaviour. In the second case, there had been a more complicated adjustment of the margins, but that code was never actually used so could be removed completely (to get to that point either fScrollUp was true, so scrollDownAtTop couldn't also be true, or fScrollDown was true, but in that case there is a check to make sure scrollDownAtTop is false).

While testing, I also noticed that one of the ScrollRegion calls in the AdjustCursorPosition function was not setting the horizontal range correctly - the scrolling should always affect the full buffer width rather than just the viewport width - so I've fixed that now as well.

Validation Steps Performed

For commands like RI, IL, DL, etc. where we've changed the implementation but not the behaviour, there were already unit tests that could confirm that the new implementation was still producing the correct results.

Where there has been a change in behaviour - namely for the ICH and DCH commands, and the ScrollConsoleScreenBuffer API - I've extended the existing unit tests to check that they still function correctly even when the DECSTBM margins are set (which would previously have caused them to fail).

I've also tested manually with the test cases in issues #2543 and #2659, and confirmed that they now work as expected.

🔄 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/2764 **Author:** [@j4james](https://github.com/j4james) **Created:** 9/15/2019 **Status:** ✅ Merged **Merged:** 9/24/2019 **Merged by:** [@DHowett-MSFT](https://github.com/DHowett-MSFT) **Base:** `master` ← **Head:** `fix-unwanted-margins` --- ### 📝 Commits (8) - [`d94be7a`](https://github.com/microsoft/terminal/commit/d94be7a95c79bb80e8ee7533c02e48f8eefcf123) Apply DECSTBM margins before scrolling for the RI, IL, and DL escape sequences. - [`e8bcb60`](https://github.com/microsoft/terminal/commit/e8bcb606b6c3c09c72a442ee9e1f0d625299f4b8) Apply DECSTBM margins before scrolling for the SU and SD escape sequences. - [`cb9b0c4`](https://github.com/microsoft/terminal/commit/cb9b0c48a08e4e90d66d0e5c8b59bb6172797e81) Remove the unwanted DECSTBM margin clipping from the ScrollRegion function. - [`74bf34e`](https://github.com/microsoft/terminal/commit/74bf34e75b1b202156ec046f321cab9adec8dfb8) Remove the unneeded margin modification in the AdjustCursorPosition function. - [`1394d84`](https://github.com/microsoft/terminal/commit/1394d84366a96ee188b44d5b6934e15253fb52a7) Extend the ICH and DCH screenbuffer tests, to check that they also work outside the DECSTBM margin boundaries. - [`64f3ad9`](https://github.com/microsoft/terminal/commit/64f3ad91e542ddce62dd81946fa575427ab7017b) Extend the ScrollConsoleScreenBuffer API test, to check that it is not affected by the DECSTBM margins. - [`8f730b9`](https://github.com/microsoft/terminal/commit/8f730b9c810a8bbe29a4324a1ebdb9ebb61bd5b1) Make sure the scrolling in AdjustCursorPosition always affects the full buffer width. - [`7ab2026`](https://github.com/microsoft/terminal/commit/7ab2026bfe842d299cb8d53470fe564120e14409) Reset the margins on exit from tests that change them, otherwise those margins can persist into following tests. ### 📊 Changes **6 files changed** (+56 additions, -55 deletions) <details> <summary>View changed files</summary> 📝 `src/host/_stream.cpp` (+3 -27) 📝 `src/host/getset.cpp` (+10 -0) 📝 `src/host/output.cpp` (+0 -25) 📝 `src/host/ut_host/ApiRoutinesTests.cpp` (+10 -0) 📝 `src/host/ut_host/ScreenBufferTests.cpp` (+26 -0) 📝 `src/terminal/adapter/adaptDispatch.cpp` (+7 -3) </details> ### 📄 Description ## Summary of the Pull Request The [`DECSTBM`](https://vt100.net/docs/vt510-rm/DECSTBM.html) margins are meant to define the range of lines within which certain vertical scrolling operations take place. However, we were applying these margin restrictions in the `ScrollRegion` function, which is also used in a number of places that shouldn't be affected by `DECSTBM`. This includes the [`ICH`](https://vt100.net/docs/vt510-rm/ICH.html) and [`DCH`](https://vt100.net/docs/vt510-rm/DCH.html) escape sequences (which are only affected by the horizontal margins, which we don't yet support), the [`ScrollConsoleScreenBuffer`](https://docs.microsoft.com/en-us/windows/console/scrollconsolescreenbuffer) API (which is public Console API, not meant to be affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback extension (which isn't really scrolling as such, but uses the `ScrollRegion` function to manipulate the scrollback buffer). This PR moves the margin clipping out of the `ScrollRegion` function, so it can be applied exclusively in the places that need it. ## PR Checklist * [x] Closes #2543 and #2659 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #2543 ## Detailed Description of the Pull Request / Additional comments With the margin clipping removed from the `ScrollRegion` function, it now had to be applied manually in the places it was actually required. This included: * The `DoSrvPrivateReverseLineFeed` function (for the [`RI`](https://vt100.net/docs/vt510-rm/chapter4.html#T4-2) control): This was just a matter of updating the bottom of the scroll rect to the bottom margin (at least when the margins were actually set), since the top of the scroll rect would always be the top of the viewport. * The `DoSrvPrivateModifyLinesImpl` function (for the [`IL`](https://vt100.net/docs/vt510-rm/IL.html) and [`DL`](https://vt100.net/docs/vt510-rm/DL.html) commands): Again this was just a matter of updating the bottom of the scroll rect, since the cursor position would always determine the top of the scroll rect. * The `AdaptDispatch::_ScrollMovement` method (for the [`SU`](https://vt100.net/docs/vt510-rm/SU.html) and [`SD`](https://vt100.net/docs/vt510-rm/SD.html) commands): This required updating both the top and bottom coordinates of the scroll rect, and also a simpler destination Y coordinate (the way the `ScrollRegion` function worked before, the caller was expected to take the margins into account when determining the destination). On the plus side, there was now no longer a need to override the margins when calling `ScrollRegion` in the `AdjustCursorPosition` function. In the first case, [the margins had needed to be cleared](https://github.com/microsoft/terminal/blob/3d35e396b257d9281c6ccaa488d237b35c506d7e/src/host/_stream.cpp#L143-L145), but that is now the default behaviour. In the second case, there had been [a more complicated adjustment of the margins](https://github.com/microsoft/terminal/blob/3d35e396b257d9281c6ccaa488d237b35c506d7e/src/host/_stream.cpp#L196-L209), but that code was never actually used so could be removed completely (to get to that point either _fScrollUp_ was true, so _scrollDownAtTop_ couldn't also be true, or _fScrollDown_ was true, but in that case there is a check to make sure _scrollDownAtTop_ is false). While testing, I also noticed that one of the `ScrollRegion` calls in the `AdjustCursorPosition` function was not setting the horizontal range correctly - the scrolling should always affect the full buffer width rather than just the viewport width - so I've fixed that now as well. ## Validation Steps Performed For commands like `RI`, `IL`, `DL`, etc. where we've changed the implementation but not the behaviour, there were already unit tests that could confirm that the new implementation was still producing the correct results. Where there has been a change in behaviour - namely for the `ICH` and `DCH` commands, and the `ScrollConsoleScreenBuffer` API - I've extended the existing unit tests to check that they still function correctly even when the `DECSTBM` margins are set (which would previously have caused them to fail). I've also tested manually with the test cases in issues #2543 and #2659, and confirmed that they now work as expected. --- <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:07:01 +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#25062
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?