Review Request 126793: KF5 (Picture) Frame plasmoid

Mon Jan 18 12:00:22 UTC 2016

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

Nice to see this applet coming back!

I was wondering if there could be more code shared between this applet and our image wallpaper, perhaps even "just" allow the image wallpaper to function also as an applet.

You have a lot of empty lines with whitespace in your code. Please check your editor settings.

applets/mediaframe/package/contents/code/utility.js (line 19)
<https://git.reviewboard.kde.org/r/126793/#comment62318>

    This function seems unused

applets/mediaframe/package/contents/code/utility.js (line 29)
<https://git.reviewboard.kde.org/r/126793/#comment62319>

    Also given this one is used only once I wonder if the utility.js is needed at all.

applets/mediaframe/package/contents/config/config.qml (line 31)
<https://git.reviewboard.kde.org/r/126793/#comment62320>

    This is an action icon and shouldn't be used for this prupose

applets/mediaframe/package/contents/config/main.xml (line 9)
<https://git.reviewboard.kde.org/r/126793/#comment62321>

    Where does this come from?

applets/mediaframe/package/contents/ui/ConfigGeneral.qml (line 34)
<https://git.reviewboard.kde.org/r/126793/#comment62322>

    Can't this also be an alias property? At least try to avoid having the default value hardcoded in two places

applets/mediaframe/package/contents/ui/ConfigGeneral.qml (line 55)
<https://git.reviewboard.kde.org/r/126793/#comment62324>

    I would suggest using a SpinBox here, it also supports appending a suffix

applets/mediaframe/package/contents/ui/ConfigGeneral.qml (line 69)
<https://git.reviewboard.kde.org/r/126793/#comment62323>

    Use Label from QtQuick.Controls instead of QtQuick Text

applets/mediaframe/package/contents/ui/ConfigGeneral.qml (line 80)
<https://git.reviewboard.kde.org/r/126793/#comment62325>

    This shouldn't be neccessary.

    Plasma populates all cfg_foo properties with the stored value or the default if none is set.

applets/mediaframe/package/contents/ui/ConfigGeneral.qml (line 85)
<https://git.reviewboard.kde.org/r/126793/#comment62326>

    These RowLayouts around the actual control seem superfluous

applets/mediaframe/package/contents/ui/ConfigPaths.qml (line 35)
<https://git.reviewboard.kde.org/r/126793/#comment62327>

    pathList should be a StringList which will result in a JS Array, then you don't need all of this JSON.parse stuff.

applets/mediaframe/package/contents/ui/main.qml (lines 47 - 48)
<https://git.reviewboard.kde.org/r/126793/#comment62328>

    Item already has implicitWidth/implicitHeight, no need to define them explicitly

applets/mediaframe/package/contents/ui/main.qml (line 55)
<https://git.reviewboard.kde.org/r/126793/#comment62330>

    PlasmaCore.Types.NoBackground, I think, string enums are deprecated

applets/mediaframe/package/contents/ui/main.qml (lines 57 - 58)
<https://git.reviewboard.kde.org/r/126793/#comment62329>

    Layout.minimumWidth / height ?

applets/mediaframe/package/contents/ui/main.qml (line 63)
<https://git.reviewboard.kde.org/r/126793/#comment62331>

    You can just access plasmoid.configuration, don't store this in a separate object as then it won't update when the config changes.

applets/mediaframe/package/contents/ui/main.qml (lines 65 - 66)
<https://git.reviewboard.kde.org/r/126793/#comment62347>

    I was wondering if, instead of storing the source, you have two Item properties holding the first and other image and then you can just swap the items.

    Perhaps have a look at [plasma-workspace]/wallpapers/image/imagepackage/contents/ui/main.qml

applets/mediaframe/package/contents/ui/main.qml (lines 73 - 75)
<https://git.reviewboard.kde.org/r/126793/#comment62332>

    These could be readonly property

applets/mediaframe/package/contents/ui/main.qml (lines 230 - 260)
<https://git.reviewboard.kde.org/r/126793/#comment62333>

    All of this was unneccessary if you accessed plasmoid.configuration directly

applets/mediaframe/package/contents/ui/main.qml (line 267)
<https://git.reviewboard.kde.org/r/126793/#comment62335>

    The default value is false already

applets/mediaframe/package/contents/ui/main.qml (line 268)
<https://git.reviewboard.kde.org/r/126793/#comment62334>

    The braces are not needed:

    onTriggered: nextItem()

applets/mediaframe/package/contents/ui/main.qml (line 299)
<https://git.reviewboard.kde.org/r/126793/#comment62336>

    I would like to be able to configure this

