Review Request: [Picture Frame] Update function for url's and configuration of download path

[etienne.rebetez at oberwallis.ch]

> On 2010-03-29 11:08:34, Sebastian Kügler wrote:
> >

Thanks for your review. Next time i'll try to do better. (Allredy in progress)


> On 2010-03-29 11:08:34, Sebastian Kügler wrote:
> > trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h, line 77
> > <http://reviewboard.kde.org/r/3334/diff/1/?file=21279#file21279line77>
> >
> >     Maybe reloadAutomatically() or toggleAutoReload(), the function name isn't really clear

Yes, i'm pretty bad in giving names. 
In the next diff, i'll follow Aarons advice and therefor this function isn't needed anymore.


> On 2010-03-29 11:08:34, Sebastian Kügler wrote:
> > trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h, line 52
> > <http://reviewboard.kde.org/r/3334/diff/1/?file=21279#file21279line52>
> >
> >     trailing spaces, please get rid of those

Ok;)


> On 2010-03-29 11:08:34, Sebastian Kügler wrote:
> > trunk/KDE/kdeplasma-addons/applets/frame/configdialog.cpp, line 160
> > <http://reviewboard.kde.org/r/3334/diff/1/?file=21280#file21280line160>
> >
> >     the livecam naming is a bit confusing, maybe put what it does? Something with automatic reloading?
> >     
> >     no const needed here afaik

i changed that.


> On 2010-03-29 11:08:34, Sebastian Kügler wrote:
> > trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp, line 116
> > <http://reviewboard.kde.org/r/3334/diff/1/?file=21282#file21282line116>
> >
> >     "" -> QString() (also elsewhere)

thanks, didn't know that.


> On 2010-03-29 11:08:34, Sebastian Kügler wrote:
> > trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp, line 119
> > <http://reviewboard.kde.org/r/3334/diff/1/?file=21282#file21282line119>
> >
> >     Use the one from KGlobal here as default that makes more sense

will be kicket out in the next diff.


> On 2010-03-29 11:08:34, Sebastian Kügler wrote:
> > trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp, line 452
> > <http://reviewboard.kde.org/r/3334/diff/1/?file=21282#file21282line452>
> >
> >     kDebug(), English please :)

Yeah. forgot those...


> On 2010-03-29 11:08:34, Sebastian Kügler wrote:
> > trunk/KDE/kdeplasma-addons/applets/frame/imageSettings.ui, line 160
> > <http://reviewboard.kde.org/r/3334/diff/1/?file=21283#file21283line160>
> >
> >     hh:mm should be good enough, this way the text would look less confusing

Good point. I'll change that.


- eti


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3334/#review4733
-----------------------------------------------------------


On 2010-03-27 15:08:02, eti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3334/
> -----------------------------------------------------------
> 
> (Updated 2010-03-27 15:08:02)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> I was quite happy that the picture frame could handle url's in 4.4. 
> So i added the ability to update the picture with an url periodicaly (i.e. live cams, weather data or <picture that changes over time>).  
> This would be a rather small fix, but i also came accross Bug 222759 and with the updatefunction the download path got quite spamed.
> Now it's possible to set the download path and the ability to ovewrite the existing picture when downloaded. 
> 
> By the way, i also removed the picture class from the slideshow class. The slideshow now manages only the path's and the frame class acceses the picture class directly.
> 
> Let me know wat you think;)
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdeplasma-addons/applets/frame/CMakeLists.txt 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/configdialog.cpp 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/frame.h 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/imageSettings.ui 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/picture.h 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/picture.cpp 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/slideshow.h 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/slideshow.cpp 1105894 
>   trunk/KDE/kdeplasma-addons/applets/frame/urlSettings.ui PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/3334/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> eti
> 
>