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/#review99665
-----------------------------------------------------------




CMakeLists.txt (line 106)
<https://git.reviewboard.kde.org/r/129072/#comment67007>

    What's the point of this? Simply check at runtime if KIO knows of a handler for appstream:// and if not don't display the option?
    
    DesktopExecParser::hasSchemeHandler or somesuch



app/kipiinterface.h (line 32)
<https://git.reviewboard.kde.org/r/129072/#comment67008>

    libkipi should watch the dir and notify consumers of changed plugins, there is no reason why gwenview would watch for plugins of kipi instead of libkipi itself



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

    use nullptr



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

    i18nc is your friend to give semantic context to the string https://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics



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

    use nullptr



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

    use by-reference syntax https://wiki.qt.io/New_Signal_Slot_Syntax



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

    debug



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

    nullptr



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

    what's with the 5 seconds?


- Harald Sitter


On Sept. 29, 2016, 12:53 p.m., Jonathan Riddell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129072/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2016, 12:53 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
> -----
> 
>   CMakeLists.txt 82b219a 
>   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/20160929/d35f7f47/attachment.html>