Part of the "Get rid of TpAsv API" quest.
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!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.