[Spice-devel] [PATCH 02/14] channel-main: Use GTask instead of GSimpleAsyncResult

[Fabiano Fidêncio]

On Wed, Jan 20, 2016 at 2:39 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Wed, Jan 20, 2016 at 02:29:19PM +0100, Fabiano Fidêncio wrote:
>> On Wed, Jan 20, 2016 at 2:03 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> > On Wed, Jan 20, 2016 at 01:38:52PM +0100, Fabiano Fidêncio wrote:
>> >> On Mon, Jan 18, 2016 at 11:31 AM, Christophe Fergeau
>> >> <cfergeau at redhat.com> wrote:
>> >> > On Mon, Jan 18, 2016 at 10:05:38AM +0100, Fabiano Fidêncio wrote:
>> >> >> @@ -1794,15 +1785,15 @@ static void file_xfer_close_cb(GObject      *object,
>> >> >>
>> >> >>      /* Notify to user that files have been transferred or something error
>> >> >>         happened. */
>> >> >> -    res = g_simple_async_result_new(G_OBJECT(self->priv->channel),
>> >> >> -                                    self->priv->callback,
>> >> >> -                                    self->priv->user_data,
>> >> >> -                                    spice_main_file_copy_async);
>> >> >> +    task = g_task_new(self->priv->channel,
>> >> >> +                      self->priv->cancellable,
>> >> >> +                      self->priv->callback,
>> >> >> +                      self->priv->user_data);
>> >> >>      if (self->priv->error) {
>> >> >> -        g_simple_async_result_take_error(res, self->priv->error);
>> >> >> -        g_simple_async_result_set_op_res_gboolean(res, FALSE);
>> >> >> +        g_task_return_error(task, self->priv->error);
>> >> >> +        g_task_return_boolean(task, FALSE);
>> >> >
>> >> > Not sure what GTask behaviour will be if you queue these 2 calls. Just
>> >> > calling g_task_return_error() is going to be enough as
>> >> > g_task_propagate_boolean() returns FALSE when an error is set.
>> >>
>> >> Hmm. It shouldn't be in this way. I missed this part :-\
>> >> So, I do believe that in case of explicit errors we can call
>> >> g_task_return_boolean() instead of g_task_return_error() (as I did
>> >> with the other situations similar to this one).
>> >>
>> >> Is okay for you if I do the same here?
>> >
>> > I would do
>> >       if (self->priv->error) {
>> > -        g_simple_async_result_take_error(res, self->priv->error);
>> > -        g_simple_async_result_set_op_res_gboolean(res, FALSE);
>> > +        g_task_return_error(task, self->priv->error);
>> >
>> >
>> > it seems you are suggesting keeping the other one?
>> > (g_task_return_boolean).
>>
>>
>> Yep. At this point you already know that an error has occurred. That's
>> the reason I would prefer to have a g_task_return_boolean().
>>
>> Otherwise we could have something like:
>>
>> gboolean has_error = self->priv->error != NULL;
>> g_task_return_error(task, self->priv->error)
>> if (!has_error) {
>>    ...
>> }
>
> I don't really understand what you are getting at.
> The way I read the code is
> "if we have an error, then return from the async call and report an
> error, otherwise keep going or return success", and then
> g_propagate_boolean in the _finish() method is going to do the right
> thing for you, report an error if there was one, return a boolean
> otherwise.

What I was trying to say is that we have an if-else there, both using
g_task_return_boolean(task, FALSE/TRUE). Accepting your idea to use
g_task_return_error() we could change the code for something like
this:

ffidenci at cat ~/src/upstream/spice-gtk $ git diff src/channel-main.c
diff --git a/src/channel-main.c b/src/channel-main.c
index 6c0f238..52e08ce 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1770,6 +1770,7 @@ static void file_xfer_close_cb(GObject      *object,
     GTask *task;
     SpiceFileTransferTask *self;
     GError *error = NULL;
+    gboolean has_error;

     self = user_data;

@@ -1789,11 +1790,11 @@ static void file_xfer_close_cb(GObject      *object,
                       self->priv->cancellable,
                       self->priv->callback,
                       self->priv->user_data);
-    if (self->priv->error) {
-        g_task_return_error(task, self->priv->error);
-        g_task_return_boolean(task, FALSE);
-    } else {
-        g_task_return_boolean(task, TRUE);
+
+    has_error = self->priv->error != NULL;
+
+    g_task_return_error(taks, self->priv->error);
+    if (!has_error) {
         if (spice_util_get_debug()) {
             gint64 now = g_get_monotonic_time();
             gchar *basename = g_file_get_basename(self->priv->file);

Does it make sense for you?