Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Erase Color Mode (DECECM) #15469

Merged
merged 6 commits into from Jun 9, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented May 29, 2023

The Erase Color Mode determines what attributes are written to the
buffer when erasing content, or when new content is scrolled onto the
screen. When the mode is reset (which is the default), we erase with the
active colors, but with rendition attributes cleared. When the mode is
set, we erase with the default attributes, i.e. with neither color nor
rendition attributes applied.

This could be used to address the problem described in issue #10556.

Most of the affected operations are handled within the AdaptDispatch
class, so I've simply updated them all to use a new helper method which
returns the appropriate erase attributes for the active mode.

However, there were a couple of operations that are handled elsewhere,
and which now require the erase attributes to be passed to them as a
parameter.

  • The TextBuffer::IncrementCircularBuffer method, which is used to
    recycle the topmost row when scrolling past the bottom of the buffer.

  • The TextBuffer::SetCurrentLineRendition method, which has to clear
    the second half of the line when switching to a double width rendition.

  • The ITerminalApi::UseAlternateScreenBuffer method, which has to
    clear the screen when switching to the alternate buffer.

Then there is also a Clear Buffer action in Windows Terminal, which is
ultimately handled by the SCREEN_INFORMATION::ClearBuffer method in
ConHost. That class has no access to the erase color mode, so has no way
of knowing which attributes to use. So I've now rewritten it to use the
AdaptDispatch::EraseInDisplay method to handle the erasing.

Validation Steps Performed

I wrote a little test script that exercises the operations affected by
DECECM, which @al20878 kindly tested for us on a real DEC VT525, and
provided screenshots of the output. I've manually confirmed that our
implementation exactly matches those results.

I've also added a unit test that runs through the same set of operations
and verified that each of them is using the appropriate attributes when
DECECM is enabled and enabled.

Closes #14983

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-VT Virtual Terminal sequence support labels May 29, 2023
@j4james
Copy link
Collaborator Author

j4james commented May 29, 2023

Some screenshots for comparison with the VT525 output in #14983 (comment).

image

@j4james j4james marked this pull request as ready for review May 29, 2023 02:18
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seriously reviewed the whole thing and not a single comment haha. Thanks for doing this!

src/host/outputStream.hpp Show resolved Hide resolved
@lhecker
Copy link
Member

lhecker commented Jun 8, 2023

BTW I was reading what EraseInDisplay does and it'd be nice if we could replace the _FillRect calls that operate on entire rows with ones that call ROW::Reset() instead. With the upcoming PR #15501 this would be much much faster. With #15524 it'd be additionally curious if we could instead just decommit the entire cleared memory area, for instance when we encounter a sequence to clear the scrollback. (Just thinking out loud.)

@j4james
Copy link
Collaborator Author

j4james commented Jun 8, 2023

BTW I was reading what EraseInDisplay does and it'd be nice if we could replace the _FillRect calls that operate on entire rows with ones that call ROW::Reset() instead.

Yeah, that sounds like a good idea. _EraseScrollback in particular should be a big win. But note that it doesn't clear the full height of the buffer - you still need to retain one viewport worth of content - so we couldn't just nuke the entire thing.

@DHowett DHowett enabled auto-merge (squash) June 8, 2023 23:51
@DHowett
Copy link
Member

DHowett commented Jun 8, 2023

I've kicked the test job in CI :)

@DHowett DHowett merged commit 7a3bf70 into microsoft:main Jun 9, 2023
11 of 12 checks passed
@j4james j4james deleted the feature-dececm branch January 2, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Erase Color Mode (DECECM)
4 participants