Skip to content

fix(gpui): unbind macOS ⌘ shortcuts and wire ⌘Q to quit - #240

Closed
zhbjerry wants to merge 2 commits into
Kuddev:mainfrom
zhbjerry:fix/macos-command-keys-20260921
Closed

zhbjerry wants to merge 2 commits into
Kuddev:mainfrom
zhbjerry:fix/macos-command-keys-20260921

Conversation

@zhbjerry

@zhbjerry zhbjerry commented Sep 21, 2026

Copy link
Copy Markdown

Result / 用户结果

Fixes #238.

On macOS the shell's ⌘ shortcut set lived outside the keybinding model, so it could not be released, and Action::Quit was never wired at all. Now:

  • Clearing an action releases its ⌘ key together with its configured key. Clearing the Shell picker row writes keybind=Ctrl+K:ReceiveChar and keybind=cmd+k:ReceiveChar, and ⌘K stops opening the panel immediately, without a restart. Restoring the row brings both keys back.
  • ⌘Q quits through the same path as tray quit: drafts and session are saved before PTYs stop.
  • On macOS both the keymap page and the lines the app writes into pebrel_settings.txt spell the super modifier Cmd+ instead of Win+, so Ctrl+Win+F / Win+F are gone.

Users upgrading from a released build must clear the row once more: their existing file predates the alias vocabulary and holds no line for the ⌘ half. Nothing else changes for them — win+ / super+ / cmd+ stay equivalent in keybind= lines, and the settings page continues to show the configuration table's keys.

Out of scope / 不在本次范围

Unbinding ⌘K does not make it clear the screen, and this PR does not add that. The key is released, not re-bound to anything:

  • The GPUI terminal encoder translates Ctrl, Alt and named-key combinations and returns None for a pure ⌘ combination (nebula_app/src/gpui_shell/terminal/keymap.rs:276), so nothing reaches the PTY.
  • Pebrel's own macOS default table already declares ⌘K as clear screen (nebula_app/src/config/bindings/defaults.rs:367: Esc("\x0c") plus ClearHistory), but workspace_binding_in_context has no arm for Action::Esc / Action::ClearHistory, so the shell never implemented it.
  • Doing it properly means adding a clear-screen action (write 0x0c, clear scrollback) and deciding whether macOS ⌘K should belong to that action instead of the Shell picker — a default-behavior change. I can submit it separately once the maintainer agrees on scope.

Ctrl+L still passes through to the shell (defaults.rs:81 keeps it as ReceiveChar); it clears the screen at the shell level but not the app's scrollback.

Design / 设计边界

  • Responsibility and affected modules: display::keymap stays the single authority for defaults, unbinding and restoring; gpui_shell/workspace/keyboard_bindings.rs only translates that table into gpui bindings. MACOS_COMMAND_ALIASES replaces the second hand-written ⌘ list in bind_macos_command_keys — keeping two copies is what produced the defect.
  • Why this belongs here; interfaces that remain unchanged: default_shortcuts() gains the alias entries on macOS, deduped against config-table entries that already cover the same action and key. Entries are written verbatim into keybind= lines because the reload path matches a restored default by string, so the table spelling is the persisted spelling (cmd+…, modifier order as in canonical_combo). The keybind= format, the default_shortcuts() signature, the settings page contents and the legacy shell are unchanged.
  • Action::Quit maps to a new QuitApp action whose handler defers windowing::quit_all. It is registered in keyboard_bindings::init because nebula_app/src/gpui_shell/workspace.rs sits at its hard budget ratchet and cannot grow.
  • Dependency, data-format, threading, or lifetime changes: none. The only new persisted lines are the cleared keys themselves.
  • Compatibility and migration/fallback behavior: covered under Result; a user who wants the ⌘ half released again just clears the row once in this build.
  • Decision record: architecture/notes/nebula_app/gpui_shell/2026-09-21-macos-command-key-aliases.md — single authority, rejected alternatives, and the ⌘K clear-screen scope note.

