Review Request: Added System Wallpaper and My Downloaded Wallpaper checkboxes to sildeshow config

[Aaron Seigo]

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


i like the idea a lot. there are two issues that must be addressed before it can be committed, however. 

one is quite trivial: put curly braces around the one liner if statements, kdelibs style. e.g.:

the other is almost as trivial: the new checkboxes need the labels fixed. they also need to be put into the layout properly like the rest of the widgets: a QLabel on right with the text, checkbox (with empty text!) on the right. all the widgets should line up from top to bottom along the "center" line of the label<->widget margin.

with those fixes, this can go in. thanks for the patch :)


/trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.cpp
<http://svn.reviewboard.kde.org/r/5705/#comment8739>

    use braces, even on single line statements.



/trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/slideshowconfig.ui
<http://svn.reviewboard.kde.org/r/5705/#comment8741>

    should be plural and use sentence capitalization: System wallpaper



/trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/slideshowconfig.ui
<http://svn.reviewboard.kde.org/r/5705/#comment8740>

    should be plural, and use sentence capitalization: "My downloaded wallpapers"


- Aaron


On 2010-10-28 15:28:16, Jeremy Whiting wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5705/
> -----------------------------------------------------------
> 
> (Updated 2010-10-28 15:28:16)
> 
> 
> Review request for Plasma and Davide Bettio.
> 
> 
> Summary
> -------
> 
> Prompted by bug https://bugs.kde.org/show_bug.cgi?id=253360 and my own itch at not having the ability to easly add ~/.kde/share/wallpaper/ to the slideshow, I added these two checkboxes from this wishlist bug.  Let me know if you think the tooltips should be reworded I just took an initial stab at them.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.h 1190556 
>   /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/image.cpp 1189844 
>   /trunk/KDE/kdebase/workspace/plasma/generic/wallpapers/image/slideshowconfig.ui 1189844 
> 
> Diff: http://svn.reviewboard.kde.org/r/5705/diff
> 
> 
> Testing
> -------
> 
> I works quite well actually.  I still think we need to disable the apply and ok buttons when no checkbox is checked and no paths are in the listbox, but we work around that by putting the system folder in the list if there are no folders in the kconfig.
> 
> 
> Thanks,
> 
> Jeremy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20101029/c6cafde8/attachment.htm