From aee0e7531a6b0aebc6d2a4476aa0fb1df2c3744a Mon Sep 17 00:00:00 2001 From: James Holderness Date: Mon, 20 Apr 2020 00:43:04 +0100 Subject: [PATCH 01/15] Pass the full text attributes through to the renderer's UpdateDrawingBrushes method. --- src/renderer/base/renderer.cpp | 6 +--- src/renderer/dx/DxRenderer.cpp | 6 ++-- src/renderer/dx/DxRenderer.hpp | 3 +- src/renderer/gdi/gdirenderer.hpp | 3 +- src/renderer/gdi/state.cpp | 10 +++---- src/renderer/inc/IRenderEngine.hpp | 4 +-- src/renderer/uia/UiaRenderer.cpp | 6 ++-- src/renderer/uia/UiaRenderer.hpp | 3 +- src/renderer/vt/Xterm256Engine.cpp | 44 ++++++++++++++---------------- src/renderer/vt/Xterm256Engine.hpp | 7 ++--- src/renderer/vt/XtermEngine.cpp | 19 ++++++------- src/renderer/vt/XtermEngine.hpp | 5 ++-- src/renderer/vt/vtrenderer.hpp | 3 +- 13 files changed, 49 insertions(+), 70 deletions(-) diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index 8ec560a37d2..16a30a77294 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -1090,14 +1090,10 @@ void Renderer::_PaintSelection(_In_ IRenderEngine* const pEngine) { const COLORREF rgbForeground = _pData->GetForegroundColor(textAttributes); const COLORREF rgbBackground = _pData->GetBackgroundColor(textAttributes); - const WORD legacyAttributes = textAttributes.GetLegacyAttributes(); - const auto extendedAttrs = textAttributes.GetExtendedAttributes(); // The last color needs to be each engine's responsibility. If it's local to this function, // then on the next engine we might not update the color. - RETURN_IF_FAILED(pEngine->UpdateDrawingBrushes(rgbForeground, rgbBackground, legacyAttributes, extendedAttrs, isSettingDefaultBrushes)); - - return S_OK; + return pEngine->UpdateDrawingBrushes(rgbForeground, rgbBackground, textAttributes, isSettingDefaultBrushes); } // Routine Description: diff --git a/src/renderer/dx/DxRenderer.cpp b/src/renderer/dx/DxRenderer.cpp index 8966ff18e54..959c1016735 100644 --- a/src/renderer/dx/DxRenderer.cpp +++ b/src/renderer/dx/DxRenderer.cpp @@ -1521,15 +1521,13 @@ CATCH_RETURN() // Arguments: // - colorForeground - Foreground brush color // - colorBackground - Background brush color -// - legacyColorAttribute - -// - extendedAttrs - +// - textAttributes - // - isSettingDefaultBrushes - Lets us know that these are the default brushes to paint the swapchain background or selection // Return Value: // - S_OK or relevant DirectX error. [[nodiscard]] HRESULT DxEngine::UpdateDrawingBrushes(COLORREF const colorForeground, COLORREF const colorBackground, - const WORD /*legacyColorAttribute*/, - const ExtendedAttributes /*extendedAttrs*/, + const TextAttribute& /*textAttributes*/, bool const isSettingDefaultBrushes) noexcept { // GH#5098: If we're rendering with cleartype text, we need to always render diff --git a/src/renderer/dx/DxRenderer.hpp b/src/renderer/dx/DxRenderer.hpp index dc66ae1f668..23b9a19ec0c 100644 --- a/src/renderer/dx/DxRenderer.hpp +++ b/src/renderer/dx/DxRenderer.hpp @@ -95,8 +95,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(COLORREF const colorForeground, COLORREF const colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, bool const isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& fiFontInfoDesired, FontInfo& fiFontInfo) noexcept override; [[nodiscard]] HRESULT UpdateDpi(int const iDpi) noexcept override; diff --git a/src/renderer/gdi/gdirenderer.hpp b/src/renderer/gdi/gdirenderer.hpp index 50f2d543af0..7037aa641b1 100644 --- a/src/renderer/gdi/gdirenderer.hpp +++ b/src/renderer/gdi/gdirenderer.hpp @@ -56,8 +56,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& FontInfoDesired, _Out_ FontInfo& FontInfo) noexcept override; diff --git a/src/renderer/gdi/state.cpp b/src/renderer/gdi/state.cpp index 1053bea5e00..26635983116 100644 --- a/src/renderer/gdi/state.cpp +++ b/src/renderer/gdi/state.cpp @@ -173,18 +173,16 @@ GdiEngine::~GdiEngine() // Routine Description: // - This method will set the GDI brushes in the drawing context (and update the hung-window background color) // Arguments: -// - colorForeground - Foreground Color -// - colorBackground - Background colo -// - legacyColorAttribute - -// - extendedAttrs - +// - colorForeground - Foreground color +// - colorBackground - Background color +// - textAttributes - // - isSettingDefaultBrushes - Lets us know that the default brushes are being set so we can update the DC background // and the hung app background painting color // Return Value: // - S_OK if set successfully or relevant GDI error via HRESULT. [[nodiscard]] HRESULT GdiEngine::UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD /*legacyColorAttribute*/, - const ExtendedAttributes /*extendedAttrs*/, + const TextAttribute& /*textAttributes*/, const bool isSettingDefaultBrushes) noexcept { RETURN_IF_FAILED(_FlushBufferLines()); diff --git a/src/renderer/inc/IRenderEngine.hpp b/src/renderer/inc/IRenderEngine.hpp index 96929dd83f7..f48e46e8c22 100644 --- a/src/renderer/inc/IRenderEngine.hpp +++ b/src/renderer/inc/IRenderEngine.hpp @@ -14,6 +14,7 @@ Author(s): #pragma once +#include "../../buffer/out/TextAttribute.hpp" #include "CursorOptions.h" #include "Cluster.hpp" #include "FontInfoDesired.hpp" @@ -84,8 +85,7 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, const bool isSettingDefaultBrushes) noexcept = 0; [[nodiscard]] virtual HRESULT UpdateFont(const FontInfoDesired& FontInfoDesired, _Out_ FontInfo& FontInfo) noexcept = 0; diff --git a/src/renderer/uia/UiaRenderer.cpp b/src/renderer/uia/UiaRenderer.cpp index 0d04bc9f0d9..a95ae41bc6b 100644 --- a/src/renderer/uia/UiaRenderer.cpp +++ b/src/renderer/uia/UiaRenderer.cpp @@ -363,15 +363,13 @@ CATCH_RETURN(); // Arguments: // - colorForeground - // - colorBackground - -// - legacyColorAttribute - -// - isBold - +// - textAttributes - // - isSettingDefaultBrushes - // Return Value: // - S_FALSE since we do nothing [[nodiscard]] HRESULT UiaEngine::UpdateDrawingBrushes(const COLORREF /*colorForeground*/, const COLORREF /*colorBackground*/, - const WORD /*legacyColorAttribute*/, - const ExtendedAttributes /*extendedAttrs*/, + const TextAttribute& /*textAttributes*/, const bool /*isSettingDefaultBrushes*/) noexcept { return S_FALSE; diff --git a/src/renderer/uia/UiaRenderer.hpp b/src/renderer/uia/UiaRenderer.hpp index f4a011a5285..5ea18ea4eb7 100644 --- a/src/renderer/uia/UiaRenderer.hpp +++ b/src/renderer/uia/UiaRenderer.hpp @@ -62,8 +62,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& fiFontInfoDesired, FontInfo& fiFontInfo) noexcept override; [[nodiscard]] HRESULT UpdateDpi(int const iDpi) noexcept override; diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 2fed81c2ab8..d9aedd80e6e 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -12,8 +12,7 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, const IDefaultColorProvider& colorProvider, const Viewport initialViewport, const std::basic_string_view colorTable) : - XtermEngine(std::move(hPipe), colorProvider, initialViewport, colorTable, false), - _lastExtendedAttrsState{ ExtendedAttributes::Normal } + XtermEngine(std::move(hPipe), colorProvider, initialViewport, colorTable, false) { } @@ -23,17 +22,14 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // Arguments: // - colorForeground: The RGB Color to use to paint the foreground text. // - colorBackground: The RGB Color to use to paint the background of the text. -// - legacyColorAttribute: A console attributes bit field specifying the brush -// colors we should use. -// - extendedAttrs - extended text attributes (italic, underline, etc.) to use. +// - textAttributes - text attributes (bold, italic, underline, etc.) to use. // - isSettingDefaultBrushes: indicates if we should change the background color of // the window. Unused for VT // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT Xterm256Engine::UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, const bool /*isSettingDefaultBrushes*/) noexcept { //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE @@ -43,34 +39,32 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // we'll have already painted the text by the time PaintBufferGridLines // is called. // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE - RETURN_IF_FAILED(_UpdateUnderline(legacyColorAttribute)); + RETURN_IF_FAILED(_UpdateUnderline(textAttributes)); // Only do extended attributes in xterm-256color, as to not break telnet.exe. - RETURN_IF_FAILED(_UpdateExtendedAttrs(extendedAttrs)); + RETURN_IF_FAILED(_UpdateExtendedAttrs(textAttributes)); return VtEngine::_RgbUpdateDrawingBrushes(colorForeground, colorBackground, - WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Bold), + textAttributes.IsBold(), _colorTable); } // Routine Description: // - Write a VT sequence to either start or stop underlining text. // Arguments: -// - legacyColorAttribute: A console attributes bit field containing information -// about the underlining state of the text. +// - textAttributes - text attributes (bold, italic, underline, etc.) to use. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT Xterm256Engine::_UpdateExtendedAttrs(const ExtendedAttributes extendedAttrs) noexcept +[[nodiscard]] HRESULT Xterm256Engine::_UpdateExtendedAttrs(const TextAttribute& textAttributes) noexcept { // Helper lambda to check if a state (attr) has changed since it's last // value (lastState), and appropriately start/end that state with the given // begin/end functions. - auto updateFlagAndState = [extendedAttrs, this](const ExtendedAttributes attr, - std::function beginFn, - std::function endFn) -> HRESULT { - const bool flagSet = WI_AreAllFlagsSet(extendedAttrs, attr); - const bool lastState = WI_AreAllFlagsSet(_lastExtendedAttrsState, attr); + auto updateFlagAndState = [this](const bool flagSet, + const bool lastState, + std::function beginFn, + std::function endFn) -> HRESULT { if (flagSet != lastState) { if (flagSet) @@ -81,31 +75,35 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, { RETURN_IF_FAILED(endFn(this)); } - WI_ToggleAllFlags(_lastExtendedAttrsState, attr); } return S_OK; }; - auto hr = updateFlagAndState(ExtendedAttributes::Italics, + auto hr = updateFlagAndState(textAttributes.IsItalic(), + _lastTextAttributes.IsItalic(), &Xterm256Engine::_BeginItalics, &Xterm256Engine::_EndItalics); RETURN_IF_FAILED(hr); - hr = updateFlagAndState(ExtendedAttributes::Blinking, + hr = updateFlagAndState(textAttributes.IsBlinking(), + _lastTextAttributes.IsBlinking(), &Xterm256Engine::_BeginBlink, &Xterm256Engine::_EndBlink); RETURN_IF_FAILED(hr); - hr = updateFlagAndState(ExtendedAttributes::Invisible, + hr = updateFlagAndState(textAttributes.IsInvisible(), + _lastTextAttributes.IsInvisible(), &Xterm256Engine::_BeginInvisible, &Xterm256Engine::_EndInvisible); RETURN_IF_FAILED(hr); - hr = updateFlagAndState(ExtendedAttributes::CrossedOut, + hr = updateFlagAndState(textAttributes.IsCrossedOut(), + _lastTextAttributes.IsCrossedOut(), &Xterm256Engine::_BeginCrossedOut, &Xterm256Engine::_EndCrossedOut); RETURN_IF_FAILED(hr); + _lastTextAttributes = textAttributes; return S_OK; } diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index e4e5562f6c4..434a466c593 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -32,18 +32,17 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT ManuallyClearScrollback() noexcept override; private: - [[nodiscard]] HRESULT _UpdateExtendedAttrs(const ExtendedAttributes extendedAttrs) noexcept; + [[nodiscard]] HRESULT _UpdateExtendedAttrs(const TextAttribute& textAttributes) noexcept; // We're only using Italics, Blinking, Invisible and Crossed Out for now // See GH#2916 for adding a more complete implementation. - ExtendedAttributes _lastExtendedAttrsState; + TextAttribute _lastTextAttributes; #ifdef UNIT_TESTING friend class VtRendererTest; diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 20d872842f5..3fcd3cafd3f 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -137,13 +137,13 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // Routine Description: // - Write a VT sequence to either start or stop underlining text. // Arguments: -// - legacyColorAttribute: A console attributes bit field containing information -// about the underlining state of the text. +// - textAttributes: text attributes containing information about the +// underlining state of the text. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT XtermEngine::_UpdateUnderline(const WORD legacyColorAttribute) noexcept +[[nodiscard]] HRESULT XtermEngine::_UpdateUnderline(const TextAttribute& textAttributes) noexcept { - bool textUnderlined = WI_IsFlagSet(legacyColorAttribute, COMMON_LVB_UNDERSCORE); + bool textUnderlined = textAttributes.IsUnderlined(); if (textUnderlined != _usingUnderLine) { if (textUnderlined) @@ -165,17 +165,14 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // Arguments: // - colorForeground: The RGB Color to use to paint the foreground text. // - colorBackground: The RGB Color to use to paint the background of the text. -// - legacyColorAttribute: A console attributes bit field specifying the brush -// colors we should use. -// - extendedAttrs - extended text attributes (italic, underline, etc.) to use. +// - textAttributes - text attributes (bold, italic, underline, etc.) to use. // - isSettingDefaultBrushes: indicates if we should change the background color of // the window. Unused for VT // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT XtermEngine::UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, const bool /*isSettingDefaultBrushes*/) noexcept { //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE @@ -185,11 +182,11 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // we'll have already painted the text by the time PaintBufferGridLines // is called. // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE - RETURN_IF_FAILED(_UpdateUnderline(legacyColorAttribute)); + RETURN_IF_FAILED(_UpdateUnderline(textAttributes)); // The base xterm mode only knows about 16 colors return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, colorBackground, - WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Bold), + textAttributes.IsBold(), _colorTable); } diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index cf5249ab6ac..f73e9c0bb39 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -42,8 +42,7 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, @@ -65,7 +64,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT _MoveCursor(const COORD coord) noexcept override; - [[nodiscard]] HRESULT _UpdateUnderline(const WORD wLegacyAttrs) noexcept; + [[nodiscard]] HRESULT _UpdateUnderline(const TextAttribute& textAttributes) noexcept; [[nodiscard]] HRESULT _DoUpdateTitle(const std::wstring& newTitle) noexcept override; diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 613c2048fd4..6213dc3cc24 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -77,8 +77,7 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + const TextAttribute& textAttributes, const bool isSettingDefaultBrushes) noexcept = 0; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& pfiFontInfoDesired, _Out_ FontInfo& pfiFontInfo) noexcept override; From 8ae61470e6980e14c406fe43d24436e2ed0999fe Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 21 Apr 2020 13:33:34 +0100 Subject: [PATCH 02/15] Pass the IRenderData to UpdateDrawingBrushes instead of the COLORREFs. --- src/renderer/base/renderer.cpp | 5 +---- src/renderer/dx/DxRenderer.cpp | 15 ++++++++------- src/renderer/dx/DxRenderer.hpp | 7 +++---- src/renderer/gdi/gdirenderer.hpp | 5 ++--- src/renderer/gdi/state.cpp | 13 +++++++------ src/renderer/inc/IRenderEngine.hpp | 7 +++---- src/renderer/uia/UiaRenderer.cpp | 8 +++----- src/renderer/uia/UiaRenderer.hpp | 5 ++--- src/renderer/vt/Xterm256Engine.cpp | 15 +++++---------- src/renderer/vt/Xterm256Engine.hpp | 5 ++--- src/renderer/vt/XtermEngine.cpp | 15 +++++---------- src/renderer/vt/XtermEngine.hpp | 5 ++--- src/renderer/vt/paint.cpp | 24 ++++++++++++++---------- src/renderer/vt/vtrenderer.hpp | 15 ++++++--------- 14 files changed, 63 insertions(+), 81 deletions(-) diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index 16a30a77294..74e6df2ba5c 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -1088,12 +1088,9 @@ void Renderer::_PaintSelection(_In_ IRenderEngine* const pEngine) // - [[nodiscard]] HRESULT Renderer::_UpdateDrawingBrushes(_In_ IRenderEngine* const pEngine, const TextAttribute textAttributes, const bool isSettingDefaultBrushes) { - const COLORREF rgbForeground = _pData->GetForegroundColor(textAttributes); - const COLORREF rgbBackground = _pData->GetBackgroundColor(textAttributes); - // The last color needs to be each engine's responsibility. If it's local to this function, // then on the next engine we might not update the color. - return pEngine->UpdateDrawingBrushes(rgbForeground, rgbBackground, textAttributes, isSettingDefaultBrushes); + return pEngine->UpdateDrawingBrushes(textAttributes, _pData, isSettingDefaultBrushes); } // Routine Description: diff --git a/src/renderer/dx/DxRenderer.cpp b/src/renderer/dx/DxRenderer.cpp index 959c1016735..b9f723ea9ee 100644 --- a/src/renderer/dx/DxRenderer.cpp +++ b/src/renderer/dx/DxRenderer.cpp @@ -1519,16 +1519,14 @@ CATCH_RETURN() // Routine Description: // - Updates the default brush colors used for drawing // Arguments: -// - colorForeground - Foreground brush color -// - colorBackground - Background brush color -// - textAttributes - +// - textAttributes - Text attributes to use for the brush color +// - pData - The interface to console data structures required for rendering // - isSettingDefaultBrushes - Lets us know that these are the default brushes to paint the swapchain background or selection // Return Value: // - S_OK or relevant DirectX error. -[[nodiscard]] HRESULT DxEngine::UpdateDrawingBrushes(COLORREF const colorForeground, - COLORREF const colorBackground, - const TextAttribute& /*textAttributes*/, - bool const isSettingDefaultBrushes) noexcept +[[nodiscard]] HRESULT DxEngine::UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, + const bool isSettingDefaultBrushes) noexcept { // GH#5098: If we're rendering with cleartype text, we need to always render // onto an opaque background. If our background's opacity is 1.0f, that's @@ -1541,6 +1539,9 @@ CATCH_RETURN() const bool usingTransparency = _defaultTextBackgroundOpacity != 1.0f; const bool forceOpaqueBG = usingCleartype && !usingTransparency; + const COLORREF colorForeground = pData->GetForegroundColor(textAttributes); + const COLORREF colorBackground = pData->GetBackgroundColor(textAttributes); + _foregroundColor = _ColorFFromColorRef(OPACITY_OPAQUE | colorForeground); _backgroundColor = _ColorFFromColorRef((forceOpaqueBG ? OPACITY_OPAQUE : 0) | colorBackground); diff --git a/src/renderer/dx/DxRenderer.hpp b/src/renderer/dx/DxRenderer.hpp index 23b9a19ec0c..e0089832c3e 100644 --- a/src/renderer/dx/DxRenderer.hpp +++ b/src/renderer/dx/DxRenderer.hpp @@ -93,10 +93,9 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintCursor(const CursorOptions& options) noexcept override; - [[nodiscard]] HRESULT UpdateDrawingBrushes(COLORREF const colorForeground, - COLORREF const colorBackground, - const TextAttribute& textAttributes, - bool const isSettingDefaultBrushes) noexcept override; + [[nodiscard]] HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, + const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& fiFontInfoDesired, FontInfo& fiFontInfo) noexcept override; [[nodiscard]] HRESULT UpdateDpi(int const iDpi) noexcept override; [[nodiscard]] HRESULT UpdateViewport(const SMALL_RECT srNewViewport) noexcept override; diff --git a/src/renderer/gdi/gdirenderer.hpp b/src/renderer/gdi/gdirenderer.hpp index 7037aa641b1..bae4ea2eda2 100644 --- a/src/renderer/gdi/gdirenderer.hpp +++ b/src/renderer/gdi/gdirenderer.hpp @@ -54,9 +54,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintCursor(const CursorOptions& options) noexcept override; - [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& textAttributes, + [[nodiscard]] HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& FontInfoDesired, _Out_ FontInfo& FontInfo) noexcept override; diff --git a/src/renderer/gdi/state.cpp b/src/renderer/gdi/state.cpp index 26635983116..10be2c6a5f2 100644 --- a/src/renderer/gdi/state.cpp +++ b/src/renderer/gdi/state.cpp @@ -173,16 +173,14 @@ GdiEngine::~GdiEngine() // Routine Description: // - This method will set the GDI brushes in the drawing context (and update the hung-window background color) // Arguments: -// - colorForeground - Foreground color -// - colorBackground - Background color -// - textAttributes - +// - textAttributes - Text attributes to use for the brush color +// - pData - The interface to console data structures required for rendering // - isSettingDefaultBrushes - Lets us know that the default brushes are being set so we can update the DC background // and the hung app background painting color // Return Value: // - S_OK if set successfully or relevant GDI error via HRESULT. -[[nodiscard]] HRESULT GdiEngine::UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& /*textAttributes*/, +[[nodiscard]] HRESULT GdiEngine::UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool isSettingDefaultBrushes) noexcept { RETURN_IF_FAILED(_FlushBufferLines()); @@ -190,6 +188,9 @@ GdiEngine::~GdiEngine() RETURN_HR_IF_NULL(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), _hdcMemoryContext); // Set the colors for painting text + const COLORREF colorForeground = pData->GetForegroundColor(textAttributes); + const COLORREF colorBackground = pData->GetBackgroundColor(textAttributes); + if (colorForeground != _lastFg) { RETURN_HR_IF(E_FAIL, CLR_INVALID == SetTextColor(_hdcMemoryContext, colorForeground)); diff --git a/src/renderer/inc/IRenderEngine.hpp b/src/renderer/inc/IRenderEngine.hpp index f48e46e8c22..8df30a60035 100644 --- a/src/renderer/inc/IRenderEngine.hpp +++ b/src/renderer/inc/IRenderEngine.hpp @@ -14,10 +14,10 @@ Author(s): #pragma once -#include "../../buffer/out/TextAttribute.hpp" #include "CursorOptions.h" #include "Cluster.hpp" #include "FontInfoDesired.hpp" +#include "IRenderData.hpp" namespace Microsoft::Console::Render { @@ -83,9 +83,8 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT PaintCursor(const CursorOptions& options) noexcept = 0; - [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& textAttributes, + [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool isSettingDefaultBrushes) noexcept = 0; [[nodiscard]] virtual HRESULT UpdateFont(const FontInfoDesired& FontInfoDesired, _Out_ FontInfo& FontInfo) noexcept = 0; diff --git a/src/renderer/uia/UiaRenderer.cpp b/src/renderer/uia/UiaRenderer.cpp index a95ae41bc6b..e6bdbe19e4f 100644 --- a/src/renderer/uia/UiaRenderer.cpp +++ b/src/renderer/uia/UiaRenderer.cpp @@ -361,15 +361,13 @@ CATCH_RETURN(); // - Updates the default brush colors used for drawing // For UIA, this doesn't mean anything. So do nothing. // Arguments: -// - colorForeground - -// - colorBackground - // - textAttributes - +// - pData - // - isSettingDefaultBrushes - // Return Value: // - S_FALSE since we do nothing -[[nodiscard]] HRESULT UiaEngine::UpdateDrawingBrushes(const COLORREF /*colorForeground*/, - const COLORREF /*colorBackground*/, - const TextAttribute& /*textAttributes*/, +[[nodiscard]] HRESULT UiaEngine::UpdateDrawingBrushes(const TextAttribute& /*textAttributes*/, + const gsl::not_null /*pData*/, const bool /*isSettingDefaultBrushes*/) noexcept { return S_FALSE; diff --git a/src/renderer/uia/UiaRenderer.hpp b/src/renderer/uia/UiaRenderer.hpp index 5ea18ea4eb7..1bc595510cb 100644 --- a/src/renderer/uia/UiaRenderer.hpp +++ b/src/renderer/uia/UiaRenderer.hpp @@ -60,9 +60,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintCursor(const CursorOptions& options) noexcept override; - [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& textAttributes, + [[nodiscard]] HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& fiFontInfoDesired, FontInfo& fiFontInfo) noexcept override; [[nodiscard]] HRESULT UpdateDpi(int const iDpi) noexcept override; diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index d9aedd80e6e..24f530aa0d8 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -20,16 +20,14 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // - Write a VT sequence to change the current colors of text. Writes true RGB // color sequences. // Arguments: -// - colorForeground: The RGB Color to use to paint the foreground text. -// - colorBackground: The RGB Color to use to paint the background of the text. -// - textAttributes - text attributes (bold, italic, underline, etc.) to use. +// - textAttributes - Text attributes to use for the colors and character rendition +// - pData - The interface to console data structures required for rendering // - isSettingDefaultBrushes: indicates if we should change the background color of // the window. Unused for VT // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT Xterm256Engine::UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& textAttributes, +[[nodiscard]] HRESULT Xterm256Engine::UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool /*isSettingDefaultBrushes*/) noexcept { //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE @@ -44,10 +42,7 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // Only do extended attributes in xterm-256color, as to not break telnet.exe. RETURN_IF_FAILED(_UpdateExtendedAttrs(textAttributes)); - return VtEngine::_RgbUpdateDrawingBrushes(colorForeground, - colorBackground, - textAttributes.IsBold(), - _colorTable); + return VtEngine::_RgbUpdateDrawingBrushes(textAttributes, pData, _colorTable); } // Routine Description: diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 434a466c593..af8dbb9e2be 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -30,9 +30,8 @@ namespace Microsoft::Console::Render virtual ~Xterm256Engine() override = default; - [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& textAttributes, + [[nodiscard]] HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT ManuallyClearScrollback() noexcept override; diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 3fcd3cafd3f..f5c67e19e03 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -163,16 +163,14 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // - Write a VT sequence to change the current colors of text. Only writes // 16-color attributes. // Arguments: -// - colorForeground: The RGB Color to use to paint the foreground text. -// - colorBackground: The RGB Color to use to paint the background of the text. -// - textAttributes - text attributes (bold, italic, underline, etc.) to use. +// - textAttributes - Text attributes to use for the colors and character rendition +// - pData - The interface to console data structures required for rendering // - isSettingDefaultBrushes: indicates if we should change the background color of // the window. Unused for VT // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT XtermEngine::UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& textAttributes, +[[nodiscard]] HRESULT XtermEngine::UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool /*isSettingDefaultBrushes*/) noexcept { //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE @@ -184,10 +182,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE RETURN_IF_FAILED(_UpdateUnderline(textAttributes)); // The base xterm mode only knows about 16 colors - return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, - colorBackground, - textAttributes.IsBold(), - _colorTable); + return VtEngine::_16ColorUpdateDrawingBrushes(textAttributes, pData, _colorTable); } // Routine Description: diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index f73e9c0bb39..aafc7f4ade2 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -40,9 +40,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintCursor(const CursorOptions& options) noexcept override; - [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& textAttributes, + [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index b4abab752bd..a438efe81e3 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -185,19 +185,21 @@ using namespace Microsoft::Console::Types; // - Write a VT sequence to change the current colors of text. Writes true RGB // color sequences. // Arguments: -// - colorForeground: The RGB Color to use to paint the foreground text. -// - colorBackground: The RGB Color to use to paint the background of the text. +// - textAttributes: Text attributes to use for the colors. +// - pData: The interface to console data structures required for rendering. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_RgbUpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const bool isBold, +[[nodiscard]] HRESULT VtEngine::_RgbUpdateDrawingBrushes(const TextAttribute& textAttributes, + const IRenderData* pData, const std::basic_string_view colorTable) noexcept { + const COLORREF colorForeground = pData->GetForegroundColor(textAttributes); + const COLORREF colorBackground = pData->GetBackgroundColor(textAttributes); const bool fgChanged = colorForeground != _LastFG; const bool bgChanged = colorBackground != _LastBG; const bool fgIsDefault = colorForeground == _colorProvider.GetDefaultForeground(); const bool bgIsDefault = colorBackground == _colorProvider.GetDefaultBackground(); + const bool isBold = textAttributes.IsBold(); // If both the FG and BG should be the defaults, emit a SGR reset. if ((fgChanged || bgChanged) && fgIsDefault && bgIsDefault) @@ -268,21 +270,23 @@ using namespace Microsoft::Console::Types; // find the colors in the color table that are nearest to the input colors, // and write those indices to the pipe. // Arguments: -// - colorForeground: The RGB Color to use to paint the foreground text. -// - colorBackground: The RGB Color to use to paint the background of the text. +// - textAttributes: Text attributes to use for the colors. +// - pData: The interface to console data structures required for rendering. // - ColorTable: An array of colors to find the closest match to. // - cColorTable: size of the color table. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_16ColorUpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const bool isBold, +[[nodiscard]] HRESULT VtEngine::_16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes, + const IRenderData* pData, const std::basic_string_view colorTable) noexcept { + const COLORREF colorForeground = pData->GetForegroundColor(textAttributes); + const COLORREF colorBackground = pData->GetBackgroundColor(textAttributes); const bool fgChanged = colorForeground != _LastFG; const bool bgChanged = colorBackground != _LastBG; const bool fgIsDefault = colorForeground == _colorProvider.GetDefaultForeground(); const bool bgIsDefault = colorBackground == _colorProvider.GetDefaultBackground(); + const bool isBold = textAttributes.IsBold(); // If both the FG and BG should be the defaults, emit a SGR reset. if ((fgChanged || bgChanged) && fgIsDefault && bgIsDefault) diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 6213dc3cc24..9a26f1cb627 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -75,9 +75,8 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT PaintCursor(const CursorOptions& options) noexcept override; - [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const TextAttribute& textAttributes, + [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, const bool isSettingDefaultBrushes) noexcept = 0; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& pfiFontInfoDesired, _Out_ FontInfo& pfiFontInfo) noexcept override; @@ -212,13 +211,11 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT _RequestWin32Input() noexcept; [[nodiscard]] virtual HRESULT _MoveCursor(const COORD coord) noexcept = 0; - [[nodiscard]] HRESULT _RgbUpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const bool isBold, + [[nodiscard]] HRESULT _RgbUpdateDrawingBrushes(const TextAttribute& textAttributes, + const IRenderData* pData, const std::basic_string_view colorTable) noexcept; - [[nodiscard]] HRESULT _16ColorUpdateDrawingBrushes(const COLORREF colorForeground, - const COLORREF colorBackground, - const bool isBold, + [[nodiscard]] HRESULT _16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes, + const IRenderData* pData, const std::basic_string_view colorTable) noexcept; bool _WillWriteSingleChar() const; From e27d39bcb097d1dde9f8ce3a9cdbe027480f10c4 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 3 May 2020 02:40:17 +0100 Subject: [PATCH 03/15] Update VtRendererTests to account for changes to the renderer interface. --- src/host/ut_host/VtRendererTests.cpp | 120 ++++++++++----------------- 1 file changed, 45 insertions(+), 75 deletions(-) diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 8d5a092ba29..8d884fb5b6d 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -432,6 +432,7 @@ void VtRendererTest::Xterm256TestColors() std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); + RenderData renderData; // Verify the first paint emits a clear and go home qExpectedInput.push_back("\x1b[2J"); @@ -450,29 +451,23 @@ void VtRendererTest::Xterm256TestColors() L"These values were picked for ease of formatting raw COLORREF values.")); qExpectedInput.push_back("\x1b[38;2;1;2;3m"); qExpectedInput.push_back("\x1b[48;2;5;6;7m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(0x00030201, - 0x00070605, - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ 0x00030201, 0x00070605 }, + &renderData, false)); TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"----Change only the BG----")); qExpectedInput.push_back("\x1b[48;2;7;8;9m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(0x00030201, - 0x00090807, - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ 0x00030201, 0x00090807 }, + &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the FG----")); qExpectedInput.push_back("\x1b[38;2;10;11;12m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(0x000c0b0a, - 0x00090807, - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ 0x000c0b0a, 0x00090807 }, + &renderData, false)); }); @@ -480,10 +475,8 @@ void VtRendererTest::Xterm256TestColors() Log::Comment(NoThrowString().Format( L"Make sure that color setting persists across EndPaint/StartPaint")); qExpectedInput.push_back(EMPTY_CALLBACK_SENTINEL); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(0x000c0b0a, - 0x00090807, - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ 0x000c0b0a, 0x00090807 }, + &renderData, false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback }); @@ -496,57 +489,45 @@ void VtRendererTest::Xterm256TestColors() L"Begin by setting the default colors - FG,BG = BRIGHT_WHITE,DARK_BLACK")); qExpectedInput.push_back("\x1b[m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], - g_ColorTable[0], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + &renderData, false)); TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"----Change only the BG----")); qExpectedInput.push_back("\x1b[41m"); // Background DARK_RED - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], - g_ColorTable[4], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[4] }, + &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the FG----")); qExpectedInput.push_back("\x1b[37m"); // Foreground DARK_WHITE - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], - g_ColorTable[4], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], g_ColorTable[4] }, + &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the BG to something not in the table----")); qExpectedInput.push_back("\x1b[48;2;1;1;1m"); // Background DARK_BLACK - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], - 0x010101, - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], 0x010101 }, + &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the BG to the 'Default' background----")); qExpectedInput.push_back("\x1b[49m"); // Background DARK_BLACK - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], - g_ColorTable[0], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], g_ColorTable[0] }, + &renderData, false)); Log::Comment(NoThrowString().Format( L"----Back to defaults----")); qExpectedInput.push_back("\x1b[m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], - g_ColorTable[0], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + &renderData, false)); }); @@ -554,10 +535,8 @@ void VtRendererTest::Xterm256TestColors() Log::Comment(NoThrowString().Format( L"Make sure that color setting persists across EndPaint/StartPaint")); qExpectedInput.push_back(EMPTY_CALLBACK_SENTINEL); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], - g_ColorTable[0], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + &renderData, false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback }); @@ -688,31 +667,31 @@ void VtRendererTest::Xterm256TestExtendedAttributes() VERIFY_SUCCEEDED(TestData::TryGetValue(L"invisible", invisible)); VERIFY_SUCCEEDED(TestData::TryGetValue(L"crossedOut", crossedOut)); - ExtendedAttributes desiredAttrs{ ExtendedAttributes::Normal }; + TextAttribute desiredAttrs; std::vector onSequences, offSequences; // Collect up a VT sequence to set the state given the method properties if (italics) { - WI_SetFlag(desiredAttrs, ExtendedAttributes::Italics); + desiredAttrs.SetItalics(true); onSequences.push_back("\x1b[3m"); offSequences.push_back("\x1b[23m"); } if (blink) { - WI_SetFlag(desiredAttrs, ExtendedAttributes::Blinking); + desiredAttrs.SetBlinking(true); onSequences.push_back("\x1b[5m"); offSequences.push_back("\x1b[25m"); } if (invisible) { - WI_SetFlag(desiredAttrs, ExtendedAttributes::Invisible); + desiredAttrs.SetInvisible(true); onSequences.push_back("\x1b[8m"); offSequences.push_back("\x1b[28m"); } if (crossedOut) { - WI_SetFlag(desiredAttrs, ExtendedAttributes::CrossedOut); + desiredAttrs.SetCrossedOut(true); onSequences.push_back("\x1b[9m"); offSequences.push_back("\x1b[29m"); } @@ -746,7 +725,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes() L"----Turn the extended attributes off----")); TestPaint(*engine, [&]() { std::copy(offSequences.cbegin(), offSequences.cend(), std::back_inserter(qExpectedInput)); - VERIFY_SUCCEEDED(engine->_UpdateExtendedAttrs(ExtendedAttributes::Normal)); + VERIFY_SUCCEEDED(engine->_UpdateExtendedAttrs({})); }); Log::Comment(NoThrowString().Format( @@ -952,6 +931,7 @@ void VtRendererTest::XtermTestColors() std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); + RenderData renderData; // Verify the first paint emits a clear and go home qExpectedInput.push_back("\x1b[2J"); @@ -969,53 +949,45 @@ void VtRendererTest::XtermTestColors() L"Begin by setting the default colors - FG,BG = BRIGHT_WHITE,DARK_BLACK")); qExpectedInput.push_back("\x1b[m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], - g_ColorTable[0], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + &renderData, false)); TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"----Change only the BG----")); qExpectedInput.push_back("\x1b[41m"); // Background DARK_RED - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], - g_ColorTable[4], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[4] }, + &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the FG----")); qExpectedInput.push_back("\x1b[37m"); // Foreground DARK_WHITE - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], - g_ColorTable[4], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], g_ColorTable[4] }, + &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the BG to something not in the table----")); qExpectedInput.push_back("\x1b[40m"); // Background DARK_BLACK - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], 0x010101, 0, ExtendedAttributes::Normal, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], 0x010101 }, + &renderData, + false)); Log::Comment(NoThrowString().Format( L"----Change only the BG to the 'Default' background----")); qExpectedInput.push_back("\x1b[40m"); // Background DARK_BLACK - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], - g_ColorTable[0], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], g_ColorTable[0] }, + &renderData, false)); Log::Comment(NoThrowString().Format( L"----Back to defaults----")); qExpectedInput.push_back("\x1b[m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], - g_ColorTable[0], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + &renderData, false)); }); @@ -1023,10 +995,8 @@ void VtRendererTest::XtermTestColors() Log::Comment(NoThrowString().Format( L"Make sure that color setting persists across EndPaint/StartPaint")); qExpectedInput.push_back(EMPTY_CALLBACK_SENTINEL); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], - g_ColorTable[0], - 0, - ExtendedAttributes::Normal, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + &renderData, false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback }); From 626fca4bda980241de714a9dfdd722d4d33171ea Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sat, 13 Jun 2020 23:54:05 +0100 Subject: [PATCH 04/15] Reimplement the Xterm256Engine UpdateDrawingBrushes method preserving the attribute color format. --- src/buffer/out/TextAttribute.cpp | 20 ++++++ src/buffer/out/TextAttribute.hpp | 4 ++ src/buffer/out/TextColor.cpp | 4 +- src/buffer/out/TextColor.h | 6 +- src/renderer/vt/VtSequences.cpp | 18 ++++++ src/renderer/vt/Xterm256Engine.cpp | 8 +-- src/renderer/vt/Xterm256Engine.hpp | 4 -- src/renderer/vt/XtermEngine.cpp | 7 +-- src/renderer/vt/XtermEngine.hpp | 1 - src/renderer/vt/paint.cpp | 99 +++++++++++++----------------- src/renderer/vt/state.cpp | 1 + src/renderer/vt/vtrenderer.hpp | 9 +-- 12 files changed, 102 insertions(+), 79 deletions(-) diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp index 1d8692ccf19..b6931d46168 100644 --- a/src/buffer/out/TextAttribute.cpp +++ b/src/buffer/out/TextAttribute.cpp @@ -75,6 +75,26 @@ COLORREF TextAttribute::_GetRgbBackground(std::basic_string_view color return _background.GetColor(colorTable, defaultColor, false); } +TextColor TextAttribute::GetForeground() const noexcept +{ + return _foreground; +} + +TextColor TextAttribute::GetBackground() const noexcept +{ + return _background; +} + +void TextAttribute::SetForeground(const TextColor foreground) noexcept +{ + _foreground = foreground; +} + +void TextAttribute::SetBackground(const TextColor background) noexcept +{ + _background = background; +} + void TextAttribute::SetForeground(const COLORREF rgbForeground) noexcept { _foreground = TextColor(rgbForeground); diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index edb6e771012..1b2ed5898ee 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -106,6 +106,10 @@ class TextAttribute final ExtendedAttributes GetExtendedAttributes() const noexcept; + TextColor GetForeground() const noexcept; + TextColor GetBackground() const noexcept; + void SetForeground(const TextColor foreground) noexcept; + void SetBackground(const TextColor background) noexcept; void SetForeground(const COLORREF rgbForeground) noexcept; void SetBackground(const COLORREF rgbBackground) noexcept; void SetIndexedForeground(const BYTE fgIndex) noexcept; diff --git a/src/buffer/out/TextColor.cpp b/src/buffer/out/TextColor.cpp index 4e40d42dcf3..75b1ef5e6b4 100644 --- a/src/buffer/out/TextColor.cpp +++ b/src/buffer/out/TextColor.cpp @@ -164,7 +164,7 @@ COLORREF TextColor::GetColor(std::basic_string_view colorTable, } else if (IsRgb()) { - return _GetRGB(); + return GetRGB(); } else if (IsIndex16() && brighten) { @@ -214,7 +214,7 @@ BYTE TextColor::GetLegacyIndex(const BYTE defaultIndex) const noexcept // - // Return Value: // - a COLORREF containing our stored value -COLORREF TextColor::_GetRGB() const noexcept +COLORREF TextColor::GetRGB() const noexcept { return RGB(_red, _green, _blue); } diff --git a/src/buffer/out/TextColor.h b/src/buffer/out/TextColor.h index 5178898215b..bcd8d27806a 100644 --- a/src/buffer/out/TextColor.h +++ b/src/buffer/out/TextColor.h @@ -98,6 +98,8 @@ struct TextColor return _index; } + COLORREF GetRGB() const noexcept; + private: ColorType _meta : 2; union @@ -107,8 +109,6 @@ struct TextColor BYTE _green; BYTE _blue; - COLORREF _GetRGB() const noexcept; - #ifdef UNIT_TESTING friend class TextBufferTests; template @@ -149,7 +149,7 @@ namespace WEX } else if (color.IsRgb()) { - return WEX::Common::NoThrowString().Format(L"{RGB:0x%06x}", color._GetRGB()); + return WEX::Common::NoThrowString().Format(L"{RGB:0x%06x}", color.GetRGB()); } else { diff --git a/src/renderer/vt/VtSequences.cpp b/src/renderer/vt/VtSequences.cpp index 013a16da2de..8cc55bd73cf 100644 --- a/src/renderer/vt/VtSequences.cpp +++ b/src/renderer/vt/VtSequences.cpp @@ -249,6 +249,24 @@ using namespace Microsoft::Console::Render; return _WriteFormattedString(&fmt, vtIndex); } +// Method Description: +// - Formats and writes a sequence to change the current text attributes to an +// indexed color from the 256-color table. +// Arguments: +// - wAttr: Windows color table index to emit as a VT sequence +// - fIsForeground: true if we should emit the foreground sequence, false for background +// Return Value: +// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. +[[nodiscard]] HRESULT VtEngine::_SetGraphicsRendition256Color(const WORD index, + const bool fIsForeground) noexcept +{ + const std::string fmt = fIsForeground ? + "\x1b[38;5;%dm" : + "\x1b[48;5;%dm"; + + return _WriteFormattedString(&fmt, ::Xterm256ToWindowsIndex(index)); +} + // Method Description: // - Formats and writes a sequence to change the current text attributes to an // RGB color. diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 24f530aa0d8..38721b99405 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -27,9 +27,11 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT Xterm256Engine::UpdateDrawingBrushes(const TextAttribute& textAttributes, - const gsl::not_null pData, + const gsl::not_null /*pData*/, const bool /*isSettingDefaultBrushes*/) noexcept { + RETURN_IF_FAILED(VtEngine::_RgbUpdateDrawingBrushes(textAttributes)); + //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE // flag is there. If the state of that flag is different then our // current state, change the underlining state. @@ -40,9 +42,7 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, RETURN_IF_FAILED(_UpdateUnderline(textAttributes)); // Only do extended attributes in xterm-256color, as to not break telnet.exe. - RETURN_IF_FAILED(_UpdateExtendedAttrs(textAttributes)); - - return VtEngine::_RgbUpdateDrawingBrushes(textAttributes, pData, _colorTable); + return _UpdateExtendedAttrs(textAttributes); } // Routine Description: diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index af8dbb9e2be..00b17cce50f 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -39,10 +39,6 @@ namespace Microsoft::Console::Render private: [[nodiscard]] HRESULT _UpdateExtendedAttrs(const TextAttribute& textAttributes) noexcept; - // We're only using Italics, Blinking, Invisible and Crossed Out for now - // See GH#2916 for adding a more complete implementation. - TextAttribute _lastTextAttributes; - #ifdef UNIT_TESTING friend class VtRendererTest; friend class ConptyOutputTests; diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index f5c67e19e03..bca2f362081 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -17,7 +17,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, VtEngine(std::move(hPipe), colorProvider, initialViewport), _colorTable(colorTable), _fUseAsciiOnly(fUseAsciiOnly), - _usingUnderLine(false), _needToDisableCursor(false), _lastCursorIsVisible(false), _nextCursorIsVisible(true) @@ -144,7 +143,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, [[nodiscard]] HRESULT XtermEngine::_UpdateUnderline(const TextAttribute& textAttributes) noexcept { bool textUnderlined = textAttributes.IsUnderlined(); - if (textUnderlined != _usingUnderLine) + if (textUnderlined != _lastTextAttributes.IsUnderlined()) { if (textUnderlined) { @@ -154,7 +153,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, { RETURN_IF_FAILED(_EndUnderline()); } - _usingUnderLine = textUnderlined; + _lastTextAttributes.SetUnderline(textUnderlined); } return S_OK; } @@ -471,7 +470,7 @@ try // important information to send to the connected Terminal. if (_newBottomLine) { - _newBottomLineBG = _LastBG; + _newBottomLineBG = _lastTextAttributes.GetBackground(); } return S_OK; diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index aafc7f4ade2..8748fe5f9db 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -56,7 +56,6 @@ namespace Microsoft::Console::Render protected: const std::basic_string_view _colorTable; const bool _fUseAsciiOnly; - bool _usingUnderLine; bool _needToDisableCursor; bool _lastCursorIsVisible; bool _nextCursorIsVisible; diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index a438efe81e3..1d5df962a96 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -186,80 +186,65 @@ using namespace Microsoft::Console::Types; // color sequences. // Arguments: // - textAttributes: Text attributes to use for the colors. -// - pData: The interface to console data structures required for rendering. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_RgbUpdateDrawingBrushes(const TextAttribute& textAttributes, - const IRenderData* pData, - const std::basic_string_view colorTable) noexcept +[[nodiscard]] HRESULT VtEngine::_RgbUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept { - const COLORREF colorForeground = pData->GetForegroundColor(textAttributes); - const COLORREF colorBackground = pData->GetBackgroundColor(textAttributes); - const bool fgChanged = colorForeground != _LastFG; - const bool bgChanged = colorBackground != _LastBG; - const bool fgIsDefault = colorForeground == _colorProvider.GetDefaultForeground(); - const bool bgIsDefault = colorBackground == _colorProvider.GetDefaultBackground(); - const bool isBold = textAttributes.IsBold(); + const auto fg = textAttributes.GetForeground(); + const auto bg = textAttributes.GetBackground(); + auto lastFg = _lastTextAttributes.GetForeground(); + auto lastBg = _lastTextAttributes.GetBackground(); // If both the FG and BG should be the defaults, emit a SGR reset. - if ((fgChanged || bgChanged) && fgIsDefault && bgIsDefault) + if (fg.IsDefault() && bg.IsDefault() && !(lastFg.IsDefault() && lastBg.IsDefault())) { - // SGR Reset will also clear out the boldness of the text. + // SGR Reset will clear all attributes. RETURN_IF_FAILED(_SetGraphicsDefault()); - _LastFG = colorForeground; - _LastBG = colorBackground; - _lastWasBold = false; + _lastTextAttributes = {}; + lastFg = {}; + lastBg = {}; + } - // I'm not sure this is possible currently, but if the text is bold, but - // default colors, make sure we bold it. - if (isBold) + if (fg != lastFg) + { + if (fg.IsDefault()) { - RETURN_IF_FAILED(_SetGraphicsBoldness(isBold)); - _lastWasBold = isBold; + RETURN_IF_FAILED(_SetGraphicsRenditionDefaultColor(true)); } + else if (fg.IsIndex16()) + { + RETURN_IF_FAILED(_SetGraphicsRendition16Color(fg.GetIndex(), true)); + } + else if (fg.IsIndex256()) + { + RETURN_IF_FAILED(_SetGraphicsRendition256Color(fg.GetIndex(), true)); + } + else if (fg.IsRgb()) + { + RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(fg.GetRGB(), true)); + } + _lastTextAttributes.SetForeground(fg); } - else + + if (bg != lastBg) { - if (_lastWasBold != isBold) + if (bg.IsDefault()) { - RETURN_IF_FAILED(_SetGraphicsBoldness(isBold)); - _lastWasBold = isBold; + RETURN_IF_FAILED(_SetGraphicsRenditionDefaultColor(false)); } - - WORD wFoundColor = 0; - if (fgChanged) + else if (bg.IsIndex16()) { - if (fgIsDefault) - { - RETURN_IF_FAILED(_SetGraphicsRenditionDefaultColor(true)); - } - else if (::FindTableIndex(colorForeground, colorTable, &wFoundColor)) - { - RETURN_IF_FAILED(_SetGraphicsRendition16Color(wFoundColor, true)); - } - else - { - RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(colorForeground, true)); - } - _LastFG = colorForeground; + RETURN_IF_FAILED(_SetGraphicsRendition16Color(bg.GetIndex(), false)); } - - if (bgChanged) + else if (bg.IsIndex256()) { - if (bgIsDefault) - { - RETURN_IF_FAILED(_SetGraphicsRenditionDefaultColor(false)); - } - else if (::FindTableIndex(colorBackground, colorTable, &wFoundColor)) - { - RETURN_IF_FAILED(_SetGraphicsRendition16Color(wFoundColor, false)); - } - else - { - RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(colorBackground, false)); - } - _LastBG = colorBackground; + RETURN_IF_FAILED(_SetGraphicsRendition256Color(bg.GetIndex(), false)); + } + else if (bg.IsRgb()) + { + RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(bg.GetRGB(), false)); } + _lastTextAttributes.SetBackground(bg); } return S_OK; @@ -451,7 +436,7 @@ using namespace Microsoft::Console::Types; // than when we emitted the line, we can't optimize out the spaces from it. // We'll still need to emit those spaces, so that the connected terminal // will have the same background color on those blank cells. - const bool bgMatched = _newBottomLineBG.has_value() ? (_newBottomLineBG.value() == _LastBG) : true; + const bool bgMatched = _newBottomLineBG.has_value() ? (_newBottomLineBG.value() == _lastTextAttributes.GetBackground()) : true; // If we're not using erase char, but we did erase all at the start of the // frame, don't add spaces at the end. diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index c03b6574634..1682297f91a 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -31,6 +31,7 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, RenderEngineBase(), _hFile(std::move(pipe)), _colorProvider(colorProvider), + _lastTextAttributes(INVALID_COLOR, INVALID_COLOR), _LastFG(INVALID_COLOR), _LastBG(INVALID_COLOR), _lastWasBold(false), diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 9a26f1cb627..085ff8908a8 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -118,6 +118,7 @@ namespace Microsoft::Console::Render const Microsoft::Console::IDefaultColorProvider& _colorProvider; + TextAttribute _lastTextAttributes; COLORREF _LastFG; COLORREF _LastBG; bool _lastWasBold; @@ -155,7 +156,7 @@ namespace Microsoft::Console::Render bool _delayedEolWrap{ false }; bool _resizeQuirk{ false }; - std::optional _newBottomLineBG{ std::nullopt }; + std::optional _newBottomLineBG{ std::nullopt }; [[nodiscard]] HRESULT _Write(std::string_view const str) noexcept; [[nodiscard]] HRESULT _WriteFormattedString(const std::string* const pFormat, ...) noexcept; @@ -181,6 +182,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT _ChangeTitle(const std::string& title) noexcept; [[nodiscard]] HRESULT _SetGraphicsRendition16Color(const WORD wAttr, const bool fIsForeground) noexcept; + [[nodiscard]] HRESULT _SetGraphicsRendition256Color(const WORD index, + const bool fIsForeground) noexcept; [[nodiscard]] HRESULT _SetGraphicsRenditionRGBColor(const COLORREF color, const bool fIsForeground) noexcept; [[nodiscard]] HRESULT _SetGraphicsRenditionDefaultColor(const bool fIsForeground) noexcept; @@ -211,9 +214,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT _RequestWin32Input() noexcept; [[nodiscard]] virtual HRESULT _MoveCursor(const COORD coord) noexcept = 0; - [[nodiscard]] HRESULT _RgbUpdateDrawingBrushes(const TextAttribute& textAttributes, - const IRenderData* pData, - const std::basic_string_view colorTable) noexcept; + [[nodiscard]] HRESULT _RgbUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept; [[nodiscard]] HRESULT _16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes, const IRenderData* pData, const std::basic_string_view colorTable) noexcept; From 6f73dd7641497d56df4a140db91f32489cff187d Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 14 Jun 2020 00:30:52 +0100 Subject: [PATCH 05/15] Simplify and consolidate the meta/extended attribute updates. --- src/renderer/vt/VtSequences.cpp | 101 ++++++++--------------------- src/renderer/vt/Xterm256Engine.cpp | 90 +++++++++++-------------- src/renderer/vt/XtermEngine.cpp | 9 +-- src/renderer/vt/paint.cpp | 4 +- src/renderer/vt/vtrenderer.hpp | 23 ++----- 5 files changed, 76 insertions(+), 151 deletions(-) diff --git a/src/renderer/vt/VtSequences.cpp b/src/renderer/vt/VtSequences.cpp index 8cc55bd73cf..32d80c31236 100644 --- a/src/renderer/vt/VtSequences.cpp +++ b/src/renderer/vt/VtSequences.cpp @@ -192,18 +192,6 @@ using namespace Microsoft::Console::Render; return _Write("\x1b[H"); } -// Method Description: -// - Formats and writes a sequence change the boldness of the following text. -// Arguments: -// - isBold: If true, we'll embolden the text. Otherwise we'll debolden the text. -// Return Value: -// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_SetGraphicsBoldness(const bool isBold) noexcept -{ - const std::string_view fmt = isBold ? "\x1b[1m" : "\x1b[22m"; - return _Write(fmt); -} - // Method Description: // - Formats and writes a sequence to change the current text attributes to the default. // Arguments: @@ -346,113 +334,80 @@ using namespace Microsoft::Console::Render; } // Method Description: -// - Writes a sequence to tell the terminal to start underlining text +// - Formats and writes a sequence to change the boldness of the following text. // Arguments: -// - -// Return Value: -// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_BeginUnderline() noexcept -{ - return _Write("\x1b[4m"); -} - -// Method Description: -// - Writes a sequence to tell the terminal to stop underlining text -// Arguments: -// - -// Return Value: -// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_EndUnderline() noexcept -{ - return _Write("\x1b[24m"); -} - -// Method Description: -// - Writes a sequence to tell the terminal to start italicizing text -// Arguments: -// - -// Return Value: -// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_BeginItalics() noexcept -{ - return _Write("\x1b[3m"); -} - -// Method Description: -// - Writes a sequence to tell the terminal to stop italicizing text -// Arguments: -// - +// - isBold: If true, we'll embolden the text. Otherwise we'll debolden the text. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_EndItalics() noexcept +[[nodiscard]] HRESULT VtEngine::_SetBold(const bool isBold) noexcept { - return _Write("\x1b[23m"); + return _Write(isBold ? "\x1b[1m" : "\x1b[22m"); } // Method Description: -// - Writes a sequence to tell the terminal to start blinking text +// - Formats and writes a sequence to change the underline of the following text. // Arguments: -// - +// - isUnderlined: If true, we'll underline the text. Otherwise we'll remove the underline. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_BeginBlink() noexcept +[[nodiscard]] HRESULT VtEngine::_SetUnderline(const bool isUnderlined) noexcept { - return _Write("\x1b[5m"); + return _Write(isUnderlined ? "\x1b[4m" : "\x1b[24m"); } // Method Description: -// - Writes a sequence to tell the terminal to stop blinking text +// - Formats and writes a sequence to change the italics of the following text. // Arguments: -// - +// - isItalic: If true, we'll italicize the text. Otherwise we'll remove the italics. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_EndBlink() noexcept +[[nodiscard]] HRESULT VtEngine::_SetItalics(const bool isItalic) noexcept { - return _Write("\x1b[25m"); + return _Write(isItalic ? "\x1b[3m" : "\x1b[23m"); } // Method Description: -// - Writes a sequence to tell the terminal to start marking text as invisible +// - Formats and writes a sequence to change the blinking of the following text. // Arguments: -// - +// - isBlinking: If true, we'll start the text blinking. Otherwise we'll stop the blinking. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_BeginInvisible() noexcept +[[nodiscard]] HRESULT VtEngine::_SetBlinking(const bool isBlinking) noexcept { - return _Write("\x1b[8m"); + return _Write(isBlinking ? "\x1b[5m" : "\x1b[25m"); } // Method Description: -// - Writes a sequence to tell the terminal to stop marking text as invisible +// - Formats and writes a sequence to change the visibility of the following text. // Arguments: -// - +// - isInvisible: If true, we'll make the text invisible. Otherwise we'll make it visible. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_EndInvisible() noexcept +[[nodiscard]] HRESULT VtEngine::_SetInvisible(const bool isInvisible) noexcept { - return _Write("\x1b[28m"); + return _Write(isInvisible ? "\x1b[8m" : "\x1b[28m"); } // Method Description: -// - Writes a sequence to tell the terminal to start crossing-out text +// - Formats and writes a sequence to change the crossed out state of the following text. // Arguments: -// - +// - isCrossedOut: If true, we'll cross out the text. Otherwise we'll stop crossing out. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_BeginCrossedOut() noexcept +[[nodiscard]] HRESULT VtEngine::_SetCrossedOut(const bool isCrossedOut) noexcept { - return _Write("\x1b[9m"); + return _Write(isCrossedOut ? "\x1b[9m" : "\x1b[29m"); } // Method Description: -// - Writes a sequence to tell the terminal to stop crossing-out text +// - Formats and writes a sequence to change the reversed state of the following text. // Arguments: -// - +// - isReversed: If true, we'll reverse the text. Otherwise we'll remove the reversed state. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_EndCrossedOut() noexcept +[[nodiscard]] HRESULT VtEngine::_SetReverseVideo(const bool isReversed) noexcept { - return _Write("\x1b[29m"); + return _Write(isReversed ? "\x1b[7m" : "\x1b[27m"); } // Method Description: diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 38721b99405..5c013da74b2 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -31,74 +31,60 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, const bool /*isSettingDefaultBrushes*/) noexcept { RETURN_IF_FAILED(VtEngine::_RgbUpdateDrawingBrushes(textAttributes)); - - //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE - // flag is there. If the state of that flag is different then our - // current state, change the underlining state. - // We have to do this here, instead of in PaintBufferGridLines, because - // we'll have already painted the text by the time PaintBufferGridLines - // is called. - // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE - RETURN_IF_FAILED(_UpdateUnderline(textAttributes)); - // Only do extended attributes in xterm-256color, as to not break telnet.exe. return _UpdateExtendedAttrs(textAttributes); } // Routine Description: -// - Write a VT sequence to either start or stop underlining text. +// - Write a VT sequence to update the character rendition attributes. // Arguments: // - textAttributes - text attributes (bold, italic, underline, etc.) to use. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT Xterm256Engine::_UpdateExtendedAttrs(const TextAttribute& textAttributes) noexcept { - // Helper lambda to check if a state (attr) has changed since it's last - // value (lastState), and appropriately start/end that state with the given - // begin/end functions. - auto updateFlagAndState = [this](const bool flagSet, - const bool lastState, - std::function beginFn, - std::function endFn) -> HRESULT { - if (flagSet != lastState) - { - if (flagSet) - { - RETURN_IF_FAILED(beginFn(this)); - } - else - { - RETURN_IF_FAILED(endFn(this)); - } - } - return S_OK; - }; + if (textAttributes.IsBold() != _lastTextAttributes.IsBold()) + { + RETURN_IF_FAILED(_SetBold(textAttributes.IsBold())); + _lastTextAttributes.SetBold(textAttributes.IsBold()); + } + + if (textAttributes.IsUnderlined() != _lastTextAttributes.IsUnderlined()) + { + RETURN_IF_FAILED(_SetUnderline(textAttributes.IsUnderlined())); + _lastTextAttributes.SetUnderline(textAttributes.IsUnderlined()); + } + + if (textAttributes.IsItalic() != _lastTextAttributes.IsItalic()) + { + RETURN_IF_FAILED(_SetItalics(textAttributes.IsItalic())); + _lastTextAttributes.SetItalics(textAttributes.IsItalic()); + } - auto hr = updateFlagAndState(textAttributes.IsItalic(), - _lastTextAttributes.IsItalic(), - &Xterm256Engine::_BeginItalics, - &Xterm256Engine::_EndItalics); - RETURN_IF_FAILED(hr); + if (textAttributes.IsBlinking() != _lastTextAttributes.IsBlinking()) + { + RETURN_IF_FAILED(_SetBlinking(textAttributes.IsBlinking())); + _lastTextAttributes.SetBlinking(textAttributes.IsBlinking()); + } - hr = updateFlagAndState(textAttributes.IsBlinking(), - _lastTextAttributes.IsBlinking(), - &Xterm256Engine::_BeginBlink, - &Xterm256Engine::_EndBlink); - RETURN_IF_FAILED(hr); + if (textAttributes.IsInvisible() != _lastTextAttributes.IsInvisible()) + { + RETURN_IF_FAILED(_SetInvisible(textAttributes.IsInvisible())); + _lastTextAttributes.SetInvisible(textAttributes.IsInvisible()); + } - hr = updateFlagAndState(textAttributes.IsInvisible(), - _lastTextAttributes.IsInvisible(), - &Xterm256Engine::_BeginInvisible, - &Xterm256Engine::_EndInvisible); - RETURN_IF_FAILED(hr); + if (textAttributes.IsCrossedOut() != _lastTextAttributes.IsCrossedOut()) + { + RETURN_IF_FAILED(_SetCrossedOut(textAttributes.IsCrossedOut())); + _lastTextAttributes.SetCrossedOut(textAttributes.IsCrossedOut()); + } - hr = updateFlagAndState(textAttributes.IsCrossedOut(), - _lastTextAttributes.IsCrossedOut(), - &Xterm256Engine::_BeginCrossedOut, - &Xterm256Engine::_EndCrossedOut); - RETURN_IF_FAILED(hr); + if (textAttributes.IsReverseVideo() != _lastTextAttributes.IsReverseVideo()) + { + RETURN_IF_FAILED(_SetReverseVideo(textAttributes.IsReverseVideo())); + _lastTextAttributes.SetReverseVideo(textAttributes.IsReverseVideo()); + } - _lastTextAttributes = textAttributes; return S_OK; } diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index bca2f362081..32e5d680185 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -145,14 +145,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, bool textUnderlined = textAttributes.IsUnderlined(); if (textUnderlined != _lastTextAttributes.IsUnderlined()) { - if (textUnderlined) - { - RETURN_IF_FAILED(_BeginUnderline()); - } - else - { - RETURN_IF_FAILED(_EndUnderline()); - } + RETURN_IF_FAILED(_SetUnderline(textUnderlined)); _lastTextAttributes.SetUnderline(textUnderlined); } return S_OK; diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 1d5df962a96..7d6ca062013 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -285,7 +285,7 @@ using namespace Microsoft::Console::Types; // default colors, make sure we bold it. if (isBold) { - RETURN_IF_FAILED(_SetGraphicsBoldness(isBold)); + RETURN_IF_FAILED(_SetBold(isBold)); _lastWasBold = isBold; } } @@ -293,7 +293,7 @@ using namespace Microsoft::Console::Types; { if (_lastWasBold != isBold) { - RETURN_IF_FAILED(_SetGraphicsBoldness(isBold)); + RETURN_IF_FAILED(_SetBold(isBold)); _lastWasBold = isBold; } diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 085ff8908a8..3245a828067 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -188,26 +188,17 @@ namespace Microsoft::Console::Render const bool fIsForeground) noexcept; [[nodiscard]] HRESULT _SetGraphicsRenditionDefaultColor(const bool fIsForeground) noexcept; - [[nodiscard]] HRESULT _SetGraphicsBoldness(const bool isBold) noexcept; - [[nodiscard]] HRESULT _SetGraphicsDefault() noexcept; [[nodiscard]] HRESULT _ResizeWindow(const short sWidth, const short sHeight) noexcept; - [[nodiscard]] HRESULT _BeginUnderline() noexcept; - [[nodiscard]] HRESULT _EndUnderline() noexcept; - - [[nodiscard]] HRESULT _BeginItalics() noexcept; - [[nodiscard]] HRESULT _EndItalics() noexcept; - - [[nodiscard]] HRESULT _BeginBlink() noexcept; - [[nodiscard]] HRESULT _EndBlink() noexcept; - - [[nodiscard]] HRESULT _BeginInvisible() noexcept; - [[nodiscard]] HRESULT _EndInvisible() noexcept; - - [[nodiscard]] HRESULT _BeginCrossedOut() noexcept; - [[nodiscard]] HRESULT _EndCrossedOut() noexcept; + [[nodiscard]] HRESULT _SetBold(const bool isBold) noexcept; + [[nodiscard]] HRESULT _SetUnderline(const bool isUnderlined) noexcept; + [[nodiscard]] HRESULT _SetItalics(const bool isItalic) noexcept; + [[nodiscard]] HRESULT _SetBlinking(const bool isBlinking) noexcept; + [[nodiscard]] HRESULT _SetInvisible(const bool isInvisible) noexcept; + [[nodiscard]] HRESULT _SetCrossedOut(const bool isCrossedOut) noexcept; + [[nodiscard]] HRESULT _SetReverseVideo(const bool isReversed) noexcept; [[nodiscard]] HRESULT _RequestCursor() noexcept; From c111b7a1429d4c5a32b074a8efc0c5b04023fed1 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 14 Jun 2020 00:49:47 +0100 Subject: [PATCH 06/15] Reimplement the 16-color UpdateDrawingBrushes method to avoid doing palette searches. --- src/buffer/out/TextAttribute.cpp | 2 +- src/buffer/out/TextColor.cpp | 5 ++ src/buffer/out/TextColor.h | 1 + src/renderer/vt/XtermEngine.cpp | 4 +- src/renderer/vt/paint.cpp | 89 +++++++++++++++----------------- src/renderer/vt/state.cpp | 3 -- src/renderer/vt/vtrenderer.hpp | 7 +-- 7 files changed, 52 insertions(+), 59 deletions(-) diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp index b6931d46168..38769752d48 100644 --- a/src/buffer/out/TextAttribute.cpp +++ b/src/buffer/out/TextAttribute.cpp @@ -16,7 +16,7 @@ WORD TextAttribute::GetLegacyAttributes(const WORD defaultAttributes) const noex const BYTE fgIndex = _foreground.GetLegacyIndex(defaultAttributes & FG_ATTRS); const BYTE bgIndex = _background.GetLegacyIndex((defaultAttributes & BG_ATTRS) >> 4); const WORD metaAttrs = _wAttrLegacy & META_ATTRS; - const bool brighten = _foreground.IsIndex16() && IsBold(); + const bool brighten = IsBold() && _foreground.CanBeBrightened(); return fgIndex | (bgIndex << 4) | metaAttrs | (brighten ? FOREGROUND_INTENSITY : 0); } diff --git a/src/buffer/out/TextColor.cpp b/src/buffer/out/TextColor.cpp index 75b1ef5e6b4..3a664dff316 100644 --- a/src/buffer/out/TextColor.cpp +++ b/src/buffer/out/TextColor.cpp @@ -50,6 +50,11 @@ constexpr std::array Index256ToIndex16 = { // clang-format on +bool TextColor::CanBeBrightened() const noexcept +{ + return IsIndex16() || IsDefault(); +} + bool TextColor::IsLegacy() const noexcept { return IsIndex16() || (IsIndex256() && _index < 16); diff --git a/src/buffer/out/TextColor.h b/src/buffer/out/TextColor.h index bcd8d27806a..16698fbd94c 100644 --- a/src/buffer/out/TextColor.h +++ b/src/buffer/out/TextColor.h @@ -77,6 +77,7 @@ struct TextColor friend constexpr bool operator==(const TextColor& a, const TextColor& b) noexcept; friend constexpr bool operator!=(const TextColor& a, const TextColor& b) noexcept; + bool CanBeBrightened() const noexcept; bool IsLegacy() const noexcept; bool IsIndex16() const noexcept; bool IsIndex256() const noexcept; diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 32e5d680185..9907533ba76 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -162,7 +162,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT XtermEngine::UpdateDrawingBrushes(const TextAttribute& textAttributes, - const gsl::not_null pData, + const gsl::not_null /*pData*/, const bool /*isSettingDefaultBrushes*/) noexcept { //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE @@ -174,7 +174,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE RETURN_IF_FAILED(_UpdateUnderline(textAttributes)); // The base xterm mode only knows about 16 colors - return VtEngine::_16ColorUpdateDrawingBrushes(textAttributes, pData, _colorTable); + return VtEngine::_16ColorUpdateDrawingBrushes(textAttributes); } // Routine Description: diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 7d6ca062013..feafe528884 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -252,66 +252,61 @@ using namespace Microsoft::Console::Types; // Routine Description: // - Write a VT sequence to change the current colors of text. It will try to -// find the colors in the color table that are nearest to the input colors, -// and write those indices to the pipe. +// find ANSI colors that are nearest to the input colors, and write those +// indices to the pipe. // Arguments: // - textAttributes: Text attributes to use for the colors. -// - pData: The interface to console data structures required for rendering. -// - ColorTable: An array of colors to find the closest match to. -// - cColorTable: size of the color table. // Return Value: // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT VtEngine::_16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes, - const IRenderData* pData, - const std::basic_string_view colorTable) noexcept +[[nodiscard]] HRESULT VtEngine::_16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept { - const COLORREF colorForeground = pData->GetForegroundColor(textAttributes); - const COLORREF colorBackground = pData->GetBackgroundColor(textAttributes); - const bool fgChanged = colorForeground != _LastFG; - const bool bgChanged = colorBackground != _LastBG; - const bool fgIsDefault = colorForeground == _colorProvider.GetDefaultForeground(); - const bool bgIsDefault = colorBackground == _colorProvider.GetDefaultBackground(); - const bool isBold = textAttributes.IsBold(); + const auto fg = textAttributes.GetForeground(); + const auto bg = textAttributes.GetBackground(); + auto lastFg = _lastTextAttributes.GetForeground(); + auto lastBg = _lastTextAttributes.GetBackground(); - // If both the FG and BG should be the defaults, emit a SGR reset. - if ((fgChanged || bgChanged) && fgIsDefault && bgIsDefault) + // If either FG or BG have changed to default, emit a SGR reset. + // We can't reset FG and BG to default individually. + if ((fg.IsDefault() && !lastFg.IsDefault()) || (bg.IsDefault() && !lastBg.IsDefault())) { - // SGR Reset will also clear out the boldness of the text. + // SGR Reset will clear all attributes. RETURN_IF_FAILED(_SetGraphicsDefault()); - _LastFG = colorForeground; - _LastBG = colorBackground; - _lastWasBold = false; - // I'm not sure this is possible currently, but if the text is bold, but - // default colors, make sure we bold it. - if (isBold) - { - RETURN_IF_FAILED(_SetBold(isBold)); - _lastWasBold = isBold; - } + _lastTextAttributes = {}; + lastFg = {}; + lastBg = {}; } - else - { - if (_lastWasBold != isBold) - { - RETURN_IF_FAILED(_SetBold(isBold)); - _lastWasBold = isBold; - } - if (fgChanged) - { - const WORD wNearestFg = ::FindNearestTableIndex(colorForeground, colorTable); - RETURN_IF_FAILED(_SetGraphicsRendition16Color(wNearestFg, true)); + // We use the legacy color calculations to generate an approximation of the + // colors in the 16-color table. + auto fgIndex = fg.GetLegacyIndex(0); + auto bgIndex = bg.GetLegacyIndex(0); - _LastFG = colorForeground; - } + // If the bold attribute is set, and the foreground can be brightened, then do so. + const bool brighten = textAttributes.IsBold() && fg.CanBeBrightened(); + fgIndex |= (brighten ? FOREGROUND_INTENSITY : 0); - if (bgChanged) - { - const WORD wNearestBg = ::FindNearestTableIndex(colorBackground, colorTable); - RETURN_IF_FAILED(_SetGraphicsRendition16Color(wNearestBg, false)); + // To actually render bright colors, though, we need to use SGR bold. + const auto needBold = fgIndex > 7; + if (needBold != _lastTextAttributes.IsBold()) + { + RETURN_IF_FAILED(_SetBold(needBold)); + _lastTextAttributes.SetBold(needBold); + } - _LastBG = colorBackground; - } + // After which we drop the hight bits, since only colors 0 to 7 are supported. + fgIndex &= 7; + bgIndex &= 7; + + if (!fg.IsDefault() && (lastFg.IsDefault() || fgIndex != lastFg.GetIndex())) + { + RETURN_IF_FAILED(_SetGraphicsRendition16Color(fgIndex, true)); + _lastTextAttributes.SetIndexedForeground(fgIndex); + } + + if (!bg.IsDefault() && (lastBg.IsDefault() || bgIndex != lastBg.GetIndex())) + { + RETURN_IF_FAILED(_SetGraphicsRendition16Color(bgIndex, false)); + _lastTextAttributes.SetIndexedBackground(bgIndex); } return S_OK; diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index 1682297f91a..dfefc312d41 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -32,9 +32,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, _hFile(std::move(pipe)), _colorProvider(colorProvider), _lastTextAttributes(INVALID_COLOR, INVALID_COLOR), - _LastFG(INVALID_COLOR), - _LastBG(INVALID_COLOR), - _lastWasBold(false), _lastViewport(initialViewport), _invalidMap(initialViewport.Dimensions()), _lastText({ 0 }), diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 3245a828067..08a22f14dfe 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -119,9 +119,6 @@ namespace Microsoft::Console::Render const Microsoft::Console::IDefaultColorProvider& _colorProvider; TextAttribute _lastTextAttributes; - COLORREF _LastFG; - COLORREF _LastBG; - bool _lastWasBold; Microsoft::Console::Types::Viewport _lastViewport; @@ -206,9 +203,7 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT _MoveCursor(const COORD coord) noexcept = 0; [[nodiscard]] HRESULT _RgbUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept; - [[nodiscard]] HRESULT _16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes, - const IRenderData* pData, - const std::basic_string_view colorTable) noexcept; + [[nodiscard]] HRESULT _16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept; bool _WillWriteSingleChar() const; From 1c31b4fc55cb2957ea33c8d49a92478b8ede58c4 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 14 Jun 2020 01:00:16 +0100 Subject: [PATCH 07/15] Add support for reverse video in the 16-color Xterm engine and correct the order attributes are applied. --- src/renderer/vt/XtermEngine.cpp | 42 ++++++++++++--------------------- src/renderer/vt/XtermEngine.hpp | 2 -- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 9907533ba76..f7390baead1 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -133,24 +133,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, return S_OK; } -// Routine Description: -// - Write a VT sequence to either start or stop underlining text. -// Arguments: -// - textAttributes: text attributes containing information about the -// underlining state of the text. -// Return Value: -// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. -[[nodiscard]] HRESULT XtermEngine::_UpdateUnderline(const TextAttribute& textAttributes) noexcept -{ - bool textUnderlined = textAttributes.IsUnderlined(); - if (textUnderlined != _lastTextAttributes.IsUnderlined()) - { - RETURN_IF_FAILED(_SetUnderline(textUnderlined)); - _lastTextAttributes.SetUnderline(textUnderlined); - } - return S_OK; -} - // Routine Description: // - Write a VT sequence to change the current colors of text. Only writes // 16-color attributes. @@ -165,16 +147,22 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, const gsl::not_null /*pData*/, const bool /*isSettingDefaultBrushes*/) noexcept { - //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE - // flag is there. If the state of that flag is different then our - // current state, change the underlining state. - // We have to do this here, instead of in PaintBufferGridLines, because - // we'll have already painted the text by the time PaintBufferGridLines - // is called. - // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE - RETURN_IF_FAILED(_UpdateUnderline(textAttributes)); // The base xterm mode only knows about 16 colors - return VtEngine::_16ColorUpdateDrawingBrushes(textAttributes); + RETURN_IF_FAILED(VtEngine::_16ColorUpdateDrawingBrushes(textAttributes)); + + // And the only supported meta attributes are reverse video and underline + if (textAttributes.IsReverseVideo() != _lastTextAttributes.IsReverseVideo()) + { + RETURN_IF_FAILED(_SetReverseVideo(textAttributes.IsReverseVideo())); + _lastTextAttributes.SetReverseVideo(textAttributes.IsReverseVideo()); + } + if (textAttributes.IsUnderlined() != _lastTextAttributes.IsUnderlined()) + { + RETURN_IF_FAILED(_SetUnderline(textAttributes.IsUnderlined())); + _lastTextAttributes.SetUnderline(textAttributes.IsUnderlined()); + } + + return S_OK; } // Routine Description: diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 8748fe5f9db..496b516c236 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -62,8 +62,6 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT _MoveCursor(const COORD coord) noexcept override; - [[nodiscard]] HRESULT _UpdateUnderline(const TextAttribute& textAttributes) noexcept; - [[nodiscard]] HRESULT _DoUpdateTitle(const std::wstring& newTitle) noexcept override; #ifdef UNIT_TESTING From fb3afaf16c476512d0c0fa19af9840e682bebcd9 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 14 Jun 2020 01:31:02 +0100 Subject: [PATCH 08/15] Update unit tests to match the new behavior of the VT render engines. --- src/host/ut_host/VtRendererTests.cpp | 55 +++++++++++++++++----------- src/inc/test/CommonState.hpp | 2 +- 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 8d884fb5b6d..3fa0153b581 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -486,47 +486,53 @@ void VtRendererTest::Xterm256TestColors() // test actually uses an RGB value instead of the closest match. Log::Comment(NoThrowString().Format( - L"Begin by setting the default colors - FG,BG = BRIGHT_WHITE,DARK_BLACK")); + L"Begin by setting the default colors")); qExpectedInput.push_back("\x1b[m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({}, &renderData, false)); TestPaint(*engine, [&]() { + TextAttribute textAttributes; + Log::Comment(NoThrowString().Format( L"----Change only the BG----")); + textAttributes.SetIndexedBackground(FOREGROUND_RED); qExpectedInput.push_back("\x1b[41m"); // Background DARK_RED - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[4] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the FG----")); + textAttributes.SetIndexedForeground(FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE); qExpectedInput.push_back("\x1b[37m"); // Foreground DARK_WHITE - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], g_ColorTable[4] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the BG to something not in the table----")); - qExpectedInput.push_back("\x1b[48;2;1;1;1m"); // Background DARK_BLACK - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], 0x010101 }, + textAttributes.SetBackground(0x010101); + qExpectedInput.push_back("\x1b[48;2;1;1;1m"); // Background RGB(1,1,1) + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the BG to the 'Default' background----")); - qExpectedInput.push_back("\x1b[49m"); // Background DARK_BLACK - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], g_ColorTable[0] }, + textAttributes.SetDefaultBackground(); + qExpectedInput.push_back("\x1b[49m"); // Background default + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( L"----Back to defaults----")); - + textAttributes = {}; qExpectedInput.push_back("\x1b[m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); }); @@ -535,7 +541,7 @@ void VtRendererTest::Xterm256TestColors() Log::Comment(NoThrowString().Format( L"Make sure that color setting persists across EndPaint/StartPaint")); qExpectedInput.push_back(EMPTY_CALLBACK_SENTINEL); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({}, &renderData, false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback @@ -946,47 +952,54 @@ void VtRendererTest::XtermTestColors() L"Test changing the text attributes")); Log::Comment(NoThrowString().Format( - L"Begin by setting the default colors - FG,BG = BRIGHT_WHITE,DARK_BLACK")); + L"Begin by setting the default colors")); qExpectedInput.push_back("\x1b[m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({}, &renderData, false)); TestPaint(*engine, [&]() { + TextAttribute textAttributes; + Log::Comment(NoThrowString().Format( L"----Change only the BG----")); + textAttributes.SetBackground(g_ColorTable[4]); qExpectedInput.push_back("\x1b[41m"); // Background DARK_RED - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[4] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the FG----")); + textAttributes.SetForeground(g_ColorTable[7]); qExpectedInput.push_back("\x1b[37m"); // Foreground DARK_WHITE - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], g_ColorTable[4] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the BG to something not in the table----")); + textAttributes.SetBackground(0x010101); qExpectedInput.push_back("\x1b[40m"); // Background DARK_BLACK - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], 0x010101 }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( L"----Change only the BG to the 'Default' background----")); - qExpectedInput.push_back("\x1b[40m"); // Background DARK_BLACK - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[7], g_ColorTable[0] }, + textAttributes.SetDefaultBackground(); + qExpectedInput.push_back("\x1b[m"); // Both foreground and background default + qExpectedInput.push_back("\x1b[37m"); // Reapply foreground DARK_WHITE + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( L"----Back to defaults----")); - + textAttributes = {}; qExpectedInput.push_back("\x1b[m"); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); }); @@ -995,7 +1008,7 @@ void VtRendererTest::XtermTestColors() Log::Comment(NoThrowString().Format( L"Make sure that color setting persists across EndPaint/StartPaint")); qExpectedInput.push_back(EMPTY_CALLBACK_SENTINEL); - VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({ g_ColorTable[15], g_ColorTable[0] }, + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes({}, &renderData, false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback diff --git a/src/inc/test/CommonState.hpp b/src/inc/test/CommonState.hpp index 4c5fde73628..7d68df71f2a 100644 --- a/src/inc/test/CommonState.hpp +++ b/src/inc/test/CommonState.hpp @@ -157,7 +157,7 @@ class CommonState UINT uiCursorSize = 12; - auto initialAttributes = useDefaultAttributes ? gci.GetDefaultAttributes() : + auto initialAttributes = useDefaultAttributes ? TextAttribute{} : TextAttribute{ FOREGROUND_BLUE | FOREGROUND_GREEN | BACKGROUND_RED | BACKGROUND_INTENSITY }; m_backupTextBufferInfo.swap(gci.pCurrentScreenBuffer->_textBuffer); From 5cc251fb74ccf978710afa6abb6074437370178c Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 14 Jun 2020 03:12:59 +0100 Subject: [PATCH 09/15] Cleanup FindTableIndex methods that are no longer needed. --- src/host/conattrs.cpp | 117 ------------------------------------------ src/inc/conattrs.hpp | 7 --- 2 files changed, 124 deletions(-) diff --git a/src/host/conattrs.cpp b/src/host/conattrs.cpp index b64ff757fea..721ea309d26 100644 --- a/src/host/conattrs.cpp +++ b/src/host/conattrs.cpp @@ -17,100 +17,6 @@ Author(s): #include "..\inc\conattrs.hpp" #include -struct _HSL -{ - double h, s, l; - - // constructs an HSL color from a RGB Color. - explicit _HSL(const COLORREF rgb) - { - const double r = (double)GetRValue(rgb); - const double g = (double)GetGValue(rgb); - const double b = (double)GetBValue(rgb); - - const auto [min, max] = std::minmax({ r, g, b }); - - const auto diff = max - min; - const auto sum = max + min; - // Luminance - l = max / 255.0; - - // Saturation - s = (max == 0) ? 0 : diff / max; - - //Hue - double q = (diff == 0) ? 0 : 60.0 / diff; - if (max == r) - { - h = (g < b) ? ((360.0 + q * (g - b)) / 360.0) : ((q * (g - b)) / 360.0); - } - else if (max == g) - { - h = (120.0 + q * (b - r)) / 360.0; - } - else if (max == b) - { - h = (240.0 + q * (r - g)) / 360.0; - } - else - { - h = 0; - } - } -}; - -//Routine Description: -// Finds the "distance" between a given HSL color and an RGB color, using the HSL color space. -// This function is designed such that the caller would convert one RGB color to HSL ahead of time, -// then compare many RGB colors to that first color. -//Arguments: -// - phslColorA - a pointer to the first color, as a HSL color. -// - rgbColorB - The second color to compare, in RGB color. -// Return value: -// The "distance" between the two. -static double _FindDifference(const _HSL* const phslColorA, const COLORREF rgbColorB) -{ - const _HSL hslColorB = _HSL(rgbColorB); - return sqrt(pow((hslColorB.h - phslColorA->h), 2) + - pow((hslColorB.s - phslColorA->s), 2) + - pow((hslColorB.l - phslColorA->l), 2)); -} - -//Routine Description: -// For a given RGB color Color, finds the nearest color from the array ColorTable, and returns the index of that match. -//Arguments: -// - Color - The RGB color to fine the nearest color to. -// - ColorTable - The array of colors to find a nearest color from. -// Return value: -// The index in ColorTable of the nearest match to Color. -WORD FindNearestTableIndex(const COLORREF Color, const std::basic_string_view ColorTable) -{ - // Quick check for an exact match in the color table: - for (WORD i = 0; i < ColorTable.size(); i++) - { - if (Color == ColorTable[i]) - { - return i; - } - } - - // Did not find an exact match - do an expensive comparison to the elements - // of the table to find the nearest color. - const _HSL hslColor = _HSL(Color); - WORD closest = 0; - double minDiff = _FindDifference(&hslColor, ColorTable[0]); - for (WORD i = 1; i < ColorTable.size(); i++) - { - double diff = _FindDifference(&hslColor, ColorTable[i]); - if (diff < minDiff) - { - minDiff = diff; - closest = i; - } - } - return closest; -} - // Function Description: // - Converts the value of a xterm color table index to the windows color table equivalent. // Arguments: @@ -144,26 +50,3 @@ WORD Xterm256ToWindowsIndex(const size_t xtermTableEntry) noexcept return xtermTableEntry < 16 ? XtermToWindowsIndex(xtermTableEntry) : static_cast(xtermTableEntry); } - -//Routine Description: -// Returns the exact entry from the color table, if it's in there. -//Arguments: -// - Color - The RGB color to fine the nearest color to. -// - ColorTable - The array of colors to find a nearest color from. -// Return value: -// The index in ColorTable of the nearest match to Color. -bool FindTableIndex(const COLORREF Color, - const std::basic_string_view ColorTable, - _Out_ WORD* const pFoundIndex) -{ - *pFoundIndex = 0; - for (WORD i = 0; i < ColorTable.size(); i++) - { - if (ColorTable[i] == Color) - { - *pFoundIndex = i; - return true; - } - } - return false; -} diff --git a/src/inc/conattrs.hpp b/src/inc/conattrs.hpp index a33deaf49ff..4c4ad36f1d8 100644 --- a/src/inc/conattrs.hpp +++ b/src/inc/conattrs.hpp @@ -23,13 +23,6 @@ enum class ExtendedAttributes : BYTE }; DEFINE_ENUM_FLAG_OPERATORS(ExtendedAttributes); -WORD FindNearestTableIndex(const COLORREF Color, - const std::basic_string_view ColorTable); - -bool FindTableIndex(const COLORREF Color, - const std::basic_string_view ColorTable, - _Out_ WORD* const pFoundIndex); - WORD XtermToWindowsIndex(const size_t index) noexcept; WORD Xterm256ToWindowsIndex(const size_t index) noexcept; WORD XtermToLegacy(const size_t xtermForeground, const size_t xtermBackground); From 8b7dbf44dd90c6c99b7bbd1d3e460c79ea30e53a Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 14 Jun 2020 03:47:32 +0100 Subject: [PATCH 10/15] Remove the ColorProvider interface that's no longer needed. --- .../ConptyRoundtripTests.cpp | 1 - src/host/VtIo.cpp | 3 -- src/host/server.h | 5 +-- src/host/ut_host/ConptyOutputTests.cpp | 1 - src/host/ut_host/VtIoTests.cpp | 33 ++------------- src/host/ut_host/VtRendererTests.cpp | 41 ++++++------------- src/inc/IDefaultColorProvider.hpp | 28 ------------- src/interactivity/win32/menu.cpp | 3 +- src/renderer/vt/Xterm256Engine.cpp | 3 +- src/renderer/vt/Xterm256Engine.hpp | 1 - src/renderer/vt/XtermEngine.cpp | 3 +- src/renderer/vt/XtermEngine.hpp | 1 - src/renderer/vt/state.cpp | 2 - src/renderer/vt/vtrenderer.hpp | 4 -- 14 files changed, 19 insertions(+), 110 deletions(-) delete mode 100644 src/inc/IDefaultColorProvider.hpp diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index 215d48ac492..b2369e64b04 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -116,7 +116,6 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final Viewport initialViewport = currentBuffer.GetViewport(); auto vtRenderEngine = std::make_unique(std::move(hFile), - gci, initialViewport, gci.Get16ColorTable()); auto pfn = std::bind(&ConptyRoundtripTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2); diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index abbdf001550..afa4b272d23 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -155,20 +155,17 @@ VtIo::VtIo() : { case VtIoMode::XTERM_256: _pVtRenderEngine = std::make_unique(std::move(_hOutput), - gci, initialViewport, gci.Get16ColorTable()); break; case VtIoMode::XTERM: _pVtRenderEngine = std::make_unique(std::move(_hOutput), - gci, initialViewport, gci.Get16ColorTable(), false); break; case VtIoMode::XTERM_ASCII: _pVtRenderEngine = std::make_unique(std::move(_hOutput), - gci, initialViewport, gci.Get16ColorTable(), true); diff --git a/src/host/server.h b/src/host/server.h index 9dcb035c380..df96e102912 100644 --- a/src/host/server.h +++ b/src/host/server.h @@ -29,8 +29,6 @@ Revision History: #include "..\host\RenderData.hpp" -#include "..\inc\IDefaultColorProvider.hpp" - // clang-format off // Flags flags #define CONSOLE_IS_ICONIC 0x00000001 @@ -75,8 +73,7 @@ class CommandHistory; class CONSOLE_INFORMATION : public Settings, - public Microsoft::Console::IIoProvider, - public Microsoft::Console::IDefaultColorProvider + public Microsoft::Console::IIoProvider { public: CONSOLE_INFORMATION(); diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index 6b0e4adbd39..d8c2f85789b 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -85,7 +85,6 @@ class ConptyOutputTests Viewport initialViewport = currentBuffer.GetViewport(); auto vtRenderEngine = std::make_unique(std::move(hFile), - gci, initialViewport, gci.Get16ColorTable()); auto pfn = std::bind(&ConptyOutputTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2); diff --git a/src/host/ut_host/VtIoTests.cpp b/src/host/ut_host/VtIoTests.cpp index d8ea0e436ed..404eb7a21dd 100644 --- a/src/host/ut_host/VtIoTests.cpp +++ b/src/host/ut_host/VtIoTests.cpp @@ -45,20 +45,6 @@ class Microsoft::Console::VirtualTerminal::VtIoTests TEST_METHOD(BasicAnonymousPipeOpeningWithSignalChannelTest); }; -class VtIoTestColorProvider : public Microsoft::Console::IDefaultColorProvider -{ -public: - virtual ~VtIoTestColorProvider() = default; - COLORREF GetDefaultForeground() const - { - return RGB(0xff, 0xff, 0xff); - } - COLORREF GetDefaultBackground() const - { - return RGB(0, 0, 0); - } -}; - using namespace Microsoft::Console; using namespace Microsoft::Console::VirtualTerminal; using namespace Microsoft::Console::Render; @@ -112,7 +98,6 @@ void VtIoTests::DtorTestJustEngine() const WORD colorTableSize = 16; COLORREF colorTable[colorTableSize]; - VtIoTestColorProvider p; Log::Comment(NoThrowString().Format( L"New some engines and delete them")); @@ -123,21 +108,21 @@ void VtIoTests::DtorTestJustEngine() wil::unique_hfile hOutputFile; hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderer256 = new Xterm256Engine(std::move(hOutputFile), p, SetUpViewport(), colorTable); + auto pRenderer256 = new Xterm256Engine(std::move(hOutputFile), SetUpViewport(), colorTable); Log::Comment(NoThrowString().Format(L"Made Xterm256Engine")); delete pRenderer256; Log::Comment(NoThrowString().Format(L"Deleted.")); hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderEngineXterm = new XtermEngine(std::move(hOutputFile), p, SetUpViewport(), colorTable, false); + auto pRenderEngineXterm = new XtermEngine(std::move(hOutputFile), SetUpViewport(), colorTable, false); Log::Comment(NoThrowString().Format(L"Made XtermEngine")); delete pRenderEngineXterm; Log::Comment(NoThrowString().Format(L"Deleted.")); hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderEngineXtermAscii = new XtermEngine(std::move(hOutputFile), p, SetUpViewport(), colorTable, true); + auto pRenderEngineXtermAscii = new XtermEngine(std::move(hOutputFile), SetUpViewport(), colorTable, true); Log::Comment(NoThrowString().Format(L"Made XtermEngine")); delete pRenderEngineXtermAscii; Log::Comment(NoThrowString().Format(L"Deleted.")); @@ -154,7 +139,6 @@ void VtIoTests::DtorTestDeleteVtio() const WORD colorTableSize = 16; COLORREF colorTable[colorTableSize]; - VtIoTestColorProvider p; Log::Comment(NoThrowString().Format( L"New some engines and delete them")); @@ -170,7 +154,6 @@ void VtIoTests::DtorTestDeleteVtio() VtIo* vtio = new VtIo(); Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable); Log::Comment(NoThrowString().Format(L"Made Xterm256Engine")); @@ -181,7 +164,6 @@ void VtIoTests::DtorTestDeleteVtio() vtio = new VtIo(); Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable, false); @@ -193,7 +175,6 @@ void VtIoTests::DtorTestDeleteVtio() vtio = new VtIo(); Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable, true); @@ -213,7 +194,6 @@ void VtIoTests::DtorTestStackAlloc() const WORD colorTableSize = 16; COLORREF colorTable[colorTableSize]; - VtIoTestColorProvider p; Log::Comment(NoThrowString().Format( L"make some engines and let them fall out of scope")); @@ -228,7 +208,6 @@ void VtIoTests::DtorTestStackAlloc() { VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable); } @@ -237,7 +216,6 @@ void VtIoTests::DtorTestStackAlloc() { VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable, false); @@ -247,7 +225,6 @@ void VtIoTests::DtorTestStackAlloc() { VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable, true); @@ -265,7 +242,6 @@ void VtIoTests::DtorTestStackAllocMany() const WORD colorTableSize = 16; COLORREF colorTable[colorTableSize]; - VtIoTestColorProvider p; Log::Comment(NoThrowString().Format( L"Try an make a whole bunch all at once, and have them all fall out of scope at once.")); @@ -279,14 +255,12 @@ void VtIoTests::DtorTestStackAllocMany() hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio1; vtio1._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable); hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio2; vtio2._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable, false); @@ -294,7 +268,6 @@ void VtIoTests::DtorTestStackAllocMany() hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio3; vtio3._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - p, SetUpViewport(), colorTable, true); diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 3fa0153b581..817417b5c9e 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -37,23 +37,6 @@ static const std::string CURSOR_HOME = "\x1b[H"; // We don't use null because that will confuse the VERIFY macros re: string length. const char* const EMPTY_CALLBACK_SENTINEL = "\xff"; -class VtRenderTestColorProvider : public Microsoft::Console::IDefaultColorProvider -{ -public: - virtual ~VtRenderTestColorProvider() = default; - - COLORREF GetDefaultForeground() const - { - return g_ColorTable[15]; - } - COLORREF GetDefaultBackground() const - { - return g_ColorTable[0]; - } -}; - -VtRenderTestColorProvider p; - class Microsoft::Console::Render::VtRendererTest { TEST_CLASS(VtRendererTest); @@ -184,7 +167,7 @@ void VtRendererTest::TestPaint(VtEngine& engine, std::function pfn) void VtRendererTest::VtSequenceHelperTests() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -241,7 +224,7 @@ void VtRendererTest::VtSequenceHelperTests() void VtRendererTest::Xterm256TestInvalidate() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -429,7 +412,7 @@ void VtRendererTest::Xterm256TestInvalidate() void VtRendererTest::Xterm256TestColors() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); RenderData renderData; @@ -551,7 +534,7 @@ void VtRendererTest::Xterm256TestColors() void VtRendererTest::Xterm256TestCursor() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -703,7 +686,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes() } wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -747,7 +730,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes() void VtRendererTest::XtermTestInvalidate() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, false); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable, false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -934,7 +917,7 @@ void VtRendererTest::XtermTestInvalidate() void VtRendererTest::XtermTestColors() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, false); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable, false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); RenderData renderData; @@ -1018,7 +1001,7 @@ void VtRendererTest::XtermTestColors() void VtRendererTest::XtermTestCursor() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, false); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable, false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1127,7 +1110,7 @@ void VtRendererTest::XtermTestCursor() void VtRendererTest::TestWrapping() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1180,7 +1163,7 @@ void VtRendererTest::TestResize() { Viewport view = SetUpViewport(); wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - auto engine = std::make_unique(std::move(hFile), p, view, g_ColorTable); + auto engine = std::make_unique(std::move(hFile), view, g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1217,7 +1200,7 @@ void VtRendererTest::TestCursorVisibility() { Viewport view = SetUpViewport(); wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - auto engine = std::make_unique(std::move(hFile), p, view, g_ColorTable); + auto engine = std::make_unique(std::move(hFile), view, g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1342,7 +1325,7 @@ void VtRendererTest::FormattedString() Viewport view = SetUpViewport(); wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - auto engine = std::make_unique(std::move(hFile), p, view, g_ColorTable); + auto engine = std::make_unique(std::move(hFile), view, g_ColorTable); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); diff --git a/src/inc/IDefaultColorProvider.hpp b/src/inc/IDefaultColorProvider.hpp deleted file mode 100644 index df99f5c0354..00000000000 --- a/src/inc/IDefaultColorProvider.hpp +++ /dev/null @@ -1,28 +0,0 @@ -/*++ -Copyright (c) Microsoft Corporation. -Licensed under the MIT license. - -Module Name: -- IDefaultColorProvider.hpp - -Abstract: -- Provides an abstraction for acquiring the default colors of a console object. - -Author(s): -- Mike Griese (migrie) 11 Oct 2017 ---*/ - -#pragma once - -namespace Microsoft::Console -{ - class IDefaultColorProvider - { - public: - virtual ~IDefaultColorProvider() = 0; - virtual COLORREF GetDefaultForeground() const = 0; - virtual COLORREF GetDefaultBackground() const = 0; - }; - - inline Microsoft::Console::IDefaultColorProvider::~IDefaultColorProvider() {} -} diff --git a/src/interactivity/win32/menu.cpp b/src/interactivity/win32/menu.cpp index bba8402750c..9a4cfc7864e 100644 --- a/src/interactivity/win32/menu.cpp +++ b/src/interactivity/win32/menu.cpp @@ -375,8 +375,7 @@ void Menu::s_ShowPropertiesDialog(HWND const hwnd, BOOL const Defaults) pStateInfo->InterceptCopyPaste = gci.GetInterceptCopyPaste(); - // Get the properties from the settings - CONSOLE_INFORMATION overloads - // these methods to implement IDefaultColorProvider + // Get the properties from the settings pStateInfo->DefaultForeground = gci.GetDefaultForegroundColor(); pStateInfo->DefaultBackground = gci.GetDefaultBackgroundColor(); diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 5c013da74b2..97417effad7 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -9,10 +9,9 @@ using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, - const IDefaultColorProvider& colorProvider, const Viewport initialViewport, const std::basic_string_view colorTable) : - XtermEngine(std::move(hPipe), colorProvider, initialViewport, colorTable, false) + XtermEngine(std::move(hPipe), initialViewport, colorTable, false) { } diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 00b17cce50f..648868c9ccc 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -24,7 +24,6 @@ namespace Microsoft::Console::Render { public: Xterm256Engine(_In_ wil::unique_hfile hPipe, - const Microsoft::Console::IDefaultColorProvider& colorProvider, const Microsoft::Console::Types::Viewport initialViewport, const std::basic_string_view colorTable); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index f7390baead1..cf75796ad2b 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -10,11 +10,10 @@ using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, - const IDefaultColorProvider& colorProvider, const Viewport initialViewport, const std::basic_string_view colorTable, const bool fUseAsciiOnly) : - VtEngine(std::move(hPipe), colorProvider, initialViewport), + VtEngine(std::move(hPipe), initialViewport), _colorTable(colorTable), _fUseAsciiOnly(fUseAsciiOnly), _needToDisableCursor(false), diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 496b516c236..1d555a15fe2 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -28,7 +28,6 @@ namespace Microsoft::Console::Render { public: XtermEngine(_In_ wil::unique_hfile hPipe, - const Microsoft::Console::IDefaultColorProvider& colorProvider, const Microsoft::Console::Types::Viewport initialViewport, const std::basic_string_view colorTable, const bool fUseAsciiOnly); diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index dfefc312d41..b6f4560f8a3 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -26,11 +26,9 @@ const COORD VtEngine::INVALID_COORDS = { -1, -1 }; // Return Value: // - An instance of a Renderer. VtEngine::VtEngine(_In_ wil::unique_hfile pipe, - const IDefaultColorProvider& colorProvider, const Viewport initialViewport) : RenderEngineBase(), _hFile(std::move(pipe)), - _colorProvider(colorProvider), _lastTextAttributes(INVALID_COLOR, INVALID_COLOR), _lastViewport(initialViewport), _invalidMap(initialViewport.Dimensions()), diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 08a22f14dfe..0e7db13fe5b 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -16,7 +16,6 @@ Author(s): #pragma once #include "../inc/RenderEngineBase.hpp" -#include "../../inc/IDefaultColorProvider.hpp" #include "../../inc/ITerminalOutputConnection.hpp" #include "../../inc/ITerminalOwner.hpp" #include "../../types/inc/Viewport.hpp" @@ -42,7 +41,6 @@ namespace Microsoft::Console::Render static const COORD INVALID_COORDS; VtEngine(_In_ wil::unique_hfile hPipe, - const Microsoft::Console::IDefaultColorProvider& colorProvider, const Microsoft::Console::Types::Viewport initialViewport); virtual ~VtEngine() override = default; @@ -116,8 +114,6 @@ namespace Microsoft::Console::Render std::string _formatBuffer; - const Microsoft::Console::IDefaultColorProvider& _colorProvider; - TextAttribute _lastTextAttributes; Microsoft::Console::Types::Viewport _lastViewport; From fe254f0c17c02df5bb58b2bb6784ec6bf9bd4f06 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 14 Jun 2020 11:36:06 +0100 Subject: [PATCH 11/15] Remove color tables from the VT render engines that are no longer needed. --- .../ConptyRoundtripTests.cpp | 3 +- src/host/VtIo.cpp | 5 +- src/host/ut_host/ConptyOutputTests.cpp | 3 +- src/host/ut_host/VtIoTests.cpp | 33 +++---------- src/host/ut_host/VtRendererTests.cpp | 47 ++++++------------- src/renderer/vt/Xterm256Engine.cpp | 5 +- src/renderer/vt/Xterm256Engine.hpp | 3 +- src/renderer/vt/XtermEngine.cpp | 2 - src/renderer/vt/XtermEngine.hpp | 2 - 9 files changed, 26 insertions(+), 77 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp index b2369e64b04..f4cc7a24063 100644 --- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp @@ -116,8 +116,7 @@ class TerminalCoreUnitTests::ConptyRoundtripTests final Viewport initialViewport = currentBuffer.GetViewport(); auto vtRenderEngine = std::make_unique(std::move(hFile), - initialViewport, - gci.Get16ColorTable()); + initialViewport); auto pfn = std::bind(&ConptyRoundtripTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2); vtRenderEngine->SetTestCallback(pfn); diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index afa4b272d23..17789de025f 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -155,19 +155,16 @@ VtIo::VtIo() : { case VtIoMode::XTERM_256: _pVtRenderEngine = std::make_unique(std::move(_hOutput), - initialViewport, - gci.Get16ColorTable()); + initialViewport); break; case VtIoMode::XTERM: _pVtRenderEngine = std::make_unique(std::move(_hOutput), initialViewport, - gci.Get16ColorTable(), false); break; case VtIoMode::XTERM_ASCII: _pVtRenderEngine = std::make_unique(std::move(_hOutput), initialViewport, - gci.Get16ColorTable(), true); break; default: diff --git a/src/host/ut_host/ConptyOutputTests.cpp b/src/host/ut_host/ConptyOutputTests.cpp index d8c2f85789b..a76d5c4f833 100644 --- a/src/host/ut_host/ConptyOutputTests.cpp +++ b/src/host/ut_host/ConptyOutputTests.cpp @@ -85,8 +85,7 @@ class ConptyOutputTests Viewport initialViewport = currentBuffer.GetViewport(); auto vtRenderEngine = std::make_unique(std::move(hFile), - initialViewport, - gci.Get16ColorTable()); + initialViewport); auto pfn = std::bind(&ConptyOutputTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2); vtRenderEngine->SetTestCallback(pfn); diff --git a/src/host/ut_host/VtIoTests.cpp b/src/host/ut_host/VtIoTests.cpp index 404eb7a21dd..6ef1113b86b 100644 --- a/src/host/ut_host/VtIoTests.cpp +++ b/src/host/ut_host/VtIoTests.cpp @@ -96,9 +96,6 @@ void VtIoTests::DtorTestJustEngine() L"It's here because of the strange nature of VtEngine having members\n" L"that are only defined in UNIT_TESTING")); - const WORD colorTableSize = 16; - COLORREF colorTable[colorTableSize]; - Log::Comment(NoThrowString().Format( L"New some engines and delete them")); for (int i = 0; i < 25; ++i) @@ -108,21 +105,21 @@ void VtIoTests::DtorTestJustEngine() wil::unique_hfile hOutputFile; hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderer256 = new Xterm256Engine(std::move(hOutputFile), SetUpViewport(), colorTable); + auto pRenderer256 = new Xterm256Engine(std::move(hOutputFile), SetUpViewport()); Log::Comment(NoThrowString().Format(L"Made Xterm256Engine")); delete pRenderer256; Log::Comment(NoThrowString().Format(L"Deleted.")); hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderEngineXterm = new XtermEngine(std::move(hOutputFile), SetUpViewport(), colorTable, false); + auto pRenderEngineXterm = new XtermEngine(std::move(hOutputFile), SetUpViewport(), false); Log::Comment(NoThrowString().Format(L"Made XtermEngine")); delete pRenderEngineXterm; Log::Comment(NoThrowString().Format(L"Deleted.")); hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderEngineXtermAscii = new XtermEngine(std::move(hOutputFile), SetUpViewport(), colorTable, true); + auto pRenderEngineXtermAscii = new XtermEngine(std::move(hOutputFile), SetUpViewport(), true); Log::Comment(NoThrowString().Format(L"Made XtermEngine")); delete pRenderEngineXtermAscii; Log::Comment(NoThrowString().Format(L"Deleted.")); @@ -137,9 +134,6 @@ void VtIoTests::DtorTestDeleteVtio() L"It's here because of the strange nature of VtEngine having members\n" L"that are only defined in UNIT_TESTING")); - const WORD colorTableSize = 16; - COLORREF colorTable[colorTableSize]; - Log::Comment(NoThrowString().Format( L"New some engines and delete them")); for (int i = 0; i < 25; ++i) @@ -154,8 +148,7 @@ void VtIoTests::DtorTestDeleteVtio() VtIo* vtio = new VtIo(); Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), - SetUpViewport(), - colorTable); + SetUpViewport()); Log::Comment(NoThrowString().Format(L"Made Xterm256Engine")); delete vtio; Log::Comment(NoThrowString().Format(L"Deleted.")); @@ -165,7 +158,6 @@ void VtIoTests::DtorTestDeleteVtio() Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), SetUpViewport(), - colorTable, false); Log::Comment(NoThrowString().Format(L"Made XtermEngine")); delete vtio; @@ -176,7 +168,6 @@ void VtIoTests::DtorTestDeleteVtio() Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), SetUpViewport(), - colorTable, true); Log::Comment(NoThrowString().Format(L"Made XtermEngine")); delete vtio; @@ -192,9 +183,6 @@ void VtIoTests::DtorTestStackAlloc() L"It's here because of the strange nature of VtEngine having members\n" L"that are only defined in UNIT_TESTING")); - const WORD colorTableSize = 16; - COLORREF colorTable[colorTableSize]; - Log::Comment(NoThrowString().Format( L"make some engines and let them fall out of scope")); for (int i = 0; i < 25; ++i) @@ -208,8 +196,7 @@ void VtIoTests::DtorTestStackAlloc() { VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - SetUpViewport(), - colorTable); + SetUpViewport()); } hOutputFile.reset(INVALID_HANDLE_VALUE); @@ -217,7 +204,6 @@ void VtIoTests::DtorTestStackAlloc() VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), SetUpViewport(), - colorTable, false); } @@ -226,7 +212,6 @@ void VtIoTests::DtorTestStackAlloc() VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), SetUpViewport(), - colorTable, true); } } @@ -240,9 +225,6 @@ void VtIoTests::DtorTestStackAllocMany() L"It's here because of the strange nature of VtEngine having members\n" L"that are only defined in UNIT_TESTING")); - const WORD colorTableSize = 16; - COLORREF colorTable[colorTableSize]; - Log::Comment(NoThrowString().Format( L"Try an make a whole bunch all at once, and have them all fall out of scope at once.")); for (int i = 0; i < 25; ++i) @@ -255,21 +237,18 @@ void VtIoTests::DtorTestStackAllocMany() hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio1; vtio1._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - SetUpViewport(), - colorTable); + SetUpViewport()); hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio2; vtio2._pVtRenderEngine = std::make_unique(std::move(hOutputFile), SetUpViewport(), - colorTable, false); hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio3; vtio3._pVtRenderEngine = std::make_unique(std::move(hOutputFile), SetUpViewport(), - colorTable, true); } } diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 817417b5c9e..7abc67a6040 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -28,7 +28,6 @@ using namespace Microsoft::Console; using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; -COLORREF g_ColorTable[COLOR_TABLE_SIZE]; static const std::string CLEAR_SCREEN = "\x1b[2J"; static const std::string CURSOR_HOME = "\x1b[H"; // Sometimes when we're expecting the renderengine to not write anything, @@ -43,24 +42,6 @@ class Microsoft::Console::Render::VtRendererTest TEST_CLASS_SETUP(ClassSetup) { - // clang-format off - g_ColorTable[0] = RGB( 12, 12, 12); // Black - g_ColorTable[1] = RGB( 0, 55, 218); // Dark Blue - g_ColorTable[2] = RGB( 19, 161, 14); // Dark Green - g_ColorTable[3] = RGB( 58, 150, 221); // Dark Cyan - g_ColorTable[4] = RGB(197, 15, 31); // Dark Red - g_ColorTable[5] = RGB(136, 23, 152); // Dark Magenta - g_ColorTable[6] = RGB(193, 156, 0); // Dark Yellow - g_ColorTable[7] = RGB(204, 204, 204); // Dark White - g_ColorTable[8] = RGB(118, 118, 118); // Bright Black - g_ColorTable[9] = RGB( 59, 120, 255); // Bright Blue - g_ColorTable[10] = RGB( 22, 198, 12); // Bright Green - g_ColorTable[11] = RGB( 97, 214, 214); // Bright Cyan - g_ColorTable[12] = RGB(231, 72, 86); // Bright Red - g_ColorTable[13] = RGB(180, 0, 158); // Bright Magenta - g_ColorTable[14] = RGB(249, 241, 165); // Bright Yellow - g_ColorTable[15] = RGB(242, 242, 242); // White - // clang-format on return true; } @@ -167,7 +148,7 @@ void VtRendererTest::TestPaint(VtEngine& engine, std::function pfn) void VtRendererTest::VtSequenceHelperTests() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport()); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -224,7 +205,7 @@ void VtRendererTest::VtSequenceHelperTests() void VtRendererTest::Xterm256TestInvalidate() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport()); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -412,7 +393,7 @@ void VtRendererTest::Xterm256TestInvalidate() void VtRendererTest::Xterm256TestColors() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport()); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); RenderData renderData; @@ -534,7 +515,7 @@ void VtRendererTest::Xterm256TestColors() void VtRendererTest::Xterm256TestCursor() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport()); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -686,7 +667,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes() } wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport()); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -730,7 +711,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes() void VtRendererTest::XtermTestInvalidate() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable, false); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -917,7 +898,7 @@ void VtRendererTest::XtermTestInvalidate() void VtRendererTest::XtermTestColors() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable, false); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); RenderData renderData; @@ -947,7 +928,7 @@ void VtRendererTest::XtermTestColors() Log::Comment(NoThrowString().Format( L"----Change only the BG----")); - textAttributes.SetBackground(g_ColorTable[4]); + textAttributes.SetBackground(RGB(197, 15, 31)); qExpectedInput.push_back("\x1b[41m"); // Background DARK_RED VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, @@ -955,7 +936,7 @@ void VtRendererTest::XtermTestColors() Log::Comment(NoThrowString().Format( L"----Change only the FG----")); - textAttributes.SetForeground(g_ColorTable[7]); + textAttributes.SetForeground(RGB(204, 204, 204)); qExpectedInput.push_back("\x1b[37m"); // Foreground DARK_WHITE VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, @@ -1001,7 +982,7 @@ void VtRendererTest::XtermTestColors() void VtRendererTest::XtermTestCursor() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable, false); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1110,7 +1091,7 @@ void VtRendererTest::XtermTestCursor() void VtRendererTest::TestWrapping() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), g_ColorTable); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport()); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1163,7 +1144,7 @@ void VtRendererTest::TestResize() { Viewport view = SetUpViewport(); wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - auto engine = std::make_unique(std::move(hFile), view, g_ColorTable); + auto engine = std::make_unique(std::move(hFile), view); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1200,7 +1181,7 @@ void VtRendererTest::TestCursorVisibility() { Viewport view = SetUpViewport(); wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - auto engine = std::make_unique(std::move(hFile), view, g_ColorTable); + auto engine = std::make_unique(std::move(hFile), view); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1325,7 +1306,7 @@ void VtRendererTest::FormattedString() Viewport view = SetUpViewport(); wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - auto engine = std::make_unique(std::move(hFile), view, g_ColorTable); + auto engine = std::make_unique(std::move(hFile), view); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 97417effad7..a449d912bba 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -9,9 +9,8 @@ using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, - const Viewport initialViewport, - const std::basic_string_view colorTable) : - XtermEngine(std::move(hPipe), initialViewport, colorTable, false) + const Viewport initialViewport) : + XtermEngine(std::move(hPipe), initialViewport, false) { } diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 648868c9ccc..fe07065e024 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -24,8 +24,7 @@ namespace Microsoft::Console::Render { public: Xterm256Engine(_In_ wil::unique_hfile hPipe, - const Microsoft::Console::Types::Viewport initialViewport, - const std::basic_string_view colorTable); + const Microsoft::Console::Types::Viewport initialViewport); virtual ~Xterm256Engine() override = default; diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index cf75796ad2b..a67568a4836 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -11,10 +11,8 @@ using namespace Microsoft::Console::Types; XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, const Viewport initialViewport, - const std::basic_string_view colorTable, const bool fUseAsciiOnly) : VtEngine(std::move(hPipe), initialViewport), - _colorTable(colorTable), _fUseAsciiOnly(fUseAsciiOnly), _needToDisableCursor(false), _lastCursorIsVisible(false), diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 1d555a15fe2..cfcbbaef6d2 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -29,7 +29,6 @@ namespace Microsoft::Console::Render public: XtermEngine(_In_ wil::unique_hfile hPipe, const Microsoft::Console::Types::Viewport initialViewport, - const std::basic_string_view colorTable, const bool fUseAsciiOnly); virtual ~XtermEngine() override = default; @@ -53,7 +52,6 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT WriteTerminalW(const std::wstring_view str) noexcept override; protected: - const std::basic_string_view _colorTable; const bool _fUseAsciiOnly; bool _needToDisableCursor; bool _lastCursorIsVisible; From 24311f3eba4a84243a598e37af2fdf02b3889aff Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 16 Jun 2020 10:55:17 +0100 Subject: [PATCH 12/15] Update the BGFX and WDDM render engines to use the new interface. --- src/interactivity/onecore/BgfxEngine.cpp | 8 +++----- src/interactivity/onecore/BgfxEngine.hpp | 6 ++---- src/renderer/wddmcon/WddmConRenderer.cpp | 8 +++----- src/renderer/wddmcon/WddmConRenderer.hpp | 6 ++---- 4 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/interactivity/onecore/BgfxEngine.cpp b/src/interactivity/onecore/BgfxEngine.cpp index 715ebd31f5d..8480540a65f 100644 --- a/src/interactivity/onecore/BgfxEngine.cpp +++ b/src/interactivity/onecore/BgfxEngine.cpp @@ -194,13 +194,11 @@ BgfxEngine::BgfxEngine(PVOID SharedViewBase, LONG DisplayHeight, LONG DisplayWid return HRESULT_FROM_NT(Status); } -[[nodiscard]] HRESULT BgfxEngine::UpdateDrawingBrushes(COLORREF const /*colorForeground*/, - COLORREF const /*colorBackground*/, - const WORD legacyColorAttribute, - const ExtendedAttributes /*extendedAttrs*/, +[[nodiscard]] HRESULT BgfxEngine::UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null /*pData*/, bool const /*isSettingDefaultBrushes*/) noexcept { - _currentLegacyColorAttribute = legacyColorAttribute; + _currentLegacyColorAttribute = textAttributes.GetLegacyAttributes(); return S_OK; } diff --git a/src/interactivity/onecore/BgfxEngine.hpp b/src/interactivity/onecore/BgfxEngine.hpp index d2fc5488bbf..d68be91470c 100644 --- a/src/interactivity/onecore/BgfxEngine.hpp +++ b/src/interactivity/onecore/BgfxEngine.hpp @@ -58,10 +58,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintCursor(const CursorOptions& options) noexcept override; - [[nodiscard]] HRESULT UpdateDrawingBrushes(COLORREF const colorForeground, - COLORREF const colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + [[nodiscard]] HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, bool const isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& fiFontInfoDesired, FontInfo& fiFontInfo) noexcept override; [[nodiscard]] HRESULT UpdateDpi(int const iDpi) noexcept override; diff --git a/src/renderer/wddmcon/WddmConRenderer.cpp b/src/renderer/wddmcon/WddmConRenderer.cpp index d2d1890581b..77409bb6b03 100644 --- a/src/renderer/wddmcon/WddmConRenderer.cpp +++ b/src/renderer/wddmcon/WddmConRenderer.cpp @@ -305,13 +305,11 @@ bool WddmConEngine::IsInitialized() return S_OK; } -[[nodiscard]] HRESULT WddmConEngine::UpdateDrawingBrushes(COLORREF const /*colorForeground*/, - COLORREF const /*colorBackground*/, - const WORD legacyColorAttribute, - const ExtendedAttributes /*extendedAttrs*/, +[[nodiscard]] HRESULT WddmConEngine::UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null /*pData*/, bool const /*isSettingDefaultBrushes*/) noexcept { - _currentLegacyColorAttribute = legacyColorAttribute; + _currentLegacyColorAttribute = textAttributes.GetLegacyAttributes(); return S_OK; } diff --git a/src/renderer/wddmcon/WddmConRenderer.hpp b/src/renderer/wddmcon/WddmConRenderer.hpp index ad137131c77..e4acac3fc96 100644 --- a/src/renderer/wddmcon/WddmConRenderer.hpp +++ b/src/renderer/wddmcon/WddmConRenderer.hpp @@ -50,10 +50,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT PaintCursor(const CursorOptions& options) noexcept override; - [[nodiscard]] HRESULT UpdateDrawingBrushes(COLORREF const colorForeground, - COLORREF const colorBackground, - const WORD legacyColorAttribute, - const ExtendedAttributes extendedAttrs, + [[nodiscard]] HRESULT UpdateDrawingBrushes(const TextAttribute& textAttributes, + const gsl::not_null pData, bool const isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& fiFontInfoDesired, FontInfo& fiFontInfo) noexcept override; [[nodiscard]] HRESULT UpdateDpi(int const iDpi) noexcept override; From a26ed32da0e092f0bec732aaea7ee50cf981f15f Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sat, 20 Jun 2020 21:13:27 +0100 Subject: [PATCH 13/15] Add VT renderer tests to verify rendition attributes are retained when colors are reset. --- src/host/ut_host/VtRendererTests.cpp | 155 +++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 7abc67a6040..4c52c7fa244 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -27,6 +27,7 @@ namespace Microsoft using namespace Microsoft::Console; using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; +using namespace Microsoft::Console::VirtualTerminal::DispatchTypes; static const std::string CLEAR_SCREEN = "\x1b[2J"; static const std::string CURSOR_HOME = "\x1b[H"; @@ -67,10 +68,12 @@ class Microsoft::Console::Render::VtRendererTest TEST_METHOD(Xterm256TestColors); TEST_METHOD(Xterm256TestCursor); TEST_METHOD(Xterm256TestExtendedAttributes); + TEST_METHOD(Xterm256TestAttributesAcrossReset); TEST_METHOD(XtermTestInvalidate); TEST_METHOD(XtermTestColors); TEST_METHOD(XtermTestCursor); + TEST_METHOD(XtermTestAttributesAcrossReset); TEST_METHOD(FormattedString); @@ -708,6 +711,90 @@ void VtRendererTest::Xterm256TestExtendedAttributes() VerifyExpectedInputsDrained(); } +void VtRendererTest::Xterm256TestAttributesAcrossReset() +{ + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:renditionAttribute", L"{1, 3, 4, 5, 7, 8, 9}") + END_TEST_METHOD_PROPERTIES() + + int renditionAttribute; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"renditionAttribute", renditionAttribute)); + + std::stringstream renditionSequence; + renditionSequence << "\x1b[" << renditionAttribute << "m"; + + wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport()); + auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); + engine->SetTestCallback(pfn); + RenderData renderData; + + Log::Comment(L"Make sure rendition attributes are retained when colors are reset"); + + Log::Comment(L"----Start With All Attributes Reset----"); + TextAttribute textAttributes = {}; + qExpectedInput.push_back("\x1b[m"); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + switch (renditionAttribute) + { + case GraphicsOptions::BoldBright: + Log::Comment(L"----Set Bold Attribute----"); + textAttributes.SetBold(true); + break; + case GraphicsOptions::Italics: + Log::Comment(L"----Set Italics Attribute----"); + textAttributes.SetItalics(true); + break; + case GraphicsOptions::Underline: + Log::Comment(L"----Set Underline Attribute----"); + textAttributes.SetUnderline(true); + break; + case GraphicsOptions::BlinkOrXterm256Index: + Log::Comment(L"----Set Blink Attribute----"); + textAttributes.SetBlinking(true); + break; + case GraphicsOptions::Negative: + Log::Comment(L"----Set Negative Attribute----"); + textAttributes.SetReverseVideo(true); + break; + case GraphicsOptions::Invisible: + Log::Comment(L"----Set Invisible Attribute----"); + textAttributes.SetInvisible(true); + break; + case GraphicsOptions::CrossedOut: + Log::Comment(L"----Set Crossed Out Attribute----"); + textAttributes.SetCrossedOut(true); + break; + } + qExpectedInput.push_back(renditionSequence.str()); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + Log::Comment(L"----Set Green Foreground----"); + textAttributes.SetIndexedForeground(FOREGROUND_GREEN); + qExpectedInput.push_back("\x1b[32m"); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + Log::Comment(L"----Reset Default Foreground and Retain Rendition----"); + textAttributes.SetDefaultForeground(); + qExpectedInput.push_back("\x1b[m"); + qExpectedInput.push_back(renditionSequence.str()); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + Log::Comment(L"----Set Green Background----"); + textAttributes.SetIndexedBackground(FOREGROUND_GREEN); + qExpectedInput.push_back("\x1b[42m"); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + Log::Comment(L"----Reset Default Background and Retain Rendition----"); + textAttributes.SetDefaultBackground(); + qExpectedInput.push_back("\x1b[m"); + qExpectedInput.push_back(renditionSequence.str()); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + VerifyExpectedInputsDrained(); +} + void VtRendererTest::XtermTestInvalidate() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); @@ -1088,6 +1175,74 @@ void VtRendererTest::XtermTestCursor() }); } +void VtRendererTest::XtermTestAttributesAcrossReset() +{ + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:renditionAttribute", L"{1, 4, 7}") + END_TEST_METHOD_PROPERTIES() + + int renditionAttribute; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"renditionAttribute", renditionAttribute)); + + std::stringstream renditionSequence; + renditionSequence << "\x1b[" << renditionAttribute << "m"; + + wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); + std::unique_ptr engine = std::make_unique(std::move(hFile), SetUpViewport(), false); + auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); + engine->SetTestCallback(pfn); + RenderData renderData; + + Log::Comment(L"Make sure rendition attributes are retained when colors are reset"); + + Log::Comment(L"----Start With All Attributes Reset----"); + TextAttribute textAttributes = {}; + qExpectedInput.push_back("\x1b[m"); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + switch (renditionAttribute) + { + case GraphicsOptions::BoldBright: + Log::Comment(L"----Set Bold Attribute----"); + textAttributes.SetBold(true); + break; + case GraphicsOptions::Underline: + Log::Comment(L"----Set Underline Attribute----"); + textAttributes.SetUnderline(true); + break; + case GraphicsOptions::Negative: + Log::Comment(L"----Set Negative Attribute----"); + textAttributes.SetReverseVideo(true); + break; + } + qExpectedInput.push_back(renditionSequence.str()); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + Log::Comment(L"----Set Green Foreground----"); + textAttributes.SetIndexedForeground(FOREGROUND_GREEN); + qExpectedInput.push_back("\x1b[32m"); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + Log::Comment(L"----Reset Default Foreground and Retain Rendition----"); + textAttributes.SetDefaultForeground(); + qExpectedInput.push_back("\x1b[m"); + qExpectedInput.push_back(renditionSequence.str()); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + Log::Comment(L"----Set Green Background----"); + textAttributes.SetIndexedBackground(FOREGROUND_GREEN); + qExpectedInput.push_back("\x1b[42m"); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + Log::Comment(L"----Reset Default Background and Retain Rendition----"); + textAttributes.SetDefaultBackground(); + qExpectedInput.push_back("\x1b[m"); + qExpectedInput.push_back(renditionSequence.str()); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); + + VerifyExpectedInputsDrained(); +} + void VtRendererTest::TestWrapping() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); From 81b76b498c25d79207be2622d3ba3748170b424c Mon Sep 17 00:00:00 2001 From: James Holderness Date: Sun, 21 Jun 2020 01:24:19 +0100 Subject: [PATCH 14/15] Extend the VT renderer color tests to cover all the different color types. --- src/host/ut_host/VtRendererTests.cpp | 87 ++++++++++++++++++++++++---- 1 file changed, 76 insertions(+), 11 deletions(-) diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 4c52c7fa244..1bf0cb3e8d3 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -449,8 +449,8 @@ void VtRendererTest::Xterm256TestColors() }); // Now also do the body of the 16color test as well. - // The only change is that the "Change only the BG to something not in the table" - // test actually uses an RGB value instead of the closest match. + // However, instead of using a closest match ANSI color, we can reproduce + // the exact RGB or 256-color index value stored in the TextAttribute. Log::Comment(NoThrowString().Format( L"Begin by setting the default colors")); @@ -480,9 +480,17 @@ void VtRendererTest::Xterm256TestColors() false)); Log::Comment(NoThrowString().Format( - L"----Change only the BG to something not in the table----")); - textAttributes.SetBackground(0x010101); - qExpectedInput.push_back("\x1b[48;2;1;1;1m"); // Background RGB(1,1,1) + L"----Change only the BG to an RGB value----")); + textAttributes.SetBackground(RGB(19, 161, 14)); + qExpectedInput.push_back("\x1b[48;2;19;161;14m"); // Background RGB(19,161,14) + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, + &renderData, + false)); + + Log::Comment(NoThrowString().Format( + L"----Change only the FG to an RGB value----")); + textAttributes.SetForeground(RGB(193, 156, 0)); + qExpectedInput.push_back("\x1b[38;2;193;156;0m"); // Foreground RGB(193,156,0) VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); @@ -495,6 +503,30 @@ void VtRendererTest::Xterm256TestColors() &renderData, false)); + Log::Comment(NoThrowString().Format( + L"----Change only the FG to a 256-color index----")); + textAttributes.SetIndexedForeground256(FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE); + qExpectedInput.push_back("\x1b[38;5;7m"); // Foreground DARK_WHITE (256-Color Index) + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, + &renderData, + false)); + + Log::Comment(NoThrowString().Format( + L"----Change only the BG to a 256-color index----")); + textAttributes.SetIndexedBackground256(FOREGROUND_RED); + qExpectedInput.push_back("\x1b[48;5;1m"); // Background DARK_RED (256-Color Index) + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, + &renderData, + false)); + + Log::Comment(NoThrowString().Format( + L"----Change only the FG to the 'Default' foreground----")); + textAttributes.SetDefaultForeground(); + qExpectedInput.push_back("\x1b[39m"); // Background default + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, + &renderData, + false)); + Log::Comment(NoThrowString().Format( L"----Back to defaults----")); textAttributes = {}; @@ -1015,7 +1047,7 @@ void VtRendererTest::XtermTestColors() Log::Comment(NoThrowString().Format( L"----Change only the BG----")); - textAttributes.SetBackground(RGB(197, 15, 31)); + textAttributes.SetIndexedBackground(FOREGROUND_RED); qExpectedInput.push_back("\x1b[41m"); // Background DARK_RED VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, @@ -1023,16 +1055,24 @@ void VtRendererTest::XtermTestColors() Log::Comment(NoThrowString().Format( L"----Change only the FG----")); - textAttributes.SetForeground(RGB(204, 204, 204)); + textAttributes.SetIndexedForeground(FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE); qExpectedInput.push_back("\x1b[37m"); // Foreground DARK_WHITE VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); Log::Comment(NoThrowString().Format( - L"----Change only the BG to something not in the table----")); - textAttributes.SetBackground(0x010101); - qExpectedInput.push_back("\x1b[40m"); // Background DARK_BLACK + L"----Change only the BG to an RGB value----")); + textAttributes.SetBackground(RGB(19, 161, 14)); + qExpectedInput.push_back("\x1b[42m"); // Background DARK_GREEN + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, + &renderData, + false)); + + Log::Comment(NoThrowString().Format( + L"----Change only the FG to an RGB value----")); + textAttributes.SetForeground(RGB(193, 156, 0)); + qExpectedInput.push_back("\x1b[33m"); // Foreground DARK_YELLOW VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); @@ -1041,7 +1081,32 @@ void VtRendererTest::XtermTestColors() L"----Change only the BG to the 'Default' background----")); textAttributes.SetDefaultBackground(); qExpectedInput.push_back("\x1b[m"); // Both foreground and background default - qExpectedInput.push_back("\x1b[37m"); // Reapply foreground DARK_WHITE + qExpectedInput.push_back("\x1b[33m"); // Reapply foreground DARK_YELLOW + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, + &renderData, + false)); + + Log::Comment(NoThrowString().Format( + L"----Change only the FG to a 256-color index----")); + textAttributes.SetIndexedForeground256(FOREGROUND_RED | FOREGROUND_GREEN | FOREGROUND_BLUE); + qExpectedInput.push_back("\x1b[37m"); // Foreground DARK_WHITE + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, + &renderData, + false)); + + Log::Comment(NoThrowString().Format( + L"----Change only the BG to a 256-color index----")); + textAttributes.SetIndexedBackground256(FOREGROUND_RED); + qExpectedInput.push_back("\x1b[41m"); // Background DARK_RED + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, + &renderData, + false)); + + Log::Comment(NoThrowString().Format( + L"----Change only the FG to the 'Default' foreground----")); + textAttributes.SetDefaultForeground(); + qExpectedInput.push_back("\x1b[m"); // Both foreground and background default + qExpectedInput.push_back("\x1b[41m"); // Reapply background DARK_RED VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(textAttributes, &renderData, false)); From 9c4e0a6c01bc1c4f682c91ddd5c3824ddc6f7dc8 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Tue, 23 Jun 2020 01:09:45 +0100 Subject: [PATCH 15/15] Update src/renderer/vt/paint.cpp Co-authored-by: Michael Niksa --- src/renderer/vt/paint.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index feafe528884..0a4fa55817f 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -293,7 +293,8 @@ using namespace Microsoft::Console::Types; _lastTextAttributes.SetBold(needBold); } - // After which we drop the hight bits, since only colors 0 to 7 are supported. + // After which we drop the high bits, since only colors 0 to 7 are supported. + fgIndex &= 7; bgIndex &= 7;