Evidence / 验证依据

  • Commands and actual results (all on macOS 26 arm64, pinned toolchain, --locked):
    • cargo fmt --all -- --check → clean.
    • python3 scripts/check_architecture.py --base 3050374 → no errors; pre-existing NOTICEs only; no budget or allowance raised.
    • python3 scripts/check_platform_cfg.pyplatform cfg outside platform/: 484 (budget 484) (unchanged).
    • cargo test -p nebula --bin pebrel --features gpui-shell,gpui_platform/runtime_shaders --locked display::keymap → 14 passed.
    • cargo test -p nebula --bin pebrel --features gpui-shell,gpui-test-support,gpui_platform/runtime_shaders --locked keyboard_bindings → 7 passed (includes the tab-dispatch tests).
    • cargo test -p nebula-settings --locked → 67 passed. cargo test --manifest-path tools/i18n-contract/Cargo.toml --locked → 20 passed, 1 pre-existing ignored.
    • python3 -m unittest scripts.tests.test_architecture_budgets test_architecture_dependencies test_architecture_governance → 53 passed; ... test_pebrel_branding test_windows_resources test_app_icons test_ai_session_probe → 18 passed, 1 skipped.
  • Regression tests: what fails before the fix?
    • clearing_releases_the_macos_command_alias fails without the change with ⌘K 必须按表里的写法逐字写回,实际 [("Ctrl+K", "ReceiveChar")] — i.e. the settings write only releases Ctrl+K, exactly the reported bug.
    • macos_command_aliases_round_trip_through_storage pins that every alias entry parses back to the same combination, so a cleared key is never written as an unparsable line.
    • every_macos_command_alias_binds_a_gpui_action and released_cmd_k_is_swallowed_by_no_action pin the shell wiring: the table has a binding, and the injected no-action binding suppresses the static default at Root and terminal depth.
    • super_modifier_renders_per_platform pins the per-platform keycap.
  • UI changes: screenshots, long translations, keyboard access, DPI checks: manual acceptance with a debug build of this branch, driving the real window (isolated PEBREL_CONFIG_DIR): default ⌘K opens the Shell picker → clearing the row writes Ctrl+K:ReceiveChar + cmd+k:ReceiveChar and both ⌘K and Ctrl+K stop opening it without a restart → restoring the row brings ⌘K back → ⌘Q exits and session.json records clean_exit: true; the keymap page shows Ctrl+Cmd+F for Fullscreen (was Ctrl+Win+F). No layout, translation or DPI surface is touched by this change.
  • Hot-path changes: none. default_shortcuts() runs only for the keymap page and keybind reload; the macOS branch adds one linear pass over a 25-entry table.
  • Unrun checks: Linux and Windows suites were not exercised locally (the alias table is test data there; the runtime path stays behind cfg(target_os = "macos")). There is no end-to-end test for ⌘Q — its handler runs the asynchronous draft-approval and session-save path, so it was verified in the real window instead.

Required Review / 必须确认

  • I followed CONTRIBUTING.md, docs/architecture.md, and docs/project-constraints.md; the decision is recorded under architecture/notes/.
  • I split responsibilities, not arbitrary line ranges; no duplicate behavior authority was added — the duplicated macOS ⌘ list was removed and replaced by one table.
  • python3 scripts/check_architecture.py --base 3050374 passes; budgets were not inflated to fit the change.
  • Tests cover success and failure; platform/feature coverage limitations are stated above.
  • New messages use typed i18n IDs and matching placeholders; untranslated content has an explicit fallback. (Not applicable: no user-facing message was added, comments only.)
  • Governance changes include a counterexample, corrected contract, tests, and a maintainer-reviewed decision. (Not applicable: no policy, gate or contract was changed.)

Checkboxes explain the review; they do not replace CI or maintainer approval.

@zhbjerry

zhbjerry commented Sep 21, 2026

Copy link
Copy Markdown
Author

CI status: the two red tests are pre-existing on main

Tests (ubuntu-24.04) and Tests (macos-26) fail on tests this PR does not touch, and the same tests already fail on main:

  • ai_hook::integrations::tests::generic_agent_binaries_require_a_matching_installationnebula_app/src/platform/agent_integrations.rs:178, Os { code: 21, kind: IsADirectory } in the tests::executable helper.
  • gpui_shell::workspace::details_panel::tests::file_path_edit_navigates_and_keeps_the_last_directory_on_error_or_escapenebula_app/src/gpui_shell/workspace/details_panel_tests.rs:78.

