Repository: microsoft/terminal (language: cpp)

Issue:
SGR 1 and SGR 8 don't work when adjusting lightness of indistinguishable text
### 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.

![image](https://user-images.githubusercontent.com/4181424/145468815-ef7ada85-8373-468d-bcd7-2da471911520.png)

### Actual Behavior

All of the text looks exactly the same.

![image](https://user-images.githubusercontent.com/4181424/145469002-4338741b-4214-40dc-bf38-08fa7d9c1dcf.png)

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.



Discussion / hints:
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.
(I'll hold this feature back from 1.12 stable)
> 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.
<del>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:</del>

<img width="585" alt="bright" src="https://user-images.githubusercontent.com/26824113/145493603-1d008f43-26a6-4ace-ba7a-6222af936eb8.png">

Ignore that, if I set my intense text style to be only 'Bright colors' then I can repro the issue. 
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.

![image](https://user-images.githubusercontent.com/4181424/145494742-c69b12d2-6893-4c00-a732-03cf8bc177ed.png)

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?

Fix patch:
diff --git a/src/cascadia/TerminalSettingsModel/MTSMSettings.h b/src/cascadia/TerminalSettingsModel/MTSMSettings.h
index e425bdfd877..02b0e788742 100644
--- a/src/cascadia/TerminalSettingsModel/MTSMSettings.h
+++ b/src/cascadia/TerminalSettingsModel/MTSMSettings.h
@@ -109,7 +109,7 @@ Author(s):
     X(hstring, ColorSchemeName, "colorScheme", L"Campbell")                                                                                                        \
     X(hstring, BackgroundImagePath, "backgroundImage")                                                                                                             \
     X(Model::IntenseStyle, IntenseTextStyle, "intenseTextStyle", Model::IntenseStyle::Bright)                                                                      \
-    X(bool, AdjustIndistinguishableColors, "adjustIndistinguishableColors", true)
+    X(bool, AdjustIndistinguishableColors, "adjustIndistinguishableColors", false)
 
 // Intentionally omitted Appearance settings:
 // * ForegroundKey, BackgroundKey, SelectionBackgroundKey, CursorColorKey: all optional colors
diff --git a/src/features.xml b/src/features.xml
index a8db865ddc7..00ca5a8bea0 100644
--- a/src/features.xml
+++ b/src/features.xml
@@ -79,10 +79,7 @@
     <feature>
         <name>Feature_AdjustIndistinguishableText</name>
         <description>If enabled, the foreground color will, when necessary, be automatically adjusted to make it more visible.</description>
-        <stage>AlwaysDisabled</stage>
-        <alwaysEnabledBrandingTokens>
-            <brandingToken>Dev</brandingToken>
-        </alwaysEnabledBrandingTokens>
+        <stage>AlwaysEnabled</stage>
     </feature>
     
     <feature>
diff --git a/src/renderer/base/RenderSettings.cpp b/src/renderer/base/RenderSettings.cpp
index fab1f7dc10e..1e3aaed19c4 100644
--- a/src/renderer/base/RenderSettings.cpp
+++ b/src/renderer/base/RenderSettings.cpp
@@ -13,6 +13,7 @@ using Microsoft::Console::Utils::InitializeColorTable;
 
 static constexpr size_t AdjustedFgIndex{ 16 };
 static constexpr size_t AdjustedBgIndex{ 17 };
+static constexpr size_t AdjustedBrightFgIndex{ 18 };
 
 RenderSettings::RenderSettings() noexcept
 {
@@ -77,17 +78,21 @@ void RenderSettings::ResetColorTable() noexcept
 //   color pair to the adjusted foreground for that color pair
 void RenderSettings::MakeAdjustedColorArray() noexcept
 {
-    // The color table has 16 colors, but the adjusted color table needs to be 18
-    // to include the default background and default foreground colors
-    std::array<COLORREF, 18> colorTableWithDefaults;
+    // The color table has 16 colors, but the adjusted color table needs to be 19
+    // to include the default background, default foreground and bright default foreground colors
+    std::array<COLORREF, 19> colorTableWithDefaults;
     std::copy_n(std::begin(_colorTable), 16, std::begin(colorTableWithDefaults));
     colorTableWithDefaults[AdjustedFgIndex] = GetColorAlias(ColorAlias::DefaultForeground);
     colorTableWithDefaults[AdjustedBgIndex] = GetColorAlias(ColorAlias::DefaultBackground);
 
-    for (auto fgIndex = 0; fgIndex < 18; ++fgIndex)
+    // We need to use TextColor to calculate the bright default fg
+    TextColor defaultFg;
+    colorTableWithDefaults[AdjustedBrightFgIndex] = defaultFg.GetColor(_colorTable, GetColorAliasIndex(ColorAlias::DefaultForeground), true);
+
+    for (auto fgIndex = 0; fgIndex < 19; ++fgIndex)
     {
         const auto fg = til::at(colorTableWithDefaults, fgIndex);
-        for (auto bgIndex = 0; bgIndex < 18; ++bgIndex)
+        for (auto bgIndex = 0; bgIndex < 19; ++bgIndex)
         {
             if (fgIndex == bgIndex)
             {
@@ -196,22 +201,30 @@ std::pair<COLORREF, COLORREF> RenderSettings::GetAttributeColors(const TextAttri
     if (Feature_AdjustIndistinguishableText::IsEnabled() &&
         GetRenderMode(Mode::DistinguishableColors) &&
         !dimFg &&
+        !attr.IsInvisible() &&
         (fgTextColor.IsDefault() || fgTextColor.IsLegacy()) &&
         (bgTextColor.IsDefault() || bgTextColor.IsLegacy()))
     {
         const auto bgIndex = bgTextColor.IsDefault() ? AdjustedBgIndex : bgTextColor.GetIndex();
         auto fgIndex = fgTextColor.IsDefault() ? AdjustedFgIndex : fgTextColor.GetIndex();
 
-        if (fgTextColor.IsIndex16() && (fgIndex < 8) && brightenFg)
+        if (brightenFg)
         {
             // There is a special case for intense here - we need to get the bright version of the foreground color
-            fgIndex += 8;
+            if (fgTextColor.IsIndex16() && (fgIndex < 8))
+            {
+                fgIndex += 8;
+            }
+            else if (fgTextColor.IsDefault())
+            {
+                fgIndex = AdjustedBrightFgIndex;
+            }
         }
 
         if (swapFgAndBg)
         {
             const auto fg = _adjustedForegroundColors[fgIndex][bgIndex];
-            const auto bg = fgTextColor.GetColor(_colorTable, defaultFgIndex);
+            const auto bg = fgTextColor.GetColor(_colorTable, defaultFgIndex, brightenFg);
             return { fg, bg };
         }
         else
diff --git a/src/renderer/inc/RenderSettings.hpp b/src/renderer/inc/RenderSettings.hpp
index 110075b2969..a201fad263d 100644
--- a/src/renderer/inc/RenderSettings.hpp
+++ b/src/renderer/inc/RenderSettings.hpp
@@ -47,7 +47,7 @@ namespace Microsoft::Console::Render
         til::enumset<Mode> _renderMode{ Mode::BlinkAllowed, Mode::IntenseIsBright };
         std::array<COLORREF, TextColor::TABLE_SIZE> _colorTable;
         std::array<size_t, static_cast<size_t>(ColorAlias::ENUM_COUNT)> _colorAliasIndices;
-        std::array<std::array<COLORREF, 18>, 18> _adjustedForegroundColors;
+        std::array<std::array<COLORREF, 19>, 19> _adjustedForegroundColors;
         size_t _blinkCycle = 0;
         mutable bool _blinkIsInUse = false;
         bool _blinkShouldBeFaint = false;
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index 6b2a7488ad7..3404f3b3c25 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -2221,6 +2221,11 @@ bool AdaptDispatch::SetClipboard(const std::wstring_view content)
 bool AdaptDispatch::SetColorTableEntry(const size_t tableIndex, const DWORD dwColor)
 {
     _renderSettings.SetColorTableEntry(tableIndex, dwColor);
+    if (_renderSettings.GetRenderMode(RenderSettings::Mode::DistinguishableColors))
+    {
+        // Re-calculate the adjusted colors now that one of the entries has been changed
+        _renderSettings.MakeAdjustedColorArray();
+    }
 
     // If we're a conpty, always return false, so that we send the updated color
     //      value to the terminal. Still handle the sequence so apps that use
@@ -2286,6 +2291,11 @@ bool AdaptDispatch::AssignColor(const DispatchTypes::ColorItem item, const VTInt
     case DispatchTypes::ColorItem::NormalText:
         _renderSettings.SetColorAliasIndex(ColorAlias::DefaultForeground, fgIndex);
         _renderSettings.SetColorAliasIndex(ColorAlias::DefaultBackground, bgIndex);
+        if (_renderSettings.GetRenderMode(RenderSettings::Mode::DistinguishableColors))
+        {
+            // Re-calculate the adjusted colors now that these aliases have been changed
+            _renderSettings.MakeAdjustedColorArray();
+        }
         break;
     case DispatchTypes::ColorItem::WindowFrame:
         _renderSettings.SetColorAliasIndex(ColorAlias::FrameForeground, fgIndex);


Test patch: