Review Request 122668: port of comic applet to plasma5

Tue Mar 3 02:36:06 UTC 2015

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

Ship it!

A bunch of niggles, nothing really big, but would be nice if we had these bits cleaned up before shipping the comic applet.

CMakeLists.txt
<https://git.reviewboard.kde.org/r/122668/#comment52910>

    Can we remove KDELibs4Support? It doesn't seem to be used in the linker.

applets/comic/checknewstrips.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52911>

    Remove those commented lines?

applets/comic/comic.h
<https://git.reviewboard.kde.org/r/122668/#comment52912>

    *availableComicsModel

    Or maybe just run the whole thing through astyle in a separate commit.

applets/comic/comic.h
<https://git.reviewboard.kde.org/r/122668/#comment52913>

    remove commented lines

applets/comic/comic.h
<https://git.reviewboard.kde.org/r/122668/#comment52917>

    There's an l too much at the end of the property name here, may be fixed as well while we're at it.

applets/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52914>

    compile time connect?

applets/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52915>

    compile-time connect?

applets/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52916>

    This looks a bit brittle. Perhaps we can propagate the busy / loading state throug a property and have the BusyWidget pick it up from there?

applets/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52918>

    const?

applets/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52919>

    const

applets/comic/comicupdater.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52920>

    The "intervall" typo again. If we fix this, we must not forget the config name here.

applets/comic/comicupdater.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52921>

    compile-time connection?

applets/comic/package/contents/config/config.qml
<https://git.reviewboard.kde.org/r/122668/#comment52923>

    inconsistent with other QtQuick imports (e.g. 2.1 in the next file)

applets/comic/package/contents/ui/ComicCentralView.qml
<https://git.reviewboard.kde.org/r/122668/#comment52924>

    units.gridUnit

applets/comic/package/contents/ui/ComicCentralView.qml
<https://git.reviewboard.kde.org/r/122668/#comment52925>

    clean...

applets/comic/package/contents/ui/configAppearance.qml
<https://git.reviewboard.kde.org/r/122668/#comment52926>

    over -> mouse over?

applets/comic/package/contents/ui/main.qml
<https://git.reviewboard.kde.org/r/122668/#comment52927>

    use gridUnits?

applets/comic/stripselector.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52922>

    compile-time connections

dataengines/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52928>

    // comment out the debug statements before pushing please

dataengines/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52929>

    I'm not sure if we should risk quitting the application here. Seems a bit rough to quit the Plasma shell when something goes wrong in the comic widget.

dataengines/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52930>

    Also here, qCritical seems to harsh.

dataengines/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52931>

    This may "litter" the log file too much, I think. Maybe qDebug() here?

dataengines/comic/comic.cpp
<https://git.reviewboard.kde.org/r/122668/#comment52932>

    remove?

dataengines/comic/comicproviderwrapper.h
<https://git.reviewboard.kde.org/r/122668/#comment52933>

    doesn't seem to be initialized to 0 in the .cpp file. Potentially dangerous later on.

- Sebastian Kügler

On Feb. 25, 2015, 5 p.m., Marco Martin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122668/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2015, 5 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> -------
> 
> this ports the comic applet to plasma5 (and the dataengine to kpackage, dropping the support of c++ comic plugins that don't exist anymore since KDE 4.3 anyways)
> 
> 
> Diffs
> -----
> 
>   dataengines/comic/comicproviderkross.h d80a686 
>   dataengines/comic/comicprovider.h db3c3f1 
>   dataengines/comic/comic.h 38a3cc2 
>   dataengines/CMakeLists.txt 41288df 
>   applets/comic/plasma-comic-default.desktop 69874bf 
>   applets/comic/package/contents/ui/main.qml 1862475 
>   applets/comic/package/contents/ui/configGeneral.qml PRE-CREATION 
>   applets/comic/package/contents/ui/configAppearance.qml PRE-CREATION 
>   applets/comic/package/contents/ui/configAdvanced.qml PRE-CREATION 
>   applets/comic/package/contents/ui/ImageWidget.qml e450db6 
>   applets/comic/package/contents/ui/FullViewWidget.qml f76bd4c 
>   applets/comic/package/contents/ui/ComicCentralView.qml 4204ddc 
>   applets/comic/appearanceSettings.ui 5dc1144 
>   applets/comic/comicdata.h 696dc8a 
>   applets/comic/comicinfo.cpp c7372ae 
>   applets/comic/comicmodel.h 87b5a0f 
>   applets/comic/comicmodel.cpp 349019d 
>   applets/comic/comicsaver.cpp cf92c76 
>   applets/comic/comicupdater.h PRE-CREATION 
>   applets/comic/comicupdater.cpp PRE-CREATION 
>   applets/comic/configwidget.h 0bf450f 
>   applets/comic/configwidget.cpp 0d7b149 
>   applets/comic/package/contents/config/config.qml PRE-CREATION 
>   applets/comic/package/contents/ui/ButtonBar.qml d10b915 
>   applets/comic/comicarchivedialog.cpp 35902d8 
>   applets/comic/comicarchivejob.h 2ee1428 
>   dataengines/comic/comicprovider.cpp 2e5bb16 
>   dataengines/comic/comic_package.cpp c4d466f 
>   dataengines/comic/comicproviderkross.cpp c2c9ee9 
>   dataengines/comic/comicproviderwrapper.h 499f338 
>   dataengines/comic/comicproviderwrapper.cpp f60a4f6 
>   dataengines/comic/plasma-packagestructure-comic.desktop e05786a 
>   applets/comic/package/contents/ui/ComicBottomInfo.qml f5885b1 
>   applets/comic/comicdata.cpp f61be2f 
>   applets/comic/comicarchivejob.cpp 68b42d2 
>   applets/comic/comicarchivedialog.h 2bd6551 
>   applets/comic/comic.knsrc e9ddbf7 
>   applets/comic/checknewstrips.cpp 8558c9e 
>   applets/comic/advancedsettings.ui 9fc4d21 
>   applets/comic/CMakeLists.txt 9b41b99 
>   CMakeLists.txt 45d05c7 
>   applets/CMakeLists.txt c65250a 
>   dataengines/comic/comic.cpp 5eab3fc 
>   dataengines/comic/comic_package.h 06b4b52 
>   dataengines/comic/CMakeLists.txt 28ef44f 
>   applets/comic/stripselector.cpp cd88ea3 
>   applets/comic/package/metadata.desktop c015137 
>   applets/comic/comicSettings.ui fd73d47 
>   applets/comic/comic.h d460b11 
>   applets/comic/comic.cpp 8352eee 
> 
> Diff: https://git.reviewboard.kde.org/r/122668/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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