Repository: microsoft/terminal (language: cpp)

Issue:
Issues with cursor invalidation in GDI
### Windows Terminal version

Commit 378b6594bd94cf3b27f4e309a11efe25d83de0d9

### Windows build number

10.0.19045.4291

### Other Software

_No response_

### Steps to reproduce

1. Make sure the conhost render engine is set to GDI (i.e. the `UseDx` registry entry is 0).
2. Open a WSL shell in a recent build of OpenConsole (commit 20b0bed46df71c596b4fb68edfb6bb4b4ebf1c89 or later)
3. Execute the following statements:
   ```
   printf "\e[2 q"; sleep 2; printf "\e[A"; sleep 2; printf "\e[B"
   ```


### Expected Behavior

The above script is setting the cursor to a steady block so it's easy to see, sleeping for 2 seconds, moving up a line and sleeping for another 2 seconds, then moving the cursor back down again. You should be able to see the cursor the entire time.

### Actual Behavior

Initially the cursor is visible, but as soon as we move up a line it becomes invisible for 2 seconds.

I believe this is a regression from PR #15500. Originally we would call `InvalidateCursor` (via `TriggerRedrawCursor`) whenever the cursor was moved, both to invalidate the old position and the new position. But since PR #15500, `TriggerRedrawCursor` has been nuked, and `InvalidateCursor` is now called at the start of every frame (regardless of whether the cursor is moved or not), and it only invalidates the old cursor position. So when the cursor moves, it's erased from the old position, but not redrawn in the new position.

This typically only affects vertical movement (other than some edge cases), because at some point I think we stopped caring about the horizontal extent of the invalidated region and just refresh the full width of the screen regardless. This also doesn't seem to affect the DX and Atlas engines, but I have no idea why.



Discussion / hints:
Hi I'm an AI powered bot that finds similar issues based off the issue title.

Please view the issues below to see if they solve your problem, and if the issue describes your problem please consider closing this one and thumbs upping the other issue to help us prioritize it. Thank you!

### Open similar issues:

