Re: [virt-tools-list] [PATCH virt-viewer 04/11] ovirt-foreign-menu: Add accessors for current iso and iso list

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 12:32 PM, Christophe Fergeau wrote:
> On Sun, Jul 17, 2016 at 11:13:04PM -0300, Eduardo Lima (Etrunko) wrote:
>> Signed-off-by: Eduardo Lima (Etrunko) 
>> ---
>>  src/ovirt-foreign-menu.c | 49 
>> ++--
>>  src/ovirt-foreign-menu.h |  5 -
>>  2 files changed, 39 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
>> index b071e27..2446239 100644
>> --- a/src/ovirt-foreign-menu.c
>> +++ b/src/ovirt-foreign-menu.c
>> @@ -47,6 +47,7 @@ static void 
>> ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu
>>  static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu);
>>  static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu 
>> *menu);
>>  static void ovirt_foreign_menu_fetch_iso_list_async(OvirtForeignMenu *menu);
>> +static void updated_cdrom_cb(GObject *source_object, GAsyncResult *result, 
>> gpointer user_data);
>>  
>>  G_DEFINE_TYPE (OvirtForeignMenu, ovirt_foreign_menu, G_TYPE_OBJECT)
>>  
>> @@ -85,7 +86,7 @@ enum {
>>  };
>>  
>>  
>> -static char *
>> +char *
>>  ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu *foreign_menu)
>>  {
>>  char *name;
>> @@ -100,6 +101,36 @@ 
>> ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu *foreign_menu)
>>  }
>>  
>>  
>> +void
>> +ovirt_foreign_menu_set_current_iso_name(OvirtForeignMenu *foreign_menu, 
>> char *name)
>> +{
> 
> For what it's worth, this is a bit misleading as this going to trigger
> an async update of the ISO name, and this sets "next_iso_name" more than
> "current_iso_name". I think you need to expose this an async method
> anyway, so that you can catch failures to change the ISO (ie the REST
> API call failed).
> 

Okay, I will work on it. I really did it the laziest way possible,
without any error treatment indeed.

-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com



signature.asc
Description: OpenPGP digital signature
___
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 v2 07/11] UI: Make 'Change CD' menu item a submenu under 'File' toplevel menu

2016-07-19 Thread Eduardo Lima (Etrunko) 
Signed-off-by: Eduardo Lima (Etrunko) 
---
 src/resources/ui/virt-viewer.ui | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/resources/ui/virt-viewer.ui b/src/resources/ui/virt-viewer.ui
index 6e3c5ad..af3ae46 100644
--- a/src/resources/ui/virt-viewer.ui
+++ b/src/resources/ui/virt-viewer.ui
@@ -1,6 +1,7 @@
 
+
 
-  
+  
   
   
 False
@@ -73,6 +74,13 @@
   
 
 
+  
+False
+_Change 
CD
+True
+  
+
+
   
 True
 False
@@ -247,14 +255,6 @@
 
   
 
-
-  
-False
-False
-_Change 
CD
-True
-  
-
   
   
 False
-- 
2.7.4

___
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 v2 10/11] remote-viewer: Make ovirt-foreign-menu a property

2016-07-19 Thread Eduardo Lima (Etrunko) 
The OvirtForeignMenu pointer is needed by the new ISO list dialog, and
we make it acessible via property to avoid interdependency between
objects.

Signed-off-by: Eduardo Lima (Etrunko) 
---
 src/remote-viewer-iso-list-dialog.c | 25 -
 src/remote-viewer-iso-list-dialog.h |  2 +-
 src/remote-viewer.c | 37 +
 src/virt-viewer-window.c| 14 +-
 4 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/src/remote-viewer-iso-list-dialog.c 
b/src/remote-viewer-iso-list-dialog.c
index 3ede716..045d601 100644
--- a/src/remote-viewer-iso-list-dialog.c
+++ b/src/remote-viewer-iso-list-dialog.c
@@ -24,6 +24,7 @@
 
 #include "remote-viewer-iso-list-dialog.h"
 #include "virt-viewer-util.h"
+#include "ovirt-foreign-menu.h"
 
 G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, 
GTK_TYPE_DIALOG)
 
@@ -33,6 +34,7 @@ G_DEFINE_TYPE(RemoteViewerISOListDialog, 
remote_viewer_iso_list_dialog, GTK_TYPE
 struct _RemoteViewerISOListDialogPrivate
 {
 GtkWidget *notebook;
+OvirtForeignMenu *foreign_menu;
 };
 
 enum RemoteViewerIsoListDialogPages
@@ -44,6 +46,10 @@ enum RemoteViewerIsoListDialogPages
 static void
 remote_viewer_iso_list_dialog_dispose(GObject *object)
 {
+RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object);
+RemoteViewerISOListDialogPrivate *priv = self->priv = DIALOG_PRIVATE(self);
+
+g_clear_object(&priv->foreign_menu);
 
G_OBJECT_CLASS(remote_viewer_iso_list_dialog_parent_class)->dispose(object);
 }
 
@@ -102,10 +108,19 @@ 
remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self)
 }
 
 GtkWidget *
-remote_viewer_iso_list_dialog_new(GtkWindow *parent)
+remote_viewer_iso_list_dialog_new(GtkWindow *parent, GObject *foreign_menu)
 {
-return g_object_new(REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG,
-"title", _("Change CD"),
-"transient-for", parent,
-NULL);
+GtkWidget *dialog;
+RemoteViewerISOListDialog *self;
+
+g_return_val_if_fail(foreign_menu != NULL, NULL);
+
+dialog = g_object_new(REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG,
+ "title", _("Change CD"),
+ "transient-for", parent,
+ NULL);
+
+self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
+self->priv->foreign_menu = OVIRT_FOREIGN_MENU(g_object_ref(foreign_menu));
+return dialog;
 }
diff --git a/src/remote-viewer-iso-list-dialog.h 
b/src/remote-viewer-iso-list-dialog.h
index def841b..8b936f5 100644
--- a/src/remote-viewer-iso-list-dialog.h
+++ b/src/remote-viewer-iso-list-dialog.h
@@ -51,7 +51,7 @@ struct _RemoteViewerISOListDialogClass
 
 GType remote_viewer_iso_list_dialog_get_type(void) G_GNUC_CONST;
 
-GtkWidget *remote_viewer_iso_list_dialog_new(GtkWindow *parent);
+GtkWidget *remote_viewer_iso_list_dialog_new(GtkWindow *parent, GObject 
*foreign_menu);
 
 G_END_DECLS
 
diff --git a/src/remote-viewer.c b/src/remote-viewer.c
index 95130dc..e0cd139 100644
--- a/src/remote-viewer.c
+++ b/src/remote-viewer.c
@@ -67,6 +67,13 @@ G_DEFINE_TYPE (RemoteViewer, remote_viewer, 
VIRT_VIEWER_TYPE_APP)
 #define GET_PRIVATE(o)\
 (G_TYPE_INSTANCE_GET_PRIVATE ((o), REMOTE_VIEWER_TYPE, 
RemoteViewerPrivate))
 
+enum RemoteViewerProperties {
+PROP_0,
+#ifdef HAVE_OVIRT
+PROP_OVIRT_FOREIGN_MENU,
+#endif
+};
+
 #ifdef HAVE_OVIRT
 static OvirtVm * choose_vm(GtkWindow *main_window,
char **vm_name,
@@ -213,6 +220,25 @@ end:
 }
 
 static void
