Install app metadata for plasmoidviewer and plasmaengineexplorer
ClosedPublic

Authored by kossebau on Feb 28 2018, 3:56 PM.

Details

Summary

Improves integration at runtime and with appstream-based installers.

Diff Detail

Repository
R118 Plasma SDK
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Feb 28 2018, 3:56 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 28 2018, 3:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kossebau requested review of this revision.Feb 28 2018, 3:56 PM

Target would be 5.12 branch

kossebau updated this revision to Diff 28277.Feb 28 2018, 4:13 PM

add missing setting of desktopFileName in plasmoidviewer

kossebau updated this revision to Diff 28283.Feb 28 2018, 6:22 PM

fix unencoded & in bugtracker links

lgtm but can't really comment

Can't go into 5.12 due to translations, can it?

plasmoidviewer/org.kde.plasmoidviewer.desktop
10

Will it still be accessible through KRunner?

Though you probably want to use it from terminal to pass args and see output anyway

lgtm but can't really comment

Can't go into 5.12 due to translations, can it?

New strings should be fine, that has been done elsewhere before. As it's broken feature vs. initially untranslated strings.

plasmoidviewer/org.kde.plasmoidviewer.desktop
10

It still works as commandline program, whose runner plugin seems to not rely on desktop data. But indeed that often makes no sense, as some debug output might be interesting as well, not just what can be seen in the IU.

Just to make sure we are on the same page, the reason for NoDisplay=true is that plasmoidviewer needs to be started with arguments, without any it will quit immediatly again.
So starting from menu does not work, and thus also not from krunner, at least as plain application.

ngraham requested changes to this revision.Feb 28 2018, 8:46 PM
ngraham added a subscriber: ngraham.

Appending .desktop is unnecessary and a violation of the AppStream spec. See https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#sect-Metadata-GenericComponent

The ID must follow a reverse-DNS scheme, consisting of {tld}.{vendor}.{product}, for example org.kde.Gwenview or com.hugski.ColorHug2. Ownership of {vendor}.{tld} in the domain name system guarantees uniqueness of IDs.

This revision now requires changes to proceed.Feb 28 2018, 8:46 PM
kossebau added a comment.EditedFeb 28 2018, 8:55 PM

Appending .desktop is unnecessary and a violation of the AppStream spec. See https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#sect-Metadata-GenericComponent

The ID must follow a reverse-DNS scheme, consisting of {tld}.{vendor}.{product}, for example org.kde.Gwenview or com.hugski.ColorHug2. Ownership of {vendor}.{tld} in the domain name system guarantees uniqueness of IDs.

Can you help me to see where the violation of the spec is? Appending a suffix to the app name should not break anything by itself, no? (Edit: so the [product] here is "foo.desktop")

AFAIK the desktop is used to denote that this is the app variant as useful for desktop workspaces, as opposed to e.g. mobile. (Edit: less "useful" but more about being the very id of this app variant)

Well the docs show you how an AppStream ID is to be constructed, and there's no mention of .desktop. I've had conversations with Matthias Klumpp, the AppStream maintainer, and he's indicated that my impression is accurate (and I believe he's said the validator will get much more strict in an upcoming major version).

Also I forgot to mention: since my proposed change will make the desktop file name no longer match the AppStream ID, you will also need to add:

<launchable type="desktop-id">org.kde.plasmoidviewer.desktop</launchable>

Well the docs show you how an AppStream ID is to be constructed, and there's no mention of .desktop. I've had conversations with Matthias Klumpp, the AppStream maintainer, and he's indicated that my impression is accurate (and I believe he's said the validator will get much more strict in an upcoming major version).

While there is no direct mention of a .desktop, there is some description about adding a type:

To increase the uniqueness and to distinguish between different pieces of a software suite, it is suggested to append the type name to the component-id in these cases. For example, one can use com.hugski.ColorHug2 for the client tools to control hardware, and com.hugski.ColorHug2.firmware for the runtime firmware files.

So why would you think that .desktop is not the type name as mentioned there? Any chance you can point to the relevant conversations?

