Review Request: Themed task item backgrounds

[Aaron Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://mattr.info/r/262/#review260
-----------------------------------------------------------


no real feelings one way or the other on this one, but see comments below.


/trunk/KDE/kdebase/workspace/plasma/applets/tasks/abstracttaskitem.h
<http://mattr.info/r/262/#comment192>

    this should be m_tasksThemePath, and i actually don't think it's even necessary. see comments above.



/trunk/KDE/kdebase/workspace/plasma/applets/tasks/abstracttaskitem.h
<http://mattr.info/r/262/#comment193>

    m_taskItemBackground;



/trunk/KDE/kdebase/workspace/plasma/applets/tasks/abstracttaskitem.cpp
<http://mattr.info/r/262/#comment191>

    this needs to be documented on http://techbase.kde.org/Projects/Plasma/Theme



/trunk/KDE/kdebase/workspace/plasma/applets/tasks/abstracttaskitem.cpp
<http://mattr.info/r/262/#comment194>

    if we don't find our file, taskItemBackground (or, m_taskItemBackground) is never initialized and we have a non-0'd pointer.
    
    i think it would be safer to initialize m_taskItemBackground to 0 in the initialization list above, then instead of checking for taskThemePath.isEmpty() you can just check for if (m_taskItemBackground).
    
    the theme path itself is also not needed after the constructor here as the Svg will update when the theme does, so it's just taking up memory.



/trunk/KDE/kdebase/workspace/plasma/applets/tasks/abstracttaskitem.cpp
<http://mattr.info/r/262/#comment195>

    this is a bit dangerous; attentionPainter.end() is never called, so while this probably works as is i don't believe there is any actual guarantee that attention is actually in a valid, finished state before QPainter::end() is called.
    
    it's usually safer to write such code as:
    
    {
        QPainter attentionPainter(&attention);
        taskItemBackground->paint(&attentionPainter, option->rect, "attention");
    }
    
    so that attentionPainter falls out of scope and end() is implicitly called.


- Aaron


On 2008-03-07 02:31:34, Andrew Lake wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://mattr.info/r/262/
> -----------------------------------------------------------
> 
> (Updated 2008-03-07 02:31:34)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch allows task item backgrounds to be painted using a theme svg file.  The svg file used widgets/tasks.svg and should contain the following elements: focus, hover, attention and normal.  If the svg file is absent then the backgrounds are painted as they do currently.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/applets/tasks/abstracttaskitem.cpp
>   /trunk/KDE/kdebase/workspace/plasma/applets/tasks/abstracttaskitem.h
> 
> Diff: http://mattr.info/r/262/diff
> 
> 
> Testing
> -------
> 
> Tested with simple proof-of-concept tasks.svg available here: http://www.mediafire.com/?1k1zjk3nu3d
> 
> No problems that I noticed.
> 
> 
> Thanks,
> 
> Andrew
> 
>