refactor(shortcuts)!: rewrite keyboard handling on a single menu-driven authority with hardware key codes#1556
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09f4fcf365
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if legacyKey.count == 1, let character = legacyKey.first { | ||
| resolvedKeyCode = KeyboardLayout.keyCode(for: character) |
There was a problem hiding this comment.
When decoding old settings that were recorded with Shift (for example Cmd+Shift+[ was persisted by the previous charactersIgnoringModifiers path as key: "{", shift: true), this migration asks KeyboardLayout.keyCode(for: "{"). That lookup only indexes unshifted/base characters, so it returns nil and compactMapValues drops the user's override, silently reverting or clearing customized shifted-symbol shortcuts after upgrade. Please normalize shifted legacy glyphs like {/} back to their base physical keys before dropping the entry.
Useful? React with 👍 / 👎.
…en authority with hardware key codes
datlechin
force-pushed
the
refactor/keyboard-shortcuts
branch
from
09f4fcf to
c33b87c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c33b87cf82
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static let reservedAppShortcuts: [(key: BoundKey, name: String)] = { | ||
| var shortcuts: [(key: BoundKey, name: String)] = [ | ||
| (.character("=", command: true), String(localized: "Zoom In")), | ||
| (.character("-", command: true), String(localized: "Zoom Out")) | ||
| ] |
There was a problem hiding this comment.
When a user records built-in app menu equivalents such as ⌘Q, ⌘H, ⌘M, or ⌘,, this reserved list does not catch them, and SystemHotkeyChecker only reads macOS symbolic hotkeys rather than the app’s standard menu items. The resolver therefore accepts those bindings, but AppKit already has those key equivalents in the application menu, so the customized action will either be shadowed by Quit/Hide/Minimize/Settings or create an ambiguous duplicate shortcut.
Useful? React with 👍 / 👎.
… line-cut edge tests
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55f1d6eee6
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if settings.isCustomized(action) { | ||
| Button { | ||
| settings.resetToDefault(for: action) |
There was a problem hiding this comment.
When an action has been cleared or changed because another action took its default shortcut, this reset button restores the default without running the same conflict checks used for recording. For example, after reassigning action B to action A's default and clearing A via the conflict dialog, pressing A's reset button creates two active actions with the same key in overlapping contexts, so one binding is shadowed/ambiguous despite the settings UI normally preventing this.
Useful? React with 👍 / 👎.
1 participant
Summary
Rewrites the keyboard shortcut path so there is one dispatch authority and one binding model. Shortcuts are stored as hardware key codes instead of characters, so custom bindings work on any keyboard layout and shifted symbols record correctly. Conflict detection now knows the full shortcut space (other actions, live macOS system shortcuts, built-in editor commands, and the app's reserved tab/zoom keys).
Root cause
The old system had two uncoordinated layers:
KeyCombo-> SwiftUI menu key-equivalents, read fromKeyboardSettings.NSEventlocal monitors andkeyDownoverrides that fire before menu key-equivalents (per the documentedsendEventorder) and never consultKeyboardSettings.Because layer 2 ran first, rebinding Copy or Cut changed the menu but not the editor, the data-grid Delete ignored its binding, and pagination keys were eaten by the editor. On top of that,
KeyCombostoredcharactersIgnoringModifiers, which is layout-dependent and records{forShift+[, so re-recording a default could stop matching.What changed
BoundKeymodel stores{keyCode, modifiers}. Matching compares key codes (layout-independent), and display strings are derived throughUCKeyTranslate.KeyboardSettings, contextualized by focus. A newShortcutContext(global / editor / dataGrid) lets the same combo mean different things in the editor and the grid, soCmd+[pages the grid and indents the editor without a conflict.Cmd+C/Cmd+Xmonitor. Copy and Cut now route through the responder chain, so custom bindings apply. TheCmd+Xline-cut moved into the text view'scut(_:)so it stays a responder-chain action.Find Next/Find Previousare wired.CopySymbolicHotKeys()for live system shortcuts (replacing a hand-maintained list), a built-in editor registry, and an app-reserved registry (Cmd+1-Cmd+9, zoom), and treats editor vs grid as separate contexts.KeyCombosettings migrate toBoundKeyon load; the override dictionary stays keyed by action id.Default shortcut changes (breaking)
Cmd+Nnow opens a new connection. Manage Connections keeps its File menu item.Ctrl+1/Ctrl+2(they switch macOS Spaces) move toCmd+Option+1/Cmd+Option+2.Cmd+Option+Up/Cmd+Option+Down.Cmd+G/Cmd+Shift+G).Cmd+[/Cmd+]andCmd+Fare unchanged; they are now context-scoped.Testing
KeyCombo->BoundKeymigration.swiftlint lint --strictclean on changed files.Please verify after building
Function keys and #1489
BoundKeynow treats function keys (F1-F12) as first-class: the recorder captures them with or without a modifier, they render asF5and register as menu key-equivalents, and they survive sanitization. There are no function-key defaults yet.This overlaps with open PR #1489 (F5/F9/F1 shortcuts), which adds a separate
FunctionKeyShortcutMonitor. If this PR lands first, #1489 can drop the monitor and add its function-key bindings asShortcutActiondefaults plus menu items, keeping a single dispatch authority. The two PRs also touch the same files (KeyboardShortcutModels,KeyboardSettingsView, tests, docs), so a merge order should be agreed.