> On May 18, 2011, 11:34 a.m., David Edmundson wrote: > > groups-model.cpp, line 277 > > <http://git.reviewboard.kde.org/r/101380/diff/3/?file=16628#file16628line277> > > > > beginRemoveRows code should NOT be here. > > > > removing the proxyNode will emit "itemsRemoved" and it will all hit > > your "onItemsRemovedCode" which does it.
Actually it won't. The signal is never emitted and the node is removed directly. Could use some rethinking though. > On May 18, 2011, 11:34 a.m., David Edmundson wrote: > > groups-model-item.cpp, line 97 > > <http://git.reviewboard.kde.org/r/101380/diff/3/?file=16626#file16626line97> > > > > I totally agree with my own comment here. > > Well there's not much a difference if the constructor is called in groups-mode-item or groups-model before calling this method, so ok, I'll change it. > On May 18, 2011, 11:34 a.m., David Edmundson wrote: > > contact-delegate.cpp, line 75 > > <http://git.reviewboard.kde.org/r/101380/diff/3/?file=16624#file16624line75> > > > > I don't see why you need the > > > > if (m_usingGroups) > > here. > > > > the isContact = index.data()...== qMetaType<ContactModelItem*> works > > here in either case. Indeed it does. Hm..I wonder if we shouldn't return ProxyTreeNode rather than ContactModelItem on AccountsModel::ItemRole. > On May 18, 2011, 11:34 a.m., David Edmundson wrote: > > contact-delegate.h, line 45 > > <http://git.reviewboard.kde.org/r/101380/diff/3/?file=16623#file16623line45> > > > > I don't really see why we need this. Ideally we should avoid having > > code that can go out of sync. > > > > For each item we're asked to paint we can see "is it a contact, a group > > header, or an account header". We can then do the appropriate action. > > > > Maybe we should change "isContact" in the main body of the code, to an > > enum. True, I'll look into that. - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101380/#review3378 ----------------------------------------------------------- On May 18, 2011, 8:43 a.m., Martin Klapetek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/101380/ > ----------------------------------------------------------- > > (Updated May 18, 2011, 8:43 a.m.) > > > Review request for Telepathy. > > > Summary > ------- > > This patch adds cross-account groups to contact list. > > > Diffs > ----- > > CMakeLists.txt 919df77 > account-filter-model.h 6591355 > account-filter-model.cpp 75f2126 > contact-delegate.h 46fea76 > contact-delegate.cpp 209e715 > groups-model-item.h PRE-CREATION > groups-model-item.cpp PRE-CREATION > groups-model.h PRE-CREATION > groups-model.cpp PRE-CREATION > main-widget.h 7a5e417 > main-widget.cpp 20e8003 > proxy-tree-node.h PRE-CREATION > proxy-tree-node.cpp PRE-CREATION > telepathy-kde-contactlist.notifyrc 918736e > tree-node.h adeefa4 > tree-node.cpp f892d5a > > Diff: http://git.reviewboard.kde.org/r/101380/diff > > > Testing > ------- > > Tested with several accounts with groups, also tried moving contacts between > groups from another client. > > > Thanks, > > Martin > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
