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

[Dario Freddi]

> On 2010-04-13 11:08:02, George Goldberg wrote:
> > I went over about half of this in great detail, and it looks basically all fine. However, I really don't have enough time to review code at the rate you are currently churning it out with this kind of thoroughness, so I've reviewed the rest of this fairly superficially. It looks fine, apart from the points mentioned below (all fairly minor). I propose that you go ahead and commit this once these have all been addressed, and then we leave proper review of the library classes (particularly the API) until nearer when we make a release (ie. after June, when I will have more time).

Ok - that's cool. I'll address your points and throw it in then.


> On 2010-04-13 11:08:02, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/add-contact-job.cpp, line 42
> > <http://reviewboard.kde.org/r/3415/diff/6/?file=22180#file22180line42>
> >
> >     Just out of my own curiosity, are the names "q_ptr" and "d_ptr" mandatory, or would "q" and "d" work too? (Just asking because I'm not familiar with the "Q_DECLARE_PRIVATE" and related macros.

Yes - they are a strict requirement. Q_D and Q_Q access (and do some magic on) d_ptr and q_ptr. 'q' and 'd' are generated by Q_D and Q_Q themselves, in fact.


> On 2010-04-13 11:08:02, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-bridge.h, line 232
> > <http://reviewboard.kde.org/r/3415/diff/6/?file=22199#file22199line232>
> >
> >     I would remove this line from the API docs - it's basically redundant with the "\see foo" line below. (same applies everywhere else this occurrs).

Agreed


> On 2010-04-13 11:08:02, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-bridge.h, line 86
> > <http://reviewboard.kde.org/r/3415/diff/6/?file=22199#file22199line86>
> >
> >     What about an AccountNotOnlineError? I'm assuming we shouldn't bring an account online to delete a contact (if the user tries to do a delete while that account is offline somehow). If the UI wants to enable the deletion option when the account is offline, then it should probably deal with the "AccountOfflineError" and give the user to bring the account online and try again.

Good idea - I'll add it.


> On 2010-04-13 11:08:02, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-bridge.h, line 345
> > <http://reviewboard.kde.org/r/3415/diff/6/?file=22199#file22199line345>
> >
> >     s/could/can/

ops


> On 2010-04-13 11:08:02, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-bridge.h, line 377
> > <http://reviewboard.kde.org/r/3415/diff/6/?file=22199#file22199line377>
> >
> >     s/explicitely/explicitly/g

ops^2


> On 2010-04-13 11:08:02, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-bridge.h, line 267
> > <http://reviewboard.kde.org/r/3415/diff/6/?file=22199#file22199line267>
> >
> >     You've got Nepomuk::Person here, when it should be a Nepomuk::PersonContact.

yup


> On 2010-04-13 11:08:02, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-bridge.h, line 278
> > <http://reviewboard.kde.org/r/3415/diff/6/?file=22199#file22199line278>
> >
> >     And here, it's the other way round :)
> >     
> >     Should be Nepomuk::Person instead of Nepomuk::PersonContact.

Heh, correct :D


> On 2010-04-13 11:08:02, George Goldberg wrote:
> > /trunk/playground/network/telepathy-contactlist/telepathy-bridge.h, line 256
> > <http://reviewboard.kde.org/r/3415/diff/6/?file=22199#file22199line256>
> >
> >     Please can you find some way to clarify in the docs here that it is all groups on a "local" account (not a IMAccount representing another contact).
> >     
> >     Also, what happens if the account is offline when this method is called?

Ok for the API docs. The behavior is consistent with what happens when calling the corresponding method in Tp-Qt4, so I suppose it should be fine.


- Dario


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


On 2010-03-30 10:42:54, Dario Freddi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3415/
> -----------------------------------------------------------
> 
> (Updated 2010-03-30 10:42:54)
> 
> 
> Review request for telepathy and George Goldberg.
> 
> 
> Summary
> -------
> 
> This patch adds three new classes and a variety of jobs (already ready to be librarized) for creating a Nepomuk->Telepathy bridge, making it very easy to access Tp-qt4 functions from clients using Nepomuk. Performance overhead looks trascurable.
> 
> By using this new class, contactlist is now able to add/remove people from a group, and adding/removing contacts. Metacontacts are still work in progress, but should not be a big deal.
> 
> Error handling is implemented library-side.
> 
> Everything which is not in the new classes is mostly a proof of concept, which will be refined as soon as this patch will make it in.
> 
> 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/add-contact-job.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/add-contact-job.cpp PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/add-contacts-to-group-job.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/add-contacts-to-group-job.cpp PRE-CREATION 
>   /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/meta-contact-item.h 1107996 
>   /trunk/playground/network/telepathy-contactlist/meta-contact-item.cpp 1107996 
>   /trunk/playground/network/telepathy-contactlist/remove-contacts-from-group-job.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/remove-contacts-from-group-job.cpp PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/remove-contacts-job.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/remove-contacts-job.cpp PRE-CREATION 
>   /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-base-job.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/telepathy-base-job.cpp PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/telepathy-base-job_p.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/telepathy-bridge.h PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/telepathy-bridge.cpp PRE-CREATION 
>   /trunk/playground/network/telepathy-contactlist/telepathy-bridge_p.h PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/3415/diff
> 
> 
> Testing
> -------
> 
> Works, empathy recognizes the changes correctly.
> 
> 
> Thanks,
> 
> Dario
> 
>