Refactor the SGR implementation in AdaptDispatch (#5758)

This is an attempt to simplify the SGR (Select Graphic Rendition)
implementation in conhost, to cut down on the number of methods required
in the `ConGetSet` interface, and pave the way for future improvements
and bug fixes. It already fixes one bug that prevented SGR 0 from being
correctly applied when combined with meta attributes.

* This a first step towards fixing the conpty narrowing bugs in issue
  #2661
* I'm hoping the simplification of `ConGetSet` will also help with
  #3849.
* Some of the `TextAttribute` refactoring in this PR overlaps with
  similar work in PR #1978. 

## Detailed Description of the Pull Request / Additional comments

The main point of this PR was to simplify the
`AdaptDispatch::SetGraphicsRendition` implementation. So instead of
having it call a half a dozen methods in the `ConGetSet` API, depending
on what kinds of attributes needed to be set, there is now just one call
to get current attributes, and another call to set the new value. All
adjustments to the attributes are made in the `AdaptDispatch` class, in
a simple switch statement.

To help with this refactoring, I also made some change to the
`TextAttribute` class to make it easier to work with. This included
adding a set of methods for setting (and getting) the individual
attribute flags, instead of having the calling code being exposed to the
internal attribute structures and messing with bit manipulation. I've
tried to get rid of any methods that were directly setting legacy, meta,
and extended attributes.

Other than the fix to the `SGR 0` bug, the `AdaptDispatch` refactoring
mostly follows the behaviour of the original code. In particular, it
still maps the `SGR 38/48` indexed colors to RGB instead of retaining
the index, which is what we ultimately need it to do. Fixing that will
first require the color tables to be unified (issue #1223), which I'm
hoping to address in a followup PR.

But for now, mapping the indexed colors to RGB values required adding an
an additional `ConGetSet` API to lookup the color table entries. In the
future that won't be necessary, but the API will still be useful for
other color reporting operations that we may want to support. I've made
this API, and the existing setter, standardise on index values being in
the "Xterm" order, since that'll be essential for unifying the code with
the terminal adapter one day.

I should also point out one minor change to the `SGR 38/48` behavior,
which is that out-of-range RGB colors are now ignored rather than being
clamped, since that matches the way Xterm works.

## Validation Steps Performed

This refactoring has obviously required corresponding changes to the
unit tests, but most were just minor updates to use the new
`TextAttribute` methods without any real change in behavior. However,
the adapter tests did require significant changes to accommodate the new
`ConGetSet` API. The basic structure of the tests remain the same, but
the simpler API has meant fewer values needed to be checked in each test
case. I think they are all still covering the areas there were intended
to, though, and they are all still passing.

Other than getting the unit tests to work, I've also done a bunch of
manual testing of my own. I've made sure the color tests in Vttest all
still work as well as they used to. And I've confirmed that the test
case from issue #5341 is now working correctly.

Closes #5341

.github/actions/spell-check/dictionary/apis.txt

@@ -1,3 +1,6 @@
+bitfield
+bitfields
+href
 IBox
 ICustom
 IMap

src/buffer/out/AttrRow.cpp

@@ -190,26 +190,6 @@ bool ATTR_ROW::SetAttrToEnd(const UINT iStart, const TextAttribute attr)
     return SUCCEEDED(InsertAttrRuns({ &run, 1 }, iStart, _cchRowWidth - 1, _cchRowWidth));
 }
 
-// Routine Description:
-// - Replaces all runs in the row with the given wToBeReplacedAttr with the new
-//      attribute wReplaceWith. This method is used for replacing specifically
-//      legacy attributes.
-// Arguments:
-// - wToBeReplacedAttr - the legacy attribute to replace in this row.
-// - wReplaceWith - the new value for the matching runs' attributes.
-// Return Value:
-// <none>
-void ATTR_ROW::ReplaceLegacyAttrs(_In_ WORD wToBeReplacedAttr, _In_ WORD wReplaceWith) noexcept
-{
-    TextAttribute ToBeReplaced;
-    ToBeReplaced.SetFromLegacy(wToBeReplacedAttr);
-
-    TextAttribute ReplaceWith;
-    ReplaceWith.SetFromLegacy(wReplaceWith);
-
-    ReplaceAttrs(ToBeReplaced, ReplaceWith);
-}
-
 // Method Description:
 // - Replaces all runs in the row with the given toBeReplacedAttr with the new
 //      attribute replaceWith.

src/buffer/out/AttrRow.hpp

@@ -42,7 +42,6 @@ class ATTR_ROW final
                          size_t* const pApplies) const;
 
     bool SetAttrToEnd(const UINT iStart, const TextAttribute attr);
