Rewrite of the global shortcuts kcm
ClosedPublic

Authored by davidre on Apr 11 2020, 12:04 PM.

Details

Summary

This is a total rewrite of the global shortcuts kcm from scratch. It uses an
abstract item model backend with a qml frontend. This resolves some issues the
old kcm inherently had because it just stacked KShortcutEditors. First it enables
a global serach/filter which either matches the component name, actions, default
or set key combinations across components. Secondly KGlobalAccel can have multiple
default/active key combinations for each action - the old kcm only exposed two each.
The new kcm displays all default or set key combinations.
The main visual structure is similiar to the old kcm. On the left there is a list
with all components but rather than divided into "Application Launchers" and
"Other Shortcuts" the sections are now called "Applications" and "System Services"
(cf. notifications kcm) and the components are now assigned whether the service
we find for a component is an application or not rather if the component was
added via .desktop file. In the main view the shortcuts are displayed. Each item
corresponds to one action. Initially each item is collapsed and shows the action
name and a list of the currently set key combinations for that action. In the
expanded form all default shortcuts are shown which can be activated or
deactivated and all other active shortcuts ("Custom Shortcuts"). It is possible
to change, remove or add new custom shortcuts.
The kcm and model communicate directly with the daemon over DBus. This removes
the need to awkwardly construct actions to pass to the KGlobalAccel API.

BUG: 157468
BUG: 213101
BUG: 230583
BUG: 250121
BUG: 251437
BUG: 272554
BUG: 318964
BUG: 341817
BUG: 348264
BUG: 366257
BUG: 369020
BUG: 388574
BUG: 393403
BUG: 408942
BUG: 416149
BUG: 416737
BUG: 417915
BUG: 419215
BUG: 419515
BUG: 419624
BUG: 419692
BUG: 419825
BUG: 419909
BUG: 420093
FIXED-IN: 5.19.0

Closes T7267

Test Plan

kcmshell5 kcm_keys

Diff Detail

Repository
R119 Plasma Desktop
Branch
kcmkeys2 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25465
Build 25483: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
davidre updated this revision to Diff 80224.Apr 15 2020, 6:49 PM
  • Add section checkbox
davidre marked 2 inline comments as done.Apr 15 2020, 6:50 PM
davidre updated this revision to Diff 80387.Apr 17 2020, 1:21 PM
  • Remove components as good as the current kcm
davidre marked an inline comment as done.Apr 17 2020, 1:22 PM
davidre edited the test plan for this revision. (Show Details)Apr 17 2020, 1:33 PM

This is looking really good. Other than my previous comments (most of which have been resolved, yay) I have just nitpicks left, really.

kcms/keys/package/contents/ui/ShortcutActionDelegate.qml
166

This tooltip is not working for me

kcms/keys/shortcutsmodel.cpp
166

Clicking the Defaults button does not seem to have any effect for me

davidre updated this revision to Diff 80398.Apr 17 2020, 2:55 PM
  • Fix defaults
davidre updated this revision to Diff 80405.Apr 17 2020, 3:39 PM
  • Use less DBus calls for loading
davidre added inline comments.Apr 17 2020, 3:39 PM
kcms/keys/shortcutsmodel.cpp
39

This actually crashes now

davidre marked an inline comment as done.Apr 17 2020, 3:40 PM
davidre updated this revision to Diff 80553.Apr 19 2020, 3:22 PM
  • Make loading async
  • Make everything async
ngraham added inline comments.Apr 19 2020, 7:52 PM
kcms/keys/shortcutsmodel.cpp
440

compile error

davidre updated this revision to Diff 80620.Apr 20 2020, 8:29 AM

Make it build again

davidre updated this revision to Diff 80622.Apr 20 2020, 8:33 AM
  • Add tooltip to remove toolbutton
davidre marked 4 inline comments as done.Apr 20 2020, 8:34 AM
davidre updated this revision to Diff 80629.Apr 20 2020, 9:57 AM
  • Fix index reset when clicking reset button and use own property for shortcuts listview

