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 #3100] [MERGED] Correct fill attributes when scrolling and erasing #25241

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/3100
Author: @j4james
Created: 10/6/2019
Status: ✅ Merged
Merged: 12/10/2019
Merged by: @undefined

Base: master ← Head: fix-fill-attributes

📝 Commits (10+)

  • 4c09e68 Add private scroll region API with support for standard erase attributes.
  • 093b94f Update scrolling operations to use the new API that sets the fill attributes correctly.
  • b41d2ff Update existing screen buffer tests to check that the scrolling operations fill with the correct attributes.
  • 6f2ef8f Update the line feed handling to set the fill attributes correctly for rows scrolled into view.
  • 6aa3e97 Update and add screen buffer tests to check that line feeds initialize new rows with the correct attributes.
  • 32321a9 Add private fill region API with support for standard erase attributes.
  • ba1e16f Update the erase operations to use the new API that sets the fill attributes correctly, and make sure the full buffer width is covered.
  • a83fd64 Update the VtEraseAll implementation to use the correct fill attributes, and cover the full buffer width.
  • 480a924 Update the EraseScrollback implementation to use the new APIs that set the fill attributes correctly, and make sure the full buffer width is affected.
  • d85c7e2 Replace the adapter tests with more appropriate screen buffer tests for the erase and hard reset operations.

📊 Changes

17 files changed (+637 additions, -859 deletions)

View changed files

📝 src/buffer/out/TextAttribute.cpp (+9 -0)
📝 src/buffer/out/TextAttribute.hpp (+2 -0)
📝 src/buffer/out/textBuffer.cpp (+10 -3)
📝 src/buffer/out/textBuffer.hpp (+1 -1)
📝 src/host/_output.cpp (+0 -18)
📝 src/host/_stream.cpp (+6 -3)
📝 src/host/getset.cpp (+108 -64)
📝 src/host/getset.h (+12 -0)
📝 src/host/output.cpp (+2 -1)
📝 src/host/outputStream.cpp (+51 -58)
📝 src/host/outputStream.hpp (+10 -14)
📝 src/host/screenInfo.cpp (+20 -6)
📝 src/host/ut_host/ScreenBufferTests.cpp (+339 -30)
📝 src/terminal/adapter/adaptDispatch.cpp (+39 -112)
📝 src/terminal/adapter/adaptDispatch.hpp (+1 -3)
📝 src/terminal/adapter/conGetSet.hpp (+10 -12)
📝 src/terminal/adapter/ut_adapter/adapterTest.cpp (+17 -534)

📄 Description

Summary of the Pull Request

Operations that erase areas of the screen are typically meant to do so using the current color attributes, but with the rendition attributes reset (what we refer to as meta attributes). This also includes scroll operations that have to clear the area of the screen that has scrolled into view. The only exception is the Erase Scrollback operation, which needs to reset the buffer with the default attributes. This PR updates all of these cases to apply the correct attributes when scrolling and erasing.

