[Bug 29751] Presence statuses extending & XMPP privacy list use by plugins

[bugzilla-daemon at freedesktop.org]

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

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2010-08-23 06:27:28 PDT ---
+      for (i = 0; iface->presence_statuses[i].name; i++)
+        {
+          ret = g_list_prepend (ret, &(iface->presence_statuses[i]));
+        }
+      ret = g_list_reverse (ret);

I'm allergic to this prepend-and-reverse pattern. I think this function would
be clearer written as follows (with coding style fixes like adding a blank line
before the 'for', adding the explicit != NULL):

  GQueue statuses = G_QUEUE_INIT;

  if (iface->presence_statuses != NULL)
    {
      gint i;

      for (i = 0; iface->presence_statuses[i].name != NULL; i++)
        g_queue_push_tail (&statuses, &(iface->presence_statuses[i]));
    }

  return statuses.head;

But the point is moot because if this function just returned the array of
statuses from the interface, gabble_plugin_loader_append_statuses() would be
way shorter and easier to follow (untested):

  GArray *result = g_array_new (TRUE, TRUE, sizeof (TpPresenceStatusSpec));
  guint i, j;

  for (i = 0; base_statuses[i].name != NULL; i++)
    g_array_append_val (result, base_statuses[i]);

  for (i = 0; i < priv->plugins->len; i++)
    {
      GabblePlugin *p = g_ptr_array_index (priv->plugins, i);
      TpPresenceStatusSpec *statuses =
gabble_plugin_get_custom_presence_statuses (p));

      for (j = 0; statuses[j].name != NULL; j++)
        g_array_append_val (result, statuses[j]);
    }

  return g_array_free (result, FALSE);

+      if (status)
+          return status;

Coding style dictates:

+      if (status != NULL)
+        return status;

Ditto indentation of:

+  if (iface->privacy_list_map == NULL)
+      return NULL;

I think indenting the second argument of tp_strdiff() in

+      if (!tp_strdiff (list_name,
+          iface->privacy_list_map[i].privacy_list_name))

by four spaces would be more readable.

+/* TODO: update this when telepathy-glib supports setting
+ * statuses at constructor time; until then, gabble_statuses
+ * leaks.
+ */

It's a once-per-process leak; is that so bad that it warrants an entry in
FIXME.out? :)

+GabblePresenceId _conn_presence_get_type (GabblePresence *presence);

This is the only function in that header with a _ prefix.

+        GabblePresenceId presence_type = _conn_presence_get_type (presence);
+
+        switch (presence_type)
+          {
+          case TP_CONNECTION_PRESENCE_TYPE_AVAILABLE:

The TpConnectionPresenceType enum doesn't match the GabblePresenceId enum. And
I don't understand this function body at all now. Do plugins have to assign IDs
to presences that fall off the end of the GabblePresenceId enum or something?

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