[KMoreTools] Enable installing tools via appstream url
ClosedPublic

Authored by nicolasfella on Jun 24 2018, 3:05 PM.

Details

Summary

When a tool is not installed add an action to trigger the appstream url if available. This allows installing via Discover or other software stores.

Add GParted url as example. More urls should be added.

Test Plan

Install GParted from Dolphin

Diff Detail

Repository
R304 KNewStuff
Branch
appstream
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 284
Build 284: arc lint + arc unit
nicolasfella created this revision.Jun 24 2018, 3:05 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 24 2018, 3:05 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
nicolasfella requested review of this revision.Jun 24 2018, 3:05 PM
  • Whitespace
nicolasfella edited the summary of this revision. (Show Details)Jun 24 2018, 3:09 PM
  • Add more appstream urls
  • Add kmousetool url

First of all, thanks for adding this feature. This was missing a long time :-). I will do some comments in the code.

src/kmoretools/kmoretools.h
488–491

Please add a comment and add something like "@since 5.xx" (see elsewhere in the this file) to indicate since which frameworks version this will be available.

dhaumann requested changes to this revision.Jun 24 2018, 8:50 PM
dhaumann added a subscriber: dhaumann.

Looks already pretty good to me, but please add API documentation.

Rule of thumb: if you extend a header file that is documented, then follow this scheme and also document your new functions

src/kmoretools/kmoretools.h
488

API documentation is missing. Please also add @since 5.48, since this will be the next frameworks release, see: https://community.kde.org/Schedules/Frameworks

This revision now requires changes to proceed.Jun 24 2018, 8:50 PM
  • Add documentation
nicolasfella marked 2 inline comments as done.Jun 24 2018, 9:04 PM
gregormi added inline comments.Jun 24 2018, 9:06 PM
src/kmoretools/kmoretools.h
500

I am not so familiar with appstream. Why not only setting the COMPONENT-ID instead of the whole URL? Then the method would be named setAppstreamComponentId.

gregormi added inline comments.Jun 24 2018, 9:24 PM
src/kmoretools/kmoretoolspresets.cpp
59

I have these thoughts when I see the ADD_ENTRY lines:

  1. Appstream data is _the_ important source of app data. So I would move it directly after the URL count and before the homepage
  1. Mostly, desktopEntryName and appstreamComponentId are equal. So, maybe one could use a placeholder (e.g. ~) to reuse the desktopEntryName

ADD_ENTRY("catfish", 1, "http://www.twotoasts.de/index.php/catfish/", "appstream://catfish");
would become
ADD_ENTRY("catfish", 1, "~", "http://www.twotoasts.de/index.php/catfish/");

ADD_ENTRY("giggle", 1, "https://wiki.gnome.org/Apps/giggle/", "appstream://giggle.desktop");
would become
ADD_ENTRY("giggle", 1, "giggle.desktop", "https://wiki.gnome.org/Apps/giggle/");

But in the end this is only cosmetics and not required to land this patch.

  • Replace appstreamUrl with appstreamId
gregormi added inline comments.Jun 24 2018, 9:48 PM
src/kmoretools/kmoretools_p.h
394

What does the % sign do here? Can this be used to concatenate strings? Did not try it myself.

Otherwise ready to land.

nicolasfella added inline comments.Jun 24 2018, 9:58 PM
src/kmoretools/kmoretools_p.h
394

+ would give the same result. What % does is acting as a QStringBuilder, which is more performant in theory (not that it really matters in this case)

394
gregormi accepted this revision.EditedJun 24 2018, 10:08 PM

Ok from my side. Please let also Dominik give his approval.

@gregormi

Mostly, desktopEntryName and appstreamComponentId are equal. So, maybe one could use a placeholder (e.g. ~) to reuse the desktopEntryName

Keep in mind that AppStream IDs are not guessable. Some apps use the correct format, e.g. "org.kde.konsole", while others just re-use their desktop file's filename (e.g. gucharmap.desktop). It's app-specific and can't be programmatically determined.

dhaumann resigned from this revision.Jun 25 2018, 11:45 AM

