Review Request: Initial group support

[Martin Klapetek]

> 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.
> 
> Martin Klapetek wrote:
>     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.

Ok, so no, it's not like that. proxyNode::onRemoved is called only on sourceNode (ContactModelItem) being destroyed, which does not happen in this case, because the source contact is not really touched, therefore the itemRemoved will never be emitted. So instead of calling remove() directly, it should call the onRemoved() slot to get the signal emitted as expected.

So I propose to rename the onRemoved() slot to something better and then to call it directly instead of the code above. Is that ok?


- 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/bbbde527/attachment.htm