[Bug 24107] support Statuses on TpConnection

[bugzilla-daemon at freedesktop.org]

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

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

::: telepathy-glib/connection.c
@@ +486,3 @@
+      k,
+      g_value_get_boolean (value->values + 1),
+      g_value_get_boolean (value->values + 2));

You could use tp_value_array_unpack().

@@ +516,3 @@
+  self->priv->allowed_presence_statuses = g_hash_table_new_full (g_str_hash,
+      g_str_equal, g_free, (GDestroyNotify) tp_status_spec_destroy);
+

Blank line after the if/unref block, please

@@ +521,3 @@
+
+  g_hash_table_foreach (statuses, allowed_presence_statuses_foreach,
+                        self->priv->allowed_presence_statuses);

GHashTableIter is often more readable than g_hash_table_foreach?

@@ +537,3 @@
+  if ((self->priv->allowed_presence_statuses != NULL) &&
+        (self->priv->final_statuses))
+    return;   /* already done */

This has more parentheses than it needs to. && has lower precedence than != and
we assume C programmers know that.

@@ +551,3 @@
+        (self->priv->final_statuses))
+    return;   /* not interested right now */
+

You don't need the parentheses here - && has low precedence.

@@ +2492,3 @@
+
+/**
+ * tp_status_spec_copy:

Should this be (skip)?

@@ +2509,3 @@
+                             self->status,
+                             self->may_set_on_self,
+                             self->can_have_message);

We don't indent multi-line arguments like that; see
http://telepathy.freedesktop.org/wiki/Style

@@ +2513,3 @@
+
+/**
+ * tp_status_spec_destroy:

Should this be (skip)?

@@ +2544,3 @@
+ *
+ * Returns: (element-type utf8 TelepathyGLib.StatusSpec): Dictionary from
+ * string identifiers to structs for each valid status.

In telepathy-glib we usually make these function-based accessors not return a
new ref, with a convention of using _dup_ in the name of an accessor that does
return a copy.

Should this be made available as a GObject property too, with this function as
a "C binding"?

@@ +2615,3 @@
+  else
+    tp_cli_connection_interface_simple_presence_call_set_presence (self, -1,
+        status, status_message, NULL, NULL, NULL, NULL);

The "if" block has {}, which means the "else" needs them too, even though it's
only one line.

::: telepathy-glib/connection.h
@@ +105,3 @@
+TpStatusSpec *tp_status_spec_new (TpConnectionPresenceType type,
+    const gchar *status, gboolean may_set_on_self, gboolean can_have_message)
+  G_GNUC_WARN_UNUSED_RESULT;

Does this need to be public API? If not, put it in connection-internal.h and
prefix it with an underscore.

@@ +108,3 @@
+
+TpStatusSpec *tp_status_spec_copy (TpStatusSpec *self)
+  G_GNUC_WARN_UNUSED_RESULT;

The argument should be const

::: tests/lib/contacts-conn.c
@@ +320,1 @@
     return FALSE;

You don't need this many parentheses. == and != have higher precedence than &&
and ||, and we assume C programmers will know that.

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