-    void ReplaceLegacyAttrs(const WORD wToBeReplacedAttr, const WORD wReplaceWith) noexcept;
     void ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAttribute& replaceWith) noexcept;
 
     void Resize(const size_t newWidth);

src/buffer/out/OutputCell.cpp

@@ -44,15 +44,6 @@ OutputCell::OutputCell(const std::wstring_view charData,
     _setFromStringView(charData);
 }
 
-OutputCell::OutputCell(const CHAR_INFO& charInfo) :
-    _text{ UNICODE_INVALID },
-    _dbcsAttribute{},
-    _textAttribute{ InvalidTextAttribute },
-    _behavior{ TextAttributeBehavior::Stored }
-{
-    _setFromCharInfo(charInfo);
-}
-
 OutputCell::OutputCell(const OutputCellView& cell)
 {
     _setFromOutputCellView(cell);
@@ -84,23 +75,6 @@ void OutputCell::_setFromBehavior(const TextAttributeBehavior behavior)
     THROW_HR_IF(E_INVALIDARG, behavior == TextAttributeBehavior::Stored);
 }
 
-void OutputCell::_setFromCharInfo(const CHAR_INFO& charInfo)
-{
-    _text = charInfo.Char.UnicodeChar;
-
-    if (WI_IsFlagSet(charInfo.Attributes, COMMON_LVB_LEADING_BYTE))
-    {
-        _dbcsAttribute.SetLeading();
-    }
-    else if (WI_IsFlagSet(charInfo.Attributes, COMMON_LVB_TRAILING_BYTE))
-    {
-        _dbcsAttribute.SetTrailing();
-    }
-    _textAttribute.SetFromLegacy(charInfo.Attributes);
-
-    _behavior = TextAttributeBehavior::Stored;
-}
-
 void OutputCell::_setFromStringView(const std::wstring_view view)
 {
     _text = view;

src/buffer/out/OutputCell.hpp

@@ -44,7 +44,6 @@ class OutputCell final
                const DbcsAttribute dbcsAttribute,
                const TextAttribute textAttribute);
 
-    OutputCell(const CHAR_INFO& charInfo);
     OutputCell(const OutputCellView& view);
 
     OutputCell(const OutputCell&) = default;
@@ -86,7 +85,6 @@ class OutputCell final
     TextAttributeBehavior _behavior;
 
     void _setFromBehavior(const TextAttributeBehavior behavior);
-    void _setFromCharInfo(const CHAR_INFO& charInfo);
     void _setFromStringView(const std::wstring_view view);
     void _setFromOutputCellView(const OutputCellView& cell);
 };

src/buffer/out/OutputCellIterator.cpp

@@ -518,8 +518,7 @@ OutputCellView OutputCellIterator::s_GenerateView(const CHAR_INFO& charInfo) noe
         dbcsAttr.SetTrailing();
     }
 
-    TextAttribute textAttr;
-    textAttr.SetFromLegacy(charInfo.Attributes);
+    const TextAttribute textAttr(charInfo.Attributes);
 
     const auto behavior = TextAttributeBehavior::Stored;
     return OutputCellView(glyph, dbcsAttr, textAttr, behavior);

src/buffer/out/TextAttribute.cpp

