[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 #3 from Will Thompson <will.thompson at collabora.co.uk> 2010-08-23 08:40:32 PDT ---
It would have been easier to review
http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=commitdiff;h=0794ab632674651887d6eb200965aed5caf6cebe
if it had been in smaller chunks. :(

+  if (list_name)
+    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
+        '(', "query", ':', NS_PRIVACY,
+          '(', "active",
+            '@', "name", list_name,
+          ')',
+        ')',
+        NULL);
+  else
+    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
+        '(', "query", ':', NS_PRIVACY,
+          '(', "active", ')',
+        ')',
+        NULL);

I think the following is probably clearer:

+    iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+        WOCKY_STANZA_SUB_TYPE_SET, NULL, NULL,
+        '(', "query", ':', NS_PRIVACY,
+          '(', "active",
+            '*', &active_node,
+          ')',
+        ')',
+        NULL);
+
+    if (list_name != NULL)
+      wocky_node_set_attribute (active_node, name, list_name);

I'm not overly bothered though. Maybe we should make wocky_stanza_build()
accept NULL attribute values and just skip that attribute.

Also, since list_name can be NULL, the following will crash on Windows and
other platforms where printf ("%s", NULL); crashes:

+  DEBUG ("Privacy status %s, backed by %s",
+    gabble_statuses[presence->status].name, list_name);

+  else if (!list_name && base->status != TP_CONNECTION_STATUS_CONNECTED)

list_name == NULL plz?


       conn_presence_signal_own_presence (self, NULL, &error);
       g_simple_async_result_complete_in_idle (result);
       g_object_unref (result);
+
+      goto OUT;

This leaks 'error'.

This patch seems to swap set_xep0126_invisible_cb and set_xep0186_invisible_cb
— which is fine, though it would have been nicer as a separate patch. That
said, the swap does show that the former needlessly casts user_data, while the
latter just assigns it to a GSimpleAsyncResult *. I think they should be
consistent, and maybe even use G_SIMPLE_ASYNC_RESULT().

+  g_assert (priv->privacy_statuses);

Why is this an assertion whereas earlier we had a g_return_if_fail() for it?
Both should really use != NULL anyway.

I'm having a hard time convincing myself that this is guaranteed to be NULL or
not NULL at the various points we assert about it. Maybe it could have a
comment explaining its lifecycle, at least?


+      for (i = node_iter (iq); i; i = node_iter_next (i))

There's no reason to use the node_iter() macros in new code. In fact, this code
should use Wocky's node iterators to only iterate across "list" children.

-          gchar *new_name = g_strdup_printf ("%s-gabble",
-              priv->invisible_list_name);
           g_free (priv->invisible_list_name);
-          priv->invisible_list_name = new_name;
+          priv->invisible_list_name = g_strdup ("invisible-gabble");

I think this code deliberately extended the previous name over and over,
because it can be hit more than once. If there's a test case for both
"invisible" and "invisible-gabble" existing but not satisfying
is_valid_invisible_list(), this change is okay with me, though.


+  /* This relies on the fact the first entries in the statuses table
+   * are from base_statuses. If index to the statuses table is outside
+   * the base_statuses table, the status is provided by a plugin. */
+  if (status >= (sizeof (base_statuses) / sizeof (TpPresenceStatusSpec)))

Yeah, this is pretty scary. Also, G_N_ELEMENTS.


+static TpPresenceStatusSpec test_presences[] = {
+  { "testbusy", TP_CONNECTION_PRESENCE_TYPE_BUSY, TRUE, NULL, NULL, NULL },
+  { "testaway", TP_CONNECTION_PRESENCE_TYPE_AWAY, FALSE, NULL, NULL, NULL },
+  { NULL, 0, FALSE, NULL, NULL, NULL }
+};
+
+static GabblePluginPrivacyListMap privacy_list_map[] = {
+  { "testbusy", "test-busy-list" },
+  { NULL, NULL },
+};
+

What's the point of testaway here? Why is it legal for plugins to provide
statuses that aren't linked to privacy lists?

+    elem_list=[]
+    for li in lists:
+        elem_list.append(elem('list', name=li))

elem_list = [elem('list', name=l) for l in lists]. Or even inline it into
building the iq.

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