> 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.
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 ;) )
- 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
>
>
_______________________________________________
KDE-Telepathy mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-telepathy
