Use the entire 256 table to lookup the color

Fixes #1223

  We'll lookup a color (for rendering) using the _entire_ 256 color table.
  When setting a 256 color, we'll set the TextColor to that index, instead of
  the current color at that index. Everything else is unchanged.

  **TODO**: This probably affects which TextAttributes will pass IsLegacy.
  256 color ones will now be "legacy". This is bad, we should make sure to
  not leave it that way.

src/host/VtIo.cpp

@@ -161,14 +161,14 @@ VtIo::VtIo() :
                                                                     gci,
                                                                     initialViewport,
                                                                     gci.GetColorTable(),
-                                                                    static_cast<WORD>(gci.GetColorTableSize()));
+                                                                    static_cast<WORD>(gci.GetLegacyColorTableSize()));
                 break;
             case VtIoMode::XTERM:
                 _pVtRenderEngine = std::make_unique<XtermEngine>(std::move(_hOutput),
                                                                  gci,
                                                                  initialViewport,
                                                                  gci.GetColorTable(),
-                                                                 static_cast<WORD>(gci.GetColorTableSize()),
+                                                                 static_cast<WORD>(gci.GetLegacyColorTableSize()),
                                                                  false);
                 break;
             case VtIoMode::XTERM_ASCII:
@@ -176,7 +176,7 @@ VtIo::VtIo() :
                                                                  gci,
                                                                  initialViewport,
                                                                  gci.GetColorTable(),
-                                                                 static_cast<WORD>(gci.GetColorTableSize()),
+                                                                 static_cast<WORD>(gci.GetLegacyColorTableSize()),
                                                                  true);
                 break;
             case VtIoMode::WIN_TELNET:
@@ -184,7 +184,7 @@ VtIo::VtIo() :
                                                                      gci,
                                                                      initialViewport,
                                                                      gci.GetColorTable(),
-                                                                     static_cast<WORD>(gci.GetColorTableSize()));
+                                                                     static_cast<WORD>(gci.GetLegacyColorTableSize()));
                 break;
             default:
                 return E_FAIL;

src/host/getset.cpp

@@ -917,24 +917,38 @@ void DoSrvPrivateSetConsoleXtermTextAttribute(SCREEN_INFORMATION& screenInfo,
                                               const int iXtermTableEntry,
                                               const bool fIsForeground)
 {
-    const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+    // const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     auto& buffer = screenInfo.GetActiveBuffer();
     TextAttribute NewAttributes = buffer.GetAttributes();
 
-    COLORREF rgbColor;
-    if (iXtermTableEntry < COLOR_TABLE_SIZE)
-    {
-        //Convert the xterm index to the win index
-        WORD iWinEntry = ::XtermToWindowsIndex(iXtermTableEntry);
+    // ////////////////////////
+    // COLORREF rgbColor;
+    // if (iXtermTableEntry < COLOR_TABLE_SIZE)
+    // {
+    //     //Convert the xterm index to the win index
+    //     WORD iWinEntry = ::XtermToWindowsIndex(iXtermTableEntry);
 
-        rgbColor = gci.GetColorTableEntry(iWinEntry);
+    //     rgbColor = gci.GetColorTableEntry(iWinEntry);
+    // }
+    // else
+    // {
+    //     rgbColor = gci.GetColorTableEntry(iXtermTableEntry);
+    // }
+
+    // NewAttributes.SetColor(rgbColor, fIsForeground);
+    // ////////////////////////
+    std::optional<BYTE> newFG{ std::nullopt }; // = fIsForeground ? iXtermTableEntry : std::nullopt;
+    std::optional<BYTE> newBG{ std::nullopt }; // = fIsForeground ? std::nullopt : iXtermTableEntry;
+    if (fIsForeground)
+    {
+        newFG = static_cast<BYTE>(iXtermTableEntry);
     }
     else
     {
-        rgbColor = gci.GetColorTableEntry(iXtermTableEntry);
+        newBG = static_cast<BYTE>(iXtermTableEntry);
     }
 
-    NewAttributes.SetColor(rgbColor, fIsForeground);
+    NewAttributes.SetIndexedAttributes(newFG, newBG);
 
     buffer.SetAttributes(NewAttributes);
 }

src/host/settings.cpp

@@ -81,8 +81,8 @@ Settings::Settings() :
     _CursorColor = Cursor::s_InvertCursorColor;
     _CursorType = CursorType::Legacy;
 
-    gsl::span<COLORREF> tableView = { _ColorTable, gsl::narrow<ptrdiff_t>(COLOR_TABLE_SIZE) };
-    gsl::span<COLORREF> xtermTableView = { _XtermColorTable, gsl::narrow<ptrdiff_t>(XTERM_COLOR_TABLE_SIZE) };
+    gsl::span<COLORREF> tableView = { _256ColorTable, gsl::narrow<ptrdiff_t>(COLOR_TABLE_SIZE) };
+    gsl::span<COLORREF> xtermTableView = { _256ColorTable, gsl::narrow<ptrdiff_t>(XTERM_COLOR_TABLE_SIZE) };
     ::Microsoft::Console::Utils::Initialize256ColorTable(xtermTableView);
     ::Microsoft::Console::Utils::InitializeCampbellColorTableForConhost(tableView);
 }
