Review Request: Merge request for telepathy-kde-contactlist

Martin Klapetek martin.klapetek at gmail.com
Fri Apr 1 14:31:57 CEST 2011 


> On March 29, 2011, 5:06 p.m., Dario Freddi wrote:
> > account-button.cpp, lines 102-104
> > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12726#file12726line102>
> >
> >     It is not very clear to me what you are doing here, but this routine can be extremely slow. Can you please tell me what you are trying to achieve?
> >     
> >     I also see there is more than one occurrence of that, so this should indeed optimized if possible.
> 
> Martin Klapetek wrote:
>     This paints a small overlay over the account icon. It paints the presence icon (online, offline etc) and I think this is a bit of the original code. What would be a better way to achieve painting a small emblem over an icon?
> 
> Dario Freddi wrote:
>     There is a custom KIcon constructor (please see http://api.kde.org/4.x-api/kdelibs-apidocs/kdeui/html/classKIcon.html#a6057e88b03b32ca70af2da295d626408 ) which does the dirty job itself when it comes to overlays - it is surely not much more optimized than this, but when QIcon will support overlays natively, we'll get the speed bump for free. I advise you to switch to this method, it will also give more clarity to the code.
> 
> Dario Freddi wrote:
>     However, I also see that you are trying to create a big overlay, so the method I suggested might not work (try it anyway though). A possible way to optimize this routine then can be storing the pixmap (or even the painter) somewhere to prevent at least one painting cycle to be performed each time the overlay is painted - this way the same painter will be used to paint all the icons, which is not really the most lenghty part, but it's still something.
> 
> Dario Freddi wrote:
>     Last time I am spamming you: thinking about it twice, that would do.
>     
>     class ... {
>     ...
>     private:
>         QPixmap m_errorPixmap;
>     }
>     
>     //
>     
>     Constructor::Constructor(Bla *bla) {
>     
>         // Load icons and stuff...
>         m_errorPixmap = KIconLoader::global->loadIcon("dialog-error", KIconLoader::NoGroup, 16);
>     
>         // More stuff
>     }
>     
>     Class::yourMethod() {
>         // Your routine, which becomes:
>         QPixmap pixmap = icon().pixmap(32, 32);
>         QPainter painter(&pixmap);
>         painter->drawPixmap(15, 15, 16, 16);
>     }
>     
>     Unfortunately we can cache only the overlayed pixmap - the other one will need to be loaded and drawn every time. If the routine is used VERY frequently, I advise you to load the pixmap through KIconLoader instead of getting it straight from the icon and see how it goes - IIRC, KIconLoader performs slightly better than straight KIcon conversion when talking about pixmap, but this is really dependent on how many times you are calling such a routine and how it is critical for it to be fast. Surely such a modification should give you a good speed bump: now you are loading one pixmap less per cycle, which is quite an expensive operation. Ideally, you should never load pixmaps in the drawing phase, but it looks like you can't avoid loading at least one in this case.
> 
> Dario Freddi wrote:
>     I obviously fail at copying&pasting: of course I meant
>     
>     painter->drawPixmap(15, 15, m_errorPixmap);
> 
> Martin Klapetek wrote:
>     Thanks for the tips :) Just a small clarification - the overlay changes everytime the presence changes. Also, because of that, there are several overlay pixmaps (one for each presence type + the error one), so we need to cache more pixmaps (no problem really). Next thing is the speed - it's not that critical to be fast. Imagine the connection procedure - it is connecting and once it is connected, the overlay is painted. But sure, faster (in terms of less calls) is better. I'll try to switch it to the KIconLoader. And the last - we could also cache the account icon pixmap, because that one does not change, and then use this instead of icon().pixmap() calls.
> 
> Dario Freddi wrote:
>     No problem for using multiple caches - it is definitely the right way to go :)
>     
>     About the pixmap, I'm afraid we can't. We need a new painting device every time we draw an icon, and the pixmap itself is our painting device. AFAIR, the pixmap is not deep-copied when creating an icon - instead, the stored pixmap becomes the icon itself, which means that we cannot reuse it for painting - we can cache the other one as the painting operation draws a "source" pixmap onto a target, but we need a brand new target everytime.
>     
>     One thing you could to to speed things up, though, is caching the pixmap and then perform a deep copy everytime you need to paint a new overlay, which is slightly faster than loading the pixmap from a file or an icon. So that would become:
>     
>     Class ... {
>     ...
>     private:
>         QPixmap m_errorPixmap;
>         QPixmap m_accountPixmap;
>     }
>     
>     //
>     
>     Constructor::Constructor(Bla *bla) {
>     
>         // Load icons and stuff...
>         m_errorPixmap = KIconLoader::global->loadIcon("dialog-error", KIconLoader::NoGroup, 16);
>         m_accountPixmap = icon().pixmap(32);
>     
>         // More stuff
>     }
>     
>     Class::yourMethod() {
>         // Let's create a new painting device which we will use as our new icon, and paint an overlay
>         QPixmap pixmap = m_accountPixmap.copy();
>         QPainter painter(&pixmap);
>         painter.drawPixmap(15, 15, 16, 16);
>     }
>     
>     This is indeed a better compromise, and less expensive than before, and I think this is also as far as we can go :)
>     
>     About how it is critical, it is surely not at the moment, but it will become so when you'll have to draw such a thing for contacts - in that case, you'll have to draw hundreds of overlayed icons in the blink of an eye! And getting it right will help the next guy who'll copy&paste your code (yeah, you know it will happen ;) )

Now when I'm finally looking at the code, I see one disadvantage of this cached approach. It's ok for the dialog-error icon, but the rest is actually quite ellegant as it is:

QPixmap pixmap = icon().pixmap(32, 32);
QPainter painter(&pixmap);
KIcon(a->icon()).paint(&painter, 15, 15, 16, 16);

setIcon(KIcon(pixmap));

When the presence icon overlay is painted, it takes the action icon and paints it over the account icon. Since the action icon is already loaded and displayed, isn't it cached itself (somehow)? Anyway if I add the pixmap caches for the presence icons, the code would get pretty messy with bunch of ifs/switch, this way (the code up) is much more cleaner to me.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100838/#review2218
-----------------------------------------------------------


On March 23, 2011, 9:32 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100838/
> -----------------------------------------------------------
> 
> (Updated March 23, 2011, 9:32 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This is the work that begun in my clone repo and has matured enough now to be merged back to 'upstream', where the work should continue now.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 2c62ea10556cdba38d1bca0fe63603df97414336 
>   account-button.h PRE-CREATION 
>   account-button.cpp PRE-CREATION 
>   account-filter-model.h PRE-CREATION 
>   account-filter-model.cpp PRE-CREATION 
>   accounts-model-item.h PRE-CREATION 
>   accounts-model-item.cpp PRE-CREATION 
>   accounts-model.h PRE-CREATION 
>   accounts-model.cpp PRE-CREATION 
>   contact-delegate-overlay.h PRE-CREATION 
>   contact-delegate-overlay.cpp PRE-CREATION 
>   contact-delegate.h PRE-CREATION 
>   contact-delegate.cpp PRE-CREATION 
>   contact-model-item.h PRE-CREATION 
>   contact-model-item.cpp PRE-CREATION 
>   contact-overlays.h PRE-CREATION 
>   contact-overlays.cpp PRE-CREATION 
>   contact-view-hover-button.h PRE-CREATION 
>   contact-view-hover-button.cpp PRE-CREATION 
>   filter-bar.h PRE-CREATION 
>   filter-bar.cpp PRE-CREATION 
>   main-widget.h aed6f7c8336321bc8a6ffb3b9af6b1d493f85790 
>   main-widget.cpp 6146b62eec17b54be63b594200613931673ac5fe 
>   main-widget.ui 5b0d5aaaf3a4e2f49eb030b98b15fcae3a86170c 
>   main.cpp bba0e4175e8853afe603f26ea4707f4974192d0f 
>   ontologies/CMakeLists.txt 3e0bbe8c634908f689dcd360409aeddcc6fc0d23 
>   tree-node.h PRE-CREATION 
>   tree-node.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100838/diff
> 
> 
> Testing
> -------
> 
> We did a lot of thourough testing on #kde-telepathy, also some people emailed their reports which all has been fixed.
> 
> 
> Thanks,
> 
> Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110401/70c354d5/attachment-0001.htm

More information about the KDE-Telepathy mailing list