Fix SGR indexed colors to distinguish Indexed256 color (and more)

src/buffer/out/TextAttribute.cpp

@@ -10,6 +10,11 @@ bool TextAttribute::IsLegacy() const noexcept
     return _foreground.IsLegacy() && _background.IsLegacy();
 }
 
+bool TextAttribute::IsHighColor() const noexcept
+{
+    return _foreground.IsHighColor() || _background.IsHighColor();
+}
+
 // Arguments:
 // - None
 // Return Value:
@@ -72,12 +77,22 @@ void TextAttribute::SetBackground(const COLORREF rgbBackground) noexcept
 
 void TextAttribute::SetIndexedForeground(const BYTE fgIndex) noexcept
 {
-    _foreground = TextColor(fgIndex);
+    _foreground = TextColor(fgIndex, false);
 }
 
 void TextAttribute::SetIndexedBackground(const BYTE bgIndex) noexcept
 {
-    _background = TextColor(bgIndex);
+    _background = TextColor(bgIndex, false);
+}
+
+void TextAttribute::SetIndexedForeground256(const BYTE fgIndex) noexcept
+{
+    _foreground = TextColor(fgIndex, true);
+}
+
+void TextAttribute::SetIndexedBackground256(const BYTE bgIndex) noexcept
+{
+    _background = TextColor(bgIndex, true);
 }
 
 void TextAttribute::SetColor(const COLORREF rgbColor, const bool fIsForeground) noexcept

src/buffer/out/TextAttribute.hpp

@@ -42,8 +42,8 @@ class TextAttribute final
 
     constexpr TextAttribute(const WORD wLegacyAttr) noexcept :
         _wAttrLegacy{ gsl::narrow_cast<WORD>(wLegacyAttr & META_ATTRS) },
