SGR 1 and SGR 8 don't work when adjusting lightness of indistinguishable text

[j4james]

Windows Terminal version

1.12.2931.0

Windows build number

10.0.19041.1348

Other Software

No response

Steps to reproduce

1. In the Setting UI, go to the Profiles Defaults page and select the Appearance tab.
2. Make sure the option Automaticaly adjust lightness of indistinguishable text is enabled.
3. Open a bash shell.
4. Execute the following command: printf "\e[m DEFAULT \e[1m BRIGHT \e[8m CONCEALED \e[m\n"

Expected Behavior

I'd expect the text with the SGR 1 attribute to be brighter, and the text with SGR 8 attribute to be invisible.

Actual Behavior

All of the text looks exactly the same.

This wouldn't be that big a deal if it was an option that users specifically opted in to, but I realise now that it's actually on by default. Was that really intentional? Even ignoring these problems, I would have thought the performance overhead alone was enough of a reason to make it opt-in.

[DHowett]

Augh, I knew that cribbing the color selection code would blow us up somehow. Even with this option on, we shouldn't be nudging those colors. Concealed was an oversight, even.

performance

We ended up going with an option that pre-cooks nudged colors for pairs in the lower 16x16 color grid, instead of doing it per color pair on render (which I think you'd suggested -- so we didn't nudge direct RGB colors or anything from the xterm 256 palette above the base sixteen specified in the color scheme.) That significantly reduced the performance impact.

@PankajBhojwani, would you mind taking a look at this? Bold=bright + nudge + \e[39;1m should definitely result in bright foreground. It looks like we're mishandling "bright default" in a pretty significant way.

[DHowett]

(I'll hold this feature back from 1.12 stable)

[j4james]

We ended up going with an option that pre-cooks nudged colors for pairs in the lower 16x16 color grid, instead of doing it per color pair on render (which I think you'd suggested -- so we didn't nudge direct RGB colors or anything from the xterm 256 palette above the base sixteen specified in the color scheme.) That significantly reduced the performance impact.

Yeah, I've seen the code - I've actually been trying to optimise it to some extent as part of another PR I was working on (which is how I discovered these issues). I haven't actually tested the hit on performance, but it just seems like there's still a lot code being executed there. I had actually hoped we could implement this with zero run-time overhead, but maybe that was wishful thinking.

I should also mention that there are probably other scenarios in which this option can break things - SGR 1 and SGR 8 are just the two most obvious examples. I'll follow up here with more examples if/when I find them.

[PankajBhojwani]

So I can't seem to completely repro this - I can repro the issue with the concealed text showing up when it shouldn't (looking into that right now), but the bright text seems to be working as it should for me:

Ignore that, if I set my intense text style to be only 'Bright colors' then I can repro the issue.

[j4james]

Here's another example of the kind of problem you can get with the color adjustment enabled.

1. Set your color scheme in the settings to Solarized Light.
2. Execute the following command in bash: printf "\e[m DEFAULT \e[7m REVERSED \e[m\n"
3. Using escape sequences, change the palette to Campbell.

Instead of getting the Campbell color scheme, you get a mix of the two, I think probably with the foreground colors still coming from the Solarized Light scheme.

If we could make the color adjustment algorithm only apply to the initial color scheme, then that would avoid problems like this (and also improve the performance).

Just spitballing here, but consider an algorithm that can calculate some kind of brightness value for each color in the color scheme, and sort them on a linear scale. If there is a minimum brightness difference that would make all colors reasonably distinguishable, it shouldn't be too hard to spread those values out until they're all visibly distinguishable. You then regenerate the color scheme with the adjusted brightness levels and you're done - no runtime modification required. Does that not seem feasible?