Remove Exec line for KCM service files
Closed, ResolvedPublic

Description

This idea came from the discussions about https://invent.kde.org/plasma/plasma-workspace/-/merge_requests/251 regarding https://bugs.kde.org/show_bug.cgi?id=398803.

In the newer versions of plasma we want to launch KCMs in systemsettings instead of kcmshell. For this there was a hack in the services runner implemented:
https://invent.kde.org/plasma/plasma-workspace/-/blob/master/runners/services/servicerunner.cpp#L456

So we basically do a little bit of checking , replace the exec line with systemsettings/kinfocenter+basename and proceed to launch the service. But the problem is when pinning the entry we still use out Exec line from the services file and not out services runner hack. This leads to the mentioned bug.

When discussing this with @sitter we came to the idea that we could use the fallback logic from our services runner inside of KService if no Exec line is given. Then we would internally use kcmshell5/systemsettings5/kinfocenter + basename and create our exec line this way. And as seen in the services runner this works pretty well.
But because one user might have a ancient version of plasma and an up to date frameworks version we should not use this as default, but rather as a fallback if no exec line is given. This way we also avoid issues like https://bugs.kde.org/show_bug.cgi?id=426737.

And considering that the Exec line is always program+basename it is redundant to store the information in the Exec line anyways and we can save ourselves some trouble.

alex created this task.Oct 6 2020, 3:55 PM
alex added a project: KF6.Oct 7 2020, 7:45 AM

Note that not all .desktop files are the same, e.g.:
https://lxr.kde.org/source/kde/workspace/bluedevil/src/kcm/package/metadata.desktop
https://lxr.kde.org/source/kde/workspace/plasma-desktop/kcms/autostart/package/metadata.desktop

A .desktop file that launches an executable should have an Exec= line as the spec says (IIUC), so we can leave the Exec line in and ignore it if we want.

Ideally the Exec line should be used and not the file name... :)

My 2 p's

davidre added a subscriber: davidre.Oct 7 2020, 8:09 AM

Note that not all .desktop files are the same, e.g.:
https://lxr.kde.org/source/kde/workspace/bluedevil/src/kcm/package/metadata.desktop
https://lxr.kde.org/source/kde/workspace/plasma-desktop/kcms/autostart/package/metadata.desktop

A .desktop file that launches an executable should have an Exec= line as the spec says (IIUC), so we can leave the Exec line in and ignore it if we want.

Ideally the Exec line should be used and not the file name... :)

My 2 p's

Well Type=Service is a kde extension after all and we use it for plugins (only for those?). A plugin doesn't need a Exec line imo

alex added a comment.Oct 7 2020, 8:12 AM

Ideally the Exec line should be used and not the file name... :)

The file name has to be the same for system settings. In the case of bluedevil the file gets renamed:
install(FILES package/metadata.desktop RENAME bluetooth.desktop DESTINATION ${KDE_INSTALL_KSERVICES5DIR})

Well Type=Service is a kde extension after all and we use it for plugins (only for those?).

Yeah that is the requirement for such a change. If we were to to search for our KCMs in the gnome search provider we would not get any results.

Good points.

And I didn't know the file gets renamed, thanks for the info :)

(It still feels "wrong" to remove the Exec line, but maybe that's just convention speaking).

One thing to consider is the settings io slave maybe? (Enter settings:/// into dolphin to see what it does)

alex added a comment.EditedOct 7 2020, 8:23 AM

(It still feels "wrong" to remove the Exec line, but maybe that's just convention speaking).

But it also feels wrong to have redundant information which currently causes issues ;)

One thing to consider is the settings io slave maybe? (Enter settings:/// into dolphin to see what it does)

But I would make the wild guess and say that it uses KService. So we would just use the fallback logic there.

And while discussing this with @sitter we agreed that if the Exec line might be needed (we explicitly had KIO/Frameworks in mind) we can still leave it in there for the moment. That is is why the suggestion is to use it as a fallback logic.

settings:// actually executes the raw paths by the looks of it. i.e. you actually run $prefix/kservices5/bluetooth.desktop when clicking an entry. Which does in fact rely on the Exec entry I believe. The slave is broken for me though, so I'm not sure this is exactly crucial functionality :S

alex added a comment.Oct 8 2020, 3:54 PM

settings:// actually executes the raw paths by the looks of it. i.e. you actually run $prefix/kservices5/bluetooth.desktop when clicking an entry. Which does in fact rely on the Exec entry I believe. The slave is broken for me though, so I'm not sure this is exactly crucial functionality :S

It works for me and it uses the KService API, see kio/src/gui/kprocessrunner.cpp line 81.

So we are fine in that case ;)

I am not sure about the effects of https://invent.kde.org/frameworks/kcoreaddons/-/merge_requests/32, because we need to tell if the KCM is intended to be displayed in the launcher or just withing a plugin selector somewhere in its parent component.

Alternatively we could say that the Exec line must exist and has to be empty in order for us to set the fallback exec value.

@nicolasfella Can you please share your thoughts

I assume you are worried about plugin KCMs showing up in the launcher although they shouldn't? That's kind of unrelated to that patch.

I see a few possible solutions, all somewhat magic or error prone to some degree though. We could have some kind of hint like X-KDE-ShowInLauncher/X-KDE-DontShowInLaucher. Maybe we should only show KCMs that have X-KDE-ParentApp=kcontrol.

alex added a comment.Oct 8 2020, 7:17 PM

I assume you are worried about plugin KCMs showing up in the launcher although they shouldn't? That's kind of unrelated to that patch.

Well it is related to the patch since it is about unsing the described concept as a fallback ;)

Maybe we should only show KCMs that have X-KDE-ParentApp=kcontrol

Yeah, that is a good idea, this way we are definitely on the save side.

I assume you are worried about plugin KCMs showing up in the launcher although they shouldn't? That's kind of unrelated to that patch.

I see a few possible solutions, all somewhat magic or error prone to some degree though. We could have some kind of hint like X-KDE-ShowInLauncher/X-KDE-DontShowInLaucher. Maybe we should only show KCMs that have X-KDE-ParentApp=kcontrol.

Why not use the same criteria that systemsettings itself uses?
https://invent.kde.org/plasma/systemsettings/-/blob/master/app/SettingsBase.cpp#L105

alex added a comment.EditedOct 9 2020, 8:10 AM

But the link you have pasted points to the location where the categories are fetched and this task is about the individual modules.

But also please note that this task aims to provide a fallback for KCMs that use kcmshell5, like the trash KCM. So the line after the one link you have posted is already included in the proposal https://invent.kde.org/frameworks/kservice/-/merge_requests/16/diffs#eb4cf9ceb826c4356054290b8018a6de792687da_239_248

Oh nervermind me then, I was a bit confused

alex added a comment.Oct 24 2020, 3:19 PM

The only occurrences where we need to apply this fallback logic are in https://invent.kde.org/frameworks/kio/-/merge_requests/187 handled.

After that we can remove the hack in https://invent.kde.org/plasma/plasma-workspace/-/merge_requests/385.

For konqueror and kmymoney it is debatable if the KCMs need to be available globally, see these issues and feel free to comment:
https://invent.kde.org/network/konqueror/-/issues/1
https://invent.kde.org/office/kmymoney/-/issues/32

Once those MRs have landed and the issues have been resolved I would consider this task done.

alex closed this task as Resolved.Nov 9 2020, 7:01 PM