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,