Merge the legacy and extended attributes (#14036)

This PR attempts to simplify the `TextAttribute` class by merging the
two fields that were previously storing the "legacy" attributes and the
"extended" attributes separately.

When the `TextAttribute` class is initialized with a legacy value, we
were masking off the `META_ATTRS` bits to store in the `_wAttrLegacy`
field, and then additionally clearing the `COMMON_LVB_SBCSDBCS` bits,
so there were only 5 bits that were actually used in the end. We also
had an additional `_extendedAttrs` field holding other VT attributes,
which only used 8 of its available 16 bits.

In this PR I've now merged the the two sets of attributes into one enum,
so they all fit in a single 16-bit value. The legacy attributes retain
the same bit positions they originally had, so we can mask them off from
an incoming legacy value as we did before. I've just simplified the
process somewhat by creating a `USED_META_ATTRS` mask that covers the
exact subset of meta attributes that we care about.

The new enum that holds the combined attributes has now been named
`CharacterAttributes` rather than `ExtendedAttributes`, since that seems
to be the term typically used in VT documentation. This covers both
rendition/visual attributes and logical attributes (not yet used, but we
will need them at some point to support selective erase operations).

While making these changes I also noticed the `IsLeadingByte` and
`IsTrailingByte` methods weren't actually used anywhere, and weren't
correctly implemented anyway, so I've removed those now.

## Validation Steps Performed

I've manually run a number of attribute test scripts which cover both
legacy and VT operations, and everything still appears to be working
correctly.

Closes #14031

src/buffer/out/TextAttribute.cpp

@@ -7,7 +7,7 @@
 
 // Keeping TextColor compact helps us keeping TextAttribute compact,
 // which in turn ensures that our buffer memory usage is low.
-static_assert(sizeof(TextAttribute) == 14);
+static_assert(sizeof(TextAttribute) == 12);
 static_assert(alignof(TextAttribute) == 2);
 // Ensure that we can memcpy() and memmove() the struct for performance.
 static_assert(std::is_trivially_copyable_v<TextAttribute>);
@@ -116,7 +116,7 @@ WORD TextAttribute::GetLegacyAttributes() const noexcept
 {
     const auto fgIndex = _foreground.GetLegacyIndex(s_legacyDefaultForeground);
     const auto bgIndex = _background.GetLegacyIndex(s_legacyDefaultBackground);
-    const WORD metaAttrs = _wAttrLegacy & META_ATTRS;
+    const WORD metaAttrs = static_cast<WORD>(_attrs) & USED_META_ATTRS;
     const auto brighten = IsIntense() && _foreground.CanBeBrightened();
     return fgIndex | (bgIndex << 4) | metaAttrs | (brighten ? FOREGROUND_INTENSITY : 0);
 }
@@ -217,151 +217,141 @@ void TextAttribute::SetHyperlinkId(uint16_t id) noexcept
     _hyperlinkId = id;
 }
 
