Port to KGlobalAccel
ClosedPublic

Authored by davidre on Feb 25 2019, 5:15 PM.

Details

Summary

Port to KGlobalAccel. This enables us to drop khotkeys and display a configuration
dialog inside the application. The update script correctly sets the defaults and migrates
possibly user changed shortcuts. I didn't know where to place the KActionCollection
and put it in SpectacleConfig for the moment.

FEATURE: 388592
FIXED-IN: 19.08.0

Test Plan

Shortcuts should work as before.

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidre created this revision.Feb 25 2019, 5:15 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptFeb 25 2019, 5:15 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
davidre requested review of this revision.Feb 25 2019, 5:15 PM
davidre edited the summary of this revision. (Show Details)
ngraham awarded a token.

Sorry, I feel I should have been clearer before I let you put in the time.

KGlobalAccel::self()->setGlobalShortcut(....)
As you noted this method it won't work when spectacle isn't running, and can't ever work with the current design.

Quite recently, in an attempt to move on killing khotkeys, kgloablacceld gained support for these "jump list" actions - which spectacle uses.
It sort of works already, there's an entry in the global shortcuts list already and it works if manually set.

Jumplists have the advantage that it can spawn a process (either by the Exec line, or in future via DBus with org.freedesktop.Application or ActivateAction - though that needs a bit of work)
Perfect for this sort of thing.

We don't want to add code that will create a second conflicting entry in kglobalaccel.

We need to migrate both existing setups and new defaults to use the jump lists instead.
Migration is always difficult and I'm not sure kglobalaccel even has a system for having defaults for jump list actions.


As for the code that adds a UI for modifying shortcuts. that totally makes sense. kglobalacceld is exporting the jump list actions, it might need some tricks to get these actions through the normal public API.
KGlobalAcceld does seem to be exporting the jump list actions, so it should be do-able. Might need some tricks to do it.

ngraham edited the summary of this revision. (Show Details)Feb 25 2019, 6:15 PM
ngraham requested changes to this revision.Feb 25 2019, 6:25 PM
ngraham added a subscriber: ngraham.

Also need to remove the .khotkeys business from desktop/CMakeLists.txt and ExtraDesktop.sh

This revision now requires changes to proceed.Feb 25 2019, 6:25 PM

Sorry, I feel I should have been clearer before I let you put in the time.

No problem, probably I should have asked what you mean before I started doing stuff.

KGlobalAccel::self()->setGlobalShortcut(....)
As you noted this method it won't work when spectacle isn't running, and can't ever work with the current design.

Quite recently, in an attempt to move on killing khotkeys, kgloablacceld gained support for these "jump list" actions - which spectacle uses.
It sort of works already, there's an entry in the global shortcuts list already and it works if manually set.

Jumplists have the advantage that it can spawn a process (either by the Exec line, or in future via DBus with org.freedesktop.Application or ActivateAction - though that needs a bit of work)
Perfect for this sort of thing.

We don't want to add code that will create a second conflicting entry in kglobalaccel.

We need to migrate both existing setups and new defaults to use the jump lists instead.
Migration is always difficult and I'm not sure kglobalaccel even has a system for having defaults for jump list actions.

Are you refering to the actions in the desktop file with jump list actions? Can you point me to the entry in the Global Shortcut section?


As for the code that adds a UI for modifying shortcuts. that totally makes sense. kglobalacceld is exporting the jump list actions, it might need some tricks to get these actions through the normal public API.
KGlobalAcceld does seem to be exporting the jump list actions, so it should be do-able. Might need some tricks to do it.

What do you propose as the way forward? I feel this might be a little over my head

Something like:

https://phabricator.kde.org/P336

which is based on your patch

Are you refering to the actions in the desktop file with jump list actions? Can you point me to the entry in the Global Shortcut section?

Go to the global shortcuts KCM
click on the +, then find spectacle.

It should list 5 actions under Spectacle in Application Launchers.

Is it there by default? If not that's another issue we need to work out how to fix.

Something like:

https://phabricator.kde.org/P336

which is based on your patch

Thanks a lot for you effort! I will try to do it this way

Are you refering to the actions in the desktop file with jump list actions? Can you point me to the entry in the Global Shortcut section?

Go to the global shortcuts KCM
click on the +, then find spectacle.

It should list 5 actions under Spectacle in Application Launchers.

Is it there by default? If not that's another issue we need to work out how to fix.

Yes I can add it the way you described

davidre updated this revision to Diff 52584.Feb 26 2019, 8:16 AM

