When 'kipi-plugins' is not installed, the 'Share' menu shows 'No Plugin Found'. With this patch it will present a link to install the missing plugins instead (Similarly to what the 'Plugins' menu does).
BUG: 386753
ngraham | |
rkflx |
Gwenview |
When 'kipi-plugins' is not installed, the 'Share' menu shows 'No Plugin Found'. With this patch it will present a link to install the missing plugins instead (Similarly to what the 'Plugins' menu does).
BUG: 386753
Without the kipi-plugins package installed, click on Share in the Gwenview toolbar.
It should display a menu action 'Install Plugins' which triggers the AppStream URL (appstream://photolayoutseditor.desktop).
Lint Skipped |
Unit Tests Skipped |
app/kipiexportaction.cpp | ||
---|---|---|
116 | This won't work when the user has a non-english language set. Use GwenviewConfig::defaultExportPluginText() instead of the hard-coded string |
app/kipiexportaction.cpp | ||
---|---|---|
116 | Using GwenviewConfig::defaultExportPluginText() will break the default plugin functionality when kipi-plugins is installed because it takes the value of last used plugin. However, I discovered another problem: when 'Install Plugins' is clicked in the Share menu, the action text is 'Install Plugins' whereas in the Plugins menu it has the action text '&Install Plugins' which causes the bug shown in the 3rd screenshot to happen when 'Install Plugins' is used from the Plugins menu but not when used from the Share menu. |
Thanks for this! Works great, exactly as I would expect. Also I couldn't actually get your bug to manifest for me: none of the "Install Plugin" instances ever showed an errant "&". Couple of coding style issues though.
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
app/kipiexportaction.cpp | ||
---|---|---|
126–128 | Coding style: space after the if | |
127 | Coding style: use braces even for single-line if clauses. | |
app/kipiinterface.cpp | ||
377 | Same: braces please! |
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!
Made the changes to use mInstallPluginAction instead of the action's text.
It also solved the bug I was having with the Plugins menu.
@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:
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.
app/kipiinterface.cpp | ||
---|---|---|
388–391 | Hm, I don't really like leaking that implementation detail. Let me think about an alternative way. |
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.)
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
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.
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.
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.
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 ;)
@nicolasfella Feel free to open a new bug for that, so this does not get lost in a random Phab comment ;)