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.
gregormi | |
ngraham | |
dhaumann |
Frameworks |
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.
Install GParted from Dolphin
No Linters Available |
No Unit Test Coverage |
Buildable 286 | |
Build 286: arc lint + arc unit |
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. |
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 |
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. |
src/kmoretools/kmoretoolspresets.cpp | ||
---|---|---|
59–95 | I have these thoughts when I see the ADD_ENTRY lines:
ADD_ENTRY("catfish", 1, "http://www.twotoasts.de/index.php/catfish/", "appstream://catfish"); ADD_ENTRY("giggle", 1, "https://wiki.gnome.org/Apps/giggle/", "appstream://giggle.desktop"); But in the end this is only cosmetics and not required to land this patch. |
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. |
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 | See http://doc.qt.io/qt-5/qstring.html#more-efficient-string-construction for a detailed explanation |
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.
@ngraham Since you know more about installation (e.g. via Discover), I would like you to give another +1, if possible.
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); |
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.
Hi Nate, thanks for reviewing.
src/kmoretools/kmoretoolspresets.cpp | ||
---|---|---|
60 | com.uploadedlobster.peek.desktop https://github.com/phw/peek/blob/master/data/com.uploadedlobster.peek.appdata.xml.in | |
93 | org.shutterproject.shutter |