Improve the propagation of color attributes over ConPTY (#6506)

This PR reimplements the VT rendering engines to do a better job of
preserving the original color types when propagating attributes over
ConPTY. For the 16-color renderers it provides better support for
default colors and improves the efficiency of the color narrowing
conversions. It also fixes problems with the ordering of character
renditions that could result in attributes being dropped.

Originally the base renderer would calculate the RGB color values and
legacy/extended attributes up front, passing that data on to the active
engine's `UpdateDrawingBrushes` method. With this new implementation,
the renderer now just passes through the original `TextAttribute` along
with an `IRenderData` interface, and leaves it to the engines to extract
the information they need.

The GDI and DirectX engines now have to lookup the RGB colors themselves
(via simple `IRenderData` calls), but have no need for the other
attributes. The VT engines extract the information that they need from
the `TextAttribute`, instead of having to reverse engineer it from
`COLORREF`s.

The process for the 256-color Xterm engine starts with a check for
default colors. If both foreground and background are default, it
outputs a SGR 0 reset, and clears the `_lastTextAttribute` completely to
make sure any reset state is reapplied. With that out the way, the
foreground and background are updated (if changed) in one of 4 ways.
They can either be a default value (SGR 39 and 49), a 16-color index
(using ANSI or AIX sequences), a 256-color index, or a 24-bit RGB value
(both using SGR 38 and 48 sequences).

Then once the colors are accounted for, there is a separate step that
handles the character rendition attributes (bold, italics, underline,
etc.) This step must come _after_ the color sequences, in case a SGR
reset is required, which would otherwise have cleared any character
rendition attributes if it came last (which is what happened in the
original implementation).

The process for the 16-color engines is a little different. The target
client in this case (Windows telnet) is incapable of setting default
colors individually, so we need to output an SGR 0 reset if _either_
color has changed to default. With that out the way, we use the
`TextColor::GetLegacyIndex` method to obtain an approximate 16-color
index for each color, and apply the bold attribute by brightening the
foreground index (setting bit 8) if the color type permits that.

However, since Windows telnet only supports the 8 basic ANSI colors, the
best we can do for bright colors is to output an SGR 1 attribute to get
a bright foreground. There is nothing we can do about a bright
background, so after that we just have to drop the high bit from the
colors. If the resulting index values have changed from what they were
before, we then output ANSI 8-color SGR sequences to update them.

As with the 256-color engine, there is also a final step to handle the
character rendition attributes. But in this case, the only supported
attributes are underline and reversed video.

Since the VT engines no longer depend on the active color table and
default color values, there was quite a lot of code that could now be
removed. This included the `IDefaultColorProvider` interface and
implementations, the `Find(Nearest)TableIndex` functions, and also the
associated HLS conversion and difference calculations.

VALIDATION

Other than simple API parameter changes, the majority of updates
required in the unit tests were to correct assumptions about the way the
colors should be rendered, which were the source of the narrowing bugs
this PR was trying to fix. Like passing white on black to the
`UpdateDrawingBrushes` API, and expecting it to output the default `SGR
0` sequence, or passing an RGB color and expecting an indexed SGR
sequence.

In addition to that, I've added some VT renderer tests to make sure the
rendition attributes (bold, underline, etc) are correctly retained when
a default color update causes an `SGR 0` sequence to be generated (the
source of bug #3076). And I've extended the VT renderer color tests
(both 256-color and 16-color) to make sure we're covering all of the
different color types (default, RGB, and both forms of indexed colors).

I've also tried to manually verify that all of the test cases in the
linked bug reports (and their associated duplicates) are now fixed when
this PR is applied.

Closes #2661
Closes #3076
Closes #3717
Closes #5384
Closes #5864

This is only a partial fix for #293, but I suspect the remaining cases
are unfixable.

Author: j4james

src/buffer/out/TextAttribute.cpp

@@ -31,7 +31,7 @@ WORD TextAttribute::GetLegacyAttributes() const noexcept
     const BYTE fgIndex = _foreground.GetLegacyIndex(s_legacyDefaultForeground);
     const BYTE bgIndex = _background.GetLegacyIndex(s_legacyDefaultBackground);
     const WORD metaAttrs = _wAttrLegacy & META_ATTRS;
-    const bool brighten = _foreground.IsIndex16() && IsBold();
+    const bool brighten = IsBold() && _foreground.CanBeBrightened();
     return fgIndex | (bgIndex << 4) | metaAttrs | (brighten ? FOREGROUND_INTENSITY : 0);
 }
 
@@ -90,6 +90,26 @@ COLORREF TextAttribute::_GetRgbBackground(std::basic_string_view<COLORREF> color
     return _background.GetColor(colorTable, defaultColor, false);
 }
 
+TextColor TextAttribute::GetForeground() const noexcept
+{
+    return _foreground;
+}
+
+TextColor TextAttribute::GetBackground() const noexcept
+{
+    return _background;
+}
+
+void TextAttribute::SetForeground(const TextColor foreground) noexcept
+{
+    _foreground = foreground;
+}
+
+void TextAttribute::SetBackground(const TextColor background) noexcept
+{
+    _background = background;
+}
+
 void TextAttribute::SetForeground(const COLORREF rgbForeground) noexcept
 {
     _foreground = TextColor(rgbForeground);

src/buffer/out/TextAttribute.hpp

@@ -107,6 +107,10 @@ class TextAttribute final
 
     ExtendedAttributes GetExtendedAttributes() const noexcept;
 
+    TextColor GetForeground() const noexcept;
+    TextColor GetBackground() const noexcept;
+    void SetForeground(const TextColor foreground) noexcept;
+    void SetBackground(const TextColor background) noexcept;
     void SetForeground(const COLORREF rgbForeground) noexcept;
     void SetBackground(const COLORREF rgbBackground) noexcept;
     void SetIndexedForeground(const BYTE fgIndex) noexcept;

src/buffer/out/TextColor.cpp

@@ -50,6 +50,11 @@ constexpr std::array<BYTE, 256> Index256ToIndex16 = {
 
 // clang-format on
 
+bool TextColor::CanBeBrightened() const noexcept
+{
+    return IsIndex16() || IsDefault();
+}
+
 bool TextColor::IsLegacy() const noexcept
 {
     return IsIndex16() || (IsIndex256() && _index < 16);
@@ -164,7 +169,7 @@ COLORREF TextColor::GetColor(std::basic_string_view<COLORREF> colorTable,
     }
     else if (IsRgb())
     {
-        return _GetRGB();
+        return GetRGB();
     }
     else if (IsIndex16() && brighten)
     {
@@ -214,7 +219,7 @@ BYTE TextColor::GetLegacyIndex(const BYTE defaultIndex) const noexcept
 // - <none>
 // Return Value:
 // - a COLORREF containing our stored value
-COLORREF TextColor::_GetRGB() const noexcept
+COLORREF TextColor::GetRGB() const noexcept
 {
     return RGB(_red, _green, _blue);
 }

src/buffer/out/TextColor.h

@@ -77,6 +77,7 @@ struct TextColor
     friend constexpr bool operator==(const TextColor& a, const TextColor& b) noexcept;
     friend constexpr bool operator!=(const TextColor& a, const TextColor& b) noexcept;
 
+    bool CanBeBrightened() const noexcept;
     bool IsLegacy() const noexcept;
     bool IsIndex16() const noexcept;
     bool IsIndex256() const noexcept;
@@ -98,6 +99,8 @@ struct TextColor
         return _index;
     }
 
+    COLORREF GetRGB() const noexcept;
+
 private:
     ColorType _meta : 2;
     union
@@ -107,8 +110,6 @@ struct TextColor
     BYTE _green;
     BYTE _blue;
 
-    COLORREF _GetRGB() const noexcept;
-
 #ifdef UNIT_TESTING
     friend class TextBufferTests;
     template<typename TextColor>
@@ -149,7 +150,7 @@ namespace WEX
                 }
                 else if (color.IsRgb())
                 {
-                    return WEX::Common::NoThrowString().Format(L"{RGB:0x%06x}", color._GetRGB());
+                    return WEX::Common::NoThrowString().Format(L"{RGB:0x%06x}", color.GetRGB());
                 }
                 else
                 {

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

@@ -116,9 +116,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
         Viewport initialViewport = currentBuffer.GetViewport();
 
         auto vtRenderEngine = std::make_unique<Xterm256Engine>(std::move(hFile),
-                                                               gci,
-                                                               initialViewport,
-                                                               gci.Get16ColorTable());
+                                                               initialViewport);
         auto pfn = std::bind(&ConptyRoundtripTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2);
         vtRenderEngine->SetTestCallback(pfn);
 

src/host/VtIo.cpp

@@ -155,22 +155,16 @@ VtIo::VtIo() :
             {
             case VtIoMode::XTERM_256:
                 _pVtRenderEngine = std::make_unique<Xterm256Engine>(std::move(_hOutput),
-                                                                    gci,
-                                                                    initialViewport,
-                                                                    gci.Get16ColorTable());
+                                                                    initialViewport);
                 break;
             case VtIoMode::XTERM:
                 _pVtRenderEngine = std::make_unique<XtermEngine>(std::move(_hOutput),
-                                                                 gci,
                                                                  initialViewport,
-                                                                 gci.Get16ColorTable(),
                                                                  false);
                 break;
             case VtIoMode::XTERM_ASCII:
                 _pVtRenderEngine = std::make_unique<XtermEngine>(std::move(_hOutput),
-                                                                 gci,
                                                                  initialViewport,
-                                                                 gci.Get16ColorTable(),
                                                                  true);
                 break;
             default:

src/host/conattrs.cpp

@@ -17,100 +17,6 @@ Author(s):
 #include "..\inc\conattrs.hpp"
 #include <cmath>
 
-struct _HSL
-{
-    double h, s, l;
-
-    // constructs an HSL color from a RGB Color.
-    explicit _HSL(const COLORREF rgb)
-    {
-        const double r = (double)GetRValue(rgb);
-        const double g = (double)GetGValue(rgb);
-        const double b = (double)GetBValue(rgb);
-
-        const auto [min, max] = std::minmax({ r, g, b });
-
-        const auto diff = max - min;
-        const auto sum = max + min;
-        // Luminance
-        l = max / 255.0;
-
-        // Saturation
-        s = (max == 0) ? 0 : diff / max;
-
-        //Hue
-        double q = (diff == 0) ? 0 : 60.0 / diff;
-        if (max == r)
-        {
-            h = (g < b) ? ((360.0 + q * (g - b)) / 360.0) : ((q * (g - b)) / 360.0);
-        }
-        else if (max == g)
-        {
-            h = (120.0 + q * (b - r)) / 360.0;
-        }
-        else if (max == b)
-        {
-            h = (240.0 + q * (r - g)) / 360.0;
-        }
-        else
-        {
-            h = 0;
-        }
-    }
-};
-
-//Routine Description:
-// Finds the "distance" between a given HSL color and an RGB color, using the HSL color space.
-//   This function is designed such that the caller would convert one RGB color to HSL ahead of time,
-//   then compare many RGB colors to that first color.
-//Arguments:
-// - phslColorA - a pointer to the first color, as a HSL color.
-// - rgbColorB - The second color to compare, in RGB color.
-// Return value:
-// The "distance" between the two.
-static double _FindDifference(const _HSL* const phslColorA, const COLORREF rgbColorB)
-{
-    const _HSL hslColorB = _HSL(rgbColorB);
-    return sqrt(pow((hslColorB.h - phslColorA->h), 2) +
-                pow((hslColorB.s - phslColorA->s), 2) +
-                pow((hslColorB.l - phslColorA->l), 2));
-}
-
-//Routine Description:
-// For a given RGB color Color, finds the nearest color from the array ColorTable, and returns the index of that match.
-//Arguments:
-// - Color - The RGB color to fine the nearest color to.
-// - ColorTable - The array of colors to find a nearest color from.
-// Return value:
-// The index in ColorTable of the nearest match to Color.
-WORD FindNearestTableIndex(const COLORREF Color, const std::basic_string_view<COLORREF> ColorTable)
-{
-    // Quick check for an exact match in the color table:
-    for (WORD i = 0; i < ColorTable.size(); i++)
-    {
-        if (Color == ColorTable[i])
-        {
-            return i;
-        }
-    }
-
-    // Did not find an exact match - do an expensive comparison to the elements
-    //      of the table to find the nearest color.
-    const _HSL hslColor = _HSL(Color);
-    WORD closest = 0;
-    double minDiff = _FindDifference(&hslColor, ColorTable[0]);
-    for (WORD i = 1; i < ColorTable.size(); i++)
-    {
-        double diff = _FindDifference(&hslColor, ColorTable[i]);
-        if (diff < minDiff)
-        {
-            minDiff = diff;
-            closest = i;
-        }
-    }
-    return closest;
-}
-
 // Function Description:
 // - Converts the value of a xterm color table index to the windows color table equivalent.
 // Arguments:
@@ -144,26 +50,3 @@ WORD Xterm256ToWindowsIndex(const size_t xtermTableEntry) noexcept
     return xtermTableEntry < 16 ? XtermToWindowsIndex(xtermTableEntry) :
                                   static_cast<WORD>(xtermTableEntry);
 }
-
-//Routine Description:
-// Returns the exact entry from the color table, if it's in there.
-//Arguments:
-// - Color - The RGB color to fine the nearest color to.
-// - ColorTable - The array of colors to find a nearest color from.
-// Return value:
-// The index in ColorTable of the nearest match to Color.
-bool FindTableIndex(const COLORREF Color,
-                    const std::basic_string_view<COLORREF> ColorTable,
-                    _Out_ WORD* const pFoundIndex)
-{
-    *pFoundIndex = 0;
-    for (WORD i = 0; i < ColorTable.size(); i++)
-    {
-        if (ColorTable[i] == Color)
-        {
-            *pFoundIndex = i;
-            return true;
-        }
-    }
-    return false;
-}

src/host/server.h

@@ -29,8 +29,6 @@ Revision History:
 
 #include "..\host\RenderData.hpp"
 
-#include "..\inc\IDefaultColorProvider.hpp"
-
 // clang-format off
 // Flags flags
 #define CONSOLE_IS_ICONIC               0x00000001
@@ -75,8 +73,7 @@ class CommandHistory;
 
 class CONSOLE_INFORMATION :
     public Settings,
-    public Microsoft::Console::IIoProvider,
-    public Microsoft::Console::IDefaultColorProvider
+    public Microsoft::Console::IIoProvider
 {
 public:
     CONSOLE_INFORMATION();

src/host/ut_host/ConptyOutputTests.cpp

@@ -85,9 +85,7 @@ class ConptyOutputTests
         Viewport initialViewport = currentBuffer.GetViewport();
 
         auto vtRenderEngine = std::make_unique<Xterm256Engine>(std::move(hFile),
-                                                               gci,
-                                                               initialViewport,
-                                                               gci.Get16ColorTable());
+                                                               initialViewport);
         auto pfn = std::bind(&ConptyOutputTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2);
         vtRenderEngine->SetTestCallback(pfn);