+remote_viewer_get_property(GObject *object, guint property_id,
+   GValue *value, GParamSpec *pspec)
+{
+RemoteViewer *self = REMOTE_VIEWER(object);
+RemoteViewerPrivate *priv = self->priv;
+
+switch (property_id) {
+#ifdef HAVE_OVIRT
+case PROP_OVIRT_FOREIGN_MENU:
+g_value_set_object(value, priv->ovirt_foreign_menu);
+break;
+#endif
+
+default:
+G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
+}
+}
+
+static void
 remote_viewer_class_init (RemoteViewerClass *klass)
 {
 GObjectClass *object_class = G_OBJECT_CLASS (klass);
@@ -222,6 +248,7 @@ remote_viewer_class_init (RemoteViewerClass *klass)
 
 g_type_class_add_private (klass, sizeof (RemoteViewerPrivate));
 
+object_class->get_property = remote_viewer_get_property;
 object_class->dispose = remote_viewer_dispose;
 
 g_app_class->local_command_line = remote_viewer_local_command_line;
@@ -235,6 +262,16 @@ remote_viewer_class_init (RemoteViewerClass *klass)
 #else
 (void) gtk_app_class;
 #endif
+
+#ifdef HAVE_OVIRT
+g_object_class_install_property(object_class,
+PROP_OVIRT_FOREIGN_MENU,
+g_param_spec_object("ovirt-foreign-menu",
+

Re: [virt-tools-list] [PATCH virt-viewer 10/11] remote-viewer: Make ovirt-foreign-menu a property

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 02:54 PM, Eduardo Lima (Etrunko) wrote:
> On 07/19/2016 12:57 PM, Christophe Fergeau wrote:
>>> +
>>> +self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
>>> +self->priv->foreign_menu = foreign_menu;
>>
>> I'd g_object_ref it if you need to have it around (together with 
>> g_clear_object
>> in dispose/finalize).
> 
> Okay, fixed.
> 
>>> +
>>> +#ifdef HAVE_OVIRT
>>> +g_object_class_install_property(object_class,
>>> +PROP_OVIRT_FOREIGN_MENU,
>>> +
>>> g_param_spec_pointer("ovirt-foreign-menu",
>>
>> This can be a g_param_spec_object, OvirtForeignMenu is a GObject.
> 
> Also fixed.
> 
>>
>>> + "oVirt Foreign 
>>> Menu",
>>> + "Object which is 
>>> used as interface to oVirt",
>>> + G_PARAM_READABLE 
>>> | G_PARAM_STATIC_STRINGS));
>>> +#endif
>>>  }
>>>  
>>>  static void
>>> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
>>> index 867fb86..76fe80f 100644
>>> --- a/src/virt-viewer-window.c
>>> +++ b/src/virt-viewer-window.c
>>> @@ -1072,11 +1072,25 @@ 
>>> virt_viewer_window_menu_change_cd_activate(GtkWidget *menu G_GNUC_UNUSED,
>>>  {
>>>  VirtViewerWindowPrivate *priv = self->priv;
>>>  static GtkWidget *dialog = NULL;
>>> +GValue foreign_menu = G_VALUE_INIT;
>>>  
>>>  if (dialog)
>>>  return;
>>>  
>>> -dialog = remote_viewer_iso_list_dialog_new(GTK_WINDOW(priv->window));
>>> +g_value_init(&foreign_menu, G_TYPE_POINTER);
>>> +g_object_get_property(G_OBJECT(priv->app), "ovirt-foreign-menu", 
>>> &foreign_menu);
>>
>> You can use g_object_get(G_OBJECT(priv->app), "ovirt-foreign_menu", 
>> &foreign_menu, NULL);
>> rather than a GValue.
>>
> 
> Thanks, fixed too.
> 

Actually I did make it a pointer on purpose, because I did not want to
have OvirtForeignMenu type in this file. Maybe I should change it to
GObject then?

-- 
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 10/11] remote-viewer: Make ovirt-foreign-menu a property

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 12:57 PM, Christophe Fergeau wrote:
>> +
>> +self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
>> +self->priv->foreign_menu = foreign_menu;
> 
> I'd g_object_ref it if you need to have it around (together with 
> g_clear_object
> in dispose/finalize).

Okay, fixed.

>> +
>> +#ifdef HAVE_OVIRT
>> +g_object_class_install_property(object_class,
>> +PROP_OVIRT_FOREIGN_MENU,
>> +
>> g_param_spec_pointer("ovirt-foreign-menu",
> 
> This can be a g_param_spec_object, OvirtForeignMenu is a GObject.

Also fixed.

> 
>> + "oVirt Foreign 
>> Menu",
>> + "Object which is 
>> used as interface to oVirt",
>> + G_PARAM_READABLE | 
>> G_PARAM_STATIC_STRINGS));
>> +#endif
>>  }
>>  
>>  static void
>> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
>> index 867fb86..76fe80f 100644
>> --- a/src/virt-viewer-window.c
>> +++ b/src/virt-viewer-window.c
>> @@ -1072,11 +1072,25 @@ virt_viewer_window_menu_change_cd_activate(GtkWidget 
>> *menu G_GNUC_UNUSED,
>>  {
>>  VirtViewerWindowPrivate *priv = self->priv;
>>  static GtkWidget *dialog = NULL;
>> +GValue foreign_menu = G_VALUE_INIT;
>>  
>>  if (dialog)
>>  return;
>>  
>> -dialog = remote_viewer_iso_list_dialog_new(GTK_WINDOW(priv->window));
>> +g_value_init(&foreign_menu, G_TYPE_POINTER);
>> +g_object_get_property(G_OBJECT(priv->app), "ovirt-foreign-menu", 
>> &foreign_menu);
> 
> You can use g_object_get(G_OBJECT(priv->app), "ovirt-foreign_menu", 
> &foreign_menu, NULL);
> rather than a GValue.
> 

Thanks, fixed too.

-- 
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 08/11] Introduce ISO List dialog

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 01:13 PM, Christophe Fergeau wrote:
> On Sun, Jul 17, 2016 at 11:13:08PM -0300, Eduardo Lima (Etrunko) wrote:
>> Signed-off-by: Eduardo Lima (Etrunko) 
>> ---
>>  src/Makefile.am|   3 +
>>  src/remote-viewer-iso-list-dialog.c| 115 +++
>>  src/remote-viewer-iso-list-dialog.h|  58 ++
>>  src/resources/ui/remote-viewer-iso-list.ui | 174 
>> +
>>  src/resources/virt-viewer.gresource.xml|   1 +
>>  5 files changed, 351 insertions(+)
>>  create mode 100644 src/remote-viewer-iso-list-dialog.c
>>  create mode 100644 src/remote-viewer-iso-list-dialog.h
>>  create mode 100644 src/resources/ui/remote-viewer-iso-list.ui
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 0c48e40..66a73f8 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -13,6 +13,7 @@ noinst_DATA = \
>>  resources/ui/virt-viewer-vm-connection.ui \
>>  resources/ui/virt-viewer-preferences.ui \
>>  resources/ui/remote-viewer-connect.ui \
>> +resources/ui/remote-viewer-iso-list.ui \
>>  $(NULL)
>>  
>>  EXTRA_DIST =\
>> @@ -96,6 +97,8 @@ if HAVE_OVIRT
>>  libvirt_viewer_la_SOURCES += \
>>  ovirt-foreign-menu.h \
>>  ovirt-foreign-menu.c \
>> +remote-viewer-iso-list-dialog.c \
>> +remote-viewer-iso-list-dialog.h \
>>  $(NULL)
>>  endif
>>  
>> diff --git a/src/remote-viewer-iso-list-dialog.c 
>> b/src/remote-viewer-iso-list-dialog.c
>> new file mode 100644
>> index 000..b3972ac
>> --- /dev/null
>> +++ b/src/remote-viewer-iso-list-dialog.c
>> @@ -0,0 +1,115 @@
>> +/*
>> + * Virt Viewer: A virtual machine console viewer
>> + *
>> + * Copyright (C) 2016 Red Hat, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> + */
>> +
>> +#include 
>> +
>> +#include 
>> +
>> +#include "remote-viewer-iso-list-dialog.h"
>> +#include "virt-viewer-util.h"
>> +
>> +G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, 
>> GTK_TYPE_DIALOG)
>> +
>> +#define DIALOG_PRIVATE(o) \
>> +(G_TYPE_INSTANCE_GET_PRIVATE((o), 
>> REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG, RemoteViewerISOListDialogPrivate))
>> +
>> +struct _RemoteViewerISOListDialogPrivate
>> +{
>> +GtkBuilder *builder;
>> +GtkWidget *notebook;
>> +};
>> +
>> +enum RemoteViewerIsoListDialogPages
>> +{
>> +STATUS_PAGE = 0,
>> +ISO_LIST_PAGE,
>> +};
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_dispose(GObject *object)
>> +{
>> +RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object);
>> +RemoteViewerISOListDialogPrivate *priv = self->priv;
>> +
>> +g_clear_object(&priv->builder);
>> +
>> +
>> G_OBJECT_CLASS(remote_viewer_iso_list_dialog_parent_class)->dispose(object);
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_class_init(RemoteViewerISOListDialogClass 
>> *klass)
>> +{
>> +GObjectClass *object_class = G_OBJECT_CLASS(klass);
>> +
>> +g_type_class_add_private(klass, 
>> sizeof(RemoteViewerISOListDialogPrivate));
>> +
>> +object_class->dispose = remote_viewer_iso_list_dialog_dispose;
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_response(GtkDialog *dialog,
>> +   gint response_id,
>> +   gpointer user_data G_GNUC_UNUSED)
>> +{
>> +RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
>> +RemoteViewerISOListDialogPrivate *priv = self->priv;
>> +
>> +if (response_id != GTK_RESPONSE_NONE)
>> +return;
>> +
>> +gtk_notebook_set_current_page(GTK_NOTEBOOK(priv->notebook), 
>> STATUS_PAGE);
>> +gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, 
>> FALSE);
>> +gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_CLOSE, 
>> FALSE);
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self)
>> +{
>> +GtkWidget *content = gtk_dialog_get_content_area(GTK_DIALOG(self));
>> +RemoteViewerISOListDialogPrivate *priv = self->priv = 
>> DIALOG_PRIVATE(self);
>> +
>> +gtk_widget_set_size_request(GTK_WIDGET(self), 400, 300);
>> +gtk_box_set_spacing(GTK_BOX(content), 6);
>> +
>> +priv->bu

Re: [virt-tools-list] [PATCH virt-viewer 06/11] ovirt-foreign-menu: Notify of new files even if list did not change

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 12:43 PM, Christophe Fergeau wrote:
> Why?
> 

Because with the dialog, every time the Refresh button is pressed, we
clear the ISO list and it will only be populated again when it receives
the notify::files signal.

-- 
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 05/11] ovirt-foreign-menu: Remove GtkMenu related functions

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 12:43 PM, Christophe Fergeau wrote:
> On Sun, Jul 17, 2016 at 11:13:05PM -0300, Eduardo Lima (Etrunko) wrote:
>> With this commit, we finish cleaning up ovirt foreign menu code, which
>> only deals with data, leaving the UI bits to be handled properly in the
>> new ISO list dialog.
> 
> I'm not clear exactly what this is doing from the commit log. This
> removes the old toplevel "change cd" menu, replaces it with a
> File/Change CD menu entry, which is going to be not doing anything for
> now?
> 

Yes, that's it. It is strange, but I avoided removing old code and
replacing with lots of new code only to keep the functionality working.
Same happens with the dialog code, which is introduced with no
functionality at all, and only plugged into oVirt foreign menu later on.

>> -ovirt_foreign_menu_start(foreign_menu);
>> -}
>>  
>> +ovirt_foreign_menu_updated(self);
> 
> Why do we use _updated() instead of _start()?
> 

_updated() will only make the menu option visible, while I moved the
call to _start() when dialog is shown. This whole naming seems wrong
btw, because those functions are static to remote-viewer.c and not
present in ovirt-foreign-menu.c.

-- 
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] [c...@centos.org: [Libvirt-ci] Build failed in Jenkins: virt-manager-test » libvirt-fedora-23 #1440]

2016-07-19 Thread Cole Robinson 
On 07/19/2016 12:10 PM, Daniel P. Berrange wrote:
> It appears that quite a few tests in virt-manager are currently
> failing. I'm unclear if this is due to genuine libvirt regressions,
> or the tests were relying on undefined behaviour which has now
> changed in libvirt
> 

Hmm, I'm not sure either, I can't reproduce with current libvirt.git...

- 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 03/11] ovirt-foreign-menu: Remove timer used to refresh iso list

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 12:11 PM, Christophe Fergeau wrote:
> On Mon, Jul 18, 2016 at 10:18:14AM -0300, Eduardo Lima (Etrunko) wrote:
>> On 07/18/2016 09:14 AM, Pavel Grunt wrote:
>>> On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
 With the new ISO dialog, the user triggers the refresh manually.
>>>
>>> This should come after the dialog is introduced> 
>>
>> Well, I have no plans in merging any of these patches independently,
>> they are only split so that it makes more sense to understand the whole
>> series.
> 
> I agree with Pavel, the series makes more sense if it's removed _after_
> the dialog is there. But if it's a conflict mess to move it later, let's
> keep it here.
> 

I will try reordering the commits this way. My initial idea was to merge
the series as a whole with --no-ff.

-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com



signature.asc
Description: OpenPGP digital signature
___
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 07/11] UI: Make 'Change CD' menu item a submenu under 'File' toplevel menu

2016-07-19 Thread Eduardo Lima (Etrunko) 
On 07/19/2016 12:46 PM, Christophe Fergeau wrote:
> Your glade seems to have added unrelated changes to that commit. If
> newer versions of glade are always going to make these changes, we can
> commit them separately.
> 

Dunno why all those changes got in, most likely because of different
versions of glade. I will remove the unrelated changes and keep only
what the commit is meant to do.

-- 
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 08/11] Introduce ISO List dialog