@@ -123,8 +123,9 @@ void Settings::ApplyDesktopSpecificDefaults()
     _uNumberOfHistoryBuffers = 4;
     _bHistoryNoDup = FALSE;
 
-    gsl::span<COLORREF> tableView = { _ColorTable, gsl::narrow<ptrdiff_t>(COLOR_TABLE_SIZE) };
+    gsl::span<COLORREF> tableView = { _256ColorTable, gsl::narrow<ptrdiff_t>(COLOR_TABLE_SIZE) };
     ::Microsoft::Console::Utils::InitializeCampbellColorTableForConhost(tableView);
+    // We don't need to worry about the 256 colors, because they're always set in the ctor.
 
     _fTrimLeadingZeros = false;
     _fEnableColorSelection = false;
@@ -221,7 +222,7 @@ void Settings::InitFromStateInfo(_In_ PCONSOLE_STATE_INFO pStateInfo)
     _bHistoryNoDup = pStateInfo->HistoryNoDup;
     _uHistoryBufferSize = pStateInfo->HistoryBufferSize;
     _uNumberOfHistoryBuffers = pStateInfo->NumberOfHistoryBuffers;
-    memmove(_ColorTable, pStateInfo->ColorTable, sizeof(_ColorTable));
+    memmove(_256ColorTable, pStateInfo->ColorTable, COLOR_TABLE_SIZE * sizeof(COLORREF));
     _uCodePage = pStateInfo->CodePage;
     _bWrapText = !!pStateInfo->fWrapText;
     _fFilterOnPaste = pStateInfo->fFilterOnPaste;
@@ -263,7 +264,8 @@ CONSOLE_STATE_INFO Settings::CreateConsoleStateInfo() const
     csi.HistoryNoDup = _bHistoryNoDup;
     csi.HistoryBufferSize = _uHistoryBufferSize;
     csi.NumberOfHistoryBuffers = _uNumberOfHistoryBuffers;
-    memmove(csi.ColorTable, _ColorTable, sizeof(_ColorTable));
+    // memmove(csi.ColorTable, _ColorTable, sizeof(_ColorTable));
+    memmove(csi.ColorTable, _256ColorTable, COLOR_TABLE_SIZE * sizeof(COLORREF));
     csi.CodePage = _uCodePage;
     csi.fWrapText = !!_bWrapText;
     csi.fFilterOnPaste = _fFilterOnPaste;
@@ -691,24 +693,36 @@ void Settings::SetHistoryNoDup(const bool bHistoryNoDup)
 
 const COLORREF* const Settings::GetColorTable() const
 {
-    return _ColorTable;
+    return _256ColorTable;
 }
