Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-07 Thread Eduardo Lima (Etrunko) 
On 12/07/2015 10:43 AM, Eduardo Lima (Etrunko) wrote:
> On 12/02/2015 11:25 PM, Jim Fehlig wrote:
>> On 12/02/2015 04:15 AM, Daniel P. Berrange wrote:
>>> Yep, we need to agree which min platform we're targetting. It is nice
>>> to see RHEL7 rebasing GTK, as that makes life easier. We should not
>>> only consider RHEL though. It would be desirable if someone can provide
>>> a list of GTK versions in current SLES,
>>
>> SLES11: glib 2.22, gtk 2.18.9
>> SLES12: glib 2.38, gtk 3.10.9
>>
> 
> Hi Jim,
> 
> I am trying to build with those deps, but it seems to require earlier
> version of atk too, can you tell what is the version in SLES12?
> 

For ATK, I followed the requirements in gtk configure.ac file, which is
2.7.5. and managed to build everything locally.

I tried copying the code from Glib to the virt-viewer compat, but it
proved not to be possible. So we will need to wait for SLES to upgrade
to 2.40.

Even so, I have a small cleanup patch that can be applied and
modifications to this v1. I will post them to the ML and come back to it
when the time is appropriate.

Regards, Eduardo.

-- 
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 3/4] Port to GtkApplication API's

2015-12-07 Thread Jim Fehlig 
Eduardo Lima (Etrunko) wrote:
> On 12/02/2015 11:25 PM, Jim Fehlig wrote:
>> On 12/02/2015 04:15 AM, Daniel P. Berrange wrote:
>>> Yep, we need to agree which min platform we're targetting. It is nice
>>> to see RHEL7 rebasing GTK, as that makes life easier. We should not
>>> only consider RHEL though. It would be desirable if someone can provide
>>> a list of GTK versions in current SLES,
>> SLES11: glib 2.22, gtk 2.18.9
>> SLES12: glib 2.38, gtk 3.10.9
>>
> 
> Hi Jim,
> 
> I am trying to build with those deps, but it seems to require earlier
> version of atk too, can you tell what is the version in SLES12?

2.10.0

Regards,
Jim

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

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-07 Thread Eduardo Lima (Etrunko) 
On 12/02/2015 11:25 PM, Jim Fehlig wrote:
> On 12/02/2015 04:15 AM, Daniel P. Berrange wrote:
>> Yep, we need to agree which min platform we're targetting. It is nice
>> to see RHEL7 rebasing GTK, as that makes life easier. We should not
>> only consider RHEL though. It would be desirable if someone can provide
>> a list of GTK versions in current SLES,
> 
> SLES11: glib 2.22, gtk 2.18.9
> SLES12: glib 2.38, gtk 3.10.9
> 

Hi Jim,

I am trying to build with those deps, but it seems to require earlier
version of atk too, can you tell what is the version in SLES12?

Thank you, Eduardo

-- 
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 3/4] Port to GtkApplication API's

2015-12-03 Thread Eduardo Lima (Etrunko) 
On 11/30/2015 07:29 PM, Jonathon Jongsma wrote:

>> - All Window objects must be associated with the Application, and with
>>   this, there is no need to emit our own 'window-added' signal, it will
>>   be done by GtkApplication by the time gtk_application_add_window() is
>>   called. The 'window-removed' signal has also been removed, as it was
>>   not being used anyway.
> 
> The 'window-removed' signal was not being used, but I suspect that the main
> reason it was removed was that it conflicts with a signal of the same name 
> from
> GtkApplication. Perhaps useful to note that here.

Yes, I will add it to the message.

> 
>> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
>> index 3f530a3..7a1e870 100644
>> --- a/src/remote-viewer.c
>> +++ b/src/remote-viewer.c
>> @@ -36,11 +36,9 @@
>>  
>>  #ifdef HAVE_SPICE_GTK
>>  #include 
>> -#endif
>> -
>> -#ifdef HAVE_SPICE_GTK
>>  #include "virt-viewer-session-spice.h"
>>  #endif
>> +
> 
> 
> This hunk is fine, but not strictly necessary for porting to GtkApplication.
> Consider moving it to a separate cleanup patch?
> 

Noted, I can add it to a cleanup patch together with the next one.

>> @@ -195,10 +199,14 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>>  
>>  object_class->get_property = remote_viewer_get_property;
>>  object_class->set_property = remote_viewer_set_property;
>> +object_class->dispose = remote_viewer_dispose;
>>  
>>  app_class->start = remote_viewer_start;
>>  app_class->deactivated = remote_viewer_deactivated;
>> -object_class->dispose = remote_viewer_dispose;
> 
> This move could also possibly be moved to a cleanup patch, I suppose. Not sure
> it's worth it though...

I will add it together with previous hunk.

