[Bug 38801] Missing _async() wrappers for Channel.Group, Connection.ContactList and Connection.ContactGroups interfaces

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=38801

--- Comment #4 from Xavier Claessens <xclaesse at gmail.com> 2011-07-01 04:16:19 PDT ---
(In reply to comment #3)
> ::: telepathy-glib/channel-contacts.c
> @@ +49,3 @@
> + *
> + * Invite all the given @contacts into the channel, or accept requests for
> + * channel membership for @contacts on the pending local list.
> 
> This is pretty cryptic if you are not a Telepathy expert. If the goal of this
> function is to invite people to a channel, then it should be call _invite(). If
> it it to accept an invitation it should be _accept_invite().

It does both invite and accept. I could add 2 different methods doing the same
thing but with different names for API clarity, but I don't think it has much
benefit.

> @@ +56,3 @@
> +tp_channel_group_add_members_async (TpChannel *self,
> +    guint n_contacts,
> +    TpContact * const *contacts,
> 
> Do we really want to use the n_contacts + *contacts pattern here? In practice,
> user will always has to allocate a GArray and pass array->data, array->len. The
> only simpler case is when passing only one contact for which I'd prefer to have
> another simpler API.

I think that's the most convenient way if we want to keep multi-contact API. It
has the advantages:
1) easy to make for 1 contact: TpContact *contact; foo(self, 1, &contact);
2) easy for static arrays: TpContact *contacts[] = {c1, c2}; foo(self, 2
contacts);
3) easy when getting GPtrArray from other APIs: GPtrArray *contacts =
tp_channel_group_dup_local_pending_contacts(channel);
tp_channel_group_add_contacts_async(channel, contacts->len, contacts->pdata);
to accept all local pending at once.
4) it is consistent with at least tp_connection_upgrade_contacts() and various
other APIs taking arrays like tp_proxy_prepare_async().
5) it is introspectable

otoh we could do single-contact APIs, the top commit does it for ContactList
and ContactGroups. For the TpChannel case I would call them
tp_channel_group_{invite,accept,rescind}_contact_async(TpChannel *self,
TpContact *contact, blabla);

Or do you think we need only single-contact API?

> @@ +78,3 @@
> + * Finishes tp_channel_group_remove_members_async()
> + *
> + * Returns: %FALSE if the operation failed, %TRUE otherwise.
> 
> We usually use something like "TRUE if the operation was successful, otherwise
> FALSE" which is a bit more positive. :)

Surely I copied that from somewhere else, but I agree it's better to be
positive. I'll make the change.

> @@ +102,3 @@
> + *
> + * Requests the removal of @contacts from a channel, reject their request for
> + * channel membership on the pending local list, or rescind their invitation
> on
> 
> rescind?
> 
> Same problem, this function is way too complicated. High level API should
> provide an easier API to use, not a direct mapping on the spec.

I understand, but then you would need different method names doing the same
thing just to have cool names. I'm not sure that's better.

> ::: telepathy-glib/connection-contact-list.c
> @@ +68,3 @@
> +void
> +tp_connection_request_subscription_async (TpConnection *self,
> +    guint n_contacts, TpContact * const *contacts, const gchar *message,
> 
> one arg per line.

ok

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.