[Bug 33460] IM and MUC channels should use wocky_porter_send_async(), and emit failing delivery reports

[bugzilla-daemon at freedesktop.org]

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

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|review ?                    |review-

--- Comment #7 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2011-05-19 07:52:56 PDT ---
Leave it as-is now, but I explained in Comment #2 that this should have been a
sequence of four commits.

Message util stuff
==================

> - LmMessage *msg;
> - WockyNode *node;
> + const gchar *content_type, *text;
>   guint type = TP_CHANNEL_TEXT_MESSAGE_TYPE_NORMAL;
    gboolean result = TRUE;
> - const gchar *content_type, *text;
> - gchar *id = NULL;
>   guint n_parts;
> + WockyStanza *msg = NULL;
> + WockyNode *node;
> + gchar *id = NULL;

This seems to be re-ordering the local variables for no reason? You could have
just changed the type of msg. Not a problem now, but in future please don't
obscure the diff with random re-ordering.

> -#define INVALID_ARGUMENT(msg, ...) \
> +#define RETURN_INVALID_ARGUMENT(msg, ...) \

This didn't really need renaming; the new name is slightly clearer, perhaps,
but it's enlarging the diff for no particularly compelling reason. You could
have made this a separate (fifth) commit.

>  if (!result)
> -  INVALID_ARGUMENT ("message-type must be a 32-bit unsigned integer");
> +   RETURN_INVALID_ARGUMENT("message-type must be a 32-bit unsigned integer");

This changes a 2-space indent to 3-space, which is unwanted.

>    msg = lm_message_new_with_sub_type (recipient, LM_MESSAGE_TYPE_MESSAGE,
> -      subtype);
> +          subtype);

More unnecessary re-indentation. Please look at the diff yourself before
sending it for review, and consider whether the changes make sense?

> +    /* Generate a UUID for the message */
> +  id = gabble_generate_id ();
> +  lm_message_node_set_attribute (node, "id", id);
> +  tp_message_set_string (message, 0, "message-token", id);

This duplicates code that was already there: now you're setting the id twice in
the same function (and leaking the first one).

> + if (send_nick)
> + lm_message_node_add_own_nick (node, conn);
> - tp_message_mixin_sent (obj, message, flags, id, NULL);
> - g_free (id);
> + if (type == TP_CHANNEL_TEXT_MESSAGE_TYPE_ACTION)
> + {
> + gchar *tmp;
> + tmp = g_strconcat ("/me ", text, NULL);
> + lm_message_node_add_child (node, "body", tmp);
> + g_free (tmp);
> + }
> + else
> + {
> + lm_message_node_add_child (node, "body", text);
> + }

(Sorry for the indentation damage, copying from cgit seems to eat it.)

Every "+" line here is duplicated from code that already existed: now you're
adding the nick and the text twice.

> + gabble_message_util_add_chat_state (msg, state);
> + *token = id;

Either cope with token == NULL:

    if (token != NULL)
      *token = id;
    else
      g_free (id);

or specifically say in the doc-comment that it must not be NULL. "(out)"
parameters are conventionally allowed to be NULL.

IM channel
==========

> +typedef struct __GabbleIMSendMessageCtx
> +{
> + TpBaseChannel *channel;
> + TpMessage *message;
> + gchar *token;
> +} _GabbleIMSendMessageCtx;

You could just use: typedef struct { ... } GabbleIMSendMessageCtx;

> + GabbleIMChannel *chan = GABBLE_IM_CHANNEL(context->channel);

Make GabbleIMSendMessageCtx::channel be a GabbleIMChannel and you won't need
this cast, or indeed this variable?

> + _GabbleIMSendMessageCtx *context = (_GabbleIMSendMessageCtx *) user_data;

No need for the cast: C lets you assign a (void *) to any non-const pointer
type without a cast, and gpointer is just another name for void *. (C++ would
require the cast, but Gabble is C.)

> + _gabble_im_channel_state_receive (chan, TP_CHANNEL_CHAT_STATE_GONE);

... why does successfully sending a message imply that the remote contact has
GONE?! I don't think this is what you meant.

> + gabble_conn =
> + GABBLE_CONNECTION (tp_base_channel_get_connection (base));
> +
> +
> + msg = gabble_message_util_build_stanza (message,
> + gabble_conn, 0, state, priv->peer_jid,
> + priv->send_nick, &id, &error);
> +
> +
> + porter = gabble_connection_dup_porter (gabble_conn);
> +
> +
> + if (error == NULL)

No need to leave double blank lines here; single blank lines would be plenty.

> + g_object_unref (msg);

g_object_unref (NULL) is an error. Move this into the if (error == NULL) block. 

The "if (error == NULL)" block would conventionally be "if (msg != NULL)". The
function you're calling guarantees that one of these happens:

* msg is non-NULL, error is not set, id is set
* msg is NULL, error is set, id is left untouched

(If it doesn't guarantee that, it should - that's how GObject methods that can
raise an error conventionally work. From my reading of the actual function, I
think it's true here.)

You leak the porter. dup_porter means "give me an extra reference", so you're
responsible for unreffing it once.

> if (error == NULL)
>   {
...
>     context->token = g_strdup (id);
...
>   }
>
> g_free (id);

More efficient to use:

    context->token = id;

and not free it.

MUC channel
===========

Like the IM channel, you could just use: typedef struct { ... }
GabbleMUCSendMessageCtx;

Like the IM channel, I don't see why you set the chat state to GONE on success.

> + GabbleMucChannel *chan = GABBLE_MUC_CHANNEL(context->channel);

The type is already right, you don't need to do this checked cast.

> + gchar * id = NULL;

Conventionally whitespaced as "gchar *id"

> + context = g_slice_new0(_GabbleMUCSendMessageCtx);

Space between 0 and ( please

In the case where gabble_message_util_build_stanza succeeds, you leak msg. You
need to unref it on success.

Like the IM channel, you g_strdup (id) and then g_free it, where you could just
pass ownership.

Like the IM channel, you leak one ref to the porter. Unlike the IM channel, you
only call dup_porter if build_stanza succeeded, so you only need to do the
cleanup in that code path too.

If you renamed msg to stanza, the difference between message (a TpMessage
representing an abstract message) and msg (a WockyStanza representing an XMPP
XML stanza) would be clearer.

> + flags &= TP_MESSAGE_SENDING_FLAG_REPORT_DELIVERY;

I don't think this line has any practical effect? What did you expect it to do?

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