[Bug 29531] high-level API for text channels

[bugzilla-daemon at freedesktop.org]

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

--- Comment #22 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-12-14 03:21:06 PST ---
(In reply to comment #20)
> Using "@since 0.13.UNRELEASED" is a Doxygen thing - for gtk-doc the syntax you
> want is "Since: 0.13.UNRELEASED". Similarly, use "Deprecated:" rather than
> "@deprecated". I have a telepathy-glib branch to fix all the places where
> that's wrong so far.

All the TpTextChannel API seems ok.

> Example text handler
> --------------------
> 
> message_received_cb in the example text handler should probably call
> tp_message_to_text rather than ferreting around in the message parts - in
> particular, there's a (pre-existing) bug where if sent a message with binary
> content (type 'y' rather than 's'), it'll try to echo NULL, and g_ascii_strup
> will probably assert.

Done and fixed.

> Similarly, it'll crash if the message doesn't have at least one part, as far as
> I can see?

Should be fine now I'm using tp_message_to_text().

> display_pending_messages behaves similarly. Could it just call
> message_received_cb?

Not really because the pending callback ack all the messages at once (which is
good to do in an example). But I refactored this code so remove the code
duplication.

> Automatic proxy factory
> -----------------------
> 
> + *     <para><#TP_CHANNEL_FEATURE_CORE and #TP_CHANNEL_FEATURE_GROUP for all
> + *     type of channels.</para>
> 
> Stray "<" before #.

Fixed.

> Ideally (pseudo-)constants would be prefixed with %, like
> %TP_CHANNEL_FEATURE_CORE (I'm not sure whether gtk-doc actually cares, though).

fixed.

> >      TpChannel *channel G_GNUC_UNUSED)
> >  {
> > -  return tp_automatic_proxy_factory_dup_channel_features_impl ();
> > +  return tp_automatic_proxy_factory_dup_channel_features_impl (channel);
> 
> G_GNUC_UNUSED? o rly? :-)
> 
> (Appears twice, for historical reasons.)

removed.

(In reply to comment #21)
> 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).

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

done.

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

I guess you refered to the SIG_ macros as the signal itself was already
singular. Fixed.

> +/* 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.

done.

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

I'm not convinced it's worth it tbh. And if at some point we add D-Bus API
refferring to the message-id in some way, user would be happy to be able to
use it without being blocked on tp-glib.

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

The changes I noted are:
- The forget method, but we agreed to not implement it here.
- Our API is TpMessage oriented while the Qt one uses parts. Ours is better :)
- They have a send (const QString &text, ) variant. Not useful here as we have
helper code to build a simple text message.
- There is API for more features such as inviteContacts(). We can add it
later.

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

done.

> > + * Return the a newly allocated list of not acked #TpSignalledMessage.
> 
> "#TpSignalledMessage<!-- -->s" or "#TpSignalledMessage objects" for great
> grammatical justice!

fixed.

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

Ooops indeed, changed.

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

The object path of the channel (which should be informative enough to get the
CM) is already displayed in get_sender(), just before displaying this message.
But yeah, better being clearer. done.

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

There is no TP_ARRAY_TYPE_MESSAGE_PART_LIST_LIST...

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

Yeah I was assuming that the CM is not completely shit. :)
I made it safer.

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

I added 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?

both.
I noticed that parts_list was leaked if the cb was not called. That's not
fixed by passing a GDestroyNotify.

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

done.

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

right, I changed it.

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

I removed the "please fix CM" part of the comment.

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