@@ -18,7 +18,7 @@ COLORREF TextAttribute::CalculateRgbForeground(std::basic_string_view<COLORREF>
                                                COLORREF defaultFgColor,
                                                COLORREF defaultBgColor) const noexcept
 {
-    return _IsReverseVideo() ? _GetRgbBackground(colorTable, defaultBgColor) : _GetRgbForeground(colorTable, defaultFgColor);
+    return IsReverseVideo() ? _GetRgbBackground(colorTable, defaultBgColor) : _GetRgbForeground(colorTable, defaultFgColor);
 }
 
 // Routine Description:
@@ -31,7 +31,7 @@ COLORREF TextAttribute::CalculateRgbBackground(std::basic_string_view<COLORREF>
                                                COLORREF defaultFgColor,
                                                COLORREF defaultBgColor) const noexcept
 {
-    return _IsReverseVideo() ? _GetRgbForeground(colorTable, defaultFgColor) : _GetRgbBackground(colorTable, defaultBgColor);
+    return IsReverseVideo() ? _GetRgbForeground(colorTable, defaultFgColor) : _GetRgbBackground(colorTable, defaultBgColor);
 }
 
 // Routine Description:
@@ -60,21 +60,6 @@ COLORREF TextAttribute::_GetRgbBackground(std::basic_string_view<COLORREF> color
     return _background.GetColor(colorTable, defaultColor, false);
 }
 
-void TextAttribute::SetMetaAttributes(const WORD wMeta) noexcept
-{
-    WI_UpdateFlagsInMask(_wAttrLegacy, META_ATTRS, wMeta);
-    WI_ClearAllFlags(_wAttrLegacy, COMMON_LVB_SBCSDBCS);
-}
-
-WORD TextAttribute::GetMetaAttributes() const noexcept
-{
-    WORD wMeta = _wAttrLegacy;
-    WI_ClearAllFlags(wMeta, FG_ATTRS);
-    WI_ClearAllFlags(wMeta, BG_ATTRS);
-    WI_ClearAllFlags(wMeta, COMMON_LVB_SBCSDBCS);
-    return wMeta;
-}
-
 void TextAttribute::SetForeground(const COLORREF rgbForeground) noexcept
 {
     _foreground = TextColor(rgbForeground);
@@ -85,62 +70,14 @@ void TextAttribute::SetBackground(const COLORREF rgbBackground) noexcept
     _background = TextColor(rgbBackground);
 }
 
-void TextAttribute::SetFromLegacy(const WORD wLegacy) noexcept
+void TextAttribute::SetIndexedForeground(const BYTE fgIndex) noexcept
 {
-    _wAttrLegacy = gsl::narrow_cast<WORD>(wLegacy & META_ATTRS);
-    WI_ClearAllFlags(_wAttrLegacy, COMMON_LVB_SBCSDBCS);
-    const BYTE fgIndex = gsl::narrow_cast<BYTE>(wLegacy & FG_ATTRS);
-    const BYTE bgIndex = gsl::narrow_cast<BYTE>(wLegacy & BG_ATTRS) >> 4;
     _foreground = TextColor(fgIndex);
-    _background = TextColor(bgIndex);
 }
 
-void TextAttribute::SetLegacyAttributes(const WORD attrs,
-                                        const bool setForeground,
-                                        const bool setBackground,
-                                        const bool setMeta) noexcept
-{
-    if (setForeground)
-    {
-        const BYTE fgIndex = gsl::narrow_cast<BYTE>(attrs & FG_ATTRS);
-        _foreground = TextColor(fgIndex);
-    }
-    if (setBackground)
-    {
-        const BYTE bgIndex = gsl::narrow_cast<BYTE>(attrs & BG_ATTRS) >> 4;
-        _background = TextColor(bgIndex);
-    }
-    if (setMeta)
-    {
-        SetMetaAttributes(attrs);
-    }
-}
-
-// Method Description:
-// - Sets the foreground and/or background to a particular index in the 256color
-//      table. If either parameter is nullptr, it's ignored.
-//   This method can be used to set the colors to indexes in the range [0, 255],
-//      as opposed to SetLegacyAttributes, which clamps them to [0,15]
-// Arguments:
-// - foreground: nullptr if we should ignore this attr, else a pointer to a byte
-//      value to use as an index into the 256-color table.
-// - background: nullptr if we should ignore this attr, else a pointer to a byte
-//      value to use as an index into the 256-color table.
-// Return Value:
-// - <none>
-void TextAttribute::SetIndexedAttributes(const std::optional<const BYTE> foreground,
-                                         const std::optional<const BYTE> background) noexcept
+void TextAttribute::SetIndexedBackground(const BYTE bgIndex) noexcept
 {
-    if (foreground)
-    {
-        const BYTE fgIndex = (*foreground) & 0xFF;
-        _foreground = TextColor(fgIndex);
-    }
-    if (background)
-    {
-        const BYTE bgIndex = (*background) & 0xFF;
-        _background = TextColor(bgIndex);
-    }
+    _background = TextColor(bgIndex);
 }
 
 void TextAttribute::SetColor(const COLORREF rgbColor, const bool fIsForeground) noexcept
