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