From 653594bf6d036477028dd88d7e98cef4f28805ff Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 8 Feb 2022 19:43:51 +0100 Subject: [PATCH 1/2] XtermEngine: Explicitly emit cursor state on the first frame --- .github/actions/spelling/expect/expect.txt | 1 + src/host/ut_host/VtRendererTests.cpp | 118 +++++++-------------- src/renderer/vt/XtermEngine.cpp | 23 ++-- src/renderer/vt/XtermEngine.hpp | 11 +- 4 files changed, 58 insertions(+), 95 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index c34f03a2018..73edbe9825a 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -2444,6 +2444,7 @@ TREX triaged triaging TRIANGLESTRIP +Tribool TRIMZEROHEADINGS truetype trx diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 71f45d42500..7c4aa8ef891 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -90,7 +90,29 @@ class Microsoft::Console::Render::VtRendererTest void TestPaint(VtEngine& engine, std::function pfn); Viewport SetUpViewport(); - void VerifyExpectedInputsDrained(); + void VerifyFirstPaint(VtEngine& engine) + { + // Verify the first BeginPaint emits a clear and go home + qExpectedInput.push_back("\x1b[2J"); + // Verify the first EndPaint sets the cursor state + qExpectedInput.push_back("\x1b[?25l"); + VERIFY_IS_TRUE(engine._firstPaint); + TestPaint(engine, [&]() { + VERIFY_IS_FALSE(engine._firstPaint); + }); + } + + void VerifyExpectedInputsDrained() + { + if (!qExpectedInput.empty()) + { + for (const auto& exp : qExpectedInput) + { + Log::Error(NoThrowString().Format(L"EXPECTED INPUT NEVER RECEIVED: %hs", exp.c_str())); + } + VERIFY_FAIL(L"there should be no remaining un-drained expected input"); + } + } }; Viewport VtRendererTest::SetUpViewport() @@ -103,18 +125,6 @@ Viewport VtRendererTest::SetUpViewport() return Viewport::FromInclusive(view); } -void VtRendererTest::VerifyExpectedInputsDrained() -{ - if (!qExpectedInput.empty()) - { - for (const auto& exp : qExpectedInput) - { - Log::Error(NoThrowString().Format(L"EXPECTED INPUT NEVER RECEIVED: %hs", exp.c_str())); - } - VERIFY_FAIL(L"there should be no remaining un-drained expected input"); - } -} - bool VtRendererTest::WriteCallback(const char* const pch, size_t const cch) { std::string actualString = std::string(pch, cch); @@ -212,12 +222,7 @@ void VtRendererTest::Xterm256TestInvalidate() auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); - // Verify the first paint emits a clear and go home - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_TRUE(engine->_firstPaint); - TestPaint(*engine, [&]() { - VERIFY_IS_FALSE(engine->_firstPaint); - }); + VerifyFirstPaint(*engine); const Viewport view = SetUpViewport(); @@ -402,12 +407,7 @@ void VtRendererTest::Xterm256TestColors() RenderSettings renderSettings; RenderData renderData; - // Verify the first paint emits a clear and go home - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_TRUE(engine->_firstPaint); - TestPaint(*engine, [&]() { - VERIFY_IS_FALSE(engine->_firstPaint); - }); + VerifyFirstPaint(*engine); Viewport view = SetUpViewport(); @@ -585,12 +585,7 @@ void VtRendererTest::Xterm256TestCursor() auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); - // Verify the first paint emits a clear and go home - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_TRUE(engine->_firstPaint); - TestPaint(*engine, [&]() { - VERIFY_IS_FALSE(engine->_firstPaint); - }); + VerifyFirstPaint(*engine); Viewport view = SetUpViewport(); @@ -766,12 +761,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes() auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); - // Verify the first paint emits a clear and go home - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_TRUE(engine->_firstPaint); - TestPaint(*engine, [&]() { - VERIFY_IS_FALSE(engine->_firstPaint); - }); + VerifyFirstPaint(*engine); Viewport view = SetUpViewport(); @@ -907,12 +897,7 @@ void VtRendererTest::XtermTestInvalidate() auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); - // Verify the first paint emits a clear and go home - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_TRUE(engine->_firstPaint); - TestPaint(*engine, [&]() { - VERIFY_IS_FALSE(engine->_firstPaint); - }); + VerifyFirstPaint(*engine); Viewport view = SetUpViewport(); @@ -1096,12 +1081,7 @@ void VtRendererTest::XtermTestColors() RenderSettings renderSettings; RenderData renderData; - // Verify the first paint emits a clear and go home - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_TRUE(engine->_firstPaint); - TestPaint(*engine, [&]() { - VERIFY_IS_FALSE(engine->_firstPaint); - }); + VerifyFirstPaint(*engine); Viewport view = SetUpViewport(); @@ -1234,12 +1214,7 @@ void VtRendererTest::XtermTestCursor() auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); - // Verify the first paint emits a clear and go home - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_TRUE(engine->_firstPaint); - TestPaint(*engine, [&]() { - VERIFY_IS_FALSE(engine->_firstPaint); - }); + VerifyFirstPaint(*engine); Viewport view = SetUpViewport(); @@ -1412,12 +1387,7 @@ void VtRendererTest::TestWrapping() auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); - // Verify the first paint emits a clear and go home - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_TRUE(engine->_firstPaint); - TestPaint(*engine, [&]() { - VERIFY_IS_FALSE(engine->_firstPaint); - }); + VerifyFirstPaint(*engine); Viewport view = SetUpViewport(); @@ -1465,8 +1435,10 @@ void VtRendererTest::TestResize() auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); - // Verify the first paint emits a clear and go home + // Verify the first BeginPaint emits a clear and go home qExpectedInput.push_back("\x1b[2J"); + // Verify the first EndPaint sets the cursor state + qExpectedInput.push_back("\x1b[?25l"); VERIFY_IS_TRUE(engine->_firstPaint); VERIFY_IS_TRUE(engine->_suppressResizeRepaint); @@ -1502,21 +1474,6 @@ void VtRendererTest::TestCursorVisibility() 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); @@ -1528,7 +1485,8 @@ void VtRendererTest::TestCursorVisibility() // 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); + qExpectedInput.push_back("\x1b[2J"); + VERIFY_IS_FALSE(engine->_nextCursorIsVisible); VERIFY_IS_FALSE(engine->_needToDisableCursor); @@ -1539,6 +1497,10 @@ void VtRendererTest::TestCursorVisibility() VERIFY_IS_TRUE(engine->_nextCursorIsVisible); VERIFY_IS_TRUE(engine->_needToDisableCursor); + // GH#12401: + // The other tests verify that the cursor is explicitly hidden on the + // first frame (VerifyFirstPaint). This test on the other hand does + // the opposite by calling PaintCursor() during the first paint cycle. qExpectedInput.push_back("\x1b[?25h"); }); diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index 4b3807df270..c1c06953b2c 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -15,7 +15,9 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, VtEngine(std::move(hPipe), initialViewport), _fUseAsciiOnly(fUseAsciiOnly), _needToDisableCursor(false), - _lastCursorIsVisible(false), + // GH#12401: Ensure a DECTCEM cursor show/hide sequence + // is emitted on the first frame no matter what. + _lastCursorIsVisible(Tribool_Invalid), _nextCursorIsVisible(true) { // Set out initial cursor position to -1, -1. This will force our initial @@ -101,28 +103,17 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, if (_lastCursorIsVisible) { _buffer.insert(0, "\x1b[?25l"); - _lastCursorIsVisible = false; + _lastCursorIsVisible = Tribool_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) + if (_lastCursorIsVisible != static_cast(_nextCursorIsVisible)) { - RETURN_IF_FAILED(_ShowCursor()); + RETURN_IF_FAILED(_nextCursorIsVisible ? _ShowCursor() : _HideCursor()); + _lastCursorIsVisible = static_cast(_nextCursorIsVisible); } - // 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()); diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 1cda1782710..35d26c09220 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -54,9 +54,18 @@ namespace Microsoft::Console::Render [[nodiscard]] HRESULT WriteTerminalW(const std::wstring_view str) noexcept override; protected: + // I'm using a non-class enum here, so that the values + // are trivially convertible and comparable to bool. + enum Tribool : uint8_t + { + Tribool_False = 0, + Tribool_True, + Tribool_Invalid, + }; + const bool _fUseAsciiOnly; bool _needToDisableCursor; - bool _lastCursorIsVisible; + Tribool _lastCursorIsVisible; bool _nextCursorIsVisible; [[nodiscard]] HRESULT _MoveCursor(const COORD coord) noexcept override; From 2c3debc5cf5640453a7b37fb9466f776556f2a33 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 8 Feb 2022 21:33:09 +0100 Subject: [PATCH 2/2] Address feedback --- src/host/ut_host/VtRendererTests.cpp | 11 +---------- src/renderer/vt/XtermEngine.cpp | 6 +++--- src/renderer/vt/XtermEngine.hpp | 8 ++++---- 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 7c4aa8ef891..6a1622f5ea1 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -1484,9 +1484,8 @@ void VtRendererTest::TestCursorVisibility() // 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. + qExpectedInput.push_back("\x1b[2J"); TestPaint(*engine, [&]() { - qExpectedInput.push_back("\x1b[2J"); - VERIFY_IS_FALSE(engine->_nextCursorIsVisible); VERIFY_IS_FALSE(engine->_needToDisableCursor); @@ -1504,7 +1503,6 @@ void VtRendererTest::TestCursorVisibility() qExpectedInput.push_back("\x1b[?25h"); }); - VERIFY_IS_TRUE(engine->_lastCursorIsVisible); VERIFY_IS_TRUE(engine->_nextCursorIsVisible); VERIFY_IS_FALSE(engine->_needToDisableCursor); @@ -1512,7 +1510,6 @@ void VtRendererTest::TestCursorVisibility() // 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); @@ -1523,7 +1520,6 @@ void VtRendererTest::TestCursorVisibility() VERIFY_IS_FALSE(engine->_needToDisableCursor); }); - VERIFY_IS_TRUE(engine->_lastCursorIsVisible); VERIFY_IS_TRUE(engine->_nextCursorIsVisible); VERIFY_IS_FALSE(engine->_needToDisableCursor); @@ -1531,7 +1527,6 @@ void VtRendererTest::TestCursorVisibility() // 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); @@ -1542,7 +1537,6 @@ void VtRendererTest::TestCursorVisibility() VERIFY_SUCCEEDED(engine->PaintCursor(options)); - VERIFY_IS_TRUE(engine->_lastCursorIsVisible); VERIFY_IS_TRUE(engine->_nextCursorIsVisible); VERIFY_IS_TRUE(engine->_needToDisableCursor); @@ -1552,7 +1546,6 @@ void VtRendererTest::TestCursorVisibility() qExpectedInput.push_back("\x1b[?25h"); }); - VERIFY_IS_TRUE(engine->_lastCursorIsVisible); VERIFY_IS_TRUE(engine->_nextCursorIsVisible); VERIFY_IS_FALSE(engine->_needToDisableCursor); @@ -1560,14 +1553,12 @@ void VtRendererTest::TestCursorVisibility() // 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 c1c06953b2c..cb8004cfd0f 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -17,7 +17,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, _needToDisableCursor(false), // GH#12401: Ensure a DECTCEM cursor show/hide sequence // is emitted on the first frame no matter what. - _lastCursorIsVisible(Tribool_Invalid), + _lastCursorIsVisible(Tribool::Invalid), _nextCursorIsVisible(true) { // Set out initial cursor position to -1, -1. This will force our initial @@ -100,10 +100,10 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, { // If the cursor was previously visible, let's hide it for this frame, // by prepending a cursor off. - if (_lastCursorIsVisible) + if (_lastCursorIsVisible != Tribool::False) { _buffer.insert(0, "\x1b[?25l"); - _lastCursorIsVisible = Tribool_False; + _lastCursorIsVisible = Tribool::False; } // If the cursor was NOT previously visible, then that's fine! we don't // need to worry, it's already off. diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 35d26c09220..2e2b58affea 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -56,11 +56,11 @@ namespace Microsoft::Console::Render protected: // I'm using a non-class enum here, so that the values // are trivially convertible and comparable to bool. - enum Tribool : uint8_t + enum class Tribool : uint8_t { - Tribool_False = 0, - Tribool_True, - Tribool_Invalid, + False = 0, + True, + Invalid, }; const bool _fUseAsciiOnly;