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@xxxxxxxxxx
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
