Fix shortcut capture and modifier-only hotkeys#261
Conversation
|
@codex review |
![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: 70f773952c
ℹ️ 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".
| var pressedModifierKeyCodes = self.pressedModifierKeyCodes | ||
| if pressedModifierKeyCodes.contains(keyCode) { | ||
| pressedModifierKeyCodes.remove(keyCode) | ||
| } else { | ||
| // Modifier released | ||
| let wasCleanPress = !self.otherKeyPressedDuringModifier | ||
| self.modifierOnlyKeyDown = false | ||
| self.otherKeyPressedDuringModifier = false | ||
| self.modifierPressStartTime = nil | ||
| pressedModifierKeyCodes.insert(keyCode) |
There was a problem hiding this comment.
Updating pressedModifierKeyCodes by toggling membership on each flagsChanged event can desynchronize the internal state whenever one modifier event is missed (for example during event-tap disable/re-enable or while shortcut capture bypasses handling). Once out of sync, modifier-only shortcuts that rely on exact key-code sets will stop firing correctly because press/release transitions are interpreted backward until restart. Build this set from the current eventModifiers/event state each time (or otherwise resync) rather than flip-flopping cached membership.
Useful? React with 👍 / 👎.
|
Added a follow-up fix in This clears stale modifier tracking when shortcut capture is active, when the event tap is disabled/recovered, and when the hotkey manager is reinitialized, so missed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02813185d2
ℹ️ 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".
|
|
||
| let reason = (type == .tapDisabledByTimeout) ? "timeout" : "user input" | ||
| DebugLogger.shared.warning("Event tap disabled by \(reason) — attempting immediate re-enable", source: "GlobalHotkeyManager") | ||
| self.resetModifierOnlyShortcutTracking() |
There was a problem hiding this comment.
Calling resetModifierOnlyShortcutTracking() on tap-disable clears isKeyPressed/isCommandModeKeyPressed/etc. immediately, but it does not stop an already-running press-and-hold recording. The release handlers only call stopRecordingIfNeeded() when those booleans are still true, so if the tap is disabled while a modifier-only hold shortcut is active, releasing the key after recovery is ignored and recording can stay stuck on until the user manually toggles it off.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 543eecd.
Reset paths now stop an active hold recording before clearing the pressed-state booleans, so tap-disable / reinit / shortcut-capture reset cannot leave a modifier-only hold recording stuck on.
| guard self.modifierOnlyKeyDown || behavior.isModeKeyPressed(), | ||
| expectedModifierKeyCodes.contains(keyCode), | ||
| !pressedModifierKeyCodes.contains(keyCode) | ||
| else { | ||
| return false |
There was a problem hiding this comment.
For multi-modifier shortcuts, when the user presses an additional modifier not in the configured set (for example configured ⌘+⌥, then pressing ⇧ before completing a chord), this path returns false without marking otherKeyPressedDuringModifier or cancelling pendingHoldModeStart. That leaves the delayed hold task alive, so the mode can still start after 150ms even though the user transitioned into a larger key combo.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 543eecd.
Pending hold-start is now cancelled when the pressed modifier set grows beyond the configured chord, so adding an extra modifier no longer allows a stale delayed hold-start to fire.
|
Pushed follow-up hardening in This removes the remaining fragile path in modifier-only tracking by rebuilding side-specific modifier state from macOS live key state on modifier changes instead of relying on cached toggle history. It also keeps the earlier fixes to stop active hold recording before reset and to cancel pending hold-start when extra modifiers join the chord. Verified with:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 543eecd665
ℹ️ 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".
| guard self.modifierOnlyKeyDown || behavior.isModeKeyPressed(), | ||
| expectedModifierKeyCodes.contains(keyCode), | ||
| !pressedModifierKeyCodes.contains(keyCode) |
There was a problem hiding this comment.
modifierOnlyKeyDown is global across all modifier-only shortcuts, so this release guard can fire for a different shortcut than the one that actually started tracking. With overlapping shortcuts (e.g., dictation = ⌘, prompt = ⌘+⌥), pressing and releasing ⌘ can enter the prompt release path first, consume the event, and skip dictation’s release cleanup/toggle path, leaving mode state inconsistent and triggering the wrong action. The release path should also verify that this behavior is the active one (for example via pendingHoldModeType/per-behavior down state) before acting.
Useful? React with 👍 / 👎.
1 participant
Description
Fixes shortcut capture ownership and modifier-only hotkey handling.
Type of Change
Related Issues
Testing
swiftlint --strict --config .swiftlint.yml Sourcesswiftformat --config .swiftformat Sourcessh build_incremental.shNotes
0b30897(changed paste method naming).Screenshots / Video