From 158a1708a62504d59d2f5000489f7ceddb90b8f2 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Mon, 3 Aug 2020 13:49:25 +0100 Subject: [PATCH] Render the SGR "underlined" attribute in the style of the font (#7148) This PR updates the rendering of the _underlined_ graphic rendition attribute, using the style specified in the active font, instead of just reusing the grid line at the bottom of the character cell. * Support for drawing the correct underline effect in the grid line renderer was added in #7107. There was already an `ExtendedAttributes` flag defined for the underlined state, but I needed to update the `SetUnderlined` and `IsUnderlined` methods in the `TextAttribute` class to use that flag now in place of the legacy `LVB_UNDERSCORE` attribute. This enables underlines set via a VT sequence to be tracked separately from `LVB_UNDERSCORE` grid lines set via the console API. I then needed to update the `Renderer::s_GetGridlines` method to activate the `GridLines::Underline` style when the `Underlined` attribute was set. The `GridLines::Bottom` style is still triggered by the `LVB_UNDERSCORE` attribute to produce the bottom grid line effect. Validation ---------- Because this is a change from the existing behaviour, certain unit tests that were expecting the `LVB_UNDERSCORE` to be toggled by `SGR 4` and `SGR 24` have now had to be updated to check the `Underlined` flag instead. There were also some UI Automation tests that were checking for `SGR 4` mapping to `LVB_UNDERSCORE` attribute, which I've now substituted with a test of the `SGR 53` overline attribute mapping to `LVB_GRID_HORIZONTAL`. These tests only work with legacy attributes, so they can't access the extended underline state, and I thought a replacement test that covered similar ground would be better than dropping the tests altogether. As far as the visual rendering is concerned, I've manually confirmed that the VT underline sequences now draw the underline in the correct position and style, while grid lines output via the console API are still displayed in their original form. Closes #2915 --- src/buffer/out/TextAttribute.cpp | 6 ++---- src/host/ft_uia/VirtualTerminalTests.cs | 16 ++++++++-------- src/host/ut_host/TextBufferTests.cpp | 4 ++-- src/inc/conattrs.hpp | 2 +- src/renderer/base/renderer.cpp | 5 +++++ src/terminal/adapter/ut_adapter/adapterTest.cpp | 6 ++++-- src/tools/vtapp/Program.cs | 6 +++--- 7 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp index 371fedb2b..ad7ee0546 100644 --- a/src/buffer/out/TextAttribute.cpp +++ b/src/buffer/out/TextAttribute.cpp @@ -246,8 +246,7 @@ bool TextAttribute::IsCrossedOut() const noexcept bool TextAttribute::IsUnderlined() const noexcept { - // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE - return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_UNDERSCORE); + return WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Underlined); } bool TextAttribute::IsOverlined() const noexcept @@ -292,8 +291,7 @@ void TextAttribute::SetCrossedOut(bool isCrossedOut) noexcept void TextAttribute::SetUnderlined(bool isUnderlined) noexcept { - // TODO:GH#2915 Treat underline separately from LVB_UNDERSCORE - WI_UpdateFlag(_wAttrLegacy, COMMON_LVB_UNDERSCORE, isUnderlined); + WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Underlined, isUnderlined); } void TextAttribute::SetOverlined(bool isOverlined) noexcept diff --git a/src/host/ft_uia/VirtualTerminalTests.cs b/src/host/ft_uia/VirtualTerminalTests.cs index 65fdf71b4..6210e2973 100644 --- a/src/host/ft_uia/VirtualTerminalTests.cs +++ b/src/host/ft_uia/VirtualTerminalTests.cs @@ -381,23 +381,23 @@ namespace Conhost.UIA.Tests ciActual = area.GetCharInfoAt(hConsole, pt); Verify.AreEqual(ciExpected, ciActual, "Verify that background bright cyan got set."); - Log.Comment("Set underline (SGR.4)"); + Log.Comment("Set overline (SGR.53)"); app.FillCursorPosition(hConsole, ref pt); app.UIRoot.SendKeys("e`"); - ciExpected.Attributes |= WinCon.CONSOLE_ATTRIBUTES.COMMON_LVB_UNDERSCORE; + ciExpected.Attributes |= WinCon.CONSOLE_ATTRIBUTES.COMMON_LVB_GRID_HORIZONTAL; ciActual = area.GetCharInfoAt(hConsole, pt); - Verify.AreEqual(ciExpected, ciActual, "Verify that underline got set."); + Verify.AreEqual(ciExpected, ciActual, "Verify that overline got set."); - Log.Comment("Clear underline (SGR.24)"); + Log.Comment("Clear overline (SGR.55)"); app.FillCursorPosition(hConsole, ref pt); app.UIRoot.SendKeys("d`"); - ciExpected.Attributes &= ~WinCon.CONSOLE_ATTRIBUTES.COMMON_LVB_UNDERSCORE; + ciExpected.Attributes &= ~WinCon.CONSOLE_ATTRIBUTES.COMMON_LVB_GRID_HORIZONTAL; ciActual = area.GetCharInfoAt(hConsole, pt); - Verify.AreEqual(ciExpected, ciActual, "Verify that underline got cleared."); + Verify.AreEqual(ciExpected, ciActual, "Verify that overline got cleared."); Log.Comment("Set negative image video (SGR.7)"); app.FillCursorPosition(hConsole, ref pt); @@ -426,14 +426,14 @@ namespace Conhost.UIA.Tests ciActual = area.GetCharInfoAt(hConsole, pt); Verify.AreEqual(ciExpected, ciActual, "Verify that we got set back to the original state."); - Log.Comment("Set multiple properties in the same message (SGR.1,37,43,4)"); + Log.Comment("Set multiple properties in the same message (SGR.1,37,43,53)"); app.FillCursorPosition(hConsole, ref pt); app.UIRoot.SendKeys("9`"); ciExpected.Attributes |= WinCon.CONSOLE_ATTRIBUTES.FOREGROUND_COLORS; ciExpected.Attributes |= WinCon.CONSOLE_ATTRIBUTES.FOREGROUND_INTENSITY; ciExpected.Attributes |= WinCon.CONSOLE_ATTRIBUTES.BACKGROUND_YELLOW; - ciExpected.Attributes |= WinCon.CONSOLE_ATTRIBUTES.COMMON_LVB_UNDERSCORE; + ciExpected.Attributes |= WinCon.CONSOLE_ATTRIBUTES.COMMON_LVB_GRID_HORIZONTAL; ciActual = area.GetCharInfoAt(hConsole, pt); Verify.AreEqual(ciExpected, ciActual, "Verify that we set foreground bright white, background yellow, and underscore in the same SGR command."); diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index 6755cc7b7..6f4351b6c 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -757,8 +757,8 @@ void TextBufferTests::TestMixedRgbAndLegacyUnderline() VERIFY_ARE_EQUAL(gci.LookupAttributeColors(attrA), std::make_pair(fgColor, bgColor)); VERIFY_ARE_EQUAL(gci.LookupAttributeColors(attrB), std::make_pair(fgColor, bgColor)); - VERIFY_ARE_EQUAL(attrA.GetLegacyAttributes() & COMMON_LVB_UNDERSCORE, 0); - VERIFY_ARE_EQUAL(attrB.GetLegacyAttributes() & COMMON_LVB_UNDERSCORE, COMMON_LVB_UNDERSCORE); + VERIFY_ARE_EQUAL(attrA.IsUnderlined(), false); + VERIFY_ARE_EQUAL(attrB.IsUnderlined(), true); wchar_t* reset = L"\x1b[0m"; stateMachine.ProcessString(reset); diff --git a/src/inc/conattrs.hpp b/src/inc/conattrs.hpp index 1ad1fa903..06d2d9d3e 100644 --- a/src/inc/conattrs.hpp +++ b/src/inc/conattrs.hpp @@ -17,7 +17,7 @@ enum class ExtendedAttributes : BYTE Invisible = 0x08, CrossedOut = 0x10, // TODO:GH#2916 add support for these to the parser as well. - Underlined = 0x20, // _technically_ different from LVB_UNDERSCORE, see TODO:GH#2915 + Underlined = 0x20, DoublyUnderlined = 0x40, // Included for completeness, but not currently supported. Faint = 0x80, }; diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index c4e9d71d9..2bbbf220b 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -865,6 +865,11 @@ IRenderEngine::GridLines Renderer::s_GetGridlines(const TextAttribute& textAttri { lines |= IRenderEngine::GridLines::Strikethrough; } + + if (textAttribute.IsUnderlined()) + { + lines |= IRenderEngine::GridLines::Underline; + } return lines; } diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 6ecdfb7f0..e3c8746f6 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -1298,7 +1298,8 @@ public: case DispatchTypes::GraphicsOptions::Underline: Log::Comment(L"Testing graphics 'Underline'"); _testGetSet->_attribute = TextAttribute{ 0 }; - _testGetSet->_expectedAttribute = TextAttribute{ COMMON_LVB_UNDERSCORE }; + _testGetSet->_expectedAttribute = TextAttribute{ 0 }; + _testGetSet->_expectedAttribute.SetUnderlined(true); break; case DispatchTypes::GraphicsOptions::Overline: Log::Comment(L"Testing graphics 'Overline'"); @@ -1331,7 +1332,8 @@ public: break; case DispatchTypes::GraphicsOptions::NoUnderline: Log::Comment(L"Testing graphics 'No Underline'"); - _testGetSet->_attribute = TextAttribute{ COMMON_LVB_UNDERSCORE }; + _testGetSet->_attribute = TextAttribute{ 0 }; + _testGetSet->_attribute.SetUnderlined(true); _testGetSet->_expectedAttribute = TextAttribute{ 0 }; break; case DispatchTypes::GraphicsOptions::NoOverline: diff --git a/src/tools/vtapp/Program.cs b/src/tools/vtapp/Program.cs index 20cff98ab..6b2017c00 100644 --- a/src/tools/vtapp/Program.cs +++ b/src/tools/vtapp/Program.cs @@ -236,11 +236,11 @@ namespace VTApp break; case 'e': Console.Write(CSI); - Console.Write("4m"); + Console.Write("53m"); break; case 'd': Console.Write(CSI); - Console.Write("24m"); + Console.Write("55m"); break; case 'r': Console.Write(CSI); @@ -260,7 +260,7 @@ namespace VTApp break; case '9': Console.Write(CSI); - Console.Write("1;37;43;4m"); + Console.Write("1;37;43;53m"); break; case '(': Console.Write(CSI);