----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100838/#review2218 -----------------------------------------------------------
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! account-button.cpp <http://git.reviewboard.kde.org/r/100838/#comment1850> Use KAction instead of QAction here account-button.cpp <http://git.reviewboard.kde.org/r/100838/#comment1851> 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 account-button.cpp <http://git.reviewboard.kde.org/r/100838/#comment1852> 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. account-button.cpp <http://git.reviewboard.kde.org/r/100838/#comment1853> Const'ify: const Tp::Presence &presence account-button.cpp <http://git.reviewboard.kde.org/r/100838/#comment1854> Use const Tp::Presence& here account-filter-model.h <http://git.reviewboard.kde.org/r/100838/#comment1855> Watch out for correct copyrights, unless you copied this code from somewhere else. accounts-model-item.cpp <http://git.reviewboard.kde.org/r/100838/#comment1858> Urgh - do not do that. Declare AccountsModelItem as a metatype const_cast to AccountsModelItem and do QVariant::fromValue() from there. Even though I'd try to avoid that const_cast if possible, but it seems rather unfeasible. accounts-model.h <http://git.reviewboard.kde.org/r/100838/#comment1856> 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. accounts-model.cpp <http://git.reviewboard.kde.org/r/100838/#comment1857> I don't really see the point of that, can you please explain? contact-delegate.h <http://git.reviewboard.kde.org/r/100838/#comment1859> Copyright missing contact-delegate.cpp <http://git.reviewboard.kde.org/r/100838/#comment1860> Copyright missing P.S.: Hey, this looks like some of my old code btw :D contact-model-item.h <http://git.reviewboard.kde.org/r/100838/#comment1861> Apparently, those do not need to be virtual contact-model-item.cpp <http://git.reviewboard.kde.org/r/100838/#comment1862> Same as before main-widget.cpp <http://git.reviewboard.kde.org/r/100838/#comment1863> KMenu instead of QMenu - Dario 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
