[Bug 27175] Make TpMessage usable in clients

[bugzilla-daemon at freedesktop.org]

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

--- Comment #16 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-12-08 08:40:05 PST ---
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.)

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