Re: [Evolution-hackers] Heads Up: More Camel API breakage in 2.31

2010-07-13 Thread Christian Hilberg
On Tuesday 13 July 2010 Matthew Barnes wrote:
> On Tue, 2010-07-13 at 18:50 +0200, Christian Hilberg wrote:
> > Is there a sensible way how we could deal with CamelException in our
> > evolution-kolab plugin, which will be developed against 2.30? This is,
> > can CamelException be wrapped in a way which will ease up the transition
> > to GError, once our plugin will be ported to 2.31?
> There are a few habits to follow with CamelException which will ease the
> transition to GError:
> [...]

Thanks, note taken. We'll try to catch you on every similar move. :)

Greetings,

Christian

-- 
kernel concepts GbRTel: +49-271-771091-14
Sieghuetter Hauptweg 48Fax: +49-271-771091-19
D-57072 Siegen
http://www.kernelconcepts.de/


signature.asc
Description: This is a digitally signed message part.
___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Heads Up: More Camel API breakage in 2.31

2010-07-13 Thread Matthew Barnes
On Tue, 2010-07-13 at 18:50 +0200, Christian Hilberg wrote:
> Is there a sensible way how we could deal with CamelException in our 
> evolution-kolab plugin, which will be developed against 2.30? This is, can 
> CamelException be wrapped in a way which will ease up the transition to 
> GError, once our plugin will be ported to 2.31?

There are a few habits to follow with CamelException which will ease the
transition to GError:


  - A CamelException instance allows you to set multiple errors on it
such that new errors silently overwrite old errors.  This is evil,
and I spent a lot of time cleaning up old code that relied on it.

Check for an error and react accordingly after -every- step that
could fail.  Don't do a bunch of steps and then check for error. 
This is good programming practice anyway.


  - By convention, passing a GError is always optional.  Not so with
CamelException.  This gets you into trouble when you have to examine
the CamelException for specific error codes: you can count on the
caller passing you a CamelException, you CAN'T count on the caller
passing you a GError.

So while it may be tempting to use the caller's CamelException
directly, do this instead: pass a locally allocated CamelException
to the failable function, examine -that- for specific error codes,
and if necessary propagate it to the CamelException that was passed
in by the caller.  That's the pattern you would use with GError.

So for example:

gboolean
do_something (CamelFolder *folder,
  CamelException *ex)
{
CamelException local_ex;

camel_exception_init (&local_ex);

do_something_that_can_fail (folder, &local_ex);

/* With CamelException, you could safely use and examine
 * "ex" directly, since "ex" should never be NULL. */

if (camel_exception_get_id (&local_ex) == ERROR_CODE)
... do some damage control ...

/* Propagate the error to the caller's CamelException. */
if (camel_exception_is_set (&local_ex)) {
camel_exception_xfer (ex, &local_ex);
return FALSE;
}

return TRUE;
}

Equivalent function with GError:

gboolean
do_something (CamelFolder *folder,
  GError **error)
{
GError *local_error = NULL;

do_something_that_can_fail (folder, &local_error);

/* With GError, "error" might be NULL.  So we need our
 * own local GError that we know is safe to examine. */

if (g_error_matches (local_error, ERROR_DOMAIN, ERROR_CODE))
... do some damage control ...

/* Propagate the error to the caller's GError. */
if (local_error != NULL) {
g_propagate_error (error, local_error);
return FALSE;
}

return TRUE;
}

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Heads Up: More Camel API breakage in 2.31

2010-07-13 Thread Christian Hilberg
Hi,

On Thursday 08 July 2010 Matthew Barnes wrote:
> I finally finished converting Camel's error handling code to GError.
> CamelException is no more.

Is there a sensible way how we could deal with CamelException in our 
evolution-kolab plugin, which will be developed against 2.30? This is, can 
CamelException be wrapped in a way which will ease up the transition to 
GError, once our plugin will be ported to 2.31?

Best regards,

Christian

-- 
kernel concepts GbRTel: +49-271-771091-14
Sieghuetter Hauptweg 48Fax: +49-271-771091-19
D-57072 Siegen
http://www.kernelconcepts.de/


signature.asc
Description: This is a digitally signed message part.
___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Heads Up: More Camel API breakage in 2.31

