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 the "faint" graphic rendition attribute #6873

Merged
7 commits merged into from
Jul 13, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Jul 11, 2020

Summary of the Pull Request

This PR adds support for the SGR 2 escape sequence, which enables the
ANSI faint graphic rendition attribute. When a character is output
with this attribute set, it uses a dimmer version of the active
foreground color.

PR Checklist

Detailed Description of the Pull Request / Additional comments

There was already an ExtendedAttributes::Faint flag in the
TextAttribute class, but I needed to add SetFaint and IsFaint
methods to access that flag, and update the SetGraphicsRendition
methods of the two dispatchers to set the attribute on receipt of the
SGR 2 sequence. I also had to update the existing SGR 22 handler to
reset Faint in addition to Bold, since they share the same reset
sequence. For that reason, I thought it a good idea to change the name
of the SGR 22 enum to NotBoldOrFaint.

For the purpose of rendering, I've updated the
TextAttribute::CalculateRgbColors method to return a dimmer version of
the foreground color when the Faint attribute is set. This is simply
achieved by dividing each color component by two, which produces a
reasonable effect without being too complicated. Note that the Faint
effect is applied before Reverse Video, so if the output it reversed,
it's the background that will be faint.

The only other complication was the update of the Xterm256Engine in
the VT renderer. As mentioned above, Bold and Faint share the same
reset sequence, so to forward that state over conpty we have to go
through a slightly more complicated process than with other attributes.
We first check whether either attribute needs to be turned off to send
the reset sequence, and then check if the individual attributes need to
be turned on again.

Validation

I've extended the existing SGR unit tests to cover the new attribute in
the AdapterTest, the ScreenBufferTests, and the VtRendererTest,
and added a test to confirm the color calculations when Faint is set
in the TextAttributeTests.

I've also done a bunch of manual testing with all the different VT color
types and confirmed that our output is comparable to most other
terminals.

@j4james j4james marked this pull request as ready for review July 11, 2020 12:53
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

This is great.

I wonder if, if we ever do get perceptual grading going, there's value in making faint more faint (=harder to perceive) rather than just darker.

Given that darkening is comparable to other terminal emulators, it's more of a curiosity than a desire. 😄

Thanks for doing this.

@j4james
Copy link
Collaborator Author

j4james commented Jul 11, 2020

Btw, here are a couple of screenshots comparing how other terminals handle this. From left to right, top to bottom: XTerm, VTE (Gnome Terminal), Mintty, and then us. In each case I've got a color test pattern rendered with normal attributes first, and with a faint version below. I think XTerm and VTE are a bit brighter than us, but Mintty looks almost identical. The overall effect is about the same though.

image

@ghost ghost added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Jul 13, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Fantastic as always. Thanks!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 13, 2020
@ghost
Copy link

ghost commented Jul 13, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 7d677c5 into microsoft:master Jul 13, 2020
@ghost
Copy link

ghost commented Jul 22, 2020

🎉Windows Terminal Preview v1.2.2022.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for SGR 2 Faint
3 participants