[virt-tools-list] [PATCH virt-viewer v3] RFC: Simplify file transfer dialog UI
Jonathon Jongsma
jjongsma at redhat.com
Tue Aug 16 20:26:56 UTC 2016
On Tue, 2016-08-16 at 18:10 +0200, Victor Toso wrote:
> Hi,
>
> On Tue, Aug 16, 2016 at 10:30:57AM -0500, Jonathon Jongsma wrote:
> >
> > When transferring a large number of files, the file transfer dialog
> > was unusable because the window size would be larger than the
> > client
> > desktop. To solve this, remove the list of individual files (and
> > the
> > ability to cancel each file transfer independantly) and only
> > display a
> > single overall progress bar that shows the status of all ongoing
> > transfers.
>
> We might miss the cancel per each file transfer as we don't know
> which
> files are being copied, which files had error and which files were
> totally copied to the guest.
>
> Let's take two situations:
>
> 1) One operation with several files
> - Drag and drop several files with different sizes. You'll see the
> counter decrease but from UI you can't say which files are being
> transferred. Let's say that, by mistake, you selected a big file
> and
> would like to cancel it, you can't.
>
> 2) Several operations
> - Drag and drop one big file; then another big file; then another.
> Now
> you will see the counter at '3'. Based on network performance, you
> might regret that and wanted to cancel two transfers to use the
> bandwidth only on one per time. (Old days with dial-up feelings!)
>
> The previous version, you were hiding the files and that was great.
> You
> could work around situations by let the user cancel the file
> transfers
> of individual files by themselves. Not anymore.
>
> I think the use-case is different from Nautilus as mentioned
> previously
> by Fabiano [0] - It is more likely download/upload then plain copy on
> mounted devices.
>
> [0] https://www.redhat.com/archives/virt-tools-list/2016-August/msg00
> 048.html
>
> With that being said, I agree that the major goal is moving towards
> better guest integration but I still think that we will need
> SpiceFileTransferTask and client-side (transfer) information so this
> UI
> (re)work on virt-viewer is still important for the long run.
>
> Cheers,
> toso
Paraphrasing a conversation from IRC:
Yes, I agree that there may be times when canceling the transfer of an
individual file may be convenient. But I don't think it will matter
very often. So I'd prefer to keep it simple for now and maybe add the
ability to cancel individual files later if it becomes obvious that
it's necessary.
>
> >
> >
> > This patch requires new API from spice-gtk to calculate the overall
> > progress:
> > spice_file_transfer_task_get_total_bytes()
> > spice_file_transfer_task_get_transferred_bytes()
> > ---
> > Changes in v3:
> > - patch should now apply to git master
> >
> > src/Makefile.am | 1 +
> > .../ui/virt-viewer-file-transfer-dialog.ui | 87
> > ++++++++++++
> > src/resources/virt-viewer.gresource.xml | 1 +
> > src/virt-viewer-file-transfer-dialog.c | 152 ++++++++-
> > ------------
> > 4 files changed, 146 insertions(+), 95 deletions(-)
> > create mode 100644 src/resources/ui/virt-viewer-file-transfer-
> > dialog.ui
> >
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 0c48e40..272c4ff 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -13,6 +13,7 @@ noinst_DATA = \
> > resources/ui/virt-viewer-vm-connection.ui \
> > resources/ui/virt-viewer-preferences.ui \
> > resources/ui/remote-viewer-connect.ui \
> > + resources/ui/virt-viewer-file-transfer-dialog.ui \
> > $(NULL)
> >
> > EXTRA_DIST = \
> > diff --git a/src/resources/ui/virt-viewer-file-transfer-dialog.ui
> > b/src/resources/ui/virt-viewer-file-transfer-dialog.ui
> > new file mode 100644
> > index 0000000..e3514d6
> > --- /dev/null
> > +++ b/src/resources/ui/virt-viewer-file-transfer-dialog.ui
> > @@ -0,0 +1,87 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<interface>
> > + <requires lib="gtk+" version="2.24"/>
> > + <!-- interface-naming-policy project-wide -->
> > + <template class="VirtViewerFileTransferDialog"
> > parent="GtkDialog">
> > + <property name="default_width">400</property>
> > + <property name="can_focus">False</property>
> > + <property name="border_width">5</property>
> > + <property name="type_hint">dialog</property>
> > + <child internal-child="vbox">
> > + <object class="GtkVBox" id="dialog-vbox1">
> > + <property name="visible">True</property>
> > + <property name="can_focus">False</property>
> > + <property name="spacing">12</property>
> > + <property name="border-width">12</property>
> > + <child internal-child="action_area">
> > + <object class="GtkHButtonBox" id="dialog-action_area1">
> > + <property name="visible">True</property>
> > + <property name="can_focus">False</property>
> > + <property name="layout_style">end</property>
> > + <child>
> > + <placeholder/>
> > + </child>
> > + <child>
> > + <object class="GtkButton" id="button1">
> > + <property name="label">gtk-cancel</property>
> > + <property name="visible">True</property>
> > + <property name="can_focus">True</property>
> > + <property name="receives_default">True</property>
> > + <property name="use_underline">True</property>
> > + <property name="use_stock">True</property>
> > + </object>
> > + <packing>
> > + <property name="expand">False</property>
> > + <property name="fill">False</property>
> > + <property name="position">1</property>
> > + </packing>
> > + </child>
> > + </object>
> > + <packing>
> > + <property name="expand">True</property>
> > + <property name="fill">True</property>
> > + <property name="position">0</property>
> > + </packing>
> > + </child>
> > + <child>
> > + <object class="GtkVBox" id="vbox1">
> > + <property name="visible">True</property>
> > + <property name="can_focus">False</property>
> > + <property name="spacing">12</property>
> > + <child>
> > + <object class="GtkLabel" id="transfer_summary">
> > + <property name="visible">True</property>
> > + <property name="can_focus">False</property>
> > + <property name="label"
> > translatable="yes">label</property>
> > + </object>
> > + <packing>
> > + <property name="expand">True</property>
> > + <property name="fill">True</property>
> > + <property name="position">0</property>
> > + </packing>
> > + </child>
> > + <child>
> > + <object class="GtkProgressBar" id="progressbar">
> > + <property name="visible">True</property>
> > + <property name="can_focus">False</property>
> > + </object>
> > + <packing>
> > + <property name="expand">True</property>
> > + <property name="fill">True</property>
> > + <property name="position">1</property>
> > + </packing>
> > + </child>
> > + </object>
> > + <packing>
> > + <property name="expand">True</property>
> > + <property name="fill">True</property>
> > + <property name="position">1</property>
> > + </packing>
> > + </child>
> > + </object>
> > + </child>
> > + <action-widgets>
> > + <action-widget response="-6">button1</action-widget>
> > + </action-widgets>
> > + </template>
> > +</interface>
> > diff --git a/src/resources/virt-viewer.gresource.xml
> > b/src/resources/virt-viewer.gresource.xml
> > index b8ced29..f9b4a9f 100644
> > --- a/src/resources/virt-viewer.gresource.xml
> > +++ b/src/resources/virt-viewer.gresource.xml
> > @@ -8,6 +8,7 @@
> > <file>ui/virt-viewer-preferences.ui</file>
> > <file>ui/virt-viewer-vm-connection.ui</file>
> > <file>ui/virt-viewer.ui</file>
> > + <file>ui/virt-viewer-file-transfer-dialog.ui</file>
> > <file alias="icons/16x16/virt-
> > viewer.png">../../icons/16x16/virt-viewer.png</file>
> > <file alias="icons/22x22/virt-
> > viewer.png">../../icons/22x22/virt-viewer.png</file>
> > <file alias="icons/24x24/virt-
> > viewer.png">../../icons/24x24/virt-viewer.png</file>
> > diff --git a/src/virt-viewer-file-transfer-dialog.c b/src/virt-
> > viewer-file-transfer-dialog.c
> > index c8b50f0..cef5448 100644
> > --- a/src/virt-viewer-file-transfer-dialog.c
> > +++ b/src/virt-viewer-file-transfer-dialog.c
> > @@ -23,26 +23,31 @@
> > #include "virt-viewer-file-transfer-dialog.h"
> > #include <glib/gi18n.h>
> >
> > -G_DEFINE_TYPE(VirtViewerFileTransferDialog,
> > virt_viewer_file_transfer_dialog, GTK_TYPE_DIALOG)
> > -
> > -#define FILE_TRANSFER_DIALOG_PRIVATE(o) \
> > - (G_TYPE_INSTANCE_GET_PRIVATE((o),
> > VIRT_VIEWER_TYPE_FILE_TRANSFER_DIALOG,
> > VirtViewerFileTransferDialogPrivate))
> > -
> > struct _VirtViewerFileTransferDialogPrivate
> > {
> > /* GHashTable<SpiceFileTransferTask, widgets> */
> > - GHashTable *file_transfers;
> > + GSList *file_transfers;
> > guint timer_show_src;
> > guint timer_hide_src;
> > + GtkWidget *transfer_summary;
> > + GtkWidget *progressbar;
> > };
> >
> > +G_DEFINE_TYPE_WITH_PRIVATE(VirtViewerFileTransferDialog,
> > virt_viewer_file_transfer_dialog, GTK_TYPE_DIALOG)
> > +
> > +#define FILE_TRANSFER_DIALOG_PRIVATE(o) \
> > + (G_TYPE_INSTANCE_GET_PRIVATE((o),
> > VIRT_VIEWER_TYPE_FILE_TRANSFER_DIALOG,
> > VirtViewerFileTransferDialogPrivate))
> > +
> >
> > static void
> > virt_viewer_file_transfer_dialog_dispose(GObject *object)
> > {
> > VirtViewerFileTransferDialog *self =
> > VIRT_VIEWER_FILE_TRANSFER_DIALOG(object);
> >
> > - g_clear_pointer(&self->priv->file_transfers,
> > g_hash_table_unref);
> > + if (self->priv->file_transfers) {
> > + g_slist_free_full(self->priv->file_transfers,
> > g_object_unref);
> > + self->priv->file_transfers = NULL;
> > + }
> >
> > G_OBJECT_CLASS(virt_viewer_file_transfer_dialog_parent_class)-
> > >dispose(object);
> > }
> > @@ -51,8 +56,16 @@ static void
> > virt_viewer_file_transfer_dialog_class_init(VirtViewerFileTransfer
> > DialogClass *klass)
> > {
> > GObjectClass *object_class = G_OBJECT_CLASS(klass);
> > + GtkWidgetClass *widget_class = GTK_WIDGET_CLASS(klass);
> >
> > - g_type_class_add_private(klass,
> > sizeof(VirtViewerFileTransferDialogPrivate));
> > + gtk_widget_class_set_template_from_resource(widget_class,
> > + "/org/virt-
> > manager/virt-viewer/ui/virt-viewer-file-transfer-dialog.ui");
> > + gtk_widget_class_bind_template_child_private(widget_class,
> > + VirtViewerFileTra
> > nsferDialog,
> > + transfer_summary)
> > ;
> > + gtk_widget_class_bind_template_child_private(widget_class,
> > + VirtViewerFileTra
> > nsferDialog,
> > + progressbar);
> >
> > object_class->dispose =
> > virt_viewer_file_transfer_dialog_dispose;
> > }
> > @@ -63,16 +76,13 @@ dialog_response(GtkDialog *dialog,
> > gpointer user_data G_GNUC_UNUSED)
> > {
> > VirtViewerFileTransferDialog *self =
> > VIRT_VIEWER_FILE_TRANSFER_DIALOG(dialog);
> > - GHashTableIter iter;
> > - gpointer key, value;
> > + GSList *slist = self->priv->file_transfers;
> >
> > switch (response_id) {
> > case GTK_RESPONSE_CANCEL:
> > /* cancel all current tasks */
> > - g_hash_table_iter_init(&iter, self->priv-
> > >file_transfers);
> > -
> > - while (g_hash_table_iter_next(&iter, &key, &value)) {
> > - spice_file_transfer_task_cancel(SPICE_FILE_TRANSFE
> > R_TASK(key));
> > + for (slist = self->priv->file_transfers; slist !=
> > NULL; slist = g_slist_next(slist)) {
> > + spice_file_transfer_task_cancel(SPICE_FILE_TRANSFE
> > R_TASK(slist->data));
> > }
> > break;
> > case GTK_RESPONSE_DELETE_EVENT:
> > @@ -83,53 +93,6 @@ dialog_response(GtkDialog *dialog,
> > }
> > }
> >
> > -static void task_cancel_clicked(GtkButton *button G_GNUC_UNUSED,
> > - gpointer user_data)
> > -{
> > - SpiceFileTransferTask *task = user_data;
> > - spice_file_transfer_task_cancel(task);
> > -}
> > -
> > -typedef struct {
> > - GtkWidget *vbox;
> > - GtkWidget *hbox;
> > - GtkWidget *progress;
> > - GtkWidget *label;
> > - GtkWidget *cancel;
> > -} TaskWidgets;
> > -
> > -static TaskWidgets *task_widgets_new(SpiceFileTransferTask *task)
> > -{
> > - TaskWidgets *w = g_new0(TaskWidgets, 1);
> > - gchar *filename;
> > -
> > - w->vbox = gtk_box_new(GTK_ORIENTATION_VERTICAL, 6);
> > - w->hbox = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, 12);
> > - w->progress = gtk_progress_bar_new();
> > - filename = spice_file_transfer_task_get_filename(task);
> > - w->label = gtk_label_new(filename);
> > - g_free(filename);
> > - w->cancel = gtk_button_new_from_icon_name("gtk-cancel",
> > GTK_ICON_SIZE_SMALL_TOOLBAR);
> > - gtk_widget_set_hexpand(w->progress, TRUE);
> > - gtk_widget_set_valign(w->progress, GTK_ALIGN_CENTER);
> > - gtk_widget_set_hexpand(w->label, TRUE);
> > - gtk_widget_set_valign(w->label, GTK_ALIGN_END);
> > - gtk_widget_set_halign(w->label, GTK_ALIGN_START);
> > - gtk_widget_set_hexpand(w->cancel, FALSE);
> > - gtk_widget_set_valign(w->cancel, GTK_ALIGN_CENTER);
> > -
> > - g_signal_connect(w->cancel, "clicked",
> > - G_CALLBACK(task_cancel_clicked), task);
> > -
> > - gtk_box_pack_start(GTK_BOX(w->hbox), w->progress, TRUE, TRUE,
> > 0);
> > - gtk_box_pack_start(GTK_BOX(w->hbox), w->cancel, FALSE, TRUE,
> > 0);
> > - gtk_box_pack_start(GTK_BOX(w->vbox), w->label, TRUE, TRUE, 0);
> > - gtk_box_pack_start(GTK_BOX(w->vbox), w->hbox, TRUE, TRUE, 0);
> > -
> > - gtk_widget_show_all(w->vbox);
> > - return w;
> > -}
> > -
> > static gboolean delete_event(GtkWidget *widget,
> > GdkEvent *event G_GNUC_UNUSED,
> > gpointer user_data G_GNUC_UNUSED)
> > @@ -143,18 +106,10 @@ static gboolean delete_event(GtkWidget
> > *widget,
> > static void
> > virt_viewer_file_transfer_dialog_init(VirtViewerFileTransferDialog
> > *self)
> > {
> > - GtkBox *content =
> > GTK_BOX(gtk_dialog_get_content_area(GTK_DIALOG(self)));
> > + gtk_widget_init_template(GTK_WIDGET(self));
> >
> > self->priv = FILE_TRANSFER_DIALOG_PRIVATE(self);
> >
> > - gtk_widget_set_size_request(GTK_WIDGET(content), 400, -1);
> > - gtk_container_set_border_width(GTK_CONTAINER(content), 12);
> > - self->priv->file_transfers =
> > g_hash_table_new_full(g_direct_hash, g_direct_equal,
> > - g_object_un
> > ref,
> > - (GDestroyNo
> > tify)g_free);
> > - gtk_dialog_add_button(GTK_DIALOG(self), _("Cancel"),
> > GTK_RESPONSE_CANCEL);
> > - gtk_dialog_set_default_response(GTK_DIALOG(self),
> > - GTK_RESPONSE_CANCEL);
> > g_signal_connect(self, "response",
> > G_CALLBACK(dialog_response), NULL);
> > g_signal_connect(self, "delete-event",
> > G_CALLBACK(delete_event), NULL);
> > }
> > @@ -169,17 +124,38 @@
> > virt_viewer_file_transfer_dialog_new(GtkWindow *parent)
> > NULL);
> > }
> >
> > -static void task_progress_notify(GObject *object,
> > +static void update_global_progress(VirtViewerFileTransferDialog
> > *self)
> > +{
> > + GSList *slist;
> > + guint64 total = 0, transferred = 0;
> > + gchar *message = NULL;
> > + guint n_files = 0;
> > + gdouble fraction = 1.0;
> > +
> > + for (slist = self->priv->file_transfers; slist != NULL; slist
> > = g_slist_next(slist)) {
> > + SpiceFileTransferTask *task = slist->data;
> > + total += spice_file_transfer_task_get_total_bytes(task);
> > + transferred +=
> > spice_file_transfer_task_get_transferred_bytes(task);
> > + n_files++;
> > + }
> > +
> > + if (n_files > 0)
> > + fraction = (gdouble)transferred / total;
> > + message = g_strdup_printf(ngettext("Transferring %d file...",
> > + "Transferring %d files...",
> > n_files),
> > + n_files);
> > + gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(self->priv-
> > >progressbar), fraction);
> > + gtk_label_set_text(GTK_LABEL(self->priv->transfer_summary),
> > message);
> > + g_free(message);
> > +}
> > +
> > +static void task_progress_notify(GObject *object G_GNUC_UNUSED,
> > GParamSpec *pspec G_GNUC_UNUSED,
> > gpointer user_data)
> > {
> > VirtViewerFileTransferDialog *self =
> > VIRT_VIEWER_FILE_TRANSFER_DIALOG(user_data);
> > - SpiceFileTransferTask *task =
> > SPICE_FILE_TRANSFER_TASK(object);
> > - TaskWidgets *w = g_hash_table_lookup(self->priv-
> > >file_transfers, task);
> > - g_return_if_fail(w);
> >
> > - double pct = spice_file_transfer_task_get_progress(task);
> > - gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(w->progress),
> > pct);
> > + update_global_progress(self);
> > }
> >
> > static gboolean hide_transfer_dialog(gpointer data)
> > @@ -195,7 +171,6 @@ static gboolean hide_transfer_dialog(gpointer
> > data)
> >
> > typedef struct {
> > VirtViewerFileTransferDialog *self;
> > - TaskWidgets *widgets;
> > SpiceFileTransferTask *task;
> > } TaskFinishedData;
> >
> > @@ -203,9 +178,6 @@ static gboolean task_finished_remove(gpointer
> > user_data)
> > {
> > TaskFinishedData *d = user_data;
> >
> > - gtk_widget_destroy(d->widgets->vbox);
> > -
> > - g_free(d->widgets);
> > g_object_unref(d->task);
> > g_free(d);
> >
> > @@ -218,26 +190,21 @@ static void
> > task_finished(SpiceFileTransferTask *task,
> > {
> > TaskFinishedData *d;
> > VirtViewerFileTransferDialog *self =
> > VIRT_VIEWER_FILE_TRANSFER_DIALOG(user_data);
> > - TaskWidgets *w = g_hash_table_lookup(self->priv-
> > >file_transfers, task);
> >
> > if (error && !g_error_matches(error, G_IO_ERROR,
> > G_IO_ERROR_CANCELLED))
> > g_warning("File transfer task %p failed: %s", task, error-
> > >message);
> >
> > - g_return_if_fail(w);
> > - gtk_widget_set_sensitive(w->cancel, FALSE);
> > -
> > -
> > d = g_new0(TaskFinishedData, 1);
> > d->self = self;
> > - d->widgets = w;
> > d->task = task;
> >
> > g_timeout_add(500, task_finished_remove, d);
> >
> > - g_hash_table_steal(self->priv->file_transfers, task);
> > + self->priv->file_transfers = g_slist_remove(self->priv-
> > >file_transfers, task);
> > + update_global_progress(self);
> >
> > /* if this is the last transfer, close the dialog */
> > - if (!g_hash_table_size(d->self->priv->file_transfers)) {
> > + if (!g_slist_length(d->self->priv->file_transfers)) {
> > /* cancel any pending 'show' operations if all tasks
> > complete before
> > * the dialog can be shown */
> > if (self->priv->timer_show_src) {
> > @@ -254,6 +221,7 @@ static gboolean
> > show_transfer_dialog_delayed(gpointer user_data)
> > VirtViewerFileTransferDialog *self = user_data;
> >
> > self->priv->timer_show_src = 0;
> > + update_global_progress(self);
> > gtk_widget_show(GTK_WIDGET(self));
> >
> > return G_SOURCE_REMOVE;
> > @@ -282,13 +250,7 @@ static void
> > show_transfer_dialog(VirtViewerFileTransferDialog *self)
> > void
> > virt_viewer_file_transfer_dialog_add_task(VirtViewerFileTransferDia
> > log *self,
> > SpiceFileTransferTa
> > sk *task)
> > {
> > - GtkBox *content =
> > GTK_BOX(gtk_dialog_get_content_area(GTK_DIALOG(self)));
> > - TaskWidgets *w = task_widgets_new(task);
> > -
> > - gtk_box_pack_start(content,
> > - w->vbox,
> > - TRUE, TRUE, 12);
> > - g_hash_table_insert(self->priv->file_transfers,
> > g_object_ref(task), w);
> > + self->priv->file_transfers = g_slist_prepend(self->priv-
> > >file_transfers, g_object_ref(task));
> > g_signal_connect(task, "notify::progress",
> > G_CALLBACK(task_progress_notify), self);
> > g_signal_connect(task, "finished", G_CALLBACK(task_finished),
> > self);
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
More information about the virt-tools-list
mailing list