Review Request 126793: KF5 (Picture) Frame plasmoid

Fri Feb 5 12:47:10 UTC 2016

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/package/contents/config/main.xml, line 9
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434179#file434179line9>
> >
> >     Where does this come from?

I've set that value based on what I found a resonable default for the slideshow interval - are there any KDE / global specifics on this?

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/package/contents/config/config.qml, line 31
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434178#file434178line31>
> >
> >     This is an action icon and shouldn't be used for this prupose

Cool - using "folder" instead

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/package/contents/ui/ConfigPaths.qml, line 35
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434181#file434181line35>
> >
> >     pathList should be a StringList which will result in a JS Array, then you don't need all of this JSON.parse stuff.

Good point. I will still need the JSON parse for the contents of the array though (objects `{ path: "/some/path", "type":"file/dir" }` ) - unless there's another way?

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/package/contents/ui/main.qml, line 585
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434182#file434182line585>
> >
> >     I think there's a plasmoid.needsConfiguring property that does exactly that.

I can't find it - and using plasmoid.setConfigurationRequired(true,'Testing 1 2 3') doesn't work for me?

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/package/contents/ui/main.qml, lines 57-58
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434182#file434182line57>
> >
> >     Layout.minimumWidth / height ?

I'm using these instead now
```
width: units.gridUnit * 20
height: units.gridUnit * 13
```

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/package/contents/ui/main.qml, line 299
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434182#file434182line299>
> >
> >     I would like to be able to configure this

Added a configuration option to change it

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/package/contents/ui/main.qml, line 445
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434182#file434182line445>
> >
> >     I guess if you placed the MouseArea behind (ie. code-wise, above) the buttons, it would just work?

Not quite as hovering the buttons will then hide the overlay :(

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/plugin/mediaframe.cpp, line 43
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434185#file434185line43>
> >
> >     Don't we have something re-usable elsewhere for this?

No clue?

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/plugin/mediaframe.cpp, line 242
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434185#file434185line242>
> >
> >     We don't use the "callback" battern.

signals?

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/plugin/mediaframe.cpp, line 98
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434185#file434185line98>
> >
> >     QString::fromLatin1 or so?

Which one? The str or the return value?

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/plugin/mediaframe.cpp, line 108
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434185#file434185line108>
> >
> >     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

I don't know of any faster:
http://stackoverflow.com/a/16351957/1904615

I've used the .isEmpty()

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/plugin/mediaframe.cpp, line 114
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434185#file434185line114>
> >
> >     Doesn't static QFile::exists() do that? I don't know if it returns true for folders, though.

I can't find anything in the documentation on what it does for folder :/

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/plugin/mediaframe.cpp, lines 131-132
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434185#file434185line131>
> >
> >     You're creating an url just to make a local file from it again?

It's to make sure that the path/url can be read by QDir, QFile etc.
QDir and QFile doesn't work on my system when the protocol is present?

So this makes it easier to check for remote files via ftp:// or http:// etc.

> On Jan. 18, 2016, noon, Kai Uwe Broulik wrote:
> > applets/mediaframe/plugin/mediaframe.cpp, line 142
> > <https://git.reviewboard.kde.org/r/126793/diff/1/?file=434185#file434185line142>
> >
> >     I wonder if this should be done in another thread, have a look at [plasma-workspace]/wallpapers/image/image.cpp where it does that.

It should be done in a thread - I just haven't got time right now to implement a decent directory reader :(

- Lars

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

On Jan. 18, 2016, 11:27 a.m., 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 a.m.)
> 
> 
> 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/20160205/acf4a64a/attachment-0001.html>