Bug 30094

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.

> +  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).

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!

(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.

(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.

(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

(In reply to comment #6)
> Pushed, 209fa2d

Forgot to free the GError, d43e83f.

r+ for master, thanks!

Thank, merged.