[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:       "Aaron Seigo" <aseigo () kde ! org>
Date:       2010-03-23 17:50:31
Message-ID: 20100323175031.352.85352 () localhost
[Download RAW message or body]


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

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