Review Request: Merge request for telepathy-kde-contactlist

[Dario Freddi]

> 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.

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


-----------------------------------------------------------
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/20110330/6b15f75d/attachment-0001.htm