[Bug 29531] high-level API for text channels

[bugzilla-daemon at freedesktop.org]

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

--- Comment #21 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-12-13 09:50:52 PST ---
API review
----------

The documentation of ack_message(s)_async should use the verb "acknowledge",
even if you want to keep the shorter C name (which would be reasonable, IMO).

Whichever of the ack_message(s)_async functions you think will be more commonly
used should explain what "acknowledge" means in Telepathy terms (i.e. "I have
shown it to the user now", remove from pending-message queue, only the
channel's main handler is allowed to do it), and the less common one should
hyperlink to the more common.

Either ::pending-messages-removed should really be plural, or it should have
the "s" removed from "messages". I think it's OK being singular.

+/* Move this as TpMessage (or TpSignalledMessage?) API ? */
+static guint
+get_pending_message_id (TpMessage *msg,

It probably shouldn't be API outside telepathy-glib (because you should be
using TpTextChannel instead!), but having an internal
_tp_signalled_message_get_pending_id() seems sensible.

It might even be worth removing pending-message-id from the parts, and putting
it somewhere else in the struct, to discourage misuse.

It'd be nice to do a compare/contrast of this class vs. telepathy-qt4's
equivalent before merging it. One obvious difference is that telepathy-qt4 has
more Features, but some of those are actually unnecessary now (for instance,
the "what can I send?" Feature is redundant now that we've decided those
properties should have always been immutable).

Implementation review
---------------------

I've only done a drive-by review of the implementation - I'll need to have
another look through this, I think.

send_message_cb will get token = "" if there's no token, whereas
tp_text_channel_send_message_finish claims that that's token = NULL. Something
needs to translate - I'd do it in send_message_cb, personally. message_sent_cb
gets this right.

> + * Return the a newly allocated list of not acked #TpSignalledMessage.

"#TpSignalledMessage<!-- -->s" or "#TpSignalledMessage objects" for great
grammatical justice!

"unacknowledged" sounds a bit better than "not acked" IMO :-)

> +      DEBUG ("Pending messages may be re-ordered, please fix CM");

If you're going to have messages like that, please cite which CM it is (the
connection's object path, perhaps). The same in message_sent_cb.

> +get_pending_messages_cb (TpProxy *proxy,
...
> +  messages = g_value_get_boxed (value);

This crashes if the CM returns the wrong D-Bus type; please check with
G_VALUE_HOLDS first.

+      if (sender_ids->len > 0)
+        {
+          /* Use the sender ID rather than the handles */
+          tp_connection_get_contacts_by_id (conn, sender_ids->len,
+              (const gchar * const *) sender_ids->pdata,

This is checking for "there exists a message with a message-sender-id". The
list of senders will get misaligned with the list of copied message blobs
unless all the messages have one.

In practice this would only happen if the CM was insane, so I'd be happy with
changing the check to "every message has a message-sender-id", and falling back
to starting from the handles if any of them lacks the ID.

+      parts_list = g_list_prepend (parts_list, copy_parts (parts));

IMO this is too far away from the g_list_reverse() to not be astonishing. wjt
would say "use a GQueue, which can append in O(1) time"; or I'd be reasonably
happy with a comment.

Are you passing @self through the weak_object mechanism because you want this
operation cancelled on destruction, or just because you wanted a second
user_data?

> +got_sender_contact_by_id_cb (TpConnection *connection,
>...
> +      goto out;
> +    }
> +
> +  sender = contacts[0];
> +
> +out:

This could easily avoid the goto: if (error != NULL) { debug } else if
(n_contacts == 1) { success } else { debug }. The same for the by-handles
variant.

self->priv->pending_messages should definitely be a GQueue.

> +      DEBUG ("Message received on Channel %s doesn't have message-sender, "
> +          "please fix CM", tp_proxy_get_object_path (self));

This is not an error. Messages can legitimately have no particular sender (we
don't have any CMs that actually use this feature, but the spec claims it
should work).

We should perhaps implement it in Gabble, in fact, for messages from rooms... I
wonder how many UIs that would break? :-)

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