2010-07-11 Thread David Woodhouse
On Sun, 2010-07-11 at 09:05 -0400, Matthew Barnes wrote:
> Instead I saw this cute stunt in quite a few
> places:
> 
> CamelException ex;
> 
> camel_exception_init (&ex);
> do_something_that_can_fail (folder, message, &ex);
> camel_exception_clear (&ex); 

Hey, at least you didn't see this:

 imapx_server_fetch_new_messages (is, is->select_pending, TRUE, TRUE,
  /* FIXME */ camel_exception_new());

Now you've committed the GError conversion, I can commit a sane version
of that. Thanks :)

-- 
dwmw2

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Heads Up: More Camel API breakage in 2.31

2010-07-11 Thread Matthew Barnes
On Sun, 2010-07-11 at 13:08 +0100, David Woodhouse wrote:
> I note that in some places you've elected not to propagate the error
> pointer. For example in imapx_parse_status_info(): 
> 
> -imapx_parse_status_info (struct _CamelIMAPXStream *is, CamelException *ex)
> +imapx_parse_status_info (struct _CamelIMAPXStream *is, GError **error)
> -   sinfo->messages = camel_imapx_stream_number 
> (is, ex);
> +   sinfo->messages = camel_imapx_stream_number 
> (is, NULL);
> ... etc.

I already talked to David about this on IRC but also answering here for
posterity.  I elected not to pass a GError where I saw inadequate error
checking in the logic.  In the example above the function was returning
a non-NULL "_state_info" struct whether an error got set or not, and the
calling logic treats a non-NULL return value as successful.  So if a
GError were set there it would probably have leaked.

Another scenario: Camel used to complain with a runtime warning if you
passed NULL to its CamelException functions, presumably to help enforce
proper error checking.  Instead I saw this cute stunt in quite a few
places:

CamelException ex;

camel_exception_init (&ex);
do_something_that_can_fail (folder, message, &ex);
camel_exception_clear (&ex);

... assume success and carry on ...

Those cases got replaced with a NULL GError too.

I wasn't setting out to fix all our bad error handling -- that would've
been too overwhelming.  But I should have at least marked those places
with a FIXME comment so we can grep them easily and go back and clean
them up.  I will reexamine my patches and add comments so those places
aren't lost.


___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Heads Up: More Camel API breakage in 2.31

2010-07-11 Thread David Woodhouse
On Thu, 2010-07-08 at 11:58 -0400, Matthew Barnes wrote:
> I finally finished converting Camel's error handling code to GError.
> CamelException is no more.  I plan to push this later today after a bit
> more testing.  It will debut in 2.31.5 along with another libcamel
> soname increment.

I note that in some places you've elected not to propagate the error
pointer. For example in imapx_parse_status_info(): 

-imapx_parse_status_info (struct _CamelIMAPXStream *is, CamelException *ex)
+imapx_parse_status_info (struct _CamelIMAPXStream *is, GError **error)
-   sinfo->messages = camel_imapx_stream_number 
(is, ex);
+   sinfo->messages = camel_imapx_stream_number 
(is, NULL);
... etc.

Why is that? Don't we need to ensure that these subfunction calls return
errors properly? Previously, if they failed to parse the element they're
supposed to parse (a number in this case), they'd set the exception
which would eventually get noticed (although I would have thought the
old code _should_ have included !camel_exception_is_set() in the loop
conditions, as other loops did). But now, it just gets lost.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


[Evolution-hackers] Heads Up: More Camel API breakage in 2.31

2010-07-08 Thread Matthew Barnes
I finally finished converting Camel's error handling code to GError.
CamelException is no more.  I plan to push this later today after a bit
more testing.  It will debut in 2.31.5 along with another libcamel
soname increment.

As boring an announcement as this is, it's actually pretty important in
moving Camel closer to embracing GIO.  To that end, failable disk and
network I/O functions in Camel will now set G_IO_ERRORs [1].  Also, for
cancellations, G_IO_ERROR_CANCELLED replaces CAMEL_ERROR_USER_CANCEL.

Next step is introduce GCancellable into Camel's blocking API.  But
that's a much more disruptive change and I'm not sure if that will land
in time for 3.0, as I need to turn my attention to other higher priority
projects for awhile.

Matthew Barnes


[1] http://library.gnome.org/devel/gio/stable/gio-GIOError.html



___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers