Fix PowerDevil shortcuts migration
ClosedPublic

Authored by davidedmundson on Feb 19 2018, 8:02 PM.

Details

Summary

5.11 -> 5.12 migrated shortcuts from kded5 to the correct component name
org_kde_powerdevil for good reasons
However, despite a kconfupdate script working correctly and moving the
old keys, kglobalaccel is running whilst we update and it syncs the old
values later. On future loads it detects the duplication and removes the
powerdevil entries.

This patch removes the old powermanagent entries in the kded5 component
at powerdevil startup which is at runtime where we can talk to
kglobalaccel, but before powerdevil has registered the new actions.

This should handle both users that haven't upgraded to 5.12 yet, as well
as users who hit the previous config script.

Ideally kconf_update needs to be patched to do migration at appropriate
times.

BUG: 389991
FIXED-IN: 5.12.2

Test Plan

"broke" my config by putting in values in the old place with a custom shortcut for poweroff
Restarted kglobalaccel
Ran powerdevil
Shortucts worked
Cleanly quit kglobalaccel
Config stayed synced

Diff Detail

Repository
R122 Powerdevil
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson created this revision.Feb 19 2018, 8:02 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 19 2018, 8:02 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Feb 19 2018, 8:02 PM
cfeck added a subscriber: cfeck.Feb 19 2018, 9:48 PM

Regarding the comments, I prefer a space after '//' for better readability.

daemon/powerdevilapp.cpp
169

Missing space after 'for'.

170

Please move brace to previous line.

aacid added a subscriber: aacid.Feb 19 2018, 11:21 PM
aacid added inline comments.
daemon/powerdevilapp.cpp
177

Won't this return QList<QKeySequence>() since you've set it on the line just before?

daemon/powerdevilapp.cpp
177

You'd think so, but no.

setShortcut(QAction, sequence, KGlobalAccel::NoAutoloading)
would do exactly that

setShortcut(QAction, sequence, KGlobalAccel::Autoloading)
(which this defaults to)
will make KGlobalAccel load whatever it has stored already in its config file.

fvogt added a subscriber: fvogt.Feb 20 2018, 9:34 AM
fvogt added inline comments.
daemon/powerdevilapp.cpp
177

Then use ::Autoloading explicitly?

broulik accepted this revision.Feb 20 2018, 12:08 PM
broulik added a subscriber: broulik.

lgtm

shipit once you addressed the other comments

This revision is now accepted and ready to land.Feb 20 2018, 12:08 PM
davidedmundson marked 2 inline comments as done.

Changes

This revision was automatically updated to reflect the committed changes.