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

[Aaron Seigo]

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


personally, i don't see the need for a url configuration page at all. the download path should be the from desktop-wide coniguration and the picture should always be overwritten (the frame plasmoid is not a downloader; see comments below for more on this).

the "update picture every" option doesn't follow the same UI style as seen on e.g. the slideshow configuration. it should probably be something like

Autoupdate: [  01 hours 00 Mins ]

with a specialValueText of "Never" so when it is set to 0 hours and 0 mins, it shows (and means) "Never"


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

    Spelling and grammar mistakes.



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

    shouldn't this be in the cache directory, rather than in the user's downloads folder? and yes, i see that you just pulled this from Picture::slotFinished, but that looks wrong too :)
    
    i guess the real question is this: is the picture frame an image collection downloader or is it a "show a picture on my computer screen" plasmoid? if it's the latter, then there's no need to save the image somewhere permanent (e.g. ~/Downloads) and there's no need to have a "should we overwrite the picture" option as it should only ever keep one image around on disk at any time: the image it's currently showing.



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

    please pay attention to whitespace usage, keep the code in the files consistent.


- Aaron


On 2010-03-21 19:46:38, eti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3334/
> -----------------------------------------------------------
> 
> (Updated 2010-03-21 19:46:38)
> 
> 
> 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
> -------
> 
> 
> Screenshots
> -----------
> 
> Picture Frame settings
>   http://reviewboard.kde.org/r/3334/s/335/
> 
> 
> Thanks,
> 
> eti
> 
>