currentIndex was reset to 0 when resetting, now warnings are generated when switching components need to figure out why

davidre marked an inline comment as done.Apr 20 2020, 9:59 AM
davidre updated this revision to Diff 80631.Apr 20 2020, 10:00 AM

Use Nate's string

Clicking the Apply button makes System Settings crash for me:

1#0 QExplicitlySharedDataPointer<QDBusPendingCallPrivate>::operator= (o=...,
2 this=this@entry=0x7fffffffb960)
3 at ../../include/QtCore/../../src/corelib/tools/qshareddata.h:204
4#1 QDBusPendingCall::operator= (this=this@entry=0x7fffffffb960, other=...)
5 at qdbuspendingcall.cpp:290
6#2 0x00007ffff5a46375 in QDBusPendingReplyData::assign (this=this@entry=0x7fffffffb960,
7 other=...) at qdbuspendingreply.cpp:260
8#3 0x00007fffd40f5888 in QDBusPendingReply<void, void, void, void, void, void, void, void>::assign (call=..., this=0x7fffffffb960)
9 at /usr/include/qt5/QtDBus/qdbuspendingreply.h:195
10#4 QDBusPendingReply<void, void, void, void, void, void, void, void>::operator= (
11 call=..., this=0x7fffffffb960) at /usr/include/qt5/QtDBus/qdbuspendingreply.h:141
12#5 QDBusPendingReply<void, void, void, void, void, void, void, void>::QDBusPendingReply
13 (call=..., this=0x7fffffffb960) at /usr/include/qt5/QtDBus/qdbuspendingreply.h:135
14#6 ShortcutsModel::<lambda()>::operator() (__closure=0x2e2d8c0)
15 at /home/nate/kde/src/plasma-desktop/kcms/keys/shortcutsmodel.cpp:156
16#7 QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, ShortcutsModel::save()::<lambda()> >::call (arg=<optimized out>, f=...)
17 at /usr/include/qt5/QtCore/qobjectdefs_impl.h:146
18#8 QtPrivate::Functor<ShortcutsModel::save()::<lambda()>, 0>::call<QtPrivate::List<>, void> (arg=<optimized out>, f=...) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:256
19#9 QtPrivate::QFunctorSlotObject<ShortcutsModel::save()::<lambda()>, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void **, bool *) (
20 which=<optimized out>, this_=0x2e2d8b0, r=<optimized out>, a=<optimized out>,
21 ret=<optimized out>) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:439
22#10 0x00007ffff57329fe in QtPrivate::QSlotObjectBase::call (a=0x7fffffffbb10,
23 r=0x7fffe4013820, this=0x2e2d8b0)
24 at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394
25#11 doActivate<false> (sender=0x27e5570, signal_index=3, argv=0x7fffffffbb10)
26 at kernel/qobject.cpp:3870
27#12 0x00007ffff572d1bf in QMetaObject::activate (sender=<optimized out>,
28 m=m@entry=0x7ffff5a635e0 <QDBusPendingCallWatcher::staticMetaObject>,
29 local_signal_index=local_signal_index@entry=0, argv=argv@entry=0x7fffffffbb10)
30 at kernel/qobject.cpp:3930
31#13 0x00007ffff5a44a5f in QDBusPendingCallWatcher::finished (this=<optimized out>,
32 _t1=<optimized out>) at .moc/moc_qdbuspendingcall.cpp:158
33#14 0x00007ffff5a44b60 in QDBusPendingCallWatcherPrivate::_q_finished (
34 this=<optimized out>) at qdbuspendingcall.cpp:495
35#15 QDBusPendingCallWatcher::qt_static_metacall (_o=<optimized out>, _c=<optimized out>,
36 _id=<optimized out>, _a=<optimized out>) at .moc/moc_qdbuspendingcall.cpp:86
37#16 0x00007ffff572aa19 in QObject::event (this=0x27e5570, e=0x253c020)
38 at kernel/qobject.cpp:1339
39#17 0x00007ffff62a3caf in QApplicationPrivate::notify_helper (this=this@entry=0x474710,
40 receiver=receiver@entry=0x27e5570, e=e@entry=0x253c020)
41 at kernel/qapplication.cpp:3684
42#18 0x00007ffff62acdf0 in QApplication::notify (this=0x7fffffffc1e0, receiver=0x27e5570,
43 e=0x253c020) at kernel/qapplication.cpp:3430
44#19 0x00007ffff56fe002 in QCoreApplication::notifyInternal2 (receiver=0x27e5570,
45--Type <RET> for more, q to quit, c to continue without paging--
46 event=0x253c020) at ../../include/QtCore/../../src/corelib/kernel/qobject.h:153
47#20 0x00007ffff5700794 in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0,
48 event_type=0, data=0x455ef0) at kernel/qcoreapplication.cpp:1832
49#21 0x00007ffff5755573 in postEventSourceDispatch (s=s@entry=0x542fe0)
50 at kernel/qeventdispatcher_glib.cpp:277
51#22 0x00007ffff3cc7048 in g_main_dispatch (context=0x7fffec005000)
52 at ../glib/gmain.c:3216
53#23 g_main_context_dispatch (context=context@entry=0x7fffec005000)
54 at ../glib/gmain.c:3881
55#24 0x00007ffff3cc73d0 in g_main_context_iterate (context=context@entry=0x7fffec005000,
56 block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
57 at ../glib/gmain.c:3954
58#25 0x00007ffff3cc745f in g_main_context_iteration (context=0x7fffec005000,
59 may_block=may_block@entry=1) at ../glib/gmain.c:4015
60#26 0x00007ffff5754bee in QEventDispatcherGlib::processEvents (this=0x541c50, flags=...)
61 at kernel/qeventdispatcher_glib.cpp:423
62#27 0x00007ffff56fcb9b in QEventLoop::exec (this=this@entry=0x7fffffffc0e0, flags=...,
63 flags@entry=...) at ../../include/QtCore/../../src/corelib/global/qflags.h:136
64#28 0x00007ffff5704972 in QCoreApplication::exec ()
65 at ../../include/QtCore/../../src/corelib/global/qflags.h:118
66#29 0x000000000040d7f5 in main (argc=<optimized out>, argv=<optimized out>)
67 at /home/nate/kde/src/systemsettings/app/main.cpp:101

