Review Request: Adding "Set Wallpaper Image" option in Picture frame

[Aaron J. Seigo]

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



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1434>

    should be "Set as Wallpaper Image"?



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1436>

    missing whitespace.



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1439>

    missing whitespace.



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1440>

    missing whitespace and incorrectly indented.



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1435>

    missing whitespace.
    
    more importantly, it shouldn't be checking the name() since that is translated (and can change whenever we feel like changing it in the .desktop file). it should be checking pluginName() instead.



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1438>

    missing whitespace.



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1437>

    missing whitespace.



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1441>

    this is broken in various ways:
    
    * if the image in the frame changes in the next 300ms, the wrong image will be set
    
    * it's an arbitrary time between when the wallpaper plugin is loaded (which, btw, is immediately) and this is called. there is nothing that says that 300ms is enough time.
    
    in any case, the _real_ fix here is to check in Plasma::Wallpaper::setUrls() if the wallpaper has been initialized yet and if not to do so. i'll make this fix in wallpaper and then this line can just be removed.



applets/frame/frame.cpp
<http://git.reviewboard.kde.org/r/100764/#comment1442>

    you need to check if (containment()->wallpaper()) here as setWallpaper can fail.


- Aaron J.


On Feb. 27, 2011, 7:55 p.m., Sinny Kumari wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100764/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2011, 7:55 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Adding "Set Wallpaper Image" feature in Picture Frame. If the User right clicks on Picture Frame, there will be an option "Set Wallpaper Image". This Option will set the current Image Of Picture Frame as Wallpaper Image. Done changes according to suggestion given by Todd and Aaron. Continuation of http://svn.reviewboard.kde.org/r/6416/.
> 
> 
> Diffs
> -----
> 
>   applets/frame/frame.cpp 871ebef 
>   applets/frame/frame.h bf7b95d 
> 
> Diff: http://git.reviewboard.kde.org/r/100764/diff
> 
> 
> Testing
> -------
> 
> working fine
> Added  QTimer::singleShot(300,this, SLOT(setImageAsWallpaper())); line in frame.cpp because Plugin takes some time to load.
> Not sure it is correct way to do or not. Any suggestion?
> 
> 
> Thanks,
> 
> Sinny
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20110303/706ae1d1/attachment-0001.htm