Bug 29531 (TpTextChannel)

 I'd like to have an high level text channel / Messages API as well. This is currently blocked by bug #29218 as we need to decide if we go for the subclass or view approach.

You can already observe existing channels but not using an Approver. You need to use an observer and set the "recover" flag. Best way is probably to define your own object derived from TpBaseClient  and make it an Observer and Approver.

I'd like to move Empathy 3.0 to the brigh Messages interface future and having this API looks like the best way to do it.

Let's start the API brainstorming. I'll focus on the Text and Messages features for now.

For the record the tp-qt4 equivalent of this API is http://telepathy.freedesktop.org/doc/telepathy-qt4/classTp_1_1TextChannel.html

How should we call this object? TpTextChannel?

I guess we are going to re-use TpMessage.

Would it be ok to make Message mandatory in order to use it? I'd say yes as we want to make it mandatory anyway (bug #29376).

The constructor would follow the usual channel pattern:

TpTextChannel *tp_text_channel_new (TpConnection *conn,
    const gchar *object_path,
    const GHashTable *immutable_properties,
    GError **error);


Properties and accessors
------------------------

const gchar * const * tp_text_channel_get_supported_content_types (
    TpTextChannel *self);

TpMessagePartSupportFlags tp_text_channel_get_message_part_support_flags (
    TpTextChannel *self);

TpDeliveryReportingSupportFlags tp_text_channel_get_delivery_reporting_support (
    TpTextChannel *self);

/* Return a list of TpMessage */
GList * tp_text_channel_get_pending_messages (TpTextChannel *self);

Methods
-------

const gchar * tp_text_channel_send_message (TpTextChannel *self,
    TpMessage *message,
    TpMessageSendingFlags flags);

Or should we have an _async function completing when the message has actually
be sent (or sending failed)? Then we could have something like:

const gchar * tp_text_channel_send_message_async (TpTextChannel *self,
    TpMessage *message,
    TpMessageSendingFlags flags,
    GAsyncReadyCallback callback,
    gpointer user_data);

gboolean tp_text_channel_send_message_finish (TpTextChannel *self,
    TpMessageSendingFlags *flags,
    GAsyncResult *result,
    GError **error);

void tp_text_channel_ack_messages_async (TpTextChannel *self,
    GList *messages, /* list of TpMessage */
    GAsyncReadyCallback callback,
    gpointer user_data);

gboolean tp_text_channel_ack_messages_finish (TpTextChannel *self,
    GAsyncResult *result,
    GError **error);

+ probably a _ack_message_{async.finish} as an helper for the single message
case

void tp_text_channel_forget (TpTextChannel *self,
    GList *messages, /* list of TpMessage */);

and maybe a forget_all() ?

Signals
-------

"message-received" (TpMessage *message)

"message-sent" (TpMessage *message, TpMessageSendingFlags flags, const gchar *token)

"pending-messages-removed" (GList *list of TpMessage)

Do we want to bind the "LostMessage" signal? We can't do much with it.

Features
--------

TP_TEXT_CHANNEL_FEATURE_PENDING : used to retrive pending messages

gboolean tp_text_channel_chat_state_supported (TpTextChannel *self);

void tp_text_channel_set_chat_state_async (TpTextChannel *self,
    TpChannelChatState state,
    GAsyncReadyCallback callback,
    gpointer user_data);

gboolean tp_text_channel_set_chat_state_finish (TpTextChannel *self,
    GAsyncResult *result,
    GError **error);

/* TpContact -> TpChannelChatState */
GHashTable * tp_text_channel_get_chat_states (TpTextChannel *self);

"chat-state-changed" : (TpContact *contact, TpChannelChatState state)

TP_TEXT_CHANNEL_FEATURE_CHAT_STATE : fetch chat states and connect ChatStateChanged

I have implemented most of the API described in Comment 3:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/text-channel-29531

- How should we deal with delivery reporting/failure. See the FIXME in
send_message_cb. Have a monitor object or something ?

- I didn't add API for MessageSent as it relies on the way we expose message
tokens (cf first point).

- Do we want _forget() as it tp-qt4 ?

- I didn't add API on GetPendingMessageContent() as I'm not sure how we should
represent this. Have API on TpSignalledMessage to retrieve some parts? Have a
kind of TpSignalledMessage preparation?

(In reply to comment #5)
> - I didn't add API on GetPendingMessageContent() as I'm not sure how we should
> represent this. Have API on TpSignalledMessage to retrieve some parts? Have a
> kind of TpSignalledMessage preparation?

Don't expose it. It's never been implemented, and is unworkable in its current form. Bug 26417 has more info.

(In reply to comment #5)
> - How should we deal with delivery reporting/failure. See the FIXME in
> send_message_cb. Have a monitor object or something ?

We could have something like that:

gboolean tp_text_channel_send_message_finish (TpTextChannel *self,
    GAsyncResult *result,
    TpMessageDeliveryMonitor **monitor,
    GError **error);

TpMessageDeliveryMonitor::delivery-report-received(TpSignalledMessage *)

But should we then expose delivery report in TpTextChannel::message-received?
I guess we should, except if the message-sent signal create a
TpMessageDeliveryMonitor as well so non-sender process can track the delivery.

But if the delivery report is never send we'll end up with monitor objects
staying around for all the lifetime of the channel.

Actually I'm wondering if that's not overkill and we should just give the
token in tp_text_channel_send_message_finish() and let the user do the
delivery-report mapping itself if it cares.

(In reply to comment #4)
> /* TpContact -> TpChannelChatState */
> GHashTable * tp_text_channel_get_chat_states (TpTextChannel *self);
> 
> "chat-state-changed" : (TpContact *contact, TpChannelChatState state)
> 
> TP_TEXT_CHANNEL_FEATURE_CHAT_STATE : fetch chat states and connect
> ChatStateChanged

Actually most of this already exists: TP_CHANNEL_FEATURE_CHAT_STATES,
"chat-state-changed" and tp_channel_get_chat_state().
Should we deprecate this API and replace it by a TpTextChannel version?

Maybe the new version of tp_channel_get_chat_state() and the sig should take a TpContact
instead?

Do we want the plural version? I guess it can be useful.

> gboolean tp_text_channel_chat_state_supported (TpTextChannel *self);

Do we want that or client should just check the interfaces supported?

> void tp_text_channel_set_chat_state_async (TpTextChannel *self,
>     TpChannelChatState state,
>     GAsyncReadyCallback callback,
>     gpointer user_data);
> 
> gboolean tp_text_channel_set_chat_state_finish (TpTextChannel *self,
>     GAsyncResult *result,
>     GError **error);

We still need this.

This looks as though it's on the right track.

> void tp_text_channel_forget (TpTextChannel *self,
>     GList *messages, /* list of TpMessage */);
> 
> and maybe a forget_all() ?

This is for Observers/Approvers to use, in situations where a Handler would acknowledge the message; in the Messages-not-mandatory world, you can't rely on being told about message acknowledgement. I'd be inclined to just press onwards with implementing Messages and stop caring about this case; the version of Empathy/Tpl/whatever that requires Messages can have a versioned Breaks on older CMs.

> Actually I'm wondering if that's not overkill and we should just give the
> token in tp_text_channel_send_message_finish() and let the user do the
> delivery-report mapping itself if it cares.

I agree, do that, precisely because we can't guarantee we'll ever be notified.

> Actually most of this already exists: TP_CHANNEL_FEATURE_CHAT_STATES,
> "chat-state-changed" and tp_channel_get_chat_state().
> Should we deprecate this API and replace it by a TpTextChannel version?

Perhaps one day. I'd be inclined to just leave it as-is for now.

> Maybe the new version of tp_channel_get_chat_state() and the sig should
> take a TpContact instead?

RESOLVED LATER, IMO :-)

> Do we want the plural version? I guess it can be useful.

I don't think returning the entire hash table is useful in practice; just have a lookup mechanism by handle, ID and/or contact. UIs should have a list of contacts (usually of length 1 in practice), and check the contact's chat state when drawing its icon (or whatever).

----------------------------------

Drive-by implementation review:

> add example_echo_2_channel_get_props()

Should never be necessary. Use TpExportableChannel:immutable-properties instead; if there are properties missing (perhaps the three from Messages), add them.

> +      const GHashTable *part = tp_message_peek (msg, 1);
> +      const gchar *text = tp_asv_get_string (part, "content");

We should have convenience API tp_message_get_text() for this, but it should basically be parts_to_text() from message-mixin.c (which deals with more subtleties, including the possibility of non-text content).

(In reply to comment #9)
> This looks as though it's on the right track.
> 
> > void tp_text_channel_forget (TpTextChannel *self,
> >     GList *messages, /* list of TpMessage */);
> > 
> > and maybe a forget_all() ?
> 
> This is for Observers/Approvers to use, in situations where a Handler would
> acknowledge the message; in the Messages-not-mandatory world, you can't rely on
> being told about message acknowledgement. I'd be inclined to just press onwards
> with implementing Messages and stop caring about this case; the version of
> Empathy/Tpl/whatever that requires Messages can have a versioned Breaks on
> older CMs.
>

Agreed.

> > Actually I'm wondering if that's not overkill and we should just give the
> > token in tp_text_channel_send_message_finish() and let the user do the
> > delivery-report mapping itself if it cares.
> 
> I agree, do that, precisely because we can't guarantee we'll ever be notified.

done.

> > add example_echo_2_channel_get_props()
> 
> Should never be necessary. Use TpExportableChannel:immutable-properties
> instead; if there are properties missing (perhaps the three from Messages), add
> them.

Done.
I guess http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=508af359ff78b4a5d8ea0d722ec2d046f9c4a9df
can be cherry picked and merged.

> > +      const GHashTable *part = tp_message_peek (msg, 1);
> > +      const gchar *text = tp_asv_get_string (part, "content");
> 
> We should have convenience API tp_message_get_text() for this, but it should
> basically be parts_to_text() from message-mixin.c (which deals with more
> subtleties, including the possibility of non-text content).

Ok, I'll add that to the message client branch.

(In reply to comment #10)
> I guess
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=508af359ff78b4a5d8ea0d722ec2d046f9c4a9df
> can be cherry picked and merged.

Yes please!

(In reply to comment #11)
> (In reply to comment #10)
> > I guess
> > http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=508af359ff78b4a5d8ea0d722ec2d046f9c4a9df
> > can be cherry picked and merged.
> 
> Yes please!

Done.

I added the "message-sent" signal and tp_text_channel_set_chat_state_async().

I think this branch is now ready for review. Merging is blocked by TpMessage refactoring but that shouldn't be an issue for reviewing.

I added code to fetch the TpContact of the sender.

As discussed on IRC, atm the TpSignalledMessage received from the message-sent signals don't have a sender. Should they? If yes, what's the best way to implement this?

a) Make mandatory to have message-sender[-id] define in messages announced in MessageSent. Then we can use this handle and get a TpContact from it.

b) If channel implements Group, use tp_channel_group_get_self_handle(). Then we need to prepare the GROUP feature, so we should have a TP_TEXT_CHANNEL_FEATURE_MESSAGE_SENT feature preparing GROUP if needed.

I'm still not sure about what I should do regarding Comment #14.

What should we do if a message doesn't have a message-sender? When fetching
pending messages we ignore them but when receiving a new message we don't (we
create a TpSignalledMessage without a sender). We should stay coherent here.

I am now using tp_connection_dup_contact_if_possible()

Anything else blocking this branch from being reviewed?

(In reply to comment #14)
> When fetching pending messages we ignore [messages without a sender]

I think it should be possible to receive messages with an unspecified sender (handle 0), with the API being "the handle is 0 or omitted, and the contact is NULL". We could use them for messages from the server, from the chatroom or from a contact who could not be identified, for instance. message-sender says "If 0 or omitted, the contact who sent the message could not be determined".

There is one pathological case where we might get messages with a nonzero handle but be forced to omit the sender contact:

* the CM is old, and so doesn't have immortal handles
* the CM is old, and so doesn't specify message-sender-id
* the handle was valid when we received the message, but became invalid before we could do HoldHandles on it

I think it'd be OK to deal with that by removing the sender handle (which is now meaningless).

If the CM doesn't have immortal handles, but does specify message-sender-id, it's slightly more reliable to do the "get a contact" round-trip in terms of the ID instead of the handle. You might not get the same handle back in pathological cases, in which case you need to edit the message-sender...

Hmm, I think this means we need to change _tp_signalled_message_new slightly, so that instead of asserting that the sender contact and the headers are consistent, it trusts the contact implicitly, and edits the headers to match? I'll do a mini-branch.

> a) Make mandatory to have message-sender[-id] define in messages announced in
> MessageSent. Then we can use this handle and get a TpContact from it.

I think I'd like to add this to the spec on general principles - sent messages should be as close as possible to what the peer receives - but ideally TpTextChannel shouldn't rely on it.

> b) If channel implements Group, use tp_channel_group_get_self_handle(). Then we
> need to prepare the GROUP feature, so we should have a
> TP_TEXT_CHANNEL_FEATURE_MESSAGE_SENT feature preparing GROUP if needed.

If we do (a), then it's OK for the fallback code in telepathy-glib to be a little less pedantic, so perhaps it could use the TpConnection:self-contact that I just added, and ignore the possibility of a channel-specific self handle?

That may still need to be async, because self-contact requires the CONNECTED feature on the connection, and we can theoretically start handling a channel before we've fully inspected the Connection.

(If you particularly want the channel-specific contact, I'm working on API to make every Group TpChannel guarantee that its contacts are available as TpContact objects.)

(In reply to comment #16)
> (In reply to comment #14)
> > When fetching pending messages we ignore [messages without a sender]
> 
> I think it should be possible to receive messages with an unspecified sender
> (handle 0), with the API being "the handle is 0 or omitted, and the contact is
> NULL". We could use them for messages from the server, from the chatroom or
> from a contact who could not be identified, for instance. message-sender says
> "If 0 or omitted, the contact who sent the message could not be determined".

Ok, I changed that.

> There is one pathological case where we might get messages with a nonzero
> handle but be forced to omit the sender contact:
> 
> * the CM is old, and so doesn't have immortal handles
> * the CM is old, and so doesn't specify message-sender-id
> * the handle was valid when we received the message, but became invalid before
> we could do HoldHandles on it

Humm, how can I detect such case? Should I always treat it as it when I get a
Message without message-sender-id on a connection not having immortal handles?

> I think it'd be OK to deal with that by removing the sender handle (which is
> now meaningless).
> 
> If the CM doesn't have immortal handles, but does specify message-sender-id,
> it's slightly more reliable to do the "get a contact" round-trip in terms of
> the ID instead of the handle. You might not get the same handle back in
> pathological cases, in which case you need to edit the message-sender...

I changed the code to always use ID, when possible.

> Hmm, I think this means we need to change _tp_signalled_message_new slightly,
> so that instead of asserting that the sender contact and the headers are
> consistent, it trusts the contact implicitly, and edits the headers to match?
> I'll do a mini-branch.

ok.

> > a) Make mandatory to have message-sender[-id] define in messages announced in
> > MessageSent. Then we can use this handle and get a TpContact from it.
> 
> I think I'd like to add this to the spec on general principles - sent messages
> should be as close as possible to what the peer receives - but ideally
> TpTextChannel shouldn't rely on it.

Agreed. Are you going to do the spec change or do you want me to?

> > b) If channel implements Group, use tp_channel_group_get_self_handle(). Then we
> > need to prepare the GROUP feature, so we should have a
> > TP_TEXT_CHANNEL_FEATURE_MESSAGE_SENT feature preparing GROUP if needed.
> 
> If we do (a), then it's OK for the fallback code in telepathy-glib to be a
> little less pedantic, so perhaps it could use the TpConnection:self-contact
> that I just added, and ignore the possibility of a channel-specific self
> handle?
> 
> That may still need to be async, because self-contact requires the CONNECTED
> feature on the connection, and we can theoretically start handling a channel
> before we've fully inspected the Connection.

We could add the preparation of CONNECTED in the CORE preparation of the
channel.

> (If you particularly want the channel-specific contact, I'm working on API to
> make every Group TpChannel guarantee that its contacts are available as
> TpContact objects.)

I think that's fine, as long it's just the fallback case.

(In reply to comment #17)
> > There is one pathological case where we might get messages with a nonzero
> > handle but be forced to omit the sender contact:
> > 
> > * the CM is old, and so doesn't have immortal handles
> > * the CM is old, and so doesn't specify message-sender-id
> > * the handle was valid when we received the message, but became invalid before
> > we could do HoldHandles on it
> 
> Humm, how can I detect such case? Should I always treat it as it when I get a
> Message without message-sender-id on a connection not having immortal handles?

You get a message with a nonzero message-sender but no message-sender-id, you call get_contacts_by_handle, and either you get an error or the handle comes back in the "failed" set.

> > I'll do a mini-branch.

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/messaging

> Agreed. Are you going to do the spec change or do you want me to?

I'll do it after lunch.

> We could add the preparation of CONNECTED in the CORE preparation of the
> channel.

We can't do that for TpChannel in general, because TLS and SASL channels need to be dispatched before the connection can connect.

We could perhaps do that for TpTextChannel's core feature if you wanted, though.

(In reply to comment #18)
> (In reply to comment #17)
> > > There is one pathological case where we might get messages with a nonzero
> > > handle but be forced to omit the sender contact:
> > > 
> > > * the CM is old, and so doesn't have immortal handles
> > > * the CM is old, and so doesn't specify message-sender-id
> > > * the handle was valid when we received the message, but became invalid before
> > > we could do HoldHandles on it
> > 
> > Humm, how can I detect such case? Should I always treat it as it when I get a
> > Message without message-sender-id on a connection not having immortal handles?
> 
> You get a message with a nonzero message-sender but no message-sender-id, you
> call get_contacts_by_handle, and either you get an error or the handle comes
> back in the "failed" set.

Ok, I think my code does the best it can to avoid such case.

> > We could add the preparation of CONNECTED in the CORE preparation of the
> > channel.
> 
> We can't do that for TpChannel in general, because TLS and SASL channels need
> to be dispatched before the connection can connect.
> 
> We could perhaps do that for TpTextChannel's core feature if you wanted,
> though.

Yeah that's what I meant. Or maybe we shouldn't bother as that's just a
fallback and just not send any sender if message-sender(-id) is not set AND
tp_connection_get_self_contact() returns NULL?

Review, part 1 in an ongoing series (I'll check the actual API next :-)

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.

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.

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

display_pending_messages behaves similarly. Could it just call message_received_cb?

Automatic proxy factory
-----------------------

+ *     <para><#TP_CHANNEL_FEATURE_CORE and #TP_CHANNEL_FEATURE_GROUP for all
+ *     type of channels.</para>

Stray "<" before #.

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

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

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

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

This is looking good. I need to do another review pass looking for implementation mistakes, since I wasn't entirely thorough the first time, but I think it's basically there.

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

I opened bug #32433 about that and created it manually for now.

The other things you didn't address with code changes (not removing pending-message-id; comparison with tp-qt4) both seem fine.

Re-review part 1: diffs to existing files, and the example text handler.

> +<INCLUDE>telepathy-glib/text-channel.h</INCLUDE>

Might be better to recommend telepathy-glib/telepathy-glib.h?

>   text = tp_message_to_text (message, NULL);
>   if (text == NULL)
>     return;

This always returns non-NULL (assuming nobody has g_return_val_if_fail'd), so it seems bad for an example to have this check.

Perhaps this'd make a better example:

    comment = ""

    text = tp_message_to_text (text, &flags);

    if (flags & TP_CHANNEL_TEXT_MESSAGE_FLAG_NON_TEXT_CONTENT)
      {
        comment = " (and some non-text content we don't understand)";
      }

    if (pending)
      g_print ("pending: '%s'%s", text, comment);
    else
      g_print ("received: '%s'%s", text, comment);

>    {
>      TpMessage *msg = l->data;
>
>
>      echo_message (channel, msg, TRUE);
>    }

Unnecessarily spacious :-)

> static void
> display_pending_messages (TpTextChannel *channel)

Perhaps add a comment something like

    /* The default channel factory used by the TpSimpleHandler has
     * already prepared TP_TEXT_CHANNEL_FEATURE_PENDING_MESSAGES,
     * if possible. */

Re-review part 2: the test.

>   test->wait = 2;
>   g_main_loop_run (test->mainloop);

No need to change it now, but I'm starting to prefer this idiom:

    while (thing_hasnt_happened (test) || other_thing_we_need < 6)
      g_main_context_iteration (NULL, TRUE);

which removes a lot of the strange action-at-a-distance you get from starting and stopping the main loop.

>  /* TODO: check sender */

What did you want to check?

>  /* We have to prepare the pending messages feature to be notified about
>   * incoming messages */
>  /* FIXME: Shouldn't we rename this feature then ? */

Yeah, it might be better as TP_TEXT_CHANNEL_FEATURE_MESSAGES or TP_TEXT_CHANNEL_FEATURE_INCOMING_MESSAGES.

The test looks fine.

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.

(In reply to comment #25)
> The other things you didn't address with code changes (not removing
> pending-message-id; comparison with tp-qt4) both seem fine.

humm sorry, I failed to parse this sentence. :)

(In reply to comment #26)
> Re-review part 1: diffs to existing files, and the example text handler.
> 
> > +<INCLUDE>telepathy-glib/text-channel.h</INCLUDE>
> 
> Might be better to recommend telepathy-glib/telepathy-glib.h?

If you prefer; changed.

> >   text = tp_message_to_text (message, NULL);
> >   if (text == NULL)
> >     return;
> 
> This always returns non-NULL (assuming nobody has g_return_val_if_fail'd), so
> it seems bad for an example to have this check.
> 
> Perhaps this'd make a better example:
> 
>     comment = ""
> 
>     text = tp_message_to_text (text, &flags);
> 
>     if (flags & TP_CHANNEL_TEXT_MESSAGE_FLAG_NON_TEXT_CONTENT)
>       {
>         comment = " (and some non-text content we don't understand)";
>       }
> 
>     if (pending)
>       g_print ("pending: '%s'%s", text, comment);
>     else
>       g_print ("received: '%s'%s", text, comment);

good idea; changed.

> >    {
> >      TpMessage *msg = l->data;
> >
> >
> >      echo_message (channel, msg, TRUE);
> >    }
> 
> Unnecessarily spacious :-)

removed.

> > static void
> > display_pending_messages (TpTextChannel *channel)
> 
> Perhaps add a comment something like
> 
>     /* The default channel factory used by the TpSimpleHandler has
>      * already prepared TP_TEXT_CHANNEL_FEATURE_PENDING_MESSAGES,
>      * if possible. */

added.

(In reply to comment #27)
> Re-review part 2: the test.
> 
> >   test->wait = 2;
> >   g_main_loop_run (test->mainloop);
> 
> No need to change it now, but I'm starting to prefer this idiom:
>     while (thing_hasnt_happened (test) || other_thing_we_need < 6)
>       g_main_context_iteration (NULL, TRUE);
> 
> which removes a lot of the strange action-at-a-distance you get from starting
> and stopping the main loop.

Ok, noted for future work. IIRC I borrowed this pattern from wocky's tests.

> >  /* TODO: check sender */
> 
> What did you want to check?

Seems I'm doing it now; removed.

> >  /* We have to prepare the pending messages feature to be notified about
> >   * incoming messages */
> >  /* FIXME: Shouldn't we rename this feature then ? */
> 
> Yeah, it might be better as TP_TEXT_CHANNEL_FEATURE_MESSAGES or
> TP_TEXT_CHANNEL_FEATURE_INCOMING_MESSAGES.

I picked TP_TEXT_CHANNEL_FEATURE_INCOMING_MESSAGES as it's clearer imho.

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

(In reply to comment #29)
> (In reply to comment #25)
> > The other things you didn't address with code changes (not removing
> > pending-message-id; comparison with tp-qt4) both seem fine.
> 
> humm sorry, I failed to parse this sentence. :)

Rephrasing: you convinced me that you don't need to do anything about those two things.

(In reply to comment #32)
> > 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?

Yeah, I suppose so. If we decide to do the "pending pending messages" version in tp-glib later, it won't be an API break, so we have an escape route if it turns out to be a practical problem.

OK, ship it!

Merged \o/

Will be in 0.13.10