----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118237/#review63643 -----------------------------------------------------------
Good stuff! Couple mostly minor issues below, but overall seems good. I'd like David to give a look to the merging model as he wrote one too abstract-contact-delegate.cpp <https://git.reviewboard.kde.org/r/118237/#comment44327> Seems unused? abstract-contact-delegate.cpp <https://git.reviewboard.kde.org/r/118237/#comment44334> Could we possibly get these two rowtypes into KTp:: namespace too? contact-delegate-compact.h <https://git.reviewboard.kde.org/r/118237/#comment44331> Pointer signs to the right contact-delegate-compact.cpp <https://git.reviewboard.kde.org/r/118237/#comment44332> The icons.count() is constant; int i = 0; i < icons.count(); ++i would be enough? contact-delegate.h <https://git.reviewboard.kde.org/r/118237/#comment44328> Coding style - pointer signs to the right contact-delegate.cpp <https://git.reviewboard.kde.org/r/118237/#comment44329> All these option->optV4 changes would have been easier if you've named the function argument "option" but oh well xD contact-delegate.cpp <https://git.reviewboard.kde.org/r/118237/#comment44330> icons.count() is again not changing inside the loop, the "c" has not much use contact-list-widget.cpp <https://git.reviewboard.kde.org/r/118237/#comment44333> Any reason for this change? We do this to have a space for the ">" metacontact expansion button contacts-rooms-model.h <https://git.reviewboard.kde.org/r/118237/#comment44335> Coding style const QModelIndex& parent -> const QModelIndex &parent (...and everywhere in the .cpp file too) contacts-rooms-model.cpp <https://git.reviewboard.kde.org/r/118237/#comment44337> Should this maybe also forward to the RoomsModel? rooms-model.h <https://git.reviewboard.kde.org/r/118237/#comment44338> This could also end up in types.h rooms-model.h <https://git.reviewboard.kde.org/r/118237/#comment44339> & to the right (.cpp too) rooms-model.cpp <https://git.reviewboard.kde.org/r/118237/#comment44340> kDebug (or remove, seems not too useful) - Martin Klapetek On May 21, 2014, 10:13 p.m., Dan Vrátil wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/118237/ > ----------------------------------------------------------- > > (Updated May 21, 2014, 10:13 p.m.) > > > Review request for Telepathy. > > > Repository: ktp-contact-list > > > Description > ------- > > The long-awaited Dan-sized patch is here :-) > > This patch merges the KTp::ContactsModel and RoomsModel (simple channel > observer) into a single tree model via ContactsRoomsModel. The model now > looks like > > | > |- Chat rooms > | |- [email protected] > | |- [email protected] > |- Contacts > |- Group 1 > | |- Contact 1 > | |- Contact 2 > |- Group 2 > > After a few beers, I was even able to make my model work with David's > EmptyRowFilter, so the "Chat rooms" (or "Contacts") top-level nodes will be > automatically hidden, when they have no children > > The patch is so big, because I refactored the delegates a little, but other > than that, it's just the two new models and small changes in context menu and > contact overlays to support rooms. > > > Unfortunately this patch causes ktp-contact-list to crash when switching from > "Do not show groups" to "Show contacts by groups". Interestingly, it does not > crash when switching the other way around. I'm completely clueless, so if you > guys can figure something out, it would me much appreciated. This patch > cannot go in until the crash is fixed of course. > > > Diffs > ----- > > CMakeLists.txt 1ee112a > abstract-contact-delegate.h 2339833 > abstract-contact-delegate.cpp a92cc4e > contact-delegate-compact.h 736d2e5 > contact-delegate-compact.cpp c51a210 > contact-delegate.h 9f0e5df > contact-delegate.cpp 537e204 > contact-list-widget.h 8f39b81 > contact-list-widget.cpp 3ffa1cc > contact-list-widget_p.h e4a5eb3 > contact-overlays.h a998644 > contact-overlays.cpp ee14e8f > contacts-rooms-model.h PRE-CREATION > contacts-rooms-model.cpp PRE-CREATION > context-menu.h bec4e55 > context-menu.cpp 00795d7 > empty-row-filter.cpp 01b2b48 > main-widget.cpp a07b534 > rooms-model.h PRE-CREATION > rooms-model.cpp PRE-CREATION > tooltips/tooltipmanager.cpp 555d425 > > Diff: https://git.reviewboard.kde.org/r/118237/diff/ > > > Testing > ------- > > Joined a room, room immediately appeared in the CL. Closed text-ui, room > remained in CL. Clicked the room in CL, text-ui was started again. Left the > room, it disappeared from CL. > > > File Attachments > ---------------- > > Screenshot > > https://git.reviewboard.kde.org/media/uploaded/files/2014/05/21/09b64e1d-7c31-4fc1-a3ff-08996fcd930d__ktp-cl.png > Screenshot with contact groups enabled > > https://git.reviewboard.kde.org/media/uploaded/files/2014/05/21/b6bf32cb-3e65-47e3-af15-5859e2dbdda5__ktp-cl.png > > > Thanks, > > Dan Vrátil > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
