-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Conversation
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. |
There was a problem hiding this 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!
|
||
// 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; | ||
} |
There was a problem hiding this comment.
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.
Hello @DHowett! Because this pull request has the 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 (
|
🎉 Handy links: |
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 theTextAttribute
class, but I needed to addSetDoublyUnderlined
andIsDoublyUnderlined
methods to access that flag, and update theSetGraphicsRendition
methods of the two dispatchers to set theattribute on receipt of the
SGR 21
sequence. I also had to update theexisting
SGR 24
handler to reset DoublyUnderlined in addition toUnderlined, 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
inthe 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
, andScreenBufferTests
, to make surewe're covering both the Underlined and DoublyUnderlined attributes.
I've also manually tested the
SGR 21
sequence in conhost and WindowsTerminal, with a variety of fonts and font sizes, to make sure the
rendering was reasonably distinguishable from a single underline.
Closes #2916