Review Request 120276: Initial port to frameworks for the comic dataengine.

[Martin Klapetek]

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


Progressing nicely. A piece of advice - leave the codestyle changes always till the end, it's now hard to review because there are loooots of changes and it's unclear which is just coding style and which not. But we'll manage ;)


dataengines/comic/CMakeLists.txt
<https://git.reviewboard.kde.org/r/120276/#comment46719>

    Why add the \?



dataengines/comic/CMakeLists.txt
<https://git.reviewboard.kde.org/r/120276/#comment46721>

    The plugins cannot be loaded without the .desktop file, this needs to stay in



dataengines/comic/cachedprovider.cpp
<https://git.reviewboard.kde.org/r/120276/#comment46723>

    You don't need to create the QUrl, just return the .toUrl() directly



dataengines/comic/cachedprovider.cpp
<https://git.reviewboard.kde.org/r/120276/#comment46724>

    Also here



dataengines/comic/cachedprovider.cpp
<https://git.reviewboard.kde.org/r/120276/#comment46725>

    ...aaand here



dataengines/comic/comic.cpp
<https://git.reviewboard.kde.org/r/120276/#comment46728>

    These still have space around [ ]



dataengines/comic/comic_package.cpp
<https://git.reviewboard.kde.org/r/120276/#comment46715>

    You should really keep this and instead pass the *parent to the parent class



dataengines/comic/comic_package.cpp
<https://git.reviewboard.kde.org/r/120276/#comment46716>

    You're building this class as MODULE (ie. a plugin) but you don't define any plugin macro (K_PLUGIN_FACTORY), this can't work as a plugin without it
    
    Looks like you should keep this line (but port to K_PLUGIN_FACTORY)



dataengines/comic/comicprovider.h
<https://git.reviewboard.kde.org/r/120276/#comment46717>

    This macro is still useless, remove it and replace its usage with K_PLUGIN_FACTORY


- Martin Klapetek


On Sept. 19, 2014, 9:17 p.m., Andrei Amuraritei wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120276/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2014, 9:17 p.m.)
> 
> 
> Review request for Plasma, David Edmundson, Marco Martin, and Sebastian Kügler.
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> -------
> 
> comic DataEngine initial port to frameworks.
> 
> 
> Diffs
> -----
> 
>   dataengines/comic/plasma-packagestructure-comic.desktop 67f96c0 
>   dataengines/comic/comic_package.cpp 6d2ff0b 
>   dataengines/comic/comicprovider.h 630ee8d 
>   dataengines/comic/comicprovider.cpp ab248a5 
>   dataengines/comic/comicproviderkross.h 46a9072 
>   dataengines/comic/comicproviderkross.cpp 9820f05 
>   dataengines/comic/comicproviderwrapper.h 81eee68 
>   dataengines/comic/comicproviderwrapper.cpp 48ced42 
>   dataengines/comic/plasma-dataengine-comic.desktop f9e5433 
>   dataengines/comic/CMakeLists.txt 8e382e6 
>   dataengines/comic/cachedprovider.cpp caca25e 
>   dataengines/comic/comic.h 8cc3969 
>   dataengines/comic/comic.cpp 7130f44 
>   dataengines/comic/comic_package.h 32be381 
> 
> Diff: https://git.reviewboard.kde.org/r/120276/diff/
> 
> 
> Testing
> -------
> 
> Building from source, compiles 100%, some deprecated warnings. DataEngine shows up in plasmaengineexplorer and detects installed .comic packages.
> This is the initial port, still need to review code to fix issues like whitespaces around ( or the deprecated parts.
> Thanks notmart, d_ed, sebas, bshas etc for helping.
> 
> 
> Thanks,
> 
> Andrei Amuraritei
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140919/c894e07c/attachment-0001.html>