Bug 27175 (TpMessage-in-clients)

Things needing fixing:

- consider making it a GObject, with tp_message_destroy a deprecated alias for g_object_unref?

- make client-side received-message objects read-only? (we still need the set methods to work client-side while sending a message, though)

- make it use TpVariantMap internally (Bug #30423) and use the dbus-glib escape hatch to convert to/from dbus-glib GValues when necessary?

- add a tp_message_copy?

- handle refcounting works differently in the CM and in a client

- the client-side version probably wants a small amount of TpContact interaction?

- also deal with the case of a serialized/deserialized message (use GVariant), in which handles are no longer meaningful, but string IDs are still useful?

(In reply to comment #1)
> Things needing fixing:
> 
> - consider making it a GObject, with tp_message_destroy a deprecated alias for
> g_object_unref?

Makes sense.

> - make client-side received-message objects read-only? (we still need the set
> methods to work client-side while sending a message, though)

Should we use the same object or a new one sharing a common sub-class?

> - make it use TpVariantMap internally (Bug #30423) and use the dbus-glib escape
> hatch to convert to/from dbus-glib GValues when necessary?

I guess we need both variants, right?

> - add a tp_message_copy?

Not sure that's needed, we can just ref the object.

> - handle refcounting works differently in the CM and in a client

Yeah, which make me think it could be cleaner to have 2 objects.

> - the client-side version probably wants a small amount of TpContact
> interaction?

Probably yeah. 

> - also deal with the case of a serialized/deserialized message (use GVariant),
> in which handles are no longer meaningful, but string IDs are still useful?

Humm, I'm not sure to understand. Can you ellaborate?

(In reply to comment #1)
> - consider making it a GObject, with tp_message_destroy a deprecated alias for
> g_object_unref?

I started doing this in http://git.Collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/message-client-27175

> +      if (self->reffed_handles[i] != NULL)
> +        {
> +          tp_handle_set_destroy (self->reffed_handles[i]);
> +          self->reffed_handles[i] = NULL;
> +        }

tp_clear_pointer? :-)

Looks good so far apart from that.

(In reply to comment #2)
> > - make client-side received-message objects read-only? (we still need the set
> > methods to work client-side while sending a message, though)
> 
> Should we use the same object or a new one sharing a common sub-class?

A common superclass is probably the right answer. I'd like TpMessage to be the name of the superclass if we can get away with it.

> > - make it use TpVariantMap internally (Bug #30423) and use the dbus-glib escape
> > hatch to convert to/from dbus-glib GValues when necessary?
> 
> I guess we need both variants, right?

I think we can have one as the canonical representation, and construct the other on-demand. tp_message_peek() is the only thing that strictly requires the dbus-glib representation, until we send the message.

> > - add a tp_message_copy?
> 
> Not sure that's needed, we can just ref the object.

It's mutable, though, so we want to be able to deep-copy it too. Perhaps tp_message_deep_copy()?

> > - handle refcounting works differently in the CM and in a client
> 
> Yeah, which make me think it could be cleaner to have 2 objects.

It's a sad time that the TpMessage name is already in use...

> > - also deal with the case of a serialized/deserialized message (use GVariant),
> > in which handles are no longer meaningful, but string IDs are still useful?
> 
> Humm, I'm not sure to understand. Can you ellaborate?

TpMessage is essentially an aa{sv} plus a set of reffed handles.

There are three uses for a TpMessage:

- In a CM. We can ref handles of any type, synchronously. Handle fields like parts[0]['message-sender'] are essential.

- In a client dealing with a live Channel. Handles are reffed for as long as the message is unacknowledged, so we need to make sure we have a ref before acknowledging. telepathy-glib only makes this easy for contact handles. TpTextChannel can't do this generically, because it doesn't know which uint32 keys might be a handle, and if so, which type they are.

- Serialize the aa{sv}, put it in telepathy-logger, deserialize it and use it again later. Handle fields like parts[0]['message-sender'] are completely meaningless, but if we had a message-sender-id field, that'd still be useful.

In Tpl the sender ought to be denormalized into a separate database column anyway, but we'd have the same problem with any extension field that was a handle.

Perhaps the solution is to say that message-sender is the only handle that will ever appear in a Message_Part, and deprecate tp_message_set_handle() in favour of a new tp_cm_message_set_sender()?

(In reply to comment #4)
> > +      if (self->reffed_handles[i] != NULL)
> > +        {
> > +          tp_handle_set_destroy (self->reffed_handles[i]);
> > +          self->reffed_handles[i] = NULL;
> > +        }
> 
> tp_clear_pointer? :-)
> 
> Looks good so far apart from that.

fixed.



> (In reply to comment #2)
> > > - make client-side received-message objects read-only? (we still need the set
> > > methods to work client-side while sending a message, though)
> > 
> > Should we use the same object or a new one sharing a common sub-class?
> 
> A common superclass is probably the right answer. I'd like TpMessage to be the
> name of the superclass if we can get away with it.

Most of the tp_message_* API makes sense as a base class except:
tp_message_new() : as it takes a TpBaseConnection
tp_message_take_message() ? not sure.

> > > - make it use TpVariantMap internally (Bug #30423) and use the dbus-glib escape
> > > hatch to convert to/from dbus-glib GValues when necessary?
> > 
> > I guess we need both variants, right?
> 
> I think we can have one as the canonical representation, and construct the
> other on-demand. tp_message_peek() is the only thing that strictly requires the
> dbus-glib representation, until we send the message.

Right, so we can add a variant of _peek() using variants.

> > > - add a tp_message_copy?
> > 
> > Not sure that's needed, we can just ref the object.
> 
> It's mutable, though, so we want to be able to deep-copy it too. Perhaps
> tp_message_deep_copy()?

Ok. I think _copy() is clear enough.

> > > - handle refcounting works differently in the CM and in a client
> > 
> > Yeah, which make me think it could be cleaner to have 2 objects.
> 
> It's a sad time that the TpMessage name is already in use...

What should we do?

Make TpMessage the base class and have TpCMMessage (or TpSvcMessage?) and TpClientMessage (or TpCliMessage?) ? Then we have to document than the methods above can only be used with CM messages but are named that way for historical reason.

Or we can add TpBaseMessage and keep TpMessage as it but we'll have to duplicate most of the API of tp_message_* in tp_base_message_*.

(In reply to comment #5)
> Most of the tp_message_* API makes sense as a base class except:
> tp_message_new() : as it takes a TpBaseConnection

Yeah, we should add tp_cm_message_new() or something, and eventually deprecate tp_message_new().

> tp_message_take_message() ? not sure.

I don't think clients will ever use it, but it's relatively harmless.

One subtlety is that it needs to check that the taken message has the same handle-reffing semantics as the parent message.

> > > > - handle refcounting works differently in the CM and in a client
> > > 
> > > Yeah, which make me think it could be cleaner to have 2 objects.
> > 
> > It's a sad time that the TpMessage name is already in use...
> 
> What should we do?
> 
> Make TpMessage the base class and have TpCMMessage (or TpSvcMessage?) and
> TpClientMessage (or TpCliMessage?) ? Then we have to document than the methods
> above can only be used with CM messages but are named that way for historical
> reason.

I think this is the way forward. It'd be extremely confusing if there's a structure called TpMessage that isn't the right thing for clients to use.

I think I'd call them:

TpMessage
  |
  \--- TpCMMessage (used by TpMessageMixin)
  \--- TpClientMessage (if needed, or just use the base class?)
         \--- TpSignalledMessage
              (or TpReceivedMessage, but it's used for MessageSent too)
              (has a TpContact for the sender if possible)
  \--- TpSavedMessage (the handle-less version, for the logger)

Let's reserve the tp_cli, tp_svc naming for the auto-generated stuff.

I think there should be an absolute minimum of "type-specific" API; in particular, the constructors should all return a TpMessage *.

I'm not sure whether methods like get_sender_contact should take a TpMessage * or a TpClientMessage *, and whether they should be tp_message_get_sender() or tp_client_message_get_sender().

(In reply to comment #4)
> There are three uses for a TpMessage:
[...]
> - In a CM. We can ref handles of any type, synchronously. Handle fields like
> parts[0]['message-sender'] are essential.

This breaks down into:

- Received message: the CM builds up a Message based on protocol information, then passes it to the mixin to put it on the "pending messages" queue. The mixin needs to stamp it with a pending message ID, so it must either be mutable, or be moderately cheap to copy into a mutable form.

[This is TpCMMessage.]

- Sent message: telepathy-glib gets an aa{sv} from D-Bus, builds a Message, and gives it to the CM. The CM must give the mixin back a message that corresponds to what it actually sent, which may differ (e.g. if it got truncated, the protocol always sends a particular Unicode normalization form, the protocol can only send latin-1, etc.), so the CM needs to modify or copy+modify it.

[This is also TpCMMessage. I don't think we need a separate object.]

> - In a client dealing with a live Channel. Handles are reffed for as long as
> the message is unacknowledged, so we need to make sure we have a ref before
> acknowledging. telepathy-glib only makes this easy for contact handles.
> TpTextChannel can't do this generically, because it doesn't know which uint32
> keys might be a handle, and if so, which type they are.

This breaks down into:

- MessageReceived or MessageSent signal from D-Bus: the Channel gets an aa{sv} from D-Bus, refs all applicable handles by building Contact objects, builds a Message and passes it up to the UI. Ideally, I'd also like to define a message-sender-id field, and copy the message-sender's ID to that field. The constructor can be behind-the-scenes: I'd suggest making TpMessage and its subclasses non-subclassable outside telepathy-glib, like TpContact.

[This is provisionally called TpSignalledMessage above.]

- Sending a message: the client has to construct a Message from scratch, and pass it to a method. It doesn't need to do any handle refcounting at the moment, because the sender is implicitly "me" and you're not allowed to set it explicitly. If we forbid further handle-based Message fields, we'll be able to avoid handle refcounting altogether, but perhaps the constructor ought to take a TpConnection or even a TpChannel anyway.

[This is provisionally called TpClientMessage above, or perhaps the TpMessage base class is enough.]

> Perhaps the solution is to say that message-sender is the only handle that will
> ever appear in a Message_Part, and deprecate tp_message_set_handle() in favour
> of a new tp_cm_message_set_sender()?

I'm very tempted to do this. Any thoughts?

(In reply to comment #6)
> (In reply to comment #5)
> > Most of the tp_message_* API makes sense as a base class except:
> > tp_message_new() : as it takes a TpBaseConnection
> 
> Yeah, we should add tp_cm_message_new() or something, and eventually deprecate
> tp_message_new().
> 
> > tp_message_take_message() ? not sure.
> 
> I don't think clients will ever use it, but it's relatively harmless.

I moved it to TpCMMessage for now.

> > > > > - handle refcounting works differently in the CM and in a client
> > > > 
> > > > Yeah, which make me think it could be cleaner to have 2 objects.
> > > 
> > > It's a sad time that the TpMessage name is already in use...
> > 
> > What should we do?
> > 
> > Make TpMessage the base class and have TpCMMessage (or TpSvcMessage?) and
> > TpClientMessage (or TpCliMessage?) ? Then we have to document than the methods
> > above can only be used with CM messages but are named that way for historical
> > reason.
> 
> I think this is the way forward. It'd be extremely confusing if there's a
> structure called TpMessage that isn't the right thing for clients to use.
> 
> I think I'd call them:
> 
> TpMessage
>   |
>   \--- TpCMMessage (used by TpMessageMixin)
>   \--- TpClientMessage (if needed, or just use the base class?)
>          \--- TpSignalledMessage
>               (or TpReceivedMessage, but it's used for MessageSent too)
>               (has a TpContact for the sender if possible)
>   \--- TpSavedMessage (the handle-less version, for the logger)
> 
> Let's reserve the tp_cli, tp_svc naming for the auto-generated stuff.

I have added TpCMMessage, deprecated the "wrong" TpMessage API and added TPClientMessage (and empty shell atm, I'll rebase my text-channel branch on top of this one and will continue to work on it to have a clearer idea of its API).


> > Perhaps the solution is to say that message-sender is the only handle that will
> > ever appear in a Message_Part, and deprecate tp_message_set_handle() in favour
> > of a new tp_cm_message_set_sender()?
> 
> I'm very tempted to do this. Any thoughts?

Makes sense; I've done that.

So I created TpClientMessage and started to use it:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/text-channel-29531

Could you take a quick look and check if I'm going to right direction?

So far I didn't had the need to have this object (TpMessage is enough) but I'd
say having a new class is cleaner and more future proof.

I didn't make the initial_parts / size_hint distinction in
tp_message_client_new() as I'm not sure what it really buys us.

Also, I guess we could have a tp_message_client_new_text
(TpChannelTextMessageType type, const ghcar *text); for the most common case?

I'm also not sure how we should deal with delivery reporting/failure. See the
FIXME in send_message_cb

(In reply to comment #8)
> Also, I guess we could have a tp_message_client_new_text
> (TpChannelTextMessageType type, const ghcar *text); for the most common case?

Yes! I like this idea.

(In reply to comment #9)
> (In reply to comment #8)
> > Also, I guess we could have a tp_message_client_new_text
> > (TpChannelTextMessageType type, const ghcar *text); for the most common case?
> 
> Yes! I like this idea.

added!

Drive-by comments rather than a thorough review:

Yes this seems to be on the right track.

In message.h:

> +#include <telepathy-glib/base-connection.h>

I'd prefer it if this (and the method that needs it) only appeared in cm-message.h (which would be the one included by message-mixin.h).

> +  /* Now that @self has stolen @message's parts, replace them with a stub to
> +   * keep tp_message_destroy happy.

This is, er, suspicious, now that messages are refcounted... could we use TpVariantMap's cheap copy-on-write mechanism (Bug #30423) instead?

(That'd mean the internals of a TpMessage being GPtrArray<TpVariantMap>, and mapping to and from dbus-glib whenever we touch D-Bus, in the medium term. I think that'd be worthwhile, but I can see that it's not on the shortest path for this bug.)

> +# Header files to ignore when scasignalled

o rly? :-)

(In reply to comment #8)
> I didn't make the initial_parts / size_hint distinction in
> tp_message_client_new() as I'm not sure what it really buys us.

That's fair enough; size-hint was just a premature optimization anyway.

I'm not sure whether initial-parts really makes sense as a construct-time property either; callers (in e.g. language bindings) can always just call tp_message_append_part()?

More specifically, tp_message_new (initial_parts=n) is exactly equivalent to creating a new TpMessage with only the required headers part, then calling tp_message_append_part() n-1 times; so I'd be inclined to say that it's OK to have a _new parameter that isn't a construct-time property, just as syntactic sugar similar to tp_client_message_text_new().

tp_client_message_text_new() should be renamed to tp_client_message_new_text(), because it's conceptually a secondary constructor "ClientMessage.new_text()" in the GObject object model, and GObject constructors start with "new".

(In reply to comment #11)
> In message.h:
> 
> > +#include <telepathy-glib/base-connection.h>
> 
> I'd prefer it if this (and the method that needs it) only appeared in
> cm-message.h (which would be the one included by message-mixin.h).

done.

> > +  /* Now that @self has stolen @message's parts, replace them with a stub to
> > +   * keep tp_message_destroy happy.
> 
> This is, er, suspicious, now that messages are refcounted... could we use
> TpVariantMap's cheap copy-on-write mechanism (Bug #30423) instead?
> 
> (That'd mean the internals of a TpMessage being GPtrArray<TpVariantMap>, and
> mapping to and from dbus-glib whenever we touch D-Bus, in the medium term. I
> think that'd be worthwhile, but I can see that it's not on the shortest path
> for this bug.)

TODO : I'll do that once TpVariantMap has been merged.

> > +# Header files to ignore when scasignalled
> 
> o rly? :-)

Oups; fixed. :)

> (In reply to comment #8)
> > I didn't make the initial_parts / size_hint distinction in
> > tp_message_client_new() as I'm not sure what it really buys us.
> 
> That's fair enough; size-hint was just a premature optimization anyway.
> 
> I'm not sure whether initial-parts really makes sense as a construct-time
> property either; callers (in e.g. language bindings) can always just call
> tp_message_append_part()?
> 
> More specifically, tp_message_new (initial_parts=n) is exactly equivalent to
> creating a new TpMessage with only the required headers part, then calling
> tp_message_append_part() n-1 times; so I'd be inclined to say that it's OK to
> have a _new parameter that isn't a construct-time property, just as syntactic
> sugar similar to tp_client_message_text_new().

Indeed it's not that useful, I just removed the argument for now.

> tp_client_message_text_new() should be renamed to tp_client_message_new_text(),
> because it's conceptually a secondary constructor "ClientMessage.new_text()" in
> the GObject object model, and GObject constructors start with "new".

renamed.

(Reviewing this as something distinct from, and blocking, Bug #29531.)

+  //TpCMMessage *self = TP_CM_MESSAGE (object);

Fixed later, but for reference: no C99 comments please. I'd prefer

    (void) self;

if you really must have a stub dispose().

I think message-mixin.h should probably #include <telepathy-glib/cm-message.h>, which would mean echo-message-parts' chan.c wouldn't have to.

telepathy-glib.h should #include <telepathy-glib/signalled-message.h>.

Deprecated functions should be G_GNUC_DEPRECATED.

>  static void
> -tp_message_ref_handles (TpMessage *self,
> +tp_message_ref_handles (TpMessage *msg,

Not API, so it could be tp_cm_message_ref_handles if that'd be any easier.

> +  tp_cm_message_set_handle (self, 0, "message-sender", TP_HANDLE_TYPE_CONTACT,
> +      handle);

Assuming the relevant commit from Bug #26838 is merged (opinions welcome btw), this method should add { "message-sender-id": inspected identifier } too.

> +  if (handle_or_0 != 0)
> +    tp_message_ref_handle (self, handle_type, handle_or_0);

This should call the non-deprecated one (tp_cm_message_ref_handle) if possible.

>   * #TpCMMessage represent a message send or received using the Message
>   * interface.

I'd prefer: #TpCMMessage is used within connection managers to represent a message sent or received using the Messages interface.

> + * #TpClientMessage represent a message send using the Message interface.

I'd prefer: #TpClientMessage is used within Telepathy clients to represent a message composed by a client, which it will send using the Messages interface. Its subclass #TpSignalledMessage represents messages as signalled by a connection manager.

> + * #TpSignalledMessage represent a message received using the Message interface.

#TpSignalledMessage is used within Telepathy clients to represent a message signalled by a connection manager. This can either be a message received from someone else, confirmation that a message has been sent by the local user, or a delivery report indicating that delivery of a message has succeeded or failed.

> +          echo_msg = _tp_signalled_message_new (echo);

This seems like an abuse of TpSignalledMessage - this is within a CM, so the refcounting will be all wrong.

> +gchar * tp_message_to_text (TpMessage *message,
> +    TpChannelTextMessageFlags *out_flags);

This should have G_GNUC_WARN_UNUSED_RESULT.

It seems as though it'd be cleaner to have TpSignalledMessage take the sending TpContact as a construct-time parameter, rather than sellotaping it on afterwards?

I don't think initial-parts should be a property: I think it's fair to say that if you call g_object_new() or equivalent instead of using the tp_foo_message_new "C binding", you get one part, and have to append to get more (which is very easy, particularly in high-level languages).

I certainly don't think size-hint (which is a probably-premature optimization) deserves to be a property.

> - make client-side received-message objects read-only? (we still need
> the set methods to work client-side while sending a message, though)

That'd mean having a "mutable" flag (property? class member?) which is true for TpClientMessage and TpCMMessage but false for TpSignalledMessage.

(In reply to comment #14)
> I think message-mixin.h should probably #include <telepathy-glib/cm-message.h>,
> which would mean echo-message-parts' chan.c wouldn't have to.

done.

> telepathy-glib.h should #include <telepathy-glib/signalled-message.h>.

It already does.

> Deprecated functions should be G_GNUC_DEPRECATED.

done.

> >  static void
> > -tp_message_ref_handles (TpMessage *self,
> > +tp_message_ref_handles (TpMessage *msg,
> 
> Not API, so it could be tp_cm_message_ref_handles if that'd be any easier.

renamed.

> > +  tp_cm_message_set_handle (self, 0, "message-sender", TP_HANDLE_TYPE_CONTACT,
> > +      handle);
> 
> Assuming the relevant commit from Bug #26838 is merged (opinions welcome btw),
> this method should add { "message-sender-id": inspected identifier } too.

done.

> > +  if (handle_or_0 != 0)
> > +    tp_message_ref_handle (self, handle_type, handle_or_0);
> 
> This should call the non-deprecated one (tp_cm_message_ref_handle) if possible.

done (as part of the deprecation of the old API)

> >   * #TpCMMessage represent a message send or received using the Message
> >   * interface.
> 
> I'd prefer: #TpCMMessage is used within connection managers to represent a
> message sent or received using the Messages interface.

changed.

> > + * #TpClientMessage represent a message send using the Message interface.
> 
> I'd prefer: #TpClientMessage is used within Telepathy clients to represent a
> message composed by a client, which it will send using the Messages interface.
> Its subclass #TpSignalledMessage represents messages as signalled by a
> connection manager.

changed.

> > + * #TpSignalledMessage represent a message received using the Message interface.
> 
> #TpSignalledMessage is used within Telepathy clients to represent a message
> signalled by a connection manager. This can either be a message received from
> someone else, confirmation that a message has been sent by the local user, or a
> delivery report indicating that delivery of a message has succeeded or failed.

changed.

> > +          echo_msg = _tp_signalled_message_new (echo);
> 
> This seems like an abuse of TpSignalledMessage - this is within a CM, so the
> refcounting will be all wrong.

Done (using a new _tp_cm_message_new_from_parts())

> > +gchar * tp_message_to_text (TpMessage *message,
> > +    TpChannelTextMessageFlags *out_flags);
> 
> This should have G_GNUC_WARN_UNUSED_RESULT.

done.

> It seems as though it'd be cleaner to have TpSignalledMessage take the sending
> TpContact as a construct-time parameter, rather than sellotaping it on
> afterwards?

done.

> I don't think initial-parts should be a property: I think it's fair to say that
> if you call g_object_new() or equivalent instead of using the
> tp_foo_message_new "C binding", you get one part, and have to append to get
> more (which is very easy, particularly in high-level languages).
> 
> I certainly don't think size-hint (which is a probably-premature optimization)
> deserves to be a property.

removed. tp_cm_message_new() now completely ignores size_hint. To make use of
it we should destroy parts and recrete the ptr array. Isn't that a bit
overkill for such trivial optimization? If not, we could remove size_hint
completely from tp_cm_message_new().

> > - make client-side received-message objects read-only? (we still need
> > the set methods to work client-side while sending a message, though)
> 
> That'd mean having a "mutable" flag (property? class member?) which is true for
> TpClientMessage and TpCMMessage but false for TpSignalledMessage.

done.

tp_cm_message_set_sender() should probably document that it sets both "message-sender" and "message-sender-id": something like

    Set the sender of @self, i.e. the "message-sender" and
    "message-sender-id" keys in the header.

> + * tp_client_message_new_text:
> ...
> + * 'text/plain' as 'content-typee', @type as 'message-type' and

"content-type" (no extra e)

> + * tp_client_message_new:
> ...
> + * Returns: (transfer full): a newly allocated #TpClientMessage

I think it's worth saying that the new message has part 0, the headers part, only

_tp_cm_message_new_from_parts should take a TpBaseConnection, so it can ensure handles if necessary. Either one of tp_cm_message_new() and _tp_cm_message_new_from_parts() should call the other, or there should be a "common ancestor" that they both call.

As a result of that omission, calling _tp_cm_message_new_from_parts() will currently assert or segfault when it tries to ref the sender, as far as I can see. Calling tp_cm_message_set_sender would certainly fail. I'd like to see a regression test for that, to prove that you've fixed it :-) One way to do that would be for the test to link the convenience library statically to access the internal API, like tests/test-capabilities does.

Now that handles are immortal, we could remove tp_cm_message_ref_handles(), tp_cm_message_ref_handle() and the array of handle sets, and make the wrappers in TpMessage either do nothing, or just do some g_return_if_fail() for sanity checking?

tp_cm_message_new() shouldn't have the useless size_hint parameter. tp_message_new needs to keep it for ABI reasons, but can just ignore it (to help people stay backwards compatible, it might be worth moving the "size_hint >= initial_parts" assertion into tp_message_new though).

I'd rather not officially deprecate tp_message_new(), tp_message_ref_handle(), tp_message_set_handle(), tp_message_take_message() until the replacements have been in a release and a decent number of CMs (at least the usual suspects - Gabble, Haze, Salut, Idle, tpsip) have been ported.

There should be a tp_message_is_immutable() (or perhaps tp_message_is_mutable()) if you want to assert about it.

_tp_message_immutable() should be a verb (_tp_message_set_immutable() or _tp_message_become_immutable() or something), and should set mutable to FALSE, not TRUE as it currently does :-P

I think I'd like some return_if_fails in _tp_signalled_message_new:

- if message-sender is nonzero, then the TpContact is non-NULL and has
  that handle
- if message-sender-id is nonempty, then the TpContact is non-NULL and has
  that identifier

It'd also be nice to insert message-sender-id if missing.

+  /* FIXME: remove message-sender? */

Yes, I think that'd be worth doing just before making the message immutable, with a comment explaining that it's not persistent, and you can get the non-persistent handle from the TpContact instead.

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

Nitpicking:

tp_client_message_dispose() doesn't do anything; remove it. It also has a // comment which isn't ISO C89, and we generally avoid relying on C99.

Either the first thing cm-message-internal.h does should be to #include <telepathy-glib/cm-message.h>, or the first thing the .c does should be to #include the *public* .h to verify its self-containedness; preferably both. The same applies to message-internal.h, message.h, message.c (but signalled-message looks OK).

The inclusion of base-connection.h and telepathy-glib/handle-repo.h in cm-message-internal.h looks redundant, and could probably be moved to the .c.

The "from here down is implementation-specific" part of TpMessage could usefully move into TpCMMessage?

message-mixin.c shouldn't need to include signalled-message-internal.h, which is only for the client-side.

tp_cm_message_new(), tp_message_init() and tp_cm_message_take_message() create hash tables for parts "manually". Couldn't they call tp_message_append_part()? (In the case of tp_message_init() you'd have to move the mutable = TRUE up a bit, but that's fine.)

(In reply to comment #16)
> I think I'd like some return_if_fails in _tp_signalled_message_new:
> 
> - if message-sender is nonzero, then the TpContact is non-NULL and has
>   that handle
> - if message-sender-id is nonempty, then the TpContact is non-NULL and has
>   that identifier

Also, if message-sender is absent or 0, the message-sender-id must be absent or empty, and the TpContact must be NULL.

(TpTextChannel should take care of issuing warnings / deleting inconsistent information before making the TpSignalledMessage, if the CM gives us a message where message-sender and message-sender-id aren't consistent with each other. tp_connection_dup_contact_for_immortal_handle already raises a GError if it detects inconsistency.)

I rebased the branch, the last commit your reviewed was "TpMessage: add
mutable check".

(In reply to comment #16)
> tp_cm_message_set_sender() should probably document that it sets both
> "message-sender" and "message-sender-id": something like
> 
>     Set the sender of @self, i.e. the "message-sender" and
>     "message-sender-id" keys in the header.

done.

> > + * tp_client_message_new_text:
> > ...
> > + * 'text/plain' as 'content-typee', @type as 'message-type' and
> 
> "content-type" (no extra e)

fixed.

> > + * tp_client_message_new:
> > ...
> > + * Returns: (transfer full): a newly allocated #TpClientMessage
> 
> I think it's worth saying that the new message has part 0, the headers part,
> only

done.

> _tp_cm_message_new_from_parts should take a TpBaseConnection, so it can ensure
> handles if necessary.

done.


> Either one of tp_cm_message_new() and
> _tp_cm_message_new_from_parts() should call the other, or there should be a
> "common ancestor" that they both call.

done.

> As a result of that omission, calling _tp_cm_message_new_from_parts() will
> currently assert or segfault when it tries to ref the sender, as far as I can
> see. Calling tp_cm_message_set_sender would certainly fail. I'd like to see a
> regression test for that, to prove that you've fixed it :-) One way to do that
> would be for the test to link the convenience library statically to access the
> internal API, like tests/test-capabilities does.

Oooops, nice catch. I fixed it and added a test.

> Now that handles are immortal, we could remove tp_cm_message_ref_handles(),
> tp_cm_message_ref_handle() and the array of handle sets, and make the wrappers
> in TpMessage either do nothing, or just do some g_return_if_fail() for sanity
> checking?

removed.

> tp_cm_message_new() shouldn't have the useless size_hint parameter.
> tp_message_new needs to keep it for ABI reasons, but can just ignore it (to
> help people stay backwards compatible, it might be worth moving the "size_hint
> >= initial_parts" assertion into tp_message_new though).

done.

> I'd rather not officially deprecate tp_message_new(), tp_message_ref_handle(),
> tp_message_set_handle(), tp_message_take_message() until the replacements have
> been in a release and a decent number of CMs (at least the usual suspects -
> Gabble, Haze, Salut, Idle, tpsip) have been ported.

Why? Aren't deprecate optionnal? We can still use them if needed.

> There should be a tp_message_is_immutable() (or perhaps
> tp_message_is_mutable()) if you want to assert about it.

I added tp_message_is_mutable().

> _tp_message_immutable() should be a verb (_tp_message_set_immutable() or
> _tp_message_become_immutable() or something), and should set mutable to FALSE,
> not TRUE as it currently does :-P

Fixed.

> I think I'd like some return_if_fails in _tp_signalled_message_new:
> 
> - if message-sender is nonzero, then the TpContact is non-NULL and has
>   that handle
> - if message-sender-id is nonempty, then the TpContact is non-NULL and has
>   that identifier

done.

> It'd also be nice to insert message-sender-id if missing.

good idea; done.

> +  /* FIXME: remove message-sender? */
> 
> Yes, I think that'd be worth doing just before making the message immutable,
> with a comment explaining that it's not persistent, and you can get the
> non-persistent handle from the TpContact instead.

done.

> tp_client_message_dispose() doesn't do anything; remove it. It also has a //
> comment which isn't ISO C89, and we generally avoid relying on C99.

Right; I removed it.

> Either the first thing cm-message-internal.h does should be to #include
> <telepathy-glib/cm-message.h>, or the first thing the .c does should be to
> #include the *public* .h to verify its self-containedness; preferably both. The
> same applies to message-internal.h, message.h, message.c (but signalled-message
> looks OK).

I tried to do that but hit weird compilation error as said on IRC. I didn't
manage to reproduce them in a simple case.

> The inclusion of base-connection.h and telepathy-glib/handle-repo.h in
> cm-message-internal.h looks redundant, and could probably be moved to the .c.

removed.

> The "from here down is implementation-specific" part of TpMessage could
> usefully move into TpCMMessage?

TODO

> message-mixin.c shouldn't need to include signalled-message-internal.h, which
> is only for the client-side.

removed.

> tp_cm_message_new(), tp_message_init() and tp_cm_message_take_message() create
> hash tables for parts "manually". Couldn't they call tp_message_append_part()?
> (In the case of tp_message_init() you'd have to move the mutable = TRUE up a
> bit, but that's fine.)

done.


(In reply to comment #17)
> (In reply to comment #16)
> > I think I'd like some return_if_fails in _tp_signalled_message_new:
> > 
> > - if message-sender is nonzero, then the TpContact is non-NULL and has
> >   that handle
> > - if message-sender-id is nonempty, then the TpContact is non-NULL and has
> >   that identifier
> 
> Also, if message-sender is absent or 0, the message-sender-id must be absent or
> empty, and the TpContact must be NULL.

done.

+ * @deprecated since 0.13.UNRELEASED. Handles are now immortables so there is
...
+ /* Handles are now immortables so we don't have to anything */

typo: immortal

+ * no point to ref them. Furthermore, the only handles that should be stored

grammar: the only handle that should (because you refer to it as singular afterwards)

(In reply to comment #18)
> > I'd rather not officially deprecate tp_message_new(), tp_message_ref_handle(),
> > tp_message_set_handle(), tp_message_take_message() until the replacements have
> > been in a release and a decent number of CMs (at least the usual suspects -
> > Gabble, Haze, Salut, Idle, tpsip) have been ported.
> 
> Why? Aren't deprecate optionnal? We can still use them if needed.

Most of our CMs normally compile with -Wdeprecated-declarations -Werror, so insta-deprecating things seems a little unfriendly to CM developers :-)

Since the migration path is so trivial in this case, perhaps it's not a problem.

> > Either the first thing cm-message-internal.h does should be to #include
> > <telepathy-glib/cm-message.h>, or the first thing the .c does should be to
> > #include the *public* .h to verify its self-containedness; preferably both. The
> > same applies to message-internal.h, message.h, message.c (but signalled-message
> > looks OK).
> 
> I tried to do that but hit weird compilation error as said on IRC. I didn't
> manage to reproduce them in a simple case.

That's a bit worrying, because it means one of our headers isn't properly self-contained. If you compile this:

    #include <telepathy-glib/cm-message.h>
    int main (void) { return 0; }

does it work? Likewise for message.h?

Other than that, this looks good.

(In reply to comment #19)
> + * @deprecated since 0.13.UNRELEASED. Handles are now immortables so there is
> ...
> + /* Handles are now immortables so we don't have to anything */
> 
> typo: immortal

fixed.

> + * no point to ref them. Furthermore, the only handles that should be stored
> 
> grammar: the only handle that should (because you refer to it as singular
> afterwards)

fixed.

> (In reply to comment #18)
> > > I'd rather not officially deprecate tp_message_new(), tp_message_ref_handle(),
> > > tp_message_set_handle(), tp_message_take_message() until the replacements have
> > > been in a release and a decent number of CMs (at least the usual suspects -
> > > Gabble, Haze, Salut, Idle, tpsip) have been ported.
> > 
> > Why? Aren't deprecate optionnal? We can still use them if needed.
> 
> Most of our CMs normally compile with -Wdeprecated-declarations -Werror, so
> insta-deprecating things seems a little unfriendly to CM developers :-)
> 
> Since the migration path is so trivial in this case, perhaps it's not a
> problem.

I don't think it is, best to change it ASAP.
GLib/GTK+ are not so friendly when depreacting API and people live with it. :)

> > > Either the first thing cm-message-internal.h does should be to #include
> > > <telepathy-glib/cm-message.h>, or the first thing the .c does should be to
> > > #include the *public* .h to verify its self-containedness; preferably both. The
> > > same applies to message-internal.h, message.h, message.c (but signalled-message
> > > looks OK).
> > 
> > I tried to do that but hit weird compilation error as said on IRC. I didn't
> > manage to reproduce them in a simple case.
> 
> That's a bit worrying, because it means one of our headers isn't properly
> self-contained. If you compile this:
> 
>     #include <telepathy-glib/cm-message.h>
>     int main (void) { return 0; }
> 
> does it work? Likewise for message.h?

I didn't. I moved includes out of G_BEGIN_DECLS and it's fine now.

Ship it!

Merged to master; will be in 0.13.9

For the record, I opened bug #36354 about TpSavedMessage