>>  #ifdef HAVE_SPICE_GTK
>>  app_class->activate = remote_viewer_activate;
>>  app_class->window_added = remote_viewer_window_added;
>> @@ -210,7 +218,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>>  "Spice controller",
>> 
>>  SPICE_CTRL_TYPE_CONTROLLER,
>>  G_PARAM_READWRITE |
>> -   
>>  G_PARAM_CONSTRUCT_ONLY |
>> 
>>  G_PARAM_STATIC_STRINGS));
>>  g_object_class_install_property(object_class,
>>  PROP_CTRL_FOREIGN_MENU,
>> @@ -219,7 +226,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>>  "Spice foreign 
>> menu",
>> 
>>  SPICE_CTRL_TYPE_FOREIGN_MENU,
>>  G_PARAM_READWRITE |
>> -   
>>  G_PARAM_CONSTRUCT_ONLY |
>> 
>>  G_PARAM_STATIC_STRINGS));
>>  #endif
>>  g_object_class_install_property(object_class,
>> @@ -229,7 +235,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>>   "Open recent
>> dialog",
>>   FALSE,
>>   G_PARAM_READWRITE |
>> -
>>  G_PARAM_CONSTRUCT_ONLY |
>>  
>>  G_PARAM_STATIC_STRINGS));
>>  }
> 
> 
> I understand why these properties are no longer construct-only, however I 
> wonder
> if we want to add a warning if we try to set these properties after starting 
> the
> application. We can now technically change these properties after 
> construction,
> but we only use these values within remote_viewer_start(). So any property
> changes that are made after calling remote_viewer_start() will not have any
> effect. Do we care?

I think it is fine the way it is, there is a restriction in set property
that only allows those properties being set for the first time.

>>  
>> @@ -240,11 +245,11 @@ remote_viewer_init(RemoteViewer *self)
>>  }
>>  
>>  RemoteViewer *
>> -remote_viewer_new(const gchar *uri)
>> +remote_viewer_new(void)
>>  {
>>  return g_object_new(REMOTE_VIEWER_TYPE,
>> -"guri", uri,
>> -"open-recent-dialog", uri == NULL,
>> +"application-id", "org.fedorahosted.remote-viewer",
> 
> should we use "org.spice-space.remote-viewer" instead? or "org.virt
> -manager.remote-viewer" (since virt-manager.org is where the releases are
> hosted)?
> 

Ok, simple to change, I was wondering which one to use, and ended up
with this one because of where the repository is hosted. I tend to like
"org.virt-manager" prefix rather than "org.spice-space", but I'm okay
with any of those.

>>  
>> +static const gchar*

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-03 Thread Eduardo Lima (Etrunko) 
On 12/02/2015 09:21 AM, Christophe Fergeau wrote:
> On Mon, Nov 30, 2015 at 03:29:37PM -0600, Jonathon Jongsma wrote:
>>> +app_class->add_main_options = remote_viewer_add_main_options;
>>> +app_class->handle_options = remote_viewer_handle_options;
>>> +app_class->version_string = remote_viewer_version_string;
>>> +
>>>  #ifdef HAVE_SPICE_GTK
>>>  app_class->activate = remote_viewer_activate;
>>>  app_class->window_added = remote_viewer_window_added;
>>> @@ -210,7 +218,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>>>  "Spice controller",
>>> 
>>>  SPICE_CTRL_TYPE_CONTROLLER,
>>>  G_PARAM_READWRITE |
>>> -   
>>>  G_PARAM_CONSTRUCT_ONLY |
>>> 
>>>  G_PARAM_STATIC_STRINGS));
>>>  g_object_class_install_property(object_class,
>>>  PROP_CTRL_FOREIGN_MENU,
>>> @@ -219,7 +226,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>>>  "Spice foreign 
>>> menu",
>>> 
>>>  SPICE_CTRL_TYPE_FOREIGN_MENU,
>>>  G_PARAM_READWRITE |
>>> -   
>>>  G_PARAM_CONSTRUCT_ONLY |
>>> 
>>>  G_PARAM_STATIC_STRINGS));
>>>  #endif
>>>  g_object_class_install_property(object_class,
>>> @@ -229,7 +235,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>>>   "Open recent
>>> dialog",
>>>   FALSE,
>>>   G_PARAM_READWRITE 
>>> |
>>> -
>>>  G_PARAM_CONSTRUCT_ONLY |
>>>  
>>>  G_PARAM_STATIC_STRINGS));
>>>  }
>>
>>
>> I understand why these properties are no longer construct-only, however I 
>> wonder
>> if we want to add a warning if we try to set these properties after starting 
>> the
>> application. We can now technically change these properties after 
>> construction,
>> but we only use these values within remote_viewer_start(). So any property
>> changes that are made after calling remote_viewer_start() will not have any
>> effect. Do we care?
> 
> I think these properties could be either removed or made read-only as we
> now set them from inside the class if I'm not mistaken.

Good idea. I will take a look at it.

>>
>>
>> Right now you have a virtual function the base class (handle_local_options())
>> that does some work and then calls a different virtual function
>> (handle_options()) that is implemented in each subclass (but not in the 
>> parent
>> class). I think it might be cleaner to just get rid of the separate
>> handle_options() vfunc and implement handle_local_options() in each of the
>> subclasses. Then it could chain up and call the parent vfunc to do the common
>> work in the parent class. 
> 
> Agreed, these vfunc could hopefully be improved.

As I answered in my previous email, I had a bit different flow in my
mind, but I can rework the logic to use the existing vfuncs.

