[virt-tools-list] [PATCH virt-viewer v4 2/2] Show failed file transfers

[Pavel Grunt]

Hi Jonathon,

it looks good, I put some comments inline

On Mon, 2016-10-24 at 16:04 -0500, Jonathon Jongsma 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.
> ---
>  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);

I would add a NULL check - spice_file_transfer_task_get_filename uses
g_file_get_basename which can return NULL
               if (filename == NULL)
                   continue; 
> +
> +            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;

hmm, that would require to check the length of the string in case
there was no valid filename - and avoid showing the dialog / showing a
different message

> +
> +        GtkWidget *dialog = 
usually the variable is declared at the beginning of the block

> gtk_message_dialog_new(GTK_WINDOW(self), 0, GTK_MESSAGE_ERROR,
> +                                                   GTK_BUTTONS_OK,
> +                                                   _("An error
> caused following file transfers to fail:\n%s"),
there is an extra newline (msg prepends \n for each filename)
> +                                                   msg->str);
> +        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);

I am probably making it more complicated than it should be

Pavel