Repository: microsoft/terminal (language: cpp)

Issue:
Blinking cursor shows up in TUI programs (where it shouldn't) in 1.13
### Windows Terminal version

1.13.10336.0

### Windows build number

10.0.19044.1348

### Other Software

scoop install [vifm](https://vifm.info)

_and_

scoop install [ncspot](https://github.com/hrkfdn/ncspot)

### Steps to reproduce

Just launch the programs. See below.

![cursor](https://user-images.githubusercontent.com/46838874/152695110-7285ba2e-22dc-401b-a74d-5bd7a5553780.gif)

![cursor1c](https://user-images.githubusercontent.com/46838874/152696172-6c523a5d-3249-411e-81cf-0fdd34dfccce.gif)


### Expected Behavior

The cursor wasn't showing up (at all) in 1.12 Preview. It shouldn't appear.

### Actual Behavior

The cursor appears.


Possibly related - https://github.com/microsoft/terminal/issues/12372 ?


Discussion / hints:
Can someone else reproduce this issue? I can't replicate it with vifm 0.12. (ncspot requires a spotify account.)
Neither rendering engine reproduces the issue for me.
Maybe this issue is specific to Windows 10? That seems a bit unlikely though...
I should mention that I manually set TERM to xterm-256color (Neovim needs it to display titles). It was set with 1.12 too. 

(I can provide more details about my environment if required).
Demo on Windows 20H2 (10.0.19044.1466) and `vifm` with no customization:

https://user-images.githubusercontent.com/3933920/152703172-04e7f33f-0f47-4117-a14e-233866fc6209.mp4

@rashil2000 seems to be using a custom theme with `starship` prompt (judging by the look of it) that might affect his setup.

I don't think it has anything to do with Starship. My settings.json profile ([here](https://github.com/rashil2000/dotfiles/blob/master/wt/settings.json)) launches Vifm/Ncspot directly, so it doesn't go through any shell. Nevertheless, I disabled Starship completely to test it, and issue still persists.
Here's 1.13 and 1.12 running side-by-side:

![image](https://user-images.githubusercontent.com/46838874/152721027-f4ccbc05-cb7c-4807-ac62-57424f768cc8.png)

My vifmrc is [here](https://github.com/rashil2000/dotfiles/blob/master/vifm/vifmrc)
I figured it out. Here's how to reproduce the issue:
* Download vifm: https://github.com/vifm/vifm/releases/download/v0.12/vifm-w64-se-0.12-binary.zip
* Create a file named `vifmrc` inside the directory where `vifm.exe` is
* Add a single line: `highlight clear`
* Create a profile, which has `vifm.exe` as the command line

This issue doesn't occur when:
* Launched in Windows Terminal 1.12, or...
* Launched from cmd.exe or PowerShell
> Launched from cmd.exe or PowerShell

Wow, you're right. I didn't test it from Cmd or PowerShell. Even Ncspot does not display the cursor when launched from the shell.
I think I might know what the problem is here.

There are two variables in the XtermEngine that track the cursor visibility for conpty: `_lastCursorIsVisible` indicates the last state of the cursor; and `_nextCursorIsVisible` indicates the state for the current frame. At the end of the frame, if there is a difference between `_lastCursorIsVisible` and `_nextCursorIsVisible`, we know we need to send a sequence either to show or hide the cursor.

The problem is that `_lastCursorIsVisible` is initialized as false, so if the cursor is hidden before the first frame is rendered, then we'll reach the end of the frame with both `_lastCursorIsVisible` and `_nextCursorIsVisible` as false. And that makes us think there is no need to send a _hide cursor_ sequence, because we think the cursor is already hidden. This can easily be fixed by initializing `_lastCursorIsVisible` to true.

What I don't understand, is why this would ever have worked. Unless it's a matter of timing, and in previous versions at least one frame would have been painted *before* the application had a chance to hide the cursor.

The other concern is that maybe `_lastCursorIsVisible` was intentionally set to false to guarantee that we send at least one _show cursor_ sequence on startup, in case the client terminal started with a hidden cursor? If that's the case then the `_lastCursorIsVisible` variable may need a third "unknown" state to start with.
@j4james You know what's funny? I was about to fix this issue and couldn't for my life replicate it anymore. Only 1 out of maybe 30 tabs of vifm show the cursor issue for me now - it's entirely random, proving that you're right. It's interesting this ever worked.
FYI, I found that some of the time it might look like it was working at first - i.e. the cursor appeared to be hidden - but that was just because it was positioned off screen. The minute you did something that updated the status bar, it would end up moving the cursor somewhere that would make it visible.

Fix patch:
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..6a1622f5ea1 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<void()> 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);
@@ -1527,8 +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, [&]() {
-        VERIFY_IS_FALSE(engine->_lastCursorIsVisible);
         VERIFY_IS_FALSE(engine->_nextCursorIsVisible);
         VERIFY_IS_FALSE(engine->_needToDisableCursor);
 
@@ -1539,10 +1496,13 @@ 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");
     });
 
-    VERIFY_IS_TRUE(engine->_lastCursorIsVisible);
     VERIFY_IS_TRUE(engine->_nextCursorIsVisible);
     VERIFY_IS_FALSE(engine->_needToDisableCursor);
 
@@ -1550,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);
 
@@ -1561,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);
 
@@ -1569,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);
 
@@ -1580,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);
 
@@ -1590,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);
 
@@ -1598,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 4b3807df270..cb8004cfd0f 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
@@ -98,31 +100,20 @@ 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 = 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<Tribool>(_nextCursorIsVisible))
     {
-        RETURN_IF_FAILED(_ShowCursor());
+        RETURN_IF_FAILED(_nextCursorIsVisible ? _ShowCursor() : _HideCursor());
+        _lastCursorIsVisible = static_cast<Tribool>(_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..2e2b58affea 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 class Tribool : uint8_t
+        {
+            False = 0,
+            True,
+            Invalid,
+        };
+
         const bool _fUseAsciiOnly;
         bool _needToDisableCursor;
-        bool _lastCursorIsVisible;
+        Tribool _lastCursorIsVisible;
         bool _nextCursorIsVisible;
 
         [[nodiscard]] HRESULT _MoveCursor(const COORD coord) noexcept override;


Test patch: