Review Request: Themed task item backgrounds
Aaron Seigo
aseigo at kde.org
Fri Mar 7 21:32:16 CET 2008
-----------------------------------------------------------
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
>
>
More information about the Panel-devel
mailing list