2016-07-19 Thread Christophe Fergeau 
On Sun, Jul 17, 2016 at 11:13:08PM -0300, Eduardo Lima (Etrunko) wrote:
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/Makefile.am|   3 +
>  src/remote-viewer-iso-list-dialog.c| 115 +++
>  src/remote-viewer-iso-list-dialog.h|  58 ++
>  src/resources/ui/remote-viewer-iso-list.ui | 174 
> +
>  src/resources/virt-viewer.gresource.xml|   1 +
>  5 files changed, 351 insertions(+)
>  create mode 100644 src/remote-viewer-iso-list-dialog.c
>  create mode 100644 src/remote-viewer-iso-list-dialog.h
>  create mode 100644 src/resources/ui/remote-viewer-iso-list.ui
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 0c48e40..66a73f8 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -13,6 +13,7 @@ noinst_DATA = \
>   resources/ui/virt-viewer-vm-connection.ui \
>   resources/ui/virt-viewer-preferences.ui \
>   resources/ui/remote-viewer-connect.ui \
> + resources/ui/remote-viewer-iso-list.ui \
>   $(NULL)
>  
>  EXTRA_DIST = \
> @@ -96,6 +97,8 @@ if HAVE_OVIRT
>  libvirt_viewer_la_SOURCES += \
>   ovirt-foreign-menu.h \
>   ovirt-foreign-menu.c \
> + remote-viewer-iso-list-dialog.c \
> + remote-viewer-iso-list-dialog.h \
>   $(NULL)
>  endif
>  
> diff --git a/src/remote-viewer-iso-list-dialog.c 
> b/src/remote-viewer-iso-list-dialog.c
> new file mode 100644
> index 000..b3972ac
> --- /dev/null
> +++ b/src/remote-viewer-iso-list-dialog.c
> @@ -0,0 +1,115 @@
> +/*
> + * Virt Viewer: A virtual machine console viewer
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include 
> +
> +#include 
> +
> +#include "remote-viewer-iso-list-dialog.h"
> +#include "virt-viewer-util.h"
> +
> +G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, 
> GTK_TYPE_DIALOG)
> +
> +#define DIALOG_PRIVATE(o) \
> +(G_TYPE_INSTANCE_GET_PRIVATE((o), 
> REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG, RemoteViewerISOListDialogPrivate))
> +
> +struct _RemoteViewerISOListDialogPrivate
> +{
> +GtkBuilder *builder;
> +GtkWidget *notebook;
> +};
> +
> +enum RemoteViewerIsoListDialogPages
> +{
> +STATUS_PAGE = 0,
> +ISO_LIST_PAGE,
> +};
> +
> +static void
> +remote_viewer_iso_list_dialog_dispose(GObject *object)
> +{
> +RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object);
> +RemoteViewerISOListDialogPrivate *priv = self->priv;
> +
> +g_clear_object(&priv->builder);
> +
> +
> G_OBJECT_CLASS(remote_viewer_iso_list_dialog_parent_class)->dispose(object);
> +}
> +
> +static void
> +remote_viewer_iso_list_dialog_class_init(RemoteViewerISOListDialogClass 
> *klass)
> +{
> +GObjectClass *object_class = G_OBJECT_CLASS(klass);
> +
> +g_type_class_add_private(klass, 
> sizeof(RemoteViewerISOListDialogPrivate));
> +
> +object_class->dispose = remote_viewer_iso_list_dialog_dispose;
> +}
> +
> +static void
> +remote_viewer_iso_list_dialog_response(GtkDialog *dialog,
> +   gint response_id,
> +   gpointer user_data G_GNUC_UNUSED)
> +{
> +RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
> +RemoteViewerISOListDialogPrivate *priv = self->priv;
> +
> +if (response_id != GTK_RESPONSE_NONE)
> +return;
> +
> +gtk_notebook_set_current_page(GTK_NOTEBOOK(priv->notebook), STATUS_PAGE);
> +gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, 
> FALSE);
> +gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_CLOSE, 
> FALSE);
> +}
> +
> +static void
> +remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self)
> +{
> +GtkWidget *content = gtk_dialog_get_content_area(GTK_DIALOG(self));
> +RemoteViewerISOListDialogPrivate *priv = self->priv = 
> DIALOG_PRIVATE(self);
> +
> +gtk_widget_set_size_request(GTK_WIDGET(self), 400, 300);
> +gtk_box_set_spacing(GTK_BOX(content), 6);
> +
> +priv->builder = virt_viewer_util_load_ui("remote-viewer-iso-list.ui");
> +gtk_builder_connect_signals(priv->builder, self);
> +
> +priv->notebook = GTK_WIDGET(gtk_builder_get_object(priv->buil

[virt-tools-list] [c...@centos.org: [Libvirt-ci] Build failed in Jenkins: virt-manager-test » libvirt-fedora-23 #1440]

2016-07-19 Thread Daniel P. Berrange 
It appears that quite a few tests in virt-manager are currently
failing. I'm unclear if this is due to genuine libvirt regressions,
or the tests were relying on undefined behaviour which has now
changed in libvirt

- Forwarded message from c...@centos.org -

> Date: Tue, 19 Jul 2016 15:52:37 + (GMT+00:00)
> From: c...@centos.org
> To: shaj...@redhat.com, libvirt...@redhat.com
> Subject: [Libvirt-ci] Build failed in Jenkins: virt-manager-test » 
> libvirt-fedora-23 #1440
> 
> See 
> 
> 
> --
> Started by upstream project "virt-manager-test" build number 1440
> originally caused by:
>  Started by upstream project "virt-manager-build" build number 1370
>  originally caused by:
>   Started by upstream project "libvirt-python-build" build number 1123
>   originally caused by:
>Started by upstream project "libvirt-daemon-build" build number 1543
>originally caused by:
> Started by an SCM change
> Started by an SCM change
>  Started by upstream project "virt-viewer-build" build number 1148
>  originally caused by:
>   Started by upstream project "libvirt-daemon-build" build number 1543
> [EnvInject] - Loading node environment variables.
> Building remotely on libvirt-fedora-23 (libvirt) in workspace 
> 
> [libvirt-fedora-23] $ /bin/sh -xe /tmp/hudson515687075386881451.sh
> + ./setup.py test
> running test
> s.F.F...F.ssssssF.s...Fss..s..
> ==
> FAIL: testCLI0229virt_xml_edit_simple_metadata (tests.clitest.CLITests)
> --
> Traceback (most recent call last):
>   File 
> "
>  line 1013, in 
> return lambda s: cmdtemplate(s, cmd)
>   File 
> "
>  line 1012, in cmdtemplate
> _cmdobj.run(self)
>   File 
> "
>  line 270, in run
> tests.fail(err)
> AssertionError: ./virt-xml  --connect 
> __virtinst_test__test:///
>  test-for-virtxml --edit --print-diff --define --metadata 
> name=foo-my-new-name,uuid=12345678-12F4-1234-1234-123456789AFA,description="hey
>  this is my
> new
> very,very=new desc\'",title="This is my,funky=new title" 
> Conversion outputs did not match.
> --- tests/cli-test-xml/compare/virt-xml-edit-simple-metadata.xml
> +++ Generated Output
> @@ -8,7 +8,7 @@
>  +  hey this is my
>  +new
>  +very,very=new desc'
> -   409600
> +   4194304
> 204800
> 
>  @@
> 
> 
> ==
> FAIL: testCLI0231virt_xml_edit_simple_memory (tests.clitest.CLITests)
> --
> Traceback (most recent call last):
>   File 
> "
>  line 1013, in 
> return lambda s: cmdtemplate(s, cmd)
>   File 
> "
>  line 1012, in cmdtemplate
> _cmdobj.run(self)
>   File 
> "
>  line 270, in run
> tests.fail(err)
> AssertionError: ./virt-xml  --connect 
> __virtinst_test__test:///
>  test-for-virtxml --edit --print-diff --define --memory 
> 500,maxmemory=1000,hugepages=off
> Conversion outputs did not match.
> --- tests/cli-test-xml/compare/virt-xml-edit-simple-memory.xml
> +++ Generated Output
> @@ -1,7 +1,7 @@
> 12345678-12f4-1234-1234-123456789012
> Test VM for virtxml cli tests
> 
> --  409600
> +-  4194304
>  -  204800
>  +  1024000
>  +  512000
> 
> 
> ==
> FAIL: testCLI0235virt_xml_edit_simple_blkiotune (tests.clitest.CLITests)
> --
> Traceback (most recent call last):
>   File 
> "

Re: [virt-tools-list] [PATCH virt-viewer 10/11] remote-viewer: Make ovirt-foreign-menu a property

2016-07-19 Thread Christophe Fergeau 
On Sun, Jul 17, 2016 at 11:13:10PM -0300, Eduardo Lima (Etrunko) wrote:
> The OvirtForeignMenu pointer is needed by the new ISO list dialog, and
> we make it acessible via property to avoid interdependency between
> objects.
> 
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/remote-viewer-iso-list-dialog.c | 20 +++-
>  src/remote-viewer-iso-list-dialog.h |  3 ++-
>  src/remote-viewer.c | 36 
>  src/virt-viewer-window.c| 16 +++-
>  4 files changed, 68 insertions(+), 7 deletions(-)
> 
> diff --git a/src/remote-viewer-iso-list-dialog.c 
> b/src/remote-viewer-iso-list-dialog.c
> index b3972ac..996c1f2 100644
> --- a/src/remote-viewer-iso-list-dialog.c
> +++ b/src/remote-viewer-iso-list-dialog.c
> @@ -34,6 +34,7 @@ struct _RemoteViewerISOListDialogPrivate
>  {
>  GtkBuilder *builder;
>  GtkWidget *notebook;
> +OvirtForeignMenu *foreign_menu;
>  };
>  
>  enum RemoteViewerIsoListDialogPages
> @@ -106,10 +107,19 @@ 
> remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self)
>  }
>  
>  GtkWidget *
> -remote_viewer_iso_list_dialog_new(GtkWindow *parent)
> +remote_viewer_iso_list_dialog_new(GtkWindow *parent, OvirtForeignMenu 
> *foreign_menu)
>  {
> -return g_object_new(REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG,
> -"title", _("Change CD"),
> -"transient-for", parent,
> -NULL);
> +GtkWidget *dialog;
> +RemoteViewerISOListDialog *self;
> +
> +g_return_val_if_fail(foreign_menu != NULL, NULL);
> +
> +dialog = g_object_new(REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG,
> + "title", _("Change CD"),
> + "transient-for", parent,
> + NULL);
> +
> +self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
> +self->priv->foreign_menu = foreign_menu;

I'd g_object_ref it if you need to have it around (together with g_clear_object
in dispose/finalize).

> +return dialog;
>  }
> diff --git a/src/remote-viewer-iso-list-dialog.h 
> b/src/remote-viewer-iso-list-dialog.h
> index def841b..ee7c70f 100644
> --- a/src/remote-viewer-iso-list-dialog.h
> +++ b/src/remote-viewer-iso-list-dialog.h
> @@ -22,6 +22,7 @@
>  #define __REMOTE_VIEWER_ISO_LIST_DIALOG_H__
>  
>  #include 
> +#include "ovirt-foreign-menu.h"
>  
>  G_BEGIN_DECLS
>  
> @@ -51,7 +52,7 @@ struct _RemoteViewerISOListDialogClass
>  
>  GType remote_viewer_iso_list_dialog_get_type(void) G_GNUC_CONST;
>  
> -GtkWidget *remote_viewer_iso_list_dialog_new(GtkWindow *parent);
> +GtkWidget *remote_viewer_iso_list_dialog_new(GtkWindow *parent, 
> OvirtForeignMenu *foreign_menu);
>  
>  G_END_DECLS
>  
> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index 95130dc..e4e41c4 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -67,6 +67,13 @@ G_DEFINE_TYPE (RemoteViewer, remote_viewer, 
> VIRT_VIEWER_TYPE_APP)
>  #define GET_PRIVATE(o)   
>  \
>  (G_TYPE_INSTANCE_GET_PRIVATE ((o), REMOTE_VIEWER_TYPE, 
> RemoteViewerPrivate))
>  
> +enum RemoteViewerProperties {
> +PROP_0,
> +#ifdef HAVE_OVIRT
> +PROP_OVIRT_FOREIGN_MENU,
> +#endif
> +};
> +
>  #ifdef HAVE_OVIRT
>  static OvirtVm * choose_vm(GtkWindow *main_window,
> char **vm_name,
> @@ -213,6 +220,25 @@ end:
>  }
>  
>  static void
> +remote_viewer_get_property(GObject *object, guint property_id,
> +   GValue *value, GParamSpec *pspec)
> +{
> +RemoteViewer *self = REMOTE_VIEWER(object);
> +RemoteViewerPrivate *priv = self->priv;
> +
> +switch (property_id) {
> +#ifdef HAVE_OVIRT
> +case PROP_OVIRT_FOREIGN_MENU:
> +g_value_set_pointer(value, priv->ovirt_foreign_menu);
> +break;
> +#endif
> +
> +default:
> +G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
> +}
> +}
> +
> +static void
>  remote_viewer_class_init (RemoteViewerClass *klass)
>  {
>  GObjectClass *object_class = G_OBJECT_CLASS (klass);
> @@ -222,6 +248,7 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>  
>  g_type_class_add_private (klass, sizeof (RemoteViewerPrivate));
>  
> +object_class->get_property = remote_viewer_get_property;
>  object_class->dispose = remote_viewer_dispose;
>  
>  g_app_class->local_command_line = remote_viewer_local_command_line;
> @@ -235,6 +262,15 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>  #else
>  (void) gtk_app_class;
>  #endif
> +
> +#ifdef HAVE_OVIRT
> +g_object_class_install_property(object_class,
> +PROP_OVIRT_FOREIGN_MENU,
> +
> g_param_spec_pointer("ovirt-foreign-menu",

This can be a g_param_spec_object, OvirtForeignMenu is a GObject.

> + "oVirt Foreign 
> Menu",

Re: [virt-tools-list] [PATCH virt-viewer 07/11] UI: Make 'Change CD' menu item a submenu under 'File' toplevel menu

2016-07-19 Thread Christophe Fergeau 
Your glade seems to have added unrelated changes to that commit. If
newer versions of glade are always going to make these changes, we can
commit them separately.

Christophe

On Sun, Jul 17, 2016 at 11:13:07PM -0300, Eduardo Lima (Etrunko) wrote:
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/resources/ui/virt-viewer.ui | 57 
> +++--
>  1 file changed, 21 insertions(+), 36 deletions(-)
> 
> diff --git a/src/resources/ui/virt-viewer.ui b/src/resources/ui/virt-viewer.ui
> index 6e3c5ad..ee23ba5 100644
> --- a/src/resources/ui/virt-viewer.ui
> +++ b/src/resources/ui/virt-viewer.ui
> @@ -1,6 +1,7 @@
>  
> +
>  
> -  
> +  
>
>
>  False
> @@ -23,7 +24,6 @@
>
>  True
>  False
> -False
>   translatable="yes">_File
>  True
>  
> @@ -35,7 +35,6 @@
> id="menu-file-screenshot">
>  True
>  False
> - name="use_action_appearance">False
>   translatable="yes">_Screenshot
>  True
>   handler="virt_viewer_window_menu_file_screenshot" swapped="no"/>
> @@ -46,7 +45,6 @@
>  True
>  False
>  False
> - name="use_action_appearance">False
>  _USB 
> device selection
>  True
>   handler="virt_viewer_window_menu_file_usb_device_selection" swapped="no"/>
> @@ -55,7 +53,6 @@
>  
> id="menu-file-smartcard-insert">
>  False
> - name="use_action_appearance">False
>   name="accel_path">/file/smartcard-insert
>   translatable="yes">Smartcard insertion
>  True
> @@ -65,7 +62,6 @@
>  
> id="menu-file-smartcard-remove">
>  False
> - name="use_action_appearance">False
>   name="accel_path">/file/smartcard-remove
>   translatable="yes">Smartcard removal
>  True
> @@ -73,6 +69,13 @@
>
>  
>  
> +  
> +False
> + translatable="yes">_Change CD
> +True
> +  
> +
> +
>
>  True
>  False
> @@ -89,13 +92,12 @@
>  
>  
>
> - translatable="yes">_Quit
>  True
>  False
> - name="use_action_appearance">False
> + translatable="yes">_Quit
>  True
> - modifiers="GDK_SHIFT_MASK | GDK_CONTROL_MASK"/>
>   handler="virt_viewer_window_menu_file_quit" swapped="no"/>
> + modifiers="GDK_SHIFT_MASK | GDK_CONTROL_MASK"/>
>
>  
>
> @@ -106,7 +108,6 @@
>
>  True
>  False
> -False
>   translatable="yes">_View
>  True
>  
> @@ -118,7 +119,6 @@
> id="menu-view-fullscreen">
>  True
>  False
> - name="use_action_appearance">False
>   name="accel_path">/view/toggle-fullscreen
>  _Full 
> screen
>  True
> @@ -129,7 +129,6 @@
>
>  True
>  False
> - name="use_action_appearance">False
>   translatable="yes">_Zoom
>  True
>  
> @@ -139,22 +138,22 @@
>   name="accel_group">accelgroup
>  
> id="menu-view-zoom-in">
> - name="accel_path">/view/zoom-in
> - transla

Re: [virt-tools-list] [PATCH virt-viewer 05/11] ovirt-foreign-menu: Remove GtkMenu related functions

2016-07-19 Thread Christophe Fergeau 
On Sun, Jul 17, 2016 at 11:13:05PM -0300, Eduardo Lima (Etrunko) wrote:
> With this commit, we finish cleaning up ovirt foreign menu code, which
> only deals with data, leaving the UI bits to be handled properly in the
> new ISO list dialog.

I'm not clear exactly what this is doing from the commit log. This
removes the old toplevel "change cd" menu, replaces it with a
File/Change CD menu entry, which is going to be not doing anything for
now?

> 
> This patch also updates remote-viewer to reflect the change.
> 

> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index 6d29bf2..95130dc 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -734,33 +734,11 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED 
> RestProxyAuth *auth,
>  static void
>  ovirt_foreign_menu_update(GtkApplication *gtkapp, GtkWindow *gtkwin, 
> G_GNUC_UNUSED gpointer data)
>  {
> -RemoteViewer *app = REMOTE_VIEWER(gtkapp);
> +RemoteViewer *self = REMOTE_VIEWER(gtkapp);
>  VirtViewerWindow *win = g_object_get_data(G_OBJECT(gtkwin), 
> "virt-viewer-window");
> -GtkWidget *menu = g_object_get_data(G_OBJECT(win), "foreign-menu");
> -GtkWidget *submenu;
> -
> -if (app->priv->ovirt_foreign_menu == NULL) {
> -/* nothing to do */
> -return;
> -}
> -
> -submenu = ovirt_foreign_menu_get_gtk_menu(app->priv->ovirt_foreign_menu);
> -if (submenu == NULL) {
> -/* No items to show, no point in showing the menu */
> -if (menu != NULL)
> -   gtk_widget_set_visible(menu, FALSE);
> -g_object_set_data(G_OBJECT(win), "foreign-menu", NULL);
> -return;
> -}
> -
> -if (menu == NULL) {
> -menu = 
> GTK_WIDGET(gtk_builder_get_object(virt_viewer_window_get_builder(win), 
> "menu-change-cd"));
> -g_object_set_data(G_OBJECT(win), "foreign-menu", menu);
> -gtk_widget_set_visible(menu, TRUE);
> -}
> -
> -gtk_menu_item_set_submenu(GTK_MENU_ITEM(menu), submenu);
> -gtk_widget_show_all(menu);
> +GtkBuilder *builder = virt_viewer_window_get_builder(win);
> +GtkWidget *menu = GTK_WIDGET(gtk_builder_get_object(builder, 
> "menu-change-cd"));
> +gtk_widget_set_visible(menu, self->priv->ovirt_foreign_menu != NULL);
>  }
>  
>  static void
> @@ -783,36 +761,22 @@ ovirt_foreign_menu_updated(RemoteViewer *self)
>  }
>  
>  static void
> -ovirt_foreign_menu_changed(OvirtForeignMenu *foreign_menu G_GNUC_UNUSED,
> -   GParamSpec *pspec G_GNUC_UNUSED,
> -   VirtViewerApp *app)
> -{
> -ovirt_foreign_menu_updated(REMOTE_VIEWER(app));
> -}
> -
> -
> -static void
>  virt_viewer_app_set_ovirt_foreign_menu(VirtViewerApp *app,
> OvirtForeignMenu *foreign_menu)
>  {
>  RemoteViewer *self;
> +
>  g_return_if_fail(REMOTE_VIEWER_IS(app));
>  g_return_if_fail(OVIRT_IS_FOREIGN_MENU(foreign_menu));
>  
>  self = REMOTE_VIEWER(app);
> -if (self->priv->ovirt_foreign_menu != NULL) {
> -g_object_unref(G_OBJECT(self->priv->ovirt_foreign_menu));
> -}
> +g_clear_object(&self->priv->ovirt_foreign_menu);
>  self->priv->ovirt_foreign_menu = foreign_menu;
> -g_signal_connect(G_OBJECT(foreign_menu), "notify::file",
> - (GCallback)ovirt_foreign_menu_changed, app);
> -g_signal_connect(G_OBJECT(foreign_menu), "notify::files",
> - (GCallback)ovirt_foreign_menu_changed, app);
>  g_signal_connect(G_OBJECT(app), "window-added",
>   (GCallback)ovirt_foreign_menu_update, NULL);
> -ovirt_foreign_menu_start(foreign_menu);
> -}
>  
> +ovirt_foreign_menu_updated(self);