+
+// Method Description:
+// - Sets cSize entries of the colortable, by copying the values from
+//   pColorTable into our color table. Copies at most COLOR_TABLE_SIZE (16)
+//   entries.
+// Arguments:
+// - pColorTable: The array of COLORREFs to copy from
+// - cSize the number of entries in pColorTable
+// Return Value:
+// - <none>
 void Settings::SetColorTable(_In_reads_(cSize) const COLORREF* const pColorTable, const size_t cSize)
 {
     size_t cSizeWritten = std::min(cSize, static_cast<size_t>(COLOR_TABLE_SIZE));
 
-    memmove(_ColorTable, pColorTable, cSizeWritten * sizeof(COLORREF));
+    // memmove(_ColorTable, pColorTable, cSizeWritten * sizeof(COLORREF));
+    memmove(_256ColorTable, pColorTable, cSizeWritten * sizeof(COLORREF));
 }
 void Settings::SetColorTableEntry(const size_t index, const COLORREF ColorValue)
 {
-    if (index < ARRAYSIZE(_ColorTable))
-    {
-        _ColorTable[index] = ColorValue;
-    }
-    else
-    {
-        _XtermColorTable[index] = ColorValue;
-    }
+    _256ColorTable[index] = ColorValue;
+    // if (index < ARRAYSIZE(_ColorTable))
+    // {
+    //     _ColorTable[index] = ColorValue;
+    // }
+    // else
+    // {
+    //     _XtermColorTable[index] = ColorValue;
+    // }
 }
 
 bool Settings::IsStartupTitleIsLinkNameSet() const
@@ -726,21 +740,29 @@ void Settings::UnsetStartupFlag(const DWORD dwFlagToUnset)
     _dwStartupFlags &= ~dwFlagToUnset;
 }
 
+const size_t Settings::GetLegacyColorTableSize() const
+{
+    return COLOR_TABLE_SIZE;
+    // return ARRAYSIZE(_ColorTable);
+}
+
 const size_t Settings::GetColorTableSize() const
 {
-    return ARRAYSIZE(_ColorTable);
+    return XTERM_COLOR_TABLE_SIZE;
+    // return ARRAYSIZE(_ColorTable);
 }
 
 COLORREF Settings::GetColorTableEntry(const size_t index) const
 {
-    if (index < ARRAYSIZE(_ColorTable))
-    {
-        return _ColorTable[index];
-    }
-    else
-    {
-        return _XtermColorTable[index];
-    }
+    return _256ColorTable[index];
+    // if (index < ARRAYSIZE(_ColorTable))
+    // {
+    //     return _ColorTable[index];
+    // }
+    // else
+    // {
+    //     return _XtermColorTable[index];
+    // }
 }
 
 // Routine Description:
@@ -797,7 +819,8 @@ WORD Settings::GenerateLegacyAttributes(const TextAttribute attributes) const
 // The index in ColorTable of the nearest match to Color.
 WORD Settings::FindNearestTableIndex(const COLORREF Color) const
 {
-    return ::FindNearestTableIndex(Color, _ColorTable, ARRAYSIZE(_ColorTable));
+    // return ::FindNearestTableIndex(Color, _ColorTable, ARRAYSIZE(_ColorTable));
+    return ::FindNearestTableIndex(Color, GetColorTable(), COLOR_TABLE_SIZE);
 }
 
 COLORREF Settings::GetCursorColor() const noexcept
@@ -896,7 +919,7 @@ bool Settings::GetUseDx() const noexcept
 COLORREF Settings::CalculateDefaultForeground() const noexcept
 {
     const auto fg = GetDefaultForegroundColor();
-    return fg != INVALID_COLOR ? fg : ForegroundColor(GetFillAttribute(), GetColorTable(), GetColorTableSize());
+    return fg != INVALID_COLOR ? fg : ForegroundColor(GetFillAttribute(), GetColorTable(), GetLegacyColorTableSize());
 }
 
 // Method Description:
@@ -911,7 +934,7 @@ COLORREF Settings::CalculateDefaultForeground() const noexcept
 COLORREF Settings::CalculateDefaultBackground() const noexcept
 {
     const auto bg = GetDefaultBackgroundColor();
-    return bg != INVALID_COLOR ? bg : BackgroundColor(GetFillAttribute(), GetColorTable(), GetColorTableSize());
+    return bg != INVALID_COLOR ? bg : BackgroundColor(GetFillAttribute(), GetColorTable(), GetLegacyColorTableSize());
 }
 
 // Method Description:

src/host/settings.hpp

@@ -157,6 +157,7 @@ public:
     void SetHistoryNoDup(const bool fHistoryNoDup);
 
     const COLORREF* const GetColorTable() const;
+    const size_t GetLegacyColorTableSize() const;
     const size_t GetColorTableSize() const;
     void SetColorTable(_In_reads_(cSize) const COLORREF* const pColorTable, const size_t cSize);
     void SetColorTableEntry(const size_t index, const COLORREF ColorValue);
@@ -214,7 +215,8 @@ private:
     UINT _uHistoryBufferSize;
     UINT _uNumberOfHistoryBuffers;
     BOOL _bHistoryNoDup;
-    COLORREF _ColorTable[COLOR_TABLE_SIZE];
+    // The memcpy will end after COLOR_TABLE_SIZE entries in _256ColorTable.
+    COLORREF _256ColorTable[XTERM_COLOR_TABLE_SIZE];
     // END - memcpy
     UINT _uCodePage;
     UINT _uScrollScale;
@@ -234,7 +236,7 @@ private:
     bool _fUseDx;
     bool _fCopyColor;
 
-    COLORREF _XtermColorTable[XTERM_COLOR_TABLE_SIZE];
+    // COLORREF _XtermColorTable[XTERM_COLOR_TABLE_SIZE];
 
     // this is used for the special STARTF_USESIZE mode.
     bool _fUseWindowSizePixels;

src/host/telemetry.cpp

@@ -419,7 +419,7 @@ void Telemetry::WriteFinalTraceLog()
                                     TraceLoggingBool(gci.GetQuickEdit(), "QuickEdit"),
                                     TraceLoggingValue(gci.GetWindowAlpha(), "WindowAlpha"),
                                     TraceLoggingBool(gci.GetWrapText(), "WrapText"),
-                                    TraceLoggingUInt32Array((UINT32 const*)gci.GetColorTable(), (UINT16)gci.GetColorTableSize(), "ColorTable"),
+                                    TraceLoggingUInt32Array((UINT32 const*)gci.GetColorTable(), (UINT16)gci.GetLegacyColorTableSize(), "ColorTable"),
                                     TraceLoggingValue(gci.CP, "CodePageInput"),
                                     TraceLoggingValue(gci.OutputCP, "CodePageOutput"),
                                     TraceLoggingValue(gci.GetFontSize().X, "FontSizeX"),

src/interactivity/win32/menu.cpp

@@ -337,7 +337,7 @@ void Menu::s_ShowPropertiesDialog(HWND const hwnd, BOOL const Defaults)
     pStateInfo->NumberOfHistoryBuffers = gci.GetNumberOfHistoryBuffers();
     pStateInfo->HistoryNoDup = !!(gci.Flags & CONSOLE_HISTORY_NODUP);
 
-    memmove(pStateInfo->ColorTable, gci.GetColorTable(), gci.GetColorTableSize() * sizeof(COLORREF));
+    memmove(pStateInfo->ColorTable, gci.GetColorTable(), gci.GetLegacyColorTableSize() * sizeof(COLORREF));
 
     // Create mutable copies of the titles so the propsheet can do something with them.
     if (gci.GetOriginalTitle().length() > 0)
@@ -564,7 +564,7 @@ void Menu::s_PropertiesUpdate(PCONSOLE_STATE_INFO pStateInfo)
         }
     }
 
-    gci.SetColorTable(pStateInfo->ColorTable, gci.GetColorTableSize());
+    gci.SetColorTable(pStateInfo->ColorTable, gci.GetLegacyColorTableSize());
 
     // Ensure that attributes only contain color specification.
     WI_ClearAllFlags(pStateInfo->ScreenAttributes, ~(FG_ATTRS | BG_ATTRS));