[Bug 29531] high-level API for text channels

[bugzilla-daemon at freedesktop.org]

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

--- Comment #32 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-12-16 06:55:01 PST ---
(In reply to comment #28)
> > 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).

I want for the cast option, no need to dup if not needed.

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

Oh cool; done.

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

done.

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

done.

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

Humm can't we just say that if you really care about this issue then you
should fix your CM?

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

done

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

done.

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

Oops; fixed.

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

done.

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