-bool TextAttribute::IsLeadingByte() const noexcept
-{
-    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_LEADING_BYTE);
-}
-
-bool TextAttribute::IsTrailingByte() const noexcept
-{
-    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_LEADING_BYTE);
-}
-
 bool TextAttribute::IsTopHorizontalDisplayed() const noexcept
 {
-    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_GRID_HORIZONTAL);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::TopGridline);
 }
 
 bool TextAttribute::IsBottomHorizontalDisplayed() const noexcept
 {
-    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_UNDERSCORE);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::BottomGridline);
 }
 
 bool TextAttribute::IsLeftVerticalDisplayed() const noexcept
 {
-    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_GRID_LVERTICAL);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::LeftGridline);
 }
 
 bool TextAttribute::IsRightVerticalDisplayed() const noexcept
 {
-    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_GRID_RVERTICAL);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::RightGridline);
 }
 
 void TextAttribute::SetLeftVerticalDisplayed(const bool isDisplayed) noexcept
 {
-    WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_GRID_LVERTICAL, isDisplayed);
+    WI_UpdateFlag(_attrs, CharacterAttributes::LeftGridline, isDisplayed);
 }
 
 void TextAttribute::SetRightVerticalDisplayed(const bool isDisplayed) noexcept
 {
-    WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_GRID_RVERTICAL, isDisplayed);
+    WI_UpdateFlag(_attrs, CharacterAttributes::RightGridline, isDisplayed);
 }
 
 bool TextAttribute::IsIntense() const noexcept
 {
-    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Intense);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::Intense);
 }
 
 bool TextAttribute::IsFaint() const noexcept
 {
-    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Faint);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::Faint);
 }
 
 bool TextAttribute::IsItalic() const noexcept
 {
-    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Italics);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::Italics);
 }
 
 bool TextAttribute::IsBlinking() const noexcept
 {
-    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Blinking);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::Blinking);
 }
 
 bool TextAttribute::IsInvisible() const noexcept
 {
-    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Invisible);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::Invisible);
 }
 
 bool TextAttribute::IsCrossedOut() const noexcept
 {
-    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::CrossedOut);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::CrossedOut);
 }
 
 bool TextAttribute::IsUnderlined() const noexcept
 {
-    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Underlined);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::Underlined);
 }
 
 bool TextAttribute::IsDoublyUnderlined() const noexcept
 {
-    return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::DoublyUnderlined);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::DoublyUnderlined);
 }
 
 bool TextAttribute::IsOverlined() const noexcept
 {
-    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_GRID_HORIZONTAL);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::TopGridline);
 }
 
 bool TextAttribute::IsReverseVideo() const noexcept
 {
-    return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO);
+    return WI_IsFlagSet(_attrs, CharacterAttributes::ReverseVideo);
 }
 
 void TextAttribute::SetIntense(bool isIntense) noexcept
 {
-    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Intense, isIntense);
+    WI_UpdateFlag(_attrs, CharacterAttributes::Intense, isIntense);
 }
 
 void TextAttribute::SetFaint(bool isFaint) noexcept
 {
-    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Faint, isFaint);
+    WI_UpdateFlag(_attrs, CharacterAttributes::Faint, isFaint);
 }
 
 void TextAttribute::SetItalic(bool isItalic) noexcept
 {
-    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Italics, isItalic);
+    WI_UpdateFlag(_attrs, CharacterAttributes::Italics, isItalic);
 }
 
 void TextAttribute::SetBlinking(bool isBlinking) noexcept
 {
-    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Blinking, isBlinking);
+    WI_UpdateFlag(_attrs, CharacterAttributes::Blinking, isBlinking);
 }
 
 void TextAttribute::SetInvisible(bool isInvisible) noexcept
 {
-    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Invisible, isInvisible);
+    WI_UpdateFlag(_attrs, CharacterAttributes::Invisible, isInvisible);
 }
 
 void TextAttribute::SetCrossedOut(bool isCrossedOut) noexcept
 {
-    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::CrossedOut, isCrossedOut);
+    WI_UpdateFlag(_attrs, CharacterAttributes::CrossedOut, isCrossedOut);
 }
 
 void TextAttribute::SetUnderlined(bool isUnderlined) noexcept
 {
-    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Underlined, isUnderlined);
+    WI_UpdateFlag(_attrs, CharacterAttributes::Underlined, isUnderlined);
 }
 
 void TextAttribute::SetDoublyUnderlined(bool isDoublyUnderlined) noexcept
 {
-    WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::DoublyUnderlined, isDoublyUnderlined);
+    WI_UpdateFlag(_attrs, CharacterAttributes::DoublyUnderlined, isDoublyUnderlined);
 }
 
 void TextAttribute::SetOverlined(bool isOverlined) noexcept
 {
-    WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_GRID_HORIZONTAL, isOverlined);
+    WI_UpdateFlag(_attrs, CharacterAttributes::TopGridline, isOverlined);
 }
 
 void TextAttribute::SetReverseVideo(bool isReversed) noexcept
 {
-    WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO, isReversed);
+    WI_UpdateFlag(_attrs, CharacterAttributes::ReverseVideo, isReversed);
 }
 
 // Routine Description:
 // - swaps foreground and background color
 void TextAttribute::Invert() noexcept
 {
-    WI_ToggleFlag(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO);
+    WI_ToggleFlag(_attrs, CharacterAttributes::ReverseVideo);
 }
 
 void TextAttribute::SetDefaultForeground() noexcept