Why do we use _updated() instead of _start()?

Christophe


signature.asc
Description: PGP signature
___
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 06/11] ovirt-foreign-menu: Notify of new files even if list did not change

2016-07-19 Thread Christophe Fergeau 
Why?

On Sun, Jul 17, 2016 at 11:13:06PM -0300, Eduardo Lima (Etrunko) wrote:
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/ovirt-foreign-menu.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index 24b5af2..75154bb 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -428,12 +428,15 @@ static void 
> ovirt_foreign_menu_set_files(OvirtForeignMenu *menu,
>  
>  if ((it == NULL) && (it2 == NULL)) {
>  /* sorted_files and menu->priv->files content was the same */
> +g_debug("Iso list unchanged");
>  g_list_free_full(sorted_files, (GDestroyNotify)g_free);
> -return;
> +goto end;
>  }
>  
>  g_list_free_full(menu->priv->iso_names, (GDestroyNotify)g_free);
>  menu->priv->iso_names = sorted_files;
> +
> +end:
>  g_object_notify(G_OBJECT(menu), "files");
>  }
>  
> -- 
> 2.7.4
> 
> ___
> virt-tools-list mailing list
> virt-tools-list@redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list


signature.asc
Description: PGP signature
___
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 04/11] ovirt-foreign-menu: Add accessors for current iso and iso list

