> On March 29, 2011, 5:06 p.m., Dario Freddi wrote: > > Follows a review. Protip: smaller diffs next time, I surely missed > > something :D > > > > Btw, I see this removes all the bit of Nepomuk integration me and George > > did time ago: I don't know if you guys already discussed about that, but > > I'd like at least to have George chiming in and have a look. > > > > Beware of QVariant misuse: besides being wrong, it can be dangerous! > > David Edmundson wrote: > Just to help clarify things: > > We have discussed the nepomuk side, George G was involved in the > discussion. It's part of a large plan, hence the absolutely massive diff. As > it's such a large set of changes I really want to get this merged as this > diff keeps getting larger and larger. > > The QVariant mis-use and the coding style switch comes from a sort of > upstream (Tp-Yell) for some files which we currently have a local copy of. > We've already had to do several bug fixes to it, so I guess we can add this > one too. > > > > > Martin Klapetek wrote: > Ok, I'll check for the QVariant misuses and then I will add the relevant > parts to our almost-upstream-repo too, so the upstream can pick it up. > > Also I see you catched a lot of upstream issues, I commented just > "Upstream" on them. I think this should be solved upstream first (I'll > promote the changes there). I leave the inclusion of those files to the > meeting.
Ok, then it's mostly upstream which should be bitch-slapped this time ;D Besides that, nice job on the patch :) Once you fix the remaining issues, it's a "Ship it" from me. > 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? 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. > On March 29, 2011, 5:06 p.m., Dario Freddi wrote: > > account-button.cpp, line 99 > > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12726#file12726line99> > > > > For clarity, try using QVariant::value() instead of qVariantFromValue, > > which was meant as a workaround for old compilers. Your code would become: > > > > a->data().value<Tp::Presence>().status() > > > > Check all the occurrences throughout the code > > Martin Klapetek wrote: > Wel...don't we want to support older compilers too? ;) It was mostly for > some MS compilers iirc, but sure, I can change that. Yeah, it was meant to be used to support MSVC 6 :D However, if you feel like not changing it don't worry, it's just a matter of making the whole code more readable > On March 29, 2011, 5:06 p.m., Dario Freddi wrote: > > accounts-model.h, lines 122-124 > > <http://git.reviewboard.kde.org/r/100838/diff/4/?file=12731#file12731line122> > > > > You don't really need a p-impl class here as you are not exporting the > > model, but if you want to keep it that way, declare it as: > > > > class Private; > > Private * const d; > > > > To keep this consistent with KDE's style. > > Martin Klapetek wrote: > This is again upstream file. It has some stripped header directives so it > can be used in our project without problems. I'd leave this for now, we'll > talk more about this on the meeting. Sure. Anyway, if this code comes from other project, we should attach a "This file is from blabla project" notice in the copyrights - but no hurry and not a blocker indeed, we'll talk about it in the meeting. - 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
