[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

--- Comment #25 from Eitan Isaacson <eitan.isaacson at collabora.co.uk> 2010-07-06 13:43:06 PDT ---
(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

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