D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:84432a55e969: [KMoreTools] Enable installing tools via 
appstream url (authored by nicolasfella).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D13706?vs=36652=36655#toc

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36652=36655

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Here's the last of the apps for which I could find upstream AppStream 
information:
  
  After those are updated, I'm good with this!

INLINE COMMENTS

> kmoretoolspresets.cpp:61
> +ADD_ENTRY("com.uploadedlobster.peek",   0, 
> "https://github.com/phw/peek;, "com.uploadedlobster.peek.desktop"); // easy 
> to use screen recorder, creates gif
> +ADD_ENTRY("catfish",1, 
> "http://www.twotoasts.de/index.php/catfish/;, "catfish");
> +ADD_ENTRY("ding",   0, 
> "https://www-user.tu-chemnitz.de/~fri/ding/;, ""); // Offline dict, Online: 
> http://dict.tu-chemnitz.de/dings.cgi

`catfish`

https://git.launchpad.net/catfish-search/tree/data/metainfo/catfish.appdata.xml.in#n4

> kmoretoolspresets.cpp:68
> +ADD_ENTRY("giggle", 1, 
> "https://wiki.gnome.org/Apps/giggle/;, "giggle.desktop"); // good for 
> searching in history
> +ADD_ENTRY("git-cola-folder-handler",1, 
> "https://git-cola.github.io;, "");
> +ADD_ENTRY("git-cola-view-history.kmt-edition",  1, 
> "https://git-cola.github.io;, "");

`git-cola.desktop`

https://github.com/git-cola/git-cola/blob/master/share/appdata/git-cola.appdata.xml#L3

> kmoretoolspresets.cpp:72
> +ADD_ENTRY("qgit.kmt-edition",   1, 
> "http://libre.tibirna.org/projects/qgit;, "");
> +ADD_ENTRY("gitg",   1, 
> "https://wiki.gnome.org/action/show/Apps/Gitg?action=show=Gitg;, "");
> +ADD_ENTRY("gnome-search-tool",  0, 
> "https://help.gnome.org/users/gnome-search-tool/;, 
> "gnome-search-tool.desktop"); // has good filtering options

`gitg.desktop`

https://gitlab.gnome.org/GNOME/gitg/blob/master/data/gitg.appdata.xml#L3

REPOSITORY
  R304 KNewStuff

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella marked 9 inline comments as done.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella updated this revision to Diff 36652.
nicolasfella added a comment.


  o  - Even more appstream ids. Thanks Nate

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36651=36652

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> kmoretoolspresets.cpp:60
> +ADD_ENTRY("angrysearch",0, 
> "https://github.com/DoTheEvo/ANGRYsearch;, "");
> +ADD_ENTRY("com.uploadedlobster.peek",   0, 
> "https://github.com/phw/peek;, ""); // easy to use screen recorder, creates 
> gif
> +ADD_ENTRY("catfish",1, 
> "http://www.twotoasts.de/index.php/catfish/;, "catfish");

`com.uploadedlobster.peek.desktop`

https://github.com/phw/peek/blob/master/data/com.uploadedlobster.peek.appdata.xml.in

> kmoretoolspresets.cpp:93
> +ADD_ENTRY("simplescreenrecorder",   0, 
> "http://www.maartenbaert.be/simplescreenrecorder/;, 
> "simplescreenrecorder.desktop");
> +ADD_ENTRY("shutter",0, 
> "http://shutter-project.org;, ""); // good for edit screenshot after capture
> +ADD_ENTRY("vokoscreen", 0, 
> "https://github.com/vkohaupt/vokoscreen;, ""); // feature-rich screen recorder

`org.shutterproject.shutter`

https://bugs.launchpad.net/shutter/+bug/1739971

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella updated this revision to Diff 36651.
nicolasfella added a comment.


  - Add more appstream ids

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36650=36651

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella updated this revision to Diff 36650.
nicolasfella added a comment.


  - doc++

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36620=36650

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[gregormi]

gregormi added a comment.


  In D13706#282765 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  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 ` 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 `.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.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, ngraham
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Dominik Haumann]

dhaumann added a comment.


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

INLINE COMMENTS

> kmoretools.h:490
> + * @since 5.48
> + * @return The service' appstream id
> + */

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
   */

> kmoretools.h:495
> +/**
> + * @since 5.48
> + * Sets the appstream id of the service. This is used to create a

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

> kmoretools.h:500
> + */
> +void setAppstreamId(const QString&);
> +

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);

REPOSITORY
  R304 KNewStuff

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Dominik Haumann]

dhaumann resigned from this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R304 KNewStuff

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nathaniel Graham]

ngraham added a comment.


  @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.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: ngraham, dhaumann, kde-frameworks-devel, michaelh, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[gregormi]

gregormi accepted this revision.
gregormi added a comment.


  Ok from my side. Probably Dominik also wants to approve.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> nicolasfella wrote in 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)

See http://doc.qt.io/qt-5/qstring.html#more-efficient-string-construction for a 
detailed explanation

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> gregormi wrote in 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.

+ 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)

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[gregormi]

gregormi added inline comments.

INLINE COMMENTS

> kmoretools_p.h:394
> +
> +QUrl appstreamUrl = QUrl(QStringLiteral("appstream://") % 
> appstreamId);
> +

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

Otherwise ready to land.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella updated this revision to Diff 36620.
nicolasfella added a comment.


  - Replace appstreamUrl with appstreamId

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36617=36620

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[gregormi]

gregormi added inline comments.

INLINE COMMENTS

> kmoretoolspresets.cpp:59
>  //
> -ADD_ENTRY("angrysearch",0, 
> "https://github.com/DoTheEvo/ANGRYsearch;);
> -ADD_ENTRY("com.uploadedlobster.peek",   0, 
> "https://github.com/phw/peek;); // easy to use screen recorder, creates gif
> -ADD_ENTRY("catfish",1, 
> "http://www.twotoasts.de/index.php/catfish/;);
> -ADD_ENTRY("ding",   0, 
> "https://www-user.tu-chemnitz.de/~fri/ding/;); // Offline dict, Online: 
> http://dict.tu-chemnitz.de/dings.cgi
> -ADD_ENTRY("disk",   0, 
> "https://en.opensuse.org/YaST_Disk_Controller;);
> -ADD_ENTRY("fontinst",   0, 
> "https://docs.kde.org/trunk5/en/kde-workspace/kcontrol/fontinst/;); // good 
> for previewing many fonts at once
> -ADD_ENTRY("fontmatrix", 0, 
> "https://github.com/fontmatrix/fontmatrix;);
> -ADD_ENTRY("fsearch",0, 
> "http://www.fsearch.org/;);
> -ADD_ENTRY("giggle", 1, 
> "https://wiki.gnome.org/Apps/giggle/;); // good for searching in history
> -ADD_ENTRY("git-cola-folder-handler",1, 
> "https://git-cola.github.io;);
> -ADD_ENTRY("git-cola-view-history.kmt-edition",  1, 
> "https://git-cola.github.io;);
> -ADD_ENTRY("gitk.kmt-edition",   1, 
> "http://git-scm.com/docs/gitk;);
> -ADD_ENTRY("qgit.kmt-edition",   1, 
> "http://libre.tibirna.org/projects/qgit;);
> -ADD_ENTRY("gitg",   1, 
> "https://wiki.gnome.org/action/show/Apps/Gitg?action=show=Gitg;);
> -ADD_ENTRY("gnome-search-tool",  0, 
> "https://help.gnome.org/users/gnome-search-tool/;); // has good filtering 
> options
> -ADD_ENTRY("gucharmap",  0, 
> "https://wiki.gnome.org/action/show/Apps/Gucharmap;);
> -ADD_ENTRY("gparted",0, "http://gparted.org;);
> -ADD_ENTRY("htop",   0, 
> "http://hisham.hm/htop/;);
> -ADD_ENTRY("hotshots",   1, 
> "http://sourceforge.net/projects/hotshots/;);
> -ADD_ENTRY("kaption",0, 
> "http://kde-apps.org/content/show.php/?content=139302;);
> -ADD_ENTRY("kding",  0, ""); // Offline dict; 
> unmaintained?
> -ADD_ENTRY("org.kde.kmousetool", 0, 
> "https://www.kde.org/applications/utilities/kmousetool/;);
> -ADD_ENTRY("org.gnome.clocks",   0, 
> "https://wiki.gnome.org/Apps/Clocks;);
> -ADD_ENTRY("org.kde.filelight",  1, 
> "https://utils.kde.org/projects/filelight;);
> -ADD_ENTRY("org.kde.kcharselect",0, 
> "https://utils.kde.org/projects/kcharselect/;);
> -ADD_ENTRY("org.kde.kdf",0, 
> "https://www.kde.org/applications/system/kdiskfree;);
> -ADD_ENTRY("org.kde.kfind",  1, 
> "https://www.kde.org/applications/utilities/kfind/;); // has good filtering 
> options
> -ADD_ENTRY("org.kde.partitionmanager",   0, 
> "https://www.kde.org/applications/system/kdepartitionmanager/;);
> -ADD_ENTRY("org.kde.plasma.cuttlefish.kmt-edition", 0, 
> "http://vizzzion.org/blog/2015/02/say-hi-to-cuttlefish/;);
> -ADD_ENTRY("org.kde.ksysguard",  0, 
> "https://userbase.kde.org/KSysGuard;);
> -ADD_ENTRY("org.kde.ksystemlog", 0, 
> "https://www.kde.org/applications/system/ksystemlog/;);
> -ADD_ENTRY("org.kde.ktimer", 0, 
> "https://www.kde.org/applications/utilities/ktimer/;);
> -ADD_ENTRY("org.kde.spectacle",  0, 
> "https://www.kde.org/applications/graphics/spectacle;);
> -ADD_ENTRY("simplescreenrecorder",   0, 
> "http://www.maartenbaert.be/simplescreenrecorder/;);
> -ADD_ENTRY("shutter",0, 
> "http://shutter-project.org;); // good for edit screenshot after capture
> -ADD_ENTRY("vokoscreen", 0, 
> "https://github.com/vkohaupt/vokoscreen;); // feature-rich screen recorder
> -ADD_ENTRY("xfce4-taskmanager",  0, 
> "http://goodies.xfce.org/projects/applications/xfce4-taskmanager;);
> +ADD_ENTRY("angrysearch",0, 
> "https://github.com/DoTheEvo/ANGRYsearch;, "");
> +ADD_ENTRY("com.uploadedlobster.peek",   0, 
> "https://github.com/phw/peek;, ""); // easy to use screen recorder, creates 
> gif

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

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

ADD_ENTRY("catfish", 

D13706: [KMoreTools] Enable installing tools via appstream url

[gregormi]

gregormi added inline comments.

INLINE COMMENTS

> kmoretools.h:500
> + */
> +void setAppstreamUrl(const QUrl& url);
> +

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.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella updated this revision to Diff 36617.
nicolasfella added a comment.


  - Add documentation

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36612=36617

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella marked 2 inline comments as done.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Dominik Haumann]

dhaumann requested changes to this revision.
dhaumann added a comment.
This revision now requires changes to proceed.


  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

INLINE COMMENTS

> kmoretools.h:488
>  
> +QUrl appstreamUrl() const;
> +

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

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi, dhaumann
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[gregormi]

gregormi added a comment.


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

INLINE COMMENTS

> kmoretools.h:488-491
> +QUrl appstreamUrl() const;
> +
> +void setAppstreamUrl(const QUrl& url);
> +

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.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Dominik Haumann]

dhaumann added a reviewer: gregormi.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks, gregormi
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella updated this revision to Diff 36612.
nicolasfella added a comment.


  - Add kmousetool url

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36609=36612

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella updated this revision to Diff 36609.
nicolasfella added a comment.


  - Add more appstream urls

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36606=36609

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella edited the summary of this revision.

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D13706

To: nicolasfella, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella updated this revision to Diff 36606.
nicolasfella added a comment.


  - Whitespace

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13706?vs=36604=36606

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13706: [KMoreTools] Enable installing tools via appstream url

[Nicolas Fella]

nicolasfella created this revision.
nicolasfella added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

REVISION 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

REPOSITORY
  R304 KNewStuff

BRANCH
  appstream

REVISION DETAIL
  https://phabricator.kde.org/D13706

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h
  src/kmoretools/kmoretools_p.h
  src/kmoretools/kmoretoolsconfigdialog_p.cpp
  src/kmoretools/kmoretoolspresets.cpp

To: nicolasfella, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns