The PEP XEP says ( http://xmpp.org/extensions/xep-0163.html#support ): "client SHOULD determine whether the account owner's server supports PEP; to do so, it MUST send a Service Discovery [19] information request to its own bare JID" Currently Gabble doesn't do that but query the *server* features to check if PEP is supported. This used to work fine but it seems that latest ejabberd doesn't advertise PEP in the server features any more. 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?). If we don't then there is a risk of race: a client could try to use a PEP feature and wrongly assume that the server doesn't support PEP because it hasn't been discovered yet. Also, if we don't we can't add OLPC specific interfaces once we discovered PEP because we can't change interfaces of a connected connection. On the other hand, we could say that we don't care because those interfaces are about to die. Another solution could be to always claim that we support those interfaces because it can still make sense to get OLPC info from your contacts even if you can't publish them yourself (that's what we do with the location interface).
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.