Move all blink handling into Renderer #19330

lhecker · 2025-09-08T19:39:36Z

Note that this PR is not quite finished (~90%?). Missing:

Some polish in both conhost & particularly WT integration (e.g. caching the blink interval value)
Propagating the user blink setting on the WT side

This PR moves the cursor blinker and VT blink rendition timer into Renderer. To do so, this PR introduces a generic timer system with which you can schedule arbitrary timer jobs. Thanks to this, this PR removes a crapton of code, particularly throughout conhost, and improves throughput by another ~10%. On my PC it can now churn through >400MB/s while rendering at 240FPS. Fun fact: Processing 100kB of text and rendering it in conhost now takes less time than MSCTF (TSF) needs to process 1 keyboard character. When I look at our shell code.

Validation Steps Performed

TBD

src/interactivity/win32/AccessibilityNotifier.cpp

src/renderer/base/renderer.cpp

Goal: Remove `CursorBlinker`. Problem: Spooky action at a distance via `Cursor::HasMoved`. Solution: Moved all the a11y event raising into `_stream.cpp` and pray for the best. Goal: Prevent node.js from tanking conhost performance via MSAA (WHY). Problem: `ServiceLocator`. Solution: Unserviced the locator. Debounced event raising. Performance increased by >10x. Problem 2: Lots of files changed. This PR is a prerequisite for #19330 ## Validation Steps Performed Ran NVDA with and without UIA enabled and with different delays. ✅

src/cascadia/TerminalControl/HwndTerminal.hpp

src/cascadia/TerminalControl/HwndTerminal.cpp

carlos-zamora · 2025-09-23T19:15:41Z

src/cascadia/TerminalControl/HwndTerminal.cpp


-void _stdcall TerminalSetCursorVisible(void* terminal, const bool visible)
-try
+void HwndTerminal::_setFocused(bool focused) noexcept


Big fan of this. It's nice that it's all in one function 😊

carlos-zamora · 2025-09-23T19:48:32Z

src/renderer/base/renderer.cpp

-#define FOREACH_ENGINE(var)   \
-    for (auto var : _engines) \
-        if (!var)             \
-            break;            \
-        else


Huh. We don't need the if (!var) break anymore?

carlos-zamora · 2025-09-23T20:04:57Z

src/renderer/base/renderer.cpp

