Skip to content

Overlay setting: show when a layer is active - #1337

Merged
malpern merged 1 commit into
masterfrom
feature/overlay-show-only-while-layer-held
Oct 24, 2026
Merged

malpern merged 1 commit into
masterfrom
feature/overlay-show-only-while-layer-held

Conversation

@malpern

@malpern malpern commented Oct 23, 2026 •

Copy link
Copy Markdown
Owner

Adds Settings → General → Keyboard Appearance → Show when a layer is active. It is off by default. When enabled, KeyPath leaves the overlay hidden on the base layer, reveals it for held, one-shot, or toggled layers according to the existing Hold/Tap trigger, and hides it after returning to base. The user can still open the overlay explicitly.

The existing layer-reveal policy remains the single state machine. Launch and ordinary app activation do not auto-open the overlay in this mode; explicit reopen, keyboard shortcut, menu command, and wizard handoff still can. Turning the setting on clears pending visibility restores. The guide now describes the shipped all-keyboards Caps behavior as well.

Validation: focused OverlayLayerRevealPolicyTests and PreferencesServicePersistenceTests passed locally after rebasing on current master; git diff --check passed. Live installed-app QA and refreshed GitHub CI/review are in progress.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-23T03:48:58.227757Z d0240e4 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review of the diff only. I found no blocking issues, but there are three edge cases to check.

  1. App.swift launch path. The .showOverlay-style branch now passes userRequested: true. Its log line uses \(trigger). If that trigger also covers the initial launch (not only reopen or activation), the overlay will open at launch even with "Show only while a layer is held" on. That contradicts the guide ("It stays hidden when KeyPath starts"). Please confirm the launch trigger doesn't reach this branch, or gate userRequested on reopen only.
  2. Auto-hide restore can bypass the setting. forgetPendingVisibilityRestores() runs only when the toggle is switched on. If the overlay is revealed by a layer and the user then opens Settings or the Wizard, auto-hide may set wasVisibleBeforeAutoHide = true. resetSettingsAutoHideGuard() would then restore a layer-revealed overlay after the window closes. Consider checking overlayShowsOnlyWhileLayerHeld in the restore paths (resetSettingsAutoHideGuard and app-suppression restore), or skipping the capture when the overlay was only revealed for a layer.
  3. Test coverage. The tests cover the pure policy, startupShowAllowed and preference persistence. Nothing covers overlayShowsOnlyWhileLayerHeldChanged, the restoreState early return, or the restore-after-Settings path. Even a small controller-level test for case 2 would help.

Minor: turning the setting off doesn't bring back an overlay that was hidden when it was turned on. That is probably fine, but the guide could say so.

🤖 Generated with Claude Code

@malpern

malpern commented Oct 23, 2026

Copy link
Copy Markdown
Owner Author

Thanks. Going through the points:

  1. Launch path: presentReopenSurface is called only with trigger: "reopen" (clicking the Dock icon) and "status item" (clicking the menu-bar item), and both are user actions. The launch, relaunch and activation paths call showForStartup with userRequested defaulted to false, so they stay hidden. No change.
  2. Auto-hide restore: both restore captures already leave out layer-revealed overlays. autoHideOnceForSettings records isVisibleByChoice (which is false while layerReveal.state == .revealed) before it hides, and suppressForApp records isVisibleByChoice && canAutoShow. So only an overlay the user opened themselves (⌥⌘K) is restored, and that's intended. No change.
  3. Controller-level tests: LiveKeyboardOverlayController is a window-owning singleton with a private init, so these paths aren't unit-testable without refactoring the controller, which I've left out of this PR. The decisions themselves are in the pure policy and startupShowAllowed, and both are tested.

Minor: added a sentence to the guide saying that turning the setting off doesn't reopen the overlay.

@claude

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review of PR 1337: no blocking issues found. Two things worth a look:

  1. Untested controller paths. The tests cover OverlayLayerRevealPolicy and the startupShowAllowed helper. They don't cover the controller changes: the restoreState() guard, overlayShowsOnlyWhileLayerHeldChanged, and forgetPendingVisibilityRestores. In particular, nothing checks that turning the setting on while Settings or the Wizard has auto-hidden the overlay stops resetSettingsAutoHideGuard() from reopening it. A small test with an injectable preference or visibility seam would pin that down.

  2. Other show paths. Only showForStartup and restoreState are gated. Please confirm no other path shows the overlay without going through them (for example app-suppression restore, or an activation handler that calls isVisible = true directly). forgetPendingVisibilityRestores clears the two restore flags, but only at the moment the setting is toggled. If a flag is set again afterwards, for example by a later app suppression while the setting is on, the overlay could still be restored. Consider checking the preference at restore time too.

Minor: overlayShowsOnlyWhileLayerHeldChanged sets isVisible = false when isVisibleByChoice is true. Confirm this doesn't set the explicit-hide flag or overwrite the saved visibility in a way that changes behavior when the setting is later turned off. The guide says it won't reopen the overlay, which matches.

@malpern

malpern commented Oct 23, 2026 •

Copy link
Copy Markdown
Owner Author

Thanks. Going through the points:

