[Bug 20768] Support more standard ways to be invisible (XEP-0186, maybe XEP-0126)

[bugzilla-daemon at freedesktop.org]

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

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review-, minor changes

--- Comment #24 from Will Thompson <will.thompson at collabora.co.uk> 2010-07-06 04:28:21 PDT ---
I endorse this from a functional perspective. A few style/trivia points, and
one mis-placed critical:

+def send_privacy_list_push_iq(stream, list_name):
+    iq = IQ(stream, "set")
+    iq.addUniqueId()
+    iq.addRawXml(
+        "<query xmlns='jabber:iq:privacy'><list name='%s' /></query>" % \
+            list_name)
+    stream.send(iq)
+    return iq["id"]

from gabbletest import elem_iq, elem

iq = elem_iq(stream, 'set')(
  elem(ns.PRIVACY, 'query')(
    elem('list' name=list_name)
  )
)
stream.send(iq)
return iq['id']

should work I think? addRawXml() seems unnecessary :)

Ditto other places that hardcode stringified XML in the test... I think it's
harder to read.

-            '@', "name", "invisible",
+        '@', "name", self->presence_priv->invisible_privacy_list,

Unfortunate unindenting.

+static const gchar *
+get_list_name_from_message (LmMessage *message)
+{
+  LmMessageNode *node = lm_message_node_find_child (wocky_stanza_get_top_node
(
+          message), "list");
+
+  g_return_val_if_fail (node != NULL, NULL);
+
+  return lm_message_node_get_attribute (node, "name");
+}

This will critical if the server sends a malformed stanza, which doesn't seem
right in a world where criticals indicate programming errors.

Oh actually you deleted this function later. Never mind!

+  DEBUG ("%s", list_name);

This would be more useful if it said "Creating invisible privacy list named
'%s'", list_name.

+  if (!_gabble_connection_send_with_reply (self, (LmMessage *) iq,
+          examine_new_invisible_list_cb, NULL, result, &error))
+    {
+      if (error != NULL)
+        {
+          g_simple_async_result_set_from_error (result, error);
+          g_error_free (error);
+        }
+
+      g_simple_async_result_complete_in_idle (result);
+      g_object_unref (result);
+    }

I think this should set a fallback error if
_gabble_connection_send_with_reply() erroneously returns FALSE without setting
error, or, raise a critical (because that would be a bug in
_gabble_connection_send_with_reply()).

+      gchar *new_list_name = g_strdup_printf ("%s-gabble", list_name);

So if creating invisible-gabble fails, we'll fall back to
invisible-gabble-gabble, etc.? :)

in disable_privacy_lists():

+  DEBUG (" ");

-  g_simple_async_result_complete_in_idle (result);
+  if (self->features & GABBLE_CONNECTION_FEATURES_PRESENCE_INVISIBLE)
+    priv->invisibility_method = INVISIBILITY_METHOD_PRESENCE_INVISIBLE;
+  else
+    priv->invisibility_method = INVISIBILITY_METHOD_NONE;

It'd be more useful to DEBUG whether or not we fell back to presence
type='invisible'.

It's surprising that these servers don't advertise privacy list support
properly. I noticed that http://xmpp.org/rfcs/rfc3921.html doesn't actually
specify how to advertise support for it... but it still seems like a bug in the
servers. It'd be good to document why we don't look for that cap in the source
and in the tests; so in 81d1c21 where you bin the use of PrivacyListXmlStream
(oh, should that class also be binned if it's unused?) it'd be good to comment
in the test that it's deliberately not advertising support.

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