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

[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", &args)) {
>> +        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", &title);
> 
> I believe this could be "&s", 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", &controller)) {
>> +        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;
>> +    gchar **args = NULL;
>> +
>> +    if (g_variant_dict_lookup(options, G_OPTION_REMAINING, "^as", &args)) {
>> +        /* FIXME: with gapplication we should be able to accept more than one domain */
>> +        if (args && (g_strv_length(args) != 1)) {
>> +            g_printerr(_("\nUsage: %s [OPTIONS] [DOMAIN-NAME|ID|UUID]\n\n"), PACKAGE);
>> +            ret = 0;
>> +            goto end;
>> +        }
>>  
>> -    /* should probably be properties instead */
>> -    priv->uri = g_strdup(uri);
>> -    priv->domkey = g_strdup(name);
>> -    priv->waitvm = waitvm;
>> -    priv->reconnect = reconnect;
>> +        self->priv->domkey = g_strdup(args[0]);
>> +    }
>>  
>> -    return self;
>> +    if (g_variant_dict_contains(options, OPT_WAIT)) {
>> +        g_printerr("self->priv->domkey = %s\n", self->priv->domkey);
> 
> Left-over debugging too?
> 

Yes, thank you.
-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com