Review Request: Prevent excessive painting of non-ARGB fdo system tray icons

[Jason Stubbs]

> On 2008-11-08 12:07:21, Aaron Seigo wrote:
> > a much easier, straightforward and foolproof way of doing this is to have a QTime and a QTimer, and at the start of the paintEvent method have sth like:
> > 
> > if (time.elapsed() < MIN_REFRESH_TIME) {
> >      timer->start(MIN_REFRESH_TIME - time.elapsed() + 10);
> >      return;
> > }
> > 
> > time.restart();
> > 
> > timer would be connected to a method that calls update. the rest of the paint event would remain the same, and this should compress paints to be no more than once every 1000/MIN_REFRESH_TIME (so 1/10th of a second if the min time is 100, 1/4th of a second at 250, etc..).
> > 
> > with your patch it will do 10 paints in rapid succession and just stall for the remainder of the second. this would probably break overly-agressively animated icons.
> > 
> > the use of single shot timers in your patch also provides no way of resetting the time to wait.

The 10 paints in rapid succession was half intentional in order to make it obvious that there's a painting problem somewhere else. I have redone the patch in line what you've said above though and it does simplify it a fair bit.


> On 2008-11-08 12:07:21, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/applets/systemtray/protocols/fdo/x11embedcontainer.cpp, line 191
> > <http://reviewboard.vidsolbach.de/r/264/diff/1/?file=1466#file1466line191>
> >
> >     this causes a repaint in one second whether it needs it or not, as restorePaintTokens called update()

Only if all tokens were depleted...


> On 2008-11-08 12:07:21, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/applets/systemtray/protocols/fdo/x11embedcontainer.cpp, line 189
> > <http://reviewboard.vidsolbach.de/r/264/diff/1/?file=1466#file1466line189>
> >
> >     updateBackgroundImage will still get called 10 times.

Nope, only once. A guard was already in place so I didn't need to add one.


- Jason


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.vidsolbach.de/r/264/#review255
-----------------------------------------------------------


On 2008-11-08 19:40:24, Jason Stubbs wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.vidsolbach.de/r/264/
> -----------------------------------------------------------
> 
> (Updated 2008-11-08 19:40:24)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> There are some situations - such as overlapping sibling widgets - that can cause a painting loop that can make plasma severely unresponsive. While those issues should be fixed as they arise, I think that it is better that the system tray be a little more robust as well.
> 
> This patch limits paints to 10 per second. This number is arbitrary and essentially limits the number of frames per second for animations, but can't be too high as updates are fairly expensive.
> 
> Also, is this method of using tokens ok? Is there a better way?
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/workspace/plasma/applets/systemtray/protocols/fdo/x11embedcontainer.h
>   trunk/KDE/kdebase/workspace/plasma/applets/systemtray/protocols/fdo/x11embedcontainer.cpp
> 
> Diff: http://reviewboard.vidsolbach.de/r/264/diff
> 
> 
> Testing
> -------
> 
> Resizing the panel to around 10 pixels will trigger excessive repainting. With the patch, the paints being prevented is clearly visible and plasma remains responsive. Resizing it too small will cause the icons to disappear altogether and will cause things to become unresponsive, but kDebug()s revealed that this was not due to painting. 
> 
> 
> Thanks,
> 
> Jason
> 
>