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 "doubly underlined" graphic rendition attribute #7223

Merged
4 commits merged into from
Aug 10, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Aug 8, 2020

This PR adds support for the ANSI doubly underlined graphic rendition
attribute, which is enabled by the SGR 21 escape sequence.

There was already an ExtendedAttributes::DoublyUnderlined flag in the
TextAttribute class, but I needed to add SetDoublyUnderlined and
IsDoublyUnderlined methods to access that flag, and update the
SetGraphicsRendition methods of the two dispatchers to set the
attribute on receipt of the SGR 21 sequence. I also had to update the
existing SGR 24 handler to reset DoublyUnderlined in addition to
Underlined, since they share the same reset sequence.

For the rendering, I've added a new grid line type, which essentially
just draws an additional line with the same thickness as the regular
underline, but slightly below it - I found a gap of around 0.05 "em"
between the lines looked best. If there isn't enough space in the cell
for that gap, the second line will be clamped to overlap the first, so
you then just get a thicker line. If there isn't even enough space below
for a thicker line, we move the offset above the first line, but just
enough to make it thicker.

The only other complication was the update of the Xterm256Engine in
the VT renderer. As mentioned above, the two underline attributes share
the same reset sequence, so to forward that state over conpty we require
a slightly more complicated process than with most other attributes
(similar to Bold and Faint). We first check whether either underline
attribute needs to be turned off to send the reset sequence, and then
check individually if each of them needs to be turned back on again.

Validation Steps Performed

For testing, I've extended the existing attribute tests in
AdapterTest, VTRendererTest, and ScreenBufferTests, to make sure
we're covering both the Underlined and DoublyUnderlined attributes.

I've also manually tested the SGR 21 sequence in conhost and Windows
Terminal, with a variety of fonts and font sizes, to make sure the
rendering was reasonably distinguishable from a single underline.

Closes #2916

@ghost ghost added 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 Aug 8, 2020
@j4james j4james marked this pull request as ready for review August 8, 2020 11:46
@j4james
Copy link
Collaborator Author

j4james commented Aug 8, 2020

To get an idea of what this attribute looks like, I've included a screenshot below showing both single and double underlines rendered with a variety of 12pt fonts. On the left is the conhost GDI renderer, and the right is the WT DirectX renderer.

Note how some fonts only have enough space for a thicker line, and not two distinct lines, but I think that's OK (XTerm always renders the attribute this way). Also note that this is more common in the GDI renderer because it uses a smaller line height.

underlines

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.

Lovely as always. Thank you!

Comment on lines +2302 to +2308

// But if the resulting gap isn't big enough even to register as a thicker
// line, it's better to place the second line slightly above the first.
if (lineMetrics.underlineOffset2 < lineMetrics.underlineOffset + lineMetrics.gridlineWidth)
{
lineMetrics.underlineOffset2 = lineMetrics.underlineOffset - lineMetrics.gridlineWidth;
}
Copy link
Member

Choose a reason for hiding this comment

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

I love how thoughtful this is. Thank you.

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

ghost commented Aug 10, 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 e7a1a67 into microsoft:master Aug 10, 2020
@j4james j4james deleted the feature-sgr-doubleunderline branch August 15, 2020 15:08
@ghost
Copy link

ghost commented Aug 26, 2020

🎉Windows Terminal Preview v1.3.2382.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-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 parsing Underlined and Doubly Underlined to the parser
3 participants