[Differential] [Commented On] D1976: Image of the Day wallpaper plugin

[davidedmundson (David Edmundson)]

davidedmundson added a comment.


  Seems generally good.

INLINE COMMENTS

> config.qml:27
> +
> +Column {
> +    id: root;

If you switch these to ColumnLayout / RowLayout (from QtQuick.Layouts, which you're currently not using) you can get rid of a lot of the anchors and widths in this code.

> config.qml:56
> +        id: sizePositionRow;
> +        spacing: units.largeSpacing / 2;
> +

it defeats the point of having spacing as semanticly defined macros, if people then do maths with it to get any arbitrary value

> config.qml:106
> +
> +            width: formAlignment - units.largeSpacing;
> +            anchors.verticalCenter: bgColorButton.verticalCenter;

this won't acheive anything

you've set a width, but by default there's no eliding, so it'll just overflow past here anyway.

> config.qml:264
> +        id: bgColorDialog;
> +        objectName: "bgColorDialog";
> +

why?

> main.qml:93
> +            changeImage();
> +            resetTimer();
> +        }

where is resetTimer defined?

> main.qml:105
> +            anchors.bottomMargin: Math.min((parent.height / 100) * 7.5, (parent.width / 100) * 7.5);
> +            anchors.rightMargin: 5;
> +            opacity: 0.25;

we generally try to avoid pixel sizes.

> main.qml:111
> +            asynchronous: true;
> +            mipmap: true;
> +        }

why?

you're displaying this at a fixed size

REPOSITORY
  rKDEPLASMAADDONS Plasma Addons

REVISION DETAIL
  https://phabricator.kde.org/D1976

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: bgupta, #plasma
Cc: davidedmundson, plasma-devel, #plasma, jensreuterberg, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160623/8d2eabd4/attachment-0001.html>