Bug 75581

Created attachment 94837 [details] [review]
cm-message: use tp_cm_message_get_sender() internally

Created attachment 94838 [details] [review]
use tp_message_dup_part() instead of tp_message_peek()

Created attachment 94839 [details] [review]
message: change string getter to _dup_

Created attachment 94840 [details] [review]
logger: use tp_message_get_pending_message_id()

Created attachment 94841 [details] [review]
logger: use tp_message_dup_part()

Created attachment 94842 [details] [review]
echo-message-parts: use tp_message_dup_part()

Created attachment 94843 [details] [review]
stop using tp_message_peek() in tests

Created attachment 94844 [details] [review]
stop using tp_message_peek() internally

Created attachment 94845 [details] [review]
remove tp_message_peek()

Comment on attachment 94838 [details] [review]
use tp_message_dup_part() instead of tp_message_peek()

Review of attachment 94838 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/cm-message.c
@@ +307,4 @@
>    g_return_val_if_fail (TP_IS_CM_MESSAGE (self), 0);
> +
> +  header = tp_message_dup_part (self, 0);
> +  g_variant_lookup (header, "message-sender", "u", &sender);

Either

if (!g_variant_lookup (...))
  sender = 0;

or pre-initialize it. Otherwise we'll return undefined stack contents if there is a message-sender but its type is wrong.

Comment on attachment 94839 [details] [review]
message: change string getter to _dup_

Review of attachment 94839 [details] [review]:
-----------------------------------------------------------------

::: telepathy-logger/text-channel.c
@@ +259,5 @@
>          tpl_entity_get_identifier (sender));
>  
>  
> +  token = tp_message_dup_token (message);
> +  supersedes = tp_message_dup_supersedes (message);

Inline get_message_edit_timestamp here, maybe? (Non-blocker)

@@ +277,5 @@
>        "message-type", type,
>        "message", text,
>        NULL);
>  
> +  g_free (token);

Is supersedes leaked?

Comment on attachment 94840 [details] [review]
logger: use tp_message_get_pending_message_id()

Review of attachment 94840 [details] [review]:
-----------------------------------------------------------------

Would be good to cherry-pick to logger master, maybe.

Comment on attachment 94842 [details] [review]
echo-message-parts: use tp_message_dup_part()

Review of attachment 94842 [details] [review]:
-----------------------------------------------------------------

::: examples/cm/echo-message-parts/chan.c
@@ +158,5 @@
>        if (s != NULL)
>          tp_message_set_string (received, j, "lang", s);
>  
> +      value = g_variant_dict_lookup_value (&dict, "content",
> +          G_VARIANT_TYPE_STRING);

Nitpicking: this doesn't copy binary content (it can either be an 's' or an 'ay'; iirc the latter was originally to support vCard over MMS, which sends a binary blob of unspecified encoding, containing embedded charset tags analogous to HTML <meta name="charset">).

I think that's fine for example code, though.

Comment on attachment 94843 [details] [review]
stop using tp_message_peek() in tests

Review of attachment 94843 [details] [review]:
-----------------------------------------------------------------

::: tests/dbus/cm-message.c
@@ +53,5 @@
>    GVariant *part_vardict;
>    gboolean valid;
>    const gchar *s;
>    gchar *token;
> +  guint u;

Should be specifically guint32, I think? (Although this is going to be the least of your worries if your platform has non-32-bit ints.)

(In reply to comment #12)
> Comment on attachment 94840 [details] [review] [review]
> logger: use tp_message_get_pending_message_id()
> 
> Review of attachment 94840 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Would be good to cherry-pick to logger master, maybe.

done.

Comment on attachment 94838 [details] [review]
use tp_message_dup_part() instead of tp_message_peek()

Review of attachment 94838 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/cm-message.c
@@ +307,4 @@
>    g_return_val_if_fail (TP_IS_CM_MESSAGE (self), 0);
> +
> +  header = tp_message_dup_part (self, 0);
> +  g_variant_lookup (header, "message-sender", "u", &sender);

Done.

Comment on attachment 94839 [details] [review]
message: change string getter to _dup_

Review of attachment 94839 [details] [review]:
-----------------------------------------------------------------

::: telepathy-logger/text-channel.c
@@ +259,5 @@
>          tpl_entity_get_identifier (sender));
>  
>  
> +  token = tp_message_dup_token (message);
> +  supersedes = tp_message_dup_supersedes (message);

done.

@@ +277,5 @@
>        "message-type", type,
>        "message", text,
>        NULL);
>  
> +  g_free (token);

fixed.

Comment on attachment 94843 [details] [review]
stop using tp_message_peek() in tests

Review of attachment 94843 [details] [review]:
-----------------------------------------------------------------

::: tests/dbus/cm-message.c
@@ +53,5 @@
>    GVariant *part_vardict;
>    gboolean valid;
>    const gchar *s;
>    gchar *token;
> +  guint u;

Fixed.

I pushed the !fixup to http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-message-75581

(In reply to comment #11)
> > +  token = tp_message_dup_token (message);
> > +  supersedes = tp_message_dup_supersedes (message);
> 
> Inline get_message_edit_timestamp here, maybe? (Non-blocker)

Sorry, I was unclear.

What I meant was that get_message_edit_timestamp() calls tp_message_dup_supersedes():

 get_message_edit_timestamp (TpMessage *message)
{
- if (tp_message_get_supersedes (message) != NULL)
- return get_network_timestamp (message);
+ gchar *supersedes;
+
+ supersedes = tp_message_dup_supersedes (message);
+ if (supersedes != NULL)
+ {
+ g_free (supersedes);
+ return get_network_timestamp (message);
+ }
else
- return 0;
+ {
+ return 0;
+ }
}

(sorry for the indentation damage, cgit is being invaluable)

and so you could save a malloc/free if you inlined get_message_edit_timestamp into its caller, which already needs to call tp_message_get_supersedes() anyway.

OTOH, if there are other callers, that's more annoying.

(In reply to comment #11)
> ::: telepathy-logger/text-channel.c
> @@ +277,5 @@
> > +  g_free (token);
> 
> Is supersedes leaked?

I don't see a fix for that?

(In reply to comment #20)
> I don't see a fix for that?

... because I wasn't looking hard enough. Yes, sorry, you did fix it.

(In reply to comment #19)
> (In reply to comment #11)
> > > +  token = tp_message_dup_token (message);
> > > +  supersedes = tp_message_dup_supersedes (message);
> > 
> > Inline get_message_edit_timestamp here, maybe? (Non-blocker)
> 
> Sorry, I was unclear.
> 
> What I meant was that get_message_edit_timestamp() calls
> tp_message_dup_supersedes():
> 
>  get_message_edit_timestamp (TpMessage *message)
> {
> - if (tp_message_get_supersedes (message) != NULL)
> - return get_network_timestamp (message);
> + gchar *supersedes;
> +
> + supersedes = tp_message_dup_supersedes (message);
> + if (supersedes != NULL)
> + {
> + g_free (supersedes);
> + return get_network_timestamp (message);
> + }
> else
> - return 0;
> + {
> + return 0;
> + }
> }
> 
> (sorry for the indentation damage, cgit is being invaluable)
> 
> and so you could save a malloc/free if you inlined
> get_message_edit_timestamp into its caller, which already needs to call
> tp_message_get_supersedes() anyway.
> 
> OTOH, if there are other callers, that's more annoying.

Done in the top commit of http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-message-75581

Great. Ship it!

Merged; thanks!