Match the actions the desktop file
Like this? I used setGlobalShortcut instead of setDefaultShortccut because the shortcuts weren't active by default and documentation says that it sets active and default shortcut. However this still only works for me for _launch.
On the positive side if Spectacle is run the jump list actions get added to the Global Shortcuts list.

Getting there!

Most of the shortcuts don't appear in the editor:

Also the list should be a bit taller, since we have enough room in the window to show all the contents without a scrollbar.

Finally, in addition to the defaults, we'll probably want a kconf update script to migrate the existing khotkeys shortcuts for users who had previously changed them. See https://techbase.kde.org/Development/Tools/Using_kconf_update

this still only works for me for _launch.

Weird.
If you want to move further on investigating this, I would suggest using a tool called "bustle" (if it's not in your distro it's available in flatpak), it's a DBus monitor/recorder

On the positive side if Spectacle is run the jump list actions get added to the Global Shortcuts list.

\o/
I think it shows the concept works.

Before shipping we need to somehow make this work out of the box before starting spectacle the first time.

There's no infrastructure to do that currently, nor really a full plan of how it's going to work together.
Hacking on this is going to be a somewhat hard task across lots of different areas.

davidre added a comment.EditedFeb 26 2019, 4:56 PM

Getting there!

Most of the shortcuts don't appear in the editor:

That was what I meant by they're not activated. If you select one the global shortcut should be there but not selected. But what's weird is that it only works for you for "Capture Current Screen" and for me it only works for _launch. I guess David will probalys know why.

Also the list should be a bit taller, since we have enough room in the window to show all the contents without a scrollbar.

Fully agree.

Finally, in addition to the defaults, we'll probably want a kconf update script to migrate the existing khotkeys shortcuts for users who had previously changed them. See https://techbase.kde.org/Development/Tools/Using_kconf_update

We also have to figure something out for having the shortcuts before the user opened Spectacle for the first time.

this still only works for me for _launch.

Weird.
If you want to move further on investigating this, I would suggest using a tool called "bustle" (if it's not in your distro it's available in flatpak), it's a DBus monitor/recorder

On the positive side if Spectacle is run the jump list actions get added to the Global Shortcuts list.

\o/
I think it shows the concept works.

Before shipping we need to somehow make this work out of the box before starting spectacle the first time.

There's no infrastructure to do that currently, nor really a full plan of how it's going to work together.
Hacking on this is going to be a somewhat hard task across lots of different areas.

Thanks for the tip. I will also try it on a another user to see if it works there for yet another shourtcut.

@ngraham: The Current Monitor action was not included in the khotkeys file before. I gave it the shift+alt screenshot because it was free.

davidre added a comment.EditedFeb 27 2019, 10:12 AM