-        _foreground{ gsl::narrow_cast<BYTE>(wLegacyAttr & FG_ATTRS) },
-        _background{ gsl::narrow_cast<BYTE>((wLegacyAttr & BG_ATTRS) >> 4) },
+        _foreground{ gsl::narrow_cast<BYTE>(wLegacyAttr & FG_ATTRS), true },
+        _background{ gsl::narrow_cast<BYTE>((wLegacyAttr & BG_ATTRS) >> 4), true },
         _extendedAttrs{ ExtendedAttributes::Normal }
     {
         // If we're given lead/trailing byte information with the legacy color, strip it.
@@ -59,12 +59,13 @@ class TextAttribute final
     {
     }
 
-    constexpr WORD GetLegacyAttributes() const noexcept
+    WORD GetLegacyAttributes() const noexcept
     {
         const BYTE fg = (_foreground.GetIndex() & FG_ATTRS);
         const BYTE bg = (_background.GetIndex() << 4) & BG_ATTRS;
         const WORD meta = (_wAttrLegacy & META_ATTRS);
-        return (fg | bg | meta) | (IsBold() ? FOREGROUND_INTENSITY : 0);
+        const bool brighten = _foreground.IsIndex16() && IsBold();
+        return (fg | bg | meta) | (brighten ? FOREGROUND_INTENSITY : 0);
     }
 
     // Method Description:
@@ -79,15 +80,16 @@ class TextAttribute final
     //      the background not be a legacy style attribute.
     // Return Value:
     // - a WORD with legacy-style attributes for this textattribute.
-    constexpr WORD GetLegacyAttributes(const BYTE defaultFgIndex,
-                                       const BYTE defaultBgIndex) const noexcept
+    WORD GetLegacyAttributes(const BYTE defaultFgIndex,
+                             const BYTE defaultBgIndex) const noexcept
     {
         const BYTE fgIndex = _foreground.IsLegacy() ? _foreground.GetIndex() : defaultFgIndex;
         const BYTE bgIndex = _background.IsLegacy() ? _background.GetIndex() : defaultBgIndex;
         const BYTE fg = (fgIndex & FG_ATTRS);
         const BYTE bg = (bgIndex << 4) & BG_ATTRS;
         const WORD meta = (_wAttrLegacy & META_ATTRS);
-        return (fg | bg | meta) | (IsBold() ? FOREGROUND_INTENSITY : 0);
+        const bool brighten = _foreground.IsIndex16() && IsBold();
+        return (fg | bg | meta) | (brighten ? FOREGROUND_INTENSITY : 0);
     }
 
     COLORREF CalculateRgbForeground(std::basic_string_view<COLORREF> colorTable,
@@ -117,6 +119,7 @@ class TextAttribute final
     friend constexpr bool operator!=(const WORD& legacyAttr, const TextAttribute& attr) noexcept;
 
     bool IsLegacy() const noexcept;
+    bool IsHighColor() const noexcept;
     bool IsBold() const noexcept;
     bool IsItalic() const noexcept;
     bool IsBlinking() const noexcept;
@@ -139,6 +142,8 @@ class TextAttribute final
     void SetBackground(const COLORREF rgbBackground) noexcept;
     void SetIndexedForeground(const BYTE fgIndex) noexcept;
     void SetIndexedBackground(const BYTE bgIndex) noexcept;
+    void SetIndexedForeground256(const BYTE fgIndex) noexcept;
+    void SetIndexedBackground256(const BYTE bgIndex) noexcept;
     void SetColor(const COLORREF rgbColor, const bool fIsForeground) noexcept;
 
     void SetDefaultForeground() noexcept;
@@ -149,11 +154,6 @@ class TextAttribute final
 
     void SetStandardErase() noexcept;
 
-    constexpr bool IsRgb() const noexcept
-    {
-        return _foreground.IsRgb() || _background.IsRgb();
-    }
-
     // This returns whether this attribute, if printed directly next to another attribute, for the space
     // character, would look identical to the other one.
     bool HasIdenticalVisualRepresentationForBlankSpace(const TextAttribute& other, const bool inverted = false) const noexcept
@@ -224,26 +224,6 @@ constexpr bool operator!=(const TextAttribute& a, const TextAttribute& b) noexce
     return !(a == b);
 }
 
-constexpr bool operator==(const TextAttribute& attr, const WORD& legacyAttr) noexcept
-{
-    return attr.GetLegacyAttributes() == legacyAttr;
-}
-
-constexpr bool operator!=(const TextAttribute& attr, const WORD& legacyAttr) noexcept
-{
-    return !(attr == legacyAttr);
-}
-
-constexpr bool operator==(const WORD& legacyAttr, const TextAttribute& attr) noexcept
-{
-    return attr == legacyAttr;
-}
-
-constexpr bool operator!=(const WORD& legacyAttr, const TextAttribute& attr) noexcept
-{
-    return !(attr == legacyAttr);
-}
-
 #ifdef UNIT_TESTING
 
 #define LOG_ATTR(attr) (Log::Comment(NoThrowString().Format( \

src/buffer/out/TextColor.cpp

@@ -4,6 +4,36 @@
 #include "precomp.h"
 #include "TextColor.h"
 
+bool TextColor::IsLegacy() const noexcept
+{
+    return IsIndex16() || (IsIndex256() && _index < 16);
+}
+
+bool TextColor::IsHighColor() const noexcept
+{
+    return IsRgb() || (IsIndex256() && _index >= 16);
+}
+
+bool TextColor::IsIndex16() const noexcept
+{
+    return _meta == ColorType::IsIndex16;
+}
+
+bool TextColor::IsIndex256() const noexcept
+{
+    return _meta == ColorType::IsIndex256;
+}
+
+bool TextColor::IsDefault() const noexcept
+{
+    return _meta == ColorType::IsDefault;
+}
+
+bool TextColor::IsRgb() const noexcept
+{
+    return _meta == ColorType::IsRgb;
+}
+
 // Method Description:
 // - Sets the color value of this attribute, and sets this color to be an RGB
 //      attribute.
@@ -23,11 +53,12 @@ void TextColor::SetColor(const COLORREF rgbColor) noexcept
 // - Sets this TextColor to be a legacy-style index into the color table.
 // Arguments:
 // - index: the index of the colortable we should use for this TextColor.
+// - isIndex256: is this a 256 color index (true) or a 16 color index (false).
 // Return Value:
 // - <none>
-void TextColor::SetIndex(const BYTE index) noexcept
+void TextColor::SetIndex(const BYTE index, const bool isIndex256) noexcept
 {
-    _meta = ColorType::IsIndex;
+    _meta = isIndex256 ? ColorType::IsIndex256 : ColorType::IsIndex16;
     _index = index;
 }
 
@@ -48,15 +79,15 @@ void TextColor::SetDefault() noexcept
 //   * If we're an RGB color, we'll use that value.
 //   * If we're an indexed color table value, we'll use that index to look up
 //     our value in the provided color table.
-//     - If brighten is true, and the index is in the "dark" portion of the
-//       color table (indices [0,7]), then we'll look up the bright version of
-//       this color (from indices [8,15]). This should be true for
-//       TextAttributes that are "Bold" and we're treating bold as bright
-//       (which is the default behavior of most terminals.)
+//     - If brighten is true, and we've got a 16 color index in the "dark"
+//       portion of the color table (indices [0,7]), then we'll look up the
+//       bright version of this color (from indices [8,15]). This should be
+//       true for TextAttributes that are "Bold" and we're treating bold as
+//       bright (which is the default behavior of most terminals.)
 //   * If we're a default color, we'll return the default color provided.
 // Arguments:
-// - colorTable: The table of colors we should use to look up the value of a
-//      legacy attribute from
+// - colorTable: The table of colors we should use to look up the value of
+//      an indexed attribute from.
 // - defaultColor: The color value to use if we're a default attribute.
 // - brighten: if true, we'll brighten a dark color table index.
 // Return Value:
@@ -94,21 +125,13 @@ COLORREF TextColor::GetColor(std::basic_string_view<COLORREF> colorTable,
     {
         return _GetRGB();
     }
+    else if (IsIndex16() && brighten)
+    {
+        return colorTable.at(_index | 8);
+    }
     else
     {
-        FAIL_FAST_IF(colorTable.size() < _index);
-        // If the color is already bright (it's in index [8,15] or it's a
-        //       256color value [16,255], then boldness does nothing.
-        if (brighten && _index < 8)
-        {
-            FAIL_FAST_IF(colorTable.size() < 16);
-            FAIL_FAST_IF(gsl::narrow_cast<size_t>(_index) + 8 > colorTable.size());
-            return colorTable.at(gsl::narrow_cast<size_t>(_index) + 8);
-        }
-        else
-        {
-            return colorTable.at(_index);
-        }
+        return colorTable.at(_index);
     }
 }
 

src/buffer/out/TextColor.h

@@ -41,9 +41,10 @@ Revision History:
 
 enum class ColorType : BYTE
 {
-    IsIndex = 0x0,
-    IsDefault = 0x1,
-    IsRgb = 0x2
+    IsIndex256 = 0x0,
+    IsIndex16 = 0x1,
+    IsDefault = 0x2,
+    IsRgb = 0x3
 };
 
 struct TextColor
@@ -57,9 +58,9 @@ struct TextColor
     {
     }
 
-    constexpr TextColor(const BYTE wLegacyAttr) noexcept :
-        _meta{ ColorType::IsIndex },
-        _index{ wLegacyAttr },
+    constexpr TextColor(const BYTE index, const bool isIndex256) noexcept :
+        _meta{ isIndex256 ? ColorType::IsIndex256 : ColorType::IsIndex16 },
+        _index{ index },
         _green{ 0 },
         _blue{ 0 }
     {
@@ -76,23 +77,15 @@ struct TextColor
     friend constexpr bool operator==(const TextColor& a, const TextColor& b) noexcept;
     friend constexpr bool operator!=(const TextColor& a, const TextColor& b) noexcept;
 
-    constexpr bool IsLegacy() const noexcept
-    {
-        return !(IsDefault() || IsRgb());
-    }
-
-    constexpr bool IsDefault() const noexcept
-    {
-        return _meta == ColorType::IsDefault;
-    }
-
-    constexpr bool IsRgb() const noexcept
-    {
-        return _meta == ColorType::IsRgb;
-    }
+    bool IsLegacy() const noexcept;
+    bool IsHighColor() const noexcept;
+    bool IsIndex16() const noexcept;
+    bool IsIndex256() const noexcept;
+    bool IsDefault() const noexcept;
+    bool IsRgb() const noexcept;
 
     void SetColor(const COLORREF rgbColor) noexcept;
-    void SetIndex(const BYTE index) noexcept;
+    void SetIndex(const BYTE index, const bool isIndex256) noexcept;
     void SetDefault() noexcept;
 
     COLORREF GetColor(std::basic_string_view<COLORREF> colorTable,

src/buffer/out/ut_textbuffer/TextColorTests.cpp

@@ -81,7 +81,7 @@ void TextColorTests::TestDefaultColor()
 
 void TextColorTests::TestDarkIndexColor()
 {
-    TextColor indexColor((BYTE)(7));
+    TextColor indexColor((BYTE)(7), false);
 
     VERIFY_IS_FALSE(indexColor.IsDefault());
     VERIFY_IS_TRUE(indexColor.IsLegacy());
@@ -104,7 +104,7 @@ void TextColorTests::TestDarkIndexColor()
 
 void TextColorTests::TestBrightIndexColor()
 {
-    TextColor indexColor((BYTE)(15));
+    TextColor indexColor((BYTE)(15), false);
 
     VERIFY_IS_FALSE(indexColor.IsDefault());
     VERIFY_IS_TRUE(indexColor.IsLegacy());
@@ -186,7 +186,7 @@ void TextColorTests::TestChangeColor()
     color = rgbColor.GetColor(view, _defaultBg, true);
     VERIFY_ARE_EQUAL(_defaultBg, color);
 
-    rgbColor.SetIndex(7);
+    rgbColor.SetIndex(7, false);
     color = rgbColor.GetColor(view, _defaultFg, false);
     VERIFY_ARE_EQUAL(_colorTable[7], color);
 
@@ -199,7 +199,7 @@ void TextColorTests::TestChangeColor()
     color = rgbColor.GetColor(view, _defaultBg, true);
     VERIFY_ARE_EQUAL(_colorTable[15], color);
 
-    rgbColor.SetIndex(15);
+    rgbColor.SetIndex(15, false);
     color = rgbColor.GetColor(view, _defaultFg, false);
     VERIFY_ARE_EQUAL(_colorTable[15], color);
 

src/cascadia/TerminalCore/ITerminalApi.hpp

@@ -21,6 +21,8 @@ namespace Microsoft::Terminal::Core
         virtual bool SetTextToDefaults(bool foreground, bool background) noexcept = 0;
         virtual bool SetTextForegroundIndex(BYTE colorIndex) noexcept = 0;
         virtual bool SetTextBackgroundIndex(BYTE colorIndex) noexcept = 0;
+        virtual bool SetTextForegroundIndex256(BYTE colorIndex) noexcept = 0;
+        virtual bool SetTextBackgroundIndex256(BYTE colorIndex) noexcept = 0;
         virtual bool SetTextRgbColor(COLORREF color, bool foreground) noexcept = 0;
         virtual bool BoldText(bool boldOn) noexcept = 0;
         virtual bool UnderlineText(bool underlineOn) noexcept = 0;

src/cascadia/TerminalCore/Terminal.hpp

@@ -81,6 +81,8 @@ class Microsoft::Terminal::Core::Terminal final :
     bool SetTextToDefaults(bool foreground, bool background) noexcept override;
     bool SetTextForegroundIndex(BYTE colorIndex) noexcept override;
     bool SetTextBackgroundIndex(BYTE colorIndex) noexcept override;
+    bool SetTextForegroundIndex256(BYTE colorIndex) noexcept override;
+    bool SetTextBackgroundIndex256(BYTE colorIndex) noexcept override;
     bool SetTextRgbColor(COLORREF color, bool foreground) noexcept override;
     bool BoldText(bool boldOn) noexcept override;
     bool UnderlineText(bool underlineOn) noexcept override;

src/cascadia/TerminalCore/TerminalApi.cpp

@@ -57,6 +57,22 @@ bool Terminal::SetTextBackgroundIndex(BYTE colorIndex) noexcept
     return true;
 }
 
+bool Terminal::SetTextForegroundIndex256(BYTE colorIndex) noexcept
+{
+    TextAttribute attrs = _buffer->GetCurrentAttributes();
+    attrs.SetIndexedForeground256(colorIndex);
+    _buffer->SetCurrentAttributes(attrs);
+    return true;
+}
+
+bool Terminal::SetTextBackgroundIndex256(BYTE colorIndex) noexcept
+{
+    TextAttribute attrs = _buffer->GetCurrentAttributes();
+    attrs.SetIndexedBackground256(colorIndex);
+    _buffer->SetCurrentAttributes(attrs);
+    return true;
+}
+
 bool Terminal::SetTextRgbColor(COLORREF color, bool foreground) noexcept
 {
     TextAttribute attrs = _buffer->GetCurrentAttributes();

src/cascadia/TerminalCore/TerminalDispatchGraphics.cpp

@@ -121,8 +121,8 @@ bool TerminalDispatch::_SetRgbColorsHelper(const std::basic_string_view<Dispatch
             {
                 const auto tableIndex = til::at(options, 2);
                 success = isForeground ?
-                              _terminalApi.SetTextForegroundIndex(gsl::narrow_cast<BYTE>(tableIndex)) :
-                              _terminalApi.SetTextBackgroundIndex(gsl::narrow_cast<BYTE>(tableIndex));
+                              _terminalApi.SetTextForegroundIndex256(gsl::narrow_cast<BYTE>(tableIndex)) :
+                              _terminalApi.SetTextBackgroundIndex256(gsl::narrow_cast<BYTE>(tableIndex));
             }
         }
     }

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

@@ -119,8 +119,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final
         auto vtRenderEngine = std::make_unique<Xterm256Engine>(std::move(hFile),
                                                                gci,
                                                                initialViewport,
-                                                               gci.GetColorTable(),
-                                                               static_cast<WORD>(gci.GetColorTableSize()));
+                                                               gci.Get16ColorTable());
         auto pfn = std::bind(&ConptyRoundtripTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2);
         vtRenderEngine->SetTestCallback(pfn);