ngraham resigned from this revision.Feb 28 2018, 9:15 PM

Hmm, that's a good point. Let me get back to you.

Also I forgot to mention: since my proposed change will make the desktop file name no longer match the AppStream ID, you will also need to add:

<launchable type="desktop-id">org.kde.plasmoidviewer.desktop</launchable>

This leaves me puzzled: where is it defined that the appstream id should match the full desktop filename? I cannot find this specified either with the <id> or the <launchable> tags definitions. Adding such a <launchable> tag would be useful independently of the very id being used, no?

OK, I just asked him again over Telegram. Here was the reply:

Matthias, [28.02.18 14:18]
The general advice is to leave a desktop suffix out for new metadata
Having it doesn't hurt though

Here's the full discussion:

Nate Graham, [28.02.18 14:17]
Quick question: is it or is it not okay for TLD-style appstream IDs to end in .desktop? Is there any problem with this, or do clients ignore it when determining app uniqueness? (i.e. would org.blender.blender and org.blender.blender.desktop be considered the same app)

Matthias, [28.02.18 14:18]
It would not be considered the same app

Matthias, [28.02.18 14:18]
The general advice is to leave a desktop suffix out for new metadata

Matthias, [28.02.18 14:19]
Having it doesn't hurt though

Nate Graham, [28.02.18 14:22]
it might be nice to add this information to https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#sect-Metadata-GenericComponent, since when I tell this to people, they're often skeptical

Matthias, [28.02.18 14:22]
Which information?

Nate Graham, [28.02.18 14:22]
that it's not recommended to add .desktop as a suffix for new apps

Matthias, [28.02.18 14:23]
Well, the spec is pretty explicit in not requesting it - no example there has it, and the ID is defined as tld.domain.app

Nate Graham, [28.02.18 14:23]
for example see the discussion at https://phabricator.kde.org/D10920

Nate Graham, [28.02.18 14:24]
I pointed that out, and the reply was:

Nate Graham, [28.02.18 14:24]
"While there is no direct mention of a .desktop, there is some description about adding a type:
So why would you think that .desktop is not the type name as mentioned there? Any chance you can point to the relevant conversations?"

Matthias, [28.02.18 14:24]
I am currently on vacation, will look at that bug report when back home

Nate Graham, [28.02.18 14:24]
ok

Matthias, [28.02.18 14:25]
Oh, that type stuff if it's still in there should maybe be clarified then, I guess - will look at it later

Nate Graham, [28.02.18 14:25]
cool, thanks

Matthias, [28.02.18 14:25]
Basically, you can add a type if you want to distinguish between components in the same domain, e.g. if you develop an app and a library of the same name

Thanks for pasting the chat log, that sheds more light onto things :)
Seems to me the spec author(s?) are not sure themselves what would be the best naming pattern (besides rtld namespacing) and are still sorting that out.
Which to me leaves the choice between being consistent with existing naming practice (at least in KDE spheres) and going for some unclear motivated naming recommendation not yet hard-coded in spec policy. Where I personally then would tend to the former, leaving the .desktop suffix.

But no hard opinion, leaving the call to the maintainers.

Grepping across KDE's sources with https://lxr.kde.org/search?_filestring=&_string=launchable&_casesensitive=1 it seems that adding <launchable> needs more advertizing in general. Do I correctly assume that Discover and friends already make use of that data bit, so it's valuable to have it set? Would then go and add for apps I care a bit :)

kossebau updated this revision to Diff 28324.Mar 1 2018, 1:18 PM

given it's been talked about, let's already add <launchable> at least for
plasmaengineexplorer. plasmoidviewer cannot be launched without arguments,
thus no such tag for it

[14:38] <sebas> frinring: I'd go with what Matthias suggests, otherwise: cool stuff, +1 (so shipit as long as others are happy, too)

So going to drop the desktop suffix from the id then given majority of uttered opinions :)

Will push directly once I got confirmation from translators that adding new strings for this bug fixes (proper icon/name with windows, installable from appstream-centric installers) is okay for stable, or not.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 1 2018, 11:04 PM
This revision was automatically updated to reflect the committed changes.