PR Checklist

  • Closes Position of chinese word choice box fault (#2553)
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've not really discussed this with core contributors. I'm ready to accept this work might be rejected in favor of a different grand plan.

Detailed Description of the Pull Request / Additional comments

My initial plan was to use a special case legacy attribute value to indicate the "standard erase attribute" which could safely be passed through the legacy APIs. But this wouldn't cover the cases that required default attributes to be used. And then with the changes in PR #2668 and #2987, it became clear that our requirements could be better achieved with a couple of new private APIs that wouldn't have to depend on legacy attribute hacks at all.

To that end, I've added the PrivateFillRegion and PrivateScrollRegion APIs to the ConGetSet interface. These are just thin wrappers around the existing SCREEN_INFORMATION::Write method and the ScrollRegion function respectively, but with a simple boolean parameter to choose between filling with default attributes or the standard erase attributes (i.e the current colors but with meta attributes reset).

With those new APIs in place, I could then update most scroll operations to use PrivateScrollRegion, and most erase operations to use PrivateFillRegion.

The functions affected by scrolling included:

  • DoSrvPrivateReverseLineFeed (the RI command)
  • DoSrvPrivateModifyLinesImpl (the IL and DL commands)
  • AdaptDispatch::_InsertDeleteHelper (the ICH and DCH commands)
  • AdaptDispatch::_ScrollMovement (the SU and SD commands)

The functions affected by erasing included:

  • AdaptDispatch::_EraseSingleLineHelper (the EL command, and most ED variants)
  • AdaptDispatch::EraseCharacters (the ECH command)

While updating these erase methods, I noticed that both of them also required boundary fixes similar to those in PR #2505 (i.e. the horizontal extent of the erase operation should apply to the full width of the buffer, and not just the current viewport width), so I've addressed that at the same time.

In addition to the changes above, there were also a few special cases, the first being the line feed handling, which required updating in a number of places to use the correct erase attributes:

  • SCREEN_INFORMATION::InitializeCursorRowAttributes - this is used to initialise the rows that pan into view when the viewport is moved down the buffer.
  • TextBuffer::IncrementCircularBuffer - this occurs when we scroll passed the very end of the buffer, and a recycled row now needs to be reinitialised.
  • AdjustCursorPosition - when within margin boundaries, this relies on a couple of direct calls to ScrollRegion which needed to be passed the correct fill attributes.

The second special case was the full screen erase sequence (ESC 2 J), which is handled separately from the other ED sequences. This required updating the SCREEN_INFORMATION::VtEraseAll method to use the standard erase attributes, and also required changes to the horizontal extent of the filled area, since it should have been clearing the full buffer width (the same issue as the other erase operations mentioned above).

Finally, there was the AdaptDispatch::_EraseScrollback method, which uses both scroll and fill operations, which could now be handled by the new PrivateScrollRegion and PrivateFillRegion APIs. But in this case we needed to fill with the default attributes rather than the standard erase attributes. And again this implementation needed some changes to make sure the full width of the active area was retained after the erase, similar to the horizontal boundary issues with the other erase operations.

Once all these changes were made, there were a few areas of the code that could then be simplified quite a bit. The FillConsoleOutputCharacterW, FillConsoleOutputAttribute, and ScrollConsoleScreenBufferW were no longer needed in the ConGetSet interface, so all of that code could now be removed. The _EraseSingleLineDistanceHelper and _EraseAreaHelper methods in the AdaptDispatch class were also no longer required and could be removed.

Then there were the hacks to handle legacy default colors in the FillConsoleOutputAttributeImpl and ScrollConsoleScreenBufferWImpl implementations. Since those hacks were only needed for VT operations, and the VT code no longer calls those methods, there was no longer a need to retain that behaviour (in fact there are probably some edge cases where that behaviour might have been considered a bug when reached via the public console APIs).

Validation Steps Performed

For most of the scrolling operations there were already existing tests in place, and those could easily be extended to check that the meta attributes were correctly reset when filling the revealed lines of the scrolling region.

In the screen buffer tests, I made updates of that sort to the ScrollOperations method (handling SU, SD, IL, DL, and RI), the InsertChars and DeleteChars methods (ICH and DCH), and the VtNewlinePastViewport method (LF). I also added a new VtNewlinePastEndOfBuffer test to check the case where the line feed causes the viewport to pan past the end of the buffer.

The erase operations, however, were being covered by adapter tests, and those aren't really suited for this kind of functionality (the same sort of issue came up in PR #2505). As a result I've had to reimplement those tests as screen buffer tests.

Most of the erase operations are covered by the EraseTests method, except the for the scrollback erase which has a dedicated EraseScrollbackTests method. I've also had to replace the HardReset adapter test, but that was already mostly covered by the HardResetBuffer screen buffer test, which I've now extended slightly (it could do with some more checks, but I think that can wait for a future PR when we're fixing other RIS issues).

🔄 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/3100 **Author:** [@j4james](https://github.com/j4james) **Created:** 10/6/2019 **Status:** ✅ Merged **Merged:** 12/10/2019 **Merged by:** [@undefined](undefined) **Base:** `master` ← **Head:** `fix-fill-attributes` --- ### 📝 Commits (10+) - [`4c09e68`](https://github.com/microsoft/terminal/commit/4c09e683f74ec4d6e2109ffa5b262db7384dd14d) Add private scroll region API with support for standard erase attributes. - [`093b94f`](https://github.com/microsoft/terminal/commit/093b94fd0f9ffc307c1665993f92e283828021bd) Update scrolling operations to use the new API that sets the fill attributes correctly. - [`b41d2ff`](https://github.com/microsoft/terminal/commit/b41d2ff1b3404b0c8ee99c4bb7355eb4aee2b0ed) Update existing screen buffer tests to check that the scrolling operations fill with the correct attributes. - [`6f2ef8f`](https://github.com/microsoft/terminal/commit/6f2ef8f99a7b0e23d29b97147108af90824c4876) Update the line feed handling to set the fill attributes correctly for rows scrolled into view. - [`6aa3e97`](https://github.com/microsoft/terminal/commit/6aa3e978aa7b780096a0467584fad2bebe4692e0) Update and add screen buffer tests to check that line feeds initialize new rows with the correct attributes. - [`32321a9`](https://github.com/microsoft/terminal/commit/32321a90bec9e408c2d24bb898e7b69dd05106ab) Add private fill region API with support for standard erase attributes. - [`ba1e16f`](https://github.com/microsoft/terminal/commit/ba1e16f7720f21087ff03aa9c4b3e074bf17e9ca) Update the erase operations to use the new API that sets the fill attributes correctly, and make sure the full buffer width is covered. - [`a83fd64`](https://github.com/microsoft/terminal/commit/a83fd64d0456811e8dea676e13e49080ab3c0f32) Update the VtEraseAll implementation to use the correct fill attributes, and cover the full buffer width. - [`480a924`](https://github.com/microsoft/terminal/commit/480a92438eb6fdaa6ad992fefacc95e9ac596939) Update the EraseScrollback implementation to use the new APIs that set the fill attributes correctly, and make sure the full buffer width is affected. - [`d85c7e2`](https://github.com/microsoft/terminal/commit/d85c7e29e9ee3c819149f1bf7c0690182ba64c49) Replace the adapter tests with more appropriate screen buffer tests for the erase and hard reset operations. ### 📊 Changes **17 files changed** (+637 additions, -859 deletions) <details> <summary>View changed files</summary> 📝 `src/buffer/out/TextAttribute.cpp` (+9 -0) 📝 `src/buffer/out/TextAttribute.hpp` (+2 -0) 📝 `src/buffer/out/textBuffer.cpp` (+10 -3) 📝 `src/buffer/out/textBuffer.hpp` (+1 -1) 📝 `src/host/_output.cpp` (+0 -18) 📝 `src/host/_stream.cpp` (+6 -3) 📝 `src/host/getset.cpp` (+108 -64) 📝 `src/host/getset.h` (+12 -0) 📝 `src/host/output.cpp` (+2 -1) 📝 `src/host/outputStream.cpp` (+51 -58) 📝 `src/host/outputStream.hpp` (+10 -14) 📝 `src/host/screenInfo.cpp` (+20 -6) 📝 `src/host/ut_host/ScreenBufferTests.cpp` (+339 -30) 📝 `src/terminal/adapter/adaptDispatch.cpp` (+39 -112) 📝 `src/terminal/adapter/adaptDispatch.hpp` (+1 -3) 📝 `src/terminal/adapter/conGetSet.hpp` (+10 -12) 📝 `src/terminal/adapter/ut_adapter/adapterTest.cpp` (+17 -534) </details> ### 📄 Description ## Summary of the Pull Request Operations that erase areas of the screen are typically meant to do so using the current color attributes, but with the rendition attributes reset (what we refer to as meta attributes). This also includes scroll operations that have to clear the area of the screen that has scrolled into view. The only exception is the _Erase Scrollback_ operation, which needs to reset the buffer with the default attributes. This PR updates all of these cases to apply the correct attributes when scrolling and erasing. ## PR Checklist * [x] Closes #2553 * [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 * [ ] I've not really discussed this with core contributors. I'm ready to accept this work might be rejected in favor of a different grand plan. ## Detailed Description of the Pull Request / Additional comments My initial plan was to use a special case legacy attribute value to indicate the "standard erase attribute" which could safely be passed through the legacy APIs. But this wouldn't cover the cases that required default attributes to be used. And then with the changes in PR #2668 and #2987, it became clear that our requirements could be better achieved with a couple of new private APIs that wouldn't have to depend on legacy attribute hacks at all. To that end, I've added the `PrivateFillRegion` and `PrivateScrollRegion` APIs to the `ConGetSet` interface. These are just thin wrappers around the existing `SCREEN_INFORMATION::Write` method and the `ScrollRegion` function respectively, but with a simple boolean parameter to choose between filling with default attributes or the standard erase attributes (i.e the current colors but with meta attributes reset). With those new APIs in place, I could then update most scroll operations to use `PrivateScrollRegion`, and most erase operations to use `PrivateFillRegion`. The functions affected by scrolling included: * `DoSrvPrivateReverseLineFeed` (the RI command) * `DoSrvPrivateModifyLinesImpl` (the IL and DL commands) * `AdaptDispatch::_InsertDeleteHelper` (the ICH and DCH commands) * `AdaptDispatch::_ScrollMovement` (the SU and SD commands) The functions affected by erasing included: * `AdaptDispatch::_EraseSingleLineHelper` (the EL command, and most ED variants) * `AdaptDispatch::EraseCharacters` (the ECH command) While updating these erase methods, I noticed that both of them also required boundary fixes similar to those in PR #2505 (i.e. the horizontal extent of the erase operation should apply to the full width of the buffer, and not just the current viewport width), so I've addressed that at the same time. In addition to the changes above, there were also a few special cases, the first being the line feed handling, which required updating in a number of places to use the correct erase attributes: * `SCREEN_INFORMATION::InitializeCursorRowAttributes` - this is used to initialise the rows that pan into view when the viewport is moved down the buffer. * `TextBuffer::IncrementCircularBuffer` - this occurs when we scroll passed the very end of the buffer, and a recycled row now needs to be reinitialised. * `AdjustCursorPosition` - when within margin boundaries, this relies on a couple of direct calls to `ScrollRegion` which needed to be passed the correct fill attributes. The second special case was the full screen erase sequence (`ESC 2 J`), which is handled separately from the other ED sequences. This required updating the `SCREEN_INFORMATION::VtEraseAll` method to use the standard erase attributes, and also required changes to the horizontal extent of the filled area, since it should have been clearing the full buffer width (the same issue as the other erase operations mentioned above). Finally, there was the `AdaptDispatch::_EraseScrollback` method, which uses both scroll and fill operations, which could now be handled by the new `PrivateScrollRegion` and `PrivateFillRegion` APIs. But in this case we needed to fill with the default attributes rather than the standard erase attributes. And again this implementation needed some changes to make sure the full width of the active area was retained after the erase, similar to the horizontal boundary issues with the other erase operations. Once all these changes were made, there were a few areas of the code that could then be simplified quite a bit. The `FillConsoleOutputCharacterW`, `FillConsoleOutputAttribute`, and `ScrollConsoleScreenBufferW` were no longer needed in the `ConGetSet` interface, so all of that code could now be removed. The `_EraseSingleLineDistanceHelper` and `_EraseAreaHelper` methods in the `AdaptDispatch` class were also no longer required and could be removed. Then there were the hacks to handle legacy default colors in the `FillConsoleOutputAttributeImpl` and `ScrollConsoleScreenBufferWImpl` implementations. Since those hacks were only needed for VT operations, and the VT code no longer calls those methods, there was no longer a need to retain that behaviour (in fact there are probably some edge cases where that behaviour might have been considered a bug when reached via the public console APIs). ## Validation Steps Performed For most of the scrolling operations there were already existing tests in place, and those could easily be extended to check that the meta attributes were correctly reset when filling the revealed lines of the scrolling region. In the screen buffer tests, I made updates of that sort to the `ScrollOperations` method (handling SU, SD, IL, DL, and RI), the `InsertChars` and `DeleteChars` methods (ICH and DCH), and the `VtNewlinePastViewport` method (LF). I also added a new `VtNewlinePastEndOfBuffer` test to check the case where the line feed causes the viewport to pan past the end of the buffer. The erase operations, however, were being covered by adapter tests, and those aren't really suited for this kind of functionality (the same sort of issue came up in PR #2505). As a result I've had to reimplement those tests as screen buffer tests. Most of the erase operations are covered by the `EraseTests` method, except the for the scrollback erase which has a dedicated `EraseScrollbackTests` method. I've also had to replace the `HardReset` adapter test, but that was already mostly covered by the `HardResetBuffer` screen buffer test, which I've now extended slightly (it could do with some more checks, but I think that can wait for a future PR when we're fixing other RIS issues). --- <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:08:13 +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#25241
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?