Evidence that they are not caused by this branch:

  • main at 30503743 fails with exactly these two tests: run 35597825341ubuntu-24.04, macos-26 and macos-26-intel are all red with the same names.
  • nebula_app/src/platform/agent_integrations.rs does not exist at this PR's base commit f7ca0ec (git cat-file -e f7ca0ec:nebula_app/src/platform/agent_integrations.rsfatal: path ... does not exist). The file arrives with later main commits, so the failure comes from the merge with current main, not from the diff here.
  • Another run on the internal branch fix/cwd-session-restore-20260921 (run 35596902833, head 927a2c42, merged as PR Integrate complete Pebrel 1.9.0 desktop and Windows session recovery #231) also fails on ubuntu-24.04 with the identical agent_integrations test.

Everything this PR changes is green in CI:

display::keymap::tests::clearing_releases_the_macos_command_alias ... ok
display::keymap::tests::macos_command_aliases_round_trip_through_storage ... ok
display::keymap::tests::super_modifier_renders_per_platform ... ok
gpui_shell::workspace::keyboard_bindings::tests::macos_command_keys::every_macos_command_alias_binds_a_gpui_action ... ok
gpui_shell::workspace::keyboard_bindings::tests::macos_command_keys::released_cmd_k_is_swallowed_by_no_action ... ok
gpui_shell::workspace::keyboard_bindings::tests::cleared_shortcut_reaches_terminal_and_can_be_restored_without_restart ... ok

Passing jobs on this head: architecture-contracts, Tests (windows-2022), Tests (windows-11-arm), Release workspace (macos-26), Release workspace (macos-26-intel). Tests (macos-26-intel) was still queued when I wrote this.

本地与 CI 的失败集一致,两个红色测试在 main 上同样失败,且其中一个所在的文件在本 PR 的 base commit 上还不存在;本 PR 触及的 display::keymapkeyboard_bindings 测试全部通过。

zhbjerry added 2 commits September 21, 2026 21:14
macOS 的 ⌘ 键此前由 gpui_shell 在配置表之外单独注册:既不进
default_shortcuts()(解绑/恢复不知道它们),也没有对应的 workspace 动作。
于是解绑 Shell 选择器只写回 Ctrl+K:ReceiveChar,静态 ⌘K 继续抢键(Kuddev#238),
⌘Q 则完全没有绑定。

- display/keymap.rs:新增 MACOS_COMMAND_ALIASES,作为那套 ⌘ 绑定的唯一权威;
  default_shortcuts() 在 macOS 上把它算进默认键(解绑与恢复同一口径),
  设置页展示仍按配置表的 Ctrl 键。
- gpui_shell/workspace/keyboard_bindings.rs:静态 ⌘ 绑定改为从该表派生;
  Action::Quit 接到与托盘退出同一条路径(先落盘会话与草稿,再停 PTY)。
- 展示层:super 键在 macOS 渲染成 Cmd+,不再出现 Ctrl+Win+F / Win+F。
跨层影子绑定(键位模型不知道壳自己注册的 ⌘ 键)是原因容易被遗忘的一类改动,
按 architecture/notes/AGENTS.md 记一条:单一权威、被否定方案、以及本次明确
不处理的「解绑后 ⌘K 清屏」。
@zhbjerry
zhbjerry force-pushed the fix/macos-command-keys-20260921 branch from f8b13b2 to 68e815e Compare September 21, 2026 13:16
@zhbjerry

zhbjerry commented Sep 21, 2026

Copy link
Copy Markdown
Author

Rebased onto main @ 3050374 and force-pushed; new head is 4d08497. The diff is unchanged (4 files, +312/−28) and re-verified after the rebase:

  • cargo fmt --all -- --check → clean.
  • python3 scripts/check_architecture.py --base 3050374 → no errors, pre-existing NOTICEs only.
  • python3 scripts/check_platform_cfg.py484 (budget 484), unchanged.
  • cargo test ... display::keymap → 14 passed; cargo test ... keyboard_bindings → 7 passed.

The two failing tests noted above (agent_integrations, details_panel) are now part of the checked-out base, so they are expected to stay red here until they are fixed on main.

@zhbjerry

zhbjerry commented Sep 21, 2026

Copy link
Copy Markdown
Author

Settled CI on the rebase head (rebase onto main @ 3050374), run 35604522645:

job result
architecture-contracts pass
Tests (windows-2022) pass
Tests (windows-11-arm) pass
Release workspace (macos-26) pass
Release workspace (macos-26-intel) pass
Tests (ubuntu-24.04) fail — agent_integrations.rs:178
Tests (macos-26) fail — agent_integrations.rs:178, details_panel_tests.rs:78
Tests (macos-26-intel) fail — agent_integrations.rs:178, details_panel_tests.rs:78

The three red jobs fail only in those two tests, both untouched here and both already red on main @ 3050374 before this branch existed. No test under display::keymap or gpui_shell::workspace::keyboard_bindings fails on any platform.

@zhbjerry
zhbjerry force-pushed the fix/macos-command-keys-20260921 branch 3 times, most recently from 95853ad to 4d08497 Compare September 22, 2026 06:48
@zhbjerry

Copy link
Copy Markdown
Author

Closing this in favour of a replacement PR opened from a rebuilt branch. The diff and the commits are unchanged (4 files, +312/−28); only the branch history differs.

@zhbjerry zhbjerry closed this Sep 22, 2026
@zhbjerry
zhbjerry deleted the fix/macos-command-keys-20260921 branch September 22, 2026 06:50
@zhbjerry
zhbjerry restored the fix/macos-command-keys-20260921 branch September 22, 2026 06:51
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.

[Bug] macOS:⌘K 解绑/改绑后仍是 Shell 选择器,且 ⌘Q 无法退出

1 participant