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

[Dario Freddi]

> On 2010-03-28 13:04:33, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-handler.h, line 35
> > <http://reviewboard.kde.org/r/3415/diff/1/?file=21740#file21740line35>
> >
> >     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 :/)

Agreed, will add in the next revision.


> On 2010-03-28 13:04:33, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-handler.h, line 46
> > <http://reviewboard.kde.org/r/3415/diff/1/?file=21740#file21740line46>
> >
> >     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?

You assumed correctly.

Ok for passing a single contact and adding overloads, I fully agree.

I am just a bit concerned about passing just a PersonContact or a PimoPerson - even if the idea is nice, is it possible to retrieve the source account resource (to be clear, what is in the current implementation accountURI) from a PersonContact? If this is possible, I definitely agree as well.


- Dario


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


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