Bug 470256 - Multi-modifier Modifier-only shortcuts
Summary: Multi-modifier Modifier-only shortcuts
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kglobalaccel
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.249.0
Platform: Other Linux
: NOR wishlist
Target Milestone: ---
Assignee: fanzhuyifan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-25 13:57 UTC by Oded Arbel
Modified: 2024-03-27 15:41 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 6.1


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Oded Arbel 2023-05-25 13:57:40 UTC
SUMMARY
Using a multi-modifier shortcut is probably more useful than single modifier-only shortcut, if only because there are more of them, but the current implementation of modifier-only shortcuts only supports setting one modifier.

STEPS TO REPRODUCE
1. Open the system settings shortcuts configuration.
2. Add a new custom shortcut.
3. Press CTRL+SHIFT

OBSERVED RESULT
The shortcut display shows both buttons being pressed, but after release only one modifier is left showing. When approving the change, only the modifier that is left showing is needed to trigger the action.

EXPECTED RESULT
Both modifiers should be listed and required to trigger the action.

SOFTWARE/OS VERSIONS
Operating System: KDE neon Unstable Edition
KDE Plasma Version: 5.27.80
KDE Frameworks Version: 5.240.0
Qt Version: 6.5.0
Kernel Version: 6.2.0-20-generic (64-bit)
Graphics Platform: offscreen
Processors: 8 × Intel® Core™ i7-7820HQ CPU @ 2.90GHz
Memory: 31.2 GiB of RAM
Graphics Processor: Mesa Intel® HD Graphics 630

ADDITIONAL INFORMATION
It is interesting to note that the single modifier select from a combination of modifiers pressed, in the shortcut configuration dialog, is the first modifier to be *released*, which is weird - especially considering bug #464805.
Comment 1 Oded Arbel 2023-05-28 11:21:48 UTC
I think the problem is in kwin's `ModifierOnlyShortcuts` class. It uses `KeyEvent::modifiersRelevantForGlobalShortcuts()` - which to the best of my understanding supports detecting multiple modifiers, but it then casts it to `Qt::KeyboardModifier` which can only represent one modifier - https://invent.kde.org/plasma/kwin/-/blob/7fffe993/src/modifier_only_shortcuts.cpp#L52

Though I'm not sure that this is what the shortcuts KCM does (see https://bugs.kde.org/show_bug.cgi?id=464805#c5), it is probably still a good idea to fix this, and will likely also require changes to the config file parser at https://invent.kde.org/plasma/kwin/-/blob/7fffe993/src/options.cpp#L749 to be able to read syntax such as "ShiftControl=...".

Also, I think it would be a really good idea if this mechanism would allow to distinguish between physical keys - such as using "LeftShift" instead of any "Shift". This will make a solution to bug #470257 much more useful for people with more exotic group switching shortcuts, which xkbcommon supports - but this can't be implemented while relying on `Qt::KeyboardModifier` as the underlying data atom, as it doesn't make that distinction.
Comment 2 Andrey 2023-05-29 12:36:15 UTC
At this point, would you like try to fix it yourself, Oded?
Comment 3 Oded Arbel 2023-05-29 12:47:26 UTC
(In reply to Andrey from comment #2)
> At this point, would you like try to fix it yourself, Oded?

I can try to take a stab at it - though my free time is limited, so it will take a while.

If you have any ideas as to how one might approach a requirement to identify the physical modifier key that was pressed, I would love to hear them.
Comment 4 Andrey 2023-05-29 13:29:08 UTC
(In reply to Oded Arbel from comment #3)
> If you have any ideas as to how one might approach a requirement to identify
> the physical modifier key that was pressed, I would love to hear them.
What I see Qt itself doesn't support that, so we would have to do some stuff above that..
But not all at once, I think we should do iteratively.
Comment 5 Andrey 2023-05-29 13:43:37 UTC
On the other hand, if we rely on QKeySequence it might be a Qt restriction we can't actually overcome in Plasma..
But I'm not sure.
Comment 6 Bug Janitor Service 2024-02-20 06:58:41 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/kguiaddons/-/merge_requests/115
Comment 8 fanzhuyifan 2024-03-20 17:02:14 UTC
Git commit d20d58d4729fd94b3373b803de89054c2e2550de by Yifan Zhu.
Committed on 20/03/2024 at 17:00.
Pushed by fanzhuyifan into branch 'master'.

Enhance modifier-only shortcuts

Trigger modifier-only shortcuts trigger on release, and support multi-key
modifier-only shortcuts, which trigger only when all the modifiers are
released within some interval.
Related: bug 464805

M  +50   -19   src/globalshortcutsregistry.cpp
M  +15   -0    src/globalshortcutsregistry.h
M  +38   -1    src/sequencehelpers_p.cpp
M  +2    -0    src/sequencehelpers_p.h

https://invent.kde.org/plasma/kglobalacceld/-/commit/d20d58d4729fd94b3373b803de89054c2e2550de
Comment 9 fanzhuyifan 2024-03-20 17:36:14 UTC
This still needs the kguiaddons MR to be merged.
Comment 10 fanzhuyifan 2024-03-27 15:41:54 UTC
Git commit ccf3d654c36ef333e1302af75233a6119aca4920 by Yifan Zhu.
Committed on 27/03/2024 at 15:39.
Pushed by fanzhuyifan into branch 'master'.

recorder/KKeySequenceRecorderPrivate: support recording multi-key modifier-only shortcuts

Multi-key modifier-only shortcuts are registered if all modifiers are
released within a short interval. Pretty display the sequence in the
form of Mods + Key, without repetition.

M  +57   -0    autotests/kkeysequencerecordertest.cpp
M  +2    -0    autotests/kkeysequencerecordertest.h
M  +63   -24   src/recorder/kkeysequencerecorder.cpp

https://invent.kde.org/frameworks/kguiaddons/-/commit/ccf3d654c36ef333e1302af75233a6119aca4920