Review Request: Initial group support

[David Edmundson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101380/#review3378
-----------------------------------------------------------



account-filter-model.cpp
<http://git.reviewboard.kde.org/r/101380/#comment2813>

    put with the other local includes



account-filter-model.cpp
<http://git.reviewboard.kde.org/r/101380/#comment2812>

    don't commit commented out debug



contact-delegate.h
<http://git.reviewboard.kde.org/r/101380/#comment2814>

    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.



contact-delegate.cpp
<http://git.reviewboard.kde.org/r/101380/#comment2811>

    I don't see why you need the
    
    if (m_usingGroups)
    here. 
    
    the isContact = index.data()...== qMetaType<ContactModelItem*> works here in either case.



groups-model-item.cpp
<http://git.reviewboard.kde.org/r/101380/#comment2809>

    You might want to return true here.



groups-model-item.cpp
<http://git.reviewboard.kde.org/r/101380/#comment2810>

    I totally agree with my own comment here.
    



groups-model.cpp
<http://git.reviewboard.kde.org/r/101380/#comment2808>

    beginRemoveRows code should NOT be here.
    
    removing the proxyNode will emit "itemsRemoved" and it will all hit your "onItemsRemovedCode" which does it.


- David


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