Review Request 129072: install kipi-plugins in gwenview using appstream:/ URLs

[Harald Sitter]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129072/#review99726
-----------------------------------------------------------




app/kipiinterface.cpp (line 351)
<https://git.reviewboard.kde.org/r/129072/#comment67041>

    https://wiki.qt.io/New_Signal_Slot_Syntax



app/kipiinterface.cpp (line 359)
<https://git.reviewboard.kde.org/r/129072/#comment67043>

    Why are you introducing an unused parameter in a completely new function?



app/kipiinterface.cpp (line 360)
<https://git.reviewboard.kde.org/r/129072/#comment67044>

    Given that this is the only thing it calls, and it does so from only one connection I think this function should go away and you should use a lambda in the `connect(...)` instead.



app/kipiinterface.cpp (line 368)
<https://git.reviewboard.kde.org/r/129072/#comment67040>

    so. upon reflecting on the qtimer issue I think you are missing a trick here.
    
    the watcher needs to be persistent. you'd see that if you implemented this in libkipi...
    
    what you want to do is watch for changes. not one change. many changes. arbitrary changes that you do not know when they appear.
    
    why is this you may ask. libkipi would not know that an install action was triggered, that's a frontend thing. what libkipi should care about is plugins changing.
    
    if I uninstall kipi-plugins the plugins should disappear. if gwenview is running and I install digikam (which pulls in kipi-plugins), gwenview should now list the kipi plugins and not point me to a discover window which will tell me that the thing is already installed.
    
    ultimately this would also make the qtimer business work more reliably. if you have a persistent watcher it can continiously send the update signal which means that iff the installation takes longer than whatever arbitrary timeout you defne the menu simply gets reloaded again. which is annoying and meh, but doesn't need additional code and will make sure all plugins are actually correctly listed.


- Harald Sitter


On Sept. 30, 2016, 5:05 p.m., Jonathan Riddell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129072/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2016, 5:05 p.m.)
> 
> 
> Review request for Gwenview, Kipi-plugins, Aleix Pol Gonzalez, and Harald Sitter.
> 
> 
> Repository: gwenview
> 
> 
> Description
> -------
> 
> Alternative to patch https://git.reviewboard.kde.org/r/129004 for comparison, this uses an appstream:// URL to launch Plasma Discover (or whatever you have that handles those URLs) to install kipi-plugins.  A file watcher will update the menu when it spots the plugins have been installed.
> 
> As discussed on other review request, I prefer the built in approach as user experience.  Discover is quite a complex window that gets shown to the user and it may not be clear that the user needs to click install and wait then close Discover and check the menu again.  The simple modal dialog of the other patch seems easier to users.
> 
> 
> Diffs
> -----
> 
>   app/kipiinterface.h 1c06cd4 
>   app/kipiinterface.cpp 04dccb3 
> 
> Diff: https://git.reviewboard.kde.org/r/129072/diff/
> 
> 
> Testing
> -------
> 
> built it, tried to use discover to install kipi (reverted to using apt as it doesn't work on Neon currently) and noted the menu gets filled
> 
> 
> Thanks,
> 
> Jonathan Riddell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/gwenview-devel/attachments/20161003/b69bab76/attachment.html>