Skip to content

fix(vt): report cursor state a reset changes - #952

Open
Rohilalala wants to merge 1 commit into
charmbracelet:mainfrom
Rohilalala:fix/vt-cursor-visibility-on-reset
Open

fix(vt): report cursor state a reset changes#952
Rohilalala wants to merge 1 commit into
charmbracelet:mainfrom
Rohilalala:fix/vt-cursor-visibility-on-reset

Conversation

@Rohilalala

Copy link
Copy Markdown

Fixes #936.

Screen.Reset assigns a zero Cursor rather than going through the setters that carry the callbacks, and by the time resetModes restores the mode table the values already match — so nothing reports the change:

emu.Write([]byte("\x1b[?25l")) // CursorVisibility(false)
emu.Write([]byte("\x1bc"))     // cursor is visible again, silently

Scope

The issue is about visibility. Position and style are the same bug in the same function — RIS sends the cursor home and clears its style just as silently — so this covers all three. Verified before the change: a cursor moved to (5,3) with a blinking-underline style reports 0 position changes and 0 style changes across a reset. Happy to trim it back to visibility alone if you would rather keep the change narrow.

I went with reporting rather than adding a getter, since it needs no new API and matches how the other cursor state is already observed. A CursorVisible() getter would also solve the issue if you would prefer that shape.

Why the callbacks are muted during the reset

Several paths inside a reset touch the cursor, and setAltScreenMode reports visibility unconditionally — even when it has not changed. Reporting each of them would turn one reset into a burst of changes, and a reset arriving from the alternate screen would report visibility twice. So the three cursor callbacks are muted for the duration and each real change reported once at the end.

Restoring them happens in a defer. Without it, a callback that panics mid-reset — AltScreen fires in there — and is recovered by the caller would leave the cursor callbacks nil permanently, silencing every later change. There is a test for exactly that.

Tests

  • visibility reported once after a reset that changes it
  • no report when the cursor was already visible
  • exactly one report when the reset also leaves the alternate screen
  • position and style each reported once
  • callbacks still work after a panicking callback during a reset

Each was checked against a deliberately broken version to be sure it fails for the right reason: dropping the report, and turning the defer into a plain call at the end, each turn the matching test red.

go test ./... -race -count=3 -shuffle=on passes; golangci-lint run --config ../.golangci.yml ./... reports only the two findings already on main (emulator.go:421, csi.go:39), in files this PR does not touch.

Noticed, not fixed

setAltScreenMode reports CursorVisibility on every screen switch whether or not visibility changed, unlike setCursorHidden which checks first. That is what forced the muting here rather than a simple compare. It looked like its own change to make, but I am happy to fold it in.

A reset returns the cursor to visible, unstyled and at the home position,
and reports none of it. [Screen.Reset] assigns a zero Cursor rather than
going through the setters that carry the callbacks, and by the time the
mode table is restored the values already match, so the change never
reaches a consumer:

    emu.Write([]byte("\x1b[?25l")) // CursorVisibility(false)
    emu.Write([]byte("\x1bc"))     // cursor is visible again, silently

Visibility, position and style are only observable through their
callbacks, so a renderer drawing the host terminal's own cursor keeps
drawing a hidden one, in the wrong place, in the wrong style, until some
later application happens to change each.

The three callbacks are now muted for the duration of the reset and each
change reported once at the end. Muting matters because several paths
inside a reset touch the cursor — leaving the alternate screen reports
visibility unconditionally, for one — and a reset is a single event to a
consumer, not a burst. Restoring them happens in a defer, so a callback
that panics during the reset, recovered by the caller, cannot leave the
cursor silenced for good.

Tests cover the reported visibility change, no report when visibility did
not change, a single report when the reset also leaves the alternate
screen, the position and style reports, and the panicking-callback path.

Fixes charmbracelet#936
@Rohilalala
Rohilalala force-pushed the fix/vt-cursor-visibility-on-reset branch from 656625a to 110232c Compare August 24, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vt: expose cursor visibility after a terminal reset

1 participant