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.

Sign up for GitHub

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

Jump to bottom

Add support for SGR 2 Faint #6703

Closed
willmcgugan opened this issue Jun 27, 2020 · 9 comments · Fixed by #6873
Closed

Add support for SGR 2 Faint #6703

willmcgugan opened this issue Jun 27, 2020 · 9 comments · Fixed by #6873
Assignees
@DHowett
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.x

Comments

@willmcgugan
Copy link

willmcgugan commented Jun 27, 2020 •
edited by DHowett
Loading

We do not support Faint text.

Original content

Description of the new feature/enhancement

Windows Terminal doesn't appear to support the dim ansi escape code (SGR 2) or italic (SGR 3). Adding support for these would I think bring WT up to feature parity with most OSX and Linux terminals.

There are other SGR escape codes that would be nice to have, but aren't as common or as useful as dim and italic.

Proposed technical implementation details (optional)

Dim strikes me as an easy one to achieve. I imagine italic would be a little trickier.
@willmcgugan willmcgugan added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 27, 2020
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 27, 2020
@DHowett
Copy link
Member

DHowett commented Jun 27, 2020

Thanks!

This is a curious issue. We're tracking all manner of different versions of "terminal doesn't render specific attributes":

I just started tackling #109, despite it being divisive, and I think the infrastructure we need to build to get it off the ground (a "base + variant" system for fonts) is going to be required for this.

I'm going to close this shortly but rework all of the above issues into an amalgamated "we should support different attributes, from parsing through to rendering" issue. Does that sound good?

@willmcgugan
Copy link
Author

Sounds good to me 👍

@csdvrx csdvrx mentioned this issue Jul 3, 2020
Open
@csdvrx
Copy link

csdvrx commented Jul 3, 2020

I'm going to close this shortly but rework all of the above issues into an amalgamated "we should support different attributes, from parsing through to rendering" issue. Does that sound good?

I added #6779 which provides a test script and an expected output.

It links another issue: there is no font switching, which would be useful for mixing different fonts.

@j4james
Copy link
Collaborator

j4james commented Jul 9, 2020 •
edited
Loading

@DHowett I've just realised this has been assigned to you, and wanted to check exactly which parts you're working on, or planning to work on. Reason being I was about to start on the faint attribute, as a follow up to the color refactoring in PR #6853, but if you already have plans for that, I can leave it to you.

Edit: I should add that my intention was to treat the faint attribute as entirely separate from bold/bright - i.e. both can be applied at the same time. My reasoning for this is explained in issue #2916 (comment).

@DHowett
Copy link
Member

DHowett commented Jul 9, 2020

@j4james it’s assigned to me for administrative reasons; I’m going to create a scenario for the various attributes impacting the display of text, and then close/rework some of the ones we already have open to fit under it. If you want to do work in this area, you’re welcome to 😄 I just may change which issue number your PR eventually closes

@j4james j4james mentioned this issue Jul 11, 2020
Merged
6 tasks
@DHowett DHowett changed the title Support for italic and faint Add support for SGR 4 Faint Jul 13, 2020
@DHowett DHowett changed the title Add support for SGR 4 Faint Add support for SGR 2 Faint Jul 13, 2020
@DHowett
Copy link
Member

DHowett commented Jul 13, 2020

I've repurposed this to track Faint, and crossed out mention of Faint in #2916. Your pull request can unabashedly close this issue and drop mention of 2916.

@DHowett
Copy link
Member

DHowett commented Jul 13, 2020

I've filed #6879 to track "all" attributes.

@DHowett DHowett added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 13, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 13, 2020
@DHowett DHowett added this to the Terminal v1.x milestone Jul 13, 2020
@ghost ghost added the In-PR This issue has a related PR label Jul 13, 2020
@ghost ghost closed this as completed in #6873 Jul 13, 2020
@ghost ghost removed the In-PR This issue has a related PR label Jul 13, 2020
ghost pushed a commit that referenced this issue Jul 13, 2020
@j4james
Add support for the "faint" graphic rendition attribute (#6873)
7d677c5 
## 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
* [x] Closes #6703
* [x] CLA signed. 
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already. Issue number where discussion took place: #6703

## 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.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jul 13, 2020
@DHowett DHowett mentioned this issue Jul 16, 2020
Open
1 task
@ghost
Copy link

ghost commented Jul 22, 2020

🎉This issue was addressed in #6873, which has now been successfully released as Windows Terminal Preview v1.2.2022.0.:tada:

Handy links:

@DHowett
Copy link
Member

DHowett commented Aug 21, 2020

🎉 As of Windows Insider build 20197, this is also supported in the traditional console.

@zadjii-msft zadjii-msft mentioned this issue Mar 26, 2021
Open
1 task
@zadjii-msft zadjii-msft mentioned this issue Aug 23, 2022
Closed
@picatz picatz mentioned this issue Aug 29, 2022
Merged
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DHowett DHowett

Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.x
Development

Successfully merging a pull request may close this issue.

Add support for the "faint" graphic rendition attribute j4james/terminal
4 participants
@DHowett @willmcgugan @j4james @csdvrx