Review Request: Make IconTask use Plasma::Theme'd "close" button

[Aaron J. Seigo]

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



applets/icontasks/tooltips/windowpreview.cpp
<http://git.reviewboard.kde.org/r/103092/#comment6931>

    should be a const QString.



applets/icontasks/tooltips/windowpreview.cpp
<http://git.reviewboard.kde.org/r/103092/#comment6932>

    personally, i really dislike these "if not A then foo else bar". i find "if A then bar else foo" to be a lot easier to read in most cases. (there are, of course, exceptions, such as when the not branch is really small and the other branch is huge in which case having "if not A" first can make it clearer what is going on. :)



applets/icontasks/tooltips/windowpreview.cpp
<http://git.reviewboard.kde.org/r/103092/#comment6933>

    m_svg is unecessary -> it isn't used anywhere .. just make it a local variable. as it is, this is somewhat dangerous as m_svg is not initialized anywhere so we have a non-zero but invalid pointer in many cases.



applets/icontasks/tooltips/windowpreview.cpp
<http://git.reviewboard.kde.org/r/103092/#comment6934>

    as can be seen in the screenshots, the close button looks oddly large. i imagine because the svg is not being resized properly to ToolTipContent::iconSize. the result is that even though it is themed, it actually looks worse...
    
    also, this line is indendent incorrectly.


- Aaron J. Seigo


On Nov. 9, 2011, 10:22 a.m., Diego Casella wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103092/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2011, 10:22 a.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo and Craig Drummond.
> 
> 
> Description
> -------
> 
> Simple patch which makes IconTask look more consistent with Plasma Theme.
> It renders the "close" button using Plasma::Theme, and fallback to the standard window-close icon if nothing better is available :)
> 
> 
> Diffs
> -----
> 
>   applets/icontasks/tooltips/windowpreview.cpp 3e0c865 
> 
> Diff: http://git.reviewboard.kde.org/r/103092/diff/diff
> 
> 
> Testing
> -------
> 
> Compiles and works as expected (see pictures below).
> 
> 
> Screenshots
> -----------
> 
> ToolTip with themed "close" button
>   http://git.reviewboard.kde.org/r/103092/s/329/
> ToolTip with default "close" icon
>   http://git.reviewboard.kde.org/r/103092/s/330/
> 
> 
> Thanks,
> 
> Diego Casella
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20111109/2be83806/attachment.html>