@@ -195,33 +132,90 @@ void TextAttribute::SetRightVerticalDisplayed(const bool isDisplayed) noexcept
     WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_GRID_RVERTICAL, isDisplayed);
 }
 
-void TextAttribute::Embolden() noexcept
+bool TextAttribute::IsBold() const noexcept
 {
-    _SetBoldness(true);
+    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Bold);
 }
 
-void TextAttribute::Debolden() noexcept
+bool TextAttribute::IsItalic() const noexcept
 {
-    _SetBoldness(false);
+    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Italics);
 }
 
-void TextAttribute::SetExtendedAttributes(const ExtendedAttributes attrs) noexcept
+bool TextAttribute::IsBlinking() const noexcept
 {
-    _extendedAttrs = attrs;
+    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Blinking);
 }
 
-// Routine Description:
-// - swaps foreground and background color
-void TextAttribute::Invert() noexcept
+bool TextAttribute::IsInvisible() const noexcept
 {
-    WI_ToggleFlag(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO);
+    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Invisible);
+}
+
+bool TextAttribute::IsCrossedOut() const noexcept
+{
+    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::CrossedOut);
 }
 
-void TextAttribute::_SetBoldness(const bool isBold) noexcept
+bool TextAttribute::IsUnderlined() const noexcept
+{
+    // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE
+    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_UNDERSCORE);
+}
+
+bool TextAttribute::IsReverseVideo() const noexcept
+{
+    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO);
+}
+
+void TextAttribute::SetBold(bool isBold) noexcept
 {
     WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Bold, isBold);
 }
 
+void TextAttribute::SetItalics(bool isItalic) noexcept
+{
+    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Italics, isItalic);
+}
+
+void TextAttribute::SetBlinking(bool isBlinking) noexcept
+{
+    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Blinking, isBlinking);
+}
+
+void TextAttribute::SetInvisible(bool isInvisible) noexcept
+{
+    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Invisible, isInvisible);
+}
+
+void TextAttribute::SetCrossedOut(bool isCrossedOut) noexcept
+{
+    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::CrossedOut, isCrossedOut);
+}
+
+void TextAttribute::SetUnderline(bool isUnderlined) noexcept
+{
+    // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE
+    WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_UNDERSCORE, isUnderlined);
+}
+
+void TextAttribute::SetReverseVideo(bool isReversed) noexcept
+{
+    WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO, isReversed);
+}
+
+ExtendedAttributes TextAttribute::GetExtendedAttributes() const noexcept
+{
+    return _extendedAttrs;
+}
+
+// Routine Description:
+// - swaps foreground and background color
+void TextAttribute::Invert() noexcept
+{
+    WI_ToggleFlag(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO);
+}
+
 void TextAttribute::SetDefaultForeground() noexcept
 {
     _foreground = TextColor();
@@ -267,6 +261,6 @@ bool TextAttribute::BackgroundIsDefault() const noexcept
 //      requires for most erasing and filling operations.
 void TextAttribute::SetStandardErase() noexcept
 {
-    SetExtendedAttributes(ExtendedAttributes::Normal);
-    SetMetaAttributes(0);
+    _extendedAttrs = ExtendedAttributes::Normal;
+    _wAttrLegacy = 0;
 }