[Bug 29531] high-level API for text channels

[bugzilla-daemon at freedesktop.org]

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

--- Comment #28 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-12-16 04:52:54 PST ---
Re-review part 3 of 3: the actual implementation.

> GStrv tp_text_channel_get_supported_content_types (TpTextChannel *self);

Either rename it to _dup_ and make the strv be copied, or cast the strv to
(const gchar * const *) to return it (in which case you have to do the same
hack as for tp_protocol_get_authentication_types, to make gtk-doc notice the
method).

>   if (self->priv->supported_content_types == NULL)
>     {
>       DEBUG ("Channel doesn't have Messages.SupportedContentTypes in its "
>           "immutable properties");
>     }

I think it'd be worth setting self->priv->supported_content_types to a non-NULL
value, so that tp_text_channel_get_supported_content_types() always returns
non-NULL?

Indeed, you could use { "text/plain", NULL } since telepathy-spec mandates that
plain text is always allowed.

Having the two numeric things default to 0 is fine, though.

>   if (err != NULL)
>     {
>       DEBUG ("Failed to connect to MessageSent: %s", err->message);

This can only happen if the channel has been invalidated, which really
shouldn't have happened already in the ctor, so I think a WARNING or even
CRITICAL would be fine.

find_msg_by_id:
>   return msg_id - id;

I'd use (msg_id != id), to emphasize that this is not a sorting function, just
a comparator with a strange signature. (Why doesn't g_queue_find_custom take a
function that returns a boolean? We just don't know.)

The handling of PendingMessagesRemoved is a bit unfortunate. If we're waiting
for messages' senders to be resolved, and the message is removed while we wait,
then we'll ignore the removal, and it'll sit there forever...

The solution would be to have a counter for the number of "pending pending
messages", and defer processing of removed messages until it reaches 0, I
suppose. I look forward to Telepathy 1.0 making this unnecessary.

PendingMessagesRemoved should also be ignored completely (early-return) until
the initial PendingMessages have been received, I think - at the moment it'll
debug a lot and do nothing, because the GQueue is empty.

>   self->priv->retrieving_pending = TRUE;

Your logic is correct, but it seems safer to invert the sense: gboolean
got_initial_messages. Then it'll automatically start off FALSE without you
needing to do anything special.

>   _tp_proxy_set_feature_prepared (proxy,
>       TP_TEXT_CHANNEL_FEATURE_PENDING_MESSAGES, TRUE);

Do you really want tp_proxy_is_prepared (self, PENDING_MESSAGES) to return TRUE
if connecting one of the rather important signals failed?

> tp_text_channel_get_supported_content_types

I'd rather have a TP_IS_TEXT_CHANNEL check in these functions, rather than just
segfaulting if people get it wrong.

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