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

[Sebastian Kügler]

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



trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h
<http://reviewboard.kde.org/r/3334/#comment4233>

    trailing spaces, please get rid of those



trunk/KDE/kdeplasma-addons/applets/frame/configdialog.h
<http://reviewboard.kde.org/r/3334/#comment4235>

    Maybe reloadAutomatically() or toggleAutoReload(), the function name isn't really clear



trunk/KDE/kdeplasma-addons/applets/frame/configdialog.cpp
<http://reviewboard.kde.org/r/3334/#comment4234>

    remove spacs (... and trailing



trunk/KDE/kdeplasma-addons/applets/frame/configdialog.cpp
<http://reviewboard.kde.org/r/3334/#comment4236>

    the livecam naming is a bit confusing, maybe put what it does? Something with automatic reloading?
    
    no const needed here afaik



trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4238>

    "" -> QString() (also elsewhere)



trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4237>

    Use the one from KGlobal here as default that makes more sense



trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4239>

    use kDebug



trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4240>

    kDebug()



trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4241>

    kDebug(), English please :)



trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4242>

    You're creating a magic value here (""), why not use the KGlobal downloadpath?
    
    Also, this statement should read !m_downloadPath.isEmpty()



trunk/KDE/kdeplasma-addons/applets/frame/frame.cpp
<http://reviewboard.kde.org/r/3334/#comment4243>

    space between ) and {



trunk/KDE/kdeplasma-addons/applets/frame/imageSettings.ui
<http://reviewboard.kde.org/r/3334/#comment4244>

    Useful (single l), came -> cam



trunk/KDE/kdeplasma-addons/applets/frame/imageSettings.ui
<http://reviewboard.kde.org/r/3334/#comment4245>

    hh:mm should be good enough, this way the text would look less confusing



trunk/KDE/kdeplasma-addons/applets/frame/picture.h
<http://reviewboard.kde.org/r/3334/#comment4246>

    trailing space



trunk/KDE/kdeplasma-addons/applets/frame/picture.cpp
<http://reviewboard.kde.org/r/3334/#comment4247>

    4 spaces indenting



trunk/KDE/kdeplasma-addons/applets/frame/urlSettings.ui
<http://reviewboard.kde.org/r/3334/#comment4248>

    get -> gets



trunk/KDE/kdeplasma-addons/applets/frame/urlSettings.ui
<http://reviewboard.kde.org/r/3334/#comment4249>

    overwritten


- Sebastian


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