Prevent the VT engine painting unnecessarily (#17194)

When the VT render engine starts a paint operation, it first checks to
see whether there is actually something to do, and if not it can end the
frame early. However, the result of that check was being ignored, which
could sometimes result in an unwanted `SGR` reset being written to the
conpty pipe.

This was particular concerning when passing through `DCS` sequences,
because an unexpected `SGR` in the middle of the `DCS` string would
cause it to abort early.

This PR addresses the problem by making sure the `VtEngine::StartPaint`
return value is appropriately handled in the `XtermEngine` class.

## Detailed Description of the Pull Request / Additional comments

To make this work, I also needed to correct the `_cursorMoved` flag,
because that is one of things that determines whether a paint is needed
or not, but it was being set in the `InvalidateCursor` method at the
start of ever frame, regardless of whether the cursor had actually
moved.

I also took this opportunity to get rid of the `_WillWriteSingleChar`
method and the `_quickReturn` flag, which have been mostly obsolete for
a long time now. The only place the flag was still used was to optimize
single char writes when line renditions are active. But that could more
easily be handled by testing the `_invalidMap` directly.

## Validation Steps Performed

I've confirmed that the test case in issue #17117 is no longer aborting
the `DCS` color table sequence early.

Closes #17117

src/renderer/vt/XtermEngine.cpp

@@ -37,7 +37,16 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
 //      the pipe.
 [[nodiscard]] HRESULT XtermEngine::StartPaint() noexcept
 {
-    RETURN_IF_FAILED(VtEngine::StartPaint());
+    const auto hr = VtEngine::StartPaint();
+    if (hr != S_OK)
+    {
+        // If _noFlushOnEnd was set, and we didn't return early, it would usually
+        // have been reset in the EndPaint call. But since that's not going to
+        // happen now, we need to reset it here, otherwise we may mistakenly skip
+        // the flush on the next frame.
+        _noFlushOnEnd = false;
+        return hr;
+    }
 
     _trace.TraceLastText(_lastText);
 
@@ -83,15 +92,6 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
         }
     }
 
-    if (!_quickReturn)
-    {
-        if (_WillWriteSingleChar())
-        {
-            // Don't re-enable the cursor.
-            _quickReturn = true;
-        }
-    }
-
     return S_OK;
 }
 

src/renderer/vt/invalidate.cpp

@@ -75,7 +75,7 @@ CATCH_RETURN();
     }
     _skipCursor = false;
 
-    _cursorMoved = true;
+    _cursorMoved = psrRegion->origin() != _lastText;
     return S_OK;
 }
 

src/renderer/vt/math.cpp

@@ -52,38 +52,3 @@ void VtEngine::_OrRect(_Inout_ til::inclusive_rect* const pRectExisting, const t
     pRectExisting->right = std::max(pRectExisting->right, pRectToOr->right);
     pRectExisting->bottom = std::max(pRectExisting->bottom, pRectToOr->bottom);
 }
-
-// Method Description:
-// - Returns true if the invalidated region indicates that we only need to
-//      simply print text from the current cursor position. This will prevent us
-//      from sending extra VT set-up/tear down sequences (?12h/l) when all we
-//      need to do is print more text at the current cursor position.
-// Arguments:
-// - <none>
-// Return Value:
-// - true iff only the next character is invalid
-bool VtEngine::_WillWriteSingleChar() const
-{
-    // If there is no scroll delta, return false.
-    if (til::point{ 0, 0 } != _scrollDelta)
-    {
-        return false;
-    }
-
-    // If there is more than one invalid char, return false.
-    if (!_invalidMap.one())
-    {
-        return false;
-    }
-
-    // Get the single point at which things are invalid.
-    const auto invalidPoint = _invalidMap.runs().front().origin();
-
-    // Either the next character to the right or the immediately previous
-    //      character should follow this code path
-    //      (The immediate previous character would suggest a backspace)
-    auto invalidIsNext = invalidPoint == _lastText;
-    auto invalidIsLast = invalidPoint == til::point{ _lastText.x - 1, _lastText.y };
-
-    return invalidIsNext || invalidIsLast;
-}

src/renderer/vt/paint.cpp

@@ -20,30 +20,30 @@ using namespace Microsoft::Console::Types;
 //      HRESULT error code if painting didn't start successfully.
 [[nodiscard]] HRESULT VtEngine::StartPaint() noexcept
 {
+    // When unit testing, there may be no pipe, but we still need to paint.
     if (!_hFile)
     {
-        return S_FALSE;
+        return S_OK;
     }
 
     // If we're using line renditions, and this is a full screen paint, we can
     // potentially stop using them at the end of this frame.
     _stopUsingLineRenditions = _usingLineRenditions && _AllIsInvalid();
 
-    // If there's nothing to do, quick return
+    // If there's nothing to do, we won't need to paint.
     auto somethingToDo = _invalidMap.any() ||
                          _scrollDelta != til::point{ 0, 0 } ||
                          _cursorMoved ||
                          _titleChanged;
 
-    _quickReturn = !somethingToDo;
-    _trace.TraceStartPaint(_quickReturn,
+    _trace.TraceStartPaint(!somethingToDo,
                            _invalidMap,
                            _lastViewport.ToExclusive(),
                            _scrollDelta,
                            _cursorMoved,
                            _wrappedRow);
 
-    return _quickReturn ? S_FALSE : S_OK;
+    return somethingToDo ? S_OK : S_FALSE;
 }
 
 // Routine Description:
@@ -142,9 +142,9 @@ using namespace Microsoft::Console::Types;
         _usingLineRenditions = true;
     }
     // One simple optimization is that we can skip sending the line attributes
-    // when _quickReturn is true. That indicates that we're writing out a single
-    // character, which should preclude there being a rendition switch.
-    if (_usingLineRenditions && !_quickReturn)
+    // when we're writing out a single character, which should preclude there
+    // being a rendition switch.
+    if (_usingLineRenditions && !_invalidMap.one())
     {
         RETURN_IF_FAILED(_MoveCursor({ _lastText.x, targetRow }));
         switch (lineRendition)

src/renderer/vt/state.cpp

@@ -37,7 +37,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
     _pool(til::pmr::get_default_resource()),
     _invalidMap(initialViewport.Dimensions(), false, &_pool),
     _scrollDelta(0, 0),
-    _quickReturn(false),
     _clearedAllThisFrame(false),
     _cursorMoved(false),
     _resized(false),

src/renderer/vt/vtrenderer.hpp

@@ -113,7 +113,6 @@ namespace Microsoft::Console::Render
         til::point _lastText;
         til::point _scrollDelta;
 
-        bool _quickReturn;
         bool _clearedAllThisFrame;
         bool _cursorMoved;
         bool _resized;
@@ -214,8 +213,6 @@ namespace Microsoft::Console::Render
         [[nodiscard]] HRESULT _RgbUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept;
         [[nodiscard]] HRESULT _16ColorUpdateDrawingBrushes(const TextAttribute& textAttributes) noexcept;
 
-        bool _WillWriteSingleChar() const;
-
         // buffer space for these two functions to build their lines
         // so they don't have to alloc/free in a tight loop
         std::wstring _bufferLine;