windows: put the palette back when a theme browse is abandoned - #745
Open
deblasis wants to merge 2 commits into
Open
windows: put the palette back when a theme browse is abandoned#745deblasis wants to merge 2 commits into
deblasis wants to merge 2 commits into
Conversation
Closes #732 The inline picker applies every theme it names through ConfigService, so a preview repaints every surface and all the chrome, not just the pane the picker is drawn in. Nothing told a browse from a choice, so arrowing past a theme and pressing Escape left it applied for good. Reading the confirmed flag would not have fixed it, in either direction. A cancel is silent: Escape and ^C set should_quit and fall through to a notify that only fires when the selection moved, which it has not, so no final callback arrives at all. And accepting fires two callbacks -- the confirm, then a preview for the very theme just confirmed, on the same key -- so the last callback of an accepted run says "not confirmed". A run whose outcome were read off it would revert the theme the user chose. So the flag is latched when it arrives, and the close is what decides. The snapshot is taken lazily before the first preview and no other: on every later one the live colours are already a preview's, and restoring those would put back a theme the user never had. It is a capture callback rather than a value so the call site cannot take a snapshot that is not the first, and the palette is copied because the shell's is one array that theme application overwrites in place. The revert is not narrowed to the endings that look like a user cancelling. A surface freed under the picker is not the user rejecting a theme, but it is not acceptance either, and the colours belong to the app rather than that surface, so leaving them would be the same defect in every remaining window. It costs one in-memory apply, and ConfigService fences it once shutdown starts. The decision is a small state machine in Ghostty.Core rather than inline in the window, for the reason the pipe retry policy and the active-window choice are: the test project does not reference the shell, so logic left here is only ever exercised by a human arrowing through a theme list. Two things this does not fix, both pre-existing and reported rather than absorbed. A confirmed theme is not persisted -- nothing on this path writes the config file, so a chosen theme is gone on restart. And the inline and pipe paths can genuinely overlap, because only a lone +list-themes routes to the picker while any other argument form drives the pipe, so their snapshots are kept separate rather than shared.
The snapshot was a field on the window, but the palette it saves is the app's: ApplyThemeColors writes one array and fans the change out to every window. Two browses can be live at once, because each list-themes routes to whichever window is active and a window only ever closes its own picker. So browsing in one window, accepting a theme in the other, then cancelling the first restored colours from before the accept and threw the accepted theme away. That was new here -- before, the cancel did nothing at all. One slot on App now, and the rules are about the slot rather than about a run: the first preview to find it empty fills it, a confirm empties it, and a close spends it. The latch is gone. With two browses sharing a palette a latch belongs to neither, and keeping it reintroduces the same defect from the other side: accept in one window, browse on in the other, and its cancel is silenced by a flag that has nothing to do with the accept, leaving the browsed theme sitting over the accepted one. The pipe path had its own copy of the same idea -- snapshot every client connect, revert on a disconnect without a confirm -- and knew nothing about the picker. Both paths can run at once, since only a lone list-themes reaches the picker and any other argument form drives the pipe. It shares the slot now, recording before it applies. Also from the pass: the callback carries the run it belongs to, so one still queued when a second picker opens cannot record against it; the sweep for unrecorded applies reads identifiers rather than call receivers, so assigning the delegate to a local first no longer walks past it; and the reachability check now rejects a guard that returns before the call as well as one that wraps it, which is what the escape it was written for actually looked like. Three comments claimed a cancel never calls back. That holds only once the selection has moved: cancelling on the very first key does fire a preview, for the theme the list opened on, which the guard and the revert absorb between them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #732
The defect
The inline picker applies every theme it names through
ConfigService, so a preview repaints every surface and all the chrome, not just the pane the picker is drawn in. Nothing told a browse from a choice, so arrowing past a theme and pressing Escape left it applied for good.Why reading the confirmed flag would not have fixed it
Both directions are wrong, and both were verified in the Zig:
A cancel is silent, once the selection has moved. Escape and Ctrl+C set
should_quitand fall through to a notify that fires only when the index changed, which it has not -- so no final callback arrives. (The exception is cancelling on the very first key, before anything has notified: that one does fire a preview for the theme the list opened on. The guard and the revert absorb it between them.)And accepting on the first key fires two callbacks -- the confirm, then an echo preview for the theme just confirmed -- so the last callback of that run says "not confirmed". After any arrow there is no echo at all.
So the close decides, not the last callback seen.
Ownership
The first version put the snapshot on the window. The review caught that as a regression this change would have introduced: the palette is the app's,
ApplyThemeColorswrites one array and fans the change to every window, and two browses can be live at once because each+list-themesroutes to whichever window is active and a window only ever closes its own picker. Browse in W1, accept in W2, cancel in W1, and the accepted theme was thrown away for colours from before it.One slot on
Appnow, with rules about the slot rather than about a run: the first preview to find it empty fills it, a confirm empties it, a close spends it.The latch is gone, and that is the point rather than a simplification. With two browses sharing a palette a latch belongs to neither, and it reintroduces the same defect from the other side -- accept in one window, browse on in the other, and its cancel is silenced by a flag that has nothing to do with the accept, leaving the browsed theme sitting over the accepted one.
The pipe path had its own copy of the same idea, snapshotting on every client connect and reverting on a disconnect without a confirm, and knew nothing about the picker. The two genuinely overlap -- only a lone
+list-themesreaches the inline picker, while any other argument form drivesPREVIEW:/CONFIRM:on the same pipe -- so it shares the slot now and records before it applies.Also from the review
The callback carries the run it belongs to, so one still queued when a second picker opens cannot record against it -- a stale confirm would otherwise latch the new run and reproduce the original defect through the fix.
Two guards were weaker than they read. The sweep for unrecorded applies matched call receivers, so assigning the delegate to a local first walked past it; it reads identifiers now. And the reachability check rejected a wrapping
ifbut not a preceding early return, which is what the escape it was written for actually looks like.Tests
The decision is a small state machine in
Ghostty.Core, for the reason the pipe retry policy and the active-window choice are there: the test project does not reference the shell, so logic left in the window is only ever exercised by a human arrowing through a theme list.Every rule verified by mutation. Worth recording one: the first version of the clobber test stayed green under the latch mutation, because a single
End()clears a latch anyway. It only became a real test when rewritten to browse on after the confirm and assert the cancel returns the accepted colours.Suite 2304 passing, 2305 total.
Not fixed here
A confirmed theme is still not written to the config, so the choice is lost on restart -- # 743. Nothing on this path reaches the config writer, and wiring it up means deciding how
themeinteracts with explicit colour keys and with the light/dark split form.The type is still called
InlineThemePreviewSessionalthough it now governs the pipe path too, and making the apply itself the record would close the unrecorded-apply class structurally rather than by a sweep. Both are separate changes.Size-override: 1013 of 1364 countable lines are tests -- 704 of them the wiring guards and their per-rule docstrings, the rest unit tests for the pure session. The production change is 351 lines across four files. The guards are what caught this change's own ownership regression, so they belong with the fix rather than in a follow-up.