> 
>>
>> But a bigger issue is that I think that handle_local_options() is not really 
>> the
>> function we're generally expected to use here. handle_local_options() is 
>> passed
>> a GVariantDict of options, which means that you have to inspect and handle 
>> them
>> all manually rather than letting the GOptionContext stuff do the work for 
>> you.
> 
> My understanding is that this is optional. If GOptionEntry::arg_data is
> NULL, then the argument will appear in the GVariantDict. If it's not
> NULL, then the GOptionEntry::arg_data will be used.
> 
>> That means that if we want to add or remove options, we update the list of
>> GOptionEntry objects and also update the handle_local_options function to 
>> handle
>> that new option. This adds maintenance burden and makes it more likely 
>> they'll
>> get out of sync.
> 
> Kind of. If you add a new option, you would add whatever variable you
> need to set in your GOptionEntry, then you'd have to do something with
> this variable in your code. That code would now go in
> handle_local_options() rather than in an arbitary place. If you want to
> avoid having to look at the GVariantDict to get the option value, you
> can.
> 
>> If we instead handled the command_line() vfunc, we could take
>> advantage of GOptionContext parsing instead of manually inspecting the
>> GVariantDict. See 
>> https://git.gnome.org/browse/glib/tree/gio/tests/gapplication-example-cmdline3.c
>>  for an example of using

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-03 Thread Daniel P. Berrange 
On Thu, Dec 03, 2015 at 12:17:41PM -0200, Eduardo Lima (Etrunko) wrote:
> On 12/02/2015 09:09 AM, Christophe Fergeau wrote:
> > Hey,
> > 
> > On Fri, Nov 27, 2015 at 05:24:00PM -0200, Eduardo Lima (Etrunko) wrote:
> >> Most of this patch consists in code being shuffled around to fit the
> >> expected flow while using the new APIs. I tried my best to make this
> >> patch the less intrusive as possible. Main changes are:
> >>
> >> - VirtViewerApp is now a subclass of GtkApplication
> >>
> >>   Also, some mainloop calls were replaced, as follows:
> >>* gtk_main() -> g_application_run()
> >>* gtk_quit() -> g_application_quit()
> >>
> >> - Unified command line option handling:
> >>   The logic has moved from the main functions and split in three, the
> >>   common options, and specific ones for each application. With this, the
> >>   main functions were highly simplified, and now basically responsible
> >>   for instantiating the App object and running the main loop.
> >>
> >> - All Window objects must be associated with the Application, and with
> >>   this, there is no need to emit our own 'window-added' signal, it will
> >>   be done by GtkApplication by the time gtk_application_add_window() is
> >>   called. The 'window-removed' signal has also been removed, as it was
> >>   not being used anyway.
> > 
> > GApplication was added in glib 2.28, but some of the API you are using (
> > g_application_add_option_group ) was added as recently as glib 2.40.
> > configure.ac needs to be updated to reflect this, or some alternatives
> > need to be considered if the glib 2.40 is too new (el7.0 had a too old
> > glib, el7.1 is fine, fedora 21 and newer are fine too).
> 
> Ok, I will update configure.ac too

No, we have to avoid anything from 2.40 - the newest glib we can use
based on major distro support is going to be 2.38, as I don't think
it is reasonable to drop support for current SLES.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-03 Thread Eduardo Lima (Etrunko) 
On 12/02/2015 09:09 AM, Christophe Fergeau wrote:
> Hey,
> 
> On Fri, Nov 27, 2015 at 05:24:00PM -0200, Eduardo Lima (Etrunko) wrote:
>> Most of this patch consists in code being shuffled around to fit the
>> expected flow while using the new APIs. I tried my best to make this
>> patch the less intrusive as possible. Main changes are:
>>
>> - VirtViewerApp is now a subclass of GtkApplication
>>
>>   Also, some mainloop calls were replaced, as follows:
>>* gtk_main() -> g_application_run()
>>* gtk_quit() -> g_application_quit()
>>
>> - Unified command line option handling:
>>   The logic has moved from the main functions and split in three, the
>>   common options, and specific ones for each application. With this, the
>>   main functions were highly simplified, and now basically responsible
>>   for instantiating the App object and running the main loop.
>>
>> - All Window objects must be associated with the Application, and with
>>   this, there is no need to emit our own 'window-added' signal, it will
>>   be done by GtkApplication by the time gtk_application_add_window() is
>>   called. The 'window-removed' signal has also been removed, as it was
>>   not being used anyway.
> 
> GApplication was added in glib 2.28, but some of the API you are using (
> g_application_add_option_group ) was added as recently as glib 2.40.
> configure.ac needs to be updated to reflect this, or some alternatives
> need to be considered if the glib 2.40 is too new (el7.0 had a too old
> glib, el7.1 is fine, fedora 21 and newer are fine too).

Ok, I will update configure.ac too

> 
> If you start remote-viewer once, and then launch a second instance to
> connect to the same URL, then the first instance stays alive rather than
> dying. Similarly, attempting to connect to an IP/port where no SPICE
> instance is listening shows an error message, but does not exit
> remote-viewer nor show the URI selection dialog.
> 

Thanks for the use case, I will test it here.

>> @@ -195,10 +199,14 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>>  
>>  object_class->get_property = remote_viewer_get_property;
>>  object_class->set_property = remote_viewer_set_property;
>> +object_class->dispose = remote_viewer_dispose;
>>  
>>  app_class->start = remote_viewer_start;
>>  app_class->deactivated = remote_viewer_deactivated;
>> -object_class->dispose = remote_viewer_dispose;
>> +app_class->add_main_options = remote_viewer_add_main_options;
> 
> Do we really need this add_main_options vfunc? Could this be done in
> (eg) constructed instead?

