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

[Fabiano Fidêncio]

Jonathon,

On Wed, Aug 17, 2016 at 9:10 PM, Jonathon Jongsma <jjongsma at redhat.com> 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).

>
> 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?

> +        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?

Reviewed-by: Fabiano Fidêncio <fabiano at fidencio.org>

Best Regards,
-- 
Fabiano Fidêncio