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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #14 from Will Thompson <will.thompson at collabora.co.uk> 2010-04-30 12:27:16 PDT ---
(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 :(

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