fix(keybinding): resolve shortcut conflicts and improve Wayland support - #1088
Closed
robertkill wants to merge 0 commit into
Closed
robertkill wants to merge 0 commit into
robertkill wants to merge 0 commit into
Conversation
Reviewer's GuideThis PR fixes shortcut conflicts and improves Wayland/KWin integration by adding an API-modification guard, normalizing/special-casing system accelerators for KWin, synchronizing dconfig updates to KWin, tightening single-keystroke replacement behavior, and enhancing logging and diagnostics around keybinding operations. Sequence diagram for API shortcut modification with dconfig guardsequenceDiagram
actor ControlCenter
participant Manager
participant StringSet
participant ShortcutManager
participant DConfig
ControlCenter->>Manager: AddShortcutKeystroke(id, type0, keystroke)
Manager->>StringSet: Add(id)
activate StringSet
StringSet-->>Manager: (ok)
deactivate StringSet
Manager->>ShortcutManager: GetByIdType(id, type0)
ShortcutManager-->>Manager: shortcut
Manager->>ShortcutManager: ModifyShortcutKeystrokes(shortcut, maybeNil)
Manager->>ShortcutManager: AddShortcutKeystroke(shortcut, ksToAdd...)
Manager->>ShortcutManager: SaveKeystrokes via shortcut
Note over DConfig,Manager: DConfig emits change signal after SaveKeystrokes
DConfig-->>Manager: listenDConfigChanged(key=id, type0)
Manager->>StringSet: Has(id)
activate StringSet
StringSet-->>Manager: true
deactivate StringSet
Manager-->>DConfig: skip handling (return)
Manager->>StringSet: Remove(id)
activate StringSet
StringSet-->>Manager: (ok)
deactivate StringSet
ControlCenter-->>Manager: AddShortcutKeystroke returns
Sequence diagram for dconfig-driven system shortcut sync to KWin on WaylandsequenceDiagram
participant DConfig
participant Manager
participant ShortcutManager
participant NormalizeFn
participant KWinWm
DConfig-->>Manager: listenDConfigChanged(key, type0=ShortcutTypeSystem)
Manager->>Manager: check enableListenDConfig
Manager->>Manager: check modifyingShortcuts.Has(key)
alt notBeingModified
Manager->>ShortcutManager: GetByIdType(key, type0)
ShortcutManager-->>Manager: shortcut
Manager->>DConfig: Value(0, key)
DConfig-->>Manager: keystrokesRaw
Manager->>ShortcutManager: ModifyShortcutKeystrokes(shortcut, ParseKeystrokes(keystrokesRaw))
opt Wayland
Manager->>NormalizeFn: NormalizeSystemKeystrokesForKWin(key, keystrokesRaw)
NormalizeFn-->>Manager: normalizedKeystrokes
Manager->>Manager: marshal KWinAccel(Id, normalizedKeystrokes) to accelJson
Manager->>KWinWm: SetAccel(0, accelJson)
KWinWm-->>Manager: ok, err
end
Manager->>Manager: emitShortcutSignal(shortcutSignalChanged, shortcut)
else beingModifiedViaAPI
Manager-->>DConfig: skip (return)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The hardcoded KWin accelerators for
screenshotWindow,launcher, andsystemMonitorare now duplicated acrossNormalizeSystemKeystrokesForKWin,AddSystemToKwin, andsetAccelForWayland; consider centralizing these special cases in a single helper to avoid divergence or future inconsistencies. - The literal
"<Crtl><Alt>Escape"for the system monitor accelerator appears to contain a typo (CrtlvsCtrl); please double-check this against the actual KWin format to ensure the shortcut is recognized correctly. - Several new
Infoflogs (e.g. inlistenGlobalAccel,setShortForWayland,setAccelForWayland, andDeleteShortcutKeystroke) may be quite verbose in normal operation; consider downgrading high-frequency paths toDebugfor guarding them to avoid noisy logs in production.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hardcoded KWin accelerators for `screenshotWindow`, `launcher`, and `systemMonitor` are now duplicated across `NormalizeSystemKeystrokesForKWin`, `AddSystemToKwin`, and `setAccelForWayland`; consider centralizing these special cases in a single helper to avoid divergence or future inconsistencies.
- The literal `"<Crtl><Alt>Escape"` for the system monitor accelerator appears to contain a typo (`Crtl` vs `Ctrl`); please double-check this against the actual KWin format to ensure the shortcut is recognized correctly.
- Several new `Infof` logs (e.g. in `listenGlobalAccel`, `setShortForWayland`, `setAccelForWayland`, and `DeleteShortcutKeystroke`) may be quite verbose in normal operation; consider downgrading high-frequency paths to `Debugf` or guarding them to avoid noisy logs in production.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
TAG Bot New tag: 6.1.86 |
Contributor
|
TAG Bot New tag: 6.1.87 |
Contributor
|
TAG Bot New tag: 6.1.88 |
Contributor
|
TAG Bot New tag: 6.1.89 |
Contributor
|
TAG Bot New tag: 6.1.90 |
Contributor
|
TAG Bot New tag: 6.1.91 |
Contributor
|
TAG Bot New tag: 6.1.92 |
Contributor
|
TAG Bot New tag: 6.1.93 |
Contributor
|
TAG Bot New tag: 6.1.94 |
Contributor
|
TAG Bot New tag: 6.1.95 |
Contributor
|
TAG Bot New tag: 6.1.96 |
Contributor
|
TAG Bot New tag: 6.1.97 |
Contributor
|
TAG Bot New tag: 6.1.98 |
Contributor
|
TAG Bot New tag: 6.1.99 |
Contributor
|
TAG Bot New tag: 6.1.100 |
Contributor
|
TAG Bot New tag: 6.1.101 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: robertkill The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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.
Problem:
Solution:
Influence:
fix: 修复快捷键冲突和 Wayland 支持问题
问题：
解决方案：
影响范围：
PMS: BUG-355747
Summary by Sourcery
Resolve shortcut modification conflicts and improve Wayland shortcut synchronization with KWin, including special-case handling for system shortcuts.
Bug Fixes:
Enhancements: