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

Tue Apr 13 13:07:56 CEST 2010

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

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

/trunk/playground/network/telepathy-contactlist/add-contact-job.cpp
<http://reviewboard.kde.org/r/3415/#comment4290>

    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.

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

    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.

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

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

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

    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?

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

    You've got Nepomuk::Person here, when it should be a Nepomuk::PersonContact.

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

    And here, it's the other way round :)

    Should be Nepomuk::Person instead of Nepomuk::PersonContact.

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

    s/could/can/

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

    s/explicitely/explicitly/g

/trunk/playground/network/telepathy-contactlist/telepathy-bridge.cpp
<http://reviewboard.kde.org/r/3415/#comment4299>

- George

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