Review Request: No longer use a QGL for managing offscreen widgets.

[Aaron Seigo]

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


i think the idea is a good one; and even on an arm system (float vs double) we still get over 16000 offscreen widgets this way, and if we wanted more we could put them in a two dimensional grid instead of a lineary array on the canvas.

the implementation could use some tweaks perhaps though:


/trunk/KDE/kdelibs/plasma/corona.cpp
<http://reviewboard.vidsolbach.de/r/296/#comment235>

    there's no need to keep these sorted, so you could just use a QHash here..



/trunk/KDE/kdelibs/plasma/corona.cpp
<http://reviewboard.vidsolbach.de/r/296/#comment236>

    contains? would prevent creating a default value, and is a bit clearer what's going on there.



/trunk/KDE/kdelibs/plasma/corona.cpp
<http://reviewboard.vidsolbach.de/r/296/#comment237>

    values() iterates over the list, makes a new list collection containing *all* the values, and then contains on that does a linear search over that list. oi, vey!
    
    much better:
    
    QMutableHashIterator<uint, QGraphicsWidget *> it(d->offscreenWidgets);
    
    while (it.hasNext()) {
         if (it.next() == widget) {
              it.remove();
              return;
         }
    }


- Aaron


On 2008-12-09 11:03:40, Rob Scheepmaker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.vidsolbach.de/r/296/
> -----------------------------------------------------------
> 
> (Updated 2008-12-09 11:03:40)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Currently, offscreen widgets are managed by a QGraphicsGridLayout in the topleft quadrant of the scene. This brings some issues though:
> * As we know, QGL's have some known issues. Working with them can sometimes be a little fragile, and there are workarounds in the code (in popupapplet for example) to work around problems with them.
> * Each time a widget changes size, other offscreen widgets inevitably move, forcing all top level views to reposition and redraw themselves. When this happens you can notice a slight lag.
> * I've seen screenshots of a problem where certain widgets partly overlap in a toplevel view. This might have been a temporary bug, or something that happens rarely, but I think it demonstrates the fragile nature of the current approach.
> 
> Out of curiosity, I've tried experimenting with a different approach. With this patch, corona will position each offscreen widget in their own reserved sector with a size of QMAXWIDGETSIZE. This way, each widget can grow and shrink without influencing others, and it will never require us to work around layout bugs. After some changes to extenderitem (since it assumed it would always be resized after moving offscreen, which was the case before), this works quite good actually. It feels snappier while dragging extender items around, and the code looks simpler and less hacky.
> 
> This is a change with more impact then your avarage bugfix though, so I would like to now what you think of this new offscreen widget implementation.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/corona.cpp
>   /trunk/KDE/kdelibs/plasma/extender.cpp
>   /trunk/KDE/kdelibs/plasma/extenderitem.cpp
>   /trunk/KDE/kdelibs/plasma/popupapplet.cpp
>   /trunk/KDE/kdelibs/plasma/private/extenderitem_p.h
> 
> Diff: http://reviewboard.vidsolbach.de/r/296/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Rob
> 
>