Review Request: Add fade effect to wallpaper plugin.

[Matthew Dawson]

Hey,

On Monday 02 March 2009 15:06:35 Aaron J. Seigo wrote:
<snip
> 
> comments on patch (besides "thanks! always great to receive patches from new 
> faces"):
Thanks for the comments!
> 
> - coding style. {s for methods start on their own line (see updateFadedImage) 
> and there is a space between the keyword and the opening ( in a conditional: 
> "If (" not "if(" and ") {" not "){". 
Fixed!
> 
> - it should be using Plasma::Animator for its timer and a CustomTimer so this 
> can be turned off by policy and share the global timer for this. using your 
> own QTimeLine is, in general, a no-go.
I just looked at Plasma::Animator.  Would I use the Plasma::Animator::CustomAnimation function to replace the QTimeLine?  Also I looked through both the documentation and the sources, and I can't find a mention of CustomTimer.  I'm just wondering what it is.
> 
> - it seems that there should be a more performant way of doing this than 
> creating a third pixmap that is the size of the end result, painting into it 
> and then painting another into it! i'd suggest trying something like just 
> painting both images into the exposed rect as passed into the paint method 
> using the current alpha. if that produces acceptable results, i'd say go for 
> it.
After playing around with a simple demo with two colours, it appears that the 3 pixmap method works best.  I pulled most of the code from http://techbase.kde.org/Development/Tutorials/Graphics/Performance#QPixmap::setAlphaChannel.28.29 .  While 3 pixmaps does seem like overkill, it seems to be the only way that works.  I could probably get away with two pixmaps and QPainter.setOpacity, but as the above link says that is a performance killer.  I tested different combinations of methods.  If you have any other suggestion, I'm happy to try them!
> 
> * remember to free the other pixmap when the animation is done so it isn't 
> sitting around in memory. assigning a QPixmap() to it should be enough.
Will do.
> 
> regardless of how its done (other than "in hardware"), doing full screen 
> repaints isn't going to be precisely brilliant for performance, but as an 
> option it's pretty nice :) which reminds me to check the default caching mode 
> for Plasma::Containment with regards to performance (both memory usage as well 
> as speed)
> 
During testing, it appears (at least for QTimeLine) that it skips frames if it takes to long to repaint.  So on slower machines it will work, it will just be choppy.

Matthew


-- 
/*
 * Buddy system. Hairy. You really aren't expected to understand this
 *
 */
		-- From /usr/src/linux/mm/page_alloc.cA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/plasma-devel/attachments/20090303/d8a384c0/attachment.sig