[virt-tools-list] [PATCH] virt-viewer-file-transfer-dialog: improve error message

[Kevin Pouget]

hello,

On Thu, Aug 8, 2019 at 2:47 PM Victor Toso <victortoso at redhat.com> wrote:
>
> Hi,
>
> On Thu, Aug 01, 2019 at 05:31:20PM +0200, Kevin Pouget wrote:
> > This patch improves the error shown to the user when a file transfer
> > fails.
> >
> > The previous behavior was to create a simple message dialog box, with
> > the error description and the full list of the files that failed to be
> > transferred. When the list of files was long, the dialog box would
> > grow bigger than the screen.
> >
> > Now, the file list is inserted inside a scrollable widget, whose width
> > is limited to 170px.
>
> s/width/height ?

yes

> >
> > NB: these two calls would be more adapted, but they require GTK >= 3.3:
>
> Typo on 3.3? Gtk required is 3.12 and introduced in those
> functions is 3.22. Spice-gtk requires 3.22 already so this can be
> considered for virt-viewer too. Looking at
> a1fd9ee1c680 in spice-gtk (the bump to 3.22), even Fedora 26 and
> RHEL 7.4 had 3.22 so it shouldn't be a problem to bump if you
> feel like it.

yes, it was a typo, s/3.3/3.22/, but when using these functions, the
compiler was issuing warnings, that's why I removed them

> virt-viewer-file-transfer-dialog.c:247:9: warning: 'gtk_scrolled_window_set_max_content_height' is deprecated: Not available before 3.22 [-Wdeprecated-declarations]
>  247 |         gtk_scrolled_window_set_max_content_height(GTK_SCROLLED_WINDOW(scrolled_window), 170);

because of configure.ac::GTK_REQUIRED=3.12

> > > gtk_scrolled_window_set_max_content_height(GTK_SCROLLED_WINDOW(scrolled_window), 170);
> > > gtk_scrolled_window_set_propagate_natural_height(GTK_SCROLLED_WINDOW(scrolled_window), TRUE);
> >
> > This patch addresses this bug:
> >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1496356
>
> Those last three lines can be changed to
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1496356

I wasn't sure of the proper way to link the patch and the ticket!

> > Signed-off-by: Kevin Pouget <kpouget at redhat.com>
>
> Patch works fine, thanks. Let me know if you prefer sending a v2
> with any changes or if I should change the commit log and push
> (in case you can't push it), imho this one works fine
>
>     Acked-by: Victor Toso <victortoso at redhat.com>

thanks! I'm don't think I have commit rights for this repo, so if you
can please do the commit

Kevin

> > ---
> >  src/virt-viewer-file-transfer-dialog.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/virt-viewer-file-transfer-dialog.c b/src/virt-viewer-file-transfer-dialog.c
> > index a82d01a..b510d8e 100644
> > --- a/src/virt-viewer-file-transfer-dialog.c
> > +++ b/src/virt-viewer-file-transfer-dialog.c
> > @@ -200,7 +200,8 @@ static gboolean hide_transfer_dialog(gpointer data)
> >      if (self->priv->failed) {
> >          GSList *sl;
> >          GString *msg = g_string_new("");
> > -        GtkWidget *dialog;
> > +        GtkWidget *dialog, *files_label, *scrolled_window, *area;
> > +        GtkRequisition files_label_sz;
> >
> >          for (sl = self->priv->failed; sl != NULL; sl = g_slist_next(sl)) {
> >              SpiceFileTransferTask *failed_task = sl->data;
> > @@ -221,11 +222,28 @@ static gboolean hide_transfer_dialog(gpointer data)
> >
> >          dialog = gtk_message_dialog_new(GTK_WINDOW(self), 0, GTK_MESSAGE_ERROR,
> >                                          GTK_BUTTONS_OK,
> > -                                        _("An error caused the following file transfers to fail:%s"),
> > -                                        msg->str);
> > +                                        _("An error caused the following file transfers to fail:"));
> > +        gtk_window_set_title(GTK_WINDOW(dialog), "Transfer error");
> > +
> > +        scrolled_window = gtk_scrolled_window_new(NULL, NULL);
> > +        gtk_scrolled_window_set_policy(GTK_SCROLLED_WINDOW(scrolled_window),
> > +                                       GTK_POLICY_AUTOMATIC, GTK_POLICY_AUTOMATIC);
> > +
> > +        area = gtk_message_dialog_get_message_area(GTK_MESSAGE_DIALOG(dialog));
> > +        gtk_container_add(GTK_CONTAINER(area), scrolled_window);
> > +
> > +        files_label = gtk_label_new(msg->str + 1); /* skip the initial '\n' */
> > +        gtk_label_set_selectable(GTK_LABEL(files_label), TRUE);
> > +        gtk_container_add(GTK_CONTAINER(scrolled_window), files_label);
> > +
> >          g_string_free(msg, TRUE);
> >          g_signal_connect(dialog, "response", G_CALLBACK(error_dialog_response), NULL);
> > -        gtk_widget_show(dialog);
> > +        gtk_widget_show_all(dialog);
> > +
> > +        /* adjust panel to file_label height */
> > +        gtk_widget_get_preferred_size(files_label, NULL, &files_label_sz);
> > +        gtk_scrolled_window_set_min_content_height(GTK_SCROLLED_WINDOW(scrolled_window),
> > +                                                   MIN(files_label_sz.height, 170));
> >      }
> >
> >      return G_SOURCE_REMOVE;
> > --
> > 2.21.0
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list