@@ -375,11 +365,10 @@ void TextAttribute::SetDefaultBackground() noexcept
 }
 
 // Method description:
-// - Resets only the meta and extended attributes
-void TextAttribute::SetDefaultMetaAttrs() noexcept
+// - Resets only the rendition character attributes
+void TextAttribute::SetDefaultRenditionAttributes() noexcept
 {
-    _extendedAttrs = ExtendedAttributes::Normal;
-    _wAttrLegacy = 0;
+    _attrs = CharacterAttributes::Normal;
 }
 
 // Method Description:
@@ -398,10 +387,11 @@ bool TextAttribute::BackgroundIsDefault() const noexcept
 }
 
 // Routine Description:
-// - Resets the meta and extended attributes, which is what the VT standard
-//      requires for most erasing and filling operations.
+// - Resets the character attributes, which is what the VT standard
+//      requires for most erasing and filling operations. In modern
+//      applications it is also expected that hyperlinks are erased.
 void TextAttribute::SetStandardErase() noexcept
 {
-    SetDefaultMetaAttrs();
+    _attrs = CharacterAttributes::Normal;
     _hyperlinkId = 0;
 }

src/buffer/out/TextAttribute.hpp

@@ -31,31 +31,26 @@ class TextAttribute final
 {
 public:
     constexpr TextAttribute() noexcept :
-        _wAttrLegacy{ 0 },
+        _attrs{ CharacterAttributes::Normal },
         _foreground{},
         _background{},
-        _extendedAttrs{ ExtendedAttributes::Normal },
         _hyperlinkId{ 0 }
     {
     }
 
     explicit constexpr TextAttribute(const WORD wLegacyAttr) noexcept :
-        _wAttrLegacy{ gsl::narrow_cast<WORD>(wLegacyAttr & META_ATTRS) },
+        _attrs{ gsl::narrow_cast<WORD>(wLegacyAttr & USED_META_ATTRS) },
         _foreground{ gsl::at(s_legacyForegroundColorMap, wLegacyAttr & FG_ATTRS) },
         _background{ gsl::at(s_legacyBackgroundColorMap, (wLegacyAttr & BG_ATTRS) >> 4) },
-        _extendedAttrs{ ExtendedAttributes::Normal },
         _hyperlinkId{ 0 }
     {
-        // If we're given lead/trailing byte information with the legacy color, strip it.
-        WI_ClearAllFlags(_wAttrLegacy, COMMON_LVB_SBCSDBCS);
     }
 
     constexpr TextAttribute(const COLORREF rgbForeground,
                             const COLORREF rgbBackground) noexcept :
-        _wAttrLegacy{ 0 },
+        _attrs{ CharacterAttributes::Normal },
         _foreground{ rgbForeground },
         _background{ rgbBackground },
-        _extendedAttrs{ ExtendedAttributes::Normal },
         _hyperlinkId{ 0 }
     {
     }
@@ -64,8 +59,6 @@ class TextAttribute final
     static TextAttribute StripErroneousVT16VersionsOfLegacyDefaults(const TextAttribute& attribute) noexcept;
     WORD GetLegacyAttributes() const noexcept;
 
-    bool IsLeadingByte() const noexcept;
-    bool IsTrailingByte() const noexcept;
     bool IsTopHorizontalDisplayed() const noexcept;
     bool IsBottomHorizontalDisplayed() const noexcept;
     bool IsLeftVerticalDisplayed() const noexcept;
@@ -109,9 +102,9 @@ class TextAttribute final
     void SetOverlined(bool isOverlined) noexcept;
     void SetReverseVideo(bool isReversed) noexcept;
 
-    constexpr ExtendedAttributes GetExtendedAttributes() const noexcept
+    constexpr CharacterAttributes GetCharacterAttributes() const noexcept
     {
-        return _extendedAttrs;
+        return _attrs;
     }
 
     bool IsHyperlink() const noexcept;
@@ -132,7 +125,7 @@ class TextAttribute final
 
     void SetDefaultForeground() noexcept;
     void SetDefaultBackground() noexcept;
-    void SetDefaultMetaAttrs() noexcept;
+    void SetDefaultRenditionAttributes() noexcept;
 
     bool BackgroundIsDefault() const noexcept;
 
@@ -149,38 +142,33 @@ class TextAttribute final
         const auto checkForeground = (inverted != IsReverseVideo());
         return !IsAnyGridLineEnabled() && // grid lines have a visual representation
                // crossed out, doubly and singly underlined have a visual representation
-               WI_AreAllFlagsClear(_extendedAttrs, ExtendedAttributes::CrossedOut | ExtendedAttributes::DoublyUnderlined | ExtendedAttributes::Underlined) &&
+               WI_AreAllFlagsClear(_attrs, CharacterAttributes::CrossedOut | CharacterAttributes::DoublyUnderlined | CharacterAttributes::Underlined) &&
                // hyperlinks have a visual representation
                !IsHyperlink() &&
                // all other attributes do not have a visual representation
-               (_wAttrLegacy & META_ATTRS) == (other._wAttrLegacy & META_ATTRS) &&
+               _attrs == other._attrs &&
                ((checkForeground && _foreground == other._foreground) ||
                 (!checkForeground && _background == other._background)) &&
-               _extendedAttrs == other._extendedAttrs &&
                IsHyperlink() == other.IsHyperlink();
     }
 
     constexpr bool IsAnyGridLineEnabled() const noexcept
     {
-        return WI_IsAnyFlagSet(_wAttrLegacy, COMMON_LVB_GRID_HORIZONTAL | COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_UNDERSCORE);
+        return WI_IsAnyFlagSet(_attrs, CharacterAttributes::TopGridline | CharacterAttributes::LeftGridline | CharacterAttributes::RightGridline | CharacterAttributes::BottomGridline);
     }
