Re: [virt-tools-list] [virt-manager PATCH] Tiny inspection improvements

[Cole Robinson]

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

[Cole Robinson]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Pavel Grunt]

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

[Jonathon Jongsma]

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

[Christophe de Dinechin]

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 Jongsma  wrote:
> 
> ...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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Eduardo Lima (Etrunko)]

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

[Jonathon Jongsma]

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

[Jonathon Jongsma]

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

[Pavel Grunt]

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

[Christophe de Dinechin]

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

[Pavel Grunt]

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 Grunt  wrote:
> > 
> > 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

[Christophe de Dinechin]

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

[Christophe de Dinechin]

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

[Christophe de Dinechin]

Thanks for the comments.

> On 9 Feb 2017, at 11:23, Pavel Grunt  wrote:
> 
> 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

[Pavel Grunt]

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