[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: [Picture Frame] Update function for url's and
From:       etienne.rebetez () oberwallis ! ch
Date:       2010-03-29 17:00:15
Message-ID: 20100329170015.14908.60828 () localhost
[Download RAW message or body]



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

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel


[prev in list] [next in list] [prev in thread] [next in thread]