[Bug 24107] support Statuses on TpConnection

[bugzilla-daemon at freedesktop.org]

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

--- Comment #9 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-14 04:57:49 PDT ---
Review of attachment 35499:
 --> (https://bugs.freedesktop.org/review?bug=24107&attachment=35499)

I don't see a regression test here? :-)

Unfortunately the only example/test CM we have that implements this is
examples/cm/contactlist, which has an extremely simple concept of presence (a
boolean "away?" flag), but you could enhance that to store and re-broadcast the
message?

(See tests/dbus/callable.c, and its piece of Makefile.am, if you need an
example of how a test can link against an example CM.)

::: telepathy-glib/connection.c
@@ +61,3 @@
  * <listitem>connection status tracking</listitem>
  * <listitem>calling GetInterfaces() automatically</listitem>
+ * <listitem>Getting the valid presence statuses automatically</listitem>

I don't think this is really appropriate in a list of things done
automatically, given that you have to opt in. Just remove it.

We should re-do this introduction to reflect the current recommendation to do
everything possible through high-level API and de-emphasis of generated API,
but that's a job for another branch.

@@ +2465,3 @@
+  self = g_slice_new0 (TpStatusSpec);
+  self->type = type;
+  self->status = g_strdup (status);

I'd like a g_return_if_fail that @status is non-NULL, I think.

@@ +2518,3 @@
+ * The value may have changed arbitrarily during the time the Connection
+ * spends in status %TP_CONNECTION_STATUS_CONNECTING, again staying fixed for
+ * the entire time in %TP_CONNECTION_STATUS_CONNECTED.

You don't need this paragraph if you refuse to retrieve Statuses before going
CONNECTED, as you currently do.

(But, don't do that... see elsewhere.)

@@ +2537,3 @@
+    g_hash_table_ref (self->priv->allowed_presence_statuses);
+  return self->priv->allowed_presence_statuses;
+}

Blank lines around the if block, please (i.e. add a blank line before the
"return", in this case).

@@ +2559,3 @@
+ * @status: the desired status, one of the statuses returned by
+ * tp_connection_get_allowed_presence_statuses()
+ * @status_message: desired status message

I think it would be nice to accept NULL for this, and silently translate that
to "".

@@ +2588,3 @@
+      status, status_message, tp_connection_set_self_presence_cb, result,
NULL,
+      G_OBJECT (self));
+}

Don't pass @self as the weak_object. TpProxy refs itself for as long as a
tp_cli call (with a non-NULL callback) is in-flight, so it can't die; if the
callback needs it, it can get it from @proxy.

If @self wasn't automatically reffed in this way, then your purported API
guarantee of "called exactly once" would not be true, so it's a good thing it
is :-)

The callback you've written would crash if @callback is NULL. You could fix
that in the callback, but that's actually not ideal.

For better D-Bus efficiency, what you should actually do is to use a NULL
callback instead of tp_connection_set_self_presence_cb if the reply is going to
be ignored. If you do that, the proxy will just fire off a D-Bus message with
the "no reply needed" flag, and forget about it; the CM won't bother to reply,
and libdbus, dbus-glib, TpProxy and dbus-daemon won't try to track the reply
either.

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