[Spice-devel] [PATCH spice-gtk 2/2] Add ability to get sizes from SpiceFileTransferTask
Jonathon Jongsma
jjongsma at redhat.com
Fri Aug 12 19:26:23 UTC 2016
On Fri, 2016-08-12 at 09:04 -0400, Frediano Ziglio wrote:
> >
> >
> > If a client is handling multiple SpiceFileTransferTasks at one
> > time,
> > it's not currently possible to provide a single overall progress to
> > the
> > user. The only information that the client can get is the
> > percentage
> > progress. This patch adds two new properties:
> > - total-bytes: the size of the file transfer task in bytes
> > - transferred-bytes: the number of bytes already transferred
> >
> > This allows a client UI to calculate the combined progress for all
> > ongoing transfer tasks and present it to the user. Two convenience
> > functions were added to retrieve these values:
> > - spice_file_transfer_task_get_total_bytes()
> > - spice_file_transfer_task_get_transferred_bytes()
> > ---
> > src/channel-main.c | 4 ++--
> > src/map-file | 2 ++
> > src/spice-file-transfer-task-priv.h | 2 --
> > src/spice-file-transfer-task.c | 48
> > +++++++++++++++++++++++++++++++++----
> > src/spice-file-transfer-task.h | 2 ++
> > src/spice-glib-sym-file | 2 ++
> > 6 files changed, 52 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 1ea0f71..419b0c2 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -2976,8 +2976,8 @@ static void
> > file_transfer_operation_task_finished(SpiceFileTransferTask
> > *xfer_ta
> > * remaining bytes from transfer-size in order to keep the
> > coherence
> > of
> > * the information we provide to the user (total-sent and
> > transfer-size)
> > * in the progress-callback */
> > - guint64 file_size =
> > spice_file_transfer_task_get_file_size(xfer_task);
> > - guint64 bytes_read =
> > spice_file_transfer_task_get_bytes_read(xfer_task);
> > + guint64 file_size =
> > spice_file_transfer_task_get_total_bytes(xfer_task);
> > + guint64 bytes_read =
> > spice_file_transfer_task_get_transferred_bytes(xfer_task);
> > xfer_op->stats.transfer_size -= (file_size - bytes_read);
> > if (g_error_matches(error, G_IO_ERROR,
> > G_IO_ERROR_CANCELLED)) {
> > xfer_op->stats.cancelled++;
>
> I got a bit confused about this rename. Looks like
> you publish them changing the names.
Yes, these functions were previously internal functions, but I
converted them to public API. But I didn't think that the existing
names made sense from a public API point-of-view. "bytes_read" is not
very self-explanatory. It means that we've read these bytes from the
file on disk and then sent them to the agent. So I decided to change
that to "transferred_bytes". And I wanted these two functions to use
similar naming conventions, so also renamed the other one to match:
total_bytes()
transferred_bytes()
vs
file_size()
bytes_read()
Perhaps there are better names though, feel free to propose something
better.
>
> >
> > diff --git a/src/map-file b/src/map-file
> > index 8618f6e..3d92153 100644
> > --- a/src/map-file
> > +++ b/src/map-file
> > @@ -37,6 +37,8 @@ spice_display_set_grab_keys;
> > spice_file_transfer_task_cancel;
> > spice_file_transfer_task_get_filename;
> > spice_file_transfer_task_get_progress;
> > +spice_file_transfer_task_get_total_bytes;
> > +spice_file_transfer_task_get_transferred_bytes;
> > spice_file_transfer_task_get_type;
> > spice_get_option_group;
> > spice_gl_scanout_free;
> > diff --git a/src/spice-file-transfer-task-priv.h
> > b/src/spice-file-transfer-task-priv.h
> > index a7b58d8..253b3c5 100644
> > --- a/src/spice-file-transfer-task-priv.h
> > +++ b/src/spice-file-transfer-task-priv.h
> > @@ -50,8 +50,6 @@ gssize
> > spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
> > GAsyncResult *result,
> > char **buffer,
> > GError **error);
> > -guint64
> > spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> > *self);
> > -guint64
> > spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask
> > *self);
> > gboolean
> > spice_file_transfer_task_is_completed(SpiceFileTransferTask *self);
> >
> > G_END_DECLS
> > diff --git a/src/spice-file-transfer-task.c b/src/spice-file-
> > transfer-task.c
> > index e7f50da..c414e40 100644
> > --- a/src/spice-file-transfer-task.c
> > +++ b/src/spice-file-transfer-task.c
> > @@ -73,6 +73,8 @@ enum {
> > PROP_TASK_CHANNEL,
> > PROP_TASK_CANCELLABLE,
> > PROP_TASK_FILE,
> > + PROP_TASK_TOTAL_BYTES,
> > + PROP_TASK_TRANSFERRED_BYTES,
> > PROP_TASK_PROGRESS,
> > };
> >
> > @@ -152,6 +154,7 @@ static void
> > spice_file_transfer_task_query_info_cb(GObject *obj,
> >
> > /* SpiceFileTransferTask's init is done, handshake for file-
> > transfer
> > will
> > * start soon. First "progress" can be emitted ~ 0% */
> > + g_object_notify(G_OBJECT(self), "total-bytes");
> > g_object_notify(G_OBJECT(self), "progress");
> >
> > g_task_return_pointer(task, info, g_object_unref);
> > @@ -238,6 +241,7 @@ static void
> > spice_file_transfer_task_read_stream_cb(GObject *source_object,
> > }
> > }
> >
> > + g_object_notify(G_OBJECT(self), "transferred-bytes");
> > g_task_return_int(task, nbytes);
> > g_object_unref(task);
> > }
> > @@ -349,15 +353,13 @@ GCancellable
> > *spice_file_transfer_task_get_cancellable(SpiceFileTransferTask *se
> > return self->cancellable;
> > }
> >
> > -G_GNUC_INTERNAL
> > -guint64
> > spice_file_transfer_task_get_file_size(SpiceFileTransferTask *self)
> > +guint64
> > spice_file_transfer_task_get_total_bytes(SpiceFileTransferTask
> > *self)
> > {
> > g_return_val_if_fail(self != NULL, 0);
> > return self->file_size;
> > }
> >
> > -G_GNUC_INTERNAL
> > -guint64
> > spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask
> > *self)
> > +guint64
> > spice_file_transfer_task_get_transferred_bytes(SpiceFileTransferTas
> > k
> > *self)
> > {
> > g_return_val_if_fail(self != NULL, 0);
> > return self->read_bytes;
>
> Here however the internal variables are called file_size and
> read_bytes.
> So are the variable name misleading? Why you use different names?
I just left the variables names as they were because they are still
accurate from an internal library perspective. But I could rename the
variables if you'd like.
>
> >
> > @@ -570,6 +572,12 @@ spice_file_transfer_task_get_property(GObject
> > *object,
> > case PROP_TASK_FILE:
> > g_value_set_object(value, self->file);
> > break;
> > + case PROP_TASK_TOTAL_BYTES:
> > + g_value_set_double(value,
> > spice_file_transfer_task_get_total_bytes(self));
> > + break;
> > + case PROP_TASK_TRANSFERRED_BYTES:
> > + g_value_set_double(value,
> > spice_file_transfer_task_get_transferred_bytes(self));
> > + break;
> > case PROP_TASK_PROGRESS:
> > g_value_set_double(value,
> > spice_file_transfer_task_get_progress(self));
> > break;
> > @@ -714,6 +722,38 @@
> > spice_file_transfer_task_class_init(SpiceFileTransferTaskClass
> > *klass)
> > G_PARAM_ST
> > ATIC_STRINGS));
> >
> > /**
> > + * SpiceFileTransferTask:total-bytes:
> > + *
> > + * The total size in bytes of this file transfer.
> > + *
> > + * Since: 0.33
> > + **/
> > + g_object_class_install_property(object_class,
> > PROP_TASK_TOTAL_BYTES,
> > + g_param_spec_uint64("total-
> > bytes",
> > + "Total
> > bytes",
> > + "The size
> > in bytes
> > of the file transferred",
> > + 0,
> > G_MAXUINT64, 0,
> > + G_PARAM_RE
> > ADABLE |
> > +
> > G_PARAM_STATIC_STRINGS));
> > +
> > +
> > + /**
> > + * SpiceFileTransferTask:transferred-bytes:
> > + *
> > + * The number of bytes that have been transferred so far.
> > + *
> > + * Since: 0.33
> > + **/
> > + g_object_class_install_property(object_class,
> > PROP_TASK_TRANSFERRED_BYTES,
> > + g_param_spec_uint64("transferr
> > ed-bytes",
> > + "Transferr
> > ed bytes",
> > + "The
> > number of bytes
> > transferred",
> > + 0,
> > G_MAXUINT64, 0,
> > + G_PARAM_RE
> > ADABLE |
> > +
> > G_PARAM_STATIC_STRINGS));
> > +
> > +
> > + /**
> > * SpiceFileTransferTask:progress:
> > *
> > * The current state of the file transfer. This value
> > indicates a
> > diff --git a/src/spice-file-transfer-task.h b/src/spice-file-
> > transfer-task.h
> > index 4f179fb..d04f0ef 100644
> > --- a/src/spice-file-transfer-task.h
> > +++ b/src/spice-file-transfer-task.h
> > @@ -43,6 +43,8 @@ GType spice_file_transfer_task_get_type(void)
> > G_GNUC_CONST;
> >
> > char* spice_file_transfer_task_get_filename(SpiceFileTransferTask
> > *self);
> > void spice_file_transfer_task_cancel(SpiceFileTransferTask *self);
> > +guint64
> > spice_file_transfer_task_get_total_bytes(SpiceFileTransferTask
> > *self);
> > +guint64
> > spice_file_transfer_task_get_transferred_bytes(SpiceFileTransferTas
> > k
> > *self);
> > double spice_file_transfer_task_get_progress(SpiceFileTransferTask
> > *self);
> >
> > G_END_DECLS
> > diff --git a/src/spice-glib-sym-file b/src/spice-glib-sym-file
> > index 7d2af60..473c5ca 100644
> > --- a/src/spice-glib-sym-file
> > +++ b/src/spice-glib-sym-file
> > @@ -26,6 +26,8 @@ spice_display_gl_draw_done
> > spice_file_transfer_task_cancel
> > spice_file_transfer_task_get_filename
> > spice_file_transfer_task_get_progress
> > +spice_file_transfer_task_get_total_bytes
> > +spice_file_transfer_task_get_transferred_bytes
> > spice_file_transfer_task_get_type
> > spice_get_option_group
> > spice_gl_scanout_free
>
> Otherwise the patch looks good however I cannot see
> the entire design, I'm not expert on this APIs usage.
I will send a virt-viewer patch soon that uses this API so that you can
evaluate it more easily.
>
> Frediano
More information about the Spice-devel
mailing list