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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
