[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: Add fade effect to wallpaper plugin.
From:       Matthew Dawson <matthewjd () gmail ! com>
Date:       2009-03-03 6:59:43
Message-ID: 200903030159.54909.matthewjd () gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


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


["signature.asc" (application/pgp-signature)]

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel


[prev in list] [next in list] [prev in thread] [next in thread]