Bug 30094 - Have a convenience wrapper for wocky_porter_send_iq_async
Summary: Have a convenience wrapper for wocky_porter_send_iq_async
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Eitan Isaacson
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/ei...
Whiteboard: review+
Keywords:
Depends on:
Blocks: 29981
  Show dependency treegraph
 
Reported: 2010-09-08 14:29 UTC by Eitan Isaacson
Modified: 2010-09-27 13:28 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Eitan Isaacson 2010-09-08 14:29:00 UTC
Usually when sending an IQ we would want to have a reference to GabbleConnection in the callback. This means that GabbleConnection needs to be included almost every time in user_data either directly or through a structure.

A more straightforward solution would have GabbleConnection be the source object for the async call and wrap wocky_porter_send_iq_async.
Comment 1 Eitan Isaacson 2010-09-08 16:40:13 UTC
This branch adds utility functions for sending IQs. There are two alternative _finish functions, depending on whether you want the the XMPP error stanza, or an equivalent Telepathy error. It draws from Will's wocky_porter_send_iq_finish() function from his search-cleanup branch.
Comment 2 Simon McVittie 2010-09-09 02:51:29 UTC
> +  GError *error_ = NULL;

Call it wocky_error, please.

> +conn_util_send_iq_finish_harder (GabbleConnection *self,

conn_util_send_iq_finish_with_tp_error? But I think I'd actually prefer a single finish function with two GError** parameters, one for the Wocky error and one for the Telepathized error (bonus points if you convert to TpError lazily).
Comment 3 Eitan Isaacson 2010-09-15 10:56:27 UTC
oops, I missed this review earlier.

(In reply to comment #2)
> > +  GError *error_ = NULL;
> 
> Call it wocky_error, please.

Got rid of this function.

> 
> > +conn_util_send_iq_finish_harder (GabbleConnection *self,
> 
> conn_util_send_iq_finish_with_tp_error? But I think I'd actually prefer a
> single finish function with two GError** parameters, one for the Wocky error
> and one for the Telepathized error (bonus points if you convert to TpError
> lazily).

The downside to that kind of finish function would be that you don't get the mutually exclusive NULL pattern with error and res_pointer (you could get an xmpp stanza, and also errors).

Not sure what you mean by a lazy conversion, should I be checking for (GError **tp_error) != NULL before conversion? gabble_set_tp_error_from_wocky knows how to deal with that (although it probably could return sooner, and not pass to gabble_set_tp_conn_error_from_wocky if error == NULL).

Anyway, I added this in d1094e8. I hope the lazy "||" is not too hackish for you!
Comment 4 Will Thompson 2010-09-16 02:22:08 UTC
(In reply to comment #3)
> The downside to that kind of finish function would be that you don't get the
> mutually exclusive NULL pattern with error and res_pointer (you could get an
> xmpp stanza, and also errors).

Right, that was the idea with my original _finish_harder(): you don't have to care about the stanza at all if it has type='error'.

Simon, if you're super-super keen to only have one finish function, I would be in favour of making this function not return a stanza if an error was extracted from it. The advantage of having another function was that you could still use the _send() helper, even if you did want to do more interesting error handling.

How about for now we claim YAGNI, and make the single _finish() function return a non-NULL WockyStanza if and only if the reply wasn't type='error', and have a single GError ** parameter? Eitan, I think this would work for your code using this function. We can revisit this later if we need to.

> Not sure what you mean by a lazy conversion, should I be checking for (GError
> **tp_error) != NULL before conversion? gabble_set_tp_error_from_wocky knows how
> to deal with that (although it probably could return sooner, and not pass to
> gabble_set_tp_conn_error_from_wocky if error == NULL).
> 
> Anyway, I added this in d1094e8. I hope the lazy "||" is not too hackish for
> you!

I think this is lazy enough. If we wanted to make it lazier, the place to do that would be wocky_xmpp_error_extract(), which could short-circuit right after the 'if (type != NULL) {..}' block if none of its 'core', 'specialized', 'specialized_node' parameters are non-NULL.
Comment 5 Eitan Isaacson 2010-09-16 08:45:48 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > The downside to that kind of finish function would be that you don't get the
> > mutually exclusive NULL pattern with error and res_pointer (you could get an
> > xmpp stanza, and also errors).
> 
> Right, that was the idea with my original _finish_harder(): you don't have to
> care about the stanza at all if it has type='error'.
> 
> Simon, if you're super-super keen to only have one finish function, I would be
> in favour of making this function not return a stanza if an error was extracted
> from it. The advantage of having another function was that you could still use
> the _send() helper, even if you did want to do more interesting error handling.
> 
> How about for now we claim YAGNI, and make the single _finish() function return
> a non-NULL WockyStanza if and only if the reply wasn't type='error', and have a
> single GError ** parameter? Eitan, I think this would work for your code using
> this function. We can revisit this later if we need to.
> 

The only useful thing to me in this recent change has been changing the finish function to a boolean, so that receiving a WockyStanza is not mandatory. I think a typical use is sending an IQ and expecting an empty ack result. No need to get the stanza for that.
Comment 6 Eitan Isaacson 2010-09-16 09:05:30 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > The downside to that kind of finish function would be that you don't get the
> > > mutually exclusive NULL pattern with error and res_pointer (you could get an
> > > xmpp stanza, and also errors).
> > 
> > Right, that was the idea with my original _finish_harder(): you don't have to
> > care about the stanza at all if it has type='error'.
> > 
> > Simon, if you're super-super keen to only have one finish function, I would be
> > in favour of making this function not return a stanza if an error was extracted
> > from it. The advantage of having another function was that you could still use
> > the _send() helper, even if you did want to do more interesting error handling.
> > 
> > How about for now we claim YAGNI, and make the single _finish() function return
> > a non-NULL WockyStanza if and only if the reply wasn't type='error', and have a
> > single GError ** parameter? Eitan, I think this would work for your code using
> > this function. We can revisit this later if we need to.
> > 
> 
> The only useful thing to me in this recent change has been changing the finish
> function to a boolean, so that receiving a WockyStanza is not mandatory. I
> think a typical use is sending an IQ and expecting an empty ack result. No need
> to get the stanza for that.

Pushed, 209fa2d
Comment 7 Eitan Isaacson 2010-09-16 09:27:11 UTC
(In reply to comment #6)
> Pushed, 209fa2d

Forgot to free the GError, d43e83f.
Comment 8 Simon McVittie 2010-09-21 07:51:01 UTC
r+ for master, thanks!
Comment 9 Eitan Isaacson 2010-09-27 13:28:14 UTC
Thank, merged.


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.