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

[PR #2367] [MERGED] Make the RIS command clear the display and scrollback correctly #24901

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

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/2367
Author: @j4james
Created: 8/8/2019
Status: ✅ Merged
Merged: 8/28/2019
Merged by: @DHowett-MSFT

Base: master ← Head: fix-hard-reset

📝 Commits (5)

  • 3f0fce8 Return nothing from Viewport::Subtract when the removed viewport completely overlaps the original viewport.
  • dfa6680 Reset the SGR state before clearing the display so the correct background color is used, and clear the display before the scrollback, to prevent the last content being retained.
  • ed313c4 Add screen buffer tests to verify that a RIS hard reset clears the scrollback buffer correctly.
  • 7fbe60e Fix signed/unsigned mismatch in the updated viewport test.
  • cfb5b0e Add comment explaining the reason for the HardReset implementation order, and a more useful failure log in the screen buffer test.

📊 Changes

5 files changed (+123 additions, -67 deletions)

View changed files

📝 src/host/ut_host/ScreenBufferTests.cpp (+65 -0)
📝 src/host/ut_host/ViewportTests.cpp (+1 -11)
📝 src/terminal/adapter/adaptDispatch.cpp (+5 -4)
📝 src/terminal/adapter/ut_adapter/adapterTest.cpp (+27 -21)
📝 src/types/viewport.cpp (+25 -31)

📄 Description

Summary of the Pull Request

When the scrollback buffer is empty, the RIS escape sequence (Reset to Initial State) will fail to clear the screen, or reset any of the state. And when there is something in the scrollback, it doesn't get cleared completely, and the screen may get filled with the wrong background color (it should use the default color, but it actually uses the previously active background color). This PR attempts to fix those issues.

PR Checklist

  • Closes mc is slower and shows artifacts (#2307)
  • 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: #2307

Detailed Description of the Pull Request / Additional comments

The initial failure is caused by the SCREEN_INFORMATION::WriteRect method throwing an exception when passed an empty viewport. And the reason it's passed an empty viewport is because that's what the Viewport::Subtract method returns when the result of the subtraction is nothing. The PR fixes the problem by making the Viewport::Subtract method actually return nothing in that situation.

This is a change in the defined behavior that also required the associated viewport tests to be updated. However, it does seem a sensible change, since the Subtract method never returns empty viewports under any other circumstances. And the only place the method seems to be used is in the ScrollRegion implementation, where the previous behavior is guaranteed to throw an exception.

The other issues are fixed simply by changing the order in which things are reset in the AdaptDispatch::HardReset method. The call to SoftReset needed to be made first, so that the SGR attributes would be reset before the screen was cleared, thus making sure that the default background color would be used. And the screen needed to be cleared before the scrollback was erased, otherwise the last view of the screen would be retained in the scrollback buffer.

These changes also required existing adapter tests to be updated, but not because of a change in the expected behaviour. It's just that certain tests relied on the SoftReset happening later in the order, so weren't expecting it to be called if say the scrollback erase had failed. It doesn't seem like the tests were deliberately trying to verify that the SoftReset hadn't been called.

Validation Steps Performed

In addition to the updates to existing tests, this PR also add a new screen buffer test which verifies the display and scrollback are correctly cleared under the conditions that were previously failing.

🔄 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/2367 **Author:** [@j4james](https://github.com/j4james) **Created:** 8/8/2019 **Status:** ✅ Merged **Merged:** 8/28/2019 **Merged by:** [@DHowett-MSFT](https://github.com/DHowett-MSFT) **Base:** `master` ← **Head:** `fix-hard-reset` --- ### 📝 Commits (5) - [`3f0fce8`](https://github.com/microsoft/terminal/commit/3f0fce83e311a40b6c0528472e3081d4c090ee79) Return nothing from Viewport::Subtract when the removed viewport completely overlaps the original viewport. - [`dfa6680`](https://github.com/microsoft/terminal/commit/dfa668054c0ba8578df9e69df2f230caaa6ae812) Reset the SGR state before clearing the display so the correct background color is used, and clear the display before the scrollback, to prevent the last content being retained. - [`ed313c4`](https://github.com/microsoft/terminal/commit/ed313c45faddfdd2280c46d7e07297b406df44fa) Add screen buffer tests to verify that a RIS hard reset clears the scrollback buffer correctly. - [`7fbe60e`](https://github.com/microsoft/terminal/commit/7fbe60e1d2eadce8ba9e10aaf777049e00d997e6) Fix signed/unsigned mismatch in the updated viewport test. - [`cfb5b0e`](https://github.com/microsoft/terminal/commit/cfb5b0e874054801c740c6a17dea945da070d3d1) Add comment explaining the reason for the HardReset implementation order, and a more useful failure log in the screen buffer test. ### 📊 Changes **5 files changed** (+123 additions, -67 deletions) <details> <summary>View changed files</summary> 📝 `src/host/ut_host/ScreenBufferTests.cpp` (+65 -0) 📝 `src/host/ut_host/ViewportTests.cpp` (+1 -11) 📝 `src/terminal/adapter/adaptDispatch.cpp` (+5 -4) 📝 `src/terminal/adapter/ut_adapter/adapterTest.cpp` (+27 -21) 📝 `src/types/viewport.cpp` (+25 -31) </details> ### 📄 Description ## Summary of the Pull Request When the scrollback buffer is empty, the RIS escape sequence (Reset to Initial State) will fail to clear the screen, or reset any of the state. And when there is something in the scrollback, it doesn't get cleared completely, and the screen may get filled with the wrong background color (it should use the default color, but it actually uses the previously active background color). This PR attempts to fix those issues. ## PR Checklist * [x] Closes #2307 * [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: #2307 ## Detailed Description of the Pull Request / Additional comments The initial failure is caused by the `SCREEN_INFORMATION::WriteRect` method [throwing an exception](https://github.com/microsoft/terminal/blob/7abcc35fdf7d98bc98016cda8f4b90398c65692e/src/host/screenInfo.cpp#L2608-L2609) when passed an empty viewport. And the reason it's passed an empty viewport is because that's what the `Viewport::Subtract` method returns [when the result of the subtraction is nothing](https://github.com/microsoft/terminal/blob/9b92986b49bed8cc41fde4d6ef080921c41e6d9e/src/types/viewport.cpp#L995-L998). The PR fixes the problem by making the `Viewport::Subtract` method actually return nothing in that situation. This is a change in the defined behavior that also required the associated viewport tests to be updated. However, it does seem a sensible change, since the `Subtract` method never returns empty viewports under any other circumstances. And [the only place the method seems to be used](https://github.com/microsoft/terminal/blob/9b92986b49bed8cc41fde4d6ef080921c41e6d9e/src/host/output.cpp#L469-L476) is in the `ScrollRegion` implementation, where the previous behavior is guaranteed to throw an exception. The other issues are fixed simply by changing the order in which things are reset in the `AdaptDispatch::HardReset` method. The call to `SoftReset` needed to be made first, so that the SGR attributes would be reset before the screen was cleared, thus making sure that the default background color would be used. And the screen needed to be cleared before the scrollback was erased, otherwise the last view of the screen would be retained in the scrollback buffer. These changes also required existing adapter tests to be updated, but not because of a change in the expected behaviour. It's just that certain tests relied on the `SoftReset` happening later in the order, so weren't expecting it to be called if say the scrollback erase had failed. It doesn't seem like the tests were deliberately trying to verify that the SoftReset _hadn't_ been called. ## Validation Steps Performed In addition to the updates to existing tests, this PR also add a new screen buffer test which verifies the display and scrollback are correctly cleared under the conditions that were previously failing. --- <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:06: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#24901
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?