prefs/shortcuts: allow adding multiple shortcuts to single action#1918
Open
amezin wants to merge 1 commit into
Open
prefs/shortcuts: allow adding multiple shortcuts to single action#1918amezin wants to merge 1 commit into
amezin wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used🧠 Learnings (5)📚 Learning: 2026-01-05T05:06:57.054ZApplied to files:
📚 Learning: 2026-01-07T04:33:39.586ZApplied to files:
📚 Learning: 2026-02-19T03:46:35.526ZApplied to files:
📚 Learning: 2026-03-02T03:21:20.247ZApplied to files:
📚 Learning: 2026-02-22T09:39:19.850ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors the shortcut edit dialog to support distinct Replace and Add-Alternative actions, updates the dialog UI and key handling/validation, adjusts the ShortcutRow response handler to replace or append accelerators (removing conflicts and managing notifications), and documents the feature in CHANGELOG.md. ChangesMultiple Shortcuts per Action Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 23: Fix the spelling typo in CHANGELOG.md by replacing the word
"assinged" with "assigned" in the line that currently reads "Multiple shortcuts
can be assinged to the same action through Preferences".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8aa86777-68f7-4b87-9d5e-679fbbaa1ac3
📒 Files selected for processing (2)
CHANGELOG.mdddterm/pref/shortcuts.js
📜 Review details
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-05T05:06:57.054Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1665
File: ddterm/pref/shortcuts.js:256-262
Timestamp: 2026-01-05T05:06:57.054Z
Learning: In GNOME Shell Extensions using GTK via GJS, Gtk.Label and Gtk.ShortcutLabel cannot receive keyboard focus; calling grab_focus() on them has no effect. When reviewing code in this project (e.g., ddterm/pref/*.js), avoid relying on focus on these label widgets. If you need to manage focus, apply it to a focusable widget (like an Entry or a button), or move the focus handling to a container that can receive focus. Do not call grab_focus() on Gtk.Label or Gtk.ShortcutLabel, and verify any focus-related logic targets appropriate widgets.
Applied to files:
ddterm/pref/shortcuts.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: Ensure GTK3/GTK4 compatibility wrappers exist in the ddterm/pref/ directory so that modules can be loaded by the GNOME Shell extensions service (GTK4). This applies to all JavaScript files under ddterm/pref (e.g., ddterm/pref/*.js).
Applied to files:
ddterm/pref/shortcuts.js
📚 Learning: 2026-02-19T03:46:35.526Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/terminalpage.js:116-119
Timestamp: 2026-02-19T03:46:35.526Z
Learning: In the ddterm/gnome-shell-extension-ddterm repository, in GJS-based signal definitions (param_types), it is acceptable to use JavaScript primitives like String and Boolean directly. Do not suggest replacing them with GObject.TYPE_STRING or GObject.TYPE_BOOLEAN. This guidance applies across JavaScript files in this project (not just this file) where signal parameter types rely on native JS constructors.
Applied to files:
ddterm/pref/shortcuts.js
📚 Learning: 2026-03-02T03:21:20.247Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1774
File: ddterm/app/appwindow.js:41-47
Timestamp: 2026-03-02T03:21:20.247Z
Learning: In GJS-based code, EXPLICIT_NOTIFY suppresses automatic notify only for properties with manually defined getters/setters. If a property has EXPLICIT_NOTIFY but no manually defined setter, GJS will generate a setter that calls notify() when the value changes. Apply this understanding to ddterm's JavaScript files to avoid assuming no notify emission for such properties; verify whether a manual setter exists before deciding if notify() should be emitted or suppressed.
Applied to files:
ddterm/pref/shortcuts.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: For the ddterm GNOME Shell extension, ensure all design decisions and code reviews assume GNOME Shell as the only supported environment. Do not introduce runtime or UI assumptions for other desktops; verify compatibility with GNOME Shell APIs and extension environment in all JavaScript files under the ddterm directory (e.g., ddterm/app/appwindow.js).
Applied to files:
ddterm/pref/shortcuts.js
🪛 LanguageTool
CHANGELOG.md
[grammar] ~23-~23: Ensure spelling is correct
Context: ...]: [#1915]. - Multiple shortcuts can be assinged to the same action through Preferences ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (6)
CHANGELOG.md (1)
32-34: LGTM!ddterm/pref/shortcuts.js (5)
162-164: LGTM!
213-231: LGTM!
266-289: LGTM!
385-389: LGTM!
390-403: Add Alternative logic correctly implements conflict-free append.The freeze/thaw pattern correctly consolidates multiple value updates (remove_conflict may update value, then append updates again). The remove_conflict call before append ensures no duplicates exist within the same shortcut row.
One minor note: there's no defensive validation that
acceleratoris non-null before parsing, but this is safe in practice because:
- Backspace explicitly triggers RESPONSE_REPLACE, not ADD_ALT (line 268)
- Both buttons are disabled unless the accelerator is valid (lines 284-285)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddterm/pref/shortcuts.js`:
- Around line 245-247: The Add button label uses gettext('Add') but should match
the feature contract "Add Alternative"; update the call in the add_button
invocation to use this.gettext_domain.gettext('Add Alternative') (the same place
that references ShortcutEditDialog.RESPONSE_ADD) so the UI copy is consistent
with the PR and expected UX text.
- Around line 309-312: When input becomes invalid after switching the stack to
'accept', the stack still shows the accept child and only buttons are disabled;
update the code handling validity (the block that calls
this.#stack.set_visible_child_name('accept')) to also reset the stack to the
appropriate invalid view (e.g., call
this.#stack.set_visible_child_name('invalid' or the default child) when valid is
false) and ensure
get_widget_for_response(ShortcutEditDialog.RESPONSE_SET).grab_focus() is only
invoked for the valid branch; modify the logic around set_visible_child_name and
get_widget_for_response/RESPONSE_SET so the visible child reflects current
validity rather than leaving stale accept guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8dbd28ed-dc57-4e8d-890e-96c162fdcad3
⛔ Files ignored due to path filters (1)
docs/screenshots/shortcut-edit.pngis excluded by!**/*.png
📒 Files selected for processing (2)
CHANGELOG.mdddterm/pref/shortcuts.js
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: ci / bundle
- GitHub Check: ci / lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-05T05:06:57.054Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1665
File: ddterm/pref/shortcuts.js:256-262
Timestamp: 2026-01-05T05:06:57.054Z
Learning: In GNOME Shell Extensions using GTK via GJS, Gtk.Label and Gtk.ShortcutLabel cannot receive keyboard focus; calling grab_focus() on them has no effect. When reviewing code in this project (e.g., ddterm/pref/*.js), avoid relying on focus on these label widgets. If you need to manage focus, apply it to a focusable widget (like an Entry or a button), or move the focus handling to a container that can receive focus. Do not call grab_focus() on Gtk.Label or Gtk.ShortcutLabel, and verify any focus-related logic targets appropriate widgets.
Applied to files:
ddterm/pref/shortcuts.js
📚 Learning: 2026-01-07T04:33:39.586Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1667
File: ddterm/app/tablabel.js:10-10
Timestamp: 2026-01-07T04:33:39.586Z
Learning: Ensure GTK3/GTK4 compatibility wrappers exist in the ddterm/pref/ directory so that modules can be loaded by the GNOME Shell extensions service (GTK4). This applies to all JavaScript files under ddterm/pref (e.g., ddterm/pref/*.js).
Applied to files:
ddterm/pref/shortcuts.js
📚 Learning: 2026-02-19T03:46:35.526Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1766
File: ddterm/app/terminalpage.js:116-119
Timestamp: 2026-02-19T03:46:35.526Z
Learning: In the ddterm/gnome-shell-extension-ddterm repository, in GJS-based signal definitions (param_types), it is acceptable to use JavaScript primitives like String and Boolean directly. Do not suggest replacing them with GObject.TYPE_STRING or GObject.TYPE_BOOLEAN. This guidance applies across JavaScript files in this project (not just this file) where signal parameter types rely on native JS constructors.
Applied to files:
ddterm/pref/shortcuts.js
📚 Learning: 2026-03-02T03:21:20.247Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1774
File: ddterm/app/appwindow.js:41-47
Timestamp: 2026-03-02T03:21:20.247Z
Learning: In GJS-based code, EXPLICIT_NOTIFY suppresses automatic notify only for properties with manually defined getters/setters. If a property has EXPLICIT_NOTIFY but no manually defined setter, GJS will generate a setter that calls notify() when the value changes. Apply this understanding to ddterm's JavaScript files to avoid assuming no notify emission for such properties; verify whether a manual setter exists before deciding if notify() should be emitted or suppressed.
Applied to files:
ddterm/pref/shortcuts.js
📚 Learning: 2026-02-22T09:39:19.850Z
Learnt from: amezin
Repo: ddterm/gnome-shell-extension-ddterm PR: 1770
File: ddterm/app/appwindow.js:583-592
Timestamp: 2026-02-22T09:39:19.850Z
Learning: For the ddterm GNOME Shell extension, ensure all design decisions and code reviews assume GNOME Shell as the only supported environment. Do not introduce runtime or UI assumptions for other desktops; verify compatibility with GNOME Shell APIs and extension environment in all JavaScript files under the ddterm directory (e.g., ddterm/app/appwindow.js).
Applied to files:
ddterm/pref/shortcuts.js
🔇 Additional comments (1)
CHANGELOG.md (1)
25-26: LGTM!Also applies to: 34-34, 37-37
Extend shortcut editor to allow either overriding existing shortcut(s), or adding an extra shortcut without removing existing ones. Fixes #932
amezin
changed the title
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Looks good. |
|
The only question is why allow multiple shortcuts for the same action? User replaces a default shortcut with what they want should be sufficient... |
Member
Author
Users asked for it multiple times. Also #1915 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extend shortcut editor to allow either overriding existing shortcut(s), or adding an extra shortcut without removing existing ones.
Fixes #932