[virt-tools-list] [PATCH virt-viewer] RFC: Show failed file transfers

[Jonathon Jongsma]

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 at redhat.co
> 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 at redhat.com
> > 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