Review Request: Initial group support

[Martin Klapetek]

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110518/a4945ec4/attachment-0001.htm