Thanks! One more thought: instead of comparing the action's text, could we compare the action itself, to see if it's mInstallPluginAction?

In D13197#270991, @ngraham wrote:

Thanks! One more thought: instead of comparing the action's text, could we compare the action itself, to see if it's mInstallPluginAction?

mInstallPluginAction is a private member of KIPIInterface, thus inaccessible in the context of KIPIExportAction. Would it be sensible to introduce a getter just for this single use case?
I followed the same logic used in KIPIExportAction::init() which uses the action text stored in .config/gwenviewrc.

That might be more reliable and semantically correct, so let's try it out and see if we like it!

@faridb Awesome, works great. @ngraham already provided some valuable feedback, there's only one thing I'm hesitant about (see inline comment).

Off-topic for this patch, but still I'd like to comment:

In D13197#270590, @ngraham wrote:

If appstream://photolayoutseditor.desktop isn't available on Arch, that's a packaging bug (and please file it!). A package matching that appstream ID is present in KDE Neon. Of course, it doesn't actually install. :( Filed https://bugs.kde.org/show_bug.cgi?id=394846

Reading the commit history of that feature, this looks more like something which was added with packaging of a particular distro in mind. Why would this be called "photolayoutseditor" anyway, when that plugin is only part of a much larger set of plugins? Also, the description looks quite broken in Discover on that distro, while on other distros the description of the regular "kipi-plugins" package is perfectly reasonable.

My goal would be to make this less distro-specific, because I don't think asking other distros to call this "photolayoutseditor.desktop" is the right thing to do.

@ngraham Can you think of any other KDE app which has a similar install-on-demand function? If there is, I'd love to peek at it.

In D13197#271103, @rkflx wrote:

Reading the commit history of that feature, this looks more like something which was added with packaging of a particular distro in mind. Why would this be called "photolayoutseditor" anyway, when that plugin is only part of a much larger set of plugins? Also, the description looks quite broken in Discover on that distro, while on other distros the description of the regular "kipi-plugins" package is perfectly reasonable.

Indeed, the actual add-on itself needs a lot of love to actually work! :)

@ngraham Can you think of any other KDE app which has a similar install-on-demand function? If there is, I'd love to peek at it.

Dolphin has an example on the Services page of the settings window. It uses Get Hot New Stuff. If we wanted to eventually move these plugins into GHNS, we could either integrate a GHNS chooser into Gwenview like Dolphin does, or just continue to call an appropriate URL, because Discover has a GHNS backend and can install GHNS content.

appstream://photolayoutseditor.desktop

I wonder whether it would be feasible to ship something like kipi-plugins.desktop via upstream R480 Kipi Plugins? (Just an idea in case someone wants to investigate that, I don't have time to work in it right now.)

In D13197#271108, @ngraham wrote:

@ngraham Can you think of any other KDE app which has a similar install-on-demand function? If there is, I'd love to peek at it.

Dolphin has an example on the Services page of the settings window. It uses Get Hot New Stuff. If we wanted to eventually move these plugins into GHNS, we could either integrate a GHNS chooser into Gwenview like Dolphin does, or just continue to call an appropriate URL, because Discover has a GHNS backend and can install GHNS content.

I was more thinking about installing binary packages (I don't think GHNS can host those?). Also, I don't think we should release our software (plugins in this case) via GHNS, because there needs to be a clear distinction between trusted distro-provided packages and random untrusted user-submitted content.

Anyway, to answer my own question: https://lxr.kde.org/search?_filestring=&_string=appstream%3A%2F%2F

In D13197#271131, @rkflx wrote:

appstream://photolayoutseditor.desktop

I wonder whether it would be feasible to ship something like kipi-plugins.desktop via upstream R480 Kipi Plugins? (Just an idea in case someone wants to investigate that, I don't have time to work in it right now.)

Yeah, we should definitely ship the appstream metadata ourselves. A .desktop file isn't enough, it needs an appdata.xml file too. I can see about grabbing the one from Neon's packaging and submitting a better version of it to the official repo.

In D13197#271103, @rkflx wrote:

Hm, I don't really like leaking that implementation detail. Let me think about an alternative way.

With D13252, you probably won't need to check for installPluginAction anymore. Please have a look, and if you think this will work and my patch landed in the repo, rebase your patch on that.

In D13197#272713, @faridb wrote:

Rebase on D13252

LGTM, and indeed looks much nicer. You are really close to landing your first code contribution to KDE, but:

I have also made a small change to fix the following bug:

Yay, thanks for fixing this! I always wondered about this: Sometimes the button would work, sometimes you needed a second click. Looks like by calling updateMenu less often, updating actually works better (perhaps due to some Qt-internal races?).

However, I have to ask you to please submit a separate Diff for that and remove it here. Later on it will help tremendously when using git blame while trying to figure something out when the commit matches with the commit message and is only about a single topic. We even have a FAQ on that, as it's a common stumbling block: https://community.kde.org/Infrastructure/Phabricator#.22Please_do_that_in_a_separate_commit.22

Thanks for understanding!

Ah, one more thing: You might want to update the summary a bit to reflect the current state of the patch, as it will become the commit message. Simply click on Edit Revision on the top.

I updated the summary. Is it a suitable commit message?

In D13197#272787, @faridb wrote:

I updated the summary. Is it a suitable commit message?

Yup, LGTM. You already followed most tips from https://chris.beams.io/posts/git-commit, which is great. BTW, you can use https://secure.phabricator.com/book/phabricator/article/remarkup/ for everything on Phab (in the future, no need to change it here again ;)

Spectacle also uses KIPI plugins. A similar patch might be useful there as well :)

In D13197#282467, @nicolasfella wrote:

Spectacle also uses KIPI plugins. A similar patch might be useful there as well :)

@nicolasfella Feel free to open a new bug for that, so this does not get lost in a random Phab comment ;)