----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