I think it could be done with constructed, yes. I just need to check the
possibility.

>> +static gint
>> +remote_viewer_handle_options(VirtViewerApp *app, GVariantDict *options)
>> +{
>> +gint ret = -1;
>> +gchar *title = NULL;
>> +gchar **args = NULL;
>> +gboolean controller = FALSE;
>> +
>> +if (g_variant_dict_lookup(options, G_OPTION_REMAINING, "^as", )) {
>> +if (args && (g_strv_length(args) != 1)) {
>> +g_printerr(_("Error: can't handle multiple URIs\n"));
>> +ret = 0;
>> +goto end;
>> +}
>> +
>> +g_object_set(app, "guri", args[0], NULL);
>> +}
>> +
>> +g_variant_dict_lookup(options, OPT_TITLE, "s", );
> 
> I believe this could be "", which would allow you to avoid the g_free
> in end: (I know you told me on IRC this did not work as expected, but I
> tested it and it works locally, so there is potentially more digging to
> do).

Ok, I will double check it.

> 
>> +
>> +#ifdef HAVE_SPICE_GTK
>> +if (g_variant_dict_lookup(options, OPT_CONTROLLER, "b", )) {
>> +if (args) {
>> +g_printerr(_("Error: extra arguments given while using Spice 
>> controller\n\n"));
>> +ret = 0;
>> +goto end;
>> +} else {
>> +RemoteViewer *self = REMOTE_VIEWER(app);
>> +SpiceCtrlController *ctrl = spice_ctrl_controller_new();
>> +SpiceCtrlForeignMenu *menu = spice_ctrl_foreign_menu_new();
>> +
>> +g_object_set(self, "guest-name", "defined by Spice controller",
>> +   "controller", ctrl,
>> +   "foreign-menu", menu,
>> +   NULL);
>> +
>> +g_signal_connect(menu, "notify::title",
>> + G_CALLBACK(foreign_menu_title_changed),
>> + self);
>> +
>> +g_object_unref(ctrl);
>> +g_object_unref(menu);
>> +}
>> +}
>> +#endif
>> +
>> +if (title && !controller) {
>> +g_printerr("* setting title to '%s'\n", title);
> 
> Looks like some left over debugging
> 

Yes, thanks for the catch.

r_app_set_attach(app, attach);
>> +static gint
>> +virt_viewer_handle_options(VirtViewerApp *app, GVariantDict *options)
>> +{
>> +VirtViewer *self = VIRT_VIEWER(app);
>> +gint ret = -1;
>> +gchar *uri = NULL;
>> +

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-03 Thread Daniel P. Berrange 
On Wed, Dec 02, 2015 at 06:25:12PM -0700, Jim Fehlig wrote:
> On 12/02/2015 04:15 AM, Daniel P. Berrange wrote:
> > Yep, we need to agree which min platform we're targetting. It is nice
> > to see RHEL7 rebasing GTK, as that makes life easier. We should not
> > only consider RHEL though. It would be desirable if someone can provide
> > a list of GTK versions in current SLES,
> 
> SLES11: glib 2.22, gtk 2.18.9
> SLES12: glib 2.38, gtk 3.10.9
> 
> SLES11 builds for newer virt-viewer are already disabled.

Debian Jessie has glib 2.42, gtk 3.14.5

Ubuntu LTS 14.04 has glib 2.40.0, gtk 3.10.8

RHEL-7.1 has glib 2.40.0, gtk 3.8.8

The lowest common denominator across these major distros is

  glib 2.38
  gtk  3.8.8

So I think that is what virt-viewer should be sticking to as minimum
versions.

For glib we should make sure we set  GLIB_VERSION_MIN_REQUIRED and
GLIB_VERSION_MAX_ALLOWED to match the min version, so that we don't
accidentally introduce use of newer functions.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-02 Thread Christophe Fergeau 
On Mon, Nov 30, 2015 at 03:29:37PM -0600, Jonathon Jongsma wrote:
> > +app_class->add_main_options = remote_viewer_add_main_options;
> > +app_class->handle_options = remote_viewer_handle_options;
> > +app_class->version_string = remote_viewer_version_string;
> > +
> >  #ifdef HAVE_SPICE_GTK
> >  app_class->activate = remote_viewer_activate;
> >  app_class->window_added = remote_viewer_window_added;
> > @@ -210,7 +218,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
> >  "Spice controller",
> > 
> >  SPICE_CTRL_TYPE_CONTROLLER,
> >  G_PARAM_READWRITE |
> > -   
> >  G_PARAM_CONSTRUCT_ONLY |
> > 
> >  G_PARAM_STATIC_STRINGS));
> >  g_object_class_install_property(object_class,
> >  PROP_CTRL_FOREIGN_MENU,
> > @@ -219,7 +226,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
> >  "Spice foreign 
> > menu",
> > 
> >  SPICE_CTRL_TYPE_FOREIGN_MENU,
> >  G_PARAM_READWRITE |
> > -   
> >  G_PARAM_CONSTRUCT_ONLY |
> > 
> >  G_PARAM_STATIC_STRINGS));
> >  #endif
> >  g_object_class_install_property(object_class,
> > @@ -229,7 +235,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
> >   "Open recent
> > dialog",
> >   FALSE,
> >   G_PARAM_READWRITE 
> > |
> > -
> >  G_PARAM_CONSTRUCT_ONLY |
> >  
> >  G_PARAM_STATIC_STRINGS));
> >  }
> 
> 
> I understand why these properties are no longer construct-only, however I 
> wonder
> if we want to add a warning if we try to set these properties after starting 
> the
> application. We can now technically change these properties after 
> construction,
> but we only use these values within remote_viewer_start(). So any property
> changes that are made after calling remote_viewer_start() will not have any
> effect. Do we care?