kcms/keys/package/contents/ui/main.qml
119

When I click on it, the KCM enters this state:

121

How about "Remove all shortcuts for [model.name]"

168

Make it a level 3 Heading

174

For some reason this list view doesn't get any sctollbars when it's scrollable. Also it implements kinetic scrolling while the other list view doesn't

davidre updated this revision to Diff 80672.Apr 20 2020, 5:32 PM
  • Fix errors and leave error message disabled
davidre marked an inline comment as done.Apr 20 2020, 5:34 PM

Clicking the Apply button makes System Settings crash for me:

Fixed now.

davidre updated this revision to Diff 80673.Apr 20 2020, 5:49 PM
  • Fix scrollview and use level 3 heading
  • use better string
davidre marked 3 inline comments as done.Apr 20 2020, 5:49 PM

Yay. Almost there! I just see a few more things:

  1. If I delete a whole item from the left-most list, there's no obvious way to get it back (what if I delete a system entry by accident?)
  2. The Defaults button is present, but always disabled
  3. I see duplicate entries with different items inside them:
davidre updated this revision to Diff 80675.Apr 20 2020, 6:05 PM
  • Implement Kai's idea for importing
  • Fix importing
davidre marked an inline comment as done.Apr 20 2020, 6:06 PM
GB_2 added a subscriber: GB_2.Apr 20 2020, 6:08 PM

Nitpick: put the "Add Application..." button on the left (T10384).

Yay. Almost there! I just see a few more things:

  1. If I delete a whole item from the left-most list, there's no obvious way to get it back (what if I delete a system entry by accident?)

I agree. Probably should do it like other kcms with pending deletion then.

  1. The Defaults button is present, but always disabled

