Bug 38801

Here it is: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=async-op

Created attachment 48645 [details] [review]
diff or it didn't happen

Review of attachment 48645 [details] [review]:

I just reviewed the top 3 patches. Do we really want to expose those as high level API. Shouldn't we have an higher level / easier API instead?

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

@@ +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.

@@ +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. :)

@@ +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.

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

(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

So the questions raised by Guillaume are basically:

1) Do we want to keep multi-contact API, or only single-contac, or both? IMO most of the time only single-contact API is needed, and it could look nicer, but the multi-contact API is trivial to use with a single contact (1, &contact) and that could save several dbus roundtrips which is which is why we designed the spec like that. And having both is nice for the ContactList stuff because they are on tp_contact_foo_async() but for channel operations it isn't so nice to duplicate the methods...

2) Do we want both tp_channel_group_invite_async() and tp_channel_group_accept_async() methods doing the same thing, for nicer method names, or is it fine to keep tp_channel_group_add_members_async() which does both.

Comments?

(In reply to comment #5)
> So the questions raised by Guillaume are basically:
> 
> 1) Do we want to keep multi-contact API, or only single-contac, or both? IMO
> most of the time only single-contact API is needed, and it could look nicer,
> but the multi-contact API is trivial to use with a single contact (1, &contact)
> and that could save several dbus roundtrips which is which is why we designed
> the spec like that. And having both is nice for the ContactList stuff because
> they are on tp_contact_foo_async() but for channel operations it isn't so nice
> to duplicate the methods...

I'm tempted to say “add both, where add_contact_async is a tiny tiny wrapper around add_contactS_async” but I think that will clutter things.

Empathy, for instance, could well use the plural API for inviting contacts to a group chat. Removing contacts could also conceivably be plural: select 5 contacts in the sidebar and mash a [remove] button at the bottom. So I think reasonable applications may want the plural versions, so let's just have plural versions, with clear examples in the docs for how to use them singularly in C.

> 2) Do we want both tp_channel_group_invite_async() and
> tp_channel_group_accept_async() methods doing the same thing, for nicer method
> names, or is it fine to keep tp_channel_group_add_members_async() which does
> both.

It's an implementation detail that they do the same thing on D-Bus. I assume _accept_async() is for accepting other people's join requests, and there would be an _accept_invitation_async() for accepting someone inviting us?