I think these properties could be either removed or made read-only as we
now set them from inside the class if I'm not mistaken.

> > +{ OPT_TITLE, 't', 0, G_OPTION_ARG_STRING, NULL,
> > +  N_("Set window title"), NULL },
> > +#ifdef HAVE_SPICE_GTK
> > +{ OPT_CONTROLLER, '\0', 0, G_OPTION_ARG_NONE, NULL,
> > +  N_("Open connection using Spice controller communication"), NULL 
> > },
> > +#endif
> > +{ G_OPTION_REMAINING, '\0', 0, G_OPTION_ARG_STRING_ARRAY, NULL,
> > +  NULL, "URI|VV-FILE" },
> > +{ NULL },
> > +};
> > +
> > +   g_application_add_main_option_entries(app, options);
> > +
> > +#ifdef HAVE_OVIRT
> > +g_application_add_option_group(app, ovirt_get_option_group ());
> > +#endif
> > +}
> > +
> > +static gint
> > +remote_viewer_handle_options(VirtViewerApp *app, GVariantDict *options)
> > +{
> > +gint ret = -1;
> > +gchar *title = NULL;
> > +gchar **args = NULL;
> > +gboolean controller = FALSE;
> > +
> > +if (g_variant_dict_lookup(options, G_OPTION_REMAINING, "^as", )) {
> > +if (args && (g_strv_length(args) != 1)) {
> > +g_printerr(_("Error: can't handle multiple URIs\n"));
> > +ret = 0;
> > +goto end;
> > +}
> > +
> > +g_object_set(app, "guri", args[0], NULL);
> > +}
> > +
> > +g_variant_dict_lookup(options, OPT_TITLE, "s", );
> > +
> > +#ifdef HAVE_SPICE_GTK
> > +if (g_variant_dict_lookup(options, OPT_CONTROLLER, "b", )) {
> > +if (args) {
> > +g_printerr(_("Error: extra arguments given while using Spice
> > controller\n\n"));
> > +ret = 0;
> > +goto end;
> > +} else {
> > +RemoteViewer *self = REMOTE_VIEWER(app);
> > +SpiceCtrlController *ctrl = spice_ctrl_controller_new();
> > +SpiceCtrlForeignMenu *menu = spice_ctrl_foreign_menu_new();
> > +
> > +g_object_set(self, "guest-name", "defined by Spice controller",
> > +   "controller", ctrl,
> > +   "foreign-menu", menu,
> > +   NULL);
> > +
> > +g_signal_connect(menu, "notify::title",
> > + G_CALLBACK(foreign_menu_title_changed),
> > + self);
> > +
> > +g_object_unref(ctrl);
> > +

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-02 Thread Christophe Fergeau 
Hey,

On Fri, Nov 27, 2015 at 05:24:00PM -0200, Eduardo Lima (Etrunko) wrote:
> Most of this patch consists in code being shuffled around to fit the
> expected flow while using the new APIs. I tried my best to make this
> patch the less intrusive as possible. Main changes are:
> 
> - VirtViewerApp is now a subclass of GtkApplication
> 
>   Also, some mainloop calls were replaced, as follows:
>* gtk_main() -> g_application_run()
>* gtk_quit() -> g_application_quit()
> 
> - Unified command line option handling:
>   The logic has moved from the main functions and split in three, the
>   common options, and specific ones for each application. With this, the
>   main functions were highly simplified, and now basically responsible
>   for instantiating the App object and running the main loop.
> 
> - All Window objects must be associated with the Application, and with
>   this, there is no need to emit our own 'window-added' signal, it will
>   be done by GtkApplication by the time gtk_application_add_window() is
>   called. The 'window-removed' signal has also been removed, as it was
>   not being used anyway.

GApplication was added in glib 2.28, but some of the API you are using (
g_application_add_option_group ) was added as recently as glib 2.40.
configure.ac needs to be updated to reflect this, or some alternatives
need to be considered if the glib 2.40 is too new (el7.0 had a too old
glib, el7.1 is fine, fedora 21 and newer are fine too).

If you start remote-viewer once, and then launch a second instance to
connect to the same URL, then the first instance stays alive rather than
dying. Similarly, attempting to connect to an IP/port where no SPICE
instance is listening shows an error message, but does not exit
remote-viewer nor show the URI selection dialog.


> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index 3f530a3..7a1e870 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -36,11 +36,9 @@
>  
>  #ifdef HAVE_SPICE_GTK
>  #include 
> -#endif
> -
> -#ifdef HAVE_SPICE_GTK
>  #include "virt-viewer-session-spice.h"
>  #endif
> +
>  #include "virt-viewer-app.h"
>  #include "virt-viewer-auth.h"
>  #include "virt-viewer-file.h"
> @@ -84,6 +82,12 @@ static OvirtVm * choose_vm(GtkWindow *main_window,
>  #endif
>  
>  static gboolean remote_viewer_start(VirtViewerApp *self, GError **error);
> +
> +/* VirtViewerApp overrides */
> +static void remote_viewer_add_main_options(VirtViewerApp *self);
> +static gint remote_viewer_handle_options(VirtViewerApp *self, GVariantDict 
> *options);
> +static const gchar *remote_viewer_version_string(void);
> +
>  #ifdef HAVE_SPICE_GTK
>  static gboolean remote_viewer_activate(VirtViewerApp *self, GError **error);
>  static void remote_viewer_window_added(VirtViewerApp *self, VirtViewerWindow 
> *win);
> @@ -195,10 +199,14 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>  
>  object_class->get_property = remote_viewer_get_property;
>  object_class->set_property = remote_viewer_set_property;
> +object_class->dispose = remote_viewer_dispose;
>  
>  app_class->start = remote_viewer_start;
>  app_class->deactivated = remote_viewer_deactivated;
> -object_class->dispose = remote_viewer_dispose;
> +app_class->add_main_options = remote_viewer_add_main_options;

Do we really need this add_main_options vfunc? Could this be done in
(eg) constructed instead?

> +app_class->handle_options = remote_viewer_handle_options;
> +app_class->version_string = remote_viewer_version_string;
> +
>  #ifdef HAVE_SPICE_GTK
>  app_class->activate = remote_viewer_activate;
>  app_class->window_added = remote_viewer_window_added;
> @@ -210,7 +218,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>  "Spice controller",
>  
> SPICE_CTRL_TYPE_CONTROLLER,
>  G_PARAM_READWRITE |
> -
> G_PARAM_CONSTRUCT_ONLY |
>  
> G_PARAM_STATIC_STRINGS));
>  g_object_class_install_property(object_class,
>  PROP_CTRL_FOREIGN_MENU,
> @@ -219,7 +226,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>  "Spice foreign menu",
>  
> SPICE_CTRL_TYPE_FOREIGN_MENU,
>  G_PARAM_READWRITE |
> -
> G_PARAM_CONSTRUCT_ONLY |
>  
> G_PARAM_STATIC_STRINGS));
>  #endif
>  g_object_class_install_property(object_class,
> @@ -229,7 +235,6 @@ remote_viewer_class_init (RemoteViewerClass *klass)
>

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-02 Thread Daniel P. Berrange 
On Wed, Dec 02, 2015 at 12:09:56PM +0100, Christophe Fergeau wrote:
> Hey,
> 
> On Fri, Nov 27, 2015 at 05:24:00PM -0200, Eduardo Lima (Etrunko) wrote:
> > Most of this patch consists in code being shuffled around to fit the
> > expected flow while using the new APIs. I tried my best to make this
> > patch the less intrusive as possible. Main changes are:
> > 
> > - VirtViewerApp is now a subclass of GtkApplication
> > 
> >   Also, some mainloop calls were replaced, as follows:
> >* gtk_main() -> g_application_run()
> >* gtk_quit() -> g_application_quit()
> > 
> > - Unified command line option handling:
> >   The logic has moved from the main functions and split in three, the
> >   common options, and specific ones for each application. With this, the
> >   main functions were highly simplified, and now basically responsible
> >   for instantiating the App object and running the main loop.
> > 
> > - All Window objects must be associated with the Application, and with
> >   this, there is no need to emit our own 'window-added' signal, it will
> >   be done by GtkApplication by the time gtk_application_add_window() is
> >   called. The 'window-removed' signal has also been removed, as it was
> >   not being used anyway.
> 
> GApplication was added in glib 2.28, but some of the API you are using (
> g_application_add_option_group ) was added as recently as glib 2.40.
> configure.ac needs to be updated to reflect this, or some alternatives
> need to be considered if the glib 2.40 is too new (el7.0 had a too old
> glib, el7.1 is fine, fedora 21 and newer are fine too).

Yep, we need to agree which min platform we're targetting. It is nice
to see RHEL7 rebasing GTK, as that makes life easier. We should not
only consider RHEL though. It would be desirable if someone can provide
a list of GTK versions in current SLES, Ubuntu LTS, Debian and RHEL7
updates, and we can thus make sure we don't pick something that excludes
a major distro's current long term release (I'm assuming all the non-LTS
distros will be just fine since they track current state of art).

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-12-02 Thread Jim Fehlig 
On 12/02/2015 04:15 AM, Daniel P. Berrange wrote:
> Yep, we need to agree which min platform we're targetting. It is nice
> to see RHEL7 rebasing GTK, as that makes life easier. We should not
> only consider RHEL though. It would be desirable if someone can provide
> a list of GTK versions in current SLES,

SLES11: glib 2.22, gtk 2.18.9
SLES12: glib 2.38, gtk 3.10.9

SLES11 builds for newer virt-viewer are already disabled.

Regards,
Jim

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

Re: [virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-11-30 Thread Jonathon Jongsma 
On Fri, 2015-11-27 at 17:24 -0200, Eduardo Lima (Etrunko) wrote:
> Most of this patch consists in code being shuffled around to fit the
> expected flow while using the new APIs. I tried my best to make this
> patch the less intrusive as possible. Main changes are:
> 
> - VirtViewerApp is now a subclass of GtkApplication
> 
>   Also, some mainloop calls were replaced, as follows:
>* gtk_main() -> g_application_run()
>* gtk_quit() -> g_application_quit()
> 
> - Unified command line option handling:
>   The logic has moved from the main functions and split in three, the
>   common options, and specific ones for each application. With this, the
>   main functions were highly simplified, and now basically responsible
>   for instantiating the App object and running the main loop.
> 
> - All Window objects must be associated with the Application, and with
>   this, there is no need to emit our own 'window-added' signal, it will
>   be done by GtkApplication by the time gtk_application_add_window() is
>   called. The 'window-removed' signal has also been removed, as it was
>   not being used anyway.

The 'window-removed' signal was not being used, but I suspect that the main
reason it was removed was that it conflicts with a signal of the same name from
GtkApplication. Perhaps useful to note that here.

> 
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  src/remote-viewer-main.c | 122 +-
>  src/remote-viewer.c  | 137 ++---
>  src/remote-viewer.h  |   3 +-
>  src/virt-viewer-app.c| 221 ++
> -
>  src/virt-viewer-app.h|  12 ++-
>  src/virt-viewer-main.c   | 102 +-
>  src/virt-viewer.c| 105 +-
>  src/virt-viewer.h|   8 +-
>  8 files changed, 346 insertions(+), 364 deletions(-)
> 
> diff --git a/src/remote-viewer-main.c b/src/remote-viewer-main.c
> index 81cf736..0329d16 100644
> --- a/src/remote-viewer-main.c
> +++ b/src/remote-viewer-main.c
> @@ -30,32 +30,11 @@
>  #include 
>  #endif
>  
> -#ifdef HAVE_GTK_VNC
> -#include 
> -#endif
> -#ifdef HAVE_SPICE_GTK
> -#include 
> -#endif
> -#ifdef HAVE_OVIRT
> -#include 
> -#endif
> -
>  #include "remote-viewer.h"
>  #include "virt-viewer-app.h"
>  #include "virt-viewer-session.h"
>  
>  static void
> -remote_viewer_version(void)
> -{
> -g_print(_("remote-viewer version %s"), VERSION BUILDID);
> -#ifdef REMOTE_VIEWER_OS_ID
> -g_print(" (OS ID: %s)", REMOTE_VIEWER_OS_ID);
> -#endif
> -g_print("\n");
> -exit(EXIT_SUCCESS);
> -}
> -
> -static void
>  recent_add(gchar *uri, const gchar *mime_type)
>  {
>  GtkRecentManager *recent;
> @@ -87,118 +66,23 @@ static void connected(VirtViewerSession *session,
>  int
>  main(int argc, char **argv)
>  {
> -GOptionContext *context;
> -GError *error = NULL;
>  int ret = 1;
> -gchar **args = NULL;
> -gchar *uri = NULL;
> -char *title = NULL;
>  RemoteViewer *viewer = NULL;
> -#ifdef HAVE_SPICE_GTK
> -gboolean controller = FALSE;
> -#endif
>  VirtViewerApp *app;
> -const GOptionEntry options [] = {
> -{ "version", 'V', G_OPTION_FLAG_NO_ARG, G_OPTION_ARG_CALLBACK,
> -  remote_viewer_version, N_("Display version information"), NULL },
> -{ "title", 't', 0, G_OPTION_ARG_STRING, ,
> -  N_("Set window title"), NULL },
> -#ifdef HAVE_SPICE_GTK
> -{ "spice-controller", '\0', 0, G_OPTION_ARG_NONE, ,
> -  N_("Open connection using Spice controller communication"), NULL },
> -#endif
> -{ G_OPTION_REMAINING, '\0', 0, G_OPTION_ARG_STRING_ARRAY, ,
> -  NULL, "URI|VV-FILE" },
> -{ NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }
> -};
> -GOptionGroup *app_options = NULL;
>  
>  virt_viewer_util_init(_("Remote Viewer"));
>  
> -/* Setup command line options */
> -context = g_option_context_new (NULL);
> -g_option_context_set_summary(context, _("Remote viewer client"));
> -app_options = virt_viewer_app_get_option_group();
> -g_option_group_add_entries (app_options, options);
> -g_option_context_set_main_group (context, app_options);
> -g_option_context_add_group (context, gtk_get_option_group (TRUE));
> -#ifdef HAVE_GTK_VNC
> -g_option_context_add_group (context, vnc_display_get_option_group ());
> -#endif
> -#ifdef HAVE_SPICE_GTK
> -g_option_context_add_group (context, spice_get_option_group ());
> -#endif
> -#ifdef HAVE_OVIRT
> -g_option_context_add_group (context, ovirt_get_option_group ());
> -#endif
> -g_option_context_parse (context, , , );
> -if (error) {
> -char *base_name;
> -base_name = g_path_get_basename(argv[0]);
> -g_printerr(_("%s\nRun '%s --help' to see a full list of available
> command line options\n"),
> -   error->message, base_name);
> -g_free(base_name);
> -goto cleanup;
> -}
> -
> -

[virt-tools-list] [PATCH 3/4] Port to GtkApplication API's

2015-11-27 Thread Eduardo Lima (Etrunko) 
Most of this patch consists in code being shuffled around to fit the
expected flow while using the new APIs. I tried my best to make this
patch the less intrusive as possible. Main changes are:

- VirtViewerApp is now a subclass of GtkApplication

  Also, some mainloop calls were replaced, as follows:
   * gtk_main() -> g_application_run()
   * gtk_quit() -> g_application_quit()

- Unified command line option handling:
  The logic has moved from the main functions and split in three, the
  common options, and specific ones for each application. With this, the
  main functions were highly simplified, and now basically responsible
  for instantiating the App object and running the main loop.

- All Window objects must be associated with the Application, and with
  this, there is no need to emit our own 'window-added' signal, it will
  be done by GtkApplication by the time gtk_application_add_window() is
  called. The 'window-removed' signal has also been removed, as it was
  not being used anyway.

Signed-off-by: Eduardo Lima (Etrunko) 
---
 src/remote-viewer-main.c | 122 +-
 src/remote-viewer.c  | 137 ++---
 src/remote-viewer.h  |   3 +-
 src/virt-viewer-app.c| 221 ++-
 src/virt-viewer-app.h|  12 ++-
 src/virt-viewer-main.c   | 102 +-
 src/virt-viewer.c| 105 +-
 src/virt-viewer.h|   8 +-
 8 files changed, 346 insertions(+), 364 deletions(-)

diff --git a/src/remote-viewer-main.c b/src/remote-viewer-main.c
index 81cf736..0329d16 100644
--- a/src/remote-viewer-main.c
+++ b/src/remote-viewer-main.c
@@ -30,32 +30,11 @@
 #include 
 #endif
 
-#ifdef HAVE_GTK_VNC
-#include 
-#endif
-#ifdef HAVE_SPICE_GTK
-#include 
-#endif
-#ifdef HAVE_OVIRT
-#include 
-#endif
-
 #include "remote-viewer.h"
 #include "virt-viewer-app.h"
 #include "virt-viewer-session.h"
 
 static void
-remote_viewer_version(void)
-{
-g_print(_("remote-viewer version %s"), VERSION BUILDID);
-#ifdef REMOTE_VIEWER_OS_ID
-g_print(" (OS ID: %s)", REMOTE_VIEWER_OS_ID);
-#endif
-g_print("\n");
-exit(EXIT_SUCCESS);
-}
-
-static void
 recent_add(gchar *uri, const gchar *mime_type)
 {
 GtkRecentManager *recent;
@@ -87,118 +66,23 @@ static void connected(VirtViewerSession *session,
 int
 main(int argc, char **argv)
 {
-GOptionContext *context;
-GError *error = NULL;
 int ret = 1;
-gchar **args = NULL;
-gchar *uri = NULL;
-char *title = NULL;
 RemoteViewer *viewer = NULL;
-#ifdef HAVE_SPICE_GTK
-gboolean controller = FALSE;
-#endif
 VirtViewerApp *app;
-const GOptionEntry options [] = {
-{ "version", 'V', G_OPTION_FLAG_NO_ARG, G_OPTION_ARG_CALLBACK,
-  remote_viewer_version, N_("Display version information"), NULL },
-{ "title", 't', 0, G_OPTION_ARG_STRING, ,
-  N_("Set window title"), NULL },
-#ifdef HAVE_SPICE_GTK
-{ "spice-controller", '\0', 0, G_OPTION_ARG_NONE, ,
-  N_("Open connection using Spice controller communication"), NULL },
-#endif
-{ G_OPTION_REMAINING, '\0', 0, G_OPTION_ARG_STRING_ARRAY, ,
-  NULL, "URI|VV-FILE" },
-{ NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }
-};
-GOptionGroup *app_options = NULL;
 
 virt_viewer_util_init(_("Remote Viewer"));
 
-/* Setup command line options */
-context = g_option_context_new (NULL);
-g_option_context_set_summary(context, _("Remote viewer client"));
-app_options = virt_viewer_app_get_option_group();
-g_option_group_add_entries (app_options, options);
-g_option_context_set_main_group (context, app_options);
-g_option_context_add_group (context, gtk_get_option_group (TRUE));
-#ifdef HAVE_GTK_VNC
-g_option_context_add_group (context, vnc_display_get_option_group ());
-#endif
-#ifdef HAVE_SPICE_GTK
-g_option_context_add_group (context, spice_get_option_group ());
-#endif
-#ifdef HAVE_OVIRT
-g_option_context_add_group (context, ovirt_get_option_group ());
-#endif
-g_option_context_parse (context, , , );
-if (error) {
-char *base_name;
-base_name = g_path_get_basename(argv[0]);
-g_printerr(_("%s\nRun '%s --help' to see a full list of available 
command line options\n"),
-   error->message, base_name);
-g_free(base_name);
-goto cleanup;
-}
-
-g_option_context_free(context);
-
-#ifdef HAVE_SPICE_GTK
-if (controller) {
-if (args) {
-g_printerr(_("Error: extra arguments given while using Spice 
controller\n"));
-goto cleanup;
-}
-} else
-#endif
-if (args) {
-if (g_strv_length(args) > 1) {
-g_printerr(_("Error: can't handle multiple URIs\n"));
-goto cleanup;
-} else if (g_strv_length(args) == 1) {
-uri = g_strdup(args[0]);
-}
-}
-
-#ifdef HAVE_SPICE_GTK
-if