Bug 29378

I started implementing this in this branch:
http://git.Collabora.co.uk/?p=user/cassidy/telepathy-idle;a=shortlog;h=refs/heads/messages-29378

I'm neither an Idle or Messages expert so it may not be perfect.

I also have 2 questions:

- I didn't generate a message-token as Gabble does. Should I?

- messages/split-msg-sent-signal.py is broken. When splitting long messages (because of protocol limitations), Idle used to fire the Sent signal for each piece. Now I'm calling tp_message_mixin_sent() only once so this signal is fired one time. I'm not sure it really makes sense to expose in Telepathy the fact that messages are split. Furthermore, the mixin doesn't easily allow us to call tp_message_mixin_sent() multi times as the TpMessage is created by tp-glib and ownership is passed back to it once we call tp_message_mixin_sent() (so we can't re-use it for the other pieces).

Yeah, emitting Sent just once is what you should do IMO - the original message splitting impl was just a real quick hack IIRC and I didn't properly consider this it seems :) Another issue is Sent should only be emitted when the last part of the message is actually cleared from the server connection send queue in IdleConnection (not when it's only queued there), so there should be some kind of callback mechanism for dequeued messages - or was there already, I can't recall...

As for the message token, at least tp-qt4 explicitly doesn't require one to be present. You should generate one if you can in any way produce an unique id for the message, though - which might be a bit hard on IRC as there are no running serials or message ids or anything. What does gabble do, hash something? What does it hash exactly?

I'll probably take a look at the other parts sometime soon but I can't guarantee anything as I'm frankly quite busy at the moment.

Why have a NUM_SUPPORTED_MESSAGE_TYPES? G_N_ELEMENTS() seems more useful.

+ if (content_type == NULL || tp_strdiff (content_type, "text/plain"))

Why is @content_type == NULL special?

+	if (text == NULL)

Do you also need to check for strlen == 0?

+		goto despair_island;

Amusing, but I'm thinking no.

Could the message received callback be made common and part of idle-text.c ?

(In reply to comment #3)
> Why have a NUM_SUPPORTED_MESSAGE_TYPES? G_N_ELEMENTS() seems more useful.

fixed.

> + if (content_type == NULL || tp_strdiff (content_type, "text/plain"))
> 
> Why is @content_type == NULL special?

removed.

> +    if (text == NULL)
> 
> Do you also need to check for strlen == 0?

I use tp_str_empty() now.

> +        goto despair_island;
> 
> Amusing, but I'm thinking no.

That's from Gabble :p
I renamed it.

> Could the message received callback be made common and part of idle-text.c ?

Good idea. Done.

This looks good to merge, but...

> +		G_IMPLEMENT_INTERFACE(TP_TYPE_SVC_CHANNEL_TYPE_TEXT, tp_message_mixin_text_iface_init);
> +		G_IMPLEMENT_INTERFACE (TP_TYPE_SVC_CHANNEL_INTERFACE_MESSAGES, tp_message_mixin_messages_iface_init);

In future please be consistent with whitespace on consecutive lines, at least :-P

(In reply to comment #2)
> Yeah, emitting Sent just once is what you should do IMO

I disagree, actually: Sent should reflect what actually happened (in particular, if you edit the message to get rid of CTCP or whatever, the Sent signal should carry what you actually sent). Reversing this isn't a blocker for this branch, though.

(In reply to comment #1)
> - I didn't generate a message-token as Gabble does. Should I?

No. Only generate a message-token if there's some actually-unique identifier on the message.

(Note that message-token currently has impractical semantics in any case.)

> Furthermore, the mixin doesn't easily allow us to
> call tp_message_mixin_sent() multi times as the TpMessage is created by tp-glib
> and ownership is passed back to it once we call tp_message_mixin_sent() (so we
> can't re-use it for the other pieces).

The TpMessage API wasn't really designed with message splitting in mind: you'd have to do something like:

* truncate the message body in the first fragment
* call tp_message_new() for the second and subsequent fragments, and copy in the details

I think that'd work? I'll try to bear this in mind in any future work on TpMessage.

(In reply to comment #2)
> Another issue is Sent should only be emitted when the last
> part of the message is actually cleared from the server connection send queue
> in IdleConnection (not when it's only queued there), so there should be some
> kind of callback mechanism for dequeued messages - or was there already, I
> can't recall...

"Sent" really means "submitted for sending" (if you delay until you're sure the message has been sent, other UIs can't see it for a while). If the connection is disconnected while the queue is still being drained, it'd be reasonable to "receive" a Permanently_Failed delivery report for each queued message.

(In reply to comment #5)
> The TpMessage API wasn't really designed with message splitting in mind...
> I think that'd work? I'll try to bear this in mind in any future work on
> TpMessage.

Or, file a bug? :)

Merged to master.

Will be in 0.1.7, for the record