I tried it with a new user and there every shortcut was activated. I guess there were some leftovers from the Khotkeys configuration in GlobalAccel. The only difference in the DBus-calls was that the first calls for the non-active shortcuts returned 0 instead of the set value. I attached file. @ngraham, maybe you could try it with a new user, that has never seen the KHotkeys stuff, too. Because of this getting the conf update right seems even more important. I thought about this and my idea boils down to

  1. oldfile = khotkeysrc, where the user configured shortcuts have been stored
  2. in a script searching through khotkeysrc and finding the Data_X entry corresponding to spectacle
  3. deleting everything in khotkeysrc related to spectacle
  4. triggering a reread of khotkeysrc
  5. (Optional;testing required) unregisterinthe shortcuts over dbus from GlobalAccel (if the steps above weren't sufficient)
  6. Set shortcuts for the jumplist actions with qdbus to the user configured values from khotkeysrc

This could be also an entry for the initial configuration before spectacle is run. I think every user should have a khotkeysrc. Then the update script would be run at the initial setup wouldn't it? If so we can set the shortcuts to the default values if nothing is found in khotkeysrc.

davidre updated this revision to Diff 52726.EditedFeb 27 2019, 2:19 PM

Use VBoxLayout instead of Formlayout
We don't really need a FormLayout on the Shortcutspage. And this is a easy fix to have the shortcutseditor to fill the space.

(Optional;testing required) unregisterinthe shortcuts over dbus from GlobalAccel (if the steps above weren't sufficient)

They're not. It's very frustrating.
If you mod the config of kglobalaccel5 whilst it's running when it quits it saves all the entries it knows, which then means you get duplicates in kglobalaccelrc, this super breaks.

I found this out by accidentally breaking all shortcuts in powerdevil one release.

Fixing kconfupdate (somehow) might be a solution.

davidre added a comment.EditedFeb 27 2019, 2:32 PM

(Optional;testing required) unregisterinthe shortcuts over dbus from GlobalAccel (if the steps above weren't sufficient)

They're not. It's very frustrating.
If you mod the config of kglobalaccel5 whilst it's running when it quits it saves all the entries it knows, which then means you get duplicates in kglobalaccelrc, this super breaks.

I found this out by accidentally breaking all shortcuts in powerdevil one release.

Fixing kconfupdate (somehow) might be a solution.

But I don't want to mod kglobalaccelrc, my idea was to mod khotkeysrc and use qdbus to interact with kglobalaccel (removing/adding shortcuts). Wouldn't that work?

davidre updated this revision to Diff 53127.EditedMar 4 2019, 2:37 PM

Conf update
First I tried with a shell script but apparently it is impossible to send array of integers with qdbus ("can't pass arg of type ai").
I tested it with two new users, one with the default shortcut and one with changed shortcuts both got migrated and activated.
However there seems no real action to be associated with the actions yet. Nothing happens because kglobalaccel sends an empty array to klauncher.

Also what would be the correct CMake variables to install the update files?

On the positive side if Spectacle is run the jump list actions get added to the Global Shortcuts list.

\o/
I think it shows the concept works.

I think this never really worked only because I added the desktop file before manually. If I create a new user and run spectacle the shourtcuts get added but they do nothing

Also what would be the correct CMake variables to install the update files?

You can roughly copy what Gwenview does: https://cgit.kde.org/gwenview.git/tree/kconf_update

For everything else, probably @davidedmundson is gonna have to be your point of contact.

kconfupdate is fine for migration. That code looks somewhat sensible.

It shouldn't be our long term correct way to initially set things approach, and that needs designing before moving forward.

I suspect we'll need kglobalaccel to search for default shortcuts through the installed directories to find defaults.

Maybe I should move this to a phab task, but I've done some looking:

Currently kglobalaccel searches

const QStringList desktopPaths = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, QStringLiteral("kglobalaccel"), QStandardPaths::LocateDirectory);

So our first step from spectacle POV is we need to copy/symlink our main .desktop file there.

This second directory is so that kglobalaccel doesn't have to search through all application .desktop files on startup. Kinda makes sense.
The KCM looks like it needs some code to handle this case correctly. It's weirdly copying the file

This will put the entries in the KCM/globalaccel without the user having to manually add it.

As for the default shortcut

We want in our .desktop file

[Desktop Action FullScreenScreenShot]
Name=Capture Entire Desktop
X-KDE-Shortcuts=Control+P <--- new entry

This is loaded by frameworks/kserviceactioncomponent.cpp though I don't know if it works.
(it definitely has a bug that it doesn't read X-KDE-Shortcuts for the default action)

Maybe I should move this to a phab task, but I've done some looking:

Currently kglobalaccel searches

const QStringList desktopPaths = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, QStringLiteral("kglobalaccel"), QStandardPaths::LocateDirectory);

So our first step from spectacle POV is we need to copy/symlink our main .desktop file there.

This second directory is so that kglobalaccel doesn't have to search through all application .desktop files on startup. Kinda makes sense.
The KCM looks like it needs some code to handle this case correctly. It's weirdly copying the file

This will put the entries in the KCM/globalaccel without the user having to manually add it.

As for the default shortcut

We want in our .desktop file

[Desktop Action FullScreenScreenShot]
Name=Capture Entire Desktop
X-KDE-Shortcuts=Control+P <--- new entry

This is loaded by frameworks/kserviceactioncomponent.cpp though I don't know if it works.
(it definitely has a bug that it doesn't read X-KDE-Shortcuts for the default action)

I just tried it and it works! Also the default shortcuts are set. I tested it and I think we can also take everything setting default shortcuts out of the code as it it set via the desktop file.

To clarify what works:
Only installing the desktop file in a running session (User installs Spectacle) doesn't work without additional registering of shortcuts.

davidre updated this revision to Diff 53264.Mar 6 2019, 10:50 AM
  • Defaults in desktop file and symlink it to kglobalaccel also use NOAutoloading
  • Install update files

The calls to KGlobalaccel are still needed. Removing them from the updatescript caused the user specified shortcuts to be the default shortcuts.
And removing them from Spectacle broke the Editor. NoAutoloading is needed that the shortcuts are correctly set when the desktop file is installed.

The calls to KGlobalaccel are still needed. Removing them from the updatescript caused the user specified shortcuts to be the default shortcuts.

We will need them in the update script.

removing them from Spectacle broke the Editor. NoAutoloading is needed that the shortcuts are correctly set when the desktop file is installed.

That doesn't sound like something that /should/ be needed.
Lets try and see if we can fix kglobalaccel.

I went looking for the difference what happens with and without Autoloading. With Autoloading we end up in this case:
https://cgit.kde.org/kglobalaccel.git/tree/src/runtime/kglobalacceld.cpp#n531
Apparently the shortcut is not fresh

Could this be part of the problem?

-- Installing: /usr/share/applications/org.kde.spectacle.desktop
failed to create symbolic link '/usr/share/kglobalaccel/org.kde.spectacle.desktop': No such file or directory
-- Up-to-date: /usr/share/knotifications5/spectacle.notifyrc

@davidedmundson is /usr/share/kglobalaccel the correct folder? It doesn't exist in my system.

@davidedmundson is /usr/share/kglobalaccel the correct folder?

yes.

@davidre can you put this back to being broken so we know what to fix.

@davidedmundson is /usr/share/kglobalaccel the correct folder?

yes.

@davidre can you put this back to being broken so we know what to fix.

Which broken do you mean? Do you mean without NoAutloading?

davidedmundson added inline comments.Apr 3 2019, 2:24 PM
src/SpectacleCore.cpp
128

these lines

davidre updated this revision to Diff 55348.Apr 3 2019, 2:34 PM
  • remove setup shortcuts

Broken Editor:

ngraham added inline comments.Apr 3 2019, 3:13 PM
desktop/CMakeLists.txt
10

Need to create ${KDE_INSTALL_FULL_DATAROOTDIR}/kglobalaccel if it doesn't already exist.

This is how I did it in D20229:

install( DIRECTORY DESTINATION "${KDE_INSTALL_FULL_DATAROOTDIR}/kglobalaccel" )
asturmlechner added inline comments.
desktop/CMakeLists.txt
10

Confirming same resulting sandbox violation as dolphin, needs the same fix as in D21011.

davidedmundson accepted this revision.Jun 21 2019, 10:11 AM

Spoke in person, investigated what was wrong.

KShortcutsEditor looks up the shortcut in the local cache. This local cache isn't updated as we don't have any kglobalacell call.
Running setShortcut isn't a bad workaround.

We can update it when we fix kglobalaccel.

davidre updated this revision to Diff 60210.Jun 21 2019, 10:26 AM
  • remove Extradesktop.sh
  • cmake stuff
  • Reinstate setupShortcuts
  • Drop currentscreen shortcuts
davidedmundson accepted this revision.Jun 21 2019, 10:30 AM
ngraham edited the summary of this revision. (Show Details)Jun 21 2019, 10:55 AM
davidre updated this revision to Diff 60221.Jun 21 2019, 12:34 PM
  • Fix build
davidre retitled this revision from [WIP] Port to KGlobalAccel to Port to KGlobalAccel.Jun 21 2019, 1:19 PM
davidre edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jun 21 2019, 3:41 PM
This revision was automatically updated to reflect the committed changes.

Looks like a copy-paste bug.

desktop/CMakeLists.txt
10

Typo: "org.kde.dolphin.desktop" -> "org.kde.spectacle.desktop"

Looks like a copy-paste bug.

Thanks for spotting this! I guess that was also the reason it didn't work for @ngraham with a new user.

No problem! I noticed 2 other things:

  • All shortcuts for "Spectacle" in "Application Launchers" section appear twice.
  • Starting "Capture Rectangular Region" using the assigned shortcut does perform the action, but capturing the screenshot silently saves the file and exits. Before, it used to give a notification.

Are these potentially related? I can do a git bisect at a later time.

No problem! I noticed 2 other things:

  • All shortcuts for "Spectacle" in "Application Launchers" section appear twice.

If you don't delete the spectacle.khotkeys inside /usr/share/khotkeys/ (depends on your distro) I think you would have entries inside both KDE Daemon and Spectacle categories.
But it could also be related because GlobalAccel can't map the actions to the desktop file.

  • Starting "Capture Rectangular Region" using the assigned shortcut does perform the action, but capturing the screenshot silently saves the file and exits. Before, it used to give a notification.

I will have a look, there was a recent refactor that changed how that mode workes maybe that broke. But anyways it shouldn't be to hard to fix.

Are these potentially related? I can do a git bisect at a later time.

Thanks again for your time.

davidre marked an inline comment as done.Jun 23 2019, 9:47 AM