2016-07-19 Thread Christophe Fergeau 
On Sun, Jul 17, 2016 at 11:13:04PM -0300, Eduardo Lima (Etrunko) wrote:
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/ovirt-foreign-menu.c | 49 
> ++--
>  src/ovirt-foreign-menu.h |  5 -
>  2 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index b071e27..2446239 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -47,6 +47,7 @@ static void 
> ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu
>  static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu *menu);
>  static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu 
> *menu);
>  static void ovirt_foreign_menu_fetch_iso_list_async(OvirtForeignMenu *menu);
> +static void updated_cdrom_cb(GObject *source_object, GAsyncResult *result, 
> gpointer user_data);
>  
>  G_DEFINE_TYPE (OvirtForeignMenu, ovirt_foreign_menu, G_TYPE_OBJECT)
>  
> @@ -85,7 +86,7 @@ enum {
>  };
>  
>  
> -static char *
> +char *
>  ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu *foreign_menu)
>  {
>  char *name;
> @@ -100,6 +101,36 @@ ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu 
> *foreign_menu)
>  }
>  
>  
> +void
> +ovirt_foreign_menu_set_current_iso_name(OvirtForeignMenu *foreign_menu, char 
> *name)
> +{

For what it's worth, this is a bit misleading as this going to trigger
an async update of the ISO name, and this sets "next_iso_name" more than
"current_iso_name". I think you need to expose this an async method
anyway, so that you can catch failures to change the ISO (ie the REST
API call failed).

Christophe

> +g_return_if_fail(foreign_menu->priv->cdrom != NULL);
> +g_return_if_fail(foreign_menu->priv->next_iso_name == NULL);
> +
> +if (name) {
> +g_debug("Updating VM cdrom image to '%s'", name);
> +foreign_menu->priv->next_iso_name = g_strdup(name);
> +} else {
> +g_debug("Removing current cdrom image");
> +foreign_menu->priv->next_iso_name = NULL;
> +}
> +
> +g_object_set(foreign_menu->priv->cdrom,
> + "file", name,
> + NULL);
> +ovirt_cdrom_update_async(foreign_menu->priv->cdrom, TRUE,
> + foreign_menu->priv->proxy, NULL,
> + updated_cdrom_cb, foreign_menu);
> +}
> +
> +
> +GList*
> +ovirt_foreign_menu_get_iso_names(OvirtForeignMenu *foreign_menu)
> +{
> +return foreign_menu->priv->iso_names;
> +}
> +
> +
>  static void
>  ovirt_foreign_menu_get_property(GObject *object, guint property_id,
> GValue *value, GParamSpec *pspec)
> @@ -385,7 +416,7 @@ static void
>  ovirt_foreign_menu_activate_item_cb(GtkMenuItem *menuitem, gpointer 
> user_data)
>  {
>  OvirtForeignMenu *foreign_menu;
> -const char *iso_name;
> +const char *iso_name = NULL;
>  gboolean checked;
>  
>  checked = gtk_check_menu_item_get_active(GTK_CHECK_MENU_ITEM(menuitem));
> @@ -405,19 +436,9 @@ ovirt_foreign_menu_activate_item_cb(GtkMenuItem 
> *menuitem, gpointer user_data)
>  
>  if (checked) {
>  iso_name = gtk_menu_item_get_label(menuitem);
> -g_debug("Updating VM cdrom image to '%s'", iso_name);
> -foreign_menu->priv->next_iso_name = g_strdup(iso_name);
> -} else {
> -g_debug("Removing current cdrom image");
> -iso_name = NULL;
> -foreign_menu->priv->next_iso_name = NULL;
>  }
> -g_object_set(foreign_menu->priv->cdrom,
> - "file", iso_name,
> - NULL);
> -ovirt_cdrom_update_async(foreign_menu->priv->cdrom, TRUE,
> - foreign_menu->priv->proxy, NULL,
> - updated_cdrom_cb, foreign_menu);
> +
> +ovirt_foreign_menu_set_current_iso_name(foreign_menu, iso_name);
>  }
>  
>  
> diff --git a/src/ovirt-foreign-menu.h b/src/ovirt-foreign-menu.h
> index cf18b52..f1a1ddb 100644
> --- a/src/ovirt-foreign-menu.h
> +++ b/src/ovirt-foreign-menu.h
> @@ -70,7 +70,10 @@ OvirtForeignMenu* ovirt_foreign_menu_new(OvirtProxy 
> *proxy);
>  OvirtForeignMenu *ovirt_foreign_menu_new_from_file(VirtViewerFile *self);
>  void ovirt_foreign_menu_start(OvirtForeignMenu *menu);
>  
> -GtkWidget *ovirt_foreign_menu_get_gtk_menu(OvirtForeignMenu *foreign_menu);
> +char *ovirt_foreign_menu_get_current_iso_name(OvirtForeignMenu *menu);
> +void ovirt_foreign_menu_set_current_iso_name(OvirtForeignMenu *menu, char 
> *name);
> +
> +GList *ovirt_foreign_menu_get_iso_names(OvirtForeignMenu *menu);
>  
>  G_END_DECLS
>  
> -- 
> 2.7.4
> 
> ___
> virt-tools-list mailing list
> virt-tools-list@redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list


signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www

Re: [virt-tools-list] [PATCH virt-viewer 03/11] ovirt-foreign-menu: Remove timer used to refresh iso list

2016-07-19 Thread Christophe Fergeau 
On Mon, Jul 18, 2016 at 10:18:14AM -0300, Eduardo Lima (Etrunko) wrote:
> On 07/18/2016 09:14 AM, Pavel Grunt wrote:
> > On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
> >> With the new ISO dialog, the user triggers the refresh manually.
> > 
> > This should come after the dialog is introduced> 
> 
> Well, I have no plans in merging any of these patches independently,
> they are only split so that it makes more sense to understand the whole
> series.

I agree with Pavel, the series makes more sense if it's removed _after_
the dialog is there. But if it's a conflict mess to move it later, let's
keep it here.

Christophe


signature.asc
Description: PGP signature
___
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 02/11] ovirt-foreign-menu: Use g_clear_pointer/g_clear_object

2016-07-19 Thread Christophe Fergeau 

Acked-by: Christophe Fergeau 

On Sun, Jul 17, 2016 at 11:13:02PM -0300, Eduardo Lima (Etrunko) wrote:
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/ovirt-foreign-menu.c | 68 
> 
>  1 file changed, 16 insertions(+), 52 deletions(-)
> 
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index b0b8fec..889e7bc 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -143,33 +143,23 @@ ovirt_foreign_menu_set_property(GObject *object, guint 
> property_id,
>  
>  switch (property_id) {
>  case PROP_PROXY:
> -if (priv->proxy != NULL) {
> -g_object_unref(priv->proxy);
> -}
> +g_clear_object(&priv->proxy);
>  priv->proxy = g_value_dup_object(value);
>  break;
>  case PROP_API:
> -if (priv->api != NULL) {
> -g_object_unref(priv->api);
> -}
> +g_clear_object(&priv->api);
>  priv->api = g_value_dup_object(value);
>  break;
>  case PROP_VM:
> -if (priv->vm != NULL) {
> -g_object_unref(priv->vm);
> -}
> +g_clear_object(&priv->vm);
>  priv->vm = g_value_dup_object(value);
> -g_free(priv->vm_guid);
> -priv->vm_guid = NULL;
> +g_clear_pointer(&priv->vm_guid, g_free);
>  if (priv->vm != NULL) {
>  g_object_get(G_OBJECT(priv->vm), "guid", &priv->vm_guid, NULL);
>  }
>  break;
>  case PROP_VM_GUID:
> -if (priv->vm != NULL) {
> -g_object_unref(priv->vm);
> -priv->vm = NULL;
> -}
> +g_clear_object(&priv->vm);
>  g_free(priv->vm_guid);
>  priv->vm_guid = g_value_dup_string(value);
>  break;
> @@ -184,44 +174,20 @@ ovirt_foreign_menu_dispose(GObject *obj)
>  {
>  OvirtForeignMenu *self = OVIRT_FOREIGN_MENU(obj);
>  
> -if (self->priv->proxy) {
> -g_object_unref(self->priv->proxy);
> -self->priv->proxy = NULL;
> -}
> -
> -if (self->priv->api != NULL) {
> -g_object_unref(self->priv->api);
> -self->priv->api = NULL;
> -}
> -
> -if (self->priv->vm) {
> -g_object_unref(self->priv->vm);
> -self->priv->vm = NULL;
> -}
> -
> -g_free(self->priv->vm_guid);
> -self->priv->vm_guid = NULL;
> -
> -if (self->priv->files) {
> -g_object_unref(self->priv->files);
> -self->priv->files = NULL;
> -}
> -
> -if (self->priv->cdrom) {
> -g_object_unref(self->priv->cdrom);
> -self->priv->cdrom = NULL;
> -}
> +g_clear_object(&self->priv->proxy);
> +g_clear_object(&self->priv->api);
> +g_clear_object(&self->priv->vm);
> +g_clear_pointer(&self->priv->vm_guid, g_free);
> +g_clear_object(&self->priv->files);
> +g_clear_object(&self->priv->cdrom);
>  
>  if (self->priv->iso_names) {
>  g_list_free_full(self->priv->iso_names, (GDestroyNotify)g_free);
>  self->priv->iso_names = NULL;
>  }
>  
> -g_free(self->priv->current_iso_name);
> -self->priv->current_iso_name = NULL;
> -
> -g_free(self->priv->next_iso_name);
> -self->priv->next_iso_name = NULL;
> +g_clear_pointer(&self->priv->current_iso_name, g_free);
> +g_clear_pointer(&self->priv->next_iso_name, g_free);
>  
>  G_OBJECT_CLASS(ovirt_foreign_menu_parent_class)->dispose(obj);
>  }
> @@ -410,8 +376,8 @@ static void updated_cdrom_cb(GObject *source_object,
>  current_file?current_file:NULL);
>  g_object_set(foreign_menu->priv->cdrom, "file", current_file, NULL);
>  }
> -g_free(foreign_menu->priv->next_iso_name);
> -foreign_menu->priv->next_iso_name = NULL;
> +
> +g_clear_pointer(&foreign_menu->priv->next_iso_name, g_free);
>  }
>  
>  
> @@ -546,13 +512,11 @@ static void cdrom_file_refreshed_cb(GObject 
> *source_object,
>  }
>  
>  /* Content of OvirtCdrom is now current */
> -g_free(menu->priv->current_iso_name);
> +g_clear_pointer(&menu->priv->current_iso_name, g_free);
>  if (menu->priv->cdrom != NULL) {
>  g_object_get(G_OBJECT(menu->priv->cdrom),
>   "file", &menu->priv->current_iso_name,
>   NULL);
> -} else {
> -menu->priv->current_iso_name = NULL;
>  }
>  g_object_notify(G_OBJECT(menu), "file");
>  if (menu->priv->cdrom != NULL) {
> -- 
> 2.7.4
> 
> ___
> virt-tools-list mailing list
> virt-tools-list@redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list


signature.asc
Description: PGP signature
___
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 01/11] ovirt-foreign-menu: Rework states logic

