Review Request 113207: Plasmate: Request for merge, branch plasmoidviewer2 to frameworks-scratch

Sat Oct 12 19:20:32 UTC 2013

-----------------------------------------------------------
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
> 
>

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