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.

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

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