> 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. > > > > Martin Klapetek wrote: > 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. > > David Edmundson wrote: > I seem to recall it meant we could remove a method in the GroupsModel if > we made this change, but now I'm not sure. Update the code, or remove the > comment. Don't care which.
No, it won't :) It's called only in GroupsModel::addContactToGroups() and since there are signals connected in the model to the proxy node, I'll leave the proxy and delete the comment. And also I renamed the method to GroupsModelItem::addProxyContact() so it's more clear what is actually added. > 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. > > Martin Klapetek wrote: > Actually it won't. The signal is never emitted and the node is removed > directly. Could use some rethinking though. > > David Edmundson wrote: > yes it will, the next line is proxyNode->remove(); > > proxyNode->remove will call TreeNode::remove which will call deleteLater() > > this will call proxyNode::onRemoved, which will emit itemRemoved, this > propogates down the tree and hits groupsModel::onItemsRemoved. Now that you wrote that, it should indeed work like that, but GroupsModel::onItemsRemoved was never called when I tested this. Let me check again. - 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
