Review Request: Add Telepathy capabilities to tp-contactlist, and enable removing/adding contacts from/to a group

[George Goldberg]

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


I've briefly reviewed the two new classes a bit (not fully, but these are the first comments that come to mind). I've ignored error handling aspects, since as you say, that is intentionally omitted at this stage.


/trunk/playground/network/telepathy-contactlist/telepathy-handler.h
<http://reviewboard.kde.org/r/3415/#comment4208>

    Please can you add API-docs to, at least, this class. That way, it is easier for people to understand exactly what the class is trying to acheive.
    
    Also, probably should use a different name for this class, since I think the name is a bit confusing with "Handlers" in the Telepathy spec, which have nothing to do with what this class does. (Admittedly, I can't think of a better name right now :/)



/trunk/playground/network/telepathy-contactlist/telepathy-handler.h
<http://reviewboard.kde.org/r/3415/#comment4206>

    I'm assuming this, and the two methods below, are the basic public API that people would generally use out of TelepathyHandler and TelepathyAccountProxy classes.
    
    Why not have the method parameters be even simpler: we just need:
    
    removeContactFromGroup(const QString &group, const Nepomuk::PersonContact contact)
    
    An overload with QList<Nepomuk::PersonContact> might be nice too, as well as overloads for changing an entire meta-contact by doing Nepomuk::PimoPerson and QList<Nepomuk::PimoPerson>.
    
    What do you reckon?


- George


On 2010-03-28 12:30:50, Dario Freddi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3415/
> -----------------------------------------------------------
> 
> (Updated 2010-03-28 12:30:50)
> 
> 
> Review request for telepathy and George Goldberg.
> 
> 
> Summary
> -------
> 
> This patch adds two new classes (already ready to be librarized) for creating a Nepomuk->Telepathy bridge, making it very easy to retrieve Telepathy objects out of Nepomuk resources. Performance overhead looks trascurable.
> 
> By using this new class, contactlist is now able to add/remove people from a group.
> 
> Error handling is still missing, but I wanted an heads up before I go on and implement all the rest.
> 
> Also, how are we going to handle metacontacts in this particular regard? My take is to move/remove all contacts to/out of a group, and eventually creating a group for those accounts who still don't have a group named as such in case of addition.
> 
> Beware: until t-i-d will be able to catch these changes and update nepomuk accordingly, this is probably going to screw up your nepomuk DB when moving contacts around groups.
> 
> 
> Diffs
> -----
> 
>   /trunk/playground/network/telepathy-contactlist/CMakeLists.txt 1107773 
>   /trunk/playground/network/telepathy-contactlist/contact-item.h 1107996 
>   /trunk/playground/network/telepathy-contactlist/contact-item.cpp 1107996 
>   /trunk/playground/network/telepathy-contactlist/main-widget.h 1107996 
>   /trunk/playground/network/telepathy-contactlist/main-widget.cpp 1107996 
>   /trunk/playground/network/telepathy-contactlist/main-widget.ui 1107667 
>   /trunk/playground/network/telepathy-contactlist/telepathy-account-proxy.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/telepathy-account-proxy.cpp PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/telepathy-handler.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/telepathy-handler.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/3415/diff
> 
> 
> Testing
> -------
> 
> Works, empathy recognizes the changes correctly.
> 
> 
> Thanks,
> 
> Dario
> 
>