-    constexpr bool HasAnyExtendedAttributes() const noexcept
+    constexpr bool HasAnyVisualAttributes() const noexcept
     {
-        return GetExtendedAttributes() != ExtendedAttributes::Normal ||
-               IsAnyGridLineEnabled() ||
-               GetHyperlinkId() != 0 ||
-               IsReverseVideo();
+        return GetCharacterAttributes() != CharacterAttributes::Normal || GetHyperlinkId() != 0;
     }
 
 private:
     static std::array<TextColor, 16> s_legacyForegroundColorMap;
     static std::array<TextColor, 16> s_legacyBackgroundColorMap;
 
-    uint16_t _wAttrLegacy; // sizeof: 2, alignof: 2
+    CharacterAttributes _attrs; // sizeof: 2, alignof: 2
     uint16_t _hyperlinkId; // sizeof: 2, alignof: 2
     TextColor _foreground; // sizeof: 4, alignof: 1
     TextColor _background; // sizeof: 4, alignof: 1
-    ExtendedAttributes _extendedAttrs; // sizeof: 2, alignof: 2
 
 #ifdef UNIT_TESTING
     friend class TextBufferTests;
@@ -213,12 +201,11 @@ namespace WEX
             static WEX::Common::NoThrowString ToString(const TextAttribute& attr)
             {
                 return WEX::Common::NoThrowString().Format(
-                    L"{FG:%s,BG:%s,intense:%d,wLegacy:(0x%04x),ext:(0x%02x)}",
+                    L"{FG:%s,BG:%s,intense:%d,attrs:(0x%02x)}",
                     VerifyOutputTraits<TextColor>::ToString(attr._foreground).GetBuffer(),
                     VerifyOutputTraits<TextColor>::ToString(attr._background).GetBuffer(),
                     attr.IsIntense(),
-                    attr._wAttrLegacy,
-                    static_cast<DWORD>(attr._extendedAttrs));
+                    static_cast<DWORD>(attr._attrs));
             }
         };
     }

src/buffer/out/ut_textbuffer/TextAttributeTests.cpp

@@ -75,7 +75,7 @@ void TextAttributeTests::TestRoundtripMetaBits()
         auto attr = TextAttribute(expectedLegacy);
         VERIFY_IS_TRUE(attr.IsLegacy());
         VERIFY_ARE_EQUAL(expectedLegacy, attr.GetLegacyAttributes());
-        VERIFY_ARE_EQUAL(flag, attr._wAttrLegacy);
+        VERIFY_ARE_EQUAL(flag, static_cast<WORD>(attr._attrs));
     }
 }