applets/mediaframe/package/contents/ui/main.qml (line 318)
<https://git.reviewboard.kde.org/r/126793/#comment62337>

    onClicked: Qt.openUrlExternally(activeSource)
    enabled: plasmoid.configuration.openImageOnLeftClick

applets/mediaframe/package/contents/ui/main.qml (line 325)
<https://git.reviewboard.kde.org/r/126793/#comment62338>

    I would put that into a Loader to avoid shader compilation if you're not actually using it

applets/mediaframe/package/contents/ui/main.qml (lines 352 - 353)
<https://git.reviewboard.kde.org/r/126793/#comment62339>

    See if you can use OpacityAnimator instead, it runs directly on the rendering thread and won't block if the application is otherwise busy

applets/mediaframe/package/contents/ui/main.qml (line 355)
<https://git.reviewboard.kde.org/r/126793/#comment62340>

    ScriptAction {
        script: {

applets/mediaframe/package/contents/ui/main.qml (line 375)
<https://git.reviewboard.kde.org/r/126793/#comment62341>

    var type = items.isDir(url) ? "folder" : "file";

applets/mediaframe/package/contents/ui/main.qml (line 402)
<https://git.reviewboard.kde.org/r/126793/#comment62343>

    Prefer " instead of '

applets/mediaframe/package/contents/ui/main.qml (line 403)
<https://git.reviewboard.kde.org/r/126793/#comment62342>

    onClicked: {
        nextTimer.stop();
        previousItem()
    }

applets/mediaframe/package/contents/ui/main.qml (line 417)
<https://git.reviewboard.kde.org/r/126793/#comment62344>

    Avoid using hardcoded pixel sizes, use eg. units.smallSpacing

applets/mediaframe/package/contents/ui/main.qml (line 445)
<https://git.reviewboard.kde.org/r/126793/#comment62345>

    I guess if you placed the MouseArea behind (ie. code-wise, above) the buttons, it would just work?

applets/mediaframe/package/contents/ui/main.qml (lines 454 - 457)
<https://git.reviewboard.kde.org/r/126793/#comment62346>

    Replace this by declarative bindings:

    eg. have the overlay use:
    opacity: overlayMouseArea.containsMouse ? 1 : 0

    and the Timer
    running: whatever && !overlayMouseArea.containsMouse

applets/mediaframe/package/contents/ui/main.qml (line 500)
<https://git.reviewboard.kde.org/r/126793/#comment62348>

    Given you completely re-style the ProgressBar, you might as well make your own component from Rectangles. This way you could use Animators to drive the animation and avoid expensive computations the ProgressBar might be doing.

applets/mediaframe/package/contents/ui/main.qml (line 585)
<https://git.reviewboard.kde.org/r/126793/#comment62349>

    I think there's a plasmoid.needsConfiguring property that does exactly that.

applets/mediaframe/plugin/mediaframe.h (lines 19 - 20)
<https://git.reviewboard.kde.org/r/126793/#comment62352>

    This doesn't match the file's name

applets/mediaframe/plugin/mediaframe.h (line 39)
<https://git.reviewboard.kde.org/r/126793/#comment62350>

    nullptr instead of 0

applets/mediaframe/plugin/mediaframe.h (line 40)
<https://git.reviewboard.kde.org/r/126793/#comment62351>

    virtual ~MediaFrame();

applets/mediaframe/plugin/mediaframe.h (lines 86 - 87)
<https://git.reviewboard.kde.org/r/126793/#comment62353>

    bool m_random = false;
    int m_next = 0;

applets/mediaframe/plugin/mediaframe.cpp (line 43)
<https://git.reviewboard.kde.org/r/126793/#comment62354>

    Don't we have something re-usable elsewhere for this?

applets/mediaframe/plugin/mediaframe.cpp (lines 50 - 51)
<https://git.reviewboard.kde.org/r/126793/#comment62355>

    Prefer new connect syntax:

    connect(&m_watcher, &QFileSystemWatcher::directoryChanged, this, &MediaFrame::slotItemChanged);

    Also, you're in a QObject subclass, no need to speciy QObject::

applets/mediaframe/plugin/mediaframe.cpp (lines 54 - 56)
<https://git.reviewboard.kde.org/r/126793/#comment62356>

    MediaFrame::~MediaFrame() = default;

applets/mediaframe/plugin/mediaframe.cpp (line 63)
<https://git.reviewboard.kde.org/r/126793/#comment62357>

    Whitespace

applets/mediaframe/plugin/mediaframe.cpp (line 69)
<https://git.reviewboard.kde.org/r/126793/#comment62358>

    just bool is sufficient, use const & for complex types

applets/mediaframe/plugin/mediaframe.cpp (lines 71 - 72)
<https://git.reviewboard.kde.org/r/126793/#comment62359>

    if (m_random != random) {

applets/mediaframe/plugin/mediaframe.cpp (lines 80 - 81)
<https://git.reviewboard.kde.org/r/126793/#comment62360>

    if (min > max) {

applets/mediaframe/plugin/mediaframe.cpp (line 88)
<https://git.reviewboard.kde.org/r/126793/#comment62361>

    Add spaces:

    return ((qrand() % (max - min + 1)) + min);

applets/mediaframe/plugin/mediaframe.cpp (line 96)
<https://git.reviewboard.kde.org/r/126793/#comment62363>

    const QString &

applets/mediaframe/plugin/mediaframe.cpp (line 98)
<https://git.reviewboard.kde.org/r/126793/#comment62362>

    QString::fromLatin1 or so?

applets/mediaframe/plugin/mediaframe.cpp (line 108)
<https://git.reviewboard.kde.org/r/126793/#comment62364>

    Is there no faster way than to create a list of files just to check if it's empty?

    Also there's an .isEmpty() method

applets/mediaframe/plugin/mediaframe.cpp (line 114)
<https://git.reviewboard.kde.org/r/126793/#comment62365>

    Doesn't static QFile::exists() do that? I don't know if it returns true for folders, though.

applets/mediaframe/plugin/mediaframe.cpp (line 120)
<https://git.reviewboard.kde.org/r/126793/#comment62366>

    Avoid the boolean trap [1], I have no idea what "false" means if I don't look up the function signature to find out it means "non-recursive", use an enum instead.

    [1] http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html

applets/mediaframe/plugin/mediaframe.cpp (lines 125 - 126)
<https://git.reviewboard.kde.org/r/126793/#comment62367>

    if (…) {

    Also, has() isn't a particularly descriptive name

applets/mediaframe/plugin/mediaframe.cpp (lines 131 - 132)
<https://git.reviewboard.kde.org/r/126793/#comment62368>

    You're creating an url just to make a local file from it again?

applets/mediaframe/plugin/mediaframe.cpp (line 142)
<https://git.reviewboard.kde.org/r/126793/#comment62369>

    I wonder if this should be done in another thread, have a look at [plasma-workspace]/wallpapers/image/image.cpp where it does that.

applets/mediaframe/plugin/mediaframe.cpp (line 176)
<https://git.reviewboard.kde.org/r/126793/#comment62370>

    m_pathMap.insert(path, paths)

applets/mediaframe/plugin/mediaframe.cpp (lines 201 - 202)
<https://git.reviewboard.kde.org/r/126793/#comment62371>

    use .clear() instead

applets/mediaframe/plugin/mediaframe.cpp (line 242)
<https://git.reviewboard.kde.org/r/126793/#comment62372>

    We don't use the "callback" battern.

applets/mediaframe/plugin/mediaframe.cpp (lines 250 - 253)
<https://git.reviewboard.kde.org/r/126793/#comment62373>

    I guess this could be rewritten to be clearer. Also, coding style.

applets/mediaframe/plugin/mediaframe.cpp (line 300)
<https://git.reviewboard.kde.org/r/126793/#comment62374>

    Use QLatin1Char('/') and QLatin1Char('_')

- Kai Uwe Broulik

On Jan. 18, 2016, 11:27 vorm., Lars Pontoppidan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126793/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2016, 11:27 vorm.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> -------
> 
> KF5 version of the (Picture) Frame widget
> 
> 
> Diffs
> -----
> 
>   applets/CMakeLists.txt ed938f8 
>   applets/mediaframe/CMakeLists.txt PRE-CREATION 
>   applets/mediaframe/Messages.sh PRE-CREATION 
>   applets/mediaframe/package/contents/code/utility.js PRE-CREATION 
>   applets/mediaframe/package/contents/config/config.qml PRE-CREATION 
>   applets/mediaframe/package/contents/config/main.xml PRE-CREATION 
>   applets/mediaframe/package/contents/ui/ConfigGeneral.qml PRE-CREATION 
>   applets/mediaframe/package/contents/ui/ConfigPaths.qml PRE-CREATION 
>   applets/mediaframe/package/contents/ui/main.qml PRE-CREATION 
>   applets/mediaframe/package/metadata.desktop PRE-CREATION 
>   applets/mediaframe/plugin/mediaframe.h PRE-CREATION 
>   applets/mediaframe/plugin/mediaframe.cpp PRE-CREATION 
>   applets/mediaframe/plugin/mediaframeplugin.h PRE-CREATION 
>   applets/mediaframe/plugin/mediaframeplugin.cpp PRE-CREATION 
>   applets/mediaframe/plugin/qmldir PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126793/diff/
> 
> 
> Testing
> -------
> 
> kdeplasma-addons builds on Kubuntu 15.10.
> 
> 
> Thanks,
> 
> Lars Pontoppidan
> 
>

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