Bug 28312

Patch from sjokkis (not reviewed): http://cgit.freedesktop.org/~sjokkis/telepathy-glib/log/?h=presence-changed

Quick review:

Use tp_value_array_unpack() to unpack the value array. Don't look up offsets into the array (I know that's what the old code does, but it's nasty).

+ if (contact->priv->presence_type != type) {
+ contact->priv->presence_type = type;
+ g_object_notify ((GObject *) contact, "presence-type");
+ }

Not in Telepathy style. Telepathy style is documented in http://telepathy.freedesktop.org/wiki/Style -- also applies to the other if blocks.

+ gchar *status = g_value_dup_string (presence->values+1);
+ if (tp_strdiff(contact->priv->presence_status, status)) {
+ g_free (contact->priv->presence_status);
+ contact->priv->presence_status = status;
+ g_object_notify ((GObject *) contact, "presence-status");
+ }
+ g_free(status);

Memory management is incorrect here. If you use tp_value_array_unpack() you'll be given a const char * that you don't need to free, so you can just do:

+ g_free (contact->priv->presence_status);
+ contact->priv->presence_status = g_strdup (status);

Created attachment 36205 [details] [review]
Adds TpContact::presence-changed signal

Changed the criticisms above. Should be fine now. Let me know if it isn't, and I'll get on it ASAP.

Review of attachment 36205 [details] [review]:

This mostly looks good; I'd like a few minor changes.

> From: mortenmj <morten.mjelva@gmail.com>

You might want to fix your git user.name setting and commit --amend this patch, before it becomes part of our revision history :-)

::: telepathy-glib/contact.c
@@ +873,3 @@
+   * TpContact::presence-changed:
+   * @contact: A #TpContact
+   * @presence: The contact's presence type 

"The new value of #TpContact:presence-type"

That way, the API user can follow the hyperlink to find out exactly how it works.

@@ +874,3 @@
+   * @contact: A #TpContact
+   * @presence: The contact's presence type 
+   * @status: Protocol-defined description of the presence type

The new value of #TpContact:presence-status

@@ +875,3 @@
+   * @presence: The contact's presence type 
+   * @status: Protocol-defined description of the presence type
+   * @status_message: User-defined status message

The new value of #TpContact:presence-message

@@ +877,3 @@
+   * @status_message: User-defined status message
+   *
+   * Emitted when PresenceChanged is received for this contact.

I'd prefer "Emitted when this contact's presence changes"; then it's meaningful even if you haven't read the Telepathy D-Bus API recently.

@@ +1662,2 @@
+// Presence properties might have changed. 
+// Set new values, and emit the TpContact::presence-changed signal.

/* */, please; we don't use C++ comments in Telepathy C code.

I don't think this comment actually adds much value, though, so I'd be inclined to just delete it.

@@ +1677,2 @@
+  tp_value_array_unpack (presence, presence->n_values, &type, &status,
+      &message);

Use a literal "3" for the second argument; it's used as a sanity check (to be able to warn/abort the number of arguments that follow it doesn't match presence->n_values), and using presence->n_values defeats that.

Created attachment 36227 [details] [review]
Make TpContact emit a presence-changed signal

Review of attachment 36227 [details] [review]:

::: telepathy-glib/contact.c
@@ +1678,1 @@
   g_object_notify ((GObject *) contact, "presence-type");

I have one remaining concern here, which is that this changes the behaviour of notify::presence-* - previously, all three properties were updated before emitting any of the notification signals (which seems most useful to me), but now, they're updated one at a time.

If you have time to correct this before I do a telepathy-glib 0.19.7 release today, great; if not, I'll probably apply your patch, make the change myself, and get someone else to review it.

Thanks for your patience!

Created attachment 36261 [details] [review]
Make TpContact emit a presence-changed signal

Reviewed and merged, thanks. Fixed in git, will be in 0.11.7 later today.

Reviewed and merged, thanks. Fixed in git, will be in 0.11.7 later today.