Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gugullll 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 |
Reviewer's GuidePrevents shortcut-consumed keys such as Alt+Tab from being advertised to the newly focused client during wl_keyboard.enter, while swallowing their later releases to avoid stuck keys and orphan key-release events. Sequence diagram for shortcut-consumed key focus switchingsequenceDiagram
participant Keyboard
participant Helper
participant ShortcutManager
participant WSeat
participant Client
Keyboard->>Helper: KeyPress(nativeVirtualKey)
Helper->>WSeat: suppressKey(evdevKeycode)
Helper->>ShortcutManager: dispatchKeyEvent(kevent)
ShortcutManager->>WSeat: setKeyboardFocusWindow(newWindow)
WSeat->>WSeat: filterSuppressedKeycodes(keycodes, suppressedKeycodes, visibleKeycodes)
WSeat->>Client: wl_keyboard.enter(visibleKeycodes)
Keyboard->>Helper: KeyRelease(nativeVirtualKey)
Helper->>WSeat: isKeySuppressed(evdevKeycode)
Helper->>WSeat: unsuppressKey(evdevKeycode)
Helper-->>Keyboard: swallow release
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/seat/helper.cpp" line_range="2823-2826" />
<code_context>
+ // the shortcut table (e.g. a modifier was released first). The client
+ // never saw that key pressed, so forwarding its release would hand it an
+ // orphan release.
+ const uint32_t evdevKeycode = kevent->nativeVirtualKey();
+ if (kevent->type() == QEvent::KeyRelease && seat->isKeySuppressed(evdevKeycode)) {
+ seat->unsuppressKey(evdevKeycode);
+ return true;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The synthetic auto-repeat release generated by `WSeatPrivate` clears suppression for a shortcut-consumed key even though the physical key is still held. Subsequent focus changes can therefore include that still-held key in `wl_keyboard.enter`, recreating the stuck-key/orphan-state bug.
**Triggers:** When a consumed shortcut key is held long enough for the seat's auto-repeat timer to generate its press/release pair.
**Suggested fix:** Do not treat auto-repeat releases as the physical release that clears suppression; clear the entry only on the real key release, or track physical key state separately.
</issue_to_address>
### Comment 2
<location path="src/seat/helper.cpp" line_range="2818-2826" />
<code_context>
}
}
+ // A release for a key whose press was consumed by a registered shortcut
+ // must be swallowed too, no matter whether the combination still matches
+ // the shortcut table (e.g. a modifier was released first). The client
+ // never saw that key pressed, so forwarding its release would hand it an
+ // orphan release.
+ const uint32_t evdevKeycode = kevent->nativeVirtualKey();
+ if (kevent->type() == QEvent::KeyRelease && seat->isKeySuppressed(evdevKeycode)) {
+ seat->unsuppressKey(evdevKeycode);
+ return true;
+ }
+
</code_context>
<issue_to_address>
**issue (broader_impact):** The early Meta/Super handling returns before `suppressKey` runs, so a shortcut using Super/Meta does not mark that modifier as consumed. If the shortcut switches focus, the modifier remains in the keyboard enter key set even though its press was consumed, and its release can be swallowed as an orphan-prevention release.
**Triggers:** When a registered shortcut uses `Qt::Key_Meta`, `Qt::Key_Super_L`, or `Qt::Key_Super_R` and synchronously changes keyboard focus.
**Suggested fix:** Integrate the Meta/Super path with the suppression bookkeeping, marking the modifier press before any shortcut-triggered focus change and clearing it on the corresponding physical release.
</issue_to_address>| const uint32_t evdevKeycode = kevent->nativeVirtualKey(); | ||
| if (kevent->type() == QEvent::KeyRelease && seat->isKeySuppressed(evdevKeycode)) { | ||
| seat->unsuppressKey(evdevKeycode); | ||
| return true; |
There was a problem hiding this comment.
issue (bug_risk): The synthetic auto-repeat release generated by WSeatPrivate clears suppression for a shortcut-consumed key even though the physical key is still held. Subsequent focus changes can therefore include that still-held key in wl_keyboard.enter, recreating the stuck-key/orphan-state bug.
Triggers: When a consumed shortcut key is held long enough for the seat's auto-repeat timer to generate its press/release pair.
Suggested fix: Do not treat auto-repeat releases as the physical release that clears suppression; clear the entry only on the real key release, or track physical key state separately.
| // A release for a key whose press was consumed by a registered shortcut | ||
| // must be swallowed too, no matter whether the combination still matches | ||
| // the shortcut table (e.g. a modifier was released first). The client | ||
| // never saw that key pressed, so forwarding its release would hand it an | ||
| // orphan release. | ||
| const uint32_t evdevKeycode = kevent->nativeVirtualKey(); | ||
| if (kevent->type() == QEvent::KeyRelease && seat->isKeySuppressed(evdevKeycode)) { | ||
| seat->unsuppressKey(evdevKeycode); | ||
| return true; |
There was a problem hiding this comment.
issue (broader_impact): The early Meta/Super handling returns before suppressKey runs, so a shortcut using Super/Meta does not mark that modifier as consumed. If the shortcut switches focus, the modifier remains in the keyboard enter key set even though its press was consumed, and its release can be swallowed as an orphan-prevention release.
Triggers: When a registered shortcut uses Qt::Key_Meta, Qt::Key_Super_L, or Qt::Key_Super_R and synchronously changes keyboard focus.
Suggested fix: Integrate the Meta/Super path with the suppression bookkeeping, marking the modifier press before any shortcut-triggered focus change and clearing it on the corresponding physical release.
A key matching a registered shortcut is consumed by shortcut handling and never reaches the client. Exclude such keys from wl_keyboard.enter and swallow their release unconditionally, so the client never learns the key was held and never receives an orphan release. 命中已注册快捷键的按键会被快捷键处理消费,不会下发给客户端。 因此 enter 的按键集合需剔除这类按键,并无条件吞掉其 release; 客户端既不会误判该键按下,也不会收到孤立 release。 Log: 修复 Alt+Tab 切换后客户端持续收到 Tab 的问题 Influence: Alt+Tab 等快捷键切换焦点时,客户端不再从 enter 得知被快捷键消费的 按键,且这些按键的 release 无条件镜像吞掉,不会出现孤立 release。
52e4d58 to
c6ea16b
Compare
A key matching a registered shortcut is consumed by shortcut handling and never reaches the client. Exclude such keys from wl_keyboard.enter and swallow their release unconditionally, so the client never learns the key was held and never receives an orphan release.
命中已注册快捷键的按键会被快捷键处理消费,不会下发给客户端。
因此 enter 的按键集合需剔除这类按键,并无条件吞掉其 release;
客户端既不会误判该键按下,也不会收到孤立 release。
Log: 修复 Alt+Tab 切换后客户端持续收到 Tab 的问题
Influence: Alt+Tab 等快捷键切换焦点时,客户端不再从 enter 得知被快捷键消费的 按键,且这些按键的 release 无条件镜像吞掉,不会出现孤立 release。
Summary by Sourcery
Keep keys consumed by shortcut handling out of client keyboard state during focus transitions.
Bug Fixes:
Enhancements: