----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113207/#review41613 -----------------------------------------------------------
Ship it! Code looks pretty good, there's a bunch of nitpicks, but mostly minor stuff. What I'm not happy about is the amount of -- apparently -- copied code, the shell package and the containment. If we need those things, plasmate should probably just depend on the respective repos. Copying code is just a huge maintainance burden. Nevertheless, I think we can merge this already. plasmoidviewer/CMakeLists.txt <http://git.reviewboard.kde.org/r/113207/#comment30407> KF5::plasma We decided to use target names, not variables, as targets will throw errors, while vars can be empty. plasmoidviewer/main.cpp <http://git.reviewboard.kde.org/r/113207/#comment30413> Kill dead code. (So it's dead twice. ;)) plasmoidviewer/main.cpp <http://git.reviewboard.kde.org/r/113207/#comment30411> KAboutData here can go. plasmoidviewer/main.cpp <http://git.reviewboard.kde.org/r/113207/#comment30410> create the QApplication as first thing in main(), good practice. (Some things assume a QApp has been created, so this is safer in those cases.) plasmoidviewer/main.cpp <http://git.reviewboard.kde.org/r/113207/#comment30408> Use QStringLiteral for the second arg here and in the following lines, that makes it a bit easier to use a cheaper QString ctor. plasmoidviewer/main.cpp <http://git.reviewboard.kde.org/r/113207/#comment30414> The [null] is confusing. What's the idea here? plasmoidviewer/main.cpp <http://git.reviewboard.kde.org/r/113207/#comment30412> This exposes a bit of ugly API in View. s_ is used as prefix for file statics, static methods are usually not prefixed. Feel free to address this in View. plasmoidviewer/main.cpp <http://git.reviewboard.kde.org/r/113207/#comment30409> Delete this part. plasmoidviewer/qmlpackages/containment/metadata.desktop <http://git.reviewboard.kde.org/r/113207/#comment30423> dupe plasmoidviewer/qmlpackages/shell/metadata.desktop <http://git.reviewboard.kde.org/r/113207/#comment30424> This package is in plasma-framework, no? In that case, we *know* it's there (you can't build plasmoidviewer without plasma-framework installed), so we don't need a copy here. Or do we? I'd also like to get rid of the copy of the desktopcontainment, possible? plasmoidviewer/view.h <http://git.reviewboard.kde.org/r/113207/#comment30415> createCorona(), see other comment plasmoidviewer/view.cpp <http://git.reviewboard.kde.org/r/113207/#comment30416> clean please. plasmoidviewer/view.cpp <http://git.reviewboard.kde.org/r/113207/#comment30417> Would make sense to give the applet name here, that's the most likely mistake. Should be qCritical of qFatal. plasmoidviewer/view.cpp <http://git.reviewboard.kde.org/r/113207/#comment30418> WHY SHOUT? :P plasmoidviewer/view.cpp <http://git.reviewboard.kde.org/r/113207/#comment30419> Do we want to set a null containment? plasmoidviewer/view.cpp <http://git.reviewboard.kde.org/r/113207/#comment30420> QStringLiteral for the strings here, and in the following conditions. plasmoidviewer/view.cpp <http://git.reviewboard.kde.org/r/113207/#comment30421> We check for a null pointer above, but still will crash. Almost smart :) put the c-> call into the conditional above. plasmoidviewer/view.cpp <http://git.reviewboard.kde.org/r/113207/#comment30422> QStringLiteral() here and below - Sebastian Kügler On Oct. 11, 2013, 6:40 p.m., Antonis Tsiapaliokas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/113207/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2013, 6:40 p.m.) > > > Review request for Plasma. > > > Repository: plasmate > > > Description > ------- > > The plasmoidviewer2 branch contains the initial support for plasmoidviewer2. > Right now the plasmoidviewer is able to load the applet, setup the location > and the formfactor. > > > Diffs > ----- > > CMakeLists.txt 2b50b09 > plasmoidviewer/CMakeLists.txt d36fddb > plasmoidviewer/main.cpp f8a6b32 > plasmoidviewer/qmlpackages/containment/Messages.sh PRE-CREATION > plasmoidviewer/qmlpackages/containment/contents/code/LayoutManager.js > PRE-CREATION > plasmoidviewer/qmlpackages/containment/contents/config/main.xml > PRE-CREATION > plasmoidviewer/qmlpackages/containment/contents/ui/AppletAppearance.qml > PRE-CREATION > plasmoidviewer/qmlpackages/containment/contents/ui/BusyOverlay.qml > PRE-CREATION > plasmoidviewer/qmlpackages/containment/contents/ui/main.qml PRE-CREATION > plasmoidviewer/qmlpackages/containment/metadata.desktop PRE-CREATION > plasmoidviewer/qmlpackages/shell/Messages.sh PRE-CREATION > plasmoidviewer/qmlpackages/shell/contents/applet/AppletError.qml > PRE-CREATION > plasmoidviewer/qmlpackages/shell/contents/applet/CompactApplet.qml > PRE-CREATION > > plasmoidviewer/qmlpackages/shell/contents/applet/DefaultCompactRepresentation.qml > PRE-CREATION > > plasmoidviewer/qmlpackages/shell/contents/configuration/AppletConfiguration.qml > PRE-CREATION > > plasmoidviewer/qmlpackages/shell/contents/configuration/ConfigCategoryDelegate.qml > PRE-CREATION > > plasmoidviewer/qmlpackages/shell/contents/configuration/ConfigurationShortcuts.qml > PRE-CREATION > > plasmoidviewer/qmlpackages/shell/contents/configuration/MouseEventInputButton.qml > PRE-CREATION > plasmoidviewer/qmlpackages/shell/contents/defaults PRE-CREATION > plasmoidviewer/qmlpackages/shell/contents/layout.js PRE-CREATION > plasmoidviewer/qmlpackages/shell/contents/loader.qml PRE-CREATION > plasmoidviewer/qmlpackages/shell/contents/views/Desktop.qml PRE-CREATION > plasmoidviewer/qmlpackages/shell/metadata.desktop PRE-CREATION > plasmoidviewer/view.h PRE-CREATION > plasmoidviewer/view.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/113207/diff/ > > > Testing > ------- > > > Thanks, > > Antonis Tsiapaliokas > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel