Fix Control key not being recorded when setting keyboard shortcuts on macOS - #16643
Conversation
|
Hey @krishna01989! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide. Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures. |
PR Summary by QodoRecord Control modifier when capturing keyboard shortcuts on macOS
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
github-actions
Bot
added
status: changes-required
|
Your pull request conflicts with the target branch. Please merge |
github-actions
Bot
added
component: preferences
component: keybinding
status: no-bot-comments
and removed
component: keybinding
status: changes-required
|
|
||
| @Test | ||
| void controlKeyCombinedWithShiftIsRecordedAsBindingOnMacOs() { | ||
| assumeTrue(OS.OS_X); |
There was a problem hiding this comment.
use @EnabeldOnOS(OS.Mac) here and in the other tes
There was a problem hiding this comment.
This is fixed in the latest commit.
koppor
added
status: changes-required
github-actions
Bot
added
component: keybinding
status: no-bot-comments
status: changes-required
… macOS The keybinding preferences UI checked isShortcutDown(), isShiftDown(), and isAltDown() when capturing a new shortcut, but never isControlDown(). On macOS, Control is a separate modifier from Shortcut (Cmd), so pressing Control plus any key was silently ignored and no binding was recorded. Windows/Linux are unaffected since Control is already their Shortcut key, so isControlDown() and isShortcutDown() fire together there; the fix is guarded to only add the extra Control handling on macOS to avoid duplicating the modifier. Fixes JabRef#16604
… macOS The keybinding preferences UI checked isShortcutDown(), isShiftDown(), and isAltDown() when capturing a new shortcut, but never isControlDown(). On macOS, Control is a separate modifier from Shortcut (Cmd), so pressing Control plus any key was silently ignored and no binding was recorded. Windows/Linux are unaffected since Control is already their Shortcut key, so isControlDown() and isShortcutDown() fire together there; the fix is guarded to only add the extra Control handling on macOS to avoid duplicating the modifier. Also fixes upgradeKeyBindingsToJavaFX, a startup migration that unconditionally rewrote any "ctrl+" substring to "shortcut+". This predates Control being a distinct modifier and was only ever meant to convert the legacy space-separated format (e.g. "ctrl A"); left as-is, it would have silently rewritten our new intentional "ctrl+" bindings back into "shortcut+" bindings on every app restart. The migration now only touches strings still in the legacy space-separated format. Fixes JabRef#16604
Switched the macOS-only tests from a manual assumeTrue(OS.OS_X) check to JUnit 5's declarative @EnabledOnOs(OS.MAC), per maintainer review. Also shortened the CHANGELOG.md entry to fit the template's updated one-sentence, max-20-words guideline.
19db0c6 to
db30a60
Compare
github-actions
Bot
added
component: keybinding
and removed
component: keybinding
labels
github-actions
Bot
added
component: keybinding
status: no-bot-comments
and removed
component: keybinding
status: changes-required
github-actions
Bot
added
the
status: to-be-merged
3 participants
Summary
The keybinding preferences UI checked isShortcutDown(), isShiftDown(), and isAltDown() when capturing a new shortcut, but never isControlDown(). On macOS, Control is a separate modifier from Shortcut (Cmd), so pressing Control plus any key was silently ignored and no binding was recorded.
Windows/Linux are unaffected since Control is already their Shortcut key, so isControlDown() and isShortcutDown() fire together there; the fix is guarded to only add the extra Control handling on macOS to avoid duplicating the modifier.
jabref-contrib-policy:4.2:reviewed:okSteps to test
NOTE: Some Control+Command combinations are reserved by macOS itself (e.g. Control+Command+D triggers the system dictionary lookup) and won't reach the app at all — this is expected OS behavior, not a bug in this fix.
Related issues and pull requests
Closes #16604
AI usage
AIL0
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)