On Mon, 2016-05-30 at 11:54 +0200, Victor Toso wrote:
> By splitting file_xfer_send_start_msg_async we can separate in three
> different steps the spice_main_file_copy_async function:
>
> 1-) Creating tasks with spice_file_transfer_task_create_tasks which
> now returns a GList of SpiceFileTransferTask;
> 2-) Setting handlers before the SpiceFileTransferTask starts;
> 3-) Starting the task with spice_file_transfer_task_start_task
>
> (1) and (3) can be done outside channel-main scope.
>
> This change is related to split SpiceFileTransferTask from
> channel-main.
> ---
> src/channel-main.c | 150 +++++++++++++++++++++++++++++++---------------------
> -
> 1 file changed, 88 insertions(+), 62 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 702f146..7843aeb 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -62,6 +62,17 @@ static guint32
> spice_file_transfer_task_get_id(SpiceFileTransferTask *self);
> static SpiceMainChannel
> *spice_file_transfer_task_get_channel(SpiceFileTransferTask *self);
> static GCancellable
> *spice_file_transfer_task_get_cancellable(SpiceFileTransferTask *self);
> static void spice_file_transfer_task_flush_done(SpiceFileTransferTask *self,
> GError *error);
> +static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel
> *channel,
> + GFile **files,
> + GFileCopyFlags flags,
> + GCancellable
> *cancellable,
> + GFileProgressCallback
> progress_callback,
> + gpointer
> progress_callback_data,
> + SpiceFileTransferTaskFlus
> hCb flush_callback,
> + gpointer
> flush_callback_data,
> + GAsyncReadyCallback
> callback,
> + gpointer user_data);
I think I would prefer to re-order the arguments of this function. Generally the
first argument of the function should be the most important one (e.g. the 'self'
argument to a class method). In this case, since the function creates multiple
tasks for a series of files, I think that perhaps the array of files should be
the first argument. Just a personal preference thing, but I don't care too much.
> +static void spice_file_transfer_task_start_task(SpiceFileTransferTask *self);
>
> /**
> * SECTION:file-transfer-task
> @@ -3066,59 +3077,9 @@ static void task_finished(SpiceFileTransferTask *task,
> gpointer data)
> {
> SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
> - g_hash_table_remove(channel->priv->file_xfer_tasks,
> GUINT_TO_POINTER(task->id));
> -}
> -
> -static void file_xfer_send_start_msg_async(SpiceMainChannel *channel,
> - GFile **files,
> - GFileCopyFlags flags,
> - GCancellable *cancellable,
> - GFileProgressCallback
> progress_callback,
> - gpointer progress_callback_data,
> - SpiceFileTransferTaskFlushCb
> flush_callback,
> - gpointer flush_callback_data,
> - GAsyncReadyCallback callback,
> - gpointer user_data)
> -{
> - SpiceMainChannelPrivate *c = channel->priv;
> - SpiceFileTransferTask *task;
> - gint i;
> -
> - for (i = 0; files[i] != NULL && !g_cancellable_is_cancelled(cancellable);
> i++) {
> - GCancellable *task_cancellable = cancellable;
> - /* if a cancellable object was not provided for the overall
> operation,
> - * create a separate object for each file so that they can be
> cancelled
> - * separately */
> - if (!task_cancellable)
> - task_cancellable = g_cancellable_new();
> + guint32 task_id = spice_file_transfer_task_get_id(task);
>
> - task = spice_file_transfer_task_new(channel, files[i],
> task_cancellable);
> - task->flags = flags;
> - task->progress_callback = progress_callback;
> - task->progress_callback_data = progress_callback_data;
> - task->flush_callback = flush_callback;
> - task->flush_callback_data = flush_callback_data;
> - task->callback = callback;
> - task->user_data = user_data;
> -
> - CHANNEL_DEBUG(channel, "Insert a xfer task:%d to task list", task-
> >id);
> - g_hash_table_insert(c->file_xfer_tasks,
> - GUINT_TO_POINTER(task->id),
> - task);
> - g_signal_connect(task, "finished", G_CALLBACK(task_finished),
> channel);
> - g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> task);
> -
> - g_file_read_async(files[i],
> - G_PRIORITY_DEFAULT,
> - cancellable,
> - file_xfer_read_async_cb,
> - g_object_ref(task));
> - task->pending = TRUE;
> -
> - /* if we created a per-task cancellable above, free it */
> - if (!cancellable)
> - g_object_unref(task_cancellable);
> - }
> + g_hash_table_remove(channel->priv->file_xfer_tasks,
> GUINT_TO_POINTER(task_id));
> }
>
> /**
> @@ -3164,6 +3125,7 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
> gpointer user_data)
> {
> SpiceMainChannelPrivate *c = channel->priv;
> + GList *tasks, *it;
>
> g_return_if_fail(channel != NULL);
> g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> @@ -3180,16 +3142,26 @@ void spice_main_file_copy_async(SpiceMainChannel
> *channel,
> return;
> }
>
> - file_xfer_send_start_msg_async(channel,
> - sources,
> - flags,
> - cancellable,
> - progress_callback,
> - progress_callback_data,
> - file_xfer_flush_callback,
> - NULL,
> - callback,
> - user_data);
> + tasks = spice_file_transfer_task_create_tasks(channel,
> + sources,
> + flags,
> + cancellable,
> + progress_callback,
> + progress_callback_data,
> + file_xfer_flush_callback,
> + NULL,
> + callback,
> + user_data);
> + for (it = tasks; it != NULL; it = it->next) {
> + SpiceFileTransferTask *task = SPICE_FILE_TRANSFER_TASK(it->data);
> + guint32 task_id = spice_file_transfer_task_get_id(task);
> +
> + g_hash_table_insert(c->file_xfer_tasks, GUINT_TO_POINTER(task_id),
> task);
> + g_signal_connect(task, "finished", G_CALLBACK(task_finished),
> channel);
> + g_signal_emit(channel, signals[SPICE_MAIN_NEW_FILE_TRANSFER], 0,
> task);
> + spice_file_transfer_task_start_task(task);
> + }
> + g_list_free(tasks);
> }
>
> /**
> @@ -3282,6 +3254,60 @@ static void
> spice_file_transfer_task_flush_done(SpiceFileTransferTask *self, GEr
> file_xfer_continue_read(self);
> }
>
> +static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel
> *channel,
> + GFile **files,
> + GFileCopyFlags flags,
> + GCancellable
> *cancellable,
> + GFileProgressCallback
> progress_callback,
> + gpointer
> progress_callback_data,
> + SpiceFileTransferTaskFlus
> hCb flush_callback,
> + gpointer
> flush_callback_data,
> + GAsyncReadyCallback
> callback,
> + gpointer user_data)
> +{
> + SpiceFileTransferTask *task;
> + gint i;
> + GList *tasks = NULL;
> +
> + for (i = 0; files[i] != NULL && !g_cancellable_is_cancelled(cancellable);
> i++) {
> + GCancellable *task_cancellable = cancellable;
> + /* if a cancellable object was not provided for the overall
> operation,
> + * create a separate object for each file so that they can be
> cancelled
> + * separately */
> + if (!task_cancellable)
> + task_cancellable = g_cancellable_new();
> +
> + task = spice_file_transfer_task_new(channel, files[i],
> task_cancellable);
> + task->flags = flags;
> + task->progress_callback = progress_callback;
> + task->progress_callback_data = progress_callback_data;
> + task->flush_callback = flush_callback;
> + task->flush_callback_data = flush_callback_data;
> + task->callback = callback;
> + task->user_data = user_data;
> +
> + SPICE_DEBUG("Insert a xfer task:%d to task list", task->id);
> + tasks = g_list_prepend(tasks, task);
> +
> + /* if we created a per-task cancellable above, free it */
> + if (!cancellable)
> + g_object_unref(task_cancellable);
> + }
> +
> + return g_list_reverse(tasks);
> +}
> +
> +static void spice_file_transfer_task_start_task(SpiceFileTransferTask *self)
> +{
> + g_return_if_fail(self != NULL);
> + self->pending = TRUE;
> + g_file_read_async(self->file,
> + G_PRIORITY_DEFAULT,
> + self->cancellable,
> + file_xfer_read_async_cb,
> + g_object_ref(self));
> +}
> +
> static void
> spice_file_transfer_task_get_property(GObject *object,
> guint property_id,
Otherwise it looks OK
Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel