From 6a3960602f107ae609d78667173d6a66c1380037 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 25 Sep 2019 10:19:57 -0500 Subject: [PATCH 01/11] store text with extended attributes too --- src/buffer/out/TextAttribute.cpp | 14 ++++++---- src/buffer/out/TextAttribute.hpp | 48 ++++++++++++++++++++++++++------ 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp index 0c964b848e9..efa37deea5b 100644 --- a/src/buffer/out/TextAttribute.cpp +++ b/src/buffer/out/TextAttribute.cpp @@ -44,7 +44,7 @@ COLORREF TextAttribute::CalculateRgbBackground(std::basic_string_view COLORREF TextAttribute::_GetRgbForeground(std::basic_string_view colorTable, COLORREF defaultColor) const noexcept { - return _foreground.GetColor(colorTable, defaultColor, _isBold); + return _foreground.GetColor(colorTable, defaultColor, IsBold()); } // Routine Description: @@ -155,10 +155,11 @@ void TextAttribute::SetColor(const COLORREF rgbColor, const bool fIsForeground) } } -bool TextAttribute::IsBold() const noexcept -{ - return _isBold; -} +// bool TextAttribute::IsBold() const noexcept +// { +// // return _isBold; +// return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Bold); +// } bool TextAttribute::_IsReverseVideo() const noexcept { @@ -224,7 +225,8 @@ void TextAttribute::Invert() noexcept void TextAttribute::_SetBoldness(const bool isBold) noexcept { - _isBold = isBold; + WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Bold, isBold); + // _isBold = isBold; } void TextAttribute::SetDefaultForeground() noexcept diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index eb8cc6111df..f03c22ce124 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -27,6 +27,21 @@ Revision History: #include "WexTestClass.h" #endif +#pragma pack(push, 1) + +enum class ExtendedAttributes : BYTE +{ + Normal = 0x00, + Bold = 0x01, + Italics = 0x02, + Blinking = 0x04, + Invisible = 0x08, + CrossedOut = 0x10, + DoublyUnderlined = 0x20, + Faint = 0x40, +}; +DEFINE_ENUM_FLAG_OPERATORS(ExtendedAttributes); + class TextAttribute final { public: @@ -34,7 +49,8 @@ class TextAttribute final _wAttrLegacy{ 0 }, _foreground{}, _background{}, - _isBold{ false } + // _isBold{ false } + _extendedAttrs{ ExtendedAttributes::Normal } { } @@ -42,7 +58,8 @@ class TextAttribute final _wAttrLegacy{ gsl::narrow_cast(wLegacyAttr & META_ATTRS) }, _foreground{ gsl::narrow_cast(wLegacyAttr & FG_ATTRS) }, _background{ gsl::narrow_cast((wLegacyAttr & BG_ATTRS) >> 4) }, - _isBold{ false } + // _isBold{ false } + _extendedAttrs{ ExtendedAttributes::Normal } { // If we're given lead/trailing byte information with the legacy color, strip it. WI_ClearAllFlags(_wAttrLegacy, COMMON_LVB_SBCSDBCS); @@ -53,7 +70,8 @@ class TextAttribute final _wAttrLegacy{ 0 }, _foreground{ rgbForeground }, _background{ rgbBackground }, - _isBold{ false } + // _isBold{ false } + _extendedAttrs{ ExtendedAttributes::Normal } { } @@ -62,7 +80,7 @@ class TextAttribute final const BYTE fg = (_foreground.GetIndex() & FG_ATTRS); const BYTE bg = (_background.GetIndex() << 4) & BG_ATTRS; const WORD meta = (_wAttrLegacy & META_ATTRS); - return (fg | bg | meta) | (_isBold ? FOREGROUND_INTENSITY : 0); + return (fg | bg | meta) | (IsBold() ? FOREGROUND_INTENSITY : 0); } // Method Description: @@ -85,7 +103,7 @@ class TextAttribute final const BYTE fg = (fgIndex & FG_ATTRS); const BYTE bg = (bgIndex << 4) & BG_ATTRS; const WORD meta = (_wAttrLegacy & META_ATTRS); - return (fg | bg | meta) | (_isBold ? FOREGROUND_INTENSITY : 0); + return (fg | bg | meta) | (IsBold() ? FOREGROUND_INTENSITY : 0); } COLORREF CalculateRgbForeground(std::basic_string_view colorTable, @@ -131,7 +149,13 @@ class TextAttribute final friend constexpr bool operator!=(const WORD& legacyAttr, const TextAttribute& attr) noexcept; bool IsLegacy() const noexcept; - bool IsBold() const noexcept; + // bool IsBold() const noexcept; + constexpr bool IsBold() const noexcept + { + // return _isBold; + return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Bold); + // return (_extendedAttrs & ExtendedAttributes::Bold) == ExtendedAttributes::Bold; + } void SetForeground(const COLORREF rgbForeground) noexcept; void SetBackground(const COLORREF rgbBackground) noexcept; @@ -159,7 +183,8 @@ class TextAttribute final WORD _wAttrLegacy; TextColor _foreground; TextColor _background; - bool _isBold; + // bool _isBold; + ExtendedAttributes _extendedAttrs; #ifdef UNIT_TESTING friend class TextBufferTests; @@ -169,6 +194,13 @@ class TextAttribute final #endif }; +#pragma pack(pop) +// 2 for _wAttrLegacy +// 4 for _foreground +// 4 for _foreground +// 1 for _extendedAttrs +static_assert(sizeof(TextAttribute) <= 11 * sizeof(BYTE), "We should only need 11B for an entire TextColor. Any more than that is just waste"); + enum class TextAttributeBehavior { Stored, // use contained text attribute @@ -181,7 +213,7 @@ constexpr bool operator==(const TextAttribute& a, const TextAttribute& b) noexce return a._wAttrLegacy == b._wAttrLegacy && a._foreground == b._foreground && a._background == b._background && - a._isBold == b._isBold; + a._extendedAttrs == b._extendedAttrs; } constexpr bool operator!=(const TextAttribute& a, const TextAttribute& b) noexcept From bfee5fe3df96f7db57557c5b3ff67484ddc3dff3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 25 Sep 2019 11:17:27 -0500 Subject: [PATCH 02/11] Plumb attributes through all the renderers --- src/buffer/out/TextAttribute.hpp | 22 +--- src/host/ut_host/VtRendererTests.cpp | 142 +++++++++++++++++++---- src/inc/conattrs.hpp | 13 +++ src/interactivity/onecore/BgfxEngine.cpp | 2 +- src/interactivity/onecore/BgfxEngine.hpp | 2 +- src/renderer/base/renderer.cpp | 4 +- src/renderer/dx/DxRenderer.cpp | 2 +- src/renderer/dx/DxRenderer.hpp | 2 +- src/renderer/gdi/gdirenderer.hpp | 2 +- src/renderer/gdi/state.cpp | 2 +- src/renderer/inc/IRenderEngine.hpp | 2 +- src/renderer/vt/WinTelnetEngine.cpp | 8 +- src/renderer/vt/WinTelnetEngine.hpp | 2 +- src/renderer/vt/Xterm256Engine.cpp | 4 +- src/renderer/vt/Xterm256Engine.hpp | 2 +- src/renderer/vt/XtermEngine.cpp | 4 +- src/renderer/vt/XtermEngine.hpp | 2 +- src/renderer/vt/vtrenderer.hpp | 2 +- src/renderer/wddmcon/WddmConRenderer.cpp | 2 +- src/renderer/wddmcon/WddmConRenderer.hpp | 2 +- 20 files changed, 161 insertions(+), 62 deletions(-) diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index f03c22ce124..9df7a519148 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -29,19 +29,6 @@ Revision History: #pragma pack(push, 1) -enum class ExtendedAttributes : BYTE -{ - Normal = 0x00, - Bold = 0x01, - Italics = 0x02, - Blinking = 0x04, - Invisible = 0x08, - CrossedOut = 0x10, - DoublyUnderlined = 0x20, - Faint = 0x40, -}; -DEFINE_ENUM_FLAG_OPERATORS(ExtendedAttributes); - class TextAttribute final { public: @@ -149,12 +136,15 @@ class TextAttribute final friend constexpr bool operator!=(const WORD& legacyAttr, const TextAttribute& attr) noexcept; bool IsLegacy() const noexcept; - // bool IsBold() const noexcept; + constexpr bool IsBold() const noexcept { - // return _isBold; return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Bold); - // return (_extendedAttrs & ExtendedAttributes::Bold) == ExtendedAttributes::Bold; + } + + constexpr ExtendedAttributes GetExtendedAttributes() const noexcept + { + return _extendedAttrs; } void SetForeground(const COLORREF rgbForeground) noexcept; diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 9aebf916c3b..7d965372d07 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -421,25 +421,41 @@ 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(0x00030201, + 0x00070605, + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(0x00030201, + 0x00090807, + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(0x000c0b0a, + 0x00090807, + 0, + ExtendedAttributes::Normal, + false)); }); TestPaint(*engine, [&]() { 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(0x000c0b0a, + 0x00090807, + 0, + ExtendedAttributes::Normal, + false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback }); @@ -451,41 +467,69 @@ 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[4], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], + g_ColorTable[4], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], + 0x010101, + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + false)); }); TestPaint(*engine, [&]() { 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback }); } @@ -758,41 +802,65 @@ 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[4], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], + g_ColorTable[4], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], 0x010101, 0, ExtendedAttributes::Normal, 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + false)); }); TestPaint(*engine, [&]() { 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback }); } @@ -996,40 +1064,64 @@ void VtRendererTest::WinTelnetTestColors() 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[4], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], + g_ColorTable[4], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], 0x010101, 0, ExtendedAttributes::Normal, 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[7], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + false)); }); TestPaint(*engine, [&]() { 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, false, false)); + VERIFY_SUCCEEDED(engine->UpdateDrawingBrushes(g_ColorTable[15], + g_ColorTable[0], + 0, + ExtendedAttributes::Normal, + false)); WriteCallback(EMPTY_CALLBACK_SENTINEL, 1); // This will make sure nothing was written to the callback }); } diff --git a/src/inc/conattrs.hpp b/src/inc/conattrs.hpp index 5e532519080..8b258c9643e 100644 --- a/src/inc/conattrs.hpp +++ b/src/inc/conattrs.hpp @@ -8,6 +8,19 @@ Licensed under the MIT license. #define BG_ATTRS (BACKGROUND_BLUE | BACKGROUND_GREEN | BACKGROUND_RED | BACKGROUND_INTENSITY) #define META_ATTRS (COMMON_LVB_LEADING_BYTE | COMMON_LVB_TRAILING_BYTE | COMMON_LVB_GRID_HORIZONTAL | COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_REVERSE_VIDEO | COMMON_LVB_UNDERSCORE) +enum class ExtendedAttributes : BYTE +{ + Normal = 0x00, + Bold = 0x01, + Italics = 0x02, + Blinking = 0x04, + Invisible = 0x08, + CrossedOut = 0x10, + DoublyUnderlined = 0x20, + Faint = 0x40, +}; +DEFINE_ENUM_FLAG_OPERATORS(ExtendedAttributes); + WORD FindNearestTableIndex(const COLORREF Color, _In_reads_(cColorTable) const COLORREF* const ColorTable, const WORD cColorTable); diff --git a/src/interactivity/onecore/BgfxEngine.cpp b/src/interactivity/onecore/BgfxEngine.cpp index 02eb42c5777..b73c769c7a5 100644 --- a/src/interactivity/onecore/BgfxEngine.cpp +++ b/src/interactivity/onecore/BgfxEngine.cpp @@ -196,7 +196,7 @@ BgfxEngine::BgfxEngine(PVOID SharedViewBase, LONG DisplayHeight, LONG DisplayWid [[nodiscard]] HRESULT BgfxEngine::UpdateDrawingBrushes(COLORREF const /*colorForeground*/, COLORREF const /*colorBackground*/, const WORD legacyColorAttribute, - const bool /*isBold*/, + const ExtendedAttributes /*extendedAttrs*/, bool const /*isSettingDefaultBrushes*/) noexcept { _currentLegacyColorAttribute = legacyColorAttribute; diff --git a/src/interactivity/onecore/BgfxEngine.hpp b/src/interactivity/onecore/BgfxEngine.hpp index 19a255503f3..9ce46e35708 100644 --- a/src/interactivity/onecore/BgfxEngine.hpp +++ b/src/interactivity/onecore/BgfxEngine.hpp @@ -60,7 +60,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(COLORREF const colorForeground, COLORREF const colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, 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/base/renderer.cpp b/src/renderer/base/renderer.cpp index e434c5d0725..64e12c32ec6 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -872,11 +872,11 @@ 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 bool isBold = textAttributes.IsBold(); + 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, isBold, isSettingDefaultBrushes)); + RETURN_IF_FAILED(pEngine->UpdateDrawingBrushes(rgbForeground, rgbBackground, legacyAttributes, extendedAttrs, isSettingDefaultBrushes)); return S_OK; } diff --git a/src/renderer/dx/DxRenderer.cpp b/src/renderer/dx/DxRenderer.cpp index 3d52a4b68d1..bdbcf0b1bf2 100644 --- a/src/renderer/dx/DxRenderer.cpp +++ b/src/renderer/dx/DxRenderer.cpp @@ -1225,7 +1225,7 @@ enum class CursorPaintType [[nodiscard]] HRESULT DxEngine::UpdateDrawingBrushes(COLORREF const colorForeground, COLORREF const colorBackground, const WORD /*legacyColorAttribute*/, - const bool /*isBold*/, + const ExtendedAttributes /*extendedAttrs*/, bool const isSettingDefaultBrushes) noexcept { _foregroundColor = _ColorFFromColorRef(colorForeground); diff --git a/src/renderer/dx/DxRenderer.hpp b/src/renderer/dx/DxRenderer.hpp index f0960120316..1a2dd48fa32 100644 --- a/src/renderer/dx/DxRenderer.hpp +++ b/src/renderer/dx/DxRenderer.hpp @@ -80,7 +80,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(COLORREF const colorForeground, COLORREF const colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, 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 079c099ddfa..4840a181985 100644 --- a/src/renderer/gdi/gdirenderer.hpp +++ b/src/renderer/gdi/gdirenderer.hpp @@ -56,7 +56,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, 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 b55dc12f0e8..85528da422d 100644 --- a/src/renderer/gdi/state.cpp +++ b/src/renderer/gdi/state.cpp @@ -184,7 +184,7 @@ GdiEngine::~GdiEngine() [[nodiscard]] HRESULT GdiEngine::UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD /*legacyColorAttribute*/, - const bool /*isBold*/, + const ExtendedAttributes /*extendedAttrs*/, const bool isSettingDefaultBrushes) noexcept { RETURN_IF_FAILED(_FlushBufferLines()); diff --git a/src/renderer/inc/IRenderEngine.hpp b/src/renderer/inc/IRenderEngine.hpp index 2d321381318..aff56a5bb58 100644 --- a/src/renderer/inc/IRenderEngine.hpp +++ b/src/renderer/inc/IRenderEngine.hpp @@ -105,7 +105,7 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, const bool isSettingDefaultBrushes) noexcept = 0; [[nodiscard]] virtual HRESULT UpdateFont(const FontInfoDesired& FontInfoDesired, _Out_ FontInfo& FontInfo) noexcept = 0; diff --git a/src/renderer/vt/WinTelnetEngine.cpp b/src/renderer/vt/WinTelnetEngine.cpp index 614c66a3be6..e36c0eeec84 100644 --- a/src/renderer/vt/WinTelnetEngine.cpp +++ b/src/renderer/vt/WinTelnetEngine.cpp @@ -36,10 +36,14 @@ WinTelnetEngine::WinTelnetEngine(_In_ wil::unique_hfile hPipe, [[nodiscard]] HRESULT WinTelnetEngine::UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD /*legacyColorAttribute*/, - const bool isBold, + const ExtendedAttributes extendedAttrs, const bool /*isSettingDefaultBrushes*/) noexcept { - return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, colorBackground, isBold, _ColorTable, _cColorTable); + return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, + colorBackground, + WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Bold), + _ColorTable, + _cColorTable); } // Routine Description: diff --git a/src/renderer/vt/WinTelnetEngine.hpp b/src/renderer/vt/WinTelnetEngine.hpp index d025d985cd8..8c27c221833 100644 --- a/src/renderer/vt/WinTelnetEngine.hpp +++ b/src/renderer/vt/WinTelnetEngine.hpp @@ -34,7 +34,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT ScrollFrame() noexcept override; diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 7a2781d5f76..cd8d8d4b6be 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -33,7 +33,7 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, [[nodiscard]] HRESULT Xterm256Engine::UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, const bool /*isSettingDefaultBrushes*/) noexcept { //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE @@ -46,7 +46,7 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, return VtEngine::_RgbUpdateDrawingBrushes(colorForeground, colorBackground, - isBold, + WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Bold), _ColorTable, _cColorTable); } diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 7ad288b3426..738330b5388 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -35,7 +35,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, const bool isSettingDefaultBrushes) noexcept override; private: diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index b84166cb107..31d9be9c1bf 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -163,7 +163,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, [[nodiscard]] HRESULT XtermEngine::UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, const bool /*isSettingDefaultBrushes*/) noexcept { //When we update the brushes, check the wAttrs to see if the LVB_UNDERSCORE @@ -175,7 +175,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, RETURN_IF_FAILED(_UpdateUnderline(legacyColorAttribute)); // The base xterm mode only knows about 16 colors - return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, colorBackground, isBold, _ColorTable, _cColorTable); + return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, colorBackground, WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Bold), _ColorTable, _cColorTable); } // Routine Description: diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 0078030a5a5..cc1ee4c7589 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -43,7 +43,7 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, const bool isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT PaintBufferLine(std::basic_string_view const clusters, const COORD coord, diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index d95ecd5d87c..dae4a7fa348 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -69,7 +69,7 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, const bool isSettingDefaultBrushes) noexcept = 0; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& pfiFontInfoDesired, _Out_ FontInfo& pfiFontInfo) noexcept override; diff --git a/src/renderer/wddmcon/WddmConRenderer.cpp b/src/renderer/wddmcon/WddmConRenderer.cpp index 150d4a1e6e7..23804ec31f2 100644 --- a/src/renderer/wddmcon/WddmConRenderer.cpp +++ b/src/renderer/wddmcon/WddmConRenderer.cpp @@ -307,7 +307,7 @@ bool WddmConEngine::IsInitialized() [[nodiscard]] HRESULT WddmConEngine::UpdateDrawingBrushes(COLORREF const /*colorForeground*/, COLORREF const /*colorBackground*/, const WORD legacyColorAttribute, - const bool /*isBold*/, + const ExtendedAttributes /*extendedAttrs*/, bool const /*isSettingDefaultBrushes*/) noexcept { _currentLegacyColorAttribute = legacyColorAttribute; diff --git a/src/renderer/wddmcon/WddmConRenderer.hpp b/src/renderer/wddmcon/WddmConRenderer.hpp index 714e977e3d3..c214d585a8e 100644 --- a/src/renderer/wddmcon/WddmConRenderer.hpp +++ b/src/renderer/wddmcon/WddmConRenderer.hpp @@ -52,7 +52,7 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT UpdateDrawingBrushes(COLORREF const colorForeground, COLORREF const colorBackground, const WORD legacyColorAttribute, - const bool isBold, + const ExtendedAttributes extendedAttrs, bool const isSettingDefaultBrushes) noexcept override; [[nodiscard]] HRESULT UpdateFont(const FontInfoDesired& fiFontInfoDesired, FontInfo& fiFontInfo) noexcept override; [[nodiscard]] HRESULT UpdateDpi(int const iDpi) noexcept override; From 87dfc881c513dc79b2e040029db2a9faf64ed31b Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 25 Sep 2019 17:06:20 -0500 Subject: [PATCH 03/11] parse extended attrs, though we're not renderering them right --- src/buffer/out/TextAttribute.cpp | 5 ++ src/buffer/out/TextAttribute.hpp | 2 + .../TerminalCore/TerminalDispatchGraphics.cpp | 4 +- src/host/getset.cpp | 16 ++++ src/host/getset.h | 3 + src/host/outputStream.cpp | 12 +++ src/host/outputStream.hpp | 2 + src/inc/conattrs.hpp | 6 +- src/renderer/vt/Xterm256Engine.cpp | 56 ++++++++++++ src/renderer/vt/Xterm256Engine.hpp | 8 ++ src/terminal/adapter/DispatchTypes.hpp | 15 +++- src/terminal/adapter/adaptDispatch.hpp | 2 + .../adapter/adaptDispatchGraphics.cpp | 85 ++++++++++++++++++- src/terminal/adapter/conGetSet.hpp | 2 + .../adapter/ut_adapter/adapterTest.cpp | 10 +-- 15 files changed, 214 insertions(+), 14 deletions(-) diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp index efa37deea5b..747a51e90f6 100644 --- a/src/buffer/out/TextAttribute.cpp +++ b/src/buffer/out/TextAttribute.cpp @@ -216,6 +216,11 @@ void TextAttribute::Debolden() noexcept _SetBoldness(false); } +void TextAttribute::SetExtendedAttributes(const ExtendedAttributes attrs) noexcept +{ + _extendedAttrs = attrs; +} + // Routine Description: // - swaps foreground and background color void TextAttribute::Invert() noexcept diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index 9df7a519148..ece041e0c01 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -147,6 +147,8 @@ class TextAttribute final return _extendedAttrs; } + void SetExtendedAttributes(const ExtendedAttributes attrs) noexcept; + void SetForeground(const COLORREF rgbForeground) noexcept; void SetBackground(const COLORREF rgbBackground) noexcept; void SetColor(const COLORREF rgbColor, const bool fIsForeground) noexcept; diff --git a/src/cascadia/TerminalCore/TerminalDispatchGraphics.cpp b/src/cascadia/TerminalCore/TerminalDispatchGraphics.cpp index 03dab6c849a..b73b9adee1b 100644 --- a/src/cascadia/TerminalCore/TerminalDispatchGraphics.cpp +++ b/src/cascadia/TerminalCore/TerminalDispatchGraphics.cpp @@ -103,7 +103,7 @@ bool TerminalDispatch::_SetRgbColorsHelper(_In_reads_(cOptions) const DispatchTy isForeground = false; } - if (typeOpt == DispatchTypes::GraphicsOptions::RGBColor && cOptions >= 5) + if (typeOpt == DispatchTypes::GraphicsOptions::RGBColorOrFaint && cOptions >= 5) { *pcOptionsConsumed = 5; // ensure that each value fits in a byte @@ -115,7 +115,7 @@ bool TerminalDispatch::_SetRgbColorsHelper(_In_reads_(cOptions) const DispatchTy fSuccess = _terminalApi.SetTextRgbColor(color, isForeground); } - else if (typeOpt == DispatchTypes::GraphicsOptions::Xterm256Index && cOptions >= 3) + else if (typeOpt == DispatchTypes::GraphicsOptions::BlinkOrXterm256Index && cOptions >= 3) { *pcOptionsConsumed = 3; if (rgOptions[2] <= 255) // ensure that the provided index is on the table diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 16aa6633a71..4131748b163 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -970,6 +970,22 @@ void DoSrvPrivateBoldText(SCREEN_INFORMATION& screenInfo, const bool bolded) buffer.SetAttributes(attrs); } +ExtendedAttributes DoSrvPrivateGetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo) +{ + auto& buffer = screenInfo.GetActiveBuffer(); + auto attrs = buffer.GetAttributes(); + return attrs.GetExtendedAttributes(); +} + +void DoSrvPrivateSetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo, + const ExtendedAttributes extendedAttrs) +{ + auto& buffer = screenInfo.GetActiveBuffer(); + auto attrs = buffer.GetAttributes(); + attrs.SetExtendedAttributes(extendedAttrs); + buffer.SetAttributes(attrs); +} + // Routine Description: // - Sets the codepage used for translating text when calling A versions of functions affecting the output buffer. // Arguments: diff --git a/src/host/getset.h b/src/host/getset.h index 129bb8cd123..349c3c6b87c 100644 --- a/src/host/getset.h +++ b/src/host/getset.h @@ -60,6 +60,9 @@ void DoSrvPrivateSetConsoleRGBTextAttribute(SCREEN_INFORMATION& screenInfo, void DoSrvPrivateBoldText(SCREEN_INFORMATION& screenInfo, const bool bolded); +ExtendedAttributes DoSrvPrivateGetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo); +void DoSrvPrivateSetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo, const ExtendedAttributes attrs); + [[nodiscard]] NTSTATUS DoSrvPrivateEraseAll(SCREEN_INFORMATION& screenInfo); void DoSrvSetCursorStyle(SCREEN_INFORMATION& screenInfo, diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index 9f1211ce9d9..ea7312f619e 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -273,6 +273,18 @@ BOOL ConhostInternalGetSet::PrivateBoldText(const bool bolded) return TRUE; } +BOOL ConhostInternalGetSet::PrivateGetExtendedTextAttributes(ExtendedAttributes* const pAttrs) +{ + *pAttrs = DoSrvPrivateGetExtendedTextAttributes(_io.GetActiveOutputBuffer()); + return TRUE; +} + +BOOL ConhostInternalGetSet::PrivateSetExtendedTextAttributes(const ExtendedAttributes attrs) +{ + DoSrvPrivateSetExtendedTextAttributes(_io.GetActiveOutputBuffer(), attrs); + return TRUE; +} + // Routine Description: // - Connects the WriteConsoleInput API call directly into our Driver Message servicing call inside Conhost.exe // Arguments: diff --git a/src/host/outputStream.hpp b/src/host/outputStream.hpp index cc626e054ad..5cfb7bf8bbd 100644 --- a/src/host/outputStream.hpp +++ b/src/host/outputStream.hpp @@ -88,6 +88,8 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal:: const bool fIsForeground) override; BOOL PrivateBoldText(const bool bolded) override; + BOOL PrivateGetExtendedTextAttributes(ExtendedAttributes* const pAttrs) override; + BOOL PrivateSetExtendedTextAttributes(const ExtendedAttributes attrs) override; BOOL PrivateWriteConsoleInputW(_Inout_ std::deque>& events, _Out_ size_t& eventsWritten) override; diff --git a/src/inc/conattrs.hpp b/src/inc/conattrs.hpp index 8b258c9643e..90dcff6dd5d 100644 --- a/src/inc/conattrs.hpp +++ b/src/inc/conattrs.hpp @@ -16,8 +16,10 @@ enum class ExtendedAttributes : BYTE Blinking = 0x04, Invisible = 0x08, CrossedOut = 0x10, - DoublyUnderlined = 0x20, - Faint = 0x40, + // TODO:GH# add support for these to the parser as well. + Underlined = 0x20, // _technically_ different from LVB_UNDERSCORE, see GH# + DoublyUnderlined = 0x40, // Included for completeness, but not currently supported. + Faint = 0x80, // Included for completeness, but not currently supported. }; DEFINE_ENUM_FLAG_OPERATORS(ExtendedAttributes); diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index cd8d8d4b6be..8a3050df880 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -44,9 +44,65 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // is called. RETURN_IF_FAILED(_UpdateUnderline(legacyColorAttribute)); + RETURN_IF_FAILED(_UpdateExtendedAttrs(extendedAttrs)); + return VtEngine::_RgbUpdateDrawingBrushes(colorForeground, colorBackground, WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Bold), _ColorTable, _cColorTable); } + +// 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. +// 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 +{ + auto updateFlagAndState = [extendedAttrs](const ExtendedAttributes attr, + bool& lastState, + std::function beginFn, + std::function endFn) -> HRESULT { + // Unfortunately, we can't use WI_IsFlagSet here, since that requires a + // compile-time check that the flag (attr) is a _single_ flag, and that + // won't work in this case. + const bool flagSet = (extendedAttrs & attr) == attr; + if (flagSet != lastState) + { + if (flagSet) + { + RETURN_IF_FAILED(beginFn()); + } + else + { + RETURN_IF_FAILED(endFn()); + } + lastState = flagSet; + } + return S_OK; + }; + + auto hr = updateFlagAndState(ExtendedAttributes::Italics, + _usingItalics, + std::bind(&Xterm256Engine::_BeginUnderline, this), + std::bind(&Xterm256Engine::_EndUnderline, this)); + RETURN_IF_FAILED(hr); + + // const bool textItalics = WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Italics); + // if (textItalics != _usingItalics) + // { + // if (textUnderlined) + // { + // RETURN_IF_FAILED(_BeginUnderline()); + // } + // else + // { + // RETURN_IF_FAILED(_EndUnderline()); + // } + // _usingUnderLine = textUnderlined; + // } + return S_OK; +} diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 738330b5388..204574d54b4 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -39,6 +39,14 @@ namespace Microsoft::Console::Render const bool isSettingDefaultBrushes) noexcept override; private: + [[nodiscard]] HRESULT _UpdateExtendedAttrs(const ExtendedAttributes extendedAttrs) noexcept; + + bool _usingItalics{ false }; + bool _usingBlinking{ false }; + bool _usingInvisible{ false }; + bool _usingCrossedOut{ false }; + bool _usingDoublyUnderlined{ false }; + #ifdef UNIT_TESTING friend class VtRendererTest; #endif diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index e76349ce0c0..e7f1f5d92b1 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -13,21 +13,30 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes Scrollback = 3 }; + // TODO:GH# add support for DoublyUnderlined, Faint(2) to the adapter as well. enum GraphicsOptions : unsigned int { Off = 0, BoldBright = 1, - RGBColor = 2, + RGBColorOrFaint = 2, // 2 is also Faint, decreased intensity (ISO 6429). + Italics = 3, Underline = 4, - Xterm256Index = 5, + BlinkOrXterm256Index = 5, // 5 is also Blink (appears as Bold). // the 2 and 5 entries here are only for the extended graphics options // as we do not currently support those features individually Negative = 7, + Invisible = 8, + CrossedOut = 9, + DoublyUnderlined = 21, UnBold = 22, + NotItalics = 23, NoUnderline = 24, - Positive = 27, + Steady = 25, // _not_ blink + Positive = 27, // _not_ inverse + Visible = 28, // _not_ invisible + NotCrossedOut = 29, // _not_ invisible ForegroundBlack = 30, ForegroundRed = 31, ForegroundGreen = 32, diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index a4fc702c5ce..255dd2c2e6e 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -170,10 +170,12 @@ namespace Microsoft::Console::VirtualTerminal bool _SetBoldColorHelper(const DispatchTypes::GraphicsOptions option); bool _SetDefaultColorHelper(const DispatchTypes::GraphicsOptions option); + bool _SetExtendedTextAttributeHelper(const DispatchTypes::GraphicsOptions option); static bool s_IsXtermColorOption(const DispatchTypes::GraphicsOptions opt); static bool s_IsRgbColorOption(const DispatchTypes::GraphicsOptions opt); static bool s_IsBoldColorOption(const DispatchTypes::GraphicsOptions opt) noexcept; static bool s_IsDefaultColorOption(const DispatchTypes::GraphicsOptions opt) noexcept; + static bool s_IsExtendedTextAttribute(const DispatchTypes::GraphicsOptions opt) noexcept; }; } diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index f676e08adac..f68364e7e6d 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -12,6 +12,19 @@ using namespace Microsoft::Console::VirtualTerminal; using namespace Microsoft::Console::VirtualTerminal::DispatchTypes; +// Inspired from RETURN_IF_WIN32_BOOL_FALSE +// WIL doesn't include a RETURN_IF_FALSE, and RETURN_IF_WIN32_BOOL_FALSE +// will actually return the value of GLE. +#define RETURN_IF_FALSE(b) \ + do \ + { \ + BOOL __boolRet = wil::verify_bool(b); \ + if (!__boolRet) \ + { \ + return b; \ + } \ + } while (0, 0) + // Routine Description: // - Small helper to disable all color flags within a given font attributes field // Arguments: @@ -261,6 +274,27 @@ void AdaptDispatch::_SetGraphicsOptionHelper(const DispatchTypes::GraphicsOption } } +// Routine Description: +// Returns true if the GraphicsOption represents an extended color option. +// These are followed by up to 4 more values which compose the entire option. +// Return Value: +// - true if the opt is the indicator for an extended color sequence, false otherwise. +bool AdaptDispatch::s_IsExtendedTextAttribute(const DispatchTypes::GraphicsOptions opt) noexcept +{ + // TODO:GH# add support for DoublyUnderlined, Faint(RGBColorOrFaint). + // These two are currently partially implemented as other things: + // * Faint is kinda like the opposite of what bold does + // * Doubly underlined should exist in a trinary state with Underlined + return opt == DispatchTypes::GraphicsOptions::Italics || + opt == DispatchTypes::GraphicsOptions::NotItalics || + opt == DispatchTypes::GraphicsOptions::BlinkOrXterm256Index || + opt == DispatchTypes::GraphicsOptions::Steady || + opt == DispatchTypes::GraphicsOptions::Invisible || + opt == DispatchTypes::GraphicsOptions::Visible || + opt == DispatchTypes::GraphicsOptions::CrossedOut || + opt == DispatchTypes::GraphicsOptions::NotCrossedOut; +} + // Routine Description: // Returns true if the GraphicsOption represents an extended color option. // These are followed by up to 4 more values which compose the entire option. @@ -338,7 +372,7 @@ bool AdaptDispatch::_SetRgbColorsHelper(_In_reads_(cOptions) const DispatchTypes *pfIsForeground = false; } - if (typeOpt == DispatchTypes::GraphicsOptions::RGBColor && cOptions >= 5) + if (typeOpt == DispatchTypes::GraphicsOptions::RGBColorOrFaint && cOptions >= 5) { *pcOptionsConsumed = 5; // ensure that each value fits in a byte @@ -350,7 +384,7 @@ bool AdaptDispatch::_SetRgbColorsHelper(_In_reads_(cOptions) const DispatchTypes fSuccess = !!_conApi->SetConsoleRGBTextAttribute(*prgbColor, *pfIsForeground); } - else if (typeOpt == DispatchTypes::GraphicsOptions::Xterm256Index && cOptions >= 3) + else if (typeOpt == DispatchTypes::GraphicsOptions::BlinkOrXterm256Index && cOptions >= 3) { *pcOptionsConsumed = 3; if (rgOptions[2] <= 255) // ensure that the provided index is on the table @@ -372,6 +406,8 @@ bool AdaptDispatch::_SetBoldColorHelper(const DispatchTypes::GraphicsOptions opt bool AdaptDispatch::_SetDefaultColorHelper(const DispatchTypes::GraphicsOptions option) { + // TODO: reset extended attrs too + const bool fg = option == GraphicsOptions::Off || option == GraphicsOptions::ForegroundDefault; const bool bg = option == GraphicsOptions::Off || option == GraphicsOptions::BackgroundDefault; bool success = _conApi->PrivateSetDefaultAttributes(fg, bg); @@ -385,6 +421,47 @@ bool AdaptDispatch::_SetDefaultColorHelper(const DispatchTypes::GraphicsOptions return success; } +bool AdaptDispatch::_SetExtendedTextAttributeHelper(const DispatchTypes::GraphicsOptions opt) +{ + ExtendedAttributes attrs{ ExtendedAttributes::Normal }; + + RETURN_IF_FALSE(_conApi->PrivateGetExtendedTextAttributes(&attrs)); + + switch (opt) + { + case DispatchTypes::GraphicsOptions::Italics: + WI_SetFlag(attrs, ExtendedAttributes::Italics); + break; + case DispatchTypes::GraphicsOptions::NotItalics: + WI_ClearFlag(attrs, ExtendedAttributes::Italics); + break; + case DispatchTypes::GraphicsOptions::BlinkOrXterm256Index: + WI_SetFlag(attrs, ExtendedAttributes::Blinking); + break; + case DispatchTypes::GraphicsOptions::Steady: + WI_ClearFlag(attrs, ExtendedAttributes::Blinking); + break; + case DispatchTypes::GraphicsOptions::Invisible: + WI_SetFlag(attrs, ExtendedAttributes::Invisible); + break; + case DispatchTypes::GraphicsOptions::Visible: + WI_ClearFlag(attrs, ExtendedAttributes::Invisible); + break; + case DispatchTypes::GraphicsOptions::CrossedOut: + WI_SetFlag(attrs, ExtendedAttributes::CrossedOut); + break; + case DispatchTypes::GraphicsOptions::NotCrossedOut: + WI_ClearFlag(attrs, ExtendedAttributes::CrossedOut); + break; + // TODO:GH# add support for the following + // case DispatchTypes::GraphicsOptions::DoublyUnderlined: + // case DispatchTypes::GraphicsOptions::RGBColorOrFaint: + // case DispatchTypes::GraphicsOptions::DoublyUnderlined: + } + + return _conApi->PrivateSetExtendedTextAttributes(attrs); +} + // Routine Description: // - SGR - Modifies the graphical rendering options applied to the next characters written into the buffer. // - Options include colors, invert, underlines, and other "font style" type options. @@ -416,6 +493,10 @@ bool AdaptDispatch::SetGraphicsRendition(_In_reads_(cOptions) const DispatchType { fSuccess = _SetBoldColorHelper(rgOptions[i]); } + else if (s_IsExtendedTextAttribute(opt)) + { + fSuccess = _SetExtendedTextAttributeHelper(rgOptions[i]); + } else if (s_IsRgbColorOption(opt)) { COLORREF rgbColor; diff --git a/src/terminal/adapter/conGetSet.hpp b/src/terminal/adapter/conGetSet.hpp index 37b85596cab..b337fe2c7d5 100644 --- a/src/terminal/adapter/conGetSet.hpp +++ b/src/terminal/adapter/conGetSet.hpp @@ -52,6 +52,8 @@ namespace Microsoft::Console::VirtualTerminal const bool fIsForeground) = 0; virtual BOOL SetConsoleRGBTextAttribute(const COLORREF rgbColor, const bool fIsForeground) = 0; virtual BOOL PrivateBoldText(const bool bolded) = 0; + virtual BOOL PrivateGetExtendedTextAttributes(ExtendedAttributes* const pAttrs) = 0; + virtual BOOL PrivateSetExtendedTextAttributes(const ExtendedAttributes attrs) = 0; virtual BOOL PrivateWriteConsoleInputW(_Inout_ std::deque>& events, _Out_ size_t& eventsWritten) = 0; diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index b2a6f4bb88c..b8a7fce0475 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -2630,7 +2630,7 @@ class AdapterTest Log::Comment(L"Test 1: Change Foreground"); rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended; - rgOptions[1] = DispatchTypes::GraphicsOptions::Xterm256Index; + rgOptions[1] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index; rgOptions[2] = (DispatchTypes::GraphicsOptions)2; // Green _testGetSet->_wExpectedAttribute = FOREGROUND_GREEN; _testGetSet->_iExpectedXtermTableEntry = 2; @@ -2640,7 +2640,7 @@ class AdapterTest Log::Comment(L"Test 2: Change Background"); rgOptions[0] = DispatchTypes::GraphicsOptions::BackgroundExtended; - rgOptions[1] = DispatchTypes::GraphicsOptions::Xterm256Index; + rgOptions[1] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index; rgOptions[2] = (DispatchTypes::GraphicsOptions)9; // Bright Red _testGetSet->_wExpectedAttribute = FOREGROUND_GREEN | BACKGROUND_RED | BACKGROUND_INTENSITY; _testGetSet->_iExpectedXtermTableEntry = 9; @@ -2650,7 +2650,7 @@ class AdapterTest Log::Comment(L"Test 3: Change Foreground to RGB color"); rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended; - rgOptions[1] = DispatchTypes::GraphicsOptions::Xterm256Index; + rgOptions[1] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index; rgOptions[2] = (DispatchTypes::GraphicsOptions)42; // Arbitrary Color _testGetSet->_iExpectedXtermTableEntry = 42; _testGetSet->_fExpectedIsForeground = true; @@ -2659,7 +2659,7 @@ class AdapterTest Log::Comment(L"Test 4: Change Background to RGB color"); rgOptions[0] = DispatchTypes::GraphicsOptions::BackgroundExtended; - rgOptions[1] = DispatchTypes::GraphicsOptions::Xterm256Index; + rgOptions[1] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index; rgOptions[2] = (DispatchTypes::GraphicsOptions)142; // Arbitrary Color _testGetSet->_iExpectedXtermTableEntry = 142; _testGetSet->_fExpectedIsForeground = false; @@ -2671,7 +2671,7 @@ class AdapterTest // to have its own color table and translate the pre-existing RGB BG into a legacy BG. // Fortunately, the ft_api:RgbColorTests IS smart enough to test that. rgOptions[0] = DispatchTypes::GraphicsOptions::ForegroundExtended; - rgOptions[1] = DispatchTypes::GraphicsOptions::Xterm256Index; + rgOptions[1] = DispatchTypes::GraphicsOptions::BlinkOrXterm256Index; rgOptions[2] = (DispatchTypes::GraphicsOptions)9; // Bright Red _testGetSet->_wExpectedAttribute = FOREGROUND_RED | FOREGROUND_INTENSITY | BACKGROUND_RED | BACKGROUND_INTENSITY; _testGetSet->_iExpectedXtermTableEntry = 9; From dedb191e0dc36806825ea530700b861b9e5ca3fb Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Sep 2019 09:34:42 -0500 Subject: [PATCH 04/11] Render these states correctly --- src/renderer/vt/VtSequences.cpp | 88 +++++++++++++++++++ src/renderer/vt/Xterm256Engine.cpp | 38 ++++---- src/renderer/vt/Xterm256Engine.hpp | 1 - src/renderer/vt/vtrenderer.hpp | 13 ++- .../adapter/adaptDispatchGraphics.cpp | 5 +- 5 files changed, 127 insertions(+), 18 deletions(-) diff --git a/src/renderer/vt/VtSequences.cpp b/src/renderer/vt/VtSequences.cpp index 2990331a26a..a629c697b3e 100644 --- a/src/renderer/vt/VtSequences.cpp +++ b/src/renderer/vt/VtSequences.cpp @@ -345,3 +345,91 @@ using namespace Microsoft::Console::Render; { 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: +// - +// Return Value: +// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. +[[nodiscard]] HRESULT VtEngine::_EndItalics() noexcept +{ + return _Write("\x1b[23m"); +} + +// Method Description: +// - Writes a sequence to tell the terminal to start blinking text +// Arguments: +// - +// Return Value: +// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. +[[nodiscard]] HRESULT VtEngine::_BeginBlink() noexcept +{ + return _Write("\x1b[5m"); +} + +// Method Description: +// - Writes a sequence to tell the terminal to stop blinking text +// Arguments: +// - +// Return Value: +// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. +[[nodiscard]] HRESULT VtEngine::_EndBlink() noexcept +{ + return _Write("\x1b[25m"); +} + +// Method Description: +// - Writes a sequence to tell the terminal to start marking text as invisible +// Arguments: +// - +// Return Value: +// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. +[[nodiscard]] HRESULT VtEngine::_BeginInvisible() noexcept +{ + return _Write("\x1b[8m"); +} + +// Method Description: +// - Writes a sequence to tell the terminal to stop marking text as invisible +// Arguments: +// - +// Return Value: +// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. +[[nodiscard]] HRESULT VtEngine::_EndInvisible() noexcept +{ + return _Write("\x1b[28m"); +} + +// Method Description: +// - Writes a sequence to tell the terminal to start crossing-out text +// Arguments: +// - +// Return Value: +// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. +[[nodiscard]] HRESULT VtEngine::_BeginCrossedOut() noexcept +{ + return _Write("\x1b[9m"); +} + +// Method Description: +// - Writes a sequence to tell the terminal to stop crossing-out text +// Arguments: +// - +// Return Value: +// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. +[[nodiscard]] HRESULT VtEngine::_EndCrossedOut() noexcept +{ + return _Write("\x1b[29m"); +} diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 8a3050df880..f779e5dad6b 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -62,6 +62,9 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT Xterm256Engine::_UpdateExtendedAttrs(const ExtendedAttributes extendedAttrs) 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](const ExtendedAttributes attr, bool& lastState, std::function beginFn, @@ -87,22 +90,27 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, auto hr = updateFlagAndState(ExtendedAttributes::Italics, _usingItalics, - std::bind(&Xterm256Engine::_BeginUnderline, this), - std::bind(&Xterm256Engine::_EndUnderline, this)); + std::bind(&Xterm256Engine::_BeginItalics, this), + std::bind(&Xterm256Engine::_EndItalics, this)); + RETURN_IF_FAILED(hr); + + hr = updateFlagAndState(ExtendedAttributes::Blinking, + _usingBlinking, + std::bind(&Xterm256Engine::_BeginBlink, this), + std::bind(&Xterm256Engine::_EndBlink, this)); + RETURN_IF_FAILED(hr); + + hr = updateFlagAndState(ExtendedAttributes::Invisible, + _usingInvisible, + std::bind(&Xterm256Engine::_BeginInvisible, this), + std::bind(&Xterm256Engine::_EndInvisible, this)); + RETURN_IF_FAILED(hr); + + hr = updateFlagAndState(ExtendedAttributes::CrossedOut, + _usingCrossedOut, + std::bind(&Xterm256Engine::_BeginCrossedOut, this), + std::bind(&Xterm256Engine::_EndCrossedOut, this)); RETURN_IF_FAILED(hr); - // const bool textItalics = WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Italics); - // if (textItalics != _usingItalics) - // { - // if (textUnderlined) - // { - // RETURN_IF_FAILED(_BeginUnderline()); - // } - // else - // { - // RETURN_IF_FAILED(_EndUnderline()); - // } - // _usingUnderLine = textUnderlined; - // } return S_OK; } diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 204574d54b4..6a0a9f9bf14 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -45,7 +45,6 @@ namespace Microsoft::Console::Render bool _usingBlinking{ false }; bool _usingInvisible{ false }; bool _usingCrossedOut{ false }; - bool _usingDoublyUnderlined{ false }; #ifdef UNIT_TESTING friend class VtRendererTest; diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index dae4a7fa348..22bc2e0e00e 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -172,9 +172,20 @@ namespace Microsoft::Console::Render [[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 _RequestCursor() noexcept; [[nodiscard]] virtual HRESULT _MoveCursor(const COORD coord) noexcept = 0; diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index f68364e7e6d..6943a1a81d1 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -410,13 +410,16 @@ bool AdaptDispatch::_SetDefaultColorHelper(const DispatchTypes::GraphicsOptions const bool fg = option == GraphicsOptions::Off || option == GraphicsOptions::ForegroundDefault; const bool bg = option == GraphicsOptions::Off || option == GraphicsOptions::BackgroundDefault; + bool success = _conApi->PrivateSetDefaultAttributes(fg, bg); + if (success && fg && bg) { // If we're resetting both the FG & BG, also reset the meta attributes (underline) // as well as the boldness success = _conApi->PrivateSetLegacyAttributes(0, false, false, true) && - _conApi->PrivateBoldText(false); + _conApi->PrivateBoldText(false) && + _conApi->PrivateSetExtendedTextAttributes(ExtendedAttributes::Normal); } return success; } From 93aebf636a805658f7b2421bc9618a963219f5ae Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Sep 2019 10:41:04 -0500 Subject: [PATCH 05/11] Add a very extensive test --- src/host/ut_host/ScreenBufferTests.cpp | 130 +++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index aa9195ce322..6ce06d1b887 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -182,6 +182,8 @@ class ScreenBufferTests TEST_METHOD(ClearAlternateBuffer); TEST_METHOD(InitializeTabStopsInVTMode); + + TEST_METHOD(TestExtendedTextAttributes); }; void ScreenBufferTests::SingleAlternateBufferCreationTest() @@ -4403,3 +4405,131 @@ void ScreenBufferTests::InitializeTabStopsInVTMode() VERIFY_IS_TRUE(gci.GetActiveOutputBuffer().AreTabsSet()); } + +void ScreenBufferTests::TestExtendedTextAttributes() +{ + // This is a test for microsoft/terminal#2554. Refer to that issue for more + // context. + + // Run this test for each and every possible combination of states. + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:bold", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:italics", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:blink", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:invisible", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:crossedOut", L"{false, true}") + END_TEST_METHOD_PROPERTIES() + + bool bold, italics, blink, invisible, crossedOut; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"bold", bold)); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"italics", italics)); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"blink", blink)); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"invisible", invisible)); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"crossedOut", crossedOut)); + + auto& g = ServiceLocator::LocateGlobals(); + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& tbi = si.GetTextBuffer(); + auto& stateMachine = si.GetStateMachine(); + auto& cursor = tbi.GetCursor(); + + ExtendedAttributes expectedAttrs{ ExtendedAttributes::Normal }; + std::wstring vtSeq = L""; + + // Collect up a VT sequence to set the state given the method properties + if (bold) + { + WI_SetFlag(expectedAttrs, ExtendedAttributes::Bold); + vtSeq += L"\x1b[1m"; + } + if (italics) + { + WI_SetFlag(expectedAttrs, ExtendedAttributes::Italics); + vtSeq += L"\x1b[3m"; + } + if (blink) + { + WI_SetFlag(expectedAttrs, ExtendedAttributes::Blinking); + vtSeq += L"\x1b[5m"; + } + if (invisible) + { + WI_SetFlag(expectedAttrs, ExtendedAttributes::Invisible); + vtSeq += L"\x1b[8m"; + } + if (crossedOut) + { + WI_SetFlag(expectedAttrs, ExtendedAttributes::CrossedOut); + vtSeq += L"\x1b[9m"; + } + + // Helper lambda to write a VT sequence, then an "X", then check that the + // attributes of the "X" match what we think they should be. + auto validate = [&](const ExtendedAttributes expectedAttrs, + const std::wstring& vtSequence) { + auto cursorPos = cursor.GetPosition(); + + // Convert the vtSequence to something printable. Lets not set these + // attrs on the test console + std::wstring debugString = vtSequence; + { + size_t start_pos = 0; + while ((start_pos = debugString.find(L"\x1b", start_pos)) != std::string::npos) + { + debugString.replace(start_pos, 1, L"\\x1b"); + start_pos += 4; + } + } + + Log::Comment(NoThrowString().Format( + L"Testing string:\"%s\"", debugString.c_str())); + Log::Comment(NoThrowString().Format( + L"Expecting attrs:0x%02x", expectedAttrs)); + + stateMachine.ProcessString(vtSequence); + stateMachine.ProcessString(L"X"); + + auto iter = tbi.GetCellDataAt(cursorPos); + auto currentExtendedAttrs = iter->TextAttr().GetExtendedAttributes(); + VERIFY_ARE_EQUAL(expectedAttrs, currentExtendedAttrs); + }; + + // Check setting all the states collected above + validate(expectedAttrs, vtSeq); + + // One-by-one, turn off each of these states with VT, then check that the + // state matched. + if (bold) + { + WI_ClearFlag(expectedAttrs, ExtendedAttributes::Bold); + vtSeq = L"\x1b[22m"; + validate(expectedAttrs, vtSeq); + } + if (italics) + { + WI_ClearFlag(expectedAttrs, ExtendedAttributes::Italics); + vtSeq = L"\x1b[23m"; + validate(expectedAttrs, vtSeq); + } + if (blink) + { + WI_ClearFlag(expectedAttrs, ExtendedAttributes::Blinking); + vtSeq = L"\x1b[25m"; + validate(expectedAttrs, vtSeq); + } + if (invisible) + { + WI_ClearFlag(expectedAttrs, ExtendedAttributes::Invisible); + vtSeq = L"\x1b[28m"; + validate(expectedAttrs, vtSeq); + } + if (crossedOut) + { + WI_ClearFlag(expectedAttrs, ExtendedAttributes::CrossedOut); + vtSeq = L"\x1b[29m"; + validate(expectedAttrs, vtSeq); + } + + stateMachine.ProcessString(L"\x1b[0m"); +} From 2c9f539b8ef19bc95867ebd0fc950fb13fdc1491 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 26 Sep 2019 11:59:26 -0500 Subject: [PATCH 06/11] Cleanup for PR --- src/buffer/out/TextAttribute.cpp | 7 -- src/buffer/out/TextAttribute.hpp | 3 - src/host/getset.cpp | 15 +++++ src/host/outputStream.cpp | 14 ++++ src/inc/conattrs.hpp | 4 +- src/renderer/dx/DxRenderer.cpp | 2 +- src/renderer/gdi/state.cpp | 2 +- src/renderer/vt/WinTelnetEngine.cpp | 1 + src/renderer/vt/Xterm256Engine.cpp | 3 + src/renderer/vt/XtermEngine.cpp | 9 ++- src/terminal/adapter/DispatchTypes.hpp | 2 +- .../adapter/adaptDispatchGraphics.cpp | 65 +++++++++++++------ 12 files changed, 91 insertions(+), 36 deletions(-) diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp index 747a51e90f6..fb498d25112 100644 --- a/src/buffer/out/TextAttribute.cpp +++ b/src/buffer/out/TextAttribute.cpp @@ -155,12 +155,6 @@ void TextAttribute::SetColor(const COLORREF rgbColor, const bool fIsForeground) } } -// bool TextAttribute::IsBold() const noexcept -// { -// // return _isBold; -// return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Bold); -// } - bool TextAttribute::_IsReverseVideo() const noexcept { return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO); @@ -231,7 +225,6 @@ void TextAttribute::Invert() noexcept void TextAttribute::_SetBoldness(const bool isBold) noexcept { WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Bold, isBold); - // _isBold = isBold; } void TextAttribute::SetDefaultForeground() noexcept diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index ece041e0c01..3a519e099fa 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -36,7 +36,6 @@ class TextAttribute final _wAttrLegacy{ 0 }, _foreground{}, _background{}, - // _isBold{ false } _extendedAttrs{ ExtendedAttributes::Normal } { } @@ -45,7 +44,6 @@ class TextAttribute final _wAttrLegacy{ gsl::narrow_cast(wLegacyAttr & META_ATTRS) }, _foreground{ gsl::narrow_cast(wLegacyAttr & FG_ATTRS) }, _background{ gsl::narrow_cast((wLegacyAttr & BG_ATTRS) >> 4) }, - // _isBold{ false } _extendedAttrs{ ExtendedAttributes::Normal } { // If we're given lead/trailing byte information with the legacy color, strip it. @@ -175,7 +173,6 @@ class TextAttribute final WORD _wAttrLegacy; TextColor _foreground; TextColor _background; - // bool _isBold; ExtendedAttributes _extendedAttrs; #ifdef UNIT_TESTING diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 4131748b163..7dc3b939a9a 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -970,6 +970,13 @@ void DoSrvPrivateBoldText(SCREEN_INFORMATION& screenInfo, const bool bolded) buffer.SetAttributes(attrs); } +// Method Description: +// - Retrieves the active ExtendedAttributes of the given screen buffer. Text +// written to this buffer will be written with these attributes. +// Arguments: +// - screenInfo: The buffer to get the extended attrs from. +// Return Value: +// - the currently active ExtendedAttributes. ExtendedAttributes DoSrvPrivateGetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo) { auto& buffer = screenInfo.GetActiveBuffer(); @@ -977,6 +984,14 @@ ExtendedAttributes DoSrvPrivateGetExtendedTextAttributes(SCREEN_INFORMATION& scr return attrs.GetExtendedAttributes(); } +// Method Description: +// - Sets the active ExtendedAttributes of the given screen buffer. Text written +// to this buffer will be written with these attributes. +// Arguments: +// - screenInfo: The buffer to set the extended attrs for. +// - extendedAttrs: The new ExtendedAttributes to use +// Return Value: +// - void DoSrvPrivateSetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo, const ExtendedAttributes extendedAttrs) { diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index ea7312f619e..f45fc8b1deb 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -273,12 +273,26 @@ BOOL ConhostInternalGetSet::PrivateBoldText(const bool bolded) return TRUE; } +// Method Description: +// - Retrieves the currently active ExtendedAttributes. See also +// DoSrvPrivateGetExtendedTextAttributes +// Arguments: +// - pAttrs: Recieves the ExtendedAttributes value. +// Return Value: +// - TRUE if successful (see DoSrvPrivateGetExtendedTextAttributes). FALSE otherwise. BOOL ConhostInternalGetSet::PrivateGetExtendedTextAttributes(ExtendedAttributes* const pAttrs) { *pAttrs = DoSrvPrivateGetExtendedTextAttributes(_io.GetActiveOutputBuffer()); return TRUE; } +// Method Description: +// - Sets the active ExtendedAttributes of the active screen buffer. Text +// written to this buffer will be written with these attributes. +// Arguments: +// - extendedAttrs: The new ExtendedAttributes to use +// Return Value: +// - TRUE if successful (see DoSrvPrivateSetExtendedTextAttributes). FALSE otherwise. BOOL ConhostInternalGetSet::PrivateSetExtendedTextAttributes(const ExtendedAttributes attrs) { DoSrvPrivateSetExtendedTextAttributes(_io.GetActiveOutputBuffer(), attrs); diff --git a/src/inc/conattrs.hpp b/src/inc/conattrs.hpp index 90dcff6dd5d..a168b173cb9 100644 --- a/src/inc/conattrs.hpp +++ b/src/inc/conattrs.hpp @@ -16,8 +16,8 @@ enum class ExtendedAttributes : BYTE Blinking = 0x04, Invisible = 0x08, CrossedOut = 0x10, - // TODO:GH# add support for these to the parser as well. - Underlined = 0x20, // _technically_ different from LVB_UNDERSCORE, see GH# + // TODO:GH#2916 add support for these to the parser as well. + Underlined = 0x20, // _technically_ different from LVB_UNDERSCORE, see TODO:GH#2915 DoublyUnderlined = 0x40, // Included for completeness, but not currently supported. Faint = 0x80, // Included for completeness, but not currently supported. }; diff --git a/src/renderer/dx/DxRenderer.cpp b/src/renderer/dx/DxRenderer.cpp index bdbcf0b1bf2..4c11cdfc533 100644 --- a/src/renderer/dx/DxRenderer.cpp +++ b/src/renderer/dx/DxRenderer.cpp @@ -1218,7 +1218,7 @@ enum class CursorPaintType // - colorForeground - Foreground brush color // - colorBackground - Background brush color // - legacyColorAttribute - -// - isBold - +// - extendedAttrs - // - 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. diff --git a/src/renderer/gdi/state.cpp b/src/renderer/gdi/state.cpp index 85528da422d..8ab7d3b8ed3 100644 --- a/src/renderer/gdi/state.cpp +++ b/src/renderer/gdi/state.cpp @@ -176,7 +176,7 @@ GdiEngine::~GdiEngine() // - colorForeground - Foreground Color // - colorBackground - Background colo // - legacyColorAttribute - -// - isBold - +// - extendedAttrs - // - 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: diff --git a/src/renderer/vt/WinTelnetEngine.cpp b/src/renderer/vt/WinTelnetEngine.cpp index e36c0eeec84..c273232fd4a 100644 --- a/src/renderer/vt/WinTelnetEngine.cpp +++ b/src/renderer/vt/WinTelnetEngine.cpp @@ -29,6 +29,7 @@ WinTelnetEngine::WinTelnetEngine(_In_ wil::unique_hfile hPipe, // - 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. // - isSettingDefaultBrushes: indicates if we should change the background color of // the window. Unused for VT // Return Value: diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index f779e5dad6b..ec3940b756b 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -26,6 +26,7 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // - 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. // - isSettingDefaultBrushes: indicates if we should change the background color of // the window. Unused for VT // Return Value: @@ -42,8 +43,10 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // 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(legacyColorAttribute)); + // Only do extended attributes in xterm-256color, as to not break telnet.exe. RETURN_IF_FAILED(_UpdateExtendedAttrs(extendedAttrs)); return VtEngine::_RgbUpdateDrawingBrushes(colorForeground, diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 31d9be9c1bf..bc5d4a52bf9 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -156,6 +156,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // - 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. // - isSettingDefaultBrushes: indicates if we should change the background color of // the window. Unused for VT // Return Value: @@ -172,10 +173,14 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // 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(legacyColorAttribute)); // The base xterm mode only knows about 16 colors - return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, colorBackground, WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Bold), _ColorTable, _cColorTable); + return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, + colorBackground, + WI_IsFlagSet(extendedAttrs, ExtendedAttributes::Bold), + _ColorTable, + _cColorTable); } // Routine Description: diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index e7f1f5d92b1..13e41fdbbf5 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -13,7 +13,7 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes Scrollback = 3 }; - // TODO:GH# add support for DoublyUnderlined, Faint(2) to the adapter as well. + // TODO:GH#2916 add support for DoublyUnderlined, Faint(2) to the adapter as well. enum GraphicsOptions : unsigned int { Off = 0, diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index 6943a1a81d1..0e384fce444 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -49,7 +49,9 @@ void AdaptDispatch::s_DisableAllColors(_Inout_ WORD* const pAttr, const bool fIs // Arguments: // - pAttr - Pointer to font attributes field to adjust // - wApplyThis - Color values to apply to the low or high word of the font attributes field. -// - fIsForeground - TRUE = foreground color. FALSE = background color. Specifies which half of the bit field to reset and then apply wApplyThis upon. +// - fIsForeground - TRUE = foreground color. FALSE = background color. +// Specifies which half of the bit field to reset and then apply wApplyThis +// upon. // Return Value: // - void AdaptDispatch::s_ApplyColors(_Inout_ WORD* const pAttr, const WORD wApplyThis, const bool fIsForeground) @@ -75,7 +77,9 @@ void AdaptDispatch::s_ApplyColors(_Inout_ WORD* const pAttr, const WORD wApplyTh // Routine Description: // - Helper to apply the actual flags to each text attributes field. -// - Placed as a helper so it can be recursive/re-entrant for some of the convenience flag methods that perform similar/multiple operations in one command. +// - Placed as a helper so it can be recursive/re-entrant for some of the +// convenience flag methods that perform similar/multiple operations in one +// command. // Arguments: // - opt - Graphics option sent to us by the parser/requestor. // - pAttr - Pointer to the font attribute field to adjust @@ -96,6 +100,7 @@ void AdaptDispatch::_SetGraphicsOptionHelper(const DispatchTypes::GraphicsOption _fChangedMetaAttrs = true; break; case DispatchTypes::GraphicsOptions::Underline: + // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE *pAttr |= COMMON_LVB_UNDERSCORE; _fChangedMetaAttrs = true; break; @@ -275,13 +280,13 @@ void AdaptDispatch::_SetGraphicsOptionHelper(const DispatchTypes::GraphicsOption } // Routine Description: -// Returns true if the GraphicsOption represents an extended color option. -// These are followed by up to 4 more values which compose the entire option. +// Returns true if the GraphicsOption represents an extended text attribute. +// These include things such as Underlined, Italics, Blinking, etc. // Return Value: -// - true if the opt is the indicator for an extended color sequence, false otherwise. +// - true if the opt is the indicator for an extended text attribute, false otherwise. bool AdaptDispatch::s_IsExtendedTextAttribute(const DispatchTypes::GraphicsOptions opt) noexcept { - // TODO:GH# add support for DoublyUnderlined, Faint(RGBColorOrFaint). + // TODO:GH#2916 add support for DoublyUnderlined, Faint(RGBColorOrFaint). // These two are currently partially implemented as other things: // * Faint is kinda like the opposite of what bold does // * Doubly underlined should exist in a trinary state with Underlined @@ -406,8 +411,6 @@ bool AdaptDispatch::_SetBoldColorHelper(const DispatchTypes::GraphicsOptions opt bool AdaptDispatch::_SetDefaultColorHelper(const DispatchTypes::GraphicsOptions option) { - // TODO: reset extended attrs too - const bool fg = option == GraphicsOptions::Off || option == GraphicsOptions::ForegroundDefault; const bool bg = option == GraphicsOptions::Off || option == GraphicsOptions::BackgroundDefault; @@ -424,6 +427,16 @@ bool AdaptDispatch::_SetDefaultColorHelper(const DispatchTypes::GraphicsOptions return success; } +// Method Description: +// - Sets the attributes for extended text attributes. Retrieves the current +// extended attrs from the console, modifies them according to the new +// GraphicsOption, and the sets them again. +// - Notably does _not_ handle Bold, Faint, Underline, DoublyUnderlined, or +// NoUnderline. Those should be handled in TODO:GH#2916. +// Arguments: +// - opt: the graphics option to set +// Return Value: +// - True if handled successfully. False otherwise. bool AdaptDispatch::_SetExtendedTextAttributeHelper(const DispatchTypes::GraphicsOptions opt) { ExtendedAttributes attrs{ ExtendedAttributes::Normal }; @@ -456,7 +469,7 @@ bool AdaptDispatch::_SetExtendedTextAttributeHelper(const DispatchTypes::Graphic case DispatchTypes::GraphicsOptions::NotCrossedOut: WI_ClearFlag(attrs, ExtendedAttributes::CrossedOut); break; - // TODO:GH# add support for the following + // TODO:GH#2916 add support for the following // case DispatchTypes::GraphicsOptions::DoublyUnderlined: // case DispatchTypes::GraphicsOptions::RGBColorOrFaint: // case DispatchTypes::GraphicsOptions::DoublyUnderlined: @@ -466,19 +479,26 @@ bool AdaptDispatch::_SetExtendedTextAttributeHelper(const DispatchTypes::Graphic } // Routine Description: -// - SGR - Modifies the graphical rendering options applied to the next characters written into the buffer. -// - Options include colors, invert, underlines, and other "font style" type options. +// - SGR - Modifies the graphical rendering options applied to the next +// characters written into the buffer. +// - Options include colors, invert, underlines, and other "font style" +// type options. + // Arguments: -// - rgOptions - An array of options that will be applied from 0 to N, in order, one at a time by setting or removing flags in the font style properties. +// - rgOptions - An array of options that will be applied from 0 to N, in order, +// one at a time by setting or removing flags in the font style properties. // - cOptions - The count of options (a.k.a. the N in the above line of comments) // Return Value: // - True if handled successfully. False otherwise. -bool AdaptDispatch::SetGraphicsRendition(_In_reads_(cOptions) const DispatchTypes::GraphicsOptions* const rgOptions, const size_t cOptions) +bool AdaptDispatch::SetGraphicsRendition(_In_reads_(cOptions) const DispatchTypes::GraphicsOptions* const rgOptions, + const size_t cOptions) { - // We use the private function here to get just the default color attributes as a performance optimization. - // Calling the public GetConsoleScreenBufferInfoEx costs a lot of performance time/power in a tight loop - // because it has to fill the Largest Window Size by asking the OS and wastes time memcpying colors and other data - // we do not need to resolve this Set Graphics Rendition request. + // We use the private function here to get just the default color attributes + // as a performance optimization. Calling the public + // GetConsoleScreenBufferInfoEx costs a lot of performance time/power in a + // tight loop because it has to fill the Largest Window Size by asking the + // OS and wastes time memcpying colors and other data we do not need to + // resolve this Set Graphics Rendition request. WORD attr; bool fSuccess = !!_conApi->PrivateGetConsoleScreenBufferAttributes(&attr); @@ -508,14 +528,21 @@ bool AdaptDispatch::SetGraphicsRendition(_In_reads_(cOptions) const DispatchType size_t cOptionsConsumed = 0; // _SetRgbColorsHelper will call the appropriate ConApi function - fSuccess = _SetRgbColorsHelper(&(rgOptions[i]), cOptions - i, &rgbColor, &fIsForeground, &cOptionsConsumed); + fSuccess = _SetRgbColorsHelper(&(rgOptions[i]), + cOptions - i, + &rgbColor, + &fIsForeground, + &cOptionsConsumed); i += (cOptionsConsumed - 1); // cOptionsConsumed includes the opt we're currently on. } else { _SetGraphicsOptionHelper(opt, &attr); - fSuccess = !!_conApi->PrivateSetLegacyAttributes(attr, _fChangedForeground, _fChangedBackground, _fChangedMetaAttrs); + fSuccess = !!_conApi->PrivateSetLegacyAttributes(attr, + _fChangedForeground, + _fChangedBackground, + _fChangedMetaAttrs); // Make sure we un-bold if (fSuccess && opt == DispatchTypes::GraphicsOptions::Off) From 02d7d0e396c127cacad4df9a53fc4aaa3796fd5e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 1 Oct 2019 11:23:12 -0500 Subject: [PATCH 07/11] a block of PR feedback --- src/host/getset.cpp | 10 ++++--- src/host/ut_host/ScreenBufferTests.cpp | 4 +++ src/renderer/vt/Xterm256Engine.cpp | 30 +++++++++---------- src/terminal/adapter/DispatchTypes.hpp | 12 ++++---- .../adapter/adaptDispatchGraphics.cpp | 5 +++- 5 files changed, 34 insertions(+), 27 deletions(-) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 7dc3b939a9a..5463f6fbaaf 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -971,8 +971,9 @@ void DoSrvPrivateBoldText(SCREEN_INFORMATION& screenInfo, const bool bolded) } // Method Description: -// - Retrieves the active ExtendedAttributes of the given screen buffer. Text -// written to this buffer will be written with these attributes. +// - Retrieves the active ExtendedAttributes (italic, underline, etc.) of the +// given screen buffer. Text written to this buffer will be written with these +// attributes. // Arguments: // - screenInfo: The buffer to get the extended attrs from. // Return Value: @@ -985,8 +986,9 @@ ExtendedAttributes DoSrvPrivateGetExtendedTextAttributes(SCREEN_INFORMATION& scr } // Method Description: -// - Sets the active ExtendedAttributes of the given screen buffer. Text written -// to this buffer will be written with these attributes. +// - Sets the active ExtendedAttributes (italic, underline, etc.) of the given +// screen buffer. Text written to this buffer will be written with these +// attributes. // Arguments: // - screenInfo: The buffer to set the extended attrs for. // - extendedAttrs: The new ExtendedAttributes to use diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 6ce06d1b887..24bbb45a543 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -4411,6 +4411,10 @@ void ScreenBufferTests::TestExtendedTextAttributes() // This is a test for microsoft/terminal#2554. Refer to that issue for more // context. + // We're going to set every possible combination of extended attributes via + // VT, then disable them, and make sure that they are all always represented + // internally correctly. + // Run this test for each and every possible combination of states. BEGIN_TEST_METHOD_PROPERTIES() TEST_METHOD_PROPERTY(L"Data:bold", L"{false, true}") diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index ec3940b756b..9b8d165c7e5 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -68,23 +68,23 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // 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](const ExtendedAttributes attr, - bool& lastState, - std::function beginFn, - std::function endFn) -> HRESULT { + auto updateFlagAndState = [extendedAttrs, this](const ExtendedAttributes attr, + bool& lastState, + std::function beginFn, + std::function endFn) -> HRESULT { // Unfortunately, we can't use WI_IsFlagSet here, since that requires a // compile-time check that the flag (attr) is a _single_ flag, and that // won't work in this case. - const bool flagSet = (extendedAttrs & attr) == attr; + const bool flagSet = WI_AreAllFlagsSet(extendedAttrs, attr); if (flagSet != lastState) { if (flagSet) { - RETURN_IF_FAILED(beginFn()); + RETURN_IF_FAILED(beginFn(this)); } else { - RETURN_IF_FAILED(endFn()); + RETURN_IF_FAILED(endFn(this)); } lastState = flagSet; } @@ -93,26 +93,26 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, auto hr = updateFlagAndState(ExtendedAttributes::Italics, _usingItalics, - std::bind(&Xterm256Engine::_BeginItalics, this), - std::bind(&Xterm256Engine::_EndItalics, this)); + &Xterm256Engine::_BeginItalics, + &Xterm256Engine::_EndItalics); RETURN_IF_FAILED(hr); hr = updateFlagAndState(ExtendedAttributes::Blinking, _usingBlinking, - std::bind(&Xterm256Engine::_BeginBlink, this), - std::bind(&Xterm256Engine::_EndBlink, this)); + &Xterm256Engine::_BeginBlink, + &Xterm256Engine::_EndBlink); RETURN_IF_FAILED(hr); hr = updateFlagAndState(ExtendedAttributes::Invisible, _usingInvisible, - std::bind(&Xterm256Engine::_BeginInvisible, this), - std::bind(&Xterm256Engine::_EndInvisible, this)); + &Xterm256Engine::_BeginInvisible, + &Xterm256Engine::_EndInvisible); RETURN_IF_FAILED(hr); hr = updateFlagAndState(ExtendedAttributes::CrossedOut, _usingCrossedOut, - std::bind(&Xterm256Engine::_BeginCrossedOut, this), - std::bind(&Xterm256Engine::_EndCrossedOut, this)); + &Xterm256Engine::_BeginCrossedOut, + &Xterm256Engine::_EndCrossedOut); RETURN_IF_FAILED(hr); return S_OK; diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index 13e41fdbbf5..17f26c66d85 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -18,14 +18,12 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes { Off = 0, BoldBright = 1, - RGBColorOrFaint = 2, - // 2 is also Faint, decreased intensity (ISO 6429). + // The 2 and 5 entries here are for BOTH the extended graphics options, + // as well as the Faint/Blink options. + RGBColorOrFaint = 2, // 2 is also Faint, decreased intensity (ISO 6429). Italics = 3, Underline = 4, - BlinkOrXterm256Index = 5, - // 5 is also Blink (appears as Bold). - // the 2 and 5 entries here are only for the extended graphics options - // as we do not currently support those features individually + BlinkOrXterm256Index = 5, // 5 is also Blink (appears as Bold). Negative = 7, Invisible = 8, CrossedOut = 9, @@ -36,7 +34,7 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes Steady = 25, // _not_ blink Positive = 27, // _not_ inverse Visible = 28, // _not_ invisible - NotCrossedOut = 29, // _not_ invisible + NotCrossedOut = 29, ForegroundBlack = 30, ForegroundRed = 31, ForegroundGreen = 32, diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index 0e384fce444..618dc825070 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -288,7 +288,10 @@ bool AdaptDispatch::s_IsExtendedTextAttribute(const DispatchTypes::GraphicsOptio { // TODO:GH#2916 add support for DoublyUnderlined, Faint(RGBColorOrFaint). // These two are currently partially implemented as other things: - // * Faint is kinda like the opposite of what bold does + // * Faint is approximately the opposite of bold does, though it's much + // [more complicated]( + // https://github.com/microsoft/terminal/issues/2916#issuecomment-535860910) + // and less supported/used. // * Doubly underlined should exist in a trinary state with Underlined return opt == DispatchTypes::GraphicsOptions::Italics || opt == DispatchTypes::GraphicsOptions::NotItalics || From 6aca2ecd69e696b43678f47580d401c55e1a03de Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 1 Oct 2019 14:38:48 -0500 Subject: [PATCH 08/11] add 512 test cases --- src/host/ut_host/ScreenBufferTests.cpp | 199 +++++++++++++++++++++++++ 1 file changed, 199 insertions(+) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 24bbb45a543..02aa262457a 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -184,6 +184,7 @@ class ScreenBufferTests TEST_METHOD(InitializeTabStopsInVTMode); TEST_METHOD(TestExtendedTextAttributes); + TEST_METHOD(TestExtendedTextAttributesWithColors); }; void ScreenBufferTests::SingleAlternateBufferCreationTest() @@ -4537,3 +4538,201 @@ void ScreenBufferTests::TestExtendedTextAttributes() stateMachine.ProcessString(L"\x1b[0m"); } + +void ScreenBufferTests::TestExtendedTextAttributesWithColors() +{ + // This is a test for microsoft/terminal#2554. Refer to that issue for more + // context. + + // We're going to set every possible combination of extended attributes via + // VT, then set assorted colors, then disable extended attrs, then reset + // colors, in various ways, and make sure that they are all always + // represented internally correctly. + + // Run this test for each and every possible combination of states. + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:bold", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:italics", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:blink", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:invisible", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:crossedOut", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:setForegroundType", L"{0, 1, 2, 3}") + TEST_METHOD_PROPERTY(L"Data:setBackgroundType", L"{0, 1, 2, 3}") + END_TEST_METHOD_PROPERTIES() + + // colorStyle will be used to control whether we use a color from the 16 + // color table, a color from the 256 color table, or a pure RGB color. + const int UseDefault = 0; + const int Use16Color = 1; + const int Use256Color = 2; + const int UseRGBColor = 3; + + bool bold, italics, blink, invisible, crossedOut; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"bold", bold)); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"italics", italics)); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"blink", blink)); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"invisible", invisible)); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"crossedOut", crossedOut)); + + int setForegroundType, setBackgroundType; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"setForegroundType", setForegroundType), L"controls whether to use the 16 color table, 256 table, or RGB colors"); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"setBackgroundType", setBackgroundType), L"controls whether to use the 16 color table, 256 table, or RGB colors"); + + auto& g = ServiceLocator::LocateGlobals(); + auto& gci = g.getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& tbi = si.GetTextBuffer(); + auto& stateMachine = si.GetStateMachine(); + auto& cursor = tbi.GetCursor(); + + TextAttribute expectedAttr{ si.GetAttributes() }; + ExtendedAttributes expectedExtendedAttrs{ ExtendedAttributes::Normal }; + std::wstring vtSeq = L""; + + // Collect up a VT sequence to set the state given the method properties + if (bold) + { + WI_SetFlag(expectedExtendedAttrs, ExtendedAttributes::Bold); + vtSeq += L"\x1b[1m"; + } + if (italics) + { + WI_SetFlag(expectedExtendedAttrs, ExtendedAttributes::Italics); + vtSeq += L"\x1b[3m"; + } + if (blink) + { + WI_SetFlag(expectedExtendedAttrs, ExtendedAttributes::Blinking); + vtSeq += L"\x1b[5m"; + } + if (invisible) + { + WI_SetFlag(expectedExtendedAttrs, ExtendedAttributes::Invisible); + vtSeq += L"\x1b[8m"; + } + if (crossedOut) + { + WI_SetFlag(expectedExtendedAttrs, ExtendedAttributes::CrossedOut); + vtSeq += L"\x1b[9m"; + } + + // Prepare the foreground attributes + if (setForegroundType == UseDefault) + { + expectedAttr.SetDefaultForeground(); + vtSeq += L"\x1b[39m"; + } + else if (setForegroundType == Use16Color) + { + expectedAttr.SetIndexedAttributes({ static_cast(2) }, std::nullopt); + vtSeq += L"\x1b[32m"; + } + else if (setForegroundType == Use256Color) + { + expectedAttr.SetForeground(gci.GetColorTableEntry(20)); + vtSeq += L"\x1b[38;5;20m"; + } + else if (setForegroundType == UseRGBColor) + { + expectedAttr.SetForeground(RGB(1, 2, 3)); + vtSeq += L"\x1b[38;2;1;2;3m"; + } + + // Prepare the background attributes + if (setBackgroundType == UseDefault) + { + expectedAttr.SetDefaultBackground(); + vtSeq += L"\x1b[49m"; + } + else if (setBackgroundType == Use16Color) + { + expectedAttr.SetIndexedAttributes(std::nullopt, { static_cast(2) }); + vtSeq += L"\x1b[42m"; + } + else if (setBackgroundType == Use256Color) + { + expectedAttr.SetBackground(gci.GetColorTableEntry(20)); + vtSeq += L"\x1b[48;5;20m"; + } + else if (setBackgroundType == UseRGBColor) + { + expectedAttr.SetBackground(RGB(1, 2, 3)); + vtSeq += L"\x1b[48;2;1;2;3m"; + } + + expectedAttr.SetExtendedAttributes(expectedExtendedAttrs); + + // Helper lambda to write a VT sequence, then an "X", then check that the + // attributes of the "X" match what we think they should be. + auto validate = [&](const TextAttribute attr, + const std::wstring& vtSequence) { + auto cursorPos = cursor.GetPosition(); + + // Convert the vtSequence to something printable. Lets not set these + // attrs on the test console + std::wstring debugString = vtSequence; + { + size_t start_pos = 0; + while ((start_pos = debugString.find(L"\x1b", start_pos)) != std::string::npos) + { + debugString.replace(start_pos, 1, L"\\x1b"); + start_pos += 4; + } + } + + Log::Comment(NoThrowString().Format( + L"Testing string:\"%s\"", debugString.c_str())); + Log::Comment(NoThrowString().Format( + L"Expecting attrs:0x%02x", VerifyOutputTraits::ToString(attr).GetBuffer())); + + stateMachine.ProcessString(vtSequence); + stateMachine.ProcessString(L"X"); + + auto iter = tbi.GetCellDataAt(cursorPos); + const TextAttribute currentAttrs = iter->TextAttr(); + VERIFY_ARE_EQUAL(attr, currentAttrs); + }; + + // Check setting all the states collected above + validate(expectedAttr, vtSeq); + + // One-by-one, turn off each of these states with VT, then check that the + // state matched. + if (bold) + { + WI_ClearFlag(expectedExtendedAttrs, ExtendedAttributes::Bold); + expectedAttr.SetExtendedAttributes(expectedExtendedAttrs); + vtSeq = L"\x1b[22m"; + validate(expectedAttr, vtSeq); + } + if (italics) + { + WI_ClearFlag(expectedExtendedAttrs, ExtendedAttributes::Italics); + expectedAttr.SetExtendedAttributes(expectedExtendedAttrs); + vtSeq = L"\x1b[23m"; + validate(expectedAttr, vtSeq); + } + if (blink) + { + WI_ClearFlag(expectedExtendedAttrs, ExtendedAttributes::Blinking); + expectedAttr.SetExtendedAttributes(expectedExtendedAttrs); + vtSeq = L"\x1b[25m"; + validate(expectedAttr, vtSeq); + } + if (invisible) + { + WI_ClearFlag(expectedExtendedAttrs, ExtendedAttributes::Invisible); + expectedAttr.SetExtendedAttributes(expectedExtendedAttrs); + vtSeq = L"\x1b[28m"; + validate(expectedAttr, vtSeq); + } + if (crossedOut) + { + WI_ClearFlag(expectedExtendedAttrs, ExtendedAttributes::CrossedOut); + expectedAttr.SetExtendedAttributes(expectedExtendedAttrs); + vtSeq = L"\x1b[29m"; + validate(expectedAttr, vtSeq); + } + + stateMachine.ProcessString(L"\x1b[0m"); +} From efe6b3caff651edcaac61e8d58d16eb3308d4920 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 1 Oct 2019 15:49:04 -0500 Subject: [PATCH 09/11] Fix the build --- src/terminal/adapter/ut_adapter/adapterTest.cpp | 12 ++++++++++++ tools/bcz.cmd | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index b8a7fce0475..09258aca34b 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -339,6 +339,18 @@ class TestGetSet final : public ConGetSet return !!_fPrivateBoldTextResult; } + BOOL PrivateGetExtendedTextAttributes(ExtendedAttributes* const /*pAttrs*/) + { + Log::Comment(L"PrivateGetExtendedTextAttributes MOCK called..."); + return true; + } + + BOOL PrivateSetExtendedTextAttributes(const ExtendedAttributes /*attrs*/) + { + Log::Comment(L"PrivateSetExtendedTextAttributes MOCK called..."); + return true; + } + BOOL PrivateWriteConsoleInputW(_Inout_ std::deque>& events, _Out_ size_t& eventsWritten) override { diff --git a/tools/bcz.cmd b/tools/bcz.cmd index 4167b069cb9..98971b12d30 100644 --- a/tools/bcz.cmd +++ b/tools/bcz.cmd @@ -68,7 +68,7 @@ if "%_EXCLUSIVE%" == "1" ( echo Performing nuget restore... nuget.exe restore %OPENCON%\OpenConsole.sln -set _BUILD_CMDLINE="%MSBUILD%" %OPENCON%\OpenConsole.sln /t:%_MSBUILD_TARGET% /m /p:Configuration=%_LAST_BUILD_CONF% /p:Platform=%ARCH% %_APPX_ARGS% +set _BUILD_CMDLINE="%MSBUILD%" %OPENCON%\OpenConsole.sln /t:"%_MSBUILD_TARGET%" /m /p:Configuration=%_LAST_BUILD_CONF% /p:Platform=%ARCH% %_APPX_ARGS% echo %_BUILD_CMDLINE% echo Starting build... From 9cf69964552e2570631433fbb244d5a9f873c9f1 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 3 Oct 2019 14:55:21 -0500 Subject: [PATCH 10/11] Fix @carlos-zamora's suggestions --- src/buffer/out/TextAttribute.hpp | 1 - src/renderer/vt/Xterm256Engine.cpp | 9 +++++---- src/renderer/vt/Xterm256Engine.hpp | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index 3a519e099fa..1e05d653376 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -55,7 +55,6 @@ class TextAttribute final _wAttrLegacy{ 0 }, _foreground{ rgbForeground }, _background{ rgbBackground }, - // _isBold{ false } _extendedAttrs{ ExtendedAttributes::Normal } { } diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 9b8d165c7e5..136d00b034c 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -14,7 +14,11 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, const Viewport initialViewport, _In_reads_(cColorTable) const COLORREF* const ColorTable, const WORD cColorTable) : - XtermEngine(std::move(hPipe), shutdownEvent, colorProvider, initialViewport, ColorTable, cColorTable, false) + XtermEngine(std::move(hPipe), shutdownEvent, colorProvider, initialViewport, ColorTable, cColorTable, false), + _usingItalics{ false }, + _usingBlinking{ false }, + _usingInvisible{ false }, + _usingCrossedOut{ false } { } @@ -72,9 +76,6 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, bool& lastState, std::function beginFn, std::function endFn) -> HRESULT { - // Unfortunately, we can't use WI_IsFlagSet here, since that requires a - // compile-time check that the flag (attr) is a _single_ flag, and that - // won't work in this case. const bool flagSet = WI_AreAllFlagsSet(extendedAttrs, attr); if (flagSet != lastState) { diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 6a0a9f9bf14..8f733a2b0fd 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -41,10 +41,10 @@ namespace Microsoft::Console::Render private: [[nodiscard]] HRESULT _UpdateExtendedAttrs(const ExtendedAttributes extendedAttrs) noexcept; - bool _usingItalics{ false }; - bool _usingBlinking{ false }; - bool _usingInvisible{ false }; - bool _usingCrossedOut{ false }; + bool _usingItalics; + bool _usingBlinking; + bool _usingInvisible; + bool _usingCrossedOut; #ifdef UNIT_TESTING friend class VtRendererTest; From 4d41c4e1315a338627f15ccd7e948e6b4638fc3f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 4 Oct 2019 09:37:19 -0500 Subject: [PATCH 11/11] @miniksa's PR feedback --- src/buffer/out/TextAttribute.hpp | 2 +- src/renderer/vt/Xterm256Engine.cpp | 13 +++--------- src/renderer/vt/Xterm256Engine.hpp | 7 +++---- src/terminal/adapter/adaptDispatch.cpp | 20 ++++--------------- .../adapter/adaptDispatchGraphics.cpp | 16 ++------------- src/types/inc/utils.hpp | 13 ++++++++++++ 6 files changed, 26 insertions(+), 45 deletions(-) diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index 1e05d653376..213c908e8f2 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -185,7 +185,7 @@ class TextAttribute final #pragma pack(pop) // 2 for _wAttrLegacy // 4 for _foreground -// 4 for _foreground +// 4 for _background // 1 for _extendedAttrs static_assert(sizeof(TextAttribute) <= 11 * sizeof(BYTE), "We should only need 11B for an entire TextColor. Any more than that is just waste"); diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 136d00b034c..af8738a5650 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -15,10 +15,7 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, _In_reads_(cColorTable) const COLORREF* const ColorTable, const WORD cColorTable) : XtermEngine(std::move(hPipe), shutdownEvent, colorProvider, initialViewport, ColorTable, cColorTable, false), - _usingItalics{ false }, - _usingBlinking{ false }, - _usingInvisible{ false }, - _usingCrossedOut{ false } + _lastExtendedAttrsState{ ExtendedAttributes::Normal } { } @@ -73,10 +70,10 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, // value (lastState), and appropriately start/end that state with the given // begin/end functions. auto updateFlagAndState = [extendedAttrs, this](const ExtendedAttributes attr, - bool& lastState, std::function beginFn, std::function endFn) -> HRESULT { const bool flagSet = WI_AreAllFlagsSet(extendedAttrs, attr); + const bool lastState = WI_AreAllFlagsSet(_lastExtendedAttrsState, attr); if (flagSet != lastState) { if (flagSet) @@ -87,31 +84,27 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, { RETURN_IF_FAILED(endFn(this)); } - lastState = flagSet; + WI_SetAllFlags(_lastExtendedAttrsState, attr); } return S_OK; }; auto hr = updateFlagAndState(ExtendedAttributes::Italics, - _usingItalics, &Xterm256Engine::_BeginItalics, &Xterm256Engine::_EndItalics); RETURN_IF_FAILED(hr); hr = updateFlagAndState(ExtendedAttributes::Blinking, - _usingBlinking, &Xterm256Engine::_BeginBlink, &Xterm256Engine::_EndBlink); RETURN_IF_FAILED(hr); hr = updateFlagAndState(ExtendedAttributes::Invisible, - _usingInvisible, &Xterm256Engine::_BeginInvisible, &Xterm256Engine::_EndInvisible); RETURN_IF_FAILED(hr); hr = updateFlagAndState(ExtendedAttributes::CrossedOut, - _usingCrossedOut, &Xterm256Engine::_BeginCrossedOut, &Xterm256Engine::_EndCrossedOut); RETURN_IF_FAILED(hr); diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 8f733a2b0fd..729ed12434c 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -41,10 +41,9 @@ namespace Microsoft::Console::Render private: [[nodiscard]] HRESULT _UpdateExtendedAttrs(const ExtendedAttributes extendedAttrs) noexcept; - bool _usingItalics; - bool _usingBlinking; - bool _usingInvisible; - bool _usingCrossedOut; + // We're only using Italics, Blinking, Invisible and Crossed Out for now + // See GH#2916 for adding a more complete implementation. + ExtendedAttributes _lastExtendedAttrsState; #ifdef UNIT_TESTING friend class VtRendererTest; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 7123e695087..d539eb8c551 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -6,19 +6,7 @@ #include "adaptDispatch.hpp" #include "conGetSet.hpp" #include "../../types/inc/Viewport.hpp" - -// Inspired from RETURN_IF_WIN32_BOOL_FALSE -// WIL doesn't include a RETURN_IF_FALSE, and RETURN_IF_WIN32_BOOL_FALSE -// will actually return the value of GLE. -#define RETURN_IF_FALSE(b) \ - do \ - { \ - BOOL __boolRet = wil::verify_bool(b); \ - if (!__boolRet) \ - { \ - return b; \ - } \ - } while (0, 0) +#include "../../types/inc/utils.hpp" using namespace Microsoft::Console::Types; using namespace Microsoft::Console::VirtualTerminal; @@ -504,15 +492,15 @@ bool AdaptDispatch::_InsertDeleteHelper(_In_ unsigned int const uiCount, const b { // We'll be doing short math on the distance since all console APIs use shorts. So check that we can successfully convert the uint into a short first. SHORT sDistance; - RETURN_IF_FALSE(SUCCEEDED(UIntToShort(uiCount, &sDistance))); + RETURN_BOOL_IF_FALSE(SUCCEEDED(UIntToShort(uiCount, &sDistance))); // get current cursor, attributes CONSOLE_SCREEN_BUFFER_INFOEX csbiex = { 0 }; csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX); // Make sure to reset the viewport (with MoveToBottom )to where it was // before the user scrolled the console output - RETURN_IF_FALSE(_conApi->MoveToBottom()); - RETURN_IF_FALSE(_conApi->GetConsoleScreenBufferInfoEx(&csbiex)); + RETURN_BOOL_IF_FALSE(_conApi->MoveToBottom()); + RETURN_BOOL_IF_FALSE(_conApi->GetConsoleScreenBufferInfoEx(&csbiex)); const auto cursor = csbiex.dwCursorPosition; // Rectangle to cut out of the existing buffer. This is inclusive. diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index 618dc825070..85076d08238 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -5,6 +5,7 @@ #include "adaptDispatch.hpp" #include "conGetSet.hpp" +#include "../../types/inc/utils.hpp" #define ENABLE_INTSAFE_SIGNED_FUNCTIONS #include @@ -12,19 +13,6 @@ using namespace Microsoft::Console::VirtualTerminal; using namespace Microsoft::Console::VirtualTerminal::DispatchTypes; -// Inspired from RETURN_IF_WIN32_BOOL_FALSE -// WIL doesn't include a RETURN_IF_FALSE, and RETURN_IF_WIN32_BOOL_FALSE -// will actually return the value of GLE. -#define RETURN_IF_FALSE(b) \ - do \ - { \ - BOOL __boolRet = wil::verify_bool(b); \ - if (!__boolRet) \ - { \ - return b; \ - } \ - } while (0, 0) - // Routine Description: // - Small helper to disable all color flags within a given font attributes field // Arguments: @@ -444,7 +432,7 @@ bool AdaptDispatch::_SetExtendedTextAttributeHelper(const DispatchTypes::Graphic { ExtendedAttributes attrs{ ExtendedAttributes::Normal }; - RETURN_IF_FALSE(_conApi->PrivateGetExtendedTextAttributes(&attrs)); + RETURN_BOOL_IF_FALSE(_conApi->PrivateGetExtendedTextAttributes(&attrs)); switch (opt) { diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp index ff1c8dd1781..6dfa216ab4d 100644 --- a/src/types/inc/utils.hpp +++ b/src/types/inc/utils.hpp @@ -11,6 +11,19 @@ Author(s): - Mike Griese (migrie) 12-Jun-2018 --*/ +// Inspired from RETURN_IF_WIN32_BOOL_FALSE +// WIL doesn't include a RETURN_BOOL_IF_FALSE, and RETURN_IF_WIN32_BOOL_FALSE +// will actually return the value of GLE. +#define RETURN_BOOL_IF_FALSE(b) \ + do \ + { \ + BOOL __boolRet = wil::verify_bool(b); \ + if (!__boolRet) \ + { \ + return __boolRet; \ + } \ + } while (0, 0) + namespace Microsoft::Console::Utils { bool IsValidHandle(const HANDLE handle) noexcept;