Hmm it works for me when changing shortcuts or changing defaults but the initial state is wrong probably because loading is now async. Will look into it.

  1. I see duplicate entries with different items inside them:

These are not really duplicated but boil down to that the backend is represented correctly here. Some components have a different unique identifier and happen to have the same user visible name. The old kcm merged them but I don't as I think most of these should be seperate because they are related to different things.

  • The kded ones are not really duplicate but rather seperate. I think most of them are part of plasma(?) so I will see if I can change them to something more sensible (for example "Touchpad" or "Keyboard").
  • The powerdevil duplication shouldn't even exist, @davidedmundson messed up while migrating the shortcuts but also fixed that. See D10668.
  • The run command one I have not seen but they also have different user visible names? How does the old kcm handle that?

Here's how it looks with the old KCM:

So my KDE Daemon category has one entry in French for some odd reason. And the two KRunner entries are indeed still there and different, not merged.

And the Power Management entry is full of duplicates internally and it's very confusing:

So yeah, it's not great in the existing KCM. In the new one it's nice that you can delete these duplicates, but I don't actually know what will happen when I do so, and the inability to get them back if I make a mistake makes me not want to try.

Here's how it looks with the old KCM:

So my KDE Daemon category has one entry in French for some odd reason. And the two KRunner entries are indeed still there and different, not merged.

And the Power Management entry is full of duplicates internally and it's very confusing:

So yeah, it's not great in the existing KCM. In the new one it's nice that you can delete these duplicates, but I don't actually know what will happen when I do so, and the inability to get them back if I make a mistake makes me not want to try.

In the old one you can delete them too.

davidre updated this revision to Diff 80767.Apr 21 2020, 2:25 PM
  • Fix defaults
  • Add pending deletion thing
ngraham accepted this revision.Apr 21 2020, 5:06 PM

Boom.

This revision is now accepted and ready to land.Apr 21 2020, 5:06 PM
ngraham edited the summary of this revision. (Show Details)Apr 23 2020, 2:33 AM

JFYI once this is ready to land, before doing so, please do an arc amend locally to pick up the changes in the description that I've made and ensure that all those bugs get closed!

Also you can now use Kirigami.PlaceholderMessage in this diff instead of making the messages manually using level 3 Headings.

davidre updated this revision to Diff 81016.Apr 23 2020, 4:29 PM
  • Don't drop installing scheme files
davidre updated this revision to Diff 81304.Apr 27 2020, 7:25 AM
  • Use model.* properties
  • Use placeholder message
  • Still don't know why model properties are undefined when switching components
ngraham accepted this revision.Apr 27 2020, 3:20 PM

UI approved :)

davidre updated this revision to Diff 81435.Apr 28 2020, 1:49 PM
  • Make the delegate use states
davidre updated this revision to Diff 81441.Apr 28 2020, 3:36 PM
  • Reinstate the pointingHand mouseAreas
  • Make delegate automatically expand if it's the only one

I find that I'm not able to set shortcuts involving the space key. The key sequence get repeated twice in the button and the Apply button ever becomes enabled. Can you reproduce?

I find that I'm not able to set shortcuts involving the space key. The key sequence get repeated twice in the button and the Apply button ever becomes enabled. Can you reproduce?

That's the current behavior of KeySequenceItem and KKeySequenceWidget. You should be able to observe it everywhere such an item is used

I find that I'm not able to set shortcuts involving the space key. The key sequence get repeated twice in the button and the Apply button ever becomes enabled. Can you reproduce?

That's the current behavior of KeySequenceItem and KKeySequenceWidget. You should be able to observe it everywhere such an item is used

Ah, I can indeed reproduce that with other instances of that in QML software (e.g. applet config windows' shortcut pages). I think we need that fixed before this lands as otherwise it would be a regression compared to the existing KCM.

ngraham accepted this revision.Apr 30 2020, 2:28 PM

My spacebar shortcut issue will be fixed with D29292. I think this deserves to get into Plasma 5.19, and the sooner the better so there's no rush before the deadline.

This revision was automatically updated to reflect the committed changes.