Other show paths: I audited every isVisible = true and showForStartup call site.

  • Menu, ⌥⌘K, toggle and Dock/status-item reopen are user actions and are meant to open it.
  • Settings/Wizard and app-suppression restores re-record their flag on every capture, and they record isVisibleByChoice. A layer-revealed overlay is never queued for restore, even if the flag is set again later while the setting is on. Only an overlay the user opened is restored, which is intended.
  • The Help window was the exception. It recorded isVisible, so it would reopen, and save as open, an overlay that a held layer had shown. Fixed in ed71662 by recording isVisibleByChoice.

Controller tests: LiveKeyboardOverlayController is a window-owning singleton with a private init, so these paths need a refactor before they can be unit-tested, and I've left that out of this PR. They'll be checked live on the Air instead.

Minor: isVisible = false saves the visibility as hidden, but it doesn't set the explicit-hide flag. That flag is set only by hideExplicitly(). Turning the setting off leaves the overlay closed until the user opens it, as the guide says.

@claude

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review of the diff only; nothing was built or run.

I found no blocking issues. The gating is consistent: showForStartup is skipped unless userRequested is set, restoreState is skipped, and the layer-reveal policy is forced on. The three hand-off call sites (reopen, wizard success, first-success onboarding) pass userRequested: true.

Minor, non-blocking:

  • Controller tests: the new tests cover the policy, the startupShowAllowed helper and preference persistence. Nothing covers overlayShowsOnlyWhileLayerHeldChanged, the restoreState early return, or the Help window change (a layer-revealed overlay is hidden on open but not reopened on close). A small controller-level test for each would guard against regressions.
  • Switching the setting on mid-hold: overlayShowsOnlyWhileLayerHeldChanged(true) hides the overlay if it is visible by choice. If a layer-reveal is already in flight at that moment, the policy state could be out of step with the window. This is unlikely to matter, since Settings is open in the foreground.
  • Docs: the guide text is clear. It could also mention that the setting does not change how the ⌥⌘K hide state persists.

🤖 Generated with Claude Code

@malpern

malpern commented Oct 23, 2026

Copy link
Copy Markdown
Owner Author

Thanks, no change needed for these. Turning the setting on mid-hold can't happen: the toggle is in Settings, and while Settings is open the controller blocks layer reveals (revealForLayer checks hasAutoHiddenForCurrentSettingsSession), so no reveal is in progress when the toggle runs. The controller tests aren't added, for the same reason as above: the singleton needs a refactor first. Those paths are being checked live on the Air instead.

@malpern

malpern commented Oct 23, 2026

Copy link
Copy Markdown
Owner Author

Live check on the Air (build ed7166250, executable sha256 91f1158c…537c9, Developer ID signed through Scripts/air-iterate.sh; runtime verification passed):

  • Launch: with the setting on (set via defaults), KeyPath relaunched with the overlay hidden. The log shows showForStartup skipped - overlay shows only while a layer is held.
  • Layer reveal and hide: keypath://layer/nav revealed the overlay about 0.43 s after the layer change (Layer held - revealing overlay). keypath://layer/base hid it about 0.15 s later (Layer ended - hiding revealed overlay).

Not yet verified live:

  • holding a physical key, such as Space for nav;
  • a quick tap or Space+J staying hidden;
  • the Settings checkbox UI;
  • ⌥⌘K still opening the overlay.

These need someone at the Air's keyboard. Injected key events don't drive the engine, so I couldn't simulate them.

CI note: build-and-test compiled the new tests, but the KeyPathTests xctest process crashed with signal 5 partway through LauncherStoreTransactionTests, before reaching OverlayLayerRevealPolicyTests and PreferencesServicePersistenceTests. The job still passed because KEYPATH_ALLOW_TEST_RUNNER_CRASH_SUCCESS=1. The same crash happened on fix/aqua-synthetic-paste (run 39728809569), so this PR didn't cause it, but the new tests haven't actually run yet.

@malpern
malpern force-pushed the feature/overlay-show-only-while-layer-held branch from ed71662 to 038cf7c Compare October 24, 2026 18:13
@malpern malpern changed the title Overlay setting: show only while a layer is held Overlay setting: show when a layer is active Oct 24, 2026
@claude

claude Bot commented Oct 24, 2026

Copy link
Copy Markdown

Review: no blocking findings.

The gating is consistent. Startup, restoreState, Settings/Wizard restores and app-suppression restores are all covered. Guided hand-offs still open the overlay via userRequested: true. The policy and persistence tests cover the new behavior.

Minor notes, none blocking:

  • Test gap: restoreState() skipping when the setting is on, and overlayShowsOnlyWhileLayerHeldChanged(_:) (hides the overlay and clears pending restores), have no direct tests. Only the pure startupShowAllowed helper is tested.
  • Other showForStartup callers: I could only see the diff. Please confirm that every other caller either should be suppressed in this mode or passes userRequested: true. The default is false, so a missed user-initiated path would silently do nothing.
  • Scope creep: the Keyboard Setup bullet in guides/general-settings.md (Caps Lock wording) is unrelated to this change. Split it out or mention it in the PR description.
  • Naming: the pref and identifier say "ShowsOnlyWhileLayerHeld", but the UI and the guide say "Show when a layer is active". The behavior also covers toggled and one-shot layers. This is fine, but the name is slightly misleading.

@malpern
malpern merged commit 044a60a into master Oct 24, 2026
3 checks passed
@malpern
malpern deleted the feature/overlay-show-only-while-layer-held branch October 24, 2026 18:17
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.

1 participant