From f07c9a65fe6eb042a4dd1adec5a8d7765f438f61 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 27 Sep 2019 11:21:37 -0500 Subject: [PATCH 01/11] Fix snapping to the cursor in "Terminal Scrolling" mode (#2705) fixes #1222 PSReadline calls SetConsoleCursorPosition on each character they emit (go figure). When that function is called, and we set the cursor position, we'll try and "snap" the viewport to the location of the cursor, so that the cursor remains visible. However, we'd only ever do this with the visible viewport, the viewport defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in Terminal Scrolling mode, we actually need to snap the virtual viewport, so that this behavior looks more regular. (cherry picked from commit 6f4b98acb4e01190e21225531bde736099c66911) --- src/host/getset.cpp | 29 +++++++--- src/host/ut_host/ScreenBufferTests.cpp | 77 ++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 9 deletions(-) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 16aa6633a71..3bdc2daa049 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -649,31 +649,42 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont LOG_IF_FAILED(ConsoleImeResizeCompStrView()); - COORD WindowOrigin; - WindowOrigin.X = 0; - WindowOrigin.Y = 0; + // Attempt to "snap" the viewport to the cursor position. If the cursor + // is not in the current viewport, we'll try and move the viewport so + // that the cursor is visible. + // microsoft/terminal#1222 - Use the "virtual" viewport here, so that + // when the console is in terminal-scrolling mode, the viewport snaps + // back to the virtual viewport's location. + const SMALL_RECT currentViewport = gci.IsTerminalScrolling() ? + buffer.GetVirtualViewport().ToInclusive() : + buffer.GetViewport().ToInclusive(); + COORD delta{ 0 }; { - const SMALL_RECT currentViewport = buffer.GetViewport().ToInclusive(); if (currentViewport.Left > position.X) { - WindowOrigin.X = position.X - currentViewport.Left; + delta.X = position.X - currentViewport.Left; } else if (currentViewport.Right < position.X) { - WindowOrigin.X = position.X - currentViewport.Right; + delta.X = position.X - currentViewport.Right; } if (currentViewport.Top > position.Y) { - WindowOrigin.Y = position.Y - currentViewport.Top; + delta.Y = position.Y - currentViewport.Top; } else if (currentViewport.Bottom < position.Y) { - WindowOrigin.Y = position.Y - currentViewport.Bottom; + delta.Y = position.Y - currentViewport.Bottom; } } - RETURN_IF_NTSTATUS_FAILED(buffer.SetViewportOrigin(false, WindowOrigin, true)); + COORD newWindowOrigin{ 0 }; + newWindowOrigin.X = currentViewport.Left + delta.X; + newWindowOrigin.Y = currentViewport.Top + delta.Y; + // SetViewportOrigin will worry about clamping these values to the + // buffer for us. + RETURN_IF_NTSTATUS_FAILED(buffer.SetViewportOrigin(true, newWindowOrigin, true)); return S_OK; } diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index aa9195ce322..495933383cc 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -179,6 +179,8 @@ class ScreenBufferTests TEST_METHOD(RestoreDownAltBufferWithTerminalScrolling); + TEST_METHOD(SnapCursorWithTerminalScrolling); + TEST_METHOD(ClearAlternateBuffer); TEST_METHOD(InitializeTabStopsInVTMode); @@ -4288,6 +4290,81 @@ void ScreenBufferTests::RestoreDownAltBufferWithTerminalScrolling() } } +void ScreenBufferTests::SnapCursorWithTerminalScrolling() +{ + // This is a test for microsoft/terminal#1222. Refer to that issue for more + // context + + auto& g = ServiceLocator::LocateGlobals(); + CONSOLE_INFORMATION& gci = g.getConsoleInformation(); + gci.SetTerminalScrolling(true); + gci.LockConsole(); // Lock must be taken to manipulate buffer. + auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); + + auto& si = gci.GetActiveOutputBuffer(); + auto& cursor = si.GetTextBuffer().GetCursor(); + const auto originalView = si._viewport; + si._virtualBottom = originalView.BottomInclusive(); + + Log::Comment(NoThrowString().Format( + L"cursor=%s", VerifyOutputTraits::ToString(cursor.GetPosition()).GetBuffer())); + Log::Comment(NoThrowString().Format( + L"originalView=%s", VerifyOutputTraits::ToString(originalView.ToInclusive()).GetBuffer())); + + Log::Comment(NoThrowString().Format( + L"First set the viewport somewhere lower in the buffer, as if the text " + L"was output there. Manually move the cursor there as well, so the " + L"cursor is within that viewport.")); + const COORD secondWindowOrigin{ 0, 10 }; + VERIFY_SUCCEEDED(si.SetViewportOrigin(true, secondWindowOrigin, true)); + si.GetTextBuffer().GetCursor().SetPosition(secondWindowOrigin); + + const auto secondView = si._viewport; + const auto secondVirtualBottom = si._virtualBottom; + Log::Comment(NoThrowString().Format( + L"cursor=%s", VerifyOutputTraits::ToString(cursor.GetPosition()).GetBuffer())); + Log::Comment(NoThrowString().Format( + L"secondView=%s", VerifyOutputTraits::ToString(secondView.ToInclusive()).GetBuffer())); + + VERIFY_ARE_EQUAL(10, secondView.Top()); + VERIFY_ARE_EQUAL(originalView.Height() + 10, secondView.BottomExclusive()); + VERIFY_ARE_EQUAL(originalView.Height() + 10 - 1, secondVirtualBottom); + + Log::Comment(NoThrowString().Format( + L"Emulate scrolling upwards with the mouse (not moving the virtual view)")); + + const COORD thirdWindowOrigin{ 0, 2 }; + VERIFY_SUCCEEDED(si.SetViewportOrigin(true, thirdWindowOrigin, false)); + + const auto thirdView = si._viewport; + const auto thirdVirtualBottom = si._virtualBottom; + + Log::Comment(NoThrowString().Format( + L"cursor=%s", VerifyOutputTraits::ToString(cursor.GetPosition()).GetBuffer())); + Log::Comment(NoThrowString().Format( + L"thirdView=%s", VerifyOutputTraits::ToString(thirdView.ToInclusive()).GetBuffer())); + + VERIFY_ARE_EQUAL(2, thirdView.Top()); + VERIFY_ARE_EQUAL(originalView.Height() + 2, thirdView.BottomExclusive()); + VERIFY_ARE_EQUAL(secondVirtualBottom, thirdVirtualBottom); + + Log::Comment(NoThrowString().Format( + L"Call SetConsoleCursorPosition to snap to the cursor")); + VERIFY_SUCCEEDED(g.api.SetConsoleCursorPositionImpl(si, secondWindowOrigin)); + + const auto fourthView = si._viewport; + const auto fourthVirtualBottom = si._virtualBottom; + + Log::Comment(NoThrowString().Format( + L"cursor=%s", VerifyOutputTraits::ToString(cursor.GetPosition()).GetBuffer())); + Log::Comment(NoThrowString().Format( + L"thirdView=%s", VerifyOutputTraits::ToString(fourthView.ToInclusive()).GetBuffer())); + + VERIFY_ARE_EQUAL(10, fourthView.Top()); + VERIFY_ARE_EQUAL(originalView.Height() + 10, fourthView.BottomExclusive()); + VERIFY_ARE_EQUAL(secondVirtualBottom, fourthVirtualBottom); +} + void ScreenBufferTests::ClearAlternateBuffer() { // This is a test for microsoft/terminal#1189. Refer to that issue for more From bb6ae443a591a0f687150aa94b0788f9e0994a3d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 30 Sep 2019 17:38:52 -0500 Subject: [PATCH 02/11] Passthrough BEL in conpty (#2990) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🔔 [insert Chorus of the Bells here -DHowett] Fixes #2952. (cherry picked from commit 683112075501c8b733e0a869d86756cd1d5e9091) --- src/terminal/parser/OutputStateMachineEngine.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index a17699a5247..be3e1b0d604 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -44,6 +44,17 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch) { _dispatch->Execute(wch); _ClearLastChar(); + + if (wch == AsciiChars::BEL) + { + // microsoft/terminal#2952 + // If we're attached to a terminal, let's also pass the BEL through. + if (_pfnFlushToTerminal != nullptr) + { + _pfnFlushToTerminal(); + } + } + return true; } From b512fcf429dce7afe56b48ffa72726e73cbb3862 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 30 Sep 2019 18:16:31 -0700 Subject: [PATCH 03/11] make filling chars (and, thus, erase line/char) unset wrap (#2831) EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to the end of the line. Originally, we had a boolean `setWrap` that would mean... - **true**: if writing to the end of the row, SET the wrap value to true - **false**: if writing to the end of the row, DON'T CHANGE the wrap value Now we're making this bool a std::optional to allow for a ternary state. This allows for us to handle the following cases completely. Refer to the table below: ,- current wrap value | ,- are we filling the last cell in the row? | | ,- new wrap value | | | ,- comments |-- |-- |-- | | 0 | 0 | 0 | | 0 | 1 | 0 | | 0 | 1 | 1 | THIS CASE WAS HANDLED CORRECTLY | 1 | 0 | 0 | THIS CASE WAS UNHANDLED | 1 | 0 | 1 | | 1 | 1 | 1 | To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have ~setWrap~ `wrap` mean the following: - **true**: if writing to the end of the row, SET the wrap value to TRUE - **false**: if writing to the end of the row, SET the wrap value to FALSE - **nullopt**: leave the wrap value as it is Closes #1126 (cherry picked from commit 4dd9f9c180e00a80b48ebfa8105f99ef2f60c728) --- src/buffer/out/Row.cpp | 15 ++- src/buffer/out/Row.hpp | 2 +- src/buffer/out/textBuffer.cpp | 13 ++- src/buffer/out/textBuffer.hpp | 5 +- src/host/_output.cpp | 5 +- src/host/ft_host/API_FillOutputTests.cpp | 121 +++++++++++++++++++++++ src/host/screenInfo.cpp | 7 +- src/host/screenInfo.hpp | 3 +- src/host/ut_host/TextBufferTests.cpp | 74 ++++++++++++++ 9 files changed, 228 insertions(+), 17 deletions(-) diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp index 4172374af6d..6c82b718fb9 100644 --- a/src/buffer/out/Row.cpp +++ b/src/buffer/out/Row.cpp @@ -147,11 +147,11 @@ const UnicodeStorage& ROW::GetUnicodeStorage() const noexcept // Arguments: // - it - custom console iterator to use for seeking input data. bool() false when it becomes invalid while seeking. // - index - column in row to start writing at -// - setWrap - set the wrap flags if we hit the end of the row while writing and there's still more data in the iterator. +// - wrap - change the wrap flag if we hit the end of the row while writing and there's still more data in the iterator. // - limitRight - right inclusive column ID for the last write in this row. (optional, will just write to the end of row if nullopt) // Return Value: // - iterator to first cell that was not written to this row. -OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, const bool setWrap, std::optional limitRight) +OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, const std::optional wrap, std::optional limitRight) { THROW_HR_IF(E_INVALIDARG, index >= _charRow.size()); THROW_HR_IF(E_INVALIDARG, limitRight.value_or(0) >= _charRow.size()); @@ -202,10 +202,15 @@ OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, co ++it; } - // If we're asked to set the wrap status and we just filled the last column with some text, set wrap status on the row. - if (setWrap && fillingLastColumn) + // If we're asked to (un)set the wrap status and we just filled the last column with some text... + // NOTE: + // - wrap = std::nullopt --> don't change the wrap value + // - wrap = true --> we're filling cells as a steam, consider this a wrap + // - wrap = false --> we're filling cells as a block, unwrap + if (wrap.has_value() && fillingLastColumn) { - _charRow.SetWrapForced(true); + // set wrap status on the row to parameter's value. + _charRow.SetWrapForced(wrap.value()); } } else diff --git a/src/buffer/out/Row.hpp b/src/buffer/out/Row.hpp index 27d2a390928..787e7339d95 100644 --- a/src/buffer/out/Row.hpp +++ b/src/buffer/out/Row.hpp @@ -57,7 +57,7 @@ class ROW final UnicodeStorage& GetUnicodeStorage() noexcept; const UnicodeStorage& GetUnicodeStorage() const noexcept; - OutputCellIterator WriteCells(OutputCellIterator it, const size_t index, const bool setWrap, std::optional limitRight = std::nullopt); + OutputCellIterator WriteCells(OutputCellIterator it, const size_t index, const std::optional wrap = std::nullopt, std::optional limitRight = std::nullopt); friend bool operator==(const ROW& a, const ROW& b) noexcept; diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 1c51fde4bf0..fdec52c363a 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -318,10 +318,12 @@ OutputCellIterator TextBuffer::Write(const OutputCellIterator givenIt) // Arguments: // - givenIt - Iterator representing output cell data to write // - target - the row/column to start writing the text to +// - wrap - change the wrap flag if we hit the end of the row while writing and there's still more data // Return Value: // - The final position of the iterator OutputCellIterator TextBuffer::Write(const OutputCellIterator givenIt, - const COORD target) + const COORD target, + const std::optional wrap) { // Make mutable copy so we can walk. auto it = givenIt; @@ -336,7 +338,8 @@ OutputCellIterator TextBuffer::Write(const OutputCellIterator givenIt, while (it && size.IsInBounds(lineTarget)) { // Attempt to write as much data as possible onto this line. - it = WriteLine(it, lineTarget, true); + // NOTE: if wrap = true/false, we want to set the line's wrap to true/false (respectively) if we reach the end of the line + it = WriteLine(it, lineTarget, wrap); // Move to the next line down. lineTarget.X = 0; @@ -351,13 +354,13 @@ OutputCellIterator TextBuffer::Write(const OutputCellIterator givenIt, // Arguments: // - givenIt - The iterator that will dereference into cell data to insert // - target - Coordinate targeted within output buffer -// - setWrap - Whether we should try to set the wrap flag if we write up to the end of the line and have more data +// - wrap - change the wrap flag if we hit the end of the row while writing and there's still more data in the iterator. // - limitRight - Optionally restrict the right boundary for writing (e.g. stop writing earlier than the end of line) // Return Value: // - The iterator, but advanced to where we stopped writing. Use to find input consumed length or cells written length. OutputCellIterator TextBuffer::WriteLine(const OutputCellIterator givenIt, const COORD target, - const bool setWrap, + const std::optional wrap, std::optional limitRight) { // If we're not in bounds, exit early. @@ -368,7 +371,7 @@ OutputCellIterator TextBuffer::WriteLine(const OutputCellIterator givenIt, // Get the row and write the cells ROW& row = GetRowByOffset(target.Y); - const auto newIt = row.WriteCells(givenIt, target.X, setWrap, limitRight); + const auto newIt = row.WriteCells(givenIt, target.X, wrap, limitRight); // Take the cell distance written and notify that it needs to be repainted. const auto written = newIt.GetCellDistance(givenIt); diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 6f461eeefe8..ced8f0904bd 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -89,11 +89,12 @@ class TextBuffer final OutputCellIterator Write(const OutputCellIterator givenIt); OutputCellIterator Write(const OutputCellIterator givenIt, - const COORD target); + const COORD target, + const std::optional wrap = true); OutputCellIterator WriteLine(const OutputCellIterator givenIt, const COORD target, - const bool setWrap = false, + const std::optional setWrap = std::nullopt, const std::optional limitRight = std::nullopt); bool InsertCharacter(const wchar_t wch, const DbcsAttribute dbcsAttribute, const TextAttribute attr); diff --git a/src/host/_output.cpp b/src/host/_output.cpp index 6a0a9ff7013..8d9fad7640a 100644 --- a/src/host/_output.cpp +++ b/src/host/_output.cpp @@ -290,7 +290,10 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region) try { const OutputCellIterator it(character, lengthToWrite); - const auto done = screenInfo.Write(it, startingCoordinate); + + // when writing to the buffer, specifically unset wrap if we get to the last column. + // a fill operation should UNSET wrap in that scenario. See GH #1126 for more details. + const auto done = screenInfo.Write(it, startingCoordinate, false); cellsModified = done.GetInputDistance(it); // Notify accessibility diff --git a/src/host/ft_host/API_FillOutputTests.cpp b/src/host/ft_host/API_FillOutputTests.cpp index 6505dfc746e..d0b9f2fa387 100644 --- a/src/host/ft_host/API_FillOutputTests.cpp +++ b/src/host/ft_host/API_FillOutputTests.cpp @@ -58,4 +58,125 @@ class FillOutputTests &charsWritten)); VERIFY_ARE_EQUAL(1u, charsWritten); } + + TEST_METHOD(UnsetWrap) + { + // WARNING: If this test suddenly decides to start failing, + // this is because the wrap registry key is not set. + // TODO GH #2859: Get/Set Registry Key for Wrap + + HANDLE hConsole = GetStdOutputHandle(); + DWORD charsWritten = 0; + + CONSOLE_SCREEN_BUFFER_INFOEX sbiex = { 0 }; + sbiex.cbSize = sizeof(sbiex); + VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleScreenBufferInfoEx(hConsole, &sbiex)); + + const auto consoleWidth = sbiex.dwSize.X; + + std::wstring input(consoleWidth + 2, L'a'); + std::wstring filled(consoleWidth, L'b'); + + // Write until a wrap occurs + VERIFY_WIN32_BOOL_SUCCEEDED(WriteConsoleW(hConsole, + input.data(), + gsl::narrow_cast(input.size()), + &charsWritten, + nullptr)); + + // Verify wrap occurred + std::unique_ptr bufferText = std::make_unique(consoleWidth); + DWORD readSize = 0; + VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole, + bufferText.get(), + consoleWidth, + { 0, 0 }, + &readSize)); + + WEX::Common::String expected(input.c_str(), readSize); + WEX::Common::String actual(bufferText.get(), readSize); + VERIFY_ARE_EQUAL(expected, actual); + + bufferText = std::make_unique(2); + VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole, + bufferText.get(), + 2, + { 0, 1 }, + &readSize)); + + VERIFY_ARE_EQUAL(2u, readSize); + expected = WEX::Common::String(input.c_str(), readSize); + actual = WEX::Common::String(bufferText.get(), readSize); + VERIFY_ARE_EQUAL(expected, actual); + + // Fill Console Line with 'b's + VERIFY_WIN32_BOOL_SUCCEEDED(FillConsoleOutputCharacterW(hConsole, + L'b', + consoleWidth, + { 2, 0 }, + &charsWritten)); + + // Verify first line is full of 'a's then 'b's + bufferText = std::make_unique(consoleWidth); + VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole, + bufferText.get(), + consoleWidth, + { 0, 0 }, + &readSize)); + + expected = WEX::Common::String(input.c_str(), 2); + actual = WEX::Common::String(bufferText.get(), 2); + VERIFY_ARE_EQUAL(expected, actual); + + expected = WEX::Common::String(filled.c_str(), consoleWidth - 2); + actual = WEX::Common::String(&bufferText[2], readSize - 2); + VERIFY_ARE_EQUAL(expected, actual); + + // Verify second line is still has 'a's that wrapped over + bufferText = std::make_unique(2); + VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole, + bufferText.get(), + static_cast(2), + { 0, 0 }, + &readSize)); + + VERIFY_ARE_EQUAL(2u, readSize); + expected = WEX::Common::String(input.c_str(), 2); + actual = WEX::Common::String(bufferText.get(), readSize); + VERIFY_ARE_EQUAL(expected, actual); + + // Resize to be smaller by 2 + sbiex.srWindow.Right -= 2; + sbiex.dwSize.X -= 2; + VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleScreenBufferInfoEx(hConsole, &sbiex)); + + // Verify first line is full of 'a's then 'b's + bufferText = std::make_unique(consoleWidth - 2); + VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole, + bufferText.get(), + consoleWidth - static_cast(2), + { 0, 0 }, + &readSize)); + + expected = WEX::Common::String(input.c_str(), 2); + actual = WEX::Common::String(bufferText.get(), 2); + VERIFY_ARE_EQUAL(expected, actual); + + expected = WEX::Common::String(filled.c_str(), consoleWidth - 4); + actual = WEX::Common::String(&bufferText[2], readSize - 2); + VERIFY_ARE_EQUAL(expected, actual); + + // Verify second line is still has 'a's ('b's didn't wrap over) + bufferText = std::make_unique(static_cast(2)); + VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputCharacterW(hConsole, + bufferText.get(), + static_cast(2), + { 0, 0 }, + &readSize)); + + VERIFY_ARE_EQUAL(2u, readSize); + expected = WEX::Common::String(input.c_str(), 2); + actual = WEX::Common::String(bufferText.get(), readSize); + VERIFY_ARE_EQUAL(expected, actual); + } }; diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp index dc54614e9c0..ec384715803 100644 --- a/src/host/screenInfo.cpp +++ b/src/host/screenInfo.cpp @@ -2599,14 +2599,17 @@ OutputCellIterator SCREEN_INFORMATION::Write(const OutputCellIterator it) // Arguments: // - it - Iterator representing output cell data to write. // - target - The position to start writing at +// - wrap - change the wrap flag if we hit the end of the row while writing and there's still more data // Return Value: // - the iterator at its final position // Note: // - will throw exception on error. OutputCellIterator SCREEN_INFORMATION::Write(const OutputCellIterator it, - const COORD target) + const COORD target, + const std::optional wrap) { - return _textBuffer->Write(it, target); + // NOTE: if wrap = true/false, we want to set the line's wrap to true/false (respectively) if we reach the end of the line + return _textBuffer->Write(it, target, wrap); } // Routine Description: diff --git a/src/host/screenInfo.hpp b/src/host/screenInfo.hpp index d44a16e05d6..3c5c95b867e 100644 --- a/src/host/screenInfo.hpp +++ b/src/host/screenInfo.hpp @@ -131,7 +131,8 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console OutputCellIterator Write(const OutputCellIterator it); OutputCellIterator Write(const OutputCellIterator it, - const COORD target); + const COORD target, + const std::optional wrap = true); OutputCellIterator WriteRect(const OutputCellIterator it, const Microsoft::Console::Types::Viewport viewport); diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index 4a31e44697b..226b4ced32d 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -77,6 +77,8 @@ class TextBufferTests TEST_METHOD(TestWrapFlag); + TEST_METHOD(TestWrapThroughWriteLine); + TEST_METHOD(TestDoubleBytePadFlag); void DoBoundaryTest(PWCHAR const pwszInputString, @@ -197,6 +199,78 @@ void TextBufferTests::TestWrapFlag() VERIFY_IS_FALSE(Row.GetCharRow().WasWrapForced()); } +void TextBufferTests::TestWrapThroughWriteLine() +{ + TextBuffer& textBuffer = GetTbi(); + + auto VerifyWrap = [&](bool expected) { + ROW& Row = textBuffer._GetFirstRow(); + + if (expected) + { + VERIFY_IS_TRUE(Row.GetCharRow().WasWrapForced()); + } + else + { + VERIFY_IS_FALSE(Row.GetCharRow().WasWrapForced()); + } + }; + + // Construct string for testing + const auto width = textBuffer.GetSize().Width(); + std::wstring chars = L""; + for (auto i = 0; i < width; i++) + { + chars.append(L"a"); + } + const auto lineOfText = std::move(chars); + + Log::Comment(L"Case 1 : Implicit wrap (false)"); + { + TextAttribute expectedAttr(FOREGROUND_RED); + OutputCellIterator it(lineOfText, expectedAttr); + + textBuffer.WriteLine(it, { 0, 0 }); + VerifyWrap(false); + } + + Log::Comment(L"Case 2 : wrap = true"); + { + TextAttribute expectedAttr(FOREGROUND_RED); + OutputCellIterator it(lineOfText, expectedAttr); + + textBuffer.WriteLine(it, { 0, 0 }, true); + VerifyWrap(true); + } + + Log::Comment(L"Case 3: wrap = nullopt (remain as TRUE)"); + { + TextAttribute expectedAttr(FOREGROUND_RED); + OutputCellIterator it(lineOfText, expectedAttr); + + textBuffer.WriteLine(it, { 0, 0 }, std::nullopt); + VerifyWrap(true); + } + + Log::Comment(L"Case 4: wrap = false"); + { + TextAttribute expectedAttr(FOREGROUND_RED); + OutputCellIterator it(lineOfText, expectedAttr); + + textBuffer.WriteLine(it, { 0, 0 }, false); + VerifyWrap(false); + } + + Log::Comment(L"Case 5: wrap = nullopt (remain as false)"); + { + TextAttribute expectedAttr(FOREGROUND_RED); + OutputCellIterator it(lineOfText, expectedAttr); + + textBuffer.WriteLine(it, { 0, 0 }, std::nullopt); + VerifyWrap(false); + } +} + void TextBufferTests::TestDoubleBytePadFlag() { TextBuffer& textBuffer = GetTbi(); From 342e741082d910b52c6aed27ea7ad7974efb6a94 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 1 Oct 2019 10:56:06 -0500 Subject: [PATCH 04/11] When reverse indexing, preserve RGB/256 attributes (#2987) (cherry picked from commit 847d6b56adb48b894dbf18bc15ba55be14e8cb3f) --- src/host/getset.cpp | 25 +++++++++++++----- src/host/ut_host/ScreenBufferTests.cpp | 36 ++++++++++++++++++++------ 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 3bdc2daa049..4f7d29c0d8c 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -1389,12 +1389,25 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool coordDestination.X = 0; coordDestination.Y = viewport.Top + 1; - Status = NTSTATUS_FROM_HRESULT(ServiceLocator::LocateGlobals().api.ScrollConsoleScreenBufferWImpl(screenInfo, - srScroll, - coordDestination, - srScroll, - UNICODE_SPACE, - screenInfo.GetAttributes().GetLegacyAttributes())); + // Here we previously called to ScrollConsoleScreenBufferWImpl to + // perform the scrolling operation. However, that function only + // accepts a WORD for the fill attributes. That means we'd lose + // 256/RGB fidelity for fill attributes. So instead, we'll just call + // ScrollRegion ourselves, with the same params that + // ScrollConsoleScreenBufferWImpl would have. + // See microsoft/terminal#832, #2702 for more context. + try + { + LockConsole(); + auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); + ScrollRegion(screenInfo, + srScroll, + srScroll, + coordDestination, + UNICODE_SPACE, + screenInfo.GetAttributes()); + } + CATCH_LOG(); } } return Status; diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 495933383cc..6def205dd4e 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -171,7 +171,7 @@ class ScreenBufferTests TEST_METHOD(DeleteLinesInMargins); TEST_METHOD(ReverseLineFeedInMargins); - TEST_METHOD(InsertDeleteLines256Colors); + TEST_METHOD(ScrollLines256Colors); TEST_METHOD(SetOriginMode); @@ -3996,10 +3996,10 @@ void ScreenBufferTests::ReverseLineFeedInMargins() } } -void ScreenBufferTests::InsertDeleteLines256Colors() +void ScreenBufferTests::ScrollLines256Colors() { BEGIN_TEST_METHOD_PROPERTIES() - TEST_METHOD_PROPERTY(L"Data:insert", L"{false, true}") + TEST_METHOD_PROPERTY(L"Data:scrollType", L"{0, 1, 2}") TEST_METHOD_PROPERTY(L"Data:colorStyle", L"{0, 1, 2}") END_TEST_METHOD_PROPERTIES(); @@ -4009,16 +4009,22 @@ void ScreenBufferTests::InsertDeleteLines256Colors() const int Use256Color = 1; const int UseRGBColor = 2; - bool insert; + // scrollType will be used to control whether we use InsertLines, + // DeleteLines, or ReverseIndex to scroll the contents of the buffer. + const int InsertLines = 0; + const int DeleteLines = 1; + const int ReverseLineFeed = 2; + + int scrollType; int colorStyle; - VERIFY_SUCCEEDED(TestData::TryGetValue(L"insert", insert), L"whether to insert(true) or delete(false) lines"); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"scrollType", scrollType), L"controls whether to use InsertLines, DeleteLines ot ReverseLineFeed"); VERIFY_SUCCEEDED(TestData::TryGetValue(L"colorStyle", colorStyle), L"controls whether to use the 16 color table, 256 table, or RGB colors"); // This test is largely taken from repro code from // https://github.com/microsoft/terminal/issues/832#issuecomment-507447272 Log::Comment( L"Sets the attributes to a 256/RGB color, then scrolls some lines with" - L" DL. Verifies the rows are cleared with the attributes we'd expect."); + L" IL/DL/RI. Verifies the rows are cleared with the attributes we'd expect."); auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); auto& si = gci.GetActiveOutputBuffer(); @@ -4054,8 +4060,22 @@ void ScreenBufferTests::InsertDeleteLines256Colors() // Move to home stateMachine.ProcessString(L"\x1b[H"); - // Insert/Delete 10 lines - stateMachine.ProcessString(insert ? L"\x1b[10L" : L"\x1b[10M"); + // Insert/Delete/Reverse Index 10 lines + std::wstring scrollSeq = L""; + if (scrollType == InsertLines) + { + scrollSeq = L"\x1b[10L"; + } + if (scrollType == DeleteLines) + { + scrollSeq = L"\x1b[10M"; + } + if (scrollType == ReverseLineFeed) + { + // This is 10 "Reverse Index" commands, which don't accept a parameter. + scrollSeq = L"\x1bM\x1bM\x1bM\x1bM\x1bM\x1bM\x1bM\x1bM\x1bM\x1bM"; + } + stateMachine.ProcessString(scrollSeq); Log::Comment(NoThrowString().Format( L"cursor=%s", VerifyOutputTraits::ToString(cursor.GetPosition()).GetBuffer())); From 28115f7361cbb8d4f9bfbd372cfa9af0a2eecfb5 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 1 Oct 2019 12:42:33 -0500 Subject: [PATCH 05/11] do not allow CUU and CUD to move "across" margins. (#2996) If we're moving the cursor up, its vertical movement should be stopped at the top margin. It should not magically jump up to the bottom margin. Similarly, this applies to moving down and the bottom margin. Furthermore, this constraint should always apply, not just when the start position is within BOTH margins Fixes #2929. (cherry picked from commit 0ce08aff32573e1125b9b38f339058b01952b3e5) --- src/host/getset.cpp | 44 +++++-- src/host/ut_host/ScreenBufferTests.cpp | 171 +++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 9 deletions(-) diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 4f7d29c0d8c..77fb4d9c417 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -1430,18 +1430,44 @@ void DoSrvPrivateAllowCursorBlinking(SCREEN_INFORMATION& screenInfo, const bool // Make sure the cursor doesn't move outside the viewport. screenInfo.GetViewport().Clamp(clampedPos); - // Make sure the cursor stays inside the margins, but only if it started there - if (screenInfo.AreMarginsSet() && screenInfo.IsCursorInMargins(cursor.GetPosition())) + // Make sure the cursor stays inside the margins + if (screenInfo.AreMarginsSet()) { - try + const auto margins = screenInfo.GetAbsoluteScrollMargins().ToInclusive(); + + const auto cursorY = cursor.GetPosition().Y; + + const auto lo = margins.Top; + const auto hi = margins.Bottom; + + // See microsoft/terminal#2929 - If the cursor is _below_ the top + // margin, it should stay below the top margin. If it's _above_ the + // bottom, it should stay above the bottom. Cursor movements that stay + // outside the margins shouldn't necessarily be affected. For example, + // moving up while below the bottom margin shouldn't just jump straight + // to the bottom margin. See + // ScreenBufferTests::CursorUpDownOutsideMargins for a test of that + // behavior. + const bool cursorBelowTop = cursorY >= lo; + const bool cursorAboveBottom = cursorY <= hi; + + if (cursorBelowTop) + { + try + { + clampedPos.Y = std::max(clampedPos.Y, lo); + } + CATCH_RETURN(); + } + + if (cursorAboveBottom) { - const auto margins = screenInfo.GetAbsoluteScrollMargins().ToInclusive(); - const auto v = clampedPos.Y; - const auto lo = margins.Top; - const auto hi = margins.Bottom; - clampedPos.Y = std::clamp(v, lo, hi); + try + { + clampedPos.Y = std::min(clampedPos.Y, hi); + } + CATCH_RETURN(); } - CATCH_RETURN(); } cursor.SetPosition(clampedPos); diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 6def205dd4e..82b9d77f05b 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -184,6 +184,10 @@ class ScreenBufferTests TEST_METHOD(ClearAlternateBuffer); TEST_METHOD(InitializeTabStopsInVTMode); + + TEST_METHOD(CursorUpDownAcrossMargins); + TEST_METHOD(CursorUpDownOutsideMargins); + TEST_METHOD(CursorUpDownExactlyAtMargins); }; void ScreenBufferTests::SingleAlternateBufferCreationTest() @@ -4500,3 +4504,170 @@ void ScreenBufferTests::InitializeTabStopsInVTMode() VERIFY_IS_TRUE(gci.GetActiveOutputBuffer().AreTabsSet()); } + +void ScreenBufferTests::CursorUpDownAcrossMargins() +{ + // Test inspired by: https://github.com/microsoft/terminal/issues/2929 + // echo -e "\e[6;19r\e[24H\e[99AX\e[1H\e[99BY\e[r" + // This does the following: + // * sets the top and bottom DECSTBM margins to 6 and 19 + // * moves to line 24 (i.e. below the bottom margin) + // * executes the CUU sequence with a count of 99, to move up 99 lines + // * writes out X + // * moves to line 1 (i.e. above the top margin) + // * executes the CUD sequence with a count of 99, to move down 99 lines + // * writes out Y + + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& tbi = si.GetTextBuffer(); + auto& stateMachine = si.GetStateMachine(); + auto& cursor = si.GetTextBuffer().GetCursor(); + + VERIFY_IS_TRUE(si.GetViewport().BottomInclusive() > 24); + + // Set some scrolling margins + stateMachine.ProcessString(L"\x1b[6;19r"); + stateMachine.ProcessString(L"\x1b[24H"); + VERIFY_ARE_EQUAL(23, cursor.GetPosition().Y); + + stateMachine.ProcessString(L"\x1b[99A"); + VERIFY_ARE_EQUAL(5, cursor.GetPosition().Y); + stateMachine.ProcessString(L"X"); + { + auto iter = tbi.GetCellDataAt({ 0, 5 }); + VERIFY_ARE_EQUAL(L"X", iter->Chars()); + } + stateMachine.ProcessString(L"\x1b[1H"); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + + stateMachine.ProcessString(L"\x1b[99B"); + VERIFY_ARE_EQUAL(18, cursor.GetPosition().Y); + stateMachine.ProcessString(L"Y"); + { + auto iter = tbi.GetCellDataAt({ 0, 18 }); + VERIFY_ARE_EQUAL(L"Y", iter->Chars()); + } + stateMachine.ProcessString(L"\x1b[r"); +} + +void ScreenBufferTests::CursorUpDownOutsideMargins() +{ + // Test inspired by the CursorUpDownAcrossMargins test. + // echo -e "\e[6;19r\e[24H\e[1AX\e[1H\e[1BY\e[r" + // This does the following: + // * sets the top and bottom DECSTBM margins to 6 and 19 + // * moves to line 24 (i.e. below the bottom margin) + // * executes the CUU sequence with a count of 1, to move up 1 lines (still below margins) + // * writes out X + // * moves to line 1 (i.e. above the top margin) + // * executes the CUD sequence with a count of 1, to move down 1 lines (still above margins) + // * writes out Y + + // This test is different becasue the end location of the vertical movement + // should not be within the margins at all. We should not clamp this + // movement to be within the margins. + + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& tbi = si.GetTextBuffer(); + auto& stateMachine = si.GetStateMachine(); + auto& cursor = si.GetTextBuffer().GetCursor(); + + VERIFY_IS_TRUE(si.GetViewport().BottomInclusive() > 24); + + // Set some scrolling margins + stateMachine.ProcessString(L"\x1b[6;19r"); + stateMachine.ProcessString(L"\x1b[24H"); + VERIFY_ARE_EQUAL(23, cursor.GetPosition().Y); + + stateMachine.ProcessString(L"\x1b[1A"); + VERIFY_ARE_EQUAL(22, cursor.GetPosition().Y); + stateMachine.ProcessString(L"X"); + { + auto iter = tbi.GetCellDataAt({ 0, 22 }); + VERIFY_ARE_EQUAL(L"X", iter->Chars()); + } + stateMachine.ProcessString(L"\x1b[1H"); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + + stateMachine.ProcessString(L"\x1b[1B"); + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + stateMachine.ProcessString(L"Y"); + { + auto iter = tbi.GetCellDataAt({ 0, 1 }); + VERIFY_ARE_EQUAL(L"Y", iter->Chars()); + } + stateMachine.ProcessString(L"\x1b[r"); +} + +void ScreenBufferTests::CursorUpDownExactlyAtMargins() +{ + // Test inspired by the CursorUpDownAcrossMargins test. + // echo -e "\e[6;19r\e[19H\e[1B1\e[1A2\e[6H\e[1A3\e[1B4\e[r" + // This does the following: + // * sets the top and bottom DECSTBM margins to 6 and 19 + // * moves to line 19 (i.e. on the bottom margin) + // * executes the CUD sequence with a count of 1, to move down 1 lines (still on the margin) + // * writes out 1 + // * executes the CUU sequence with a count of 1, to move up 1 lines (now inside margins) + // * writes out 2 + // * moves to line 6 (i.e. on the top margin) + // * executes the CUU sequence with a count of 1, to move up 1 lines (still on the margin) + // * writes out 3 + // * executes the CUD sequence with a count of 1, to move down 1 lines (still above margins) + // * writes out 4 + + // This test is different becasue the starting location for these scroll + // operations is _exactly_ on the margins + + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& tbi = si.GetTextBuffer(); + auto& stateMachine = si.GetStateMachine(); + auto& cursor = si.GetTextBuffer().GetCursor(); + + VERIFY_IS_TRUE(si.GetViewport().BottomInclusive() > 24); + + // Set some scrolling margins + stateMachine.ProcessString(L"\x1b[6;19r"); + + stateMachine.ProcessString(L"\x1b[19;1H"); + VERIFY_ARE_EQUAL(18, cursor.GetPosition().Y); + stateMachine.ProcessString(L"\x1b[1B"); + VERIFY_ARE_EQUAL(18, cursor.GetPosition().Y); + stateMachine.ProcessString(L"1"); + { + auto iter = tbi.GetCellDataAt({ 0, 18 }); + VERIFY_ARE_EQUAL(L"1", iter->Chars()); + } + + stateMachine.ProcessString(L"\x1b[1A"); + VERIFY_ARE_EQUAL(17, cursor.GetPosition().Y); + stateMachine.ProcessString(L"2"); + { + auto iter = tbi.GetCellDataAt({ 1, 17 }); + VERIFY_ARE_EQUAL(L"2", iter->Chars()); + } + + stateMachine.ProcessString(L"\x1b[6;1H"); + VERIFY_ARE_EQUAL(5, cursor.GetPosition().Y); + + stateMachine.ProcessString(L"\x1b[1A"); + VERIFY_ARE_EQUAL(5, cursor.GetPosition().Y); + stateMachine.ProcessString(L"3"); + { + auto iter = tbi.GetCellDataAt({ 0, 5 }); + VERIFY_ARE_EQUAL(L"3", iter->Chars()); + } + + stateMachine.ProcessString(L"\x1b[1B"); + VERIFY_ARE_EQUAL(6, cursor.GetPosition().Y); + stateMachine.ProcessString(L"4"); + { + auto iter = tbi.GetCellDataAt({ 1, 6 }); + VERIFY_ARE_EQUAL(L"4", iter->Chars()); + } + + stateMachine.ProcessString(L"\x1b[r"); +} From 572e4ba88bc14cefa56e34a1a78a6eb1ce8f3009 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 2 Oct 2019 18:04:59 -0500 Subject: [PATCH 06/11] Prevent the v1 propsheet from zeroing colors, causing black text on black background. (#2651) * This fixes the registry path What's happening is the console is writing the Forcev2 setting, then the v1 console is ignoring those settings, then when you check the checkbox to save the v2 settings, we'll write the zeros out. That's obviously bad. So we'll only write the v2 settings back to the registry if the propsheet was launched from a v2 console. This does not fix the shortcut path. That'll be the next commit. * Fix the shortcut loading too fixes #2319 * remove the redundant property I added * add some notes to the bx.ps1 change (cherry picked from commit b97db6303000f2a3c670862c0771069dfcca9972) --- src/propsheet/PropSheetHandler.cpp | 6 ++ src/propsheet/console.cpp | 5 +- src/propsheet/console.h | 2 + src/propsheet/globals.cpp | 3 + src/propsheet/registry.cpp | 93 ++++++++++++++------------ src/propslib/ShortcutSerialization.cpp | 38 +++++++---- src/propslib/ShortcutSerialization.hpp | 2 +- 7 files changed, 91 insertions(+), 58 deletions(-) diff --git a/src/propsheet/PropSheetHandler.cpp b/src/propsheet/PropSheetHandler.cpp index d4b93c886e2..c6f2c5e412d 100644 --- a/src/propsheet/PropSheetHandler.cpp +++ b/src/propsheet/PropSheetHandler.cpp @@ -94,6 +94,12 @@ class ConsolePropertySheetHandler WrlFinal : public RuntimeClassfIsV2Console = GetConsoleBoolValue(CONSOLE_REGISTRY_FORCEV2, TRUE); + InitRegistryValues(gpStateInfo); gpStateInfo->Defaults = TRUE; GetRegistryValues(gpStateInfo); diff --git a/src/propsheet/console.cpp b/src/propsheet/console.cpp index cf3683dc8e0..6a40487514f 100644 --- a/src/propsheet/console.cpp +++ b/src/propsheet/console.cpp @@ -97,7 +97,10 @@ void SaveConsoleSettingsIfNeeded(const HWND hwnd) if (gpStateInfo->LinkTitle != NULL) { SetGlobalRegistryValues(); - if (!NT_SUCCESS(ShortcutSerialization::s_SetLinkValues(gpStateInfo, g_fEastAsianSystem, g_fForceV2))) + if (!NT_SUCCESS(ShortcutSerialization::s_SetLinkValues(gpStateInfo, + g_fEastAsianSystem, + g_fForceV2, + gpStateInfo->fIsV2Console))) { WCHAR szMessage[MAX_PATH + 100]; WCHAR awchBuffer[MAX_PATH] = { 0 }; diff --git a/src/propsheet/console.h b/src/propsheet/console.h index 8094ce31dba..7a98f78c28e 100644 --- a/src/propsheet/console.h +++ b/src/propsheet/console.h @@ -225,3 +225,5 @@ const unsigned int TERMINAL_PAGE_INDEX = 4; // number of property sheet pages static const int V1_NUMBER_OF_PAGES = 4; static const int NUMBER_OF_PAGES = 5; + +BOOL GetConsoleBoolValue(__in PCWSTR pszValueName, __in BOOL fDefault); diff --git a/src/propsheet/globals.cpp b/src/propsheet/globals.cpp index b7c59171cc9..01de6df00e9 100644 --- a/src/propsheet/globals.cpp +++ b/src/propsheet/globals.cpp @@ -9,6 +9,9 @@ LONG gcxScreen; LONG gcyScreen; BOOL g_fForceV2; +// If we didn't launch as a v2 console window, we don't want to persist v2 +// settings when we close, as they'll get zero'd. Use this to track the initial +// launch state. BOOL g_fEditKeys; BYTE g_bPreviewOpacity = 0x00; //sentinel value for initial test on dialog entry. Once initialized, won't be less than TRANSPARENCY_RANGE_MIN diff --git a/src/propsheet/registry.cpp b/src/propsheet/registry.cpp index 2914ae0998f..5c7a501d169 100644 --- a/src/propsheet/registry.cpp +++ b/src/propsheet/registry.cpp @@ -946,52 +946,59 @@ VOID SetRegistryValues( SetGlobalRegistryValues(); - // Save cursor type and color - dwValue = pStateInfo->CursorType; - LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, - hTitleKey, - CONSOLE_REGISTRY_CURSORTYPE, - REG_DWORD, - (BYTE*)&dwValue, - sizeof(dwValue))); + // Only save the "Terminal" settings if we launched as a v2 propsheet. The + // v1 console doesn't know anything about these settings, and their value + // will be incorrectly zero'd if we save in this state. + // See microsoft/terminal#2319 for more details. + if (gpStateInfo->fIsV2Console) + { + // Save cursor type and color + dwValue = pStateInfo->CursorType; + LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, + hTitleKey, + CONSOLE_REGISTRY_CURSORTYPE, + REG_DWORD, + (BYTE*)&dwValue, + sizeof(dwValue))); - dwValue = pStateInfo->CursorColor; - LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, - hTitleKey, - CONSOLE_REGISTRY_CURSORCOLOR, - REG_DWORD, - (BYTE*)&dwValue, - sizeof(dwValue))); + dwValue = pStateInfo->CursorColor; + LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, + hTitleKey, + CONSOLE_REGISTRY_CURSORCOLOR, + REG_DWORD, + (BYTE*)&dwValue, + sizeof(dwValue))); - dwValue = pStateInfo->InterceptCopyPaste; - LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, - hTitleKey, - CONSOLE_REGISTRY_INTERCEPTCOPYPASTE, - REG_DWORD, - (BYTE*)&dwValue, - sizeof(dwValue))); + dwValue = pStateInfo->InterceptCopyPaste; + LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, + hTitleKey, + CONSOLE_REGISTRY_INTERCEPTCOPYPASTE, + REG_DWORD, + (BYTE*)&dwValue, + sizeof(dwValue))); - dwValue = pStateInfo->TerminalScrolling; - LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, - hTitleKey, - CONSOLE_REGISTRY_TERMINALSCROLLING, - REG_DWORD, - (BYTE*)&dwValue, - sizeof(dwValue))); - dwValue = pStateInfo->DefaultForeground; - LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, - hTitleKey, - CONSOLE_REGISTRY_DEFAULTFOREGROUND, - REG_DWORD, - (BYTE*)&dwValue, - sizeof(dwValue))); - dwValue = pStateInfo->DefaultBackground; - LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, - hTitleKey, - CONSOLE_REGISTRY_DEFAULTBACKGROUND, - REG_DWORD, - (BYTE*)&dwValue, - sizeof(dwValue))); + dwValue = pStateInfo->TerminalScrolling; + LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, + hTitleKey, + CONSOLE_REGISTRY_TERMINALSCROLLING, + REG_DWORD, + (BYTE*)&dwValue, + sizeof(dwValue))); + dwValue = pStateInfo->DefaultForeground; + LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, + hTitleKey, + CONSOLE_REGISTRY_DEFAULTFOREGROUND, + REG_DWORD, + (BYTE*)&dwValue, + sizeof(dwValue))); + dwValue = pStateInfo->DefaultBackground; + LOG_IF_FAILED(RegistrySerialization::s_UpdateValue(hConsoleKey, + hTitleKey, + CONSOLE_REGISTRY_DEFAULTBACKGROUND, + REG_DWORD, + (BYTE*)&dwValue, + sizeof(dwValue))); + } // // Close the registry keys diff --git a/src/propslib/ShortcutSerialization.cpp b/src/propslib/ShortcutSerialization.cpp index a514de7c04d..39d21cc1a70 100644 --- a/src/propslib/ShortcutSerialization.cpp +++ b/src/propslib/ShortcutSerialization.cpp @@ -409,16 +409,19 @@ void ShortcutSerialization::s_GetLinkTitle(_In_ PCWSTR pwszShortcutFilename, return (SUCCEEDED(hr)) ? STATUS_SUCCESS : STATUS_UNSUCCESSFUL; } -/** -Writes the console properties out to the link it was opened from. -Arguments: - pStateInfo - pointer to structure containing information -Return Value: - A status code if something failed or S_OK -*/ +// Function Description: +// - Writes the console properties out to the link it was opened from. +// Arguments: +// - pStateInfo: pointer to structure containing information +// - writeTerminalSettings: If true, persist the "Terminal" properties only +// present in the v2 console. This should be false if called from a v11 +// console. See GH#2319 +// Return Value: +// - A status code if something failed or S_OK [[nodiscard]] NTSTATUS ShortcutSerialization::s_SetLinkValues(_In_ PCONSOLE_STATE_INFO pStateInfo, const BOOL fEastAsianSystem, - const BOOL fForceV2) + const BOOL fForceV2, + const bool writeTerminalSettings) { IShellLinkW* psl; IPersistFile* ppf; @@ -488,12 +491,21 @@ Return Value: s_SetLinkPropertyBoolValue(pps, PKEY_Console_CtrlKeyShortcutsDisabled, pStateInfo->fCtrlKeyShortcutsDisabled); s_SetLinkPropertyBoolValue(pps, PKEY_Console_LineSelection, pStateInfo->fLineSelection); s_SetLinkPropertyByteValue(pps, PKEY_Console_WindowTransparency, pStateInfo->bWindowTransparency); - s_SetLinkPropertyDwordValue(pps, PKEY_Console_CursorType, pStateInfo->CursorType); - s_SetLinkPropertyDwordValue(pps, PKEY_Console_CursorColor, pStateInfo->CursorColor); s_SetLinkPropertyBoolValue(pps, PKEY_Console_InterceptCopyPaste, pStateInfo->InterceptCopyPaste); - s_SetLinkPropertyDwordValue(pps, PKEY_Console_DefaultForeground, pStateInfo->DefaultForeground); - s_SetLinkPropertyDwordValue(pps, PKEY_Console_DefaultBackground, pStateInfo->DefaultBackground); - s_SetLinkPropertyBoolValue(pps, PKEY_Console_TerminalScrolling, pStateInfo->TerminalScrolling); + + // Only save the "Terminal" settings if we launched as a v2 + // propsheet. The v1 console doesn't know anything about + // these settings, and their value will be incorrectly + // zero'd if we save in this state. + // See microsoft/terminal#2319 for more details. + if (writeTerminalSettings) + { + s_SetLinkPropertyDwordValue(pps, PKEY_Console_CursorType, pStateInfo->CursorType); + s_SetLinkPropertyDwordValue(pps, PKEY_Console_CursorColor, pStateInfo->CursorColor); + s_SetLinkPropertyDwordValue(pps, PKEY_Console_DefaultForeground, pStateInfo->DefaultForeground); + s_SetLinkPropertyDwordValue(pps, PKEY_Console_DefaultBackground, pStateInfo->DefaultBackground); + s_SetLinkPropertyBoolValue(pps, PKEY_Console_TerminalScrolling, pStateInfo->TerminalScrolling); + } hr = pps->Commit(); pps->Release(); } diff --git a/src/propslib/ShortcutSerialization.hpp b/src/propslib/ShortcutSerialization.hpp index 27183b09c81..a21e57386f7 100644 --- a/src/propslib/ShortcutSerialization.hpp +++ b/src/propslib/ShortcutSerialization.hpp @@ -24,7 +24,7 @@ Revision History: class ShortcutSerialization { public: - [[nodiscard]] static NTSTATUS s_SetLinkValues(_In_ PCONSOLE_STATE_INFO pStateInfo, const BOOL fEastAsianSystem, const BOOL fForceV2); + [[nodiscard]] static NTSTATUS s_SetLinkValues(_In_ PCONSOLE_STATE_INFO pStateInfo, const BOOL fEastAsianSystem, const BOOL fForceV2, const bool writeTerminalSettings); [[nodiscard]] static NTSTATUS s_GetLinkConsoleProperties(_Inout_ PCONSOLE_STATE_INFO pStateInfo); [[nodiscard]] static NTSTATUS s_GetLinkValues(_Inout_ PCONSOLE_STATE_INFO pStateInfo, _Out_ BOOL* const pfReadConsoleProperties, From 9c15a8e8845bfe5b9a640b774c28551282539cd3 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 2 Oct 2019 18:11:27 -0500 Subject: [PATCH 07/11] Render the cursor state to VT (#2829) * Render the cursor state to VT * Remove TestPaintXterm entirely, as it's unused now (cherry picked from commit a9f384931e48c74b9a45d825f791542d13debdb9) --- src/host/ut_host/VtRendererTests.cpp | 183 +++++++++++++++++++-------- src/renderer/vt/XtermEngine.cpp | 73 ++++++++--- src/renderer/vt/XtermEngine.hpp | 4 + src/renderer/vt/vtrenderer.hpp | 2 +- 4 files changed, 192 insertions(+), 70 deletions(-) diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 9aebf916c3b..0fc75e6b3c6 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -121,12 +121,13 @@ class Microsoft::Console::Render::VtRendererTest TEST_METHOD(TestResize); + TEST_METHOD(TestCursorVisibility); + void Test16Colors(VtEngine* engine); std::deque qExpectedInput; bool WriteCallback(const char* const pch, size_t const cch); void TestPaint(VtEngine& engine, std::function pfn); - void TestPaintXterm(XtermEngine& engine, std::function pfn); Viewport SetUpViewport(); }; @@ -173,38 +174,6 @@ void VtRendererTest::TestPaint(VtEngine& engine, std::function pfn) VERIFY_SUCCEEDED(engine.EndPaint()); } -// Function Description: -// - Small helper to do a series of testing wrapped by StartPaint/EndPaint calls -// Also expects \x1b[?25l and \x1b[?25h on start/stop, for cursor visibility -// Arguments: -// - engine: the engine to operate on -// - pfn: A function pointer to some test code to run. -// Return Value: -// - -void VtRendererTest::TestPaintXterm(XtermEngine& engine, std::function pfn) -{ - HRESULT hr = engine.StartPaint(); - pfn(); - // If we didn't have anything to do on this frame, still execute our - // callback, but don't check for the following ?25h - if (hr != S_FALSE) - { - // If the engine has decided that it needs to disble the cursor, it'll - // insert ?25l to the front of the buffer (which won't hit this - // callback) and write ?25h to the end of the frame - if (engine._needToDisableCursor) - { - qExpectedInput.push_back("\x1b[?25h"); - } - } - - VERIFY_SUCCEEDED(engine.EndPaint()); - - VERIFY_ARE_EQUAL(qExpectedInput.size(), - static_cast(0), - L"Done painting, there shouldn't be any output we're still expecting"); -} - void VtRendererTest::VtSequenceHelperTests() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); @@ -298,7 +267,7 @@ void VtRendererTest::Xterm256TestInvalidate() L"Make sure that scrolling only invalidates part of the viewport, and sends the right sequences")); COORD scrollDelta = { 0, 1 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled one down, only top line is invalid. ----")); invalid = view.ToExclusive(); @@ -314,7 +283,7 @@ void VtRendererTest::Xterm256TestInvalidate() scrollDelta = { 0, 3 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled three down, only top 3 lines are invalid. ----")); invalid = view.ToExclusive(); @@ -328,7 +297,7 @@ void VtRendererTest::Xterm256TestInvalidate() scrollDelta = { 0, -1 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled one up, only bottom line is invalid. ----")); invalid = view.ToExclusive(); @@ -343,7 +312,7 @@ void VtRendererTest::Xterm256TestInvalidate() scrollDelta = { 0, -3 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled three up, only bottom 3 lines are invalid. ----")); invalid = view.ToExclusive(); @@ -363,7 +332,7 @@ void VtRendererTest::Xterm256TestInvalidate() VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); scrollDelta = { 0, 2 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled three down, only top 3 lines are invalid. ----")); invalid = view.ToExclusive(); @@ -557,8 +526,6 @@ void VtRendererTest::Xterm256TestCursor() L"----move to the start of this line (y stays the same)----")); qExpectedInput.push_back("\r"); VERIFY_SUCCEEDED(engine->_MoveCursor({ 0, 1 })); - - qExpectedInput.push_back("\x1b[?25h"); }); TestPaint(*engine, [&]() { @@ -629,7 +596,7 @@ void VtRendererTest::XtermTestInvalidate() L"Make sure that invalidating anything only invalidates that portion")); SMALL_RECT invalid = { 1, 1, 1, 1 }; VERIFY_SUCCEEDED(engine->Invalidate(&invalid)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { VERIFY_ARE_EQUAL(invalid, engine->_invalidRect.ToExclusive()); }); @@ -637,7 +604,7 @@ void VtRendererTest::XtermTestInvalidate() L"Make sure that scrolling only invalidates part of the viewport, and sends the right sequences")); COORD scrollDelta = { 0, 1 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled one down, only top line is invalid. ----")); invalid = view.ToExclusive(); @@ -652,7 +619,7 @@ void VtRendererTest::XtermTestInvalidate() scrollDelta = { 0, 3 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled three down, only top 3 lines are invalid. ----")); invalid = view.ToExclusive(); @@ -666,7 +633,7 @@ void VtRendererTest::XtermTestInvalidate() scrollDelta = { 0, -1 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled one up, only bottom line is invalid. ----")); invalid = view.ToExclusive(); @@ -681,7 +648,7 @@ void VtRendererTest::XtermTestInvalidate() scrollDelta = { 0, -3 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled three up, only bottom 3 lines are invalid. ----")); invalid = view.ToExclusive(); @@ -701,7 +668,7 @@ void VtRendererTest::XtermTestInvalidate() VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); scrollDelta = { 0, 2 }; VERIFY_SUCCEEDED(engine->InvalidateScroll(&scrollDelta)); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"---- Scrolled three down, only top 3 lines are invalid. ----")); invalid = view.ToExclusive(); @@ -864,8 +831,6 @@ void VtRendererTest::XtermTestCursor() L"----move to the start of this line (y stays the same)----")); qExpectedInput.push_back("\r"); VERIFY_SUCCEEDED(engine->_MoveCursor({ 0, 1 })); - - qExpectedInput.push_back("\x1b[?25h"); }); TestPaint(*engine, [&]() { @@ -1130,14 +1095,14 @@ void VtRendererTest::TestWrapping() Viewport view = SetUpViewport(); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"Make sure the cursor is at 0,0")); qExpectedInput.push_back("\x1b[H"); VERIFY_SUCCEEDED(engine->_MoveCursor({ 0, 0 })); }); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { Log::Comment(NoThrowString().Format( L"Painting a line that wrapped, then painting another line, and " L"making sure we don't manually move the cursor between those paints.")); @@ -1196,9 +1161,125 @@ void VtRendererTest::TestResize() VERIFY_SUCCEEDED(engine->UpdateViewport(newView.ToInclusive())); - TestPaintXterm(*engine, [&]() { + TestPaint(*engine, [&]() { VERIFY_ARE_EQUAL(newView, engine->_invalidRect); VERIFY_IS_FALSE(engine->_firstPaint); VERIFY_IS_FALSE(engine->_suppressResizeRepaint); }); } + +void VtRendererTest::TestCursorVisibility() +{ + Viewport view = SetUpViewport(); + wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); + auto engine = std::make_unique(std::move(hFile), _shutdownEvent, p, view, g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); + engine->SetTestCallback(pfn); + + // Verify the first paint emits a clear + qExpectedInput.push_back("\x1b[2J"); + VERIFY_IS_TRUE(engine->_firstPaint); + VERIFY_IS_FALSE(engine->_lastCursorIsVisible); + VERIFY_IS_TRUE(engine->_nextCursorIsVisible); + TestPaint(*engine, [&]() { + // During StartPaint, we'll mark the cursor as off. make sure that happens. + VERIFY_IS_FALSE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_firstPaint); + }); + + // The cursor wasn't painted in the last frame. + VERIFY_IS_FALSE(engine->_lastCursorIsVisible); + VERIFY_IS_FALSE(engine->_nextCursorIsVisible); + + COORD origin{ 0, 0 }; + + VERIFY_ARE_NOT_EQUAL(origin, engine->_lastText); + + IRenderEngine::CursorOptions options{}; + options.coordCursor = origin; + + // Frame 1: Paint the cursor at the home position. At the end of the frame, + // the cursor should be on. Because we're moving the cursor with CUP, we + // need to disable the cursor during this frame. + TestPaint(*engine, [&]() { + VERIFY_IS_FALSE(engine->_lastCursorIsVisible); + VERIFY_IS_FALSE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); + + Log::Comment(NoThrowString().Format(L"Make sure the cursor is at 0,0")); + qExpectedInput.push_back("\x1b[H"); + VERIFY_SUCCEEDED(engine->PaintCursor(options)); + + VERIFY_IS_TRUE(engine->_nextCursorIsVisible); + VERIFY_IS_TRUE(engine->_needToDisableCursor); + + qExpectedInput.push_back("\x1b[?25h"); + }); + + VERIFY_IS_TRUE(engine->_lastCursorIsVisible); + VERIFY_IS_TRUE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); + + // Frame 2: Paint the cursor again at the home position. At the end of the + // frame, the cursor should be on, the same as before. We aren't moving the + // cursor during this frame, so _needToDisableCursor will stay false. + TestPaint(*engine, [&]() { + VERIFY_IS_TRUE(engine->_lastCursorIsVisible); + VERIFY_IS_FALSE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); + + Log::Comment(NoThrowString().Format(L"If we just paint the cursor again at the same position, the cursor should not need to be disabled")); + VERIFY_SUCCEEDED(engine->PaintCursor(options)); + + VERIFY_IS_TRUE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); + }); + + VERIFY_IS_TRUE(engine->_lastCursorIsVisible); + VERIFY_IS_TRUE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); + + // Frame 3: Paint the cursor at 2,2. At the end of the frame, the cursor + // should be on, the same as before. Because we're moving the cursor with + // CUP, we need to disable the cursor during this frame. + TestPaint(*engine, [&]() { + VERIFY_IS_TRUE(engine->_lastCursorIsVisible); + VERIFY_IS_FALSE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); + + Log::Comment(NoThrowString().Format(L"Move the cursor to 2,2")); + qExpectedInput.push_back("\x1b[3;3H"); + + options.coordCursor = { 2, 2 }; + + VERIFY_SUCCEEDED(engine->PaintCursor(options)); + + VERIFY_IS_TRUE(engine->_lastCursorIsVisible); + VERIFY_IS_TRUE(engine->_nextCursorIsVisible); + VERIFY_IS_TRUE(engine->_needToDisableCursor); + + // Because _needToDisableCursor is true, we'll insert a ?25l at the + // start of the frame. Unfortunately, we can't test to make sure that + // it's there, but we can ensure that the matching ?25h is printed: + qExpectedInput.push_back("\x1b[?25h"); + }); + + VERIFY_IS_TRUE(engine->_lastCursorIsVisible); + VERIFY_IS_TRUE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); + + // Frame 4: Don't paint the cursor. At the end of the frame, the cursor + // should be off. + Log::Comment(NoThrowString().Format(L"Painting without calling PaintCursor will hide the cursor")); + TestPaint(*engine, [&]() { + VERIFY_IS_TRUE(engine->_lastCursorIsVisible); + VERIFY_IS_FALSE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); + + qExpectedInput.push_back("\x1b[?25l"); + }); + + VERIFY_IS_FALSE(engine->_lastCursorIsVisible); + VERIFY_IS_FALSE(engine->_nextCursorIsVisible); + VERIFY_IS_FALSE(engine->_needToDisableCursor); +} diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index b84166cb107..2e56da3864d 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -22,7 +22,9 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _fUseAsciiOnly(fUseAsciiOnly), _previousLineWrapped(false), _usingUnderLine(false), - _needToDisableCursor(false) + _needToDisableCursor(false), + _lastCursorIsVisible(false), + _nextCursorIsVisible(true) { // Set out initial cursor position to -1, -1. This will force our initial // paint to manually move the cursor to 0, 0, not just ignore it. @@ -45,6 +47,11 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _trace.TraceLastText(_lastText); + // Prep us to think that the cursor is not visible this frame. If it _is_ + // visible, then PaintCursor will be called, and we'll set this to true + // during the frame. + _nextCursorIsVisible = false; + if (_firstPaint) { // MSFT:17815688 @@ -73,14 +80,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, if (!_quickReturn) { - if (!_WillWriteSingleChar()) - { - // MSFT:TODO:20331739 - // Make sure to match the cursor visibility in the terminal to the console's - // // Turn off cursor - // RETURN_IF_FAILED(_HideCursor()); - } - else + if (_WillWriteSingleChar()) { // Don't re-enable the cursor. _quickReturn = true; @@ -99,22 +99,38 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write. [[nodiscard]] HRESULT XtermEngine::EndPaint() noexcept { - // MSFT:TODO:20331739 - // Make sure to match the cursor visibility in the terminal to the console's - // if (!_quickReturn) - // { - // // Turn on cursor - // RETURN_IF_FAILED(_ShowCursor()); - // } - // If during the frame we determined that the cursor needed to be disabled, // then insert a cursor off at the start of the buffer, and re-enable // the cursor here. if (_needToDisableCursor) { - _buffer.insert(0, "\x1b[25l"); + // If the cursor was previously visible, let's hide it for this frame, + // by prepending a cursor off. + if (_lastCursorIsVisible) + { + _buffer.insert(0, "\x1b[25l"); + _lastCursorIsVisible = false; + } + // If the cursor was NOT previously visible, then that's fine! we don't + // need to worry, it's already off. + } + + // If the cursor is moving from off -> on (including cases where we just + // disabled if for this frame), show the cursor at the end of the frame + if (_nextCursorIsVisible && !_lastCursorIsVisible) + { RETURN_IF_FAILED(_ShowCursor()); } + // Otherwise, if the cursor previously was visible, and it should be hidden + // (on -> off), hide it at the end of the frame. + else if (!_nextCursorIsVisible && _lastCursorIsVisible) + { + RETURN_IF_FAILED(_HideCursor()); + } + + // Update our tracker of what we thought the last cursor state of the + // terminal was. + _lastCursorIsVisible = _nextCursorIsVisible; RETURN_IF_FAILED(VtEngine::EndPaint()); @@ -178,6 +194,27 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, return VtEngine::_16ColorUpdateDrawingBrushes(colorForeground, colorBackground, isBold, _ColorTable, _cColorTable); } +// Routine Description: +// - Draws the cursor on the screen +// Arguments: +// - options - Options that affect the presentation of the cursor +// Return Value: +// - S_OK or suitable HRESULT error from writing pipe. +[[nodiscard]] HRESULT XtermEngine::PaintCursor(const IRenderEngine::CursorOptions& options) noexcept +{ + // PaintCursor is only called when the cursor is in fact visible in a single + // frame. When this is called, mark _nextCursorIsVisible as true. At the end + // of the frame, we'll decide to either turn the cursor on or not, based + // upon the previous state. + + // When this method is not called during a frame, it's because the cursor + // was not visible. In that case, at the end of the frame, + // _nextCursorIsVisible will still be false (from when we set it during + // StartPaint) + _nextCursorIsVisible = true; + return VtEngine::PaintCursor(options); +} + // Routine Description: // - Write a VT sequence to move the cursor to the specified coordinates. We // also store the last place we left the cursor for future optimizations. diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 0078030a5a5..67992a9abe0 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -40,6 +40,8 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT StartPaint() noexcept override; [[nodiscard]] HRESULT EndPaint() noexcept override; + [[nodiscard]] HRESULT PaintCursor(const CursorOptions& options) noexcept override; + [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, const WORD legacyColorAttribute, @@ -61,6 +63,8 @@ namespace Microsoft::Console::Render bool _previousLineWrapped; bool _usingUnderLine; bool _needToDisableCursor; + bool _lastCursorIsVisible; + bool _nextCursorIsVisible; [[nodiscard]] HRESULT _MoveCursor(const COORD coord) noexcept override; diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index d95ecd5d87c..8a419a7e9c6 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -64,7 +64,7 @@ namespace Microsoft::Console::Render const COORD coordTarget) noexcept override; [[nodiscard]] HRESULT PaintSelection(const SMALL_RECT rect) noexcept override; - [[nodiscard]] HRESULT PaintCursor(const CursorOptions& options) noexcept override; + [[nodiscard]] virtual HRESULT PaintCursor(const CursorOptions& options) noexcept override; [[nodiscard]] virtual HRESULT UpdateDrawingBrushes(const COLORREF colorForeground, const COLORREF colorBackground, From ba62a80206f9b95f7bb7b33584c8fce40046553c Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 3 Oct 2019 16:04:48 -0500 Subject: [PATCH 08/11] Don't put NUL in the buffer in VT mode (#3015) * Potentially fixes #1825 I haven't had a chance to test this fix on my machine with a CentOS VM quite yet, but this _should_ work Also adds a test * add a comment * woah hey this test was wrong * Revert bx.ps1 (cherry picked from commit a82d6b8c69f6e4b2fe946097da0e4c403cb42100) --- src/host/ut_host/ScreenBufferTests.cpp | 47 +++++++++++++++++-- .../parser/OutputStateMachineEngine.cpp | 8 ++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 82b9d77f05b..7abe51fe8fc 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -105,6 +105,8 @@ class ScreenBufferTests TEST_METHOD(EraseAllTests); + TEST_METHOD(OutputNULTest); + TEST_METHOD(VtResize); TEST_METHOD(VtResizeComprehensive); TEST_METHOD(VtResizeDECCOLM); @@ -373,14 +375,14 @@ void ScreenBufferTests::TestReverseLineFeed() cursor.SetPosition({ 0, 5 }); VERIFY_SUCCEEDED(screenInfo.SetViewportOrigin(true, { 0, 5 }, true)); - stateMachine.ProcessString(L"ABCDEFGH", 9); - VERIFY_ARE_EQUAL(cursor.GetPosition().X, 9); + stateMachine.ProcessString(L"ABCDEFGH"); + VERIFY_ARE_EQUAL(cursor.GetPosition().X, 8); VERIFY_ARE_EQUAL(cursor.GetPosition().Y, 5); VERIFY_ARE_EQUAL(screenInfo.GetViewport().Top(), 5); LOG_IF_FAILED(DoSrvPrivateReverseLineFeed(screenInfo)); - VERIFY_ARE_EQUAL(cursor.GetPosition().X, 9); + VERIFY_ARE_EQUAL(cursor.GetPosition().X, 8); VERIFY_ARE_EQUAL(cursor.GetPosition().Y, 5); viewport = screenInfo.GetViewport(); VERIFY_ARE_EQUAL(viewport.Top(), 5); @@ -816,6 +818,45 @@ void ScreenBufferTests::EraseAllTests() viewport.BottomInclusive())); } +void ScreenBufferTests::OutputNULTest() +{ + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& si = gci.GetActiveOutputBuffer(); + auto& stateMachine = si.GetStateMachine(); + auto& cursor = si._textBuffer->GetCursor(); + + VERIFY_ARE_EQUAL(0, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + + Log::Comment(NoThrowString().Format( + L"Writing a single NUL")); + stateMachine.ProcessString(L"\0", 1); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + + Log::Comment(NoThrowString().Format( + L"Writing many NULs")); + stateMachine.ProcessString(L"\0\0\0\0\0\0\0\0", 8); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + + Log::Comment(NoThrowString().Format( + L"Testing a single NUL followed by real text")); + stateMachine.ProcessString(L"\0foo", 4); + VERIFY_ARE_EQUAL(3, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().Y); + + stateMachine.ProcessString(L"\n"); + VERIFY_ARE_EQUAL(0, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); + + Log::Comment(NoThrowString().Format( + L"Writing NULs in between other strings")); + stateMachine.ProcessString(L"\0foo\0bar\0", 9); + VERIFY_ARE_EQUAL(6, cursor.GetPosition().X); + VERIFY_ARE_EQUAL(1, cursor.GetPosition().Y); +} + void ScreenBufferTests::VtResize() { // Run this test in isolation - for one reason or another, this breaks other tests. diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index be3e1b0d604..66daa8fd23d 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -42,6 +42,14 @@ ITermDispatch& OutputStateMachineEngine::Dispatch() noexcept // - true iff we successfully dispatched the sequence. bool OutputStateMachineEngine::ActionExecute(const wchar_t wch) { + // microsoft/terminal#1825 - VT applications expect to be able to write NUL + // and have _nothing_ happen. Filter the NULs here, so they don't fill the + // buffer with empty spaces. + if (wch == AsciiChars::NUL) + { + return true; + } + _dispatch->Execute(wch); _ClearLastChar(); From 8b3146080950417e34819923274b9e20788b2dff Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 4 Oct 2019 10:47:39 -0500 Subject: [PATCH 09/11] Return to ground when we flush the last char (#2823) The InputStateMachineEngine was incorrectly not returning to the ground state after flushing the last sequence. That means that something like alt+backspace would leave us in the Escape state, not the ground state. This makes sure we return to ground. Additionally removes the "Parser.UnitTests-common.vcxproj" file, which was originally used for a theoretical time when we only open-sourced the parser. It's unnecessary now, and we can get rid of it. Also includes a small patch to bcz.cmd, to make sure bx works with projects with a space in their name. (cherry picked from commit 53c81a08f88fc3b34f7a492d3da46b3a3651f80d) --- src/terminal/parser/stateMachine.cpp | 39 +++++++++++--- .../parser/ut_parser/InputEngineTest.cpp | 52 +++++++++++++++++++ .../ut_parser/Parser.UnitTests-common.vcxproj | 19 ------- .../parser/ut_parser/Parser.UnitTests.vcxproj | 15 +++++- 4 files changed, 98 insertions(+), 27 deletions(-) delete mode 100644 src/terminal/parser/ut_parser/Parser.UnitTests-common.vcxproj diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 99b49a58653..085db8cadf4 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -1399,6 +1399,25 @@ void StateMachine::ProcessString(const wchar_t* const rgwch, const size_t cch) } else if (s_fProcessIndividually) { + // One of the "weird things" in VT input is the case of something like + // alt+[. In VT, that's encoded as `\x1b[`. However, that's + // also the start of a CSI, and could be the start of a longer sequence, + // there's no way to know for sure. For an alt+[ keypress, + // the parser originally would just sit in the `CsiEntry` state after + // processing it, which would pollute the following keypress (e.g. + // alt+[, A would be processed like `\x1b[A`, + // which is _wrong_). + // + // Fortunately, for VT input, each keystroke comes in as an individual + // write operation. So, if at the end of processing a string for the + // InputEngine, we find that we're not in the Ground state, that implies + // that we've processed some input, but not dispatched it yet. This + // block at the end of `ProcessString` will then re-process the + // undispatched string, but it will ensure that it dispatches on the + // last character of the string. For our previous `\x1b[` scenario, that + // means we'll make sure to call `_ActionEscDispatch('[')`., which will + // properly decode the string as alt+[. + if (_pEngine->FlushAtEndOfString()) { // Reset our state, and put all but the last char in again. @@ -1413,25 +1432,31 @@ void StateMachine::ProcessString(const wchar_t* const rgwch, const size_t cch) switch (_state) { case VTStates::Ground: - return _ActionExecute(*pwch); + _ActionExecute(*pwch); + break; case VTStates::Escape: case VTStates::EscapeIntermediate: - return _ActionEscDispatch(*pwch); + _ActionEscDispatch(*pwch); + break; case VTStates::CsiEntry: case VTStates::CsiIntermediate: case VTStates::CsiIgnore: case VTStates::CsiParam: - return _ActionCsiDispatch(*pwch); + _ActionCsiDispatch(*pwch); + break; case VTStates::OscParam: case VTStates::OscString: case VTStates::OscTermination: - return _ActionOscDispatch(*pwch); + _ActionOscDispatch(*pwch); + break; case VTStates::Ss3Entry: case VTStates::Ss3Param: - return _ActionSs3Dispatch(*pwch); - default: - return; + _ActionSs3Dispatch(*pwch); + break; } + // microsoft/terminal#2746: Make sure to return to the ground state + // after dispatching the characters + _EnterGround(); } } } diff --git a/src/terminal/parser/ut_parser/InputEngineTest.cpp b/src/terminal/parser/ut_parser/InputEngineTest.cpp index fcb67d59a1e..27fbd25cb3e 100644 --- a/src/terminal/parser/ut_parser/InputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/InputEngineTest.cpp @@ -230,6 +230,7 @@ class Microsoft::Console::VirtualTerminal::InputEngineTest TEST_METHOD(AltBackspaceTest); TEST_METHOD(AltCtrlDTest); TEST_METHOD(AltIntermediateTest); + TEST_METHOD(AltBackspaceEnterTest); friend class TestInteractDispatch; }; @@ -826,3 +827,54 @@ void InputEngineTest::AltIntermediateTest() Log::Comment(NoThrowString().Format(L"Processing \"\\x05\"")); stateMachine->ProcessString(seq); } + +void InputEngineTest::AltBackspaceEnterTest() +{ + // Created as a test for microsoft/terminal#2746. See that issue for mode + // details. We're going to send an Alt+Backspace to conpty, followed by an + // enter. The enter should be processed as just a single VK_ENTER, not a + // alt+enter. + + TestState testState; + auto pfn = std::bind(&TestState::TestInputCallback, &testState, std::placeholders::_1); + + auto inputEngine = std::make_unique(new TestInteractDispatch(pfn, &testState)); + auto _stateMachine = std::make_unique(inputEngine.release()); + VERIFY_IS_NOT_NULL(_stateMachine); + testState._stateMachine = _stateMachine.get(); + + INPUT_RECORD inputRec; + + inputRec.EventType = KEY_EVENT; + inputRec.Event.KeyEvent.bKeyDown = TRUE; + inputRec.Event.KeyEvent.dwControlKeyState = LEFT_ALT_PRESSED; + inputRec.Event.KeyEvent.wRepeatCount = 1; + inputRec.Event.KeyEvent.wVirtualKeyCode = VK_BACK; + inputRec.Event.KeyEvent.wVirtualScanCode = static_cast(MapVirtualKeyW(VK_BACK, MAPVK_VK_TO_VSC)); + inputRec.Event.KeyEvent.uChar.UnicodeChar = L'\x08'; + + // First, expect a alt+backspace. + testState.vExpectedInput.push_back(inputRec); + + std::wstring seq = L"\x1b\x7f"; + Log::Comment(NoThrowString().Format(L"Processing \"\\x1b\\x7f\"")); + _stateMachine->ProcessString(seq); + + // Ensure the state machine has correctly returned to the ground state + VERIFY_ARE_EQUAL(StateMachine::VTStates::Ground, _stateMachine->_state); + + inputRec.Event.KeyEvent.wVirtualKeyCode = VK_RETURN; + inputRec.Event.KeyEvent.dwControlKeyState = 0; + inputRec.Event.KeyEvent.wVirtualScanCode = static_cast(MapVirtualKeyW(VK_RETURN, MAPVK_VK_TO_VSC)); + inputRec.Event.KeyEvent.uChar.UnicodeChar = L'\x0d'; //maybe \xa + + // Then, expect a enter + testState.vExpectedInput.push_back(inputRec); + + seq = L"\x0d"; + Log::Comment(NoThrowString().Format(L"Processing \"\\x0d\"")); + _stateMachine->ProcessString(seq); + + // Ensure the state machine has correctly returned to the ground state + VERIFY_ARE_EQUAL(StateMachine::VTStates::Ground, _stateMachine->_state); +} diff --git a/src/terminal/parser/ut_parser/Parser.UnitTests-common.vcxproj b/src/terminal/parser/ut_parser/Parser.UnitTests-common.vcxproj deleted file mode 100644 index 24754a9dd5b..00000000000 --- a/src/terminal/parser/ut_parser/Parser.UnitTests-common.vcxproj +++ /dev/null @@ -1,19 +0,0 @@ - - - - - - Create - - - - - - - - - - ..;%(AdditionalIncludeDirectories) - - - diff --git a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj index 57daef497c5..dd7acda4668 100644 --- a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj +++ b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj @@ -2,14 +2,27 @@ - + + + + + + + Create + + + + ..;%(AdditionalIncludeDirectories) + + + {18d09a24-8240-42d6-8cb6-236eee820263} From e7f8d31fc88cc50fb5a0060955e39f09f69e3340 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 4 Oct 2019 15:53:54 -0500 Subject: [PATCH 10/11] =?UTF-8?q?Add=20support=20for=20passing=20through?= =?UTF-8?q?=20extended=20text=20attributes,=20like=E2=80=A6=20(#2917)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Adds support for Italics, Blinking, Invisible, CrossedOut text, THROUGH CONPTY. This does **NOT** add support for those styles to conhost or the terminal. We will store these "Extended Text Attributes" in a `TextAttribute`. When we go to render a line, we'll see if the state has changed from our previous state, and if so, we'll appropriately toggle that state with VT. Boldness has been moved from a `bool` to a single bit in these flags. Technically, now that these are stored in the buffer, we only need to make changes to the renderers to be able to support them. That's not being done as a part of this PR however. ## References See also #2915 and #2916, which are some follow-up tasks from this fix. I thought them too risky for 20H1. ## PR Checklist * [x] Closes #2554 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated
* store text with extended attributes too * Plumb attributes through all the renderers * parse extended attrs, though we're not renderering them right * Render these states correctly * Add a very extensive test * Cleanup for PR * a block of PR feedback * add 512 test cases * Fix the build * Fix @carlos-zamora's suggestions * @miniksa's PR feedback (cherry picked from commit dec5c11e198e57c98ace463817a2cc2731d99a15) --- src/buffer/out/TextAttribute.cpp | 14 +- src/buffer/out/TextAttribute.hpp | 36 +- .../TerminalCore/TerminalDispatchGraphics.cpp | 4 +- src/host/getset.cpp | 33 ++ src/host/getset.h | 3 + src/host/outputStream.cpp | 26 ++ src/host/outputStream.hpp | 2 + src/host/ut_host/ScreenBufferTests.cpp | 333 ++++++++++++++++++ src/host/ut_host/VtRendererTests.cpp | 142 ++++++-- src/inc/conattrs.hpp | 15 + src/interactivity/onecore/BgfxEngine.cpp | 2 +- src/interactivity/onecore/BgfxEngine.hpp | 2 +- src/renderer/base/renderer.cpp | 4 +- src/renderer/dx/DxRenderer.cpp | 4 +- src/renderer/dx/DxRenderer.hpp | 2 +- src/renderer/gdi/gdirenderer.hpp | 2 +- src/renderer/gdi/state.cpp | 4 +- src/renderer/inc/IRenderEngine.hpp | 2 +- src/renderer/vt/VtSequences.cpp | 88 +++++ src/renderer/vt/WinTelnetEngine.cpp | 9 +- src/renderer/vt/WinTelnetEngine.hpp | 2 +- src/renderer/vt/Xterm256Engine.cpp | 67 +++- src/renderer/vt/Xterm256Engine.hpp | 8 +- src/renderer/vt/XtermEngine.cpp | 11 +- src/renderer/vt/XtermEngine.hpp | 2 +- src/renderer/vt/vtrenderer.hpp | 15 +- src/renderer/wddmcon/WddmConRenderer.cpp | 2 +- src/renderer/wddmcon/WddmConRenderer.hpp | 2 +- src/terminal/adapter/DispatchTypes.hpp | 21 +- src/terminal/adapter/adaptDispatch.cpp | 20 +- src/terminal/adapter/adaptDispatch.hpp | 2 + .../adapter/adaptDispatchGraphics.cpp | 132 ++++++- src/terminal/adapter/conGetSet.hpp | 2 + .../adapter/ut_adapter/adapterTest.cpp | 22 +- src/types/inc/utils.hpp | 13 + 35 files changed, 937 insertions(+), 111 deletions(-) diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp index 0c964b848e9..fb498d25112 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,11 +155,6 @@ void TextAttribute::SetColor(const COLORREF rgbColor, const bool fIsForeground) } } -bool TextAttribute::IsBold() const noexcept -{ - return _isBold; -} - bool TextAttribute::_IsReverseVideo() const noexcept { return WI_IsFlagSet(_wAttrLegacy, COMMON_LVB_REVERSE_VIDEO); @@ -215,6 +210,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 @@ -224,7 +224,7 @@ void TextAttribute::Invert() noexcept void TextAttribute::_SetBoldness(const bool isBold) noexcept { - _isBold = isBold; + WI_UpdateFlag(_extendedAttrs, ExtendedAttributes::Bold, isBold); } void TextAttribute::SetDefaultForeground() noexcept diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index eb8cc6111df..213c908e8f2 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -27,6 +27,8 @@ Revision History: #include "WexTestClass.h" #endif +#pragma pack(push, 1) + class TextAttribute final { public: @@ -34,7 +36,7 @@ class TextAttribute final _wAttrLegacy{ 0 }, _foreground{}, _background{}, - _isBold{ false } + _extendedAttrs{ ExtendedAttributes::Normal } { } @@ -42,7 +44,7 @@ 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. WI_ClearAllFlags(_wAttrLegacy, COMMON_LVB_SBCSDBCS); @@ -53,7 +55,7 @@ class TextAttribute final _wAttrLegacy{ 0 }, _foreground{ rgbForeground }, _background{ rgbBackground }, - _isBold{ false } + _extendedAttrs{ ExtendedAttributes::Normal } { } @@ -62,7 +64,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 +87,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 +133,18 @@ 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 WI_IsFlagSet(_extendedAttrs, ExtendedAttributes::Bold); + } + + constexpr ExtendedAttributes GetExtendedAttributes() const noexcept + { + return _extendedAttrs; + } + + void SetExtendedAttributes(const ExtendedAttributes attrs) noexcept; void SetForeground(const COLORREF rgbForeground) noexcept; void SetBackground(const COLORREF rgbBackground) noexcept; @@ -159,7 +172,7 @@ class TextAttribute final WORD _wAttrLegacy; TextColor _foreground; TextColor _background; - bool _isBold; + ExtendedAttributes _extendedAttrs; #ifdef UNIT_TESTING friend class TextBufferTests; @@ -169,6 +182,13 @@ class TextAttribute final #endif }; +#pragma pack(pop) +// 2 for _wAttrLegacy +// 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"); + enum class TextAttributeBehavior { Stored, // use contained text attribute @@ -181,7 +201,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 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 77fb4d9c417..6d979633b56 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -981,6 +981,39 @@ void DoSrvPrivateBoldText(SCREEN_INFORMATION& screenInfo, const bool bolded) buffer.SetAttributes(attrs); } +// Method Description: +// - 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: +// - the currently active ExtendedAttributes. +ExtendedAttributes DoSrvPrivateGetExtendedTextAttributes(SCREEN_INFORMATION& screenInfo) +{ + auto& buffer = screenInfo.GetActiveBuffer(); + auto attrs = buffer.GetAttributes(); + return attrs.GetExtendedAttributes(); +} + +// Method Description: +// - 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 +// Return Value: +// - +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..f45fc8b1deb 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -273,6 +273,32 @@ 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); + 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/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index 7abe51fe8fc..3281c08543c 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -187,6 +187,9 @@ class ScreenBufferTests TEST_METHOD(InitializeTabStopsInVTMode); + TEST_METHOD(TestExtendedTextAttributes); + TEST_METHOD(TestExtendedTextAttributesWithColors); + TEST_METHOD(CursorUpDownAcrossMargins); TEST_METHOD(CursorUpDownOutsideMargins); TEST_METHOD(CursorUpDownExactlyAtMargins); @@ -4546,6 +4549,336 @@ 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. + + // 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}") + 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"); +} + +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"); +} + void ScreenBufferTests::CursorUpDownAcrossMargins() { // Test inspired by: https://github.com/microsoft/terminal/issues/2929 diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 0fc75e6b3c6..437338cf546 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -390,25 +390,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 }); @@ -420,41 +436,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 }); } @@ -725,41 +769,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 }); } @@ -961,40 +1029,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..a168b173cb9 100644 --- a/src/inc/conattrs.hpp +++ b/src/inc/conattrs.hpp @@ -8,6 +8,21 @@ 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, + // 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. +}; +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..4c11cdfc533 100644 --- a/src/renderer/dx/DxRenderer.cpp +++ b/src/renderer/dx/DxRenderer.cpp @@ -1218,14 +1218,14 @@ 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. [[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..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: @@ -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/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/WinTelnetEngine.cpp b/src/renderer/vt/WinTelnetEngine.cpp index 614c66a3be6..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: @@ -36,10 +37,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..af8738a5650 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -14,7 +14,8 @@ 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), + _lastExtendedAttrsState{ ExtendedAttributes::Normal } { } @@ -26,6 +27,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: @@ -33,7 +35,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 @@ -42,11 +44,70 @@ 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, colorBackground, - isBold, + 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 +{ + // Helper lambda to check if a state (attr) has changed since it's last + // value (lastState), and appropriately start/end that state with the given + // begin/end functions. + auto updateFlagAndState = [extendedAttrs, this](const ExtendedAttributes attr, + std::function beginFn, + std::function endFn) -> HRESULT { + const bool flagSet = WI_AreAllFlagsSet(extendedAttrs, attr); + const bool lastState = WI_AreAllFlagsSet(_lastExtendedAttrsState, attr); + if (flagSet != lastState) + { + if (flagSet) + { + RETURN_IF_FAILED(beginFn(this)); + } + else + { + RETURN_IF_FAILED(endFn(this)); + } + WI_SetAllFlags(_lastExtendedAttrsState, attr); + } + return S_OK; + }; + + auto hr = updateFlagAndState(ExtendedAttributes::Italics, + &Xterm256Engine::_BeginItalics, + &Xterm256Engine::_EndItalics); + RETURN_IF_FAILED(hr); + + hr = updateFlagAndState(ExtendedAttributes::Blinking, + &Xterm256Engine::_BeginBlink, + &Xterm256Engine::_EndBlink); + RETURN_IF_FAILED(hr); + + hr = updateFlagAndState(ExtendedAttributes::Invisible, + &Xterm256Engine::_BeginInvisible, + &Xterm256Engine::_EndInvisible); + RETURN_IF_FAILED(hr); + + hr = updateFlagAndState(ExtendedAttributes::CrossedOut, + &Xterm256Engine::_BeginCrossedOut, + &Xterm256Engine::_EndCrossedOut); + RETURN_IF_FAILED(hr); + + return S_OK; +} diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 7ad288b3426..729ed12434c 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -35,10 +35,16 @@ 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: + [[nodiscard]] HRESULT _UpdateExtendedAttrs(const ExtendedAttributes extendedAttrs) noexcept; + + // 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; #endif diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 2e56da3864d..b48dab303af 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -172,6 +172,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: @@ -179,7 +180,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 @@ -188,10 +189,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, 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 67992a9abe0..2a482501d85 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -45,7 +45,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 8a419a7e9c6..d2c7804a9fa 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; @@ -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/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; diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index e76349ce0c0..17f26c66d85 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -13,21 +13,28 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes Scrollback = 3 }; + // TODO:GH#2916 add support for DoublyUnderlined, Faint(2) to the adapter as well. enum GraphicsOptions : unsigned int { Off = 0, BoldBright = 1, - RGBColor = 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, - Xterm256Index = 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, + DoublyUnderlined = 21, UnBold = 22, + NotItalics = 23, NoUnderline = 24, - Positive = 27, + Steady = 25, // _not_ blink + Positive = 27, // _not_ inverse + Visible = 28, // _not_ invisible + NotCrossedOut = 29, ForegroundBlack = 30, ForegroundRed = 31, ForegroundGreen = 32, 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/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..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 @@ -36,7 +37,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) @@ -62,7 +65,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 @@ -83,6 +88,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; @@ -261,6 +267,30 @@ void AdaptDispatch::_SetGraphicsOptionHelper(const DispatchTypes::GraphicsOption } } +// Routine Description: +// 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 text attribute, false otherwise. +bool AdaptDispatch::s_IsExtendedTextAttribute(const DispatchTypes::GraphicsOptions opt) noexcept +{ + // TODO:GH#2916 add support for DoublyUnderlined, Faint(RGBColorOrFaint). + // These two are currently partially implemented as other things: + // * 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 || + 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 +368,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 +380,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 @@ -374,31 +404,92 @@ 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; } +// 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 }; + + RETURN_BOOL_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#2916 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. +// - 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); @@ -416,6 +507,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; @@ -424,14 +519,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) 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..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 { @@ -2630,7 +2642,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 +2652,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 +2662,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 +2671,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 +2683,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; 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; From 361e773326443799d7a2f0c214e344e4facebe04 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Wed, 9 Oct 2019 11:01:15 -0700 Subject: [PATCH 11/11] Move the VT parser's parse state into instanced storage (#3110) The VT parser used to be keeping a boolean used to determine whether it was in bulk or single-character parse mode in a function-level static. That turned out to not be great. Fixes #3108; fixes #3073. (cherry picked from commit dd2fbef39dd477a92d0fda9839532c27159a99c6) --- src/terminal/parser/stateMachine.cpp | 19 ++- src/terminal/parser/stateMachine.hpp | 4 + .../parser/ut_parser/Parser.UnitTests.vcxproj | 1 + .../Parser.UnitTests.vcxproj.filters | 8 +- .../parser/ut_parser/StateMachineTest.cpp | 113 ++++++++++++++++++ src/terminal/parser/ut_parser/sources | 1 + 6 files changed, 134 insertions(+), 12 deletions(-) create mode 100644 src/terminal/parser/ut_parser/StateMachineTest.cpp diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 085db8cadf4..0a656342b7f 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -25,7 +25,8 @@ StateMachine::StateMachine(IStateMachineEngine* const pEngine) : // rgusParams Initialized below _sOscNextChar(0), _sOscParam(0), - _currRunLength(0) + _currRunLength(0), + _fProcessingIndividually(false) { ZeroMemory(_pwchOscStringBuffer, sizeof(_pwchOscStringBuffer)); ZeroMemory(_rgusParams, sizeof(_rgusParams)); @@ -1346,20 +1347,16 @@ void StateMachine::ProcessString(const wchar_t* const rgwch, const size_t cch) _pwchSequenceStart = rgwch; _currRunLength = 0; - // This should be static, because if one string starts a sequence, and the next finishes it, - // we want the partial sequence state to persist. - static bool s_fProcessIndividually = false; - for (size_t cchCharsRemaining = cch; cchCharsRemaining > 0; cchCharsRemaining--) { - if (s_fProcessIndividually) + if (_fProcessingIndividually) { // If we're processing characters individually, send it to the state machine. ProcessCharacter(*_pwchCurr); _pwchCurr++; if (_state == VTStates::Ground) // Then check if we're back at ground. If we are, the next character (pwchCurr) { // is the start of the next run of characters that might be printable. - s_fProcessIndividually = false; + _fProcessingIndividually = false; _pwchSequenceStart = _pwchCurr; _currRunLength = 0; } @@ -1371,13 +1368,13 @@ void StateMachine::ProcessString(const wchar_t* const rgwch, const size_t cch) FAIL_FAST_IF(!(_pwchSequenceStart + _currRunLength <= rgwch + cch)); _pEngine->ActionPrintString(_pwchSequenceStart, _currRunLength); // ... print all the chars leading up to it as part of the run... _trace.DispatchPrintRunTrace(_pwchSequenceStart, _currRunLength); - s_fProcessIndividually = true; // begin processing future characters individually... + _fProcessingIndividually = true; // begin processing future characters individually... _currRunLength = 0; _pwchSequenceStart = _pwchCurr; ProcessCharacter(*_pwchCurr); // ... Then process the character individually. if (_state == VTStates::Ground) // If the character took us right back to ground, start another run after it. { - s_fProcessIndividually = false; + _fProcessingIndividually = false; _pwchSequenceStart = _pwchCurr + 1; _currRunLength = 0; } @@ -1391,13 +1388,13 @@ void StateMachine::ProcessString(const wchar_t* const rgwch, const size_t cch) } // If we're at the end of the string and have remaining un-printed characters, - if (!s_fProcessIndividually && _currRunLength > 0) + if (!_fProcessingIndividually && _currRunLength > 0) { // print the rest of the characters in the string _pEngine->ActionPrintString(_pwchSequenceStart, _currRunLength); _trace.DispatchPrintRunTrace(_pwchSequenceStart, _currRunLength); } - else if (s_fProcessIndividually) + else if (_fProcessingIndividually) { // One of the "weird things" in VT input is the case of something like // alt+[. In VT, that's encoded as `\x1b[`. However, that's diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index ce6980191e5..a4e71a69bd9 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -150,5 +150,9 @@ namespace Microsoft::Console::VirtualTerminal const wchar_t* _pwchCurr; const wchar_t* _pwchSequenceStart; size_t _currRunLength; + + // This is tracked per state machine instance so that separate calls to Process* + // can start and finish a sequence. + bool _fProcessingIndividually; }; } diff --git a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj index dd7acda4668..65773c0f48c 100644 --- a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj +++ b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj @@ -12,6 +12,7 @@ + Create diff --git a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj.filters b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj.filters index 6aeb1ba04c5..ecc61100147 100644 --- a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj.filters +++ b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj.filters @@ -15,7 +15,13 @@ - + + Source Files + + + Source Files + + Source Files diff --git a/src/terminal/parser/ut_parser/StateMachineTest.cpp b/src/terminal/parser/ut_parser/StateMachineTest.cpp new file mode 100644 index 00000000000..7235b1ecce0 --- /dev/null +++ b/src/terminal/parser/ut_parser/StateMachineTest.cpp @@ -0,0 +1,113 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" +#include "WexTestClass.h" +#include "../../inc/consoletaeftemplates.hpp" + +#include "stateMachine.hpp" + +using namespace WEX::Common; +using namespace WEX::Logging; +using namespace WEX::TestExecution; + +namespace Microsoft +{ + namespace Console + { + namespace VirtualTerminal + { + class StateMachineTest; + class TestStateMachineEngine; + }; + }; +}; + +using namespace Microsoft::Console::VirtualTerminal; + +class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStateMachineEngine +{ +public: + bool ActionExecute(const wchar_t /* wch */) override { return true; }; + bool ActionExecuteFromEscape(const wchar_t /* wch */) override { return true; }; + bool ActionPrint(const wchar_t /* wch */) override { return true; }; + bool ActionPrintString(const wchar_t* const /* rgwch */, + size_t const /* cch */) override { return true; }; + + bool ActionPassThroughString(const wchar_t* const /* rgwch */, + size_t const /* cch */) override { return true; }; + + bool ActionEscDispatch(const wchar_t /* wch */, + const unsigned short /* cIntermediate */, + const wchar_t /* wchIntermediate */) override { return true; }; + + bool ActionClear() override { return true; }; + + bool ActionIgnore() override { return true; }; + + bool ActionOscDispatch(const wchar_t /* wch */, + const unsigned short /* sOscParam */, + wchar_t* const /* pwchOscStringBuffer */, + const unsigned short /* cchOscString */) override { return true; }; + + bool ActionSs3Dispatch(const wchar_t /* wch */, + const unsigned short* const /* rgusParams */, + const unsigned short /* cParams */) override { return true; }; + + bool FlushAtEndOfString() const override { return false; }; + bool DispatchControlCharsFromEscape() const override { return false; }; + bool DispatchIntermediatesFromEscape() const override { return false; }; + + // ActionCsiDispatch is the only method that's actually implemented. + bool ActionCsiDispatch(const wchar_t /* wch */, + const unsigned short /* cIntermediate */, + const wchar_t /* wchIntermediate */, + _In_reads_(cParams) const unsigned short* const rgusParams, + const unsigned short cParams) override + { + csiParams.emplace(rgusParams, rgusParams + cParams); + return true; + } + + // This will only be populated if ActionCsiDispatch is called. + std::optional> csiParams; +}; + +class Microsoft::Console::VirtualTerminal::StateMachineTest +{ + TEST_CLASS(StateMachineTest); + + TEST_CLASS_SETUP(ClassSetup) + { + return true; + } + + TEST_CLASS_CLEANUP(ClassCleanup) + { + return true; + } + + TEST_METHOD(TwoStateMachinesDoNotInterfereWithEachother); +}; + +void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother() +{ + auto firstEnginePtr{ std::make_unique() }; + // this dance is required because StateMachine presumes to take ownership of its engine. + const auto& firstEngine{ *firstEnginePtr.get() }; + StateMachine firstStateMachine{ firstEnginePtr.release() }; + + auto secondEnginePtr{ std::make_unique() }; + const auto& secondEngine{ *secondEnginePtr.get() }; + StateMachine secondStateMachine{ secondEnginePtr.release() }; + + firstStateMachine.ProcessString(L"\x1b[12"); // partial sequence + secondStateMachine.ProcessString(L"\x1b[3C"); // full sequence on second parser + firstStateMachine.ProcessString(L";34m"); // completion to previous partial sequence on first parser + + std::vector expectedFirstCsi{ 12u, 34u }; + std::vector expectedSecondCsi{ 3u }; + + VERIFY_ARE_EQUAL(expectedFirstCsi, firstEngine.csiParams); + VERIFY_ARE_EQUAL(expectedSecondCsi, secondEngine.csiParams); +} diff --git a/src/terminal/parser/ut_parser/sources b/src/terminal/parser/ut_parser/sources index 32867cc0ff3..5e7f6c61e11 100644 --- a/src/terminal/parser/ut_parser/sources +++ b/src/terminal/parser/ut_parser/sources @@ -23,6 +23,7 @@ SOURCES = \ $(SOURCES) \ OutputEngineTest.cpp \ InputEngineTest.cpp \ + StateMachineTest.cpp \ # The InputEngineTest requires VTRedirMapVirtualKeyW, which means we need the # ServiceLocator, which means we need the entire host and all it's dependencies,