Bug 28230

Here is a start of a branch fixing that:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/location

It doesn't block the transition state and doesn't add OLPC interfaces for now.

Note that with this branch it still don't work with my server :(
According to the XEP, the server is supposed to return:
    <identity category='pubsub' type='pep'/>
but I receive instead:
    <identity category='pubsub' type='service'/>

I guess that's an ejabberd bug so I opened https://support.process-one.net/browse/EJAB-1238

(In reply to comment #1)
> Here is a start of a branch fixing that:
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/location
> 
> It doesn't block the transition state and doesn't add OLPC interfaces for now.
> 
> Note that with this branch it still don't work with my server :(
> According to the XEP, the server is supposed to return:
>     <identity category='pubsub' type='pep'/>
> but I receive instead:
>     <identity category='pubsub' type='service'/>
> 
> I guess that's an ejabberd bug so I opened
> https://support.process-one.net/browse/EJAB-1238

+  if (!_gabble_connection_send_with_reply (conn, msg, set_location_sent_cb,
+        G_OBJECT (conn), NULL, NULL))

if you pass NULL as the cb to send_with_reply it squashes the unhandled IQ warning without needing a no-op function.

Given that you do actually use the callback in the next commit, just squash those two commits together?

Any particular reason for the bonus underscore in bare_jid__disco_cb ?

MyXmppStream is a terrible name.

+    try:
+        conn.Location.SetLocation({'lat': 0.0, 'lon': 0.0})
+    except dbus.DBusException:
+        pass
+    else:
+        assert False, "Should have had an error!"

call_async(...)
q.expect('dbus-error', error_name=cs.NOT_IMPLEMENTED)

would be clearer and test more and be shorter.

+    _, _, e = q.expect_many(
+        EventPattern('stream-iq', iq_type='set',
+            query_ns=ns.PUBSUB),
+        EventPattern('dbus-signal', signal='StatusChanged',
+            args=[cs.CONN_STATUS_CONNECTED, cs.CSR_REQUESTED]),
+        EventPattern('stream-iq', iq_type='get', to='test@localhost',
+            query_ns=ns.DISCO_INFO)
+        )

We only care about the last one, so why expect the others? (Particularly given that we shouldn't move to Connected before discoing ourself: see below.)

(In reply to comment #0)
> We should then disco our own bare jid as well. This lead to a question: should
> we block the transition to the CONNECTED state until we received the reply of
> this disco (as we do for the reply of the server disco?).

Yes we should. It's not just the OLPC interfaces that depend on this: we recently added a property to the Location interface specifying whether or not SetLocation is supported (http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Connection.Interface.Location.html#org.freedesktop.Telepathy.Connection.Interface.Location.SupportedLocationFeatures). It's not implemented yet, but in order to implement it correctly we need to wait until we've discoed ourself.

(In reply to comment #2)
> (In reply to comment #1)
> > Here is a start of a branch fixing that:
> > http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/location
> > 
> > It doesn't block the transition state and doesn't add OLPC interfaces for now.
> > 
> > Note that with this branch it still don't work with my server :(
> > According to the XEP, the server is supposed to return:
> >     <identity category='pubsub' type='pep'/>
> > but I receive instead:
> >     <identity category='pubsub' type='service'/>
> > 
> > I guess that's an ejabberd bug so I opened
> > https://support.process-one.net/browse/EJAB-1238
> 
> +  if (!_gabble_connection_send_with_reply (conn, msg, set_location_sent_cb,
> +        G_OBJECT (conn), NULL, NULL))
> 
> if you pass NULL as the cb to send_with_reply it squashes the unhandled IQ
> warning without needing a no-op function.
> 
> Given that you do actually use the callback in the next commit, just squash
> those two commits together?

squased.

> Any particular reason for the bonus underscore in bare_jid__disco_cb ?

oops. Fixed.

> MyXmppStream is a terrible name.

renamed.

> +    try:
> +        conn.Location.SetLocation({'lat': 0.0, 'lon': 0.0})
> +    except dbus.DBusException:
> +        pass
> +    else:
> +        assert False, "Should have had an error!"
> 
> call_async(...)
> q.expect('dbus-error', error_name=cs.NOT_IMPLEMENTED)

Oh didn't know (I didn't gabble since a while). Done.

> would be clearer and test more and be shorter.
> 
> +    _, _, e = q.expect_many(
> +        EventPattern('stream-iq', iq_type='set',
> +            query_ns=ns.PUBSUB),
> +        EventPattern('dbus-signal', signal='StatusChanged',
> +            args=[cs.CONN_STATUS_CONNECTED, cs.CSR_REQUESTED]),
> +        EventPattern('stream-iq', iq_type='get', to='test@localhost',
> +            query_ns=ns.DISCO_INFO)
> +        )
> 
> We only care about the last one, so why expect the others? (Particularly given
> that we shouldn't move to Connected before discoing ourself: see below.)
> 
> (In reply to comment #0)
> > We should then disco our own bare jid as well. This lead to a question: should
> > we block the transition to the CONNECTED state until we received the reply of
> > this disco (as we do for the reply of the server disco?).
> 
> Yes we should. It's not just the OLPC interfaces that depend on this: we
> recently added a property to the Location interface specifying whether or not
> SetLocation is supported
> (http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Connection.Interface.Location.html#org.freedesktop.Telepathy.Connection.Interface.Location.SupportedLocationFeatures).
> It's not implemented yet, but in order to implement it correctly we need to
> wait until we've discoed ourself.

Good point. I've done that.

> +  /* FIXME: add OLPC iface if PEP is supported? */

Please do :-)

> +              DEBUG ("Server advertise PEP support in our jid features");

"Server advertises PEP ..."

> +/**
> + * connection_disco_cb
> + *
> + * Stage 3 of connecting
...
> + */
>  static void
>  set_status_to_connected (GabbleConnection *conn)

Wrong function name in the doc-comment

> +class PEPinJidDiscoXmppStream(BaseXmlStream):

Please be consistent about whether you capitalize acronyms in camel-case or not, at least within a class's name (PEPInJIDDiscoXMPPStream vs. PepInJidDiscoXmppStream).

I'd be inclined to change the behaviour of BaseXmlStream to be correct (like PEPinJidDiscoXmppStream is now), and make the special stream class in pep-support.py behave like the old BaseXmlStream - call it PepInServerDiscoXmlStream, or OldEjabberdXmlStream, or something. I think it's better if most of our tests are realistic, and legacy behaviour is the special case.

(In reply to comment #4)
> > +  /* FIXME: add OLPC iface if PEP is supported? */
> 
> Please do :-)

Oh, you did later. Ignore that one.

I fixed all your comments:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/location

Merged to master.