Bug 20768 (xep-invisibility)

(In reply to comment #0)
> I've heard rumours that on Google servers you can become invisible by
> sending <presence type="unavailable"> but remaining connected.

This doesn't seem to work.

Having sent <presence type='unavailable'/> to Google's server, messages I sent were still delivered to my contacts, but nothing was delivered back to me.

<http://code.google.com/apis/talk/jep_extensions/shared_status.html> allows you to become invisible on Google servers, but is not exactly trivial to implement...

This lack of feature in gabble is especially annoying when empathy is used with other account types besides xmpp, like msn with butterfly, and the attempt to change the state to hidden prevents msn account from changing to hidden as well. 

It would be really really nice to have this feature in order to use empathy more comfortably.

(In reply to comment #2)
> This lack of feature in gabble is especially annoying when empathy is used with
> other account types besides xmpp, like msn with butterfly, and the attempt to
> change the state to hidden prevents msn account from changing to hidden as
> well. 

This actually isn't true: if you choose Invisible, but not all accounts support that, the accounts which don't support it are set to Busy and those which do are set to Invisible.

Then, Empathy shows the "most available" of the presences you're in, rather than claiming you're Invisible when in fact you're Busy on XMPP.

(In reply to comment #3)
> (In reply to comment #2)
> > This lack of feature in gabble is especially annoying when empathy is used with
> > other account types besides xmpp, like msn with butterfly, and the attempt to
> > change the state to hidden prevents msn account from changing to hidden as
> > well. 
> 
> This actually isn't true: if you choose Invisible, but not all accounts support
> that, the accounts which don't support it are set to Busy and those which do
> are set to Invisible.
> 
> Then, Empathy shows the "most available" of the presences you're in, rather
> than claiming you're Invisible when in fact you're Busy on XMPP.
> 

Yes, I tested this and you are indeed right. But I was sure that there is a confusion, since I got visible at times I was sure I did not mean to.

After some testing, I realised that if the networkmanager reports that the network is down and empathy disconnects (e.g., when computer is going to sleep), later when the connection comes up (e.g., at resume) and empathy connects again, empathy sets all accounts to busy, i.e., it forgets that it should first try to set the status to invisible for accounts.

(In reply to comment #4)
> After some testing, I realised that if the networkmanager reports that the
> network is down and empathy disconnects (e.g., when computer is going to
> sleep), later when the connection comes up (e.g., at resume) and empathy
> connects again, empathy sets all accounts to busy, i.e., it forgets that it
> should first try to set the status to invisible for accounts.

Please report that against Empathy (in GNOME bugzilla) if nobody has done so already; Empathy should remember what you asked for, rather than what you got.

(Not directly relevant to this bug, though.)

Is anyone aware of a server implementation of XEP-0186? Can't find it in the usual suspects: jabberd14, ejabberd or prosody. If it is so rare in the wild, I guess that a *gulp* privacy list implementation should take precedence.

Some mid-work notes regarding XEP-0126:

1. Privacy lists (xep-0016) are super silly:
 a. The server storage part, just makes implementing invisibility more complicated. We want a super simple privacy list, are we supposed to simply trust a blessed list name like "invisible"? Retrieve it each time and make sure it is what we need before making it the active list? Answer: Simply create a new list called "invisible" every session. This overwrites whatever was on the server and saves us some round trips.

 b. Multiple resource cases are tricky. What if another client is using a different list with the same name? Each client would constantly be clobbering the other client's list, or worse, the behavior of a client could change mid-session because it's active list has been modified. Answer: xep-0126 recommends that all clients use the name "invisible" for global invisibility.

2. connection.c is huge and messy, I really wish there was a better place for implementing this, hate to increase the mess, but I can't find it.

Hey, it is very exciting to see someone finally working on this. Hope to see it implemented in a soonish release! Thanks!

I think this branch is ready for review. It is based on a re-factor in bug 27779.

Taking this bug to review the branch.

_gabble_connection_create_invisible_privacy_list() and _gabble_connection_invisible_privacy_list_set_active() would be a lot more readable if they used wocky_stanza_build()! Ditto later functions that build stanzas.

In both of those functions, you're sending the IQ to the server, and ignoring the reply. (_gabble_connection_send() only returns FALSE if we're not connected; any other errors are reported asynchronously.) Is that intentional? I feel like we should probably not silently ignore setting yourself as invisible failing asynchronously.

Also... we should probably listen for privacy list pushes to detect someone deleting our invisible list. (I'm not really sure what we should do if that happens... log a debug message? Try to re-create it?)

Does this branch do the right thing if I set myself to invisible before calling Connect(), or do we get a brief flash of Available first? Ah, I see you have tests for this case... I'd be more sure it worked if they actually tested that we only send presence after turning on invisibility.

+      (conn->features & GABBLE_CONNECTION_FEATURES_PRESENCE_INVISIBLE ||
+          GABBLE_CONNECTION_FEATURES_PRIVACY == 0))

You mean |, not ||.

The second patch partially reverting the first makes it hard to review what's actually changed. Could you squash them together? And then the third patch moves all that code to a different file, and breaks some indentation:

-      if ((self->features & GABBLE_CONNECTION_FEATURES_PRIVACY) != 0)
-        lm_message_node_set_attribute (lm_message_get_node (message),
-            "type", "unavailable");
+            if ((self->features & GABBLE_CONNECTION_FEATURES_PRIVACY) != 0)
+       lm_message_node_set_attribute (lm_message_get_node (message),
+           "type", "unavailable");

Broadcasting presence type=unavailable to MUCs is a bad idea: that's how you leave a MUC! <http://xmpp.org/extensions/xep-0045.html#exit> I think when sending presence to a MUC, we should map invisible to Busy: you can't be invisible in a MUC.

(In reply to comment #11)
> In both of those functions, you're sending the IQ to the server, and ignoring
> the reply. (_gabble_connection_send() only returns FALSE if we're not
> connected; any other errors are reported asynchronously.) Is that intentional?
> I feel like we should probably not silently ignore setting yourself as
> invisible failing asynchronously.
> 
> Also... we should probably listen for privacy list pushes to detect someone
> deleting our invisible list. (I'm not really sure what we should do if that
> happens... log a debug message? Try to re-create it?)

Okay, so there are a few failure cases:

• We can't even create the 'invisible' list in the first place. I think we should wait and see if this operation succeeds before becoming connected, and if it doesn't succeed, just don't offer invisible as a presence.
• 'invisible' exists, but setting it fails. This really shouldn't happen as long as our IQ is correct (which isn't hard to ensure), so it failing means the server is buggy or something else beyond our control. I think we should handle this case by flipping our status back to Busy, so at least the user knows they're not invisible.
• Someone else blows away our 'invisible' list and replaces it with something else. I'm unsure how to deal here. An argument in favour of checking the list at login — rather than just replacing it every time — is that we would prevent this happening in the simple case of being connected with two Gabbles at once!

I won't reply inline because I radically changed the code-base.

I accounted for most of your concerns:
* The muc channel mangles the presence to dnd if it set to hidden.
* Replies to privacy list ops and invisibility setting are expected.
 - If a failure happens before connected, remove the feature from our cache and try a legacy invisibility method if it is available, if not set status to dnd (in the case of privacy lists, the feature will be removed if we cannot create a list).
 - If connected, the SetPresence won't fail, but the PresencesChanged signal will ultimately come back with dnd.

Feedback is welcome, I'll put in more privacy list features tomorrow that will answer all the different clobber cases.

(In reply to comment #13)
> I won't reply inline because I radically changed the code-base.

The commits are a bit hard to follow as a result... :)

> I accounted for most of your concerns:
> * The muc channel mangles the presence to dnd if it set to hidden.
> * Replies to privacy list ops and invisibility setting are expected.
>  - If a failure happens before connected, remove the feature from our cache and
> try a legacy invisibility method if it is available, if not set status to dnd
> (in the case of privacy lists, the feature will be removed if we cannot create
> a list).
>  - If connected, the SetPresence won't fail, but the PresencesChanged signal
> will ultimately come back with dnd.
> 
> Feedback is welcome, I'll put in more privacy list features tomorrow that will
> answer all the different clobber cases.

Looks good! I've got quite a few nit-picky comments, but nothing huge.

General comments on the tests: could you add some comments to the tests explaining what's going on?

tests/twisted/presence/invisible_xep_0186.py:

+    assert ("hidden" in conn.Get(cs.CONN_IFACE_SIPLE_PRESENCE, "Statuses",
+                                 dbus_interface=cs.PROPERTIES_IFACE).keys())

This could be assertContains("hidden", conn.Properties.Get(cs.CONN_IFACE_SIMPLE_PRESENCE, "Statuses")) I think.

Reading <http://xmpp.org/extensions/xep-0186.html#visible>, this test should check that Gabble sends a non-unavailable <presence/> stanza after sending visible. Currently it only checks that it sends <visible/>, and that it claims at the Telepathy level to be away.


tests/twisted/presence/invisible_xep_0126.py:

tests/twisted/presence/invisible_helper.py:

+    def _cb_invisible_success(self, iq):
+        reply = elem_iq(self, 'result', id=iq["id"])
+        self.send(reply)
+
+    def _cb_invisible_fail(self, iq):
+        reply = elem_iq(self, 'error', id=iq["id"])
+        self.send(reply)

These could use acknowledge_iq and send_error_reply.

src/connection.c:

+/**
+ * connection_initial_presence_cb
+ *
+ * Stage 4 of connecting, this function is called by after presence is properly
+ * set. This is required when sending more than simple <presence/> stanzas, and
+ * asynchronous results are needed.
+ *
+ */

Please update _gabble_connection_connect()'s comment to reflect there being a fourth stage.

I'm inclined that the fallbacks should be in conn_presence_set_initial_presence_async(), rather than relying on the callback to repeatedly call it. So then connection.c would just call it once, and get called back, and in the callback either it's worked, or it's failed really severely so we should give up the connection.

conn-presence:

conn_presence_set_initial_presence_finished() would more conventionally be called _finish().

set_own_status_cb() looks like this:

  if (...)
    {
      ...
      goto OUT;
    }
  if (...)
    {
      ...
    }
  else
    {
      ...
    }

  OUT:
    ...

I think the goto makes this harder to follow than just making the second 'if' an 'else if', which is the effect of this code anyway.

toggle_presence_visibility_async(): you shouldn't call g_s_a_r_complete() from the _async() function, you should use _complete_in_idle(). There's a poorly-documented[1] API contract that the callback should never be called directly from the _async() function.

In fact it seems like all the functions toggle_p_v_a() call also return synchronously if _gabble_connection_send_with_reply() returns an error immediately.

I initially thought that presence_create_invisible_privacy_list() was releasing a reference to 'result' that it does not own, but was wrong. I'm not sure whether a clarifying comment would help. (It sucks that _gabble_connection_send_with_reply() may return synchronously *or* asynchronously. wocky_porter_send_iq_async(), which it wraps for source-compat with 0.8, gets this right.)

Callbacks for _gabble_connection_send_with_reply() can't assume that the reply message is non-NULL, because the connection might have ended before the stanza was actually sent over the wire, or before we got a reply.

Oh, wait, except the wrapper in lib/loudmouth/lm-connection.c actually doesn't call the callback in this case. Greeaaaaaaat.... this means we never complete the operation.

I think this calls for tearing out at least one layer of abstraction, to be honest. I'm inclined to defer this to another time. I'll file a bug.

+      GError *error;
+      /* XEP-0018 */

should initialize error to NULL. `make check` should catch this: we have a script that greps for uninitialized GError *s.

set_xep0126_invisible(): the stanza_build calls could use some indenting. (ISTR from my time trying it out that emacs is very keen not to let you do this. :/)


[1] I incorrectly reverse-engineered how to use GSAR from reading Wocky code (some of which turned out to be wrong itself), and upon discovering I'd got it wrong, tried to write an example for the documentation. I'm happy to report that the patch was bike-shod and then forgotten at https://bugzilla.gnome.org/show_bug.cgi?id=602417 :(

(In reply to comment #14)
> I think this calls for tearing out at least one layer of abstraction, to be
> honest. I'm inclined to defer this to another time. I'll file a bug.

This is bug 27918.

I've got a branch at http://git.collabora.co.uk/?p=user/wjt/telepathy-gabble-wjt.git;a=shortlog;h=refs/heads/invisibility where I merged current master and fixed up this branch to work with it, and fixed a few of the simple points above. Working on the rest...

(In reply to comment #14)
> tests/twisted/presence/invisible_xep_0186.py:
> 
> +    assert ("hidden" in conn.Get(cs.CONN_IFACE_SIPLE_PRESENCE, "Statuses",
> +                                 dbus_interface=cs.PROPERTIES_IFACE).keys())
> 
> This could be assertContains("hidden",
> conn.Properties.Get(cs.CONN_IFACE_SIMPLE_PRESENCE, "Statuses")) I think.

Fixed in my branch.

> tests/twisted/presence/invisible_helper.py:
> 
> +    def _cb_invisible_success(self, iq):
> +        reply = elem_iq(self, 'result', id=iq["id"])
> +        self.send(reply)
> +
> +    def _cb_invisible_fail(self, iq):
> +        reply = elem_iq(self, 'error', id=iq["id"])
> +        self.send(reply)
> 
> These could use acknowledge_iq and send_error_reply.

Fixed in my branch.

> set_own_status_cb() looks like this:
> 
>   if (...)
>     {
>       ...
>       goto OUT;
>     }
>   if (...)
>     {
>       ...
>     }
>   else
>     {
>       ...
>     }
> 
>   OUT:
>     ...
> 
> I think the goto makes this harder to follow than just making the second 'if'
> an 'else if', which is the effect of this code anyway.

Fixed in my branch.

> +      GError *error;
> +      /* XEP-0018 */
> 
> should initialize error to NULL. `make check` should catch this: we have a
> script that greps for uninitialized GError *s.

Fixed.

> set_xep0126_invisible(): the stanza_build calls could use some indenting. (ISTR
> from my time trying it out that emacs is very keen not to let you do this. :/)

Fixed.

(In reply to comment #14)
> General comments on the tests: could you add some comments to the tests
> explaining what's going on?

I added some docs to the tests a bit.

> Reading <http://xmpp.org/extensions/xep-0186.html#visible>, this test should
> check that Gabble sends a non-unavailable <presence/> stanza after sending
> visible. Currently it only checks that it sends <visible/>, and that it claims
> at the Telepathy level to be away.

Fixed, and found a bug (which I fixed) in the process.

> src/connection.c:
> 
> +/**
> + * connection_initial_presence_cb
> + *
> + * Stage 4 of connecting, this function is called by after presence is
> properly
> + * set. This is required when sending more than simple <presence/> stanzas,
> and
> + * asynchronous results are needed.
> + *
> + */
> 
> Please update _gabble_connection_connect()'s comment to reflect there being a
> fourth stage.

Fixed in my branch.

> I'm inclined that the fallbacks should be in
> conn_presence_set_initial_presence_async(), rather than relying on the callback
> to repeatedly call it. So then connection.c would just call it once, and get
> called back, and in the callback either it's worked, or it's failed really
> severely so we should give up the connection.

I have a work-in-progress patch that actually removes the fallback between methods, with the rationale that if the server advertises a feature but turns out to be broken... then our server is broken.

> conn-presence:
> 
> conn_presence_set_initial_presence_finished() would more conventionally be
> called _finish().

Fixed.

> toggle_presence_visibility_async(): you shouldn't call g_s_a_r_complete() from
> the _async() function, you should use _complete_in_idle(). There's a
> poorly-documented[1] API contract that the callback should never be called
> directly from the _async() function.
> 
> In fact it seems like all the functions toggle_p_v_a() call also return
> synchronously if _gabble_connection_send_with_reply() returns an error
> immediately.

Fixed.

Remaining issues, as described in the WIP patch at the top of my branch:


    This patch is WIP because it breaks invisible_fallbacks.py (given that
    it deletes the fallbacks that patch checks), because we should also have
    tests for setting ourselves to invisible failing once the connection is
    up and running (checking that Gabble reverts you to either your previous
    presence, or DND, as we wish), and because we need to look up existing
    privacy lists to fix this case:
    
    • Gabble #1 signs in
    • Gabble #1 creates a list called 'invisible', and activates it.
    • Gabble #2 also signs in
    • Gabble #2 tries to create a list called 'invisible', but it fails
      because you can't replace privacy lists that are in use, so it gives
      up on invisibility.

Bouncing this back Eitan's way; my work on the branch fizzled out a bit, but there's some WIP...

(In reply to comment #17)
> Remaining issues, as described in the WIP patch at the top of my branch:
> 
> 
>     This patch is WIP because it breaks invisible_fallbacks.py (given that
>     it deletes the fallbacks that patch checks), because we should also have
>     tests for setting ourselves to invisible failing once the connection is
>     up and running (checking that Gabble reverts you to either your previous
>     presence, or DND, as we wish),

I am fine not having fallbacks, I guess we need to expect servers to generally work.

> and because we need to look up existing
>     privacy lists to fix this case:
> 
>     • Gabble #1 signs in
>     • Gabble #1 creates a list called 'invisible', and activates it.
>     • Gabble #2 also signs in
>     • Gabble #2 tries to create a list called 'invisible', but it fails
>       because you can't replace privacy lists that are in use, so it gives
>       up on invisibility.

From my XEP parsing, this is not a real issue. Creating a list and editing a list are semantically the same (XEP-0016 2.6 & 2.7). When you edit a list that is active in another resource, you don't get an error (although the other client gets a push notification). In other words a 'invisible' list creation stanza is guaranteed to not return a <conflict /> error from the server.

The case we need to look out for is when Gabble #2 signs in and "creates" a list, Gabble #1 will get a privacy list push notification, which in turn it needs to retrieve the list, see that it is still the standard 'invisible' list, and carry on.

If client #2 is a rogue client (Let's call in GAIM ;), and is doing it's own weird crap to the 'invisible' list, then we need to gracefully step out of it's way, by creating and activating a list called 'invisible-gabble'.

If client #1 is rogue, when Gabble #2 signs in, it will clobber #1's 'invisible' list. If #1 listens for push notifications and fights back by re-editing 'invisible', Gabble #2 will step out by creating 'invisible-gabble'.

I guess what I am saying is that the one addition that needs to be put in place is listening for push notifications and dealing with them properly.

(In reply to comment #19)
> From my XEP parsing, this is not a real issue. Creating a list and editing a
> list are semantically the same (XEP-0016 2.6 & 2.7). When you edit a list that
> is active in another resource, you don't get an error (although the other
> client gets a push notification). In other words a 'invisible' list creation
> stanza is guaranteed to not return a <conflict /> error from the server.

Aha, I misread the XEP: you can't delete lists that are in use, and I assumed you couldn't modify lists which are in use either.

> The case we need to look out for is when Gabble #2 signs in and "creates" a
> list, Gabble #1 will get a privacy list push notification, which in turn it
> needs to retrieve the list, see that it is still the standard 'invisible' list,
> and carry on.

Yep, although maybe we should relax “the standard 'invisible' list” to something like “starts with the standard <item action='deny' order='1'><presence-out/></item> rule” or something? Not overly fussed.

> If client #2 is a rogue client (Let's call in GAIM ;), and is doing it's own
> weird crap to the 'invisible' list, then we need to gracefully step out of it's
> way, by creating and activating a list called 'invisible-gabble'.
> 
> If client #1 is rogue, when Gabble #2 signs in, it will clobber #1's
> 'invisible' list. If #1 listens for push notifications and fights back by
> re-editing 'invisible', Gabble #2 will step out by creating 'invisible-gabble'.

Yeah, so something like “if an 'invisible' list already exists, or is created, and doesn't look like we want, give up and make a gabble-specific one” sounds good to me.

(I've filed bug 28726 for Google Shared Status, separating it from the concerns of this bug.)

Ok, I am finally happy with my branch. Might need some squashing, but I am satisfied with the general gist.

The changes I made introduce 3 new functional changes:

1. Listen for privacy list push notifications, if it's our invisibility list that changed, do a brief check to see that it still hides our presence. If not, go to listname-gabble (typically 'invisible-gabble').
2. Have sign on use the same code path as above. Retrieve the 'invisible' list, if it doesn't exist, create it. If it betrays it's name, create 'invisible-gabble'.
3. Don't rely on privacy lists being sent in the disco process: Prosody and some versions of ejabberd don't advertise their privacy list support. Instead try to retrieve/create a list on signon, if we reach an unexpected failure, revert to either XEP-0018, or no invisibility (dnd).

http://git.collabora.co.uk/?p=user/eitan/telepathy-gabble.git;a=shortlog;h=refs/heads/invisibility

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.

(In reply to comment #24)
> 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.

cc9e31a

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

7dfd877

> +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!

Never mind!

> 
> +  DEBUG ("%s", list_name);
> 
> This would be more useful if it said "Creating invisible privacy list named
> '%s'", list_name.
> 

This has also been deleted at some point, and something along the lines you suggested has been added.

> +  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()).
> 

a1d5aef

So I decided not to null-check at all. Some places in gabble assume error != NULL, so it's not a precedent.

jingle-factory.c:460
bytestream-ibb.c:511
muc-channel.c:1745
muc-channel.c:2315

To be on the safe side, we could simply not pass a GError...

> +      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.? :)
> 

Yup! Although this is highly unlikely since gabble just sets 'invisible-gabble' to an invisible list, so other gabble instances would see it as valid and hold their peace. They would not be chasing each other.

> 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'.
> 

9c7603d

> 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.

Yeah, it's not required in the XEPs. In any case recent ejabberds and prosody trunk do advertise.

Amended to: 3173442

Ship it!

\o\ \o/ /o/

You will be disappointed to hear that this branch breaks every single pre-existing test. This is because we always request the 'invisible' list on request, and the default stream does not reply to this iq.

Please review the HEAD of my branch for the necessary change.

Merged.