RIS reset doesn't clear the display and scrollback correctly #2307
Assignees
Labels
Area-VT
Virtual Terminal sequence support
Issue-Bug
It either shouldn't be doing this or needs an investigation.
Product-Conhost
For issues in the Console codebase
Resolution-Fix-Available
It's available in an Insiders build or a release
Milestone
Comments
ghost
added
Needs-Triage
DHowett-MSFT
added
Area-VT
|
I've tagged this up for you, but we can wait for @zadjii-msft if you don't want to start on it just yet. 😄 |
|
Yea this seems like an accurate assessment of the situation. Great find :) |
5 tasks
ghost
added
Needs-Tag-Fix
DHowett-MSFT
pushed a commit
that referenced
this issue
Aug 28, 2019
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 commit attempts to fix those issues. 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. 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. Fixes #2307.
ghost
added
Resolution-Fix-Committed
|
I tried the CI build but |
DHowett-MSFT
pushed a commit
that referenced
this issue
Sep 3, 2019
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 commit attempts to fix those issues. 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. 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. Fixes #2307. (cherry picked from commit 974e95e)
|
@Visualer Note that this is a fix for conhost - Window Terminal may have other problems of its own. |
|
🎉This issue was addressed in #2367, which has now been successfully released as Handy links: |
DHowett-MSFT
added
Resolution-Fix-Available
|
This also went out with Windows Insider Fast 18990! 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Environment
Steps to reproduce
printf "\ec"(the RIS escape sequence).printf "\e[41m\ec"(setting the background to red, followed by RIS).Expected behavior
The RIS escape sequence should clear the display with the default background color, and should erase everything in the scrollback buffer.
Actual behavior
The RIS escape sequence doesn't work at all when there isn't anything in the scrollback. And when it does work, it clears the display with the wrong color, and doesn't completely erase the scrollback buffer.
More details
The initial RIS doesn't work because of the way the
Viewport::Subtractmethod is implemented. When the removeMe viewport completely overlaps the original viewport, it returns a single empty viewport, but it really ought to return nothing. An empty viewport throws an exception when passed to theSCREEN_INFORMATION::WriteRectmethod, which is what happens when called from theScrollRegionfunction here. This in turn causes theAdaptDispatch::_EraseScrollbackmethod to fail, which then aborts theAdaptDispatch::HardResetmethod.The reason the color is wrong, and the scrollback isn't fully erased, is because of the order of the steps in the
AdaptDispatch::HardResetmethod. You need to reset the SGR state before clearing the display, otherwise it'll use the current background color for the fill color. And you need to clear the display before clearing the scrollback, otherwise the last page of content will be retained in the scrollback.I'd be happy to provide a PR with these fixes if nobody disagrees with my assessment.
The text was updated successfully, but these errors were encountered: