Make the RIS command clear the display and scrollback correctly #2367
Conversation
…letely overlaps the original viewport.
…ound color is used, and clear the display before the scrollback, to prevent the last content being retained.
…rollback buffer correctly.
|
Since @miniksa's the original author of the "SomeViewports" machinery, I've tagged him for review. He's out of the office currently. :) |
|
I believe that the only other place Viewport::Subtract is used will be okay with receiving 0 viewports instead of 1 0x0 viewport, because it would have fallen into |
|
What I'm saying is that the |
|
Whoops, chalk that up to my poor reading skills. Thanks! |
There was a problem hiding this comment.
This seems reasonable to me, and if the rest of the tests pass, then I'm pretty sure the change to Viewport::Subtract is probably fine, though @miniksa would be the one to know for sure.
|
Sorry, I clicked "re-request review" because it was next to Michael's name, but then it may have done it for Mike as well. thanks github |
ghost
requested review from
There was a problem hiding this comment.
I'm fine with the Subtract change. You're right that was only really for the context of scroll operations and I follow your reasoning on why it's OK to change.
Thank you. This is a great change and you provided excellent research and reasoning behind making it. Makes my life as a reviewer easy.
EDIT: I approved with comments because they're minor. Fix them or don't, it's not a big deal. Thanks.
…der, and a more useful failure log in the screen buffer test.
|
It looks like the x86 unit tests have timed out again. The build bot clearly hates me. |
|
Taking the N-1 results. 😄 Thanks for the contribution! |
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)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
This went out for conhost in insider build 19002! Thanks 😄 |
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
Detailed Description of the Pull Request / Additional comments
The initial failure is caused by the
SCREEN_INFORMATION::WriteRectmethod throwing an exception when passed an empty viewport. And the reason it's passed an empty viewport is because that's what theViewport::Subtractmethod returns when the result of the subtraction is nothing. The PR fixes the problem by making theViewport::Subtractmethod 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
Subtractmethod never returns empty viewports under any other circumstances. And the only place the method seems to be used is in theScrollRegionimplementation, 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::HardResetmethod. The call toSoftResetneeded 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
SoftResethappening 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.