+    else if (!IsTimerRunning(_cursorBlinker))
+    {
+        const auto actual = GetTimerInterval(_cursorBlinker);
+        auto expected = _pData->GetBlinkInterval();


Suggested change

auto expected = _pData->GetBlinkInterval();

const auto expected = _pData->GetBlinkInterval();

const?

carlos-zamora · 2025-09-23T21:20:20Z

src/host/renderData.cpp

+
+bool RenderData::IsUiaDataInitialized() const noexcept
+{
+    return true;


(Double checking) In terminalrenderdata.cpp, we have this:

terminal/src/cascadia/TerminalCore/terminalrenderdata.cpp

Lines 226 to 234 in 1b712b2

bool Terminal::IsUiaDataInitialized() const noexcept

{

// GH#11135: Windows Terminal needs to create and return an automation peer

// when a screen reader requests it. However, the terminal might not be fully

// initialized yet. So we use this to check if any crucial components of

// UiaData are not yet initialized.

_assertLocked();

return !!_mainBuffer;

}

We don't need to check the buffer here?

Terminal's startup is less consistent and orderly than conhost

carlos-zamora · 2025-09-23T21:22:25Z

src/host/output.cpp

-    // initialize cursor GH#4102 - Typically, the cursor is set to on by the
-    // cursor blinker. Unfortunately, in conpty mode, there is no cursor
-    // blinker. So, in conpty mode, we need to leave the cursor on always. The
-    // cursor can still be set to hidden, and whether the cursor should be
-    // blinking will still be passed through to the terminal, but internally,
-    // the cursor should always be on.
-    //
-    // In particular, some applications make use of a calling
-    // `SetConsoleScreenBuffer` and `SetCursorPosition` without printing any
-    // text in between these calls. If we initialize the cursor to Off in conpty
-    // mode, then the cursor will remain off until they print text. This can
-    // lead to alignment problems in the terminal, because we won't move the
-    // terminal's cursor in this _exact_ scenario.
-    screenInfo.GetTextBuffer().GetCursor().SetIsOn(gci.IsInVtIoMode());
-


Hmm... Do we need to worry about this scenario still?

Nope. ConPTY no longer renders, so it no longer needs to even care about the cursor being On, and now IsOn is also dead because IsOn was the thing we toggled every half second and that responsibility has left this planet.

carlos-zamora · 2025-09-23T21:23:49Z

src/terminal/adapter/adaptDispatch.cpp

        // We trigger a scroll rather than a redraw, since that's more efficient,
        // but we need to turn the cursor off before doing so; otherwise, a ghost
        // cursor can be left behind in the previous position.


update the comment?

DHowett

50/56

DHowett · 2025-09-26T20:42:52Z

src/renderer/base/lib/base.vcxproj.filters

    <ClCompile Include="..\renderer.cpp">
      <Filter>Source Files</Filter>
    </ClCompile>
-    <ClCompile Include="..\thread.cpp">


remove the whole file as well, and update sources. that will also help git tell us which lines are the same in the old and new code

DHowett · 2025-09-26T20:45:26Z

src/interactivity/win32/window.cpp

 // - <none>
 // Return Value:
 // - <none>
 void Window::_UpdateSystemMetrics() const


📝 Make sure nobody else called this who cares about the cursor metrics (probably not)

DHowett · 2025-09-26T20:46:32Z

src/host/selectionState.cpp

    cursor.SetSize(_d->ulSavedCursorSize);
    cursor.SetIsVisible(_d->fSavedCursorVisible);
    cursor.SetType(_d->savedCursorType);
-    cursor.SetIsOn(true);


📝 make sure selection has the same cursor behavior

DHowett · 2025-09-26T20:49:44Z

src/host/renderData.cpp

+
+bool RenderData::IsUiaDataInitialized() const noexcept
+{
+    return true;


Terminal's startup is less consistent and orderly than conhost

DHowett · 2025-09-26T21:07:48Z

src/host/output.cpp

-    // initialize cursor GH#4102 - Typically, the cursor is set to on by the
-    // cursor blinker. Unfortunately, in conpty mode, there is no cursor
-    // blinker. So, in conpty mode, we need to leave the cursor on always. The
-    // cursor can still be set to hidden, and whether the cursor should be
-    // blinking will still be passed through to the terminal, but internally,
-    // the cursor should always be on.
-    //
-    // In particular, some applications make use of a calling
-    // `SetConsoleScreenBuffer` and `SetCursorPosition` without printing any
-    // text in between these calls. If we initialize the cursor to Off in conpty
-    // mode, then the cursor will remain off until they print text. This can
-    // lead to alignment problems in the terminal, because we won't move the
-    // terminal's cursor in this _exact_ scenario.
-    screenInfo.GetTextBuffer().GetCursor().SetIsOn(gci.IsInVtIoMode());
-


Nope. ConPTY no longer renders, so it no longer needs to even care about the cursor being On, and now IsOn is also dead because IsOn was the thing we toggled every half second and that responsibility has left this planet.

DHowett · 2025-09-26T21:09:32Z

src/host/AccessibilityNotifier.cpp

        return;
    }

+    // NOTE: The assumption is that you're holding the console lock when calling any of the member functions.


hmm - what happened here? doesn't this check above need to be under lock?

DHowett · 2025-09-26T21:23:18Z

src/cascadia/TerminalCore/TerminalSelection.cpp

    {
        // Enter Mark Mode
        // NOTE: directly set cursor state. We already should have locked before calling this function.
-        _activeBuffer().GetCursor().SetIsOn(false);


do we need to inhibit the cursor?

DHowett · 2025-09-26T21:24:57Z

src/cascadia/TerminalCore/Terminal.cpp


-void Terminal::BlinkCursor() noexcept
-{
-    if (_selectionMode != SelectionInteractionMode::Mark)


do we need to inhibit the cursor for mark mode

DHowett · 2025-09-26T21:25:57Z

src/cascadia/TerminalControl/TermControl.cpp

-            return;
-        }
-
-        if (_displayCursorWhileBlurred())


📝 we still need the "force blinking when unfocused" behavior - need to confirm it still works

DHowett · 2025-09-26T21:30:52Z

src/cascadia/TerminalControl/TermControl.cpp

-
-        // Set up blinking attributes
-        auto animationsEnabled = TRUE;
-        SystemParametersInfoW(SPI_GETCLIENTAREAANIMATION, 0, &animationsEnabled, 0);


📝 must make sure disabling animations windows-wide disables blink too

github-advanced-security bot found potential problems Sep 8, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

lhecker force-pushed the dev/lhecker/cursor-blinker branch from 4de989d to 8d17e85 Compare September 8, 2025 19:44

lhecker mentioned this pull request Sep 12, 2025

Rewrite the MSAA/UIA integration into conhost #19344

Merged

lhecker changed the base branch from main to dev/lhecker/BEGONE-FOUL-SPIRIT September 14, 2025 11:16

lhecker force-pushed the dev/lhecker/cursor-blinker branch from 8d17e85 to 8390d60 Compare September 14, 2025 11:26

lhecker force-pushed the dev/lhecker/cursor-blinker branch from 8390d60 to 5742c64 Compare September 22, 2025 22:59

lhecker changed the base branch from dev/lhecker/BEGONE-FOUL-SPIRIT to main September 22, 2025 22:59

lhecker marked this pull request as ready for review September 22, 2025 22:59

Move all blink handling into Renderer

1c23430

lhecker force-pushed the dev/lhecker/cursor-blinker branch from 5742c64 to 1c23430 Compare September 23, 2025 13:55

lhecker mentioned this pull request Sep 23, 2025

WPF terminal control in a tabbed interface deadlocks when rapidly opening new tabs #19373

Open

Fix tests?

1b712b2

carlos-zamora reviewed Sep 23, 2025

View reviewed changes

Fix tests

1c07227

DHowett reviewed Sep 26, 2025

View reviewed changes

	auto expected = _pData->GetBlinkInterval();
	const auto expected = _pData->GetBlinkInterval();

	bool Terminal::IsUiaDataInitialized() const noexcept
	{
	// GH#11135: Windows Terminal needs to create and return an automation peer
	// when a screen reader requests it. However, the terminal might not be fully
	// initialized yet. So we use this to check if any crucial components of
	// UiaData are not yet initialized.
	_assertLocked();
	return !!_mainBuffer;
	}

Move all blink handling into Renderer #19330

Are you sure you want to change the base?

Move all blink handling into Renderer #19330

Uh oh!

Conversation

lhecker commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lhecker commented Sep 8, 2025 •

edited

Loading