[Bug 27175] Make TpMessage usable in clients

[bugzilla-daemon at freedesktop.org]

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

--- Comment #18 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-12-09 06:21:00 PST ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.