Add support for underline style and color in VT

[tusharsnx]

Add support for underline style and color in VT.

Underline color sequence SGR 58 (unlike SGR 38, SGR 48) only works with sub parameters, eg. \e[58:5:<n>m or \e[58:2::<r>:<g>:<b>m will work, but something like \e[58;5;<n>m won't work. This is a requirement for the implementation to avoid problems with VT clients that don't support sub parameters.

Detailed Description

• Added underlineColor to TextAttribute, and UnderlineStyle into CharacterAttributes.
• Added two new entries in GraphicOptions namely, UnderlineColor (58) and UnderlineColorDefault (59).
• SGR 58 renders a sequence with sub parameters in the VT renderer.
• SGR 4:x renders a sequence with sub parameters in the VT renderer, except for single, double, and no-underline, which still use backward-compatible SGR 4, SGR 21 and SGR 24.
• XtermEngine will send \e[4m without any styling information. This means all underline style (except NoUnderline) will be rendered as single underline.

Reference issues

• Add support for Kitty/VTE extended underlines (4:x) [SGR] #7228

PR Checklist

• Use pre-calculated maps for efficient mapping between CharacterAttributes and UnderlineStyle. (Not required)
• update DECRARA, DECCARA to respect underline color and style.
• update DECRQSS to send underline color and style in the query response.
• update DECRQPSR/DECRSPS/DECCIR
• Tests added

[zadjii-msft]

quick question, does this supersede #15599?

[DHowett]

(10/11 reviewed, but it's getting late here!)

[tusharsnx]

quick question, does this supersede #15599?

Yeah, that was from pre subparam era, so. I'll close that in a bit.

[tusharsnx]

I'd request to keep the final ✅ until pr checklist is over. I usually update them as I find more work need to be done.

I guess I should mark PRs as draft until it's ready, but I really like when I get reviews (on where I can improve) as I'm working on it ✌️. I'm not sure if "Draft" conveys that properly, instead it seems to say "don't look here! I'm still working on it"

[j4james]

Instead of storing the underline style in a separate attribute, which ends up requiring an additional two bytes, I would much prefer if we could just add it to the existing CharacterAttributes enum.

I think all we really need is one more bit. Maybe move the Faint to 0x20, and then reserve 0x40, 0x80, and 0x100 for the underline styles. Something like this:

SingleUnderline = 0x40,
DoubleUnderline = 0x80,
CurlyUnderline = 0xC0,
DottedUnderline = 0x100,
DashedUnderline = 0x140,
AnyUnderline = 0x1C0,
NoUnderline = 0,

Then you can access those attributes with something like this (pseudo code):

// Testing for a particular style from the list above.
bool TextAttribute::IsUnderlined(int style)
{
    return (_attrs & AnyUnderline) == style;
}

// Testing for any underline style if we want a separate method for that.
bool TextAttribute::IsUnderlined()
{
    return IsUnderlined(NoUnderline);
}

// Setting a particular style from the list above.
void TextAttribute::SetUnderlined(int style)
{
    _attrs = (_attrs & ~AnyUnderline) | style;
}

// Remove all underlines if we want a separate method for that.
void TextAttribute::ResetUnderlined()
{
    SetUnderlined(NoUnderline);
}

There are probably some WI_ macros you can use that'll make the implementation cleaner - I just wanted to demonstrate the general idea.

[tusharsnx]

Excellent! I think I can implement this, with little tweaks maybe.

Wondering if it's safe to assume only one of the underline style can be active at any point, including doublyUnderlined even if it's done using SGR 21. I guess we don't need to track doubly underline separately, do we?

[j4james]

I guess we don't need to track doubly underline separately, do we?

I'm glad you asked this, because we do track them separately at the moment, but I don't think we need to retain that behavior.

At the time I added double underline support there was no consensus in how this was handled amongst other terminals, so I just followed what XTerm was doing. But in retrospect I think that was probably the wrong decision, and now that we're adding more underline styles it makes even less sense. So yeah, I'd say we don't need to track double underline separately.

[tusharsnx]

I took a look at the discussion over at #2916 (comment) where you mentioned:

However in XTerm, underline and double-underline are actually separate attributes, and double-underline takes precedence when both are set, regardless of the order they were applied.

I assume this needs to be taken care of?

[tusharsnx]

This shouldn't apply to SGR 4:x.

[j4james]

However in XTerm, underline and double-underline are actually separate attributes, and double-underline takes precedence when both are set, regardless of the order they were applied.

I assume this needs to be taken care of?

No. That's what I was trying to explain above. It seemed like the right thing to do at the time, but in retrospect, copying XTerm was the wrong decision. I don't have a record of my testing from back then, but having just checked now, I can't find anyone that still matches XTerm other than us. And in our case, we only really match XTerm in the DX renderer - the Atlas renderer draws the single underline on top of the double underline when both are applied.

There is no reason for us to retain the current behavior. This is such a ridiculous edge case anyway that nobody is going to be expecting it to work in a particular way.

[tusharsnx]

TestComposedConstProperties is failing and I've no clue how it works and how to fix this :/

And TBH, it always failed in debug build the last time I checked, but never failed in release build before.

[DHowett]

Mike's working on that one :) Thanks!

[tusharsnx]

If XtermEngine should get the underline style and color support?

[tusharsnx]

Guessing we don't need this one any more, since they're as efficient as can be (just some constexpr arithmetic), right?

Yeah, the generated asm looks good! 😄

How's the rest of the checklist? Does it need any updates?

Updated the list. They are all marked as Complete.

[tusharsnx]

I've reverted the changes in DECRARA.

Let me summarise what we've discussed so far:

There are two types of applications that would be interested in DECRARA:

1. Application that just want to reverse an attribute. They're interested in toggling the state of an attribute. This kind of application depends on the fact the attributes will switch on/off based on the current state of the attribute.
2. Applications that are interested in bringing the original state back after doing two DECRARAs with exactly the same sequence in a row. The two DECRARA ops could be applied at any point in time. After the ops, the current attribute must represent the original attribute we started from.

In an ideal case, we want our implementation to satisfy both, and this is already true for all CharacterAttributes, except underline style because it has more than two states.

As @j4james mentioned, (1) can easily be achieved using DECCARA (application may need to build the sequence in a way that would 'toggle' the state and not simply 'change' it):

If you wanted to convert any underline to no-underline without needing to get the original back again, you could just use DECCARA.

OTH, there's no way for an application to achieve (2) unless DECRARA is strictly reversible.

Given the current implementation and keeping it simple to work with, we have to choose between the two usecases with DECRARA. So, we are going to prefer the (2) for our implementation. There is a problem with this approach though. The 'intermediate' state of the underline style between the two DECRARA ops can't be treated as anything useful. It may or may not be as expected. If the current style is the same style you're reversing, you get the desired result. But, if the two differs, expect an undefined behavior 🙂. This is okay for now, because it at least works for some cases, and that's what we're aiming for initially.

@j4james Can you have a look on the new changes and see if it's behaving in the way we discussed?

[j4james]

I've been playing around with a build of this PR, and it seems to work reasonably well. The only weird thing I've noticed is when you XOR say style 2 with style 4 and you get style 6. Since that's not a supported style, we don't currently do anything with it, which wouldn't be a big deal if it was consistent, but it actually leaves the conpty client in a random state.

So I think it would probably be best to make the default case the same as a single underline, like a placeholder for future underline styles we may assign to those values. And make sure that both the Renderer::s_GetGridlines and VtEngine::_SetUnderlineExtended are handling them the same way, so the results are the same in both conhost and WT.

[tusharsnx]

Playing around after some more changes in the renderer (which I'll push in a separate pr):

(thanks @lhecker for the awesome bc.exe)

Contents of 'New Text Document.txt'

\e[4m					�[4mDefault Underline Color�[m
\e[4;38:2::255::m			�[4;38:2::255::mRed Foreground with Default Underline Color�[m
\e[4;38:2::255::;58:2:::255:m		�[4;38:2::255::;58:2:::255:mRed Foreground with Green Underline Color�[m
\e[4;58:2::::255m			�[4;58:2::::255mBlue Underline Color�[m
\e[4;58:5:144m				�[4;58:5:144mIndex256 (index = 144) Underline Color�[m
\e[4m;\e[58:5:112m\e[59\e[4:0m		�[4mOnly Underline---�[58:5:112mNow with Some Color---�[59mColor Removed---�[4:0mUnderline Removed�[m
\e[4:1m\e[4:2m\[4m\e[24m		�[4:1m Single Underline---�[4:2mDouble Underline---�[4mBack to Single---�[24mNo Underline�[m
\e[4;58:2::::255m			�[4;58:2::::255mBlue Underline Color�[m
\e[4;33m				�[4;33mDefault Underline Color follows Foreground�[m
\e[4;33;1m				�[4;33;1mDefault Underline Color follows Foreground (Intense)�[m
\e[4;36;2m				�[4;36;2mDefault Underline Color follows Foreground (Faint)�[m
\e[4;36;58:5:122;2m			�[4;36;58:5:122;2mUnderline Color (when set) doesn't follow the Faint�[m
\e[4;36;7m				�[4;36;7mUnderline Color follows the Reverse Video if it's Default�[m
\e[4;36;58:5:122;7m			�[4;36;58:5:122;7mUnderline Color (when set) doesn't follow the Reverse Video�[m
\e[4;36;58:5:122;8m			�[4;36;58:5:122;8mUnderline Color follows the Invisible attribute�[m (that's invisible)

(� is just "\e")

[tusharsnx]

conhost too:

[lhecker]

@tusharsnx Thank you for persisting through this! The PR is amazing. If you have any questions whatsoever regarding the rendering code, please feel free to direct them to me. 🙂

[DHowett]

20/23 files; some of them re-reviews.

This is really great work, Tushar! Thanks so much for working on it. I've got one question, and a few more files to get through, but I think this is totally righteous stuff 😄

[tusharsnx]

So I think it would probably be best to make the default case the same as a single underline, like a placeholder for future underline styles we may assign to those values. And make sure that both the Renderer::s_GetGridlines and VtEngine::_SetUnderlineExtended are handling them the same way, so the results are the same in both conhost and WT.

@j4james I've made the changes 👍

[j4james]

@tusharsnx I haven't really reviewed the code, but I've tested a build of the PR (with some rendering hacked in so I could see what it was doing), and it looks good to me. Thank you for all the work you've put into this.

[tusharsnx]

I haven't really reviewed the code, but I've tested a build of the PR (with some rendering hacked in so I could see what it was doing)

I can understand that.

I've prepared these examples with WezTerm as the frontend:

1. Basic example:

2. DECCARA

3. DECRARA (when things work fine)

4. DECRARA (when things go wacky)

[tusharsnx]

Neovim:

[DHowett]

Thanks for doing all of this, and for complying with ours (and James') requests. I'm so stoked to finally land this, but I'll give you a couple minutes (or hours, depending on whether you're around!) to chat about open comments before I smash Merge.

[DHowett]

Congrats 😁

[AlanJhonatan]

Thanks ! 🫶