@ngraham Since you know more about installation (e.g. via Discover), I would like you to give another +1, if possible.

This revision is now accepted and ready to land.Jun 25 2018, 11:45 AM

The API documentation can / should still be improved. I tried to give an example of possible improvements.

src/kmoretools/kmoretools.h
490

What about this:

/**
 * Returns the associated appstream id that was previously set with setAppstreamId().
 * If no appstream id was set, an empty string is returned.
 *
 * @return The service's appstream id.
 *
 * @since 5.48
 */
495

Please move @since down. The tags are usually the last part in the API documentation.

500

This is also missing the parameter, which can be used for documentation:

/**
 * Sets the appstream id of the service. This is used to create a
 * appstream url for installing the service via a software store
 * (e.g. Discover). For instance, the appstream id for filelight is
 * "org.kde.filelight.desktop".
 *
 * @param id the appstream id
 *
 * @since 5.48
 */
void setAppstreamId(const QString& id);
ngraham requested changes to this revision.Jun 25 2018, 2:16 PM

Tried this out. It's a really great feature to support installation via the AppStream URL--the logical next step after an app has been recommended to you. I tried a few and installation worked perfectly via Discover. There are a few user-facing papercuts such as when the appstream URL isn't found, but that's not your fault.

I'd like to see the missing AppStream URLs filled in for all the software in this list. There are currently some claring omissions, especially for KDE software (e.g. ksysguard) for which there is definitely an appstream ID available). For any software that doesn't have any AppStream information (and is therefore not visible or installable via Discover), I would actually advocate removing it from the list, and making the presence of AppStream data a pre-condition of inclusion--the reason being that otherwise an Install button can't be presented to the user. Entries without an Install button are just frustrating, tantamount to taunting them ("here's some cool software you could use; oh, sorry, can't actually install it lol")

To find a program's appstream ID, you can do appstreamcli search <search term> to find that information on your system. There's a lot of missing data on an old distro like Neon; best to do this on a rolling release distro or a fixed-release distro with a recent release (e.g. Fedora 28 or Kubuntu 18.04) since the AppStream information is more likely to be up-to-date there. Alternatively, check out the sources, find the <app name>.appdata.xml file and find the ID there. If none exists, just default to using the desktop file name, which is what most distros currently do.

This revision now requires changes to proceed.Jun 25 2018, 2:16 PM

I'd like to see the missing AppStream URLs filled in for all the software in this list. There are currently some claring omissions, especially for KDE software (e.g. ksysguard) for which there is definitely an appstream ID available). For any software that doesn't have any AppStream information (and is therefore not visible or installable via Discover), I would actually advocate removing it from the list, and making the presence of AppStream data a pre-condition of inclusion--the reason being that otherwise an Install button can't be presented to the user. Entries without an Install button are just frustrating, tantamount to taunting them ("here's some cool software you could use; oh, sorry, can't actually install it lol")

Hi Nate, thanks for reviewing.

  • I tried appstreamcli ksysguard and appstreamcli ding. Both do not yield a result. => I vote not to remove a program from this list just because it has no obvious appstream id (yet).
  • Programs which serve the same purpose should have a comment why they are in the list, like it was done with giggle ("// good for searching in history"). In such cases, for me it would be ok if there is no appstream id yet.
  • For me, it would be ok, if Nicolas does not have to search for all the missing appstream ids - except if you have fun to do it, Nicolas :-). The patch is a definite improvement to the current state and missing ids could also be added later incrementally.
  • Add more appstream ids
ngraham added inline comments.Jun 25 2018, 8:07 PM
src/kmoretools/kmoretoolspresets.cpp
60
93

o - Even more appstream ids. Thanks Nate

nicolasfella marked 9 inline comments as done.Jun 25 2018, 8:13 PM
ngraham accepted this revision.Jun 25 2018, 9:10 PM

Here's the last of the apps for which I could find upstream AppStream information:

After those are updated, I'm good with this!

src/kmoretools/kmoretoolspresets.cpp
61
68
72
This revision is now accepted and ready to land.Jun 25 2018, 9:10 PM
This revision was automatically updated to reflect the committed changes.