On Tue, 2016-08-30 at 21:42 +0200, Fabiano Fidêncio wrote:
> Jonathon,
>
> On Wed, Aug 17, 2016 at 9:10 PM, Jonathon Jongsma <jjongsma@xxxxxxxxx
> m> wrote:
> >
> > After all ongoing file transfers are finished, save a list of the
> > file
> > transfer tasks that had errors and display the list of failed files
> > to
> > the user so that they know that a failure occurred and can
> > potentially
> > retry the transfer. Previously, we just failed silently so the user
> > may not have even been aware that the transfer failed.
> > ---
>
> As the list is now saved, can we present the "Retry" option for those
> files?
>
> >
> >
> > Undoubtedly there are many improvements that could be made here.
> > And there are
> > many alternate aproaches that could be taken.
> >
> > I could possibly have displayed the error message in the same
> > dialog that shows
> > the transfer progress, but that complicates things slightly,
> > because we'd have
> > to keep the dialog open so that the user can read the message, and
> > change the
> > button from 'cancel' to 'close' or 'ok', in addition to other
> > things. A
> > separate dedicated dialog seemed better than trying to shoehorn it
> > into the
> > progress dialog.
>
> Indeed.
>
> >
> >
> > Another question is: should the VirtViewerFileTransferDialog have
> > the
> > responsibility for handling file transfer errors at all? Or should
> > that be
> > handled at the VirtViewerSessionSpice level? If
> > VirtViewerSessionSpice handles
> > the errors and displays an error dialog, we could possibly add a
> > "Retry" button
> > since the VirtViewerSessionSpice object holds a reference to the
> > SpiceMainChannel. On the other hand, VirtViewerSessionSpice doesn't
> > currently
> > know the status of all ongoing file transfers, so it's not as easy
> > to wait
> > until the end of a batch of file transfers to display a single
> > error message
> > for all failures. And I don't think popping up a separate dialog
> > for each of 10
> > file transfer failures would be desirable.
>
> Hmmm. Here you answered my question.
>
> I'd say that, at this point, a "Retry" button isn't something we
> would
> like to have. It's a good thing for the future, though, in case we
> decide to improve things either here and in spice-gtk.
> I'd keep the approach as simple as possible for now and re-work this
> after having spice-gtk in a better shape (as I said in some mail
> thread before, depending basically on how much effort you guys want
> to
> spend in the file-transfer approach).
Right, it would be a nice feature in theory, but I think this is a
reasonable first step.
>
> >
> >
> > There are probably other non-dialog approaches we could take as
> > well, but I
> > thought I'd put this out there for discussion, since I think it's
> > important to
> > communicate *some* feedback to the user in case of failure.
> >
> >
> > src/virt-viewer-file-transfer-dialog.c | 37
> > +++++++++++++++++++++++++++++++++-
> > 1 file changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/virt-viewer-file-transfer-dialog.c b/src/virt-
> > viewer-file-transfer-dialog.c
> > index 3339ba4..626a26a 100644
> > --- a/src/virt-viewer-file-transfer-dialog.c
> > +++ b/src/virt-viewer-file-transfer-dialog.c
> > @@ -26,6 +26,7 @@
> > struct _VirtViewerFileTransferDialogPrivate
> > {
> > GSList *file_transfers;
> > + GSList *failed;
> > guint timer_show_src;
> > guint timer_hide_src;
> > GtkWidget *transfer_summary;
> > @@ -157,6 +158,14 @@ static void task_progress_notify(GObject
> > *object G_GNUC_UNUSED,
> > update_global_progress(self);
> > }
> >
> > +static void
> > +error_dialog_response(GtkDialog *dialog,
> > + gint response_id G_GNUC_UNUSED,
> > + gpointer user_data G_GNUC_UNUSED)
> > +{
> > + gtk_widget_destroy(GTK_WIDGET(dialog));
> > +}
> > +
> > static gboolean hide_transfer_dialog(gpointer data)
> > {
> > VirtViewerFileTransferDialog *self = data;
> > @@ -165,6 +174,30 @@ static gboolean hide_transfer_dialog(gpointer
> > data)
> > GTK_RESPONSE_CANCEL, FALSE);
> > self->priv->timer_hide_src = 0;
> >
> > + /* When all ongoing file transfers are finished, show errors
> > */
> > + if (self->priv->failed) {
> > + GSList *sl;
> > + GString *msg = g_string_new("");
> > +
> > + for (sl = self->priv->failed; sl != NULL; sl =
> > g_slist_next(sl)) {
> > + SpiceFileTransferTask *failed_task = sl->data;
> > + gchar *filename =
> > spice_file_transfer_task_get_filename(failed_task);
> > +
> > + g_string_append_printf(msg, "\n%s", filename);
> > + g_free(filename);
> > + }
> > + g_slist_free_full(self->priv->failed, g_object_unref);
> > + self->priv->failed = NULL;
> > +
> > + GtkWidget *dialog =
> > gtk_message_dialog_new(GTK_WINDOW(self), 0, GTK_MESSAGE_ERROR,
> > + GTK_BUTTONS_OK,
> > + _("An error
> > caused following file transfers to fail:\n%s"),
> > + msg->str);
>
> Hmm. Thinking about the use where I most used file transfer that was
> copying .dlls for debugging the windows builds ... I was transferring
> 40~50 files each time, in case an error occurs and the most part of
> those files are not transferred, this message would be quite
> unreadable, no?
Good point. And that use case (transferring lots of files at once) was
a weakness of the old file transfer dialog as well, so I should have
considered it. One possibility:
- When there are only a couple failures (perhaps < 2 or 3), we could
display the list of files as done above
- For cases where a lot of files fail (> 2 or 3) we could just display
the number of files that failed: "An error caused N file transfers to
fail"
>
> >
> > + g_string_free(msg, TRUE);
> > + g_signal_connect(dialog, "response",
> > G_CALLBACK(error_dialog_response), NULL);
> > + gtk_widget_show(dialog);
> > + }
> > +
> > return G_SOURCE_REMOVE;
> > }
> >
> > @@ -174,8 +207,10 @@ static void
> > task_finished(SpiceFileTransferTask *task,
> > {
> > VirtViewerFileTransferDialog *self =
> > VIRT_VIEWER_FILE_TRANSFER_DIALOG(user_data);
> >
> > - if (error && !g_error_matches(error, G_IO_ERROR,
> > G_IO_ERROR_CANCELLED))
> > + if (error && !g_error_matches(error, G_IO_ERROR,
> > G_IO_ERROR_CANCELLED)) {
> > + self->priv->failed = g_slist_prepend(self->priv->failed,
> > g_object_ref(task));
> > g_warning("File transfer task %p failed: %s", task, error-
> > >message);
> > + }
> >
> > self->priv->file_transfers = g_slist_remove(self->priv-
> > >file_transfers, task);
> > g_object_unref(task);
> > --
> > 2.7.4
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
> I'm wondering whether would be useful to show the list of failures in
> some log message instead of doing it in the UI.
> What do you think? Maybe do it in both ways?
I think spice-gtk already logs some details about failed file transfers
in spice_file_transfer_task_completed(). I'm not sure if we need
anything additional in virt-viewer.
Jonathon
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
