Bug 442079 - Activity hotkeys are not stored properly
Summary: Activity hotkeys are not stored properly
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kglobalaccel
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.85.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: kdelibs bugs
URL:
Keywords:
Depends on: 445277
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-06 14:10 UTC by Oleg Solovyov
Modified: 2021-12-17 17:32 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: Plasma 5.24 with Frameworks 5.90


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Solovyov 2021-09-06 14:10:25 UTC
STEPS TO REPRODUCE
1. Create an activity
2. Set hotkey to "Alt+B, Alt+F, Alt+G"
3. Save activity
4. Open activity settings

OBSERVED RESULT
Hotkey is stored as "Alt+B", other keystrokes are eaten

EXPECTED RESULT
Hotkey is "Alt+B, Alt+F, Alt+G"

SOFTWARE/OS VERSIONS
Linux/KDE Plasma: 5.11.0-27-generic (64 bit)
KDE Plasma Version: 5.22.5
KDE Frameworks Version: 5.85.0
Qt Version: 5.15.3

ADDITIONAL INFORMATION
When I set hotkey back to "Alt+B, Alt+F, Alt+G", "Conflict With Registered Global Shortcut" is shown.
Comment 1 Oleg Solovyov 2021-09-07 07:59:36 UTC
qWarning() << "scResult debug:" << activeShortcut << intListFromShortcut(activeShortcut) << shortcutFromIntList(intListFromShortcut(activeShortcut));

causes this:
scResult debug: (QKeySequence("Alt+B, Alt+F, Alt+G")) (134217794) (QKeySequence("Alt+B"))
Comment 2 Oleg Solovyov 2021-09-07 09:12:07 UTC
The issue is in KGlobalAccelPrivate::intListFromShortcut:

According to QKeySequence documentation it could contain up to 4 keys.[1]

> for (const QKeySequence &sequence : cut) {
>     ret.append(sequence[0]);
> }
Hence, in this code all keys except the first one are lost.

[1] https://doc.qt.io/qt-5/qkeysequence.html#QKeySequence-2
Comment 3 David Edmundson 2021-09-07 12:39:16 UTC
Good investigation.

Lets just set a limit on our input prompt to stop when we hit 4 keys then, so it's at least clear to the user.
Comment 4 Oleg Solovyov 2021-09-07 13:05:00 UTC
(In reply to David Edmundson from comment #3)
> Good investigation.
> 
> Lets just set a limit on our input prompt to stop when we hit 4 keys then,
> so it's at least clear to the user.

limit is already set, I can't set more than 4 keys (with modifiers, etc.)
The problem is that only first one is stored.

e.g. if I set something like "Alt+B, Alt+F, Alt+G" it will store only "Alt+B", silently omitting other keys.

Currently we're using int <-> QKeySequence which will definitely cause sequence truncation.
I'd suggest using one of the following:
int        <---> QKeySequence (dumb impl)
quint128 ? <---> QKeySequence (each key [1] with modifiers [2] uses 32 bits, use a 128bit type if exist)
int        <-X-> QKeySequence (avoid conversion and transfer )

[1] https://doc.qt.io/qt-5/qt.html#Key-enum
[2] https://doc.qt.io/qt-5/qt.html#KeyboardModifier-enum
Comment 5 Oleg Solovyov 2021-09-07 13:09:42 UTC
(In reply to Oleg Solovyov from comment #4)
> (In reply to David Edmundson from comment #3)
> > Good investigation.
> > 
> > Lets just set a limit on our input prompt to stop when we hit 4 keys then,
> > so it's at least clear to the user.
> 
> limit is already set, I can't set more than 4 keys (with modifiers, etc.)
> The problem is that only first one is stored.
> 
> e.g. if I set something like "Alt+B, Alt+F, Alt+G" it will store only
> "Alt+B", silently omitting other keys.
> 
> Currently we're using int <-> QKeySequence which will definitely cause
> sequence truncation.
> I'd suggest using one of the following:
> int        <---> QKeySequence (dumb impl)
> quint128 ? <---> QKeySequence (each key [1] with modifiers [2] uses 32 bits,
> use a 128bit type if exist)
> int        <-X-> QKeySequence (avoid conversion and transfer )
> 
> [1] https://doc.qt.io/qt-5/qt.html#Key-enum
> [2] https://doc.qt.io/qt-5/qt.html#KeyboardModifier-enum

Another idea, conversion based on QKeySequence::{from,to}String()
Comment 6 Oleg Solovyov 2021-09-08 08:56:07 UTC
made a patch [1] but it's WIP yet: actions still aren't invoked when count > 1

[1] https://invent.kde.org/osolovyov/kglobalaccel/-/commit/5d79966be20b8df79c25f3fd9559dd5387f1c1f9
Comment 7 Oleg Solovyov 2021-09-09 12:00:33 UTC
https://invent.kde.org/osolovyov/kglobalaccel/-/commits/work/avoid-convert-keyseq

Almost done.

Not Implemented Yet:
Inexact matches:
1) New sequence will be shadowed by existing one
2) New sequence will shadow existing one

bugfixes:
Don't ungrab key still to be grabbed
Comment 8 Bug Janitor Service 2021-09-10 09:05:10 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/kdeclarative/-/merge_requests/81
Comment 9 Oleg Solovyov 2021-09-10 09:20:34 UTC
Another merge request not caught by bug janitor:
(edited description, added bug number)

https://invent.kde.org/frameworks/kglobalaccel/-/merge_requests/27