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