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

src/renderer/base/renderer.cpp

src/host/renderData.cpp

src/host/output.cpp

src/terminal/adapter/adaptDispatch.cpp

DHowett

50/56

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

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

src/host/renderData.cpp

src/host/output.cpp

src/host/AccessibilityNotifier.cpp

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

I just noticed that we let the cursor blink during selections. Why do we inhibit it for mark mode then, even though mark mode is essentially just keyboard-selection?

In my mind, we want the cursor to blink during mark mode to know which endpoint is currently focused. That way, you'll know if you're moving the start or end before you press on an arrow key.

src/cascadia/TerminalControl/TermControl.cpp

…inker

carlos-zamora · 2025-11-04T21:20:04Z

src/buffer/out/cursor.cpp

 // Return Value:
 // - <none>
-void Cursor::CopyProperties(const Cursor& OtherCursor) noexcept
+void Cursor::CopyProperties(const Cursor& other) noexcept


Do we need to update _parentBuffer too?

carlos-zamora · 2025-11-04T21:28:34Z

src/cascadia/TerminalControl/TermControl.cpp

+
    void TermControl::CursorVisibility(Control::CursorDisplayState cursorVisibility)
    {
        _cursorVisibility = cursorVisibility;


It looks like cursor visibility is stored across 3 variables:

TermControl::_cursorVisibility

ControlCore::_forceCursorVisible

Renderer::_cursorVisibilityInhibitors

Could we remove the one in TermControl and just query the one in ControlCore? Or, even better, just pipe the value from Renderer all the way up, rather than having a separate boolean member on each layer?

carlos-zamora · 2025-11-04T21:33:45Z

src/cascadia/TerminalCore/Terminal.cpp


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


In my mind, we want the cursor to blink during mark mode to know which endpoint is currently focused. That way, you'll know if you're moving the start or end before you press on an arrow key.

carlos-zamora · 2025-11-04T22:11:44Z

src/cascadia/TerminalSettingsEditor/Profiles_Appearance.cpp

            _previewControl = Control::TermControl(settings, settings, *_previewConnection);
            _previewControl.IsEnabled(false);
            _previewControl.AllowFocusWhenDisabled(false);
-            _previewControl.CursorVisibility(Microsoft::Terminal::Control::CursorDisplayState::Shown);


📝 Verified that the cursor still blinks

carlos-zamora · 2025-11-04T22:38:19Z

src/renderer/base/renderer.hpp

+        static DWORD _timerToMillis(TimerRepr t) noexcept;
+
+        // Actual rendering
+        [[nodiscard]] HRESULT PaintFrame();


It's a bit weird that this one is marked private, but doesn't have the _ prefix. Plus there's also _PaintFrame() just below it.

carlos-zamora · 2025-11-04T23:29:01Z

src/host/selection.cpp

        pWindow->UpdateWindowText();
-
-        auto& an = ServiceLocator::LocateGlobals().accessibilityNotifier;
-        an.SelectionChanged();


Don't we still need this?

carlos-zamora · 2025-11-04T23:29:18Z

src/host/selection.cpp

    // Fire off an event to let accessibility apps know the selection has changed.
    auto& an = ServiceLocator::LocateGlobals().accessibilityNotifier;
    an.CursorChanged(coordBufferPos, true);
-    an.SelectionChanged();


carlos-zamora · 2025-11-04T23:41:04Z

Fully reviewed. I want to get some resolution on my comments before approving.

Regarding accessibility (#19374), I plan on taking a look at a reported memory leak in WT when UIA is enabled soon-ish. I'll lump looking into #19374 too and Leonard and I can tag team work on a11y then.

carlos-zamora · 2025-11-04T23:42:04Z

Oh, and you probably want to update the PR body 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

lhecker added 6 commits October 14, 2025 18:37

Merge remote-tracking branch 'origin/main' into dev/lhecker/cursor-bl…

0ff1f1f

…inker

Fix off-by-one bugs

7083bc0

Add support for EVENT_CONSOLE_UPDATE_SIMPLE

8214051

Fix build

55e3efe

Address feedback, Fix broadcast input

dfad2d9

Fix assert

99c6193

carlos-zamora reviewed Nov 4, 2025

View reviewed changes

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

carlos-zamora commented Nov 4, 2025

Uh oh!

carlos-zamora commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lhecker commented Sep 8, 2025 •

edited

Loading