Re: [virt-tools-list] [virt-manager PATCH] Tiny inspection improvements
On 02/08/2017 10:55 AM, Pino Toscano wrote: > Hi, > > this patch series improves the integration with libguestfs for > inspection of guests: > - show the actual OS of each guest, in addition to the product name > - fix the version of installed packages, by taking epochs into account > - use also low quality icons from guests > ACK and pushed Thanks, Cole ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-manager PATCH] virtManager/viewers: fix connection to remote SPICE with password
On 02/07/2017 12:00 PM, Pavel Hrdina wrote: > When connecting to remote SPICE we use ssh tunnel if the SPICE is > listening only on "localhost". Our ssh tunnel scheduler uses locks > to serialize the requests for FD in order to not spam user for ssh > password. > > However when the main_channel is connected and emits AUTH_ERROR > we ask user for password and request for new FD. Unfortunately > after the new request is handled we didn't unlock the scheduler > and all other request would remain waiting for the lock. > > We need to unlock every FD request for the SPICE main channel not > only the first one when the channel itself is created. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1401790 > > Signed-off-by: Pavel Hrdina> --- > virtManager/viewers.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/virtManager/viewers.py b/virtManager/viewers.py > index 2f7d2e95..54c2c973 100644 > --- a/virtManager/viewers.py > +++ b/virtManager/viewers.py > @@ -562,6 +562,8 @@ class SpiceViewer(Viewer): > # > > def _main_channel_event_cb(self, channel, event): > +self._tunnels.unlock() > + > if event == SpiceClientGLib.ChannelEvent.CLOSED: > self._emit_disconnected() > elif event == SpiceClientGLib.ChannelEvent.ERROR_AUTH: > @@ -614,7 +616,6 @@ class SpiceViewer(Viewer): > > if (type(channel) == SpiceClientGLib.MainChannel and > not self._main_channel): > -self._tunnels.unlock() > self._main_channel = channel > hid = self._main_channel.connect_after("channel-event", > self._main_channel_event_cb) > ACK - Cole ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer v2] session-spice: Pass hostname to authentication dialog
On 09/02/17 17:22, Pavel Grunt wrote: > On Thu, 2017-02-09 at 15:32 -0200, Eduardo Lima (Etrunko) wrote: >> With this patch the dialog now shows the host we are connecting to. >> >> Signed-off-by: Eduardo Lima (Etrunko)> Acked-by: Pavel Grunt Thanks, pushed. >> --- >> v2: Use proper uri if connecting via proxy. >> --- >> src/virt-viewer-session-spice.c | 7 +-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer- >> session-spice.c >> index c3fce48..9b52ec0 100644 >> --- a/src/virt-viewer-session-spice.c >> +++ b/src/virt-viewer-session-spice.c >> @@ -691,6 +691,7 @@ >> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel, >> case SPICE_CHANNEL_ERROR_AUTH: >> { >> const GError *error = NULL; >> +gchar *host = NULL; >> g_debug("main channel: auth failure (wrong >> username/password?)"); >> >> { >> @@ -717,11 +718,13 @@ >> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel, >> user = g_strdup(g_get_user_name()); >> } >> >> +g_object_get(self->priv->session, "host", , NULL); >> ret = virt_viewer_auth_collect_credentials(self->priv- >>> main_window, >> "SPICE", >> - NULL, >> + host, >> username_require >> d ? : NULL, >> ); >> +g_free(host); >> if (!ret) { >> g_signal_emit_by_name(session, "session-cancelled"); >> } else { >> @@ -750,7 +753,7 @@ >> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel, >> g_warn_if_fail(proxy != NULL); >> >> ret = virt_viewer_auth_collect_credentials(self->priv- >>> main_window, >> - "proxy", >> NULL, >> + "proxy", >> spice_uri_get_hostname(proxy), >> , >> ); >> if (!ret) { >> g_signal_emit_by_name(session, "session- >> cancelled"); -- Eduardo de Barros Lima (Etrunko) Software Engineer - RedHat etru...@redhat.com ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer v2] iso-dialog: Avoid crash when closing dialog early
On 09/02/17 17:19, Pavel Grunt wrote: > Hi Eduardo, > > On Thu, 2017-02-09 at 15:13 -0200, Eduardo Lima (Etrunko) wrote: >> We must take into account that users can close the dialog at >> anytime, >> even during an operation of fetch or set ISO has not been finished. >> This >> will cause the callbacks for those operations to be invoked with an >> invalid object, crashing the application. >> >> To fix this issue we need to pass a GCancellable to the asynchronous >> operations, so they can be cancelled if the dialog happens to get >> closed >> before they complete. >> >> Signed-off-by: Eduardo Lima (Etrunko)>> --- >> v2: >> * Move call to g_cancellable_cancel() to response handler. >> * Don't leak error objects if operation is cancelled. >> --- >> src/remote-viewer-iso-list-dialog.c | 42 >> ++--- >> 1 file changed, 34 insertions(+), 8 deletions(-) >> >> diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote- >> viewer-iso-list-dialog.c >> index 2ab5435..ed51ffa 100644 >> --- a/src/remote-viewer-iso-list-dialog.c >> +++ b/src/remote-viewer-iso-list-dialog.c >> @@ -42,6 +42,7 @@ struct _RemoteViewerISOListDialogPrivate >> GtkWidget *stack; >> GtkWidget *tree_view; >> OvirtForeignMenu *foreign_menu; >> +GCancellable *cancellable; >> }; >> >> enum RemoteViewerISOListDialogModel >> @@ -66,6 +67,8 @@ remote_viewer_iso_list_dialog_dispose(GObject >> *object) >> RemoteViewerISOListDialog *self = >> REMOTE_VIEWER_ISO_LIST_DIALOG(object); >> RemoteViewerISOListDialogPrivate *priv = self->priv; >> >> +g_clear_object(>cancellable); >> + >> if (priv->foreign_menu) { >> g_signal_handlers_disconnect_by_data(priv->foreign_menu, >> object); >> g_clear_object(>foreign_menu); >> @@ -157,17 +160,24 @@ fetch_iso_names_cb(OvirtForeignMenu >> *foreign_menu, >> const gchar *msg = error ? error->message : _("Failed to >> fetch CD names"); >> gchar *markup = g_strdup_printf("%s", msg); >> >> +g_debug("Error fetching ISO names: %s", msg); >> +if (error && error->code == G_IO_ERROR_CANCELLED) > use g_error_matches if possible > >> +goto end; >> + >> gtk_label_set_markup(GTK_LABEL(priv->status), markup); >> gtk_spinner_stop(GTK_SPINNER(priv->spinner)); >> remote_viewer_iso_list_dialog_show_error(self, msg); >> gtk_dialog_set_response_sensitive(GTK_DIALOG(self), >> GTK_RESPONSE_NONE, TRUE); >> g_free(markup); >> -g_clear_error(); >> -return; >> +goto end; >> } >> >> +g_clear_object(>cancellable); >> g_list_foreach(iso_list, (GFunc) >> remote_viewer_iso_list_dialog_foreach, self); >> remote_viewer_iso_list_dialog_show_files(self); >> + >> +end: >> +g_clear_error(); >> } >> >> >> @@ -177,7 +187,10 @@ >> remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOListDi >> alog *self) >> RemoteViewerISOListDialogPrivate *priv = self->priv; >> >> gtk_list_store_clear(priv->list_store); >> -ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, >> NULL, >> + >> +priv->cancellable = g_cancellable_new(); >> +ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, >> + priv->cancellable, >> (GAsyncReadyCallback) >> fetch_iso_names_cb, >> self); >> } >> @@ -190,8 +203,10 @@ >> remote_viewer_iso_list_dialog_response(GtkDialog *dialog, >> RemoteViewerISOListDialog *self = >> REMOTE_VIEWER_ISO_LIST_DIALOG(dialog); >> RemoteViewerISOListDialogPrivate *priv = self->priv; >> >> -if (response_id != GTK_RESPONSE_NONE) >> +if (response_id != GTK_RESPONSE_NONE) { >> +g_cancellable_cancel(priv->cancellable); >> return; >> +} >> >> gtk_spinner_start(GTK_SPINNER(priv->spinner)); >> gtk_label_set_markup(GTK_LABEL(priv->status), >> _("Loading...")); >> @@ -223,7 +238,9 @@ >> remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle >> *cell_renderer G_GNU >> gtk_dialog_set_response_sensitive(GTK_DIALOG(self), >> GTK_RESPONSE_NONE, FALSE); >> gtk_widget_set_sensitive(priv->tree_view, FALSE); >> >> -ovirt_foreign_menu_set_current_iso_name_async(priv- >>> foreign_menu, active ? NULL : name, NULL, >> +priv->cancellable = g_cancellable_new(); >> +ovirt_foreign_menu_set_current_iso_name_async(priv- >>> foreign_menu, active ? NULL : name, >> + priv- >>> cancellable, >>(GAsyncReadyCallb >> ack)ovirt_foreign_menu_iso_name_changed, >>self); >> gtk_tree_path_free(tree_path); >> @@ -308,12 +325,18 @@ >> ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu, >> * change the ISO
[virt-tools-list] [PATCH virt-viewer 08/10] virt-viewer-auth: Use GtkHeaderBar
The auxiliary text is now presented as the subtitle of the dialog Signed-off-by: Eduardo Lima (Etrunko)--- src/resources/ui/virt-viewer-auth.ui | 148 --- src/virt-viewer-auth.c | 35 + 2 files changed, 86 insertions(+), 97 deletions(-) diff --git a/src/resources/ui/virt-viewer-auth.ui b/src/resources/ui/virt-viewer-auth.ui index 2920780..367678d 100644 --- a/src/resources/ui/virt-viewer-auth.ui +++ b/src/resources/ui/virt-viewer-auth.ui @@ -1,160 +1,146 @@ + - + +350 +1 False -5 -Authentication required -True -center-on-parent -True +12 dialog -True -True - -True + False vertical 2 - -True + False end - -_Cancel -True -True -True -False -True - - -False -False -0 - + - -_OK -True -True -True -True -True -False -True - - -False -False -3 - + False -True -end -0 - - - - -True -False -0 -0 -label -True - - -False -True +False 1 - + True False -2 -2 -6 +True 6 +6 - + True False -1 -Password: +end +Username: -1 -2 +0 +0 - + True False -1 -Username: +end +Password: + +0 +1 + True True +4 +True + 1 -2 +0 True True +4 +True False True + 1 -2 1 -2 +Show password True True -False -Show password +False +True + 1 -2 2 -3 + + + False True -2 +0 - - button-cancel - button-ok - + + +True +False +emblem-ok-symbolic + + +True +False +Authentication required +True +:close + + +True +True +True +True +True +image1 +True + + + +end + + diff --git a/src/virt-viewer-auth.c b/src/virt-viewer-auth.c index 67c770c..73cc707 100644 --- a/src/virt-viewer-auth.c +++ b/src/virt-viewer-auth.c @@ -33,13 +33,23 @@ #include "virt-viewer-auth.h" #include "virt-viewer-util.h" -static void +void show_password(GtkCheckButton *check_button G_GNUC_UNUSED, GtkEntry *entry); +void button_ok_clicked(GtkButton *button G_GNUC_UNUSED, GtkDialog *dialog); + +void show_password(GtkCheckButton *check_button G_GNUC_UNUSED, GtkEntry *entry) { gtk_entry_set_visibility(entry, !gtk_entry_get_visibility(entry)); } +void +button_ok_clicked(GtkButton *button G_GNUC_UNUSED, + GtkDialog *dialog) +{ +gtk_dialog_response(dialog, GTK_RESPONSE_OK); +} + /* NOTE: if username is provided, and *username is non-NULL, the user input * field will be pre-filled with this value. The existing
[virt-tools-list] [PATCH virt-viewer 09/10] file-transfer-dialog: Use GtkHeaderBar
This dialog now presents the progress as a level bar, and also shows the percentage as on the text. The cancel button is not necessary anymore, because the all transfers are cancelled automatically when the dialog is closed before they finish. Signed-off-by: Eduardo Lima (Etrunko)--- .../ui/virt-viewer-file-transfer-dialog.ui | 72 +++--- src/virt-viewer-file-transfer-dialog.c | 59 -- 2 files changed, 48 insertions(+), 83 deletions(-) diff --git a/src/resources/ui/virt-viewer-file-transfer-dialog.ui b/src/resources/ui/virt-viewer-file-transfer-dialog.ui index 5e761c8..db46aea 100644 --- a/src/resources/ui/virt-viewer-file-transfer-dialog.ui +++ b/src/resources/ui/virt-viewer-file-transfer-dialog.ui @@ -1,22 +1,20 @@ + - - + -400 False -5 +12 +400 dialog -vertical True False -12 -12 +vertical +6 -horizontal True False end @@ -24,55 +22,34 @@ - -gtk-cancel -True -True -True -True - - -False -False -1 - + True True +3 + + + + +True +False +label +True + + +True +True 0 - -vertical + +20 True False -12 - - -True -False -label - - -True -True -0 - - - - -True -False - - -True -True -1 - - +0.149997764826 True @@ -82,8 +59,5 @@ - - button1 - diff --git a/src/virt-viewer-file-transfer-dialog.c b/src/virt-viewer-file-transfer-dialog.c index 07d25a7..62141c1 100644 --- a/src/virt-viewer-file-transfer-dialog.c +++ b/src/virt-viewer-file-transfer-dialog.c @@ -31,7 +31,7 @@ struct _VirtViewerFileTransferDialogPrivate guint timer_show_src; guint timer_hide_src; GtkWidget *transfer_summary; -GtkWidget *progressbar; +GtkWidget *levelbar; }; G_DEFINE_TYPE_WITH_PRIVATE(VirtViewerFileTransferDialog, virt_viewer_file_transfer_dialog, GTK_TYPE_DIALOG) @@ -66,52 +66,40 @@ virt_viewer_file_transfer_dialog_class_init(VirtViewerFileTransferDialogClass *k transfer_summary); gtk_widget_class_bind_template_child_private(widget_class, VirtViewerFileTransferDialog, - progressbar); + levelbar); object_class->dispose = virt_viewer_file_transfer_dialog_dispose; } -static void -dialog_response(GtkDialog *dialog, -gint response_id, -gpointer user_data G_GNUC_UNUSED) -{ -VirtViewerFileTransferDialog *self = VIRT_VIEWER_FILE_TRANSFER_DIALOG(dialog); -GSList *slist; - -switch (response_id) { -case GTK_RESPONSE_CANCEL: -/* cancel all current tasks */ -for (slist = self->priv->file_transfers; slist != NULL; slist = g_slist_next(slist)) { - spice_file_transfer_task_cancel(SPICE_FILE_TRANSFER_TASK(slist->data)); -} -break; -case GTK_RESPONSE_DELETE_EVENT: -/* silently ignore */ -break; -default: -g_warn_if_reached(); -} -} - static gboolean delete_event(GtkWidget *widget, GdkEvent *event G_GNUC_UNUSED, gpointer user_data G_GNUC_UNUSED) { -/* don't allow window to be deleted, just process the response signal, - * which may result in the window being hidden */ -gtk_dialog_response(GTK_DIALOG(widget), GTK_RESPONSE_CANCEL); +VirtViewerFileTransferDialog *self = VIRT_VIEWER_FILE_TRANSFER_DIALOG(widget); +GSList *slist; +for (slist = self->priv->file_transfers; slist != NULL; slist = g_slist_next(slist)) { +spice_file_transfer_task_cancel(SPICE_FILE_TRANSFER_TASK(slist->data)); +} return TRUE; } static void virt_viewer_file_transfer_dialog_init(VirtViewerFileTransferDialog *self) { +
[virt-tools-list] [PATCH virt-viewer 10/10] usb_device_selection: Use GtkHeaderBar
We could get rid of the auxiliary text and present it as the subtitle of the dialog, but this is handled by the widget itself. In this case, all we have to do is to set the "use-headerbar" property, and like other dialogs, the close button is not necessary anymore. Signed-off-by: Eduardo Lima (Etrunko)--- src/virt-viewer-session-spice.c | 20 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c index 9b52ec0..8177bcb 100644 --- a/src/virt-viewer-session-spice.c +++ b/src/virt-viewer-session-spice.c @@ -794,16 +794,20 @@ virt_viewer_session_spice_usb_device_selection(VirtViewerSession *session, { VirtViewerSessionSpice *self = VIRT_VIEWER_SESSION_SPICE(session); VirtViewerSessionSpicePrivate *priv = self->priv; -GtkWidget *dialog, *area, *usb_device_widget; +GtkWidget *dialog, *area, *usb_device_widget, *headerbar; /* Create the widgets */ -dialog = gtk_dialog_new_with_buttons(_("Select USB devices for redirection"), parent, - GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT, - _("_Close"), GTK_RESPONSE_ACCEPT, - NULL); -gtk_dialog_set_default_response(GTK_DIALOG(dialog), GTK_RESPONSE_ACCEPT); -gtk_container_set_border_width(GTK_CONTAINER(dialog), 12); -gtk_box_set_spacing(GTK_BOX(gtk_bin_get_child(GTK_BIN(dialog))), 12); +dialog = g_object_new(GTK_TYPE_DIALOG, + "title", _("Select USB devices for redirection"), + "transient-for", parent, + "use-header-bar", TRUE, + "border-width", 12, + "content-area-spacing", 12, + NULL); + +headerbar = gtk_dialog_get_header_bar(GTK_DIALOG(dialog)); +gtk_header_bar_set_show_close_button(GTK_HEADER_BAR(headerbar), TRUE); +gtk_header_bar_set_decoration_layout(GTK_HEADER_BAR(headerbar), ":close"); area = gtk_dialog_get_content_area(GTK_DIALOG(dialog)); -- 2.9.3 ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
[virt-tools-list] [PATCH virt-viewer 01/10] Update Gtk+ requirements to 3.12
This is new version ships with GtkHeaderBar code, which we will require for the dialogs on the application. In the future the main window could also make use of this widget, following the style of other applications. Signed-off-by: Eduardo Lima (Etrunko)--- configure.ac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 69e3708..d5eb258 100644 --- a/configure.ac +++ b/configure.ac @@ -17,8 +17,8 @@ GLIB2_REQUIRED="2.38" GLIB2_ENCODED_VERSION="GLIB_VERSION_2_38" # Keep these two definitions in agreement. -GTK_REQUIRED="3.10" -GTK_ENCODED_VERSION="GDK_VERSION_3_10" +GTK_REQUIRED="3.12" +GTK_ENCODED_VERSION="GDK_VERSION_3_12" LIBXML2_REQUIRED="2.6.0" LIBVIRT_REQUIRED="0.10.0" -- 2.9.3 ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
[virt-tools-list] [PATCH virt-viewer 02/10] iso-dialog: Use GtkHeaderBar
We now display the current ISO as subtitle on the HeaderBar. On the glade UI file, we get rid of the GtkAlignment object that was used to put some space between the tree view and dialog buttons. The "Select ISO" label is gone too. Signed-off-by: Eduardo Lima (Etrunko)--- src/remote-viewer-iso-list-dialog.c| 32 +-- src/resources/ui/remote-viewer-iso-list.ui | 87 +++--- 2 files changed, 59 insertions(+), 60 deletions(-) diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-viewer-iso-list-dialog.c index ed51ffa..2110d70 100644 --- a/src/remote-viewer-iso-list-dialog.c +++ b/src/remote-viewer-iso-list-dialog.c @@ -36,6 +36,7 @@ G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, GTK_TYPE struct _RemoteViewerISOListDialogPrivate { +GtkHeaderBar *header_bar; GtkListStore *list_store; GtkWidget *status; GtkWidget *spinner; @@ -121,6 +122,19 @@ remote_viewer_iso_list_dialog_show_files(RemoteViewerISOListDialog *self) } static void +remote_viewer_iso_list_dialog_set_subtitle(RemoteViewerISOListDialog *self, const char *iso_name) +{ +RemoteViewerISOListDialogPrivate *priv = self->priv; +gchar *subtitle = NULL; + +if (iso_name && strlen(iso_name) != 0) +subtitle = g_strdup_printf(_("Current: %s"), iso_name); + +gtk_header_bar_set_subtitle(priv->header_bar, subtitle); +g_free(subtitle); +} + +static void remote_viewer_iso_list_dialog_foreach(char *name, RemoteViewerISOListDialog *self) { RemoteViewerISOListDialogPrivate *priv = self->priv; @@ -140,6 +154,7 @@ remote_viewer_iso_list_dialog_foreach(char *name, RemoteViewerISOListDialog *sel gtk_tree_view_set_cursor(GTK_TREE_VIEW(priv->tree_view), path, NULL, FALSE); gtk_tree_view_scroll_to_cell(GTK_TREE_VIEW(priv->tree_view), path, NULL, TRUE, 0.5, 0.5); gtk_tree_path_free(path); +remote_viewer_iso_list_dialog_set_subtitle(self, current_iso); } g_free(current_iso); @@ -210,6 +225,7 @@ remote_viewer_iso_list_dialog_response(GtkDialog *dialog, gtk_spinner_start(GTK_SPINNER(priv->spinner)); gtk_label_set_markup(GTK_LABEL(priv->status), _("Loading...")); +remote_viewer_iso_list_dialog_set_subtitle(self, NULL); gtk_stack_set_visible_child_full(GTK_STACK(priv->stack), "status", GTK_STACK_TRANSITION_TYPE_NONE); gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE); @@ -265,9 +281,13 @@ remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self) RemoteViewerISOListDialogPrivate *priv = self->priv = DIALOG_PRIVATE(self); GtkBuilder *builder = virt_viewer_util_load_ui("remote-viewer-iso-list.ui"); GtkCellRendererToggle *cell_renderer; +GtkWidget *button, *image; gtk_builder_connect_signals(builder, self); +priv->header_bar = GTK_HEADER_BAR(gtk_dialog_get_header_bar(GTK_DIALOG(self))); +gtk_header_bar_set_has_subtitle(priv->header_bar, TRUE); + priv->status = GTK_WIDGET(gtk_builder_get_object(builder, "status")); priv->spinner = GTK_WIDGET(gtk_builder_get_object(builder, "spinner")); priv->stack = GTK_WIDGET(gtk_builder_get_object(builder, "stack")); @@ -281,12 +301,12 @@ remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self) g_object_unref(builder); -gtk_dialog_add_buttons(GTK_DIALOG(self), - _("Refresh"), GTK_RESPONSE_NONE, - _("Close"), GTK_RESPONSE_CLOSE, - NULL); +button = gtk_dialog_add_button(GTK_DIALOG(self), "", GTK_RESPONSE_NONE); +image = gtk_image_new_from_icon_name("view-refresh-symbolic", GTK_ICON_SIZE_BUTTON); +gtk_button_set_image(GTK_BUTTON(button), image); +gtk_button_set_always_show_image(GTK_BUTTON(button), TRUE); +gtk_widget_set_tooltip_text(button, _("Refresh")); -gtk_dialog_set_default_response(GTK_DIALOG(self), GTK_RESPONSE_CLOSE); gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE); g_signal_connect(self, "response", G_CALLBACK(remote_viewer_iso_list_dialog_response), NULL); } @@ -360,6 +380,7 @@ ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu, g_free(name); } while (gtk_tree_model_iter_next(model, )); +remote_viewer_iso_list_dialog_set_subtitle(self, current_iso); gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, TRUE); gtk_widget_set_sensitive(priv->tree_view, TRUE); g_free(current_iso); @@ -382,6 +403,7 @@ remote_viewer_iso_list_dialog_new(GtkWindow *parent, GObject *foreign_menu) "border-width", 18, "default-width", 400, "default-height", 300, + "use-header-bar", TRUE, "foreign-menu", foreign_menu,
[virt-tools-list] [PATCH virt-viewer 06/10] virt-viewer-preferences: Use GtkHeaderBar
This dialog changed very little when compared to the old version. I did change the way widgets are organized within the dialog, as it now groups the file selector together with the read-only checkbox. Signed-off-by: Eduardo Lima (Etrunko)--- src/resources/ui/virt-viewer-preferences.ui | 103 src/virt-viewer-app.c | 38 ++ 2 files changed, 82 insertions(+), 59 deletions(-) diff --git a/src/resources/ui/virt-viewer-preferences.ui b/src/resources/ui/virt-viewer-preferences.ui index f9738c5..c110335 100644 --- a/src/resources/ui/virt-viewer-preferences.ui +++ b/src/resources/ui/virt-viewer-preferences.ui @@ -1,28 +1,33 @@ + - - + + +True +False +Preferences +True +:close + + + + False -5 +6 Preferences normal - + True False +vertical - + True False end - - - - - - True @@ -31,88 +36,96 @@ - + True True +0 - + True False -18 +12 +vertical 6 True False -0 -Folder sharing - - - +start +bFolder sharing/b +True +True False -False +True 0 - + True False -6 -2 -2 -12 6 - - -Share folder -True -True -False -True - - - - - +6 Read-only True True False +start True -2 +1 1 -2 True False +4 +True select-folder 1 -2 +0 + + +Share Folder: +True +True +False +start +True + + +0 +0 + + + + + -False -False -1 +True +True +3 + +True + - + True False Spice @@ -125,7 +138,7 @@ True True -1 +0 diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c index 2e7e193..0d2d33b 100644 --- a/src/virt-viewer-app.c +++ b/src/virt-viewer-app.c @@ -2475,43 +2475,53 @@ static GtkWidget * virt_viewer_app_get_preferences(VirtViewerApp *self) { VirtViewerSession *session = virt_viewer_app_get_session(self); -GtkBuilder *builder = virt_viewer_util_load_ui("virt-viewer-preferences.ui"); gboolean can_share_folder = virt_viewer_session_can_share_folder(session); -GtkWidget *preferences = self->priv->preferences; +GtkWidget *headerbar, *check, *chooser, *readonly, *preferences =
[virt-tools-list] [PATCH virt-viewer 05/10] remote-viewer-connect: Use GtkHeaderBar
Just like the virt-viewer-connect dialog, the connect button has been moved to the header bar. Additionally, the example text is now presented as the subtitle. Signed-off-by: Eduardo Lima (Etrunko)--- src/remote-viewer-connect.c | 20 +--- src/resources/ui/remote-viewer-connect.ui | 173 +- 2 files changed, 54 insertions(+), 139 deletions(-) diff --git a/src/remote-viewer-connect.c b/src/remote-viewer-connect.c index 2fbc5ff..f3ac468 100644 --- a/src/remote-viewer-connect.c +++ b/src/remote-viewer-connect.c @@ -152,15 +152,6 @@ recent_item_activated_dialog_cb(GtkRecentChooser *chooser G_GNUC_UNUSED, gpointe shutdown_loop(ci->loop); } -static void -make_label_small(GtkLabel* label) -{ -PangoAttrList* attributes = pango_attr_list_new(); -pango_attr_list_insert(attributes, pango_attr_scale_new(0.9)); -gtk_label_set_attributes(label, attributes); -pango_attr_list_unref(attributes); -} - /** * remote_viewer_connect_dialog * @@ -174,7 +165,7 @@ make_label_small(GtkLabel* label) gboolean remote_viewer_connect_dialog(gchar **uri) { -GtkWidget *window, *label, *entry, *recent, *connect_button, *cancel_button; +GtkWidget *window, *header, *entry, *recent, *connect_button; GtkRecentFilter *rfilter; GtkBuilder *builder; gboolean active; @@ -192,13 +183,11 @@ remote_viewer_connect_dialog(gchar **uri) g_return_val_if_fail(builder != NULL, GTK_RESPONSE_NONE); window = GTK_WIDGET(gtk_builder_get_object(builder, "remote-viewer-connection-window")); +header = GTK_WIDGET(gtk_builder_get_object(builder, "headerbar")); +gtk_window_set_titlebar(GTK_WINDOW(window), header); connect_button = GTK_WIDGET(gtk_builder_get_object(builder, "connect-button")); -cancel_button = GTK_WIDGET(gtk_builder_get_object(builder, "cancel-button")); -label = GTK_WIDGET(gtk_builder_get_object(builder, "example-label")); entry = ci.entry = GTK_WIDGET(gtk_builder_get_object(builder, "connection-address-entry")); -make_label_small(GTK_LABEL(label)); - active = (gtk_entry_get_text_length(GTK_ENTRY(ci.entry)) > 0); gtk_widget_set_sensitive(GTK_WIDGET(connect_button), active); @@ -216,9 +205,6 @@ remote_viewer_connect_dialog(gchar **uri) g_signal_connect(connect_button, "clicked", G_CALLBACK(connect_button_clicked_cb), ); -/* make sure that user_data is passed as first parameter */ -g_signal_connect_swapped(cancel_button, "clicked", - G_CALLBACK(window_deleted_cb), ); g_signal_connect_swapped(window, "delete-event", G_CALLBACK(window_deleted_cb), ); diff --git a/src/resources/ui/remote-viewer-connect.ui b/src/resources/ui/remote-viewer-connect.ui index 308d121..89d17fd 100644 --- a/src/resources/ui/remote-viewer-connect.ui +++ b/src/resources/ui/remote-viewer-connect.ui @@ -1,63 +1,48 @@ - + + + +True +False +gtk-connect + + +True +False +Connection Details +For example, spice://foo.example.org:5900 +True +menu:close + + +True +True +True +Connect +image1 +True + + +end + + + +12 False -Connection details +12 +remote-viewer - + True False -10 -20 +vertical +6 - + True -False -6 - - -True -False -0 -Connection _Address -True -connection-address-entry - - - - - -False -True -0 - - - - -True -True - - -False -True -1 - - - - -True -False -0 -False -For example, spice://foo.example.org:5900 - - -False -True -2 - - +True False @@ -66,88 +51,32 @@ - + True False -6 - - -True -False -Recent connections -0 - - - - - -False -True -0 - - - - -
[virt-tools-list] [PATCH virt-viewer 04/10] virt-viewer-vm-connection: Use GtkHeaderBar
Here we moved the connect button to the header bar. Unfortunately, I could not find a "connect-symbolic" icon to be used here, so I ended up with the stock icon. Signed-off-by: Eduardo Lima (Etrunko)--- src/resources/ui/virt-viewer-vm-connection.ui | 83 +-- src/virt-viewer-vm-connection.c | 15 - 2 files changed, 40 insertions(+), 58 deletions(-) diff --git a/src/resources/ui/virt-viewer-vm-connection.ui b/src/resources/ui/virt-viewer-vm-connection.ui index f190c92..cb58701 100644 --- a/src/resources/ui/virt-viewer-vm-connection.ui +++ b/src/resources/ui/virt-viewer-vm-connection.ui @@ -1,6 +1,32 @@ - + + + +True +False +gtk-connect + + +True +False +Choose a virtual machine +True +menu:close + + +True +True +True +Connect +image1 +True + + +end + + + False 5 @@ -19,36 +45,6 @@ False end - - - _Cancel -True -True -True -True - - -False -True -0 - - - - -C_onnect -True -True -True -True -True -True - - -False -True -1 - - False @@ -86,32 +82,7 @@ 1 - - -True -False -0 -0 -4 -Available virtual machines -end -26 - - - - - -False -True -end -2 - - - - button-cancel - button-connect - diff --git a/src/virt-viewer-vm-connection.c b/src/virt-viewer-vm-connection.c index ebaa92b..0363b84 100644 --- a/src/virt-viewer-vm-connection.c +++ b/src/virt-viewer-vm-connection.c @@ -32,7 +32,7 @@ treeview_row_activated_cb(GtkTreeView *treeview G_GNUC_UNUSED, GtkTreeViewColumn *col G_GNUC_UNUSED, gpointer userdata) { -gtk_widget_activate(GTK_WIDGET(userdata)); +gtk_button_clicked(GTK_BUTTON(userdata)); } static void @@ -42,13 +42,20 @@ treeselection_changed_cb(GtkTreeSelection *selection, gpointer userdata) gtk_tree_selection_count_selected_rows(selection) == 1); } +static void +button_connect_clicked_cb(GtkButton *button G_GNUC_UNUSED, + GtkDialog *dialog) +{ +gtk_dialog_response(dialog, GTK_RESPONSE_ACCEPT); +} + gchar* virt_viewer_vm_connection_choose_name_dialog(GtkWindow *main_window, GtkTreeModel *model, GError **error) { GtkBuilder *vm_connection; -GtkWidget *dialog; +GtkWidget *dialog, *headerbar; GtkButton *button_connect; GtkTreeView *treeview; GtkTreeSelection *selection; @@ -69,12 +76,16 @@ virt_viewer_vm_connection_choose_name_dialog(GtkWindow *main_window, g_return_val_if_fail(vm_connection != NULL, NULL); dialog = GTK_WIDGET(gtk_builder_get_object(vm_connection, "vm-connection-dialog")); +headerbar = GTK_WIDGET(gtk_builder_get_object(vm_connection, "headerbar")); +gtk_window_set_titlebar(GTK_WINDOW(dialog), headerbar); gtk_window_set_transient_for(GTK_WINDOW(dialog), main_window); button_connect = GTK_BUTTON(gtk_builder_get_object(vm_connection, "button-connect")); treeview = GTK_TREE_VIEW(gtk_builder_get_object(vm_connection, "treeview")); selection = GTK_TREE_SELECTION(gtk_builder_get_object(vm_connection, "treeview-selection")); gtk_tree_view_set_model(treeview, model); +g_signal_connect(button_connect, "clicked", + G_CALLBACK(button_connect_clicked_cb), dialog); g_signal_connect(treeview, "row-activated", G_CALLBACK(treeview_row_activated_cb), button_connect); g_signal_connect(selection, "changed", -- 2.9.3 ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
[virt-tools-list] [PATCH virt-viewer 03/10] guest-details-dialog: Use GtkHeaderBar
This is the simplest dialog on the application, there was not much to change here. In order to not leave a big amount of unused space on the HeaderBar, we set the guest name as the subtitle. Signed-off-by: Eduardo Lima (Etrunko)--- src/resources/ui/virt-viewer-guest-details.ui | 64 +++ src/virt-viewer-window.c | 12 ++--- 2 files changed, 34 insertions(+), 42 deletions(-) diff --git a/src/resources/ui/virt-viewer-guest-details.ui b/src/resources/ui/virt-viewer-guest-details.ui index 209272f..7522049 100644 --- a/src/resources/ui/virt-viewer-guest-details.ui +++ b/src/resources/ui/virt-viewer-guest-details.ui @@ -1,11 +1,11 @@ - + - + False +12 Guest Details -True 400 dialog @@ -13,24 +13,13 @@ False vertical -2 +6 False end - -_Close -True -True -True -True - - -False -True -0 - + @@ -41,80 +30,81 @@ - + True False -6 6 6 -2 True False +bName:/b +True 1 -Name: -GTK_SHRINK | GTK_FILL -GTK_FILL +0 +0 True False +bGUID:/b +True 1 -GUID: +0 1 -2 -GTK_SHRINK | GTK_FILL -GTK_FILL True False -0 label True +0 1 -2 -GTK_FILL +0 True False -0 label True +0 1 -2 1 -2 -GTK_FILL False True -1 +0 - - button1 - + + +True +False +Guest Details +True +:close + + + diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c index 8eda12e..c75e75b 100644 --- a/src/virt-viewer-window.c +++ b/src/virt-viewer-window.c @@ -986,16 +986,17 @@ virt_viewer_window_menu_help_guest_details(GtkWidget *menu G_GNUC_UNUSED, VirtViewerWindow *self) { GtkBuilder *ui = virt_viewer_util_load_ui("virt-viewer-guest-details.ui"); +GtkWidget *dialog, *headerbar, *namelabel, *guidlabel; char *name = NULL; char *uuid = NULL; g_return_if_fail(ui != NULL); -GtkWidget *dialog = GTK_WIDGET(gtk_builder_get_object(ui, "guestdetailsdialog")); -GtkWidget *namelabel = GTK_WIDGET(gtk_builder_get_object(ui, "namevaluelabel")); -GtkWidget *guidlabel = GTK_WIDGET(gtk_builder_get_object(ui, "guidvaluelabel")); - -g_return_if_fail(dialog && namelabel && guidlabel); +dialog = GTK_WIDGET(gtk_builder_get_object(ui, "guestdetailsdialog")); +headerbar = GTK_WIDGET(gtk_builder_get_object(ui, "headerbar")); +gtk_window_set_titlebar(GTK_WINDOW(dialog), headerbar); +namelabel = GTK_WIDGET(gtk_builder_get_object(ui, "namevaluelabel")); +guidlabel = GTK_WIDGET(gtk_builder_get_object(ui, "guidvaluelabel")); g_object_get(self->priv->app, "guest-name", , "uuid", , NULL); @@ -1003,6 +1004,7 @@ virt_viewer_window_menu_help_guest_details(GtkWidget *menu G_GNUC_UNUSED, name = g_strdup(_("Unknown")); if (!uuid || *uuid == '\0') uuid = g_strdup(_("Unknown")); +gtk_header_bar_set_subtitle(GTK_HEADER_BAR(headerbar), name); gtk_label_set_text(GTK_LABEL(namelabel), name); gtk_label_set_text(GTK_LABEL(guidlabel), uuid); g_free(name); -- 2.9.3 ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
[virt-tools-list] [PATCH virt-viewer 07/10] about-dialog: Use GtkHeaderBar
For this one, we use the application version as the subtitle. Signed-off-by: Eduardo Lima (Etrunko)--- src/resources/ui/virt-viewer-about.ui | 20 ++-- src/virt-viewer-window.c | 9 ++--- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/resources/ui/virt-viewer-about.ui b/src/resources/ui/virt-viewer-about.ui index 28e38c8..a742ba8 100644 --- a/src/resources/ui/virt-viewer-about.ui +++ b/src/resources/ui/virt-viewer-about.ui @@ -1,10 +1,10 @@ + - + False -5 -About Virt-Viewer +6 False True center-on-parent @@ -36,6 +36,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA Marc-André Lureau The Fedora Translation Team +image-missing @@ -57,10 +58,17 @@ Marc-André Lureau 0 - - - + +True +False +About Virtual Machine Viewer +True +:close + + + + diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c index c75e75b..f7a7560 100644 --- a/src/virt-viewer-window.c +++ b/src/virt-viewer-window.c @@ -1034,14 +1034,17 @@ virt_viewer_window_menu_help_about(GtkWidget *menu G_GNUC_UNUSED, VirtViewerWindow *self) { GtkBuilder *about; -GtkWidget *dialog; +GtkWidget *dialog, *headerbar; GdkPixbuf *icon; +gchar *version = g_strdup_printf("%s %s %s", _("Version"), VERSION, BUILDID); about = virt_viewer_util_load_ui("virt-viewer-about.ui"); dialog = GTK_WIDGET(gtk_builder_get_object(about, "about")); - -gtk_about_dialog_set_version(GTK_ABOUT_DIALOG(dialog), VERSION BUILDID); +headerbar = GTK_WIDGET(gtk_builder_get_object(about, "headerbar")); +gtk_window_set_titlebar(GTK_WINDOW(dialog), headerbar); +gtk_header_bar_set_subtitle(GTK_HEADER_BAR(headerbar), version); +g_free(version); icon = gdk_pixbuf_new_from_resource(VIRT_VIEWER_RESOURCE_PREFIX"/icons/48x48/virt-viewer.png", NULL); if (icon != NULL) { -- 2.9.3 ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer v2] iso-dialog: Avoid crash when closing dialog early
Hi Eduardo, On Thu, 2017-02-09 at 15:13 -0200, Eduardo Lima (Etrunko) wrote: > We must take into account that users can close the dialog at > anytime, > even during an operation of fetch or set ISO has not been finished. > This > will cause the callbacks for those operations to be invoked with an > invalid object, crashing the application. > > To fix this issue we need to pass a GCancellable to the asynchronous > operations, so they can be cancelled if the dialog happens to get > closed > before they complete. > > Signed-off-by: Eduardo Lima (Etrunko)> --- > v2: > * Move call to g_cancellable_cancel() to response handler. > * Don't leak error objects if operation is cancelled. > --- > src/remote-viewer-iso-list-dialog.c | 42 > ++--- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote- > viewer-iso-list-dialog.c > index 2ab5435..ed51ffa 100644 > --- a/src/remote-viewer-iso-list-dialog.c > +++ b/src/remote-viewer-iso-list-dialog.c > @@ -42,6 +42,7 @@ struct _RemoteViewerISOListDialogPrivate > GtkWidget *stack; > GtkWidget *tree_view; > OvirtForeignMenu *foreign_menu; > +GCancellable *cancellable; > }; > > enum RemoteViewerISOListDialogModel > @@ -66,6 +67,8 @@ remote_viewer_iso_list_dialog_dispose(GObject > *object) > RemoteViewerISOListDialog *self = > REMOTE_VIEWER_ISO_LIST_DIALOG(object); > RemoteViewerISOListDialogPrivate *priv = self->priv; > > +g_clear_object(>cancellable); > + > if (priv->foreign_menu) { > g_signal_handlers_disconnect_by_data(priv->foreign_menu, > object); > g_clear_object(>foreign_menu); > @@ -157,17 +160,24 @@ fetch_iso_names_cb(OvirtForeignMenu > *foreign_menu, > const gchar *msg = error ? error->message : _("Failed to > fetch CD names"); > gchar *markup = g_strdup_printf("%s", msg); > > +g_debug("Error fetching ISO names: %s", msg); > +if (error && error->code == G_IO_ERROR_CANCELLED) use g_error_matches if possible > +goto end; > + > gtk_label_set_markup(GTK_LABEL(priv->status), markup); > gtk_spinner_stop(GTK_SPINNER(priv->spinner)); > remote_viewer_iso_list_dialog_show_error(self, msg); > gtk_dialog_set_response_sensitive(GTK_DIALOG(self), > GTK_RESPONSE_NONE, TRUE); > g_free(markup); > -g_clear_error(); > -return; > +goto end; > } > > +g_clear_object(>cancellable); > g_list_foreach(iso_list, (GFunc) > remote_viewer_iso_list_dialog_foreach, self); > remote_viewer_iso_list_dialog_show_files(self); > + > +end: > +g_clear_error(); > } > > > @@ -177,7 +187,10 @@ > remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOListDi > alog *self) > RemoteViewerISOListDialogPrivate *priv = self->priv; > > gtk_list_store_clear(priv->list_store); > -ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, > NULL, > + > +priv->cancellable = g_cancellable_new(); > +ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, > + priv->cancellable, > (GAsyncReadyCallback) > fetch_iso_names_cb, > self); > } > @@ -190,8 +203,10 @@ > remote_viewer_iso_list_dialog_response(GtkDialog *dialog, > RemoteViewerISOListDialog *self = > REMOTE_VIEWER_ISO_LIST_DIALOG(dialog); > RemoteViewerISOListDialogPrivate *priv = self->priv; > > -if (response_id != GTK_RESPONSE_NONE) > +if (response_id != GTK_RESPONSE_NONE) { > +g_cancellable_cancel(priv->cancellable); > return; > +} > > gtk_spinner_start(GTK_SPINNER(priv->spinner)); > gtk_label_set_markup(GTK_LABEL(priv->status), > _("Loading...")); > @@ -223,7 +238,9 @@ > remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle > *cell_renderer G_GNU > gtk_dialog_set_response_sensitive(GTK_DIALOG(self), > GTK_RESPONSE_NONE, FALSE); > gtk_widget_set_sensitive(priv->tree_view, FALSE); > > -ovirt_foreign_menu_set_current_iso_name_async(priv- > >foreign_menu, active ? NULL : name, NULL, > +priv->cancellable = g_cancellable_new(); > +ovirt_foreign_menu_set_current_iso_name_async(priv- > >foreign_menu, active ? NULL : name, > + priv- > >cancellable, > (GAsyncReadyCallb > ack)ovirt_foreign_menu_iso_name_changed, > self); > gtk_tree_path_free(tree_path); > @@ -308,12 +325,18 @@ > ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu, > * change the ISO back to the original, avoiding a possible > inconsistency. > */ > if > (!ovirt_foreign_menu_set_current_iso_name_finish(foreign_menu, > result, )) { > -
Re: [virt-tools-list] [virt-viewer updated PATCH] Show errors generated by connection dialog
On Thu, 2017-02-09 at 13:00 +0100, Christophe de Dinechin wrote: > When running 'remote-viewer' without arguments, > and selecting a non-supported protocol, e.g. ssh://foo, > the generated error was silently swallowed by the retry loop. > This was introduced in 06b2c382468876a19600374bd59475e66d488af8. > --- > src/remote-viewer.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/remote-viewer.c b/src/remote-viewer.c > index 13c6e7c..e4b0909 100644 > --- a/src/remote-viewer.c > +++ b/src/remote-viewer.c > @@ -1196,6 +1196,9 @@ cleanup: > type = NULL; > > if (!ret && priv->open_recent_dialog) { > +if (error != NULL) { > +virt_viewer_app_simple_message_dialog(app, _("Unable to > connect: %s"), error->message); > +} > g_clear_error(); > goto retry_dialog; > } Since other types of errors already result in error dialogs, I was initially concerned that this was going to display 2 error dialogs for for some error cases. But it doesn't. For my own sake (and maybe for others), I'll just quickly describe why: Let's take the case where you specify a valid protocol and a valid hostname, but there is no spice server listening on the given port. In this case, virt_viewer_app_initial_connect() actually returns TRUE as soon as we start trying to connect, and no error is set. So remote_viewer_start() exits cleanly and doesn't execute the 'goto retry_dialog' statement. The connection continues asynchronously in the background, and when the connection fails we will get a 'session- disconnected' signal. The handler function for this signal displays an error dialog and calls into virt_viewer_app_deactivate() which eventually ends in the vfunc remote_viewer_deactivated() being called, which then re-opens the connection dialog if necessary. So the error-reporting and re-trying paths for these two types of errors are completely different. It might be nice to try to unify this a bit at some point. But at the moment, it seems that this patch solves the problem. Jonathon ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer PATCH] Test error->message and, if NULL, use a default message
I tend to agree. I don’t see how any legitimate GError * (from g_error_new or the like) would have a NULL message. So this is really defensive coding. Alternate solution would be to assert, but I don’t like killing the viewer if this ever happens. Christophe > On 9 Feb 2017, at 17:44, Jonathon Jongsmawrote: > > ...now I see that this was discussed in the review thread for your > other patch. I still think this should not be necessary. If error is > non-NULL, it should be guaranteed to have a non-NULL message. > > Jonathon > > > On Thu, 2017-02-09 at 10:40 -0600, Jonathon Jongsma wrote: >> Out of curiosity, what was the situation where you encountered a non- >> NULL error with a NULL error->message? It doesn't seem like this >> should >> be necessary. >> >> Jonathon >> >> >> >> On Thu, 2017-02-09 at 13:02 +0100, Christophe de Dinechin wrote: >>> --- >>> src/ovirt-foreign-menu.c | 14 +++--- >>> src/remote-viewer.c| 22 -- >>> src/virt-viewer-app.c | 14 +++--- >>> src/virt-viewer-file-transfer-dialog.c | 2 +- >>> src/virt-viewer-file.c | 6 +++--- >>> src/virt-viewer-session-spice.c| 8 >>> src/virt-viewer-util.c | 4 >>> src/virt-viewer-util.h | 3 +++ >>> src/virt-viewer.c | 10 +- >>> 9 files changed, 46 insertions(+), 37 deletions(-) >>> >>> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c >>> index a51f2c9..8a2507f 100644 >>> --- a/src/ovirt-foreign-menu.c >>> +++ b/src/ovirt-foreign-menu.c >>> @@ -367,7 +367,7 @@ static void updated_cdrom_cb(GObject >>> *source_object, >>> const char *current_file = foreign_menu->priv- current_iso_name; >>> >>> >>> if (error != NULL) { >>> -g_warning("failed to update cdrom resource: %s", >>> error- message); >>> >>> +g_warning("failed to update cdrom resource: %s", >>> VV_MSG(error->message)); >>> g_clear_error(); >>> } >>> g_debug("setting OvirtCdrom:file back to '%s'", >>> @@ -504,7 +504,7 @@ static void cdrom_file_refreshed_cb(GObject >>> *source_object, >>> >>> ovirt_resource_refresh_finish(cdrom, result, ); >>> if (error != NULL) { >>> -g_warning("failed to refresh cdrom content: %s", error- message); >>> >>> +g_warning("failed to refresh cdrom content: %s", >>> VV_MSG(error->message)); >>> g_clear_error(); >>> return; >>> } >>> @@ -548,7 +548,7 @@ static void cdroms_fetched_cb(GObject >>> *source_object, >>> >>> ovirt_collection_fetch_finish(cdrom_collection, result, >>> ); >>> if (error != NULL) { >>> -g_warning("failed to fetch cdrom collection: %s", error- message); >>> >>> +g_warning("failed to fetch cdrom collection: %s", >>> VV_MSG(error->message)); >>> g_clear_error(); >>> return; >>> } >>> @@ -600,7 +600,7 @@ static void storage_domains_fetched_cb(GObject >>> *source_object, >>> >>> ovirt_collection_fetch_finish(collection, result, ); >>> if (error != NULL) { >>> -g_warning("failed to fetch storage domains: %s", error- message); >>> >>> +g_warning("failed to fetch storage domains: %s", >>> VV_MSG(error->message)); >>> g_clear_error(); >>> return; >>> } >>> @@ -663,7 +663,7 @@ static void vms_fetched_cb(GObject >>> *source_object, >>> collection = OVIRT_COLLECTION(source_object); >>> ovirt_collection_fetch_finish(collection, result, ); >>> if (error != NULL) { >>> -g_debug("failed to fetch VM list: %s", error->message); >>> +g_debug("failed to fetch VM list: %s", VV_MSG(error- message)); >>> >>> g_clear_error(); >>> return; >>> } >>> @@ -713,7 +713,7 @@ static void api_fetched_cb(GObject >>> *source_object, >>> proxy = OVIRT_PROXY(source_object); >>> menu->priv->api = ovirt_proxy_fetch_api_finish(proxy, result, >>> ); >>> if (error != NULL) { >>> -g_debug("failed to fetch toplevel API object: %s", error- message); >>> >>> +g_debug("failed to fetch toplevel API object: %s", >>> VV_MSG(error->message)); >>> g_clear_error(); >>> return; >>> } >>> @@ -746,7 +746,7 @@ static void iso_list_fetched_cb(GObject >>> *source_object, >>> ovirt_collection_fetch_finish(collection, result, ); >>> if (error != NULL) { >>> g_warning("failed to fetch files for ISO storage domain: >>> %s", >>> - error->message); >>> + VV_MSG(error->message)); >>> g_clear_error(); >>> return; >>> } >>> diff --git a/src/remote-viewer.c b/src/remote-viewer.c >>> index e4b0909..e4470ae 100644 >>> --- a/src/remote-viewer.c >>> +++ b/src/remote-viewer.c >>> @@ -278,7 +278,7 @@ spice_ctrl_do_connect(SpiceCtrlController *ctrl >>> G_GNUC_UNUSED, >>>
[virt-tools-list] [PATCH virt-viewer v2] session-spice: Pass hostname to authentication dialog
With this patch the dialog now shows the host we are connecting to. Signed-off-by: Eduardo Lima (Etrunko)--- v2: Use proper uri if connecting via proxy. --- src/virt-viewer-session-spice.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c index c3fce48..9b52ec0 100644 --- a/src/virt-viewer-session-spice.c +++ b/src/virt-viewer-session-spice.c @@ -691,6 +691,7 @@ virt_viewer_session_spice_main_channel_event(SpiceChannel *channel, case SPICE_CHANNEL_ERROR_AUTH: { const GError *error = NULL; +gchar *host = NULL; g_debug("main channel: auth failure (wrong username/password?)"); { @@ -717,11 +718,13 @@ virt_viewer_session_spice_main_channel_event(SpiceChannel *channel, user = g_strdup(g_get_user_name()); } +g_object_get(self->priv->session, "host", , NULL); ret = virt_viewer_auth_collect_credentials(self->priv->main_window, "SPICE", - NULL, + host, username_required ? : NULL, ); +g_free(host); if (!ret) { g_signal_emit_by_name(session, "session-cancelled"); } else { @@ -750,7 +753,7 @@ virt_viewer_session_spice_main_channel_event(SpiceChannel *channel, g_warn_if_fail(proxy != NULL); ret = virt_viewer_auth_collect_credentials(self->priv->main_window, - "proxy", NULL, + "proxy", spice_uri_get_hostname(proxy), , ); if (!ret) { g_signal_emit_by_name(session, "session-cancelled"); -- 2.9.3 ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer v2] iso-dialog: Avoid crash when closing dialog early
On 09/02/17 15:13, Eduardo Lima (Etrunko) wrote: > We must take into account that users can close the dialog at anytime, > even during an operation of fetch or set ISO has not been finished. This > will cause the callbacks for those operations to be invoked with an > invalid object, crashing the application. > > To fix this issue we need to pass a GCancellable to the asynchronous > operations, so they can be cancelled if the dialog happens to get closed > before they complete. I have modified this commit message with a link to the libgovirt bug: NOTE: This patch triggers a deadlock in libgovirt when the dialog is closed before the operation completes. Bug reported in https://bugzilla.gnome.org/show_bug.cgi?id=777808. We will need to bump libgovirt dependency whenever it has a new release. > > Signed-off-by: Eduardo Lima (Etrunko)> --- > v2: > * Move call to g_cancellable_cancel() to response handler. > * Don't leak error objects if operation is cancelled. > --- > src/remote-viewer-iso-list-dialog.c | 42 > ++--- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/src/remote-viewer-iso-list-dialog.c > b/src/remote-viewer-iso-list-dialog.c > index 2ab5435..ed51ffa 100644 > --- a/src/remote-viewer-iso-list-dialog.c > +++ b/src/remote-viewer-iso-list-dialog.c > @@ -42,6 +42,7 @@ struct _RemoteViewerISOListDialogPrivate > GtkWidget *stack; > GtkWidget *tree_view; > OvirtForeignMenu *foreign_menu; > +GCancellable *cancellable; > }; > > enum RemoteViewerISOListDialogModel > @@ -66,6 +67,8 @@ remote_viewer_iso_list_dialog_dispose(GObject *object) > RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object); > RemoteViewerISOListDialogPrivate *priv = self->priv; > > +g_clear_object(>cancellable); > + > if (priv->foreign_menu) { > g_signal_handlers_disconnect_by_data(priv->foreign_menu, object); > g_clear_object(>foreign_menu); > @@ -157,17 +160,24 @@ fetch_iso_names_cb(OvirtForeignMenu *foreign_menu, > const gchar *msg = error ? error->message : _("Failed to fetch CD > names"); > gchar *markup = g_strdup_printf("%s", msg); > > +g_debug("Error fetching ISO names: %s", msg); > +if (error && error->code == G_IO_ERROR_CANCELLED) > +goto end; > + > gtk_label_set_markup(GTK_LABEL(priv->status), markup); > gtk_spinner_stop(GTK_SPINNER(priv->spinner)); > remote_viewer_iso_list_dialog_show_error(self, msg); > gtk_dialog_set_response_sensitive(GTK_DIALOG(self), > GTK_RESPONSE_NONE, TRUE); > g_free(markup); > -g_clear_error(); > -return; > +goto end; > } > > +g_clear_object(>cancellable); > g_list_foreach(iso_list, (GFunc) remote_viewer_iso_list_dialog_foreach, > self); > remote_viewer_iso_list_dialog_show_files(self); > + > +end: > +g_clear_error(); > } > > > @@ -177,7 +187,10 @@ > remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOListDialog > *self) > RemoteViewerISOListDialogPrivate *priv = self->priv; > > gtk_list_store_clear(priv->list_store); > -ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, NULL, > + > +priv->cancellable = g_cancellable_new(); > +ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, > + priv->cancellable, > (GAsyncReadyCallback) > fetch_iso_names_cb, > self); > } > @@ -190,8 +203,10 @@ remote_viewer_iso_list_dialog_response(GtkDialog *dialog, > RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog); > RemoteViewerISOListDialogPrivate *priv = self->priv; > > -if (response_id != GTK_RESPONSE_NONE) > +if (response_id != GTK_RESPONSE_NONE) { > +g_cancellable_cancel(priv->cancellable); > return; > +} > > gtk_spinner_start(GTK_SPINNER(priv->spinner)); > gtk_label_set_markup(GTK_LABEL(priv->status), _("Loading...")); > @@ -223,7 +238,9 @@ > remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle *cell_renderer > G_GNU > gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, > FALSE); > gtk_widget_set_sensitive(priv->tree_view, FALSE); > > -ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active > ? NULL : name, NULL, > +priv->cancellable = g_cancellable_new(); > +ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active > ? NULL : name, > + priv->cancellable, > > (GAsyncReadyCallback)ovirt_foreign_menu_iso_name_changed, >self); > gtk_tree_path_free(tree_path); > @@ -308,12 +325,18 @@
[virt-tools-list] [PATCH virt-viewer v2] iso-dialog: Avoid crash when closing dialog early
We must take into account that users can close the dialog at anytime, even during an operation of fetch or set ISO has not been finished. This will cause the callbacks for those operations to be invoked with an invalid object, crashing the application. To fix this issue we need to pass a GCancellable to the asynchronous operations, so they can be cancelled if the dialog happens to get closed before they complete. Signed-off-by: Eduardo Lima (Etrunko)--- v2: * Move call to g_cancellable_cancel() to response handler. * Don't leak error objects if operation is cancelled. --- src/remote-viewer-iso-list-dialog.c | 42 ++--- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-viewer-iso-list-dialog.c index 2ab5435..ed51ffa 100644 --- a/src/remote-viewer-iso-list-dialog.c +++ b/src/remote-viewer-iso-list-dialog.c @@ -42,6 +42,7 @@ struct _RemoteViewerISOListDialogPrivate GtkWidget *stack; GtkWidget *tree_view; OvirtForeignMenu *foreign_menu; +GCancellable *cancellable; }; enum RemoteViewerISOListDialogModel @@ -66,6 +67,8 @@ remote_viewer_iso_list_dialog_dispose(GObject *object) RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object); RemoteViewerISOListDialogPrivate *priv = self->priv; +g_clear_object(>cancellable); + if (priv->foreign_menu) { g_signal_handlers_disconnect_by_data(priv->foreign_menu, object); g_clear_object(>foreign_menu); @@ -157,17 +160,24 @@ fetch_iso_names_cb(OvirtForeignMenu *foreign_menu, const gchar *msg = error ? error->message : _("Failed to fetch CD names"); gchar *markup = g_strdup_printf("%s", msg); +g_debug("Error fetching ISO names: %s", msg); +if (error && error->code == G_IO_ERROR_CANCELLED) +goto end; + gtk_label_set_markup(GTK_LABEL(priv->status), markup); gtk_spinner_stop(GTK_SPINNER(priv->spinner)); remote_viewer_iso_list_dialog_show_error(self, msg); gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, TRUE); g_free(markup); -g_clear_error(); -return; +goto end; } +g_clear_object(>cancellable); g_list_foreach(iso_list, (GFunc) remote_viewer_iso_list_dialog_foreach, self); remote_viewer_iso_list_dialog_show_files(self); + +end: +g_clear_error(); } @@ -177,7 +187,10 @@ remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOListDialog *self) RemoteViewerISOListDialogPrivate *priv = self->priv; gtk_list_store_clear(priv->list_store); -ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, NULL, + +priv->cancellable = g_cancellable_new(); +ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, + priv->cancellable, (GAsyncReadyCallback) fetch_iso_names_cb, self); } @@ -190,8 +203,10 @@ remote_viewer_iso_list_dialog_response(GtkDialog *dialog, RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog); RemoteViewerISOListDialogPrivate *priv = self->priv; -if (response_id != GTK_RESPONSE_NONE) +if (response_id != GTK_RESPONSE_NONE) { +g_cancellable_cancel(priv->cancellable); return; +} gtk_spinner_start(GTK_SPINNER(priv->spinner)); gtk_label_set_markup(GTK_LABEL(priv->status), _("Loading...")); @@ -223,7 +238,9 @@ remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle *cell_renderer G_GNU gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE); gtk_widget_set_sensitive(priv->tree_view, FALSE); -ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active ? NULL : name, NULL, +priv->cancellable = g_cancellable_new(); +ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active ? NULL : name, + priv->cancellable, (GAsyncReadyCallback)ovirt_foreign_menu_iso_name_changed, self); gtk_tree_path_free(tree_path); @@ -308,12 +325,18 @@ ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu, * change the ISO back to the original, avoiding a possible inconsistency. */ if (!ovirt_foreign_menu_set_current_iso_name_finish(foreign_menu, result, )) { -remote_viewer_iso_list_dialog_show_error(self, error ? error->message : _("Failed to change CD")); -g_clear_error(); +const gchar *msg = error ? error->message : _("Failed to change CD"); +g_debug("Error changing ISO: %s", msg); + +if (error && error->code == G_IO_ERROR_CANCELLED) +goto end; + +
Re: [virt-tools-list] [PATCH virt-viewer] iso-dialog: Avoid crash when closing dialog early
Looks like this v2 never made to the list? I am sending it again. On 03/02/17 15:28, Eduardo Lima (Etrunko) wrote: > We must take into account that users can close the dialog at anytime, > even during an operation of fetch or set ISO has not been finished. This > will cause the callbacks for those operations to be invoked with an > invalid object, crashing the application. > > To fix this issue we need to pass a GCancellable to the asynchronous > operations, so they can be cancelled if the dialog happens to get closed > before they complete. > > Signed-off-by: Eduardo Lima (Etrunko)> --- > v2: > * Move call to g_cancellable_cancel() to response handler. > * Don't leak error objects if operation is cancelled. > > --- > src/remote-viewer-iso-list-dialog.c | 42 > ++--- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/src/remote-viewer-iso-list-dialog.c > b/src/remote-viewer-iso-list-dialog.c > index f23ddb2..aa0ebbb 100644 > --- a/src/remote-viewer-iso-list-dialog.c > +++ b/src/remote-viewer-iso-list-dialog.c > @@ -42,6 +42,7 @@ struct _RemoteViewerISOListDialogPrivate > GtkWidget *stack; > GtkWidget *tree_view; > OvirtForeignMenu *foreign_menu; > +GCancellable *cancellable; > }; > > enum RemoteViewerISOListDialogModel > @@ -66,6 +67,8 @@ remote_viewer_iso_list_dialog_dispose(GObject *object) > RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object); > RemoteViewerISOListDialogPrivate *priv = self->priv; > > +g_clear_object(>cancellable); > + > if (priv->foreign_menu) { > g_signal_handlers_disconnect_by_data(priv->foreign_menu, object); > g_clear_object(>foreign_menu); > @@ -157,17 +160,24 @@ fetch_iso_names_cb(OvirtForeignMenu *foreign_menu, > const gchar *msg = error ? error->message : _("Failed to fetch CD > names"); > gchar *markup = g_strdup_printf("%s", msg); > > +g_debug("Error fetching ISO names: %s", msg); > +if (error && error->code == G_IO_ERROR_CANCELLED) > +goto end; > + > gtk_label_set_markup(GTK_LABEL(priv->status), markup); > gtk_spinner_stop(GTK_SPINNER(priv->spinner)); > remote_viewer_iso_list_dialog_show_error(self, msg); > gtk_dialog_set_response_sensitive(GTK_DIALOG(self), > GTK_RESPONSE_NONE, TRUE); > g_free(markup); > -g_clear_error(); > -return; > +goto end; > } > > +g_clear_object(>cancellable); > g_list_foreach(iso_list, (GFunc) remote_viewer_iso_list_dialog_foreach, > self); > remote_viewer_iso_list_dialog_show_files(self); > + > +end: > +g_clear_error(); > } > > > @@ -177,7 +187,10 @@ > remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOListDialog > *self) > RemoteViewerISOListDialogPrivate *priv = self->priv; > > gtk_list_store_clear(priv->list_store); > -ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, NULL, > + > +priv->cancellable = g_cancellable_new(); > +ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, > + priv->cancellable, > (GAsyncReadyCallback) > fetch_iso_names_cb, > self); > } > @@ -190,8 +203,10 @@ remote_viewer_iso_list_dialog_response(GtkDialog *dialog, > RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog); > RemoteViewerISOListDialogPrivate *priv = self->priv; > > -if (response_id != GTK_RESPONSE_NONE) > +if (response_id != GTK_RESPONSE_NONE) { > +g_cancellable_cancel(priv->cancellable); > return; > +} > > gtk_spinner_start(GTK_SPINNER(priv->spinner)); > gtk_label_set_markup(GTK_LABEL(priv->status), _("Loading...")); > @@ -223,7 +238,9 @@ > remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle *cell_renderer > G_GNU > gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, > FALSE); > gtk_widget_set_sensitive(priv->tree_view, FALSE); > > -ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active > ? NULL : name, NULL, > +priv->cancellable = g_cancellable_new(); > +ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active > ? NULL : name, > + priv->cancellable, > > (GAsyncReadyCallback)ovirt_foreign_menu_iso_name_changed, >self); > gtk_tree_path_free(tree_path); > @@ -308,12 +325,18 @@ ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu > *foreign_menu, > * change the ISO back to the original, avoiding a possible > inconsistency. > */ > if (!ovirt_foreign_menu_set_current_iso_name_finish(foreign_menu, > result, )) { > -
Re: [virt-tools-list] [virt-viewer PATCH] Test error->message and, if NULL, use a default message
...now I see that this was discussed in the review thread for your other patch. I still think this should not be necessary. If error is non-NULL, it should be guaranteed to have a non-NULL message. Jonathon On Thu, 2017-02-09 at 10:40 -0600, Jonathon Jongsma wrote: > Out of curiosity, what was the situation where you encountered a non- > NULL error with a NULL error->message? It doesn't seem like this > should > be necessary. > > Jonathon > > > > On Thu, 2017-02-09 at 13:02 +0100, Christophe de Dinechin wrote: > > --- > > src/ovirt-foreign-menu.c | 14 +++--- > > src/remote-viewer.c| 22 -- > > src/virt-viewer-app.c | 14 +++--- > > src/virt-viewer-file-transfer-dialog.c | 2 +- > > src/virt-viewer-file.c | 6 +++--- > > src/virt-viewer-session-spice.c| 8 > > src/virt-viewer-util.c | 4 > > src/virt-viewer-util.h | 3 +++ > > src/virt-viewer.c | 10 +- > > 9 files changed, 46 insertions(+), 37 deletions(-) > > > > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c > > index a51f2c9..8a2507f 100644 > > --- a/src/ovirt-foreign-menu.c > > +++ b/src/ovirt-foreign-menu.c > > @@ -367,7 +367,7 @@ static void updated_cdrom_cb(GObject > > *source_object, > > const char *current_file = foreign_menu->priv- > > > current_iso_name; > > > > > > if (error != NULL) { > > -g_warning("failed to update cdrom resource: %s", > > error- > > > message); > > > > +g_warning("failed to update cdrom resource: %s", > > VV_MSG(error->message)); > > g_clear_error(); > > } > > g_debug("setting OvirtCdrom:file back to '%s'", > > @@ -504,7 +504,7 @@ static void cdrom_file_refreshed_cb(GObject > > *source_object, > > > > ovirt_resource_refresh_finish(cdrom, result, ); > > if (error != NULL) { > > -g_warning("failed to refresh cdrom content: %s", error- > > > message); > > > > +g_warning("failed to refresh cdrom content: %s", > > VV_MSG(error->message)); > > g_clear_error(); > > return; > > } > > @@ -548,7 +548,7 @@ static void cdroms_fetched_cb(GObject > > *source_object, > > > > ovirt_collection_fetch_finish(cdrom_collection, result, > > ); > > if (error != NULL) { > > -g_warning("failed to fetch cdrom collection: %s", error- > > > message); > > > > +g_warning("failed to fetch cdrom collection: %s", > > VV_MSG(error->message)); > > g_clear_error(); > > return; > > } > > @@ -600,7 +600,7 @@ static void storage_domains_fetched_cb(GObject > > *source_object, > > > > ovirt_collection_fetch_finish(collection, result, ); > > if (error != NULL) { > > -g_warning("failed to fetch storage domains: %s", error- > > > message); > > > > +g_warning("failed to fetch storage domains: %s", > > VV_MSG(error->message)); > > g_clear_error(); > > return; > > } > > @@ -663,7 +663,7 @@ static void vms_fetched_cb(GObject > > *source_object, > > collection = OVIRT_COLLECTION(source_object); > > ovirt_collection_fetch_finish(collection, result, ); > > if (error != NULL) { > > -g_debug("failed to fetch VM list: %s", error->message); > > +g_debug("failed to fetch VM list: %s", VV_MSG(error- > > > message)); > > > > g_clear_error(); > > return; > > } > > @@ -713,7 +713,7 @@ static void api_fetched_cb(GObject > > *source_object, > > proxy = OVIRT_PROXY(source_object); > > menu->priv->api = ovirt_proxy_fetch_api_finish(proxy, result, > > ); > > if (error != NULL) { > > -g_debug("failed to fetch toplevel API object: %s", error- > > > message); > > > > +g_debug("failed to fetch toplevel API object: %s", > > VV_MSG(error->message)); > > g_clear_error(); > > return; > > } > > @@ -746,7 +746,7 @@ static void iso_list_fetched_cb(GObject > > *source_object, > > ovirt_collection_fetch_finish(collection, result, ); > > if (error != NULL) { > > g_warning("failed to fetch files for ISO storage domain: > > %s", > > - error->message); > > + VV_MSG(error->message)); > > g_clear_error(); > > return; > > } > > diff --git a/src/remote-viewer.c b/src/remote-viewer.c > > index e4b0909..e4470ae 100644 > > --- a/src/remote-viewer.c > > +++ b/src/remote-viewer.c > > @@ -278,7 +278,7 @@ spice_ctrl_do_connect(SpiceCtrlController *ctrl > > G_GNUC_UNUSED, > > GError *error = NULL; > > > > if (!virt_viewer_app_initial_connect(self, )) { > > -const gchar *msg = error ? error->message : > > +const gchar *msg = error ? VV_MSG(error->message) : > > _("Failed to initiate connection"); > >
Re: [virt-tools-list] [virt-viewer PATCH] Test error->message and, if NULL, use a default message
Out of curiosity, what was the situation where you encountered a non- NULL error with a NULL error->message? It doesn't seem like this should be necessary. Jonathon On Thu, 2017-02-09 at 13:02 +0100, Christophe de Dinechin wrote: > --- > src/ovirt-foreign-menu.c | 14 +++--- > src/remote-viewer.c| 22 -- > src/virt-viewer-app.c | 14 +++--- > src/virt-viewer-file-transfer-dialog.c | 2 +- > src/virt-viewer-file.c | 6 +++--- > src/virt-viewer-session-spice.c| 8 > src/virt-viewer-util.c | 4 > src/virt-viewer-util.h | 3 +++ > src/virt-viewer.c | 10 +- > 9 files changed, 46 insertions(+), 37 deletions(-) > > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c > index a51f2c9..8a2507f 100644 > --- a/src/ovirt-foreign-menu.c > +++ b/src/ovirt-foreign-menu.c > @@ -367,7 +367,7 @@ static void updated_cdrom_cb(GObject > *source_object, > const char *current_file = foreign_menu->priv- > >current_iso_name; > > if (error != NULL) { > -g_warning("failed to update cdrom resource: %s", error- > >message); > +g_warning("failed to update cdrom resource: %s", > VV_MSG(error->message)); > g_clear_error(); > } > g_debug("setting OvirtCdrom:file back to '%s'", > @@ -504,7 +504,7 @@ static void cdrom_file_refreshed_cb(GObject > *source_object, > > ovirt_resource_refresh_finish(cdrom, result, ); > if (error != NULL) { > -g_warning("failed to refresh cdrom content: %s", error- > >message); > +g_warning("failed to refresh cdrom content: %s", > VV_MSG(error->message)); > g_clear_error(); > return; > } > @@ -548,7 +548,7 @@ static void cdroms_fetched_cb(GObject > *source_object, > > ovirt_collection_fetch_finish(cdrom_collection, result, ); > if (error != NULL) { > -g_warning("failed to fetch cdrom collection: %s", error- > >message); > +g_warning("failed to fetch cdrom collection: %s", > VV_MSG(error->message)); > g_clear_error(); > return; > } > @@ -600,7 +600,7 @@ static void storage_domains_fetched_cb(GObject > *source_object, > > ovirt_collection_fetch_finish(collection, result, ); > if (error != NULL) { > -g_warning("failed to fetch storage domains: %s", error- > >message); > +g_warning("failed to fetch storage domains: %s", > VV_MSG(error->message)); > g_clear_error(); > return; > } > @@ -663,7 +663,7 @@ static void vms_fetched_cb(GObject > *source_object, > collection = OVIRT_COLLECTION(source_object); > ovirt_collection_fetch_finish(collection, result, ); > if (error != NULL) { > -g_debug("failed to fetch VM list: %s", error->message); > +g_debug("failed to fetch VM list: %s", VV_MSG(error- > >message)); > g_clear_error(); > return; > } > @@ -713,7 +713,7 @@ static void api_fetched_cb(GObject > *source_object, > proxy = OVIRT_PROXY(source_object); > menu->priv->api = ovirt_proxy_fetch_api_finish(proxy, result, > ); > if (error != NULL) { > -g_debug("failed to fetch toplevel API object: %s", error- > >message); > +g_debug("failed to fetch toplevel API object: %s", > VV_MSG(error->message)); > g_clear_error(); > return; > } > @@ -746,7 +746,7 @@ static void iso_list_fetched_cb(GObject > *source_object, > ovirt_collection_fetch_finish(collection, result, ); > if (error != NULL) { > g_warning("failed to fetch files for ISO storage domain: > %s", > - error->message); > + VV_MSG(error->message)); > g_clear_error(); > return; > } > diff --git a/src/remote-viewer.c b/src/remote-viewer.c > index e4b0909..e4470ae 100644 > --- a/src/remote-viewer.c > +++ b/src/remote-viewer.c > @@ -278,7 +278,7 @@ spice_ctrl_do_connect(SpiceCtrlController *ctrl > G_GNUC_UNUSED, > GError *error = NULL; > > if (!virt_viewer_app_initial_connect(self, )) { > -const gchar *msg = error ? error->message : > +const gchar *msg = error ? VV_MSG(error->message) : > _("Failed to initiate connection"); > virt_viewer_app_simple_message_dialog(self, msg); > g_clear_error(); > @@ -591,7 +591,7 @@ spice_ctrl_listen_async_cb(GObject *object, > if (error != NULL) { > virt_viewer_app_simple_message_dialog(app, > _("Controller > connection failed: %s"), > - error->message); > + VV_MSG(error- > >message)); > g_clear_error(); > exit(EXIT_FAILURE); /* TODO: make start async? */ > } > @@ -859,7 +859,7 @@
Re: [virt-tools-list] [virt-viewer PATCH] Show errors generated by connection dialog
On Thu, 2017-02-09 at 14:22 +0100, Christophe de Dinechin wrote: > > > > > > If the message is NULL, we should still display something like > > > “unknown error”. If we think it’s worth testing for error- > > > >message > > > != NULL, then I suggest another patch just for that, that fixes > > > all > > > places and not just this one. > > > > I agree > > Separate patch sent. > > > > > BTW, I did not find where the localization strings for the > > > project > > > were stored. How does localization happen ? If I add a new > > > message, > > > who should I warn to have the message translated? > > > > It is handled in zanata https://fedora.zanata.org/iteration/view/v > > irt- > > viewer/master?dswid=3794 > > Thanks. I guess my question is: how is the list of translatable > strings updated? Is that done automatically as part of the build? irrc you must download translation from zanata and do `make -C po update-po` > > I’m asking because if I touch a source file and then run ‘make’ in > the po directory, nothing is made. There is a list of source files. > So I think it should update the .po files. I’m afraid I’m not doing > something I should do to make it clear a new string was added > (besides adding the _(“foo”) marker). Am I supposed to run xgettext > manually? In Tao3D, we have a ‘make lupdate’ target for this > purpose, the Qt tool being called lupdate. > yeah, that make update-po should work > > Thanks, > Christophe > ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer PATCH] Show errors generated by connection dialog
>> >> If the message is NULL, we should still display something like >> “unknown error”. If we think it’s worth testing for error->message >> != NULL, then I suggest another patch just for that, that fixes all >> places and not just this one. > I agree Separate patch sent. >> BTW, I did not find where the localization strings for the project >> were stored. How does localization happen ? If I add a new message, >> who should I warn to have the message translated? > It is handled in zanata https://fedora.zanata.org/iteration/view/virt- > viewer/master?dswid=3794 Thanks. I guess my question is: how is the list of translatable strings updated? Is that done automatically as part of the build? I’m asking because if I touch a source file and then run ‘make’ in the po directory, nothing is made. There is a list of source files. So I think it should update the .po files. I’m afraid I’m not doing something I should do to make it clear a new string was added (besides adding the _(“foo”) marker). Am I supposed to run xgettext manually? In Tao3D, we have a ‘make lupdate’ target for this purpose, the Qt tool being called lupdate. Thanks, Christophe ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer PATCH] Show errors generated by connection dialog
On Thu, 2017-02-09 at 12:10 +0100, Christophe de Dinechin wrote: > Thanks for the comments. > > > On 9 Feb 2017, at 11:23, Pavel Gruntwrote: > > > > Hello Christophe, > > > > On Wed, 2017-02-08 at 17:48 +0100, Christophe de Dinechin wrote: > > > When running 'remote-viewer' without arguments, > > > and selecting a non-supported protocol, e.g. ssh://foo, > > > the generated error was silently swallowed by the retry loop. > > > This was introduced in 06b2c382468876a19600374bd59475e66d488af8. > > > --- > > > src/remote-viewer.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/src/remote-viewer.c b/src/remote-viewer.c > > > index 13c6e7c..c9ef4bb 100644 > > > --- a/src/remote-viewer.c > > > +++ b/src/remote-viewer.c > > > @@ -1196,6 +1196,9 @@ cleanup: > > > type = NULL; > > > > > > if (!ret && priv->open_recent_dialog) { > > > +if (error) { > > > > in case of pointers we prefer an explicit comparison to NULL (just > > a > > style - which is not followed…) > > OK > > > > > so it can be error != NULL && error->message != NULL to make > > everyone > > happy. > > If the message is NULL, we should still display something like > “unknown error”. If we think it’s worth testing for error->message > != NULL, then I suggest another patch just for that, that fixes all > places and not just this one. I agree > > > > Although as you said the error->message is never checked... > > otoh if the message is NULL then an empty dialog would appear. > > > > > +virt_viewer_app_simple_message_dialog( > > > >parent, > > > > the first param can be app > > OK. > > > > > > "%s", error->message); > > > > the string will be displayed to the user, so it should be > > translated - > > in case of the empty message, it should say something. > The incoming string is already translated, I think. > > BTW, I did not find where the localization strings for the project > were stored. How does localization happen ? If I add a new message, > who should I warn to have the message translated? It is handled in zanata https://fedora.zanata.org/iteration/view/virt- viewer/master?dswid=3794 > > > > > maybe "Unable to connect" error->message > > Today, the message is for example > Unsupported graphic type ‘tap’ > > Is it really an improvement to have: > Unable to connect: Unsupported graphic type ‘tap’ I think "Unable to connect unknown error" is better than just "unknown error" > > ? > > I personally don’t mind either way. > > ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
[virt-tools-list] [virt-viewer PATCH] Test error->message and, if NULL, use a default message
--- src/ovirt-foreign-menu.c | 14 +++--- src/remote-viewer.c| 22 -- src/virt-viewer-app.c | 14 +++--- src/virt-viewer-file-transfer-dialog.c | 2 +- src/virt-viewer-file.c | 6 +++--- src/virt-viewer-session-spice.c| 8 src/virt-viewer-util.c | 4 src/virt-viewer-util.h | 3 +++ src/virt-viewer.c | 10 +- 9 files changed, 46 insertions(+), 37 deletions(-) diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c index a51f2c9..8a2507f 100644 --- a/src/ovirt-foreign-menu.c +++ b/src/ovirt-foreign-menu.c @@ -367,7 +367,7 @@ static void updated_cdrom_cb(GObject *source_object, const char *current_file = foreign_menu->priv->current_iso_name; if (error != NULL) { -g_warning("failed to update cdrom resource: %s", error->message); +g_warning("failed to update cdrom resource: %s", VV_MSG(error->message)); g_clear_error(); } g_debug("setting OvirtCdrom:file back to '%s'", @@ -504,7 +504,7 @@ static void cdrom_file_refreshed_cb(GObject *source_object, ovirt_resource_refresh_finish(cdrom, result, ); if (error != NULL) { -g_warning("failed to refresh cdrom content: %s", error->message); +g_warning("failed to refresh cdrom content: %s", VV_MSG(error->message)); g_clear_error(); return; } @@ -548,7 +548,7 @@ static void cdroms_fetched_cb(GObject *source_object, ovirt_collection_fetch_finish(cdrom_collection, result, ); if (error != NULL) { -g_warning("failed to fetch cdrom collection: %s", error->message); +g_warning("failed to fetch cdrom collection: %s", VV_MSG(error->message)); g_clear_error(); return; } @@ -600,7 +600,7 @@ static void storage_domains_fetched_cb(GObject *source_object, ovirt_collection_fetch_finish(collection, result, ); if (error != NULL) { -g_warning("failed to fetch storage domains: %s", error->message); +g_warning("failed to fetch storage domains: %s", VV_MSG(error->message)); g_clear_error(); return; } @@ -663,7 +663,7 @@ static void vms_fetched_cb(GObject *source_object, collection = OVIRT_COLLECTION(source_object); ovirt_collection_fetch_finish(collection, result, ); if (error != NULL) { -g_debug("failed to fetch VM list: %s", error->message); +g_debug("failed to fetch VM list: %s", VV_MSG(error->message)); g_clear_error(); return; } @@ -713,7 +713,7 @@ static void api_fetched_cb(GObject *source_object, proxy = OVIRT_PROXY(source_object); menu->priv->api = ovirt_proxy_fetch_api_finish(proxy, result, ); if (error != NULL) { -g_debug("failed to fetch toplevel API object: %s", error->message); +g_debug("failed to fetch toplevel API object: %s", VV_MSG(error->message)); g_clear_error(); return; } @@ -746,7 +746,7 @@ static void iso_list_fetched_cb(GObject *source_object, ovirt_collection_fetch_finish(collection, result, ); if (error != NULL) { g_warning("failed to fetch files for ISO storage domain: %s", - error->message); + VV_MSG(error->message)); g_clear_error(); return; } diff --git a/src/remote-viewer.c b/src/remote-viewer.c index e4b0909..e4470ae 100644 --- a/src/remote-viewer.c +++ b/src/remote-viewer.c @@ -278,7 +278,7 @@ spice_ctrl_do_connect(SpiceCtrlController *ctrl G_GNUC_UNUSED, GError *error = NULL; if (!virt_viewer_app_initial_connect(self, )) { -const gchar *msg = error ? error->message : +const gchar *msg = error ? VV_MSG(error->message) : _("Failed to initiate connection"); virt_viewer_app_simple_message_dialog(self, msg); g_clear_error(); @@ -591,7 +591,7 @@ spice_ctrl_listen_async_cb(GObject *object, if (error != NULL) { virt_viewer_app_simple_message_dialog(app, _("Controller connection failed: %s"), - error->message); + VV_MSG(error->message)); g_clear_error(); exit(EXIT_FAILURE); /* TODO: make start async? */ } @@ -859,7 +859,7 @@ create_ovirt_session(VirtViewerApp *app, const char *uri, GError **err) api = ovirt_proxy_fetch_api(proxy, ); if (error != NULL) { -g_debug("failed to get oVirt 'api' collection: %s", error->message); +g_debug("failed to get oVirt 'api' collection: %s", VV_MSG(error->message)); #ifdef HAVE_OVIRT_CANCEL if (g_error_matches(error, OVIRT_REST_CALL_ERROR, OVIRT_REST_CALL_ERROR_CANCELLED)) { g_clear_error(); @@ -873,7 +873,7 @@
[virt-tools-list] [virt-viewer updated PATCH] Show errors generated by connection dialog
When running 'remote-viewer' without arguments, and selecting a non-supported protocol, e.g. ssh://foo, the generated error was silently swallowed by the retry loop. This was introduced in 06b2c382468876a19600374bd59475e66d488af8. --- src/remote-viewer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/remote-viewer.c b/src/remote-viewer.c index 13c6e7c..e4b0909 100644 --- a/src/remote-viewer.c +++ b/src/remote-viewer.c @@ -1196,6 +1196,9 @@ cleanup: type = NULL; if (!ret && priv->open_recent_dialog) { +if (error != NULL) { +virt_viewer_app_simple_message_dialog(app, _("Unable to connect: %s"), error->message); +} g_clear_error(); goto retry_dialog; } -- 2.9.3 ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer PATCH] Show errors generated by connection dialog
Thanks for the comments. > On 9 Feb 2017, at 11:23, Pavel Gruntwrote: > > Hello Christophe, > > On Wed, 2017-02-08 at 17:48 +0100, Christophe de Dinechin wrote: >> When running 'remote-viewer' without arguments, >> and selecting a non-supported protocol, e.g. ssh://foo, >> the generated error was silently swallowed by the retry loop. >> This was introduced in 06b2c382468876a19600374bd59475e66d488af8. >> --- >> src/remote-viewer.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/src/remote-viewer.c b/src/remote-viewer.c >> index 13c6e7c..c9ef4bb 100644 >> --- a/src/remote-viewer.c >> +++ b/src/remote-viewer.c >> @@ -1196,6 +1196,9 @@ cleanup: >>type = NULL; >> >>if (!ret && priv->open_recent_dialog) { >> +if (error) { > in case of pointers we prefer an explicit comparison to NULL (just a > style - which is not followed…) OK > > so it can be error != NULL && error->message != NULL to make everyone > happy. If the message is NULL, we should still display something like “unknown error”. If we think it’s worth testing for error->message != NULL, then I suggest another patch just for that, that fixes all places and not just this one. > Although as you said the error->message is never checked... > otoh if the message is NULL then an empty dialog would appear. > >> +virt_viewer_app_simple_message_dialog(>parent, > the first param can be app OK. > >> "%s", error->message); > > the string will be displayed to the user, so it should be translated - > in case of the empty message, it should say something. The incoming string is already translated, I think. BTW, I did not find where the localization strings for the project were stored. How does localization happen ? If I add a new message, who should I warn to have the message translated? > > maybe "Unable to connect" error->message Today, the message is for example Unsupported graphic type ‘tap’ Is it really an improvement to have: Unable to connect: Unsupported graphic type ‘tap’ ? I personally don’t mind either way. ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [virt-viewer PATCH] Show errors generated by connection dialog
Hello Christophe, On Wed, 2017-02-08 at 17:48 +0100, Christophe de Dinechin wrote: > When running 'remote-viewer' without arguments, > and selecting a non-supported protocol, e.g. ssh://foo, > the generated error was silently swallowed by the retry loop. > This was introduced in 06b2c382468876a19600374bd59475e66d488af8. > --- > src/remote-viewer.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/remote-viewer.c b/src/remote-viewer.c > index 13c6e7c..c9ef4bb 100644 > --- a/src/remote-viewer.c > +++ b/src/remote-viewer.c > @@ -1196,6 +1196,9 @@ cleanup: > type = NULL; > > if (!ret && priv->open_recent_dialog) { > +if (error) { in case of pointers we prefer an explicit comparison to NULL (just a style - which is not followed...) so it can be error != NULL && error->message != NULL to make everyone happy. Although as you said the error->message is never checked... otoh if the message is NULL then an empty dialog would appear. > +virt_viewer_app_simple_message_dialog(>parent, the first param can be app > "%s", error->message); the string will be displayed to the user, so it should be translated - in case of the empty message, it should say something. maybe "Unable to connect" error->message Thanks, Pavel P.S.: Looking at the code I can see dialogs having mentioned issues... > +} > g_clear_error(); > goto retry_dialog; > } ___ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list