GNOME Bugzilla – Bug 575900
Async version of gdk_pixbuf_new_from_stream
Last modified: 2010-12-13 13:26:23 UTC
It would be nice to have an async version of gdk_pixbuf_new_from_stream and gdk_pixbuf_new_from_stream_at_scale. Patch coming up.
Created attachment 130929 [details] [review] Add async gdk_pixbuf_new_from_stream methods Adds gdk_pixbuf_new_from_stream[_at_scale]_async, gdk_pixbuf_new_from_stream_finish, as well as gdk_pixbuf_save_to_stream_async and gdk_pixbuf_save_to_stream_finish.
A few style related comments, no in-depth review: +static void +new_from_stream_thread (GSimpleAsyncResult *result, + GInputStream *stream, + GCancellable *cancellable) +{ + GdkPixbuf *pixbuf; + AtScaleData *data; Please use 2 spaces for intendation. + /* Set the new pixbuf as the result, or error out */ + if (pixbuf == NULL) { + g_simple_async_result_set_from_error (result, error); + g_error_free (error); + } else { + g_simple_async_result_set_op_res_gpointer (result, pixbuf, g_object_unref); + } The "if" blocks should look like so: if (pixbuf == NULL) { ... } else { ... } + * Since: 2.18 Too late for 2.18 unfortunately, so this should be 2.20. gdk_pixbuf_new_from_stream_at_scale_async is missing a g_return_if_fail (!cancellable || G_IS_CANCELLABLE ()). gdk_pixbuf_new_from_stream_finish should have g_return_val_if_fail (!error || (error && !*error)).
> Please use 2 spaces for intendation. No, this it gdk-pixbuf, which for historic reasons uses a different style. > Too late for 2.18 unfortunately, so this should be 2.20. No, we just released 2.16. 2.18 is still a possibility
(In reply to comment #3) > > Please use 2 spaces for intendation. > > No, this it gdk-pixbuf, which for historic reasons uses a different style. > > > Too late for 2.18 unfortunately, so this should be 2.20. > > No, we just released 2.16. 2.18 is still a possibility Yikes, was somehow thinking of Glib there, thanks for the correction. And yes, GdkPixbuf's special style apparentely still confuses me.
Created attachment 132077 [details] [review] Add async gdk_pixbuf_new_from_stream methods (updated) This adds the missing checks to all relevant new functions.
Ping? This is needed for Totem 2.27.
There is nothing in this that you cannot do in totem itself.
That is true, but at the moment we just have this code copied-and-pasted. Perhaps I should've said "this would be nice for Totem 2.27". :)
Would be nice to have in GTK+ 3.x Any comments?
Comment on attachment 132077 [details] [review] Add async gdk_pixbuf_new_from_stream methods (updated) Some little comments: From a style point of view the patch should be reworked: Do not use tabs and use 2 spaces for indentation. Also, update the Since tag to 'Since: 3.0'
(In reply to comment #10) > (From update of attachment 132077 [details] [review]) > Some little comments: From a style point of view the patch should be reworked: > Do not use tabs and use 2 spaces for indentation. See comment 3. > Also, update the Since tag to 'Since: 3.0' Right, easy fix.
Created attachment 176192 [details] [review] Async version of gdk_pixbuf_new_from_stream Add async versions of gdk_pixbuf_new_from_stream and gdk_pixbuf_new_from_stream_at_scale.
Review of attachment 176192 [details] [review]: looks generally good to me. ::: gdk-pixbuf/gdk-pixbuf-io.c @@ +1621,3 @@ + return NULL; + + pixbuf = GDK_PIXBUF (g_simple_async_result_get_op_res_gpointer (result)); it's friday, and I might be on crack, but doesn't: object = SAFE_TYPE_CAST (function_that_might_return_null ()); will warn/assert if null is actually returned? since this line is followed by a NULL check and there's a GError involved as well, I'd probably skip the safe cast. also, the return value for get_op_res_gpointer is a gpointer, there's no actual need for a cast (and a type-safe case at that).
Created attachment 176196 [details] [review] Async version of gdk_pixbuf_new_from_stream Add async versions of gdk_pixbuf_new_from_stream and gdk_pixbuf_new_from_stream_at_scale.
Review of attachment 176196 [details] [review]: It's amazing what problems one finds when looking at one's own code again a while later. If you don't want to do another iteration, I will. ::: gdk-pixbuf/gdk-pixbuf-io.c @@ +1462,3 @@ + pixbuf = gdk_pixbuf_new_from_stream (stream, cancellable, &error); + + g_free (data); /* GSimpleAsyncResult doesn't destroy result pointers when setting a new value over the top */ That's not true any more; this was originally to work around a bug in GSimpleAsyncResult. @@ +1470,3 @@ + g_error_free (error); + } else { + g_simple_async_result_set_op_res_gpointer (result, pixbuf, g_object_unref); Wouldn't it be tidier to g_object_ref(pixbuf) here instead of in gdk_pixbuf_new_from_stream_finish()? That would mean the ref/unref are next to each other, and gdk_pixbuf_new_from_stream_finish() would return the original reference created by gdk_pixbuf_new_from_stream(). @@ +2610,3 @@ + g_strfreev (data->values); + g_free (data->type); + g_free (data); Shouldn't data->stream be unreffed here too? @@ +2632,3 @@ + &error); + + save_to_stream_async_data_free (data); /* GSimpleAsyncResult doesn't destroy result pointers when setting a new value over the top */ Again, this shouldn't be called. @@ +2688,3 @@ + va_end (args); + + data = g_new (SaveToStreamAsyncData, 1); Use g_slice_new()?
Attachment 176196 [details] pushed as 92e27ca - Async version of gdk_pixbuf_new_from_stream
(In reply to comment #15) > Review of attachment 176196 [details] [review]: > > It's amazing what problems one finds when looking at one's own code again a > while later. If you don't want to do another iteration, I will. > > ::: gdk-pixbuf/gdk-pixbuf-io.c > @@ +1462,3 @@ > + pixbuf = gdk_pixbuf_new_from_stream (stream, cancellable, &error); > + > + g_free (data); /* GSimpleAsyncResult doesn't destroy result pointers when > setting a new value over the top */ > > That's not true any more; this was originally to work around a bug in > GSimpleAsyncResult. Removed. > @@ +1470,3 @@ > + g_error_free (error); > + } else { > + g_simple_async_result_set_op_res_gpointer (result, pixbuf, > g_object_unref); > > Wouldn't it be tidier to g_object_ref(pixbuf) here instead of in > gdk_pixbuf_new_from_stream_finish()? That would mean the ref/unref are next to > each other, and gdk_pixbuf_new_from_stream_finish() would return the original > reference created by gdk_pixbuf_new_from_stream(). Done. > @@ +2610,3 @@ > + g_strfreev (data->values); > + g_free (data->type); > + g_free (data); > > Shouldn't data->stream be unreffed here too? Done. > @@ +2632,3 @@ > + &error); > + > + save_to_stream_async_data_free (data); /* GSimpleAsyncResult doesn't > destroy result pointers when setting a new value over the top */ > > Again, this shouldn't be called. Done. > @@ +2688,3 @@ > + va_end (args); > + > + data = g_new (SaveToStreamAsyncData, 1); > > Use g_slice_new()? Done. commit 8f4cf69dbc6e07289d18dd1e516b39ce178617d5 Author: Bastien Nocera <hadess@hadess.net> Date: Mon Dec 13 12:26:13 2010 +0000 Fixes from review for new async functions Correct review comments by Philip Withnall in: https://bugzilla.gnome.org/show_bug.cgi?id=575900
+ g_simple_async_result_set_from_error (result, error); + g_error_free (error); There's now g_simple_async_result_take_error() for this construct.
(In reply to comment #18) > + g_simple_async_result_set_from_error (result, error); > + g_error_free (error); > > There's now g_simple_async_result_take_error() for this construct. Done.