Refactor the renderer color calculations (#6853)

This is a refactoring of the renderer color calculations to simplify the
implementation, and to make it easier to support additional
color-altering rendition attributes in the future (e.g. _faint_ and
_conceal_).

## References

* This is a followup to PRs #3817 and #6809, which introduced additional
  complexity in the color calculations, and which suggested the need for
  refactoring. 

## Detailed Description of the Pull Request / Additional comments

When we added support for `DECSCNM`, that required the foreground and
background color lookup methods to be able to return the opposite of
what was requested when the reversed mode was set. That made those
methods unnecessarily complicated, and I thought we could simplify them
considerably just by combining the calculations into a single method
that derived both colors at the same time.

And since both conhost and Windows Terminal needed to perform the same
calculations, it also made sense to move that functionality into the
`TextAttribute` class, where it could easily be shared.

In general this way of doing things is a bit more efficient. However, it
does result in some unnecessary work when only one of the colors is
required, as is the case for the gridline painter. So to make that less
of an issue, I've reordered the gridline code a bit so it at least
avoids looking up the colors when no gridlines are needed.

## Validation Steps Performed

Because of the API changes, quite a lot of the unit tests had to be
updated. For example instead of verifying colors with two separate calls
to `LookupForegroundColor` and `LookupBackgroundColor`, that's now
achieved with a single `LookupAttributeColors` call, comparing against a
pair of values. The specifics of the tests haven't changed though, and
they're all still working as expected.

I've also manually confirmed that the various color sequences and
rendition attributes are rendering correctly with the new refactoring.

src/buffer/out/TextAttribute.cpp

@@ -81,54 +81,27 @@ bool TextAttribute::IsLegacy() const noexcept
     return _foreground.IsLegacy() && _background.IsLegacy();
 }
 
-// Arguments:
-// - None
-// Return Value:
-// - color that should be displayed as the foreground color
-COLORREF TextAttribute::CalculateRgbForeground(std::basic_string_view<COLORREF> colorTable,
-                                               COLORREF defaultFgColor,
-                                               COLORREF defaultBgColor) const noexcept
-{
-    return IsReverseVideo() ? _GetRgbBackground(colorTable, defaultBgColor) : _GetRgbForeground(colorTable, defaultFgColor);
-}
-
-// Routine Description:
-// - Calculates rgb background color based off of current color table and active modification attributes
-// Arguments:
-// - None
-// Return Value:
-// - color that should be displayed as the background color
-COLORREF TextAttribute::CalculateRgbBackground(std::basic_string_view<COLORREF> colorTable,
-                                               COLORREF defaultFgColor,
-                                               COLORREF defaultBgColor) const noexcept
-{
-    return IsReverseVideo() ? _GetRgbForeground(colorTable, defaultFgColor) : _GetRgbBackground(colorTable, defaultBgColor);
-}
-
-// Routine Description:
-// - gets rgb foreground color, possibly based off of current color table. Does not take active modification
-// attributes into account
-// Arguments:
-// - None
-// Return Value:
-// - color that is stored as the foreground color
-COLORREF TextAttribute::_GetRgbForeground(std::basic_string_view<COLORREF> colorTable,
-                                          COLORREF defaultColor) const noexcept
-{
-    return _foreground.GetColor(colorTable, defaultColor, IsBold());
-}
-
 // Routine Description:
-// - gets rgb background color, possibly based off of current color table. Does not take active modification
-// attributes into account
+// - Calculates rgb colors based off of current color table and active modification attributes.
 // Arguments:
-// - None
+// - colorTable: the current color table rgb values.
+// - defaultFgColor: the default foreground color rgb value.
+// - defaultBgColor: the default background color rgb value.
+// - reverseScreenMode: true if the screen mode is reversed.
 // Return Value:
-// - color that is stored as the background color
-COLORREF TextAttribute::_GetRgbBackground(std::basic_string_view<COLORREF> colorTable,
-                                          COLORREF defaultColor) const noexcept
-{
-    return _background.GetColor(colorTable, defaultColor, false);
+// - the foreground and background colors that should be displayed.
+std::pair<COLORREF, COLORREF> TextAttribute::CalculateRgbColors(const std::basic_string_view<COLORREF> colorTable,
+                                                                const COLORREF defaultFgColor,
+                                                                const COLORREF defaultBgColor,
+                                                                const bool reverseScreenMode) const noexcept
+{
+    auto fg = _foreground.GetColor(colorTable, defaultFgColor, IsBold());
+    auto bg = _background.GetColor(colorTable, defaultBgColor);
+    if (IsReverseVideo() ^ reverseScreenMode)
+    {
+        std::swap(fg, bg);
+    }
+    return { fg, bg };
 }
 
 TextColor TextAttribute::GetForeground() const noexcept

src/buffer/out/TextAttribute.hpp

@@ -63,12 +63,10 @@ class TextAttribute final
     static TextAttribute StripErroneousVT16VersionsOfLegacyDefaults(const TextAttribute& attribute) noexcept;
     WORD GetLegacyAttributes() const noexcept;
 
-    COLORREF CalculateRgbForeground(std::basic_string_view<COLORREF> colorTable,
-                                    COLORREF defaultFgColor,
-                                    COLORREF defaultBgColor) const noexcept;
-    COLORREF CalculateRgbBackground(std::basic_string_view<COLORREF> colorTable,
-                                    COLORREF defaultFgColor,
-                                    COLORREF defaultBgColor) const noexcept;
+    std::pair<COLORREF, COLORREF> CalculateRgbColors(const std::basic_string_view<COLORREF> colorTable,
+                                                     const COLORREF defaultFgColor,
+                                                     const COLORREF defaultBgColor,
+                                                     const bool reverseScreenMode = false) const noexcept;
 
     bool IsLeadingByte() const noexcept;
     bool IsTrailingByte() const noexcept;
@@ -154,11 +152,6 @@ class TextAttribute final
     }
 
 private:
-    COLORREF _GetRgbForeground(std::basic_string_view<COLORREF> colorTable,
-                               COLORREF defaultColor) const noexcept;
-    COLORREF _GetRgbBackground(std::basic_string_view<COLORREF> colorTable,
-                               COLORREF defaultColor) const noexcept;
-
     static constexpr TextColor s_LegacyIndexOrDefault(const BYTE requestedIndex, const BYTE defaultIndex)
     {
         return requestedIndex == defaultIndex ? TextColor{} : TextColor{ requestedIndex, true };

src/buffer/out/TextColor.h

@@ -90,7 +90,7 @@ struct TextColor
 
     COLORREF GetColor(std::basic_string_view<COLORREF> colorTable,
                       const COLORREF defaultColor,
-                      const bool brighten) const noexcept;
+                      const bool brighten = false) const noexcept;
 
     BYTE GetLegacyIndex(const BYTE defaultIndex) const noexcept;
 

src/buffer/out/textBuffer.cpp

@@ -1461,18 +1461,16 @@ void TextBuffer::_ExpandTextRow(SMALL_RECT& textRow) const
 // - includeCRLF - inject CRLF pairs to the end of each line
 // - trimTrailingWhitespace - remove the trailing whitespace at the end of each line
 // - textRects - the rectangular regions from which the data will be extracted from the buffer (i.e.: selection rects)
-// - GetForegroundColor - function used to map TextAttribute to RGB COLORREF for foreground color. If null, only extract the text.
-// - GetBackgroundColor - function used to map TextAttribute to RGB COLORREF for background color. If null, only extract the text.
+// - GetAttributeColors - function used to map TextAttribute to RGB COLORREFs. If null, only extract the text.
 // Return Value:
 // - The text, background color, and foreground color data of the selected region of the text buffer.
 const TextBuffer::TextAndColor TextBuffer::GetText(const bool includeCRLF,
                                                    const bool trimTrailingWhitespace,
                                                    const std::vector<SMALL_RECT>& selectionRects,
-                                                   std::function<COLORREF(TextAttribute&)> GetForegroundColor,
-                                                   std::function<COLORREF(TextAttribute&)> GetBackgroundColor) const
+                                                   std::function<std::pair<COLORREF, COLORREF>(const TextAttribute&)> GetAttributeColors) const
 {
     TextAndColor data;
-    const bool copyTextColor = GetForegroundColor && GetBackgroundColor;
+    const bool copyTextColor = GetAttributeColors != nullptr;
 
     // preallocate our vectors to reduce reallocs
     size_t const rows = selectionRects.size();
@@ -1517,9 +1515,8 @@ const TextBuffer::TextAndColor TextBuffer::GetText(const bool includeCRLF,
 
                 if (copyTextColor)
                 {
-                    auto cellData = cell.TextAttr();
-                    COLORREF const CellFgAttr = GetForegroundColor(cellData);
-                    COLORREF const CellBkAttr = GetBackgroundColor(cellData);
+                    const auto cellData = cell.TextAttr();
+                    const auto [CellFgAttr, CellBkAttr] = GetAttributeColors(cellData);
                     for (const wchar_t wch : cell.Chars())
                     {
                         selectionFgAttr.push_back(CellFgAttr);

src/buffer/out/textBuffer.hpp

@@ -152,8 +152,7 @@ class TextBuffer final
     const TextAndColor GetText(const bool lineSelection,
                                const bool trimTrailingWhitespace,
                                const std::vector<SMALL_RECT>& textRects,
-                               std::function<COLORREF(TextAttribute&)> GetForegroundColor = nullptr,
-                               std::function<COLORREF(TextAttribute&)> GetBackgroundColor = nullptr) const;
+                               std::function<std::pair<COLORREF, COLORREF>(const TextAttribute&)> GetAttributeColors = nullptr) const;
 
     static std::string GenHTML(const TextAndColor& rows,
                                const int fontHeightPoints,

src/buffer/out/ut_textbuffer/TextAttributeTests.cpp

@@ -138,21 +138,17 @@ void TextAttributeTests::TestTextAttributeColorGetters()
     //      values when reverse video is not set
     VERIFY_IS_FALSE(attr.IsReverseVideo());
 
-    VERIFY_ARE_EQUAL(red, attr._GetRgbForeground(view, _defaultFg));
-    VERIFY_ARE_EQUAL(red, attr.CalculateRgbForeground(view, _defaultFg, _defaultBg));
-
-    VERIFY_ARE_EQUAL(green, attr._GetRgbBackground(view, _defaultBg));
-    VERIFY_ARE_EQUAL(green, attr.CalculateRgbBackground(view, _defaultFg, _defaultBg));
+    VERIFY_ARE_EQUAL(red, attr.GetForeground().GetColor(view, _defaultFg));
+    VERIFY_ARE_EQUAL(green, attr.GetBackground().GetColor(view, _defaultBg));
+    VERIFY_ARE_EQUAL(std::make_pair(red, green), attr.CalculateRgbColors(view, _defaultFg, _defaultBg));
 
     // with reverse video set, calculated foreground/background values should be
     //      switched while getters stay the same
     attr.SetReverseVideo(true);
 
-    VERIFY_ARE_EQUAL(red, attr._GetRgbForeground(view, _defaultFg));
-    VERIFY_ARE_EQUAL(green, attr.CalculateRgbForeground(view, _defaultFg, _defaultBg));
-
-    VERIFY_ARE_EQUAL(green, attr._GetRgbBackground(view, _defaultBg));
-    VERIFY_ARE_EQUAL(red, attr.CalculateRgbBackground(view, _defaultFg, _defaultBg));
+    VERIFY_ARE_EQUAL(red, attr.GetForeground().GetColor(view, _defaultFg));
+    VERIFY_ARE_EQUAL(green, attr.GetBackground().GetColor(view, _defaultBg));
+    VERIFY_ARE_EQUAL(std::make_pair(green, red), attr.CalculateRgbColors(view, _defaultFg, _defaultBg));
 }
 
 void TextAttributeTests::TestReverseDefaultColors()
@@ -166,42 +162,34 @@ void TextAttributeTests::TestReverseDefaultColors()
     //      values when reverse video is not set
     VERIFY_IS_FALSE(attr.IsReverseVideo());
 
-    VERIFY_ARE_EQUAL(_defaultFg, attr._GetRgbForeground(view, _defaultFg));
-    VERIFY_ARE_EQUAL(_defaultFg, attr.CalculateRgbForeground(view, _defaultFg, _defaultBg));
-
-    VERIFY_ARE_EQUAL(_defaultBg, attr._GetRgbBackground(view, _defaultBg));
-    VERIFY_ARE_EQUAL(_defaultBg, attr.CalculateRgbBackground(view, _defaultFg, _defaultBg));
+    VERIFY_ARE_EQUAL(_defaultFg, attr.GetForeground().GetColor(view, _defaultFg));
+    VERIFY_ARE_EQUAL(_defaultBg, attr.GetBackground().GetColor(view, _defaultBg));
+    VERIFY_ARE_EQUAL(std::make_pair(_defaultFg, _defaultBg), attr.CalculateRgbColors(view, _defaultFg, _defaultBg));
 
     // with reverse video set, calculated foreground/background values should be
     //      switched while getters stay the same
     attr.SetReverseVideo(true);
     VERIFY_IS_TRUE(attr.IsReverseVideo());
 
-    VERIFY_ARE_EQUAL(_defaultFg, attr._GetRgbForeground(view, _defaultFg));
-    VERIFY_ARE_EQUAL(_defaultBg, attr.CalculateRgbForeground(view, _defaultFg, _defaultBg));
-
-    VERIFY_ARE_EQUAL(_defaultBg, attr._GetRgbBackground(view, _defaultBg));
-    VERIFY_ARE_EQUAL(_defaultFg, attr.CalculateRgbBackground(view, _defaultFg, _defaultBg));
+    VERIFY_ARE_EQUAL(_defaultFg, attr.GetForeground().GetColor(view, _defaultFg));
+    VERIFY_ARE_EQUAL(_defaultBg, attr.GetBackground().GetColor(view, _defaultBg));
+    VERIFY_ARE_EQUAL(std::make_pair(_defaultBg, _defaultFg), attr.CalculateRgbColors(view, _defaultFg, _defaultBg));
 
     attr.SetForeground(red);
     VERIFY_IS_TRUE(attr.IsReverseVideo());
 
-    VERIFY_ARE_EQUAL(red, attr._GetRgbForeground(view, _defaultFg));
-    VERIFY_ARE_EQUAL(_defaultBg, attr.CalculateRgbForeground(view, _defaultFg, _defaultBg));
-
-    VERIFY_ARE_EQUAL(_defaultBg, attr._GetRgbBackground(view, _defaultBg));
-    VERIFY_ARE_EQUAL(red, attr.CalculateRgbBackground(view, _defaultFg, _defaultBg));
+    VERIFY_ARE_EQUAL(red, attr.GetForeground().GetColor(view, _defaultFg));
+    VERIFY_ARE_EQUAL(_defaultBg, attr.GetBackground().GetColor(view, _defaultBg));
+    VERIFY_ARE_EQUAL(std::make_pair(_defaultBg, red), attr.CalculateRgbColors(view, _defaultFg, _defaultBg));
 
     attr.Invert();
     VERIFY_IS_FALSE(attr.IsReverseVideo());
     attr.SetDefaultForeground();
     attr.SetBackground(green);
 
-    VERIFY_ARE_EQUAL(_defaultFg, attr._GetRgbForeground(view, _defaultFg));
-    VERIFY_ARE_EQUAL(_defaultFg, attr.CalculateRgbForeground(view, _defaultFg, _defaultBg));
-
-    VERIFY_ARE_EQUAL(green, attr._GetRgbBackground(view, _defaultBg));
-    VERIFY_ARE_EQUAL(green, attr.CalculateRgbBackground(view, _defaultFg, _defaultBg));
+    VERIFY_ARE_EQUAL(_defaultFg, attr.GetForeground().GetColor(view, _defaultFg));
+    VERIFY_ARE_EQUAL(green, attr.GetBackground().GetColor(view, _defaultBg));
+    VERIFY_ARE_EQUAL(std::make_pair(_defaultFg, green), attr.CalculateRgbColors(view, _defaultFg, _defaultBg));
 }
 
 void TextAttributeTests::TestRoundtripDefaultColors()

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

@@ -732,7 +732,7 @@ try
         {
             const auto& fontData = _actualFont;
             int const iFontHeightPoints = fontData.GetUnscaledSize().Y; // this renderer uses points already
-            const COLORREF bgColor = _terminal->GetBackgroundColor(_terminal->GetDefaultBrushColors());
+            const COLORREF bgColor = _terminal->GetAttributeColors(_terminal->GetDefaultBrushColors()).second;
 
             std::string HTMLToPlaceOnClip = TextBuffer::GenHTML(rows, iFontHeightPoints, fontData.GetFaceName(), bgColor);
             _CopyToSystemClipboard(HTMLToPlaceOnClip, L"HTML Format");

src/cascadia/TerminalCore/Terminal.hpp

@@ -139,8 +139,7 @@ class Microsoft::Terminal::Core::Terminal final :
 #pragma region IRenderData
     // These methods are defined in TerminalRenderData.cpp
     const TextAttribute GetDefaultBrushColors() noexcept override;
-    const COLORREF GetForegroundColor(const TextAttribute& attr) const noexcept override;
-    const COLORREF GetBackgroundColor(const TextAttribute& attr) const noexcept override;
+    std::pair<COLORREF, COLORREF> GetAttributeColors(const TextAttribute& attr) const noexcept override;
     COORD GetCursorPosition() const noexcept override;
     bool IsCursorVisible() const noexcept override;
     bool IsCursorOn() const noexcept override;

src/cascadia/TerminalCore/TerminalSelection.cpp

@@ -253,14 +253,12 @@ const TextBuffer::TextAndColor Terminal::RetrieveSelectedTextFromBuffer(bool sin
 {
     const auto selectionRects = _GetSelectionRects();
 
-    std::function<COLORREF(TextAttribute&)> GetForegroundColor = std::bind(&Terminal::GetForegroundColor, this, std::placeholders::_1);
-    std::function<COLORREF(TextAttribute&)> GetBackgroundColor = std::bind(&Terminal::GetBackgroundColor, this, std::placeholders::_1);
+    const auto GetAttributeColors = std::bind(&Terminal::GetAttributeColors, this, std::placeholders::_1);
 
     return _buffer->GetText(!singleLine,
                             !singleLine,
                             selectionRects,
-                            GetForegroundColor,
-                            GetBackgroundColor);
+                            GetAttributeColors);
 }
 
 // Method Description:

src/cascadia/TerminalCore/terminalrenderdata.cpp

@@ -50,38 +50,20 @@ const TextAttribute Terminal::GetDefaultBrushColors() noexcept
     return TextAttribute{};
 }
 
-const COLORREF Terminal::GetForegroundColor(const TextAttribute& attr) const noexcept
+std::pair<COLORREF, COLORREF> Terminal::GetAttributeColors(const TextAttribute& attr) const noexcept
 {
-    COLORREF fgColor{};
-    if (_screenReversed)
-    {
-        fgColor = attr.CalculateRgbBackground({ _colorTable.data(), _colorTable.size() }, _defaultFg, _defaultBg);
-    }
-    else
-    {
-        fgColor = attr.CalculateRgbForeground({ _colorTable.data(), _colorTable.size() }, _defaultFg, _defaultBg);
-    }
-    return 0xff000000 | fgColor;
-}
-
-const COLORREF Terminal::GetBackgroundColor(const TextAttribute& attr) const noexcept
-{
-    COLORREF bgColor{};
-    if (_screenReversed)
-    {
-        bgColor = attr.CalculateRgbForeground({ _colorTable.data(), _colorTable.size() }, _defaultFg, _defaultBg);
-    }
-    else
-    {
-        bgColor = attr.CalculateRgbBackground({ _colorTable.data(), _colorTable.size() }, _defaultFg, _defaultBg);
-    }
+    auto colors = attr.CalculateRgbColors({ _colorTable.data(), _colorTable.size() },
+                                          _defaultFg,
+                                          _defaultBg,
+                                          _screenReversed);
+    colors.first |= 0xff000000;
     // We only care about alpha for the default BG (which enables acrylic)
     // If the bg isn't the default bg color, or reverse video is enabled, make it fully opaque.
     if (!attr.BackgroundIsDefault() || (attr.IsReverseVideo() ^ _screenReversed))
     {
-        return 0xff000000 | bgColor;
+        colors.second |= 0xff000000;
     }
-    return bgColor;
+    return colors;
 }
 
 COORD Terminal::GetCursorPosition() const noexcept

src/host/renderData.cpp

@@ -325,25 +325,14 @@ const std::wstring RenderData::GetConsoleTitle() const noexcept
 }
 
 // Routine Description:
-// - Converts a text attribute into the foreground RGB value that should be presented, applying
+// - Converts a text attribute into the RGB values that should be presented, applying
 //   relevant table translation information and preferences.
 // Return Value:
-// - ARGB color value
-const COLORREF RenderData::GetForegroundColor(const TextAttribute& attr) const noexcept
+// - ARGB color values for the foreground and background
+std::pair<COLORREF, COLORREF> RenderData::GetAttributeColors(const TextAttribute& attr) const noexcept
 {
     const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-    return gci.LookupForegroundColor(attr);
-}
-
-// Routine Description:
-// - Converts a text attribute into the background RGB value that should be presented, applying
-//   relevant table translation information and preferences.
-// Return Value:
-// - ARGB color value
-const COLORREF RenderData::GetBackgroundColor(const TextAttribute& attr) const noexcept
-{
-    const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-    return gci.LookupBackgroundColor(attr);
+    return gci.LookupAttributeColors(attr);
 }
 #pragma endregion
 

src/host/renderData.hpp

@@ -37,8 +37,7 @@ class RenderData final :
 #pragma region IRenderData
     const TextAttribute GetDefaultBrushColors() noexcept override;
 
-    const COLORREF GetForegroundColor(const TextAttribute& attr) const noexcept override;
-    const COLORREF GetBackgroundColor(const TextAttribute& attr) const noexcept override;
+    std::pair<COLORREF, COLORREF> GetAttributeColors(const TextAttribute& attr) const noexcept override;
 
     COORD GetCursorPosition() const noexcept override;
     bool IsCursorVisible() const noexcept override;