(In reply to comment #4)
> (In reply to comment #3)
> > @@ +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.


Fixed

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

Fixed

(In reply to comment #6)
> (In reply to comment #5)
> > So the questions raised by Guillaume are basically:
> > 
> > 1) Do we want to keep multi-contact API, or only single-contac, or both? IMO
> > most of the time only single-contact API is needed, and it could look nicer,
> > but the multi-contact API is trivial to use with a single contact (1, &contact)
> > and that could save several dbus roundtrips which is which is why we designed
> > the spec like that. And having both is nice for the ContactList stuff because
> > they are on tp_contact_foo_async() but for channel operations it isn't so nice
> > to duplicate the methods...
> 
> I'm tempted to say “add both, where add_contact_async is a tiny tiny wrapper
> around add_contactS_async” but I think that will clutter things.
> 
> Empathy, for instance, could well use the plural API for inviting contacts to a
> group chat. Removing contacts could also conceivably be plural: select 5
> contacts in the sidebar and mash a [remove] button at the bottom. So I think
> reasonable applications may want the plural versions, so let's just have plural
> versions, with clear examples in the docs for how to use them singularly in C.

Fair enough.

> > 2) Do we want both tp_channel_group_invite_async() and
> > tp_channel_group_accept_async() methods doing the same thing, for nicer method
> > names, or is it fine to keep tp_channel_group_add_members_async() which does
> > both.
> 
> It's an implementation detail that they do the same thing on D-Bus. I assume
> _accept_async() is for accepting other people's join requests, and there would
> be an _accept_invitation_async() for accepting someone inviting us?

I agree, especially for the accept_invite case: why should user have to pass
his self TpContact to accept an invitation?

What's using this actually? Text for room invitations; StreamedMedia iirc,
Call? Maybe this should be part of the TpChannel subclass to properly watch
the exact semantic depending of the channel types?

I think local-pending on TpChannel is only legacy for contact_list channels. For text/call channels it would only contain the self-handle anyway.

So if that's true, I totally agree it would be nicer to have tp_channel-group_accept_invitation() that does a AddMembers with self handle.

Going to make that change in my branch unless someone speaks now :D

Created attachment 49125 [details] [review]
Add _tp_contacts_to_handles()

Help implementing _async(guint n_contacts, TpContact **contacts) methods

Created attachment 49126 [details] [review]
Add async wrapper for RequestSubscription, AuthorizePublication, RemoveContacts, Unsubscribe and Unpublish

Created attachment 49127 [details] [review]
Add async wrapper for SetGroupMembers, AddToGroup, RemoveFromGroup, RemoveGroup and RenameGroup

Created attachment 49128 [details] [review]
add convenient wrappers for single-contact operations

Created attachment 49129 [details] [review]
Be more positive in doc

Review of attachment 49125 [details] [review]:

Looks good.

Review of attachment 49126 [details] [review]:

::: docs/reference/telepathy-glib-sections.txt
@@ +3543,3 @@
 tp_contact_info_list_copy
 tp_contact_info_list_free
+<SUBSECTION contact-list>

You should either add
<INCLUDE>telepathy-glib/connection-contact-list.h</INCLUDE>
or include it in connection.h

::: telepathy-glib/connection-contact-list.c
@@ +49,3 @@
+    g_return_if_fail (TP_IS_CONNECTION (self)); \
+    g_return_if_fail (tp_proxy_has_interface_by_id (self, \
+        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_LIST)); \

Isn't this too much? That means apps will have to check if the connection does implement CL before using this function if they don't want to raise a critical.

@@ +72,3 @@
+ * @message: an optional plain-text message from the user, to send to those
+ *  @contacts with the subscription request. The
+ *  #TpConnection:request-uses-message property indicates whether this message

This property doesn't exist yet.

::: telepathy-glib/connection-contact-list.h
@@ +1,2 @@
+/*
+ * connection.h - proxy for a Telepathy connection

bouh

Review of attachment 49127 [details] [review]:

::: telepathy-glib/connection-contact-list.c
@@ +313,3 @@
+    g_return_if_fail (TP_IS_CONNECTION (self)); \
+    g_return_if_fail (tp_proxy_has_interface_by_id (self, \
+    GSimpleAsyncResult *result; \

Same comment as above.

@@ +315,3 @@
+        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_GROUPS)); \
+    g_return_if_fail (_tp_contacts_to_handles (self, n_contacts, contacts, \
+    GArray *handles; \

add g_return_if_fail (group != NULL);

@@ +496,3 @@
+    g_return_if_fail (tp_proxy_has_interface_by_id (self,
+        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_GROUPS));
+}

check group != NULL

@@ +555,3 @@
+    g_return_if_fail (tp_proxy_has_interface_by_id (self,
+        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_GROUPS));
+ * @callback: a callback to call when the operation finishes

Same here, for boths.

Review of attachment 49128 [details] [review]:

::: docs/reference/telepathy-glib-sections.txt
@@ +4210,3 @@
 tp_contact_set_contact_groups_finish
 tp_contact_has_feature
+<SUBSECTION operations>

Same problem regarding the included header.

::: telepathy-glib/contact-operations.c
@@ +1,3 @@
+/* Async operations for TpContact
+ *
+ * Copyright (C) 2011 Collabora Ltd. <http://www.collabora.co.uk/>

I'm sure Will won't approve this without a proper ©

@@ +32,3 @@
+ * @user_data: data to pass to @callback
+ *
+ * Convenient wrapper for tp_connection_request_subscription_async() on single

"on a single contact" ? Not sure, check with a native.

Review of attachment 49129 [details] [review]:

This patch can easily be splitted and squashed with the appropriate commits.

(In reply to comment #16)
> Review of attachment 49126 [details] [review]:
> 
> ::: docs/reference/telepathy-glib-sections.txt
> @@ +3543,3 @@
>  tp_contact_info_list_copy
>  tp_contact_info_list_free
> +<SUBSECTION contact-list>
> 
> You should either add
> <INCLUDE>telepathy-glib/connection-contact-list.h</INCLUDE>
> or include it in connection.h

It should be the global telepathy-glib.h actually. Fixed.

> ::: telepathy-glib/connection-contact-list.c
> @@ +49,3 @@
> +    g_return_if_fail (TP_IS_CONNECTION (self)); \
> +    g_return_if_fail (tp_proxy_has_interface_by_id (self, \
> +        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_LIST)); \
> 
> Isn't this too much? That means apps will have to check if the connection does
> implement CL before using this function if they don't want to raise a critical.

I'm not going to make fallbacks to legacy contact-list channels, so we really need ContactList iface. It's better to warn loudly than having apps relying on wrong assumptions. I think apps should wait for ContactList to be implemented into tp-python though.

> @@ +72,3 @@
> + * @message: an optional plain-text message from the user, to send to those
> + *  @contacts with the subscription request. The
> + *  #TpConnection:request-uses-message property indicates whether this message
> 
> This property doesn't exist yet.

right, dropped that part.

> ::: telepathy-glib/connection-contact-list.h
> @@ +1,2 @@
> +/*
> + * connection.h - proxy for a Telepathy connection
> 
> bouh

Fixed


(In reply to comment #17)
> Review of attachment 49127 [details] [review]:
> @@ +315,3 @@
> +        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_GROUPS)); \
> +    g_return_if_fail (_tp_contacts_to_handles (self, n_contacts, contacts, \
> +    GArray *handles; \
> 
> add g_return_if_fail (group != NULL);

fixed

> @@ +496,3 @@
> +    g_return_if_fail (tp_proxy_has_interface_by_id (self,
> +        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_GROUPS));
> +}
> 
> check group != NULL
fixed

> @@ +555,3 @@
> +    g_return_if_fail (tp_proxy_has_interface_by_id (self,
> +        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_GROUPS));
> + * @callback: a callback to call when the operation finishes
> 
> Same here, for boths.

fixed

(In reply to comment #18)
> Review of attachment 49128 [details] [review]:
> 
> ::: docs/reference/telepathy-glib-sections.txt
> @@ +4210,3 @@
>  tp_contact_set_contact_groups_finish
>  tp_contact_has_feature
> +<SUBSECTION operations>
> 
> Same problem regarding the included header.

fixed

> ::: telepathy-glib/contact-operations.c
> @@ +1,3 @@
> +/* Async operations for TpContact
> + *
> + * Copyright (C) 2011 Collabora Ltd. <http://www.collabora.co.uk/>
> 
> I'm sure Will won't approve this without a proper ©

fixed

> @@ +32,3 @@
> + * @user_data: data to pass to @callback
> + *
> + * Convenient wrapper for tp_connection_request_subscription_async() on single
> 
> "on a single contact" ? Not sure, check with a native.

fixed

(In reply to comment #19)
> Review of attachment 49129 [details] [review]:
> 
> This patch can easily be splitted and squashed with the appropriate commits.

done

(In reply to comment #20)
> > ::: telepathy-glib/connection-contact-list.c
> > @@ +49,3 @@
> > +    g_return_if_fail (TP_IS_CONNECTION (self)); \
> > +    g_return_if_fail (tp_proxy_has_interface_by_id (self, \
> > +        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_LIST)); \
> > 
> > Isn't this too much? That means apps will have to check if the connection does
> > implement CL before using this function if they don't want to raise a critical.
> 
> I'm not going to make fallbacks to legacy contact-list channels, so we really
> need ContactList iface. It's better to warn loudly than having apps relying on
> wrong assumptions. I think apps should wait for ContactList to be implemented
> into tp-python though.

I agree, but shouldn't we fail the async operation rather than potentially
crash the app? g_return_if_fail() are used if the app did something wrong but
in that case it's not its fault if a CM is too old.

Channel.Group is bug #37359, remove it from the title.

(In reply to comment #22)
> (In reply to comment #20)
> > > ::: telepathy-glib/connection-contact-list.c
> > > @@ +49,3 @@
> > > +    g_return_if_fail (TP_IS_CONNECTION (self)); \
> > > +    g_return_if_fail (tp_proxy_has_interface_by_id (self, \
> > > +        TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACT_LIST)); \
> > > 
> > > Isn't this too much? That means apps will have to check if the connection does
> > > implement CL before using this function if they don't want to raise a critical.
> > 
> > I'm not going to make fallbacks to legacy contact-list channels, so we really
> > need ContactList iface. It's better to warn loudly than having apps relying on
> > wrong assumptions. I think apps should wait for ContactList to be implemented
> > into tp-python though.
> 
> I agree, but shouldn't we fail the async operation rather than potentially
> crash the app? g_return_if_fail() are used if the app did something wrong but
> in that case it's not its fault if a CM is too old.

Ok, smcv agree with you too, so I modified my code.

++

Branch merged !