2016-07-19 Thread Christophe Fergeau 
On Mon, Jul 18, 2016 at 10:25:25AM -0300, Eduardo Lima (Etrunko) wrote:
> Use switch/case instead of lots of conditional blocks
> 
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/ovirt-foreign-menu.c | 41 +++--
>  1 file changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> index 33ff4f1..80e40a1 100644
> --- a/src/ovirt-foreign-menu.c
> +++ b/src/ovirt-foreign-menu.c
> @@ -312,51 +312,40 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu 
> *menu,
>  g_return_if_fail(current_state >= STATE_0);
>  g_return_if_fail(current_state < STATE_ISOS);
>  
> -current_state++;
> -
> -if (current_state == STATE_API) {
> +switch (++current_state) {
> +case STATE_API:
>  if (menu->priv->api == NULL) {
>  ovirt_foreign_menu_fetch_api_async(menu);
> -} else {
> -current_state++;
> +break;
>  }
> -}
> -
> -if (current_state == STATE_VM) {
> +case STATE_VM:
>  if (menu->priv->vm == NULL) {
>  ovirt_foreign_menu_fetch_vm_async(menu);
> -} else {
> -current_state++;
> +break;
>  }

I'd add some comment every time we fall through to the next state,
otherwise it can be confusing as there is a break; statement, but in a
conditional.

Christophe


signature.asc
Description: PGP signature
___
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 01/11] ovirt-foreign-menu: Rework states logic

2016-07-19 Thread Christophe Fergeau 
On Mon, Jul 18, 2016 at 11:15:03AM +0200, Pavel Grunt wrote:
> Hi Eduardo,
> 
> On Sun, 2016-07-17 at 23:13 -0300, Eduardo Lima (Etrunko) wrote:
> > Use switch/case instead of lots of conditional blocks
> Yes, it is more readable
> > 
> > Signed-off-by: Eduardo Lima (Etrunko) 
> > ---
> >  src/ovirt-foreign-menu.c | 76 +++--
> > ---
> >  1 file changed, 36 insertions(+), 40 deletions(-)
> > 
> > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> > index 33ff4f1..b0b8fec 100644
> > --- a/src/ovirt-foreign-menu.c
> > +++ b/src/ovirt-foreign-menu.c
> > @@ -312,51 +312,47 @@ ovirt_foreign_menu_next_async_step(OvirtForeignMenu
> > *menu,
> >  g_return_if_fail(current_state >= STATE_0);
> >  g_return_if_fail(current_state < STATE_ISOS);
> >  
> > -current_state++;
> my preference is to keep the increment outside the switch statement
> 
> > -
> > -if (current_state == STATE_API) {
> > -if (menu->priv->api == NULL) {
> > -ovirt_foreign_menu_fetch_api_async(menu);
> > -} else {
> > -current_state++;
> > +switch (++current_state) {
> Actually the increment is not needed at all thanks to your changes, imo
> switch(current_state + 1) would be more readable

I'd go for that too as this commit removes all the other current_state++
occurrences.

Christophe


signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list