- [Flickering in conhost when scrolling top part of screen (#11274)](https://github.com/microsoft/terminal/issues/11274),  similarity score: 0.76

### Closed similar issues:
- [More cursor droppings (#8213)](https://github.com/microsoft/terminal/issues/8213),  similarity score: 0.80
- [Cursor droppings are back in conhost (#12739)](https://github.com/microsoft/terminal/issues/12739),  similarity score: 0.77
- [Weird scrolling regression in the GDI renderer (#13270)](https://github.com/microsoft/terminal/issues/13270),  similarity score: 0.76

> Note: You can give me feedback by thumbs upping or thumbs downing this comment.
I love you github-actions bot, and I hope you'll spare my life when the robot revolution comes, but I feel a little bit hurt when you suggest a bunch of my own issues back to me, like idiot human can't remember that he's already raised this issue before. Although to be fair, that's not an unreasonable assumption.
As to how we fix this issue, I can get it to work again by replicating most of the following code a couple of lines down:
https://github.com/microsoft/terminal/blob/378b6594bd94cf3b27f4e309a11efe25d83de0d9/src/renderer/base/renderer.cpp#L124-L160

That is, after we've reinitialized `_currentCursorOptions` with the new position, we need to invalidate that new position as well (I'm not sure about the `_compositionCache`, so I just ignored that bit). I don't know if this is a good solution, though.

Fix patch:
diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp
index 82b1eaea1b4..91e5772006e 100644
--- a/src/renderer/base/renderer.cpp
+++ b/src/renderer/base/renderer.cpp
@@ -121,96 +121,14 @@ IRenderData* Renderer::GetRenderData() const noexcept
         // Last chance check if anything scrolled without an explicit invalidate notification since the last frame.
         _CheckViewportAndScroll();
 
-        if (_currentCursorOptions)
-        {
-            const auto& buffer = _pData->GetTextBuffer();
-            const auto view = buffer.GetSize();
-            const auto coord = _currentCursorOptions->coordCursor;
-
-            // If we had previously drawn a composition at the previous cursor position
-            // we need to invalidate the entire line because who knows what changed.
-            // (It's possible to figure that out, but not worth the effort right now.)
-            if (_compositionCache)
-            {
-                til::rect rect{ 0, coord.y, til::CoordTypeMax, coord.y + 1 };
-                if (view.TrimToViewport(&rect))
-                {
-                    FOREACH_ENGINE(pEngine)
-                    {
-                        LOG_IF_FAILED(pEngine->Invalidate(&rect));
-                    }
-                }
-            }
-            else
-            {
-                const auto lineRendition = buffer.GetLineRendition(coord.y);
-                const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1;
-
-                til::rect rect{ coord.x, coord.y, coord.x + cursorWidth, coord.y + 1 };
-                rect = BufferToScreenLine(rect, lineRendition);
+        _invalidateCurrentCursor(); // Invalidate the previous cursor position.
+        _invalidateOldComposition();
 
-                if (view.TrimToViewport(&rect))
-                {
-                    FOREACH_ENGINE(pEngine)
-                    {
-                        LOG_IF_FAILED(pEngine->InvalidateCursor(&rect));
-                    }
-                }
-            }
-        }
-
-        _currentCursorOptions = _GetCursorInfo();
+        _updateCursorInfo();
         _compositionCache.reset();
 
-        // Invalidate the line that the active TSF composition is on,
-        // so that _PaintBufferOutput() actually gets a chance to draw it.
-        if (!_pData->activeComposition.text.empty())
-        {
-            const auto viewport = _pData->GetViewport();
-            const auto coordCursor = _pData->GetCursorPosition();
-
-            til::rect line{ 0, coordCursor.y, til::CoordTypeMax, coordCursor.y + 1 };
-            if (viewport.TrimToViewport(&line))
-            {
-                viewport.ConvertToOrigin(&line);
-
-                FOREACH_ENGINE(pEngine)
-                {
-                    LOG_IF_FAILED(pEngine->Invalidate(&line));
-                }
-
-                auto& buffer = _pData->GetTextBuffer();
-                auto& scratch = buffer.GetScratchpadRow();
-
-                std::wstring_view text{ _pData->activeComposition.text };
-                RowWriteState state{
-                    .columnLimit = buffer.GetRowByOffset(line.top).GetReadableColumnCount(),
-                };
-
-                state.text = text.substr(0, _pData->activeComposition.cursorPos);
-                scratch.ReplaceText(state);
-                const auto cursorOffset = state.columnEnd;
-
-                state.text = text.substr(_pData->activeComposition.cursorPos);
-                state.columnBegin = state.columnEnd;
-                scratch.ReplaceText(state);
-
-                // Ideally the text is inserted at the position of the cursor (`coordCursor`),
-                // but if we got more text than fits into the remaining space until the end of the line,
-                // then we'll insert the text aligned to the end of the line.
-                const auto remaining = state.columnLimit - state.columnEnd;
-                const auto beg = std::clamp(coordCursor.x, 0, remaining);
-
-                const auto baseAttribute = buffer.GetRowByOffset(coordCursor.y).GetAttrByColumn(coordCursor.x);
-                _compositionCache.emplace(til::point{ beg, coordCursor.y }, baseAttribute);
-
-                // Fake-move the cursor to where it needs to be in the active composition.
-                if (_currentCursorOptions)
-                {
-                    _currentCursorOptions->coordCursor.x = std::min(beg + cursorOffset, line.right - 1);
-                }
-            }
-        }
+        _invalidateCurrentCursor(); // Invalidate the new cursor position.
+        _prepareNewComposition();
 
         FOREACH_ENGINE(pEngine)
         {
@@ -510,6 +428,20 @@ bool Renderer::_CheckViewportAndScroll()
     }
 
     _ScrollPreviousSelection(coordDelta);
+
+    // The cursor may have moved out of or into the viewport. Update the .inViewport property.
+    {
+        const auto view = ScreenToBufferLine(srNewViewport, _currentCursorOptions.lineRendition);
+        const auto coordCursor = _currentCursorOptions.coordCursor;
+
+        // Note that we allow the X coordinate to be outside the left border by 1 position,
+        // because the cursor could still be visible if the focused character is double width.
+        const auto xInRange = coordCursor.x >= view.left - 1 && coordCursor.x <= view.right;
+        const auto yInRange = coordCursor.y >= view.top && coordCursor.y <= view.bottom;
+
+        _currentCursorOptions.inViewport = xInRange && yInRange;
+    }
+
     return true;
 }
 
@@ -1188,58 +1120,153 @@ bool Renderer::_isInHoveredInterval(const til::point coordTarget) const noexcept
 // - <none>
 // Return Value:
 // - nullopt if the cursor is off or out-of-frame, otherwise a CursorOptions
-[[nodiscard]] std::optional<CursorOptions> Renderer::_GetCursorInfo()
+void Renderer::_updateCursorInfo()
 {
-    if (_pData->IsCursorVisible())
+    // Get cursor position in buffer
+    auto coordCursor = _pData->GetCursorPosition();
+
+    // GH#3166: Only draw the cursor if it's actually in the viewport. It
+    // might be on the line that's in that partially visible row at the
+    // bottom of the viewport, the space that's not quite a full line in
+    // height. Since we don't draw that text, we shouldn't draw the cursor
+    // there either.
+
+    // The cursor is never rendered as double height, so we don't care about
+    // the exact line rendition - only whether it's double width or not.
+    const auto doubleWidth = _pData->GetTextBuffer().IsDoubleWidthLine(coordCursor.y);
+    const auto lineRendition = doubleWidth ? LineRendition::DoubleWidth : LineRendition::SingleWidth;
+
+    // We need to convert the screen coordinates of the viewport to an
+    // equivalent range of buffer cells, taking line rendition into account.
+    const auto viewport = _pData->GetViewport().ToInclusive();
+    const auto view = ScreenToBufferLine(viewport, lineRendition);
+
+    // Note that we allow the X coordinate to be outside the left border by 1 position,
+    // because the cursor could still be visible if the focused character is double width.
+    const auto xInRange = coordCursor.x >= view.left - 1 && coordCursor.x <= view.right;
+    const auto yInRange = coordCursor.y >= view.top && coordCursor.y <= view.bottom;
+
+    // Adjust cursor Y offset to viewport.
+    // The viewport X offset is saved in the options and handled with a transform.
+    coordCursor.y -= view.top;
+
+    const auto cursorColor = _renderSettings.GetColorTableEntry(TextColor::CURSOR_COLOR);
+    const auto useColor = cursorColor != INVALID_COLOR;
+
+    _currentCursorOptions.coordCursor = coordCursor;
+    _currentCursorOptions.viewportLeft = viewport.left;
+    _currentCursorOptions.lineRendition = lineRendition;
+    _currentCursorOptions.ulCursorHeightPercent = _pData->GetCursorHeight();
+    _currentCursorOptions.cursorPixelWidth = _pData->GetCursorPixelWidth();
+    _currentCursorOptions.fIsDoubleWidth = _pData->IsCursorDoubleWidth();
+    _currentCursorOptions.cursorType = _pData->GetCursorStyle();
+    _currentCursorOptions.fUseColor = useColor;
+    _currentCursorOptions.cursorColor = cursorColor;
+    _currentCursorOptions.isVisible = _pData->IsCursorVisible();
+    _currentCursorOptions.isOn = _currentCursorOptions.isVisible && _pData->IsCursorOn();
+    _currentCursorOptions.inViewport = xInRange && yInRange;
+}
+
+void Renderer::_invalidateCurrentCursor() const
+{
+    if (!_currentCursorOptions.inViewport || !_currentCursorOptions.isOn)
     {
-        // Get cursor position in buffer
-        auto coordCursor = _pData->GetCursorPosition();
+        return;
+    }
 
-        // GH#3166: Only draw the cursor if it's actually in the viewport. It
-        // might be on the line that's in that partially visible row at the
-        // bottom of the viewport, the space that's not quite a full line in
-        // height. Since we don't draw that text, we shouldn't draw the cursor
-        // there either.
+    const auto& buffer = _pData->GetTextBuffer();
+    const auto view = buffer.GetSize();
+    const auto coord = _currentCursorOptions.coordCursor;
 
-        // The cursor is never rendered as double height, so we don't care about
-        // the exact line rendition - only whether it's double width or not.
-        const auto doubleWidth = _pData->GetTextBuffer().IsDoubleWidthLine(coordCursor.y);
-        const auto lineRendition = doubleWidth ? LineRendition::DoubleWidth : LineRendition::SingleWidth;
+    const auto lineRendition = buffer.GetLineRendition(coord.y);
+    const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1;
 
-        // We need to convert the screen coordinates of the viewport to an
-        // equivalent range of buffer cells, taking line rendition into account.
-        const auto view = ScreenToBufferLine(_pData->GetViewport().ToInclusive(), lineRendition);
+    til::rect rect{ coord.x, coord.y, coord.x + cursorWidth, coord.y + 1 };
+    rect = BufferToScreenLine(rect, lineRendition);
 
-        // Note that we allow the X coordinate to be outside the left border by 1 position,
-        // because the cursor could still be visible if the focused character is double width.
-        const auto xInRange = coordCursor.x >= view.left - 1 && coordCursor.x <= view.right;
-        const auto yInRange = coordCursor.y >= view.top && coordCursor.y <= view.bottom;
-        if (xInRange && yInRange)
+    if (view.TrimToViewport(&rect))
+    {
+        FOREACH_ENGINE(pEngine)
+        {
+            LOG_IF_FAILED(pEngine->InvalidateCursor(&rect));
+        }
+    }
+}
+
+// If we had previously drawn a composition at the previous cursor position
+// we need to invalidate the entire line because who knows what changed.
+// (It's possible to figure that out, but not worth the effort right now.)
+void Renderer::_invalidateOldComposition() const
+{
+    if (!_compositionCache || !_currentCursorOptions.inViewport)
+    {
+        return;
+    }
+
+    const auto& buffer = _pData->GetTextBuffer();
+    const auto view = buffer.GetSize();
+    const auto coord = _currentCursorOptions.coordCursor;
+
+    til::rect rect{ 0, coord.y, til::CoordTypeMax, coord.y + 1 };
+    if (view.TrimToViewport(&rect))
+    {
+        FOREACH_ENGINE(pEngine)
         {
-            // Adjust cursor Y offset to viewport.
-            // The viewport X offset is saved in the options and handled with a transform.
-            coordCursor.y -= view.top;
-
-            auto cursorColor = _renderSettings.GetColorTableEntry(TextColor::CURSOR_COLOR);
-            auto useColor = cursorColor != INVALID_COLOR;
-
-            // Build up the cursor parameters including position, color, and drawing options
-            CursorOptions options;
-            options.coordCursor = coordCursor;
-            options.viewportLeft = _pData->GetViewport().Left();
-            options.lineRendition = lineRendition;
-            options.ulCursorHeightPercent = _pData->GetCursorHeight();
-            options.cursorPixelWidth = _pData->GetCursorPixelWidth();
-            options.fIsDoubleWidth = _pData->IsCursorDoubleWidth();
-            options.cursorType = _pData->GetCursorStyle();
-            options.fUseColor = useColor;
-            options.cursorColor = cursorColor;
-            options.isOn = _pData->IsCursorOn();
-
-            return { options };
+            LOG_IF_FAILED(pEngine->Invalidate(&rect));
         }
     }
-    return std::nullopt;
+}
+
+// Invalidate the line that the active TSF composition is on,
+// so that _PaintBufferOutput() actually gets a chance to draw it.
+void Renderer::_prepareNewComposition()
+{
+    if (_pData->activeComposition.text.empty())
+    {
+        return;
+    }
+
+    const auto viewport = _pData->GetViewport();
+    const auto coordCursor = _pData->GetCursorPosition();
+
+    til::rect line{ 0, coordCursor.y, til::CoordTypeMax, coordCursor.y + 1 };
+    if (viewport.TrimToViewport(&line))
+    {
+        viewport.ConvertToOrigin(&line);
+
+        FOREACH_ENGINE(pEngine)
+        {
+            LOG_IF_FAILED(pEngine->Invalidate(&line));
+        }
+
+        auto& buffer = _pData->GetTextBuffer();
+        auto& scratch = buffer.GetScratchpadRow();
+
+        std::wstring_view text{ _pData->activeComposition.text };
+        RowWriteState state{
+            .columnLimit = buffer.GetRowByOffset(line.top).GetReadableColumnCount(),
+        };
+
+        state.text = text.substr(0, _pData->activeComposition.cursorPos);
+        scratch.ReplaceText(state);
+        const auto cursorOffset = state.columnEnd;
+
+        state.text = text.substr(_pData->activeComposition.cursorPos);
+        state.columnBegin = state.columnEnd;
+        scratch.ReplaceText(state);
+
+        // Ideally the text is inserted at the position of the cursor (`coordCursor`),
+        // but if we got more text than fits into the remaining space until the end of the line,
+        // then we'll insert the text aligned to the end of the line.
+        const auto remaining = state.columnLimit - state.columnEnd;
+        const auto beg = std::clamp(coordCursor.x, 0, remaining);
+
+        const auto baseAttribute = buffer.GetRowByOffset(coordCursor.y).GetAttrByColumn(coordCursor.x);
+        _compositionCache.emplace(til::point{ beg, coordCursor.y }, baseAttribute);
+
+        // Fake-move the cursor to where it needs to be in the active composition.
+        _currentCursorOptions.coordCursor.x = std::min(beg + cursorOffset, line.right - 1);
+    }
 }
 
 // Routine Description:
@@ -1250,9 +1277,9 @@ bool Renderer::_isInHoveredInterval(const til::point coordTarget) const noexcept
 // - <none>
 void Renderer::_PaintCursor(_In_ IRenderEngine* const pEngine)
 {
-    if (_currentCursorOptions)
+    if (_currentCursorOptions.inViewport && _currentCursorOptions.isVisible)
     {
-        LOG_IF_FAILED(pEngine->PaintCursor(*_currentCursorOptions));
+        LOG_IF_FAILED(pEngine->PaintCursor(_currentCursorOptions));
     }
 }
 
@@ -1385,10 +1412,7 @@ void Renderer::_ScrollPreviousSelection(const til::point delta)
             rc += delta;
         }
 
-        if (_currentCursorOptions)
-        {
-            _currentCursorOptions->coordCursor += delta;
-        }
+        _currentCursorOptions.coordCursor += delta;
     }
 }
 
diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp
index 1944beb4b92..8c4308c5a87 100644
--- a/src/renderer/base/renderer.hpp
+++ b/src/renderer/base/renderer.hpp
@@ -122,7 +122,10 @@ namespace Microsoft::Console::Render
         void _ScrollPreviousSelection(const til::point delta);
         [[nodiscard]] HRESULT _PaintTitle(IRenderEngine* const pEngine);
         bool _isInHoveredInterval(til::point coordTarget) const noexcept;
-        [[nodiscard]] std::optional<CursorOptions> _GetCursorInfo();
+        void _updateCursorInfo();
+        void _invalidateCurrentCursor() const;
+        void _invalidateOldComposition() const;
+        void _prepareNewComposition();
         [[nodiscard]] HRESULT _PrepareRenderInfo(_In_ IRenderEngine* const pEngine);
 
         const RenderSettings& _renderSettings;
@@ -134,7 +137,7 @@ namespace Microsoft::Console::Render
         uint16_t _hyperlinkHoveredId = 0;
         std::optional<interval_tree::IntervalTree<til::point, size_t>::interval> _hoveredInterval;
         Microsoft::Console::Types::Viewport _viewport;
-        std::optional<CursorOptions> _currentCursorOptions;
+        CursorOptions _currentCursorOptions;
         std::optional<CompositionCache> _compositionCache;
         std::vector<Cluster> _clusterBuffer;
         std::vector<til::rect> _previousSelection;
diff --git a/src/renderer/inc/CursorOptions.h b/src/renderer/inc/CursorOptions.h
index d3c766c7b54..a0304261141 100644
--- a/src/renderer/inc/CursorOptions.h
+++ b/src/renderer/inc/CursorOptions.h
@@ -49,9 +49,14 @@ namespace Microsoft::Console::Render
         // Color to use for drawing instead of the default
         COLORREF cursorColor;
 
+        // The other kind of on/off state for the cursor, because VtEngine needs it to handle \x1b[?25l/h.
+        bool isVisible;
         // Is the cursor currently visually visible?
         // If the cursor has blinked off, this is false.
         // if the cursor has blinked on, this is true.
         bool isOn;
+
+        // Is the cursor within the viewport of the renderer?
+        bool inViewport;
     };
 }


Test patch: