The following branch refactor the PEP code and move it to a Wocky component. http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/pubsub
Can you get more than one <item> in an event? My reading of XEP-0060 suggests you might. We should probably handle that case by calling the handler callback n times? <http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=commitdiff;h=90cf6e92879906b62db2c2d03f336180e706c073>: :(. I think this is much less clear than the code it replaces; I can't see the content for the boilerplate. Ditto the following patch, to a slightly lesser extent, and make_publish_stanza(). Why does wocky_pubsub_register_event_handler return an int, rather than the PubsubEventHandler pointer (opaque, of course). + if (priv->handler_id != 0) + { + wocky_porter_unregister_handler (priv->porter, priv->handler_id); + priv->handler_id = 0; + } + + if (priv->porter != NULL) + g_object_unref (priv->porter); If priv->porter can be NULL, then the first block will break. But the id will be non-zero iff the porter is non-NULL, so that'll never happen. It'd be clearer as: if (priv->porter != NULL) { g_assert (priv->handler_id != 0); ... } I guess the fact that handlers on the porter return uints rather than opaque pointers is why register_event_handler() does that? If the WockyPubsub is disposed with outstanding requests, do the callbacks get called? • If so, is the DBusGMethodInvocation still valid if the GabbleConnection it came from has died? • If so, we're safe, because the callbacks so far (at least in d4ad9554b0d3) don't use the conn if the request failed, but it feels accidental. • If not, we crash. • If not, we leak. Asserting that priv->porter is non-NULL in wocky_pubsub_send_query_async seems wrong: surely client code could call this before the connection's connected? Does anything check the gboolean returned by gabble_pubsub_event_handler()? + /* TODO: subscribe to node if needed */ I wonder if it'd be worth having a wrapper around the changed cb which does the 'make a handle from the jid' stuff for you? I enjoy how this branch makes a WockyPubsub, and then towards the end stops directly using it. And then deletes it? What?!
(In reply to comment #1) > <http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=commitdiff;h=90cf6e92879906b62db2c2d03f336180e706c073>: > :(. I think this is much less clear than the code it replaces; I can't > see the content for the boilerplate. Ditto the following patch, to a > slightly lesser extent, and make_publish_stanza(). Yeah, I agree here. The reason I wrote _build() in the first place was to improve readability, but I think this variant squanders much of that advantage.
(In reply to comment #1) > Can you get more than one <item> in an event? My reading of XEP-0060 > suggests you might. We should probably handle that case by calling the > handler callback n times? Good point, but that shouldn't happen with PEP. Anyway, the new API doesn't have a callback anymore. > <http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=commitdiff;h=90cf6e92879906b62db2c2d03f336180e706c073>: > :(. I think this is much less clear than the code it replaces; I can't > see the content for the boilerplate. Well that's how Sjoerd implemented _build in Wocky. If you think the Gabble API is better, feel free to open a Wocky bug to discuss improvements. > Ditto the following patch, to a > slightly lesser extent, and make_publish_stanza(). Humm; I don't understand. What do you mean? > Why does wocky_pubsub_register_event_handler return an int, rather > than the PubsubEventHandler pointer (opaque, of course). This was inspired from the porter register API. Anyway, this function doesn't exist in WockyPepService any more. > + if (priv->handler_id != 0) > + { > + wocky_porter_unregister_handler (priv->porter, priv->handler_id); > + priv->handler_id = 0; > + } > + > + if (priv->porter != NULL) > + g_object_unref (priv->porter); > > If priv->porter can be NULL, then the first block will break. But > the id will be non-zero iff the porter is non-NULL, so that'll never > happen. It'd be clearer as: > > if (priv->porter != NULL) > { > g_assert (priv->handler_id != 0); > ... > } Good point; I did that. > I guess the fact that handlers on the porter return uints rather than > opaque pointers is why register_event_handler() does that? yep :) > If the WockyPubsub is disposed with outstanding requests, do the > callbacks get called? > • If so, is the DBusGMethodInvocation still valid if the > GabbleConnection it came from has died? > • If so, we're safe, because the callbacks so far (at least in > d4ad9554b0d3) don't use the conn if the request failed, but it > feels accidental. > • If not, we crash. > • If not, we leak. It can't. The GSimpleAsyncResult has a ref on the source object so it can't be disposed while the operation is running. > Asserting that priv->porter is non-NULL in > wocky_pubsub_send_query_async seems wrong: surely client code could > call this before the connection's connected? Function now raises an error if there is no porter. > Does anything check the gboolean returned by > gabble_pubsub_event_handler()? Yes; it tells to the stanza dispatcher if the handler actually handled the stanza or not. > + /* TODO: subscribe to node if needed */ I need the Wocky capabilities component to do that. Gabble still announce the +notify so it's fine for now. > I wonder if it'd be worth having a wrapper around the changed cb which > does the 'make a handle from the jid' stuff for you? Not sure. In the Brand fully Wockified Gabble World; Gabble should use contacts objects. > I enjoy how this branch makes a WockyPubsub, and then towards the end > stops directly using it. And then deletes it? What?! Yeah, that's because after discussion we Sjoerd we decided to have a separated API for PEP and for Pubsub. I didn't re-record the branch as most of the changes were still useful in the new version and I didn't want to fight conflicts of the dead. :)
(In reply to comment #3) > (In reply to comment #1) > > Can you get more than one <item> in an event? My reading of XEP-0060 > > suggests you might. We should probably handle that case by calling the > > handler callback n times? > > Good point, but that shouldn't happen with PEP. Anyway, the new API doesn't > have a callback anymore. Okay, sure. I guess it's something to bear in mind. > > <http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=commitdiff;h=90cf6e92879906b62db2c2d03f336180e706c073>: > > :(. I think this is much less clear than the code it replaces; I can't > > see the content for the boilerplate. > > Well that's how Sjoerd implemented _build in Wocky. If you think the Gabble API > is better, feel free to open a Wocky bug to discuss improvements. I'll do that. > > Ditto the following patch, to a > > slightly lesser extent, and make_publish_stanza(). > > Humm; I don't understand. What do you mean? They're just further examples. > > Why does wocky_pubsub_register_event_handler return an int, rather > > than the PubsubEventHandler pointer (opaque, of course). > > This was inspired from the porter register API. Anyway, this function doesn't > exist in WockyPepService any more. Okay, just wondered. > > If the WockyPubsub is disposed with outstanding requests, do the > > callbacks get called? > > • If so, is the DBusGMethodInvocation still valid if the > > GabbleConnection it came from has died? > > • If so, we're safe, because the callbacks so far (at least in > > d4ad9554b0d3) don't use the conn if the request failed, but it > > feels accidental. > > • If not, we crash. > > • If not, we leak. > > It can't. The GSimpleAsyncResult has a ref on the source object so it can't be > disposed while the operation is running. Okay; so further to real life discussions, we need to make sure that nothing will break if the callbacks fire after (eg) the GabbleConnection's dead. > > Asserting that priv->porter is non-NULL in > > wocky_pubsub_send_query_async seems wrong: surely client code could > > call this before the connection's connected? > > Function now raises an error if there is no porter. Looks good. > > Does anything check the gboolean returned by > > gabble_pubsub_event_handler()? > > Yes; it tells to the stanza dispatcher if the handler actually handled the > stanza or not. Okay. > > + /* TODO: subscribe to node if needed */ > > I need the Wocky capabilities component to do that. Gabble still announce the > +notify so it's fine for now. Okay. Guess we should file a bug, blocked by the capabilities component. > > I wonder if it'd be worth having a wrapper around the changed cb which > > does the 'make a handle from the jid' stuff for you? > > Not sure. In the Brand fully Wockified Gabble World; Gabble should use contacts > objects. I agree. In the meantime, helpers for handle↔jid conversions might be good. > > I enjoy how this branch makes a WockyPubsub, and then towards the end > > stops directly using it. And then deletes it? What?! > > Yeah, that's because after discussion we Sjoerd we decided to have a separated > API for PEP and for Pubsub. I didn't re-record the branch as most of the > changes were still useful in the new version and I didn't want to fight > conflicts of the dead. :) Okay, that's reasonable!
(In reply to comment #4) > > > If the WockyPubsub is disposed with outstanding requests, do the > > > callbacks get called? > > > • If so, is the DBusGMethodInvocation still valid if the > > > GabbleConnection it came from has died? > > > • If so, we're safe, because the callbacks so far (at least in > > > d4ad9554b0d3) don't use the conn if the request failed, but it > > > feels accidental. > > > • If not, we crash. > > > • If not, we leak. > > > > It can't. The GSimpleAsyncResult has a ref on the source object so it can't be > > disposed while the operation is running. > > Okay; so further to real life discussions, we need to make sure that > nothing will break if the callbacks fire after (eg) the > GabbleConnection's dead. Should be ok once my "iq-reply" branch is merged. Pending send IQ operations are garanteed to be completed when the other side closed his stream or when we force the closing of our connection. That implies that these operations will be completed before the disposing of the Gabble connection. > > > + /* TODO: subscribe to node if needed */ > > > > I need the Wocky capabilities component to do that. Gabble still announce the > > +notify so it's fine for now. > > Okay. Guess we should file a bug, blocked by the capabilities component. I opened #24133 and #24134. > > > I wonder if it'd be worth having a wrapper around the changed cb which > > > does the 'make a handle from the jid' stuff for you? > > > > Not sure. In the Brand fully Wockified Gabble World; Gabble should use contacts > > objects. > > I agree. In the meantime, helpers for handle↔jid conversions might be > good. Will add that.
(In reply to comment #5) > > > > I wonder if it'd be worth having a wrapper around the changed cb which > > > > does the 'make a handle from the jid' stuff for you? > > > > > > Not sure. In the Brand fully Wockified Gabble World; Gabble should use contacts > > > objects. > > > > I agree. In the meantime, helpers for handle↔jid conversions might be > > good. > > Will add that. I added ensure_bare_contact_from_jid (). A ensure_handle_from_bare_contact () helper wouldn't help us as the caller still have to unref the handler once it's done (and so needs a contact repo, etc).
Ship it.
Branch merged to master; thanks a lot for the review. I'll close the bug once the PEP code as actually be moved to Wocky.
Fixed in 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.