[virt-tools-list] [PATCH 3/4] Port to GtkApplication API's
Christophe Fergeau
cfergeau at redhat.com
Wed Dec 2 11:09:56 UTC 2015
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 <spice-controller.h>
> -#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)
> "Open recent dialog",
> FALSE,
> G_PARAM_READWRITE |
> - G_PARAM_CONSTRUCT_ONLY |
> G_PARAM_STATIC_STRINGS));
> }
>
> @@ -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",
> + "flags", G_APPLICATION_NON_UNIQUE,
> NULL);
> }
>
> @@ -267,26 +272,6 @@ foreign_menu_title_changed(SpiceCtrlForeignMenu *menu G_GNUC_UNUSED,
> spice_foreign_menu_updated(self);
> }
>
> -RemoteViewer *
> -remote_viewer_new_with_controller(void)
> -{
> - RemoteViewer *self;
> - SpiceCtrlController *ctrl = spice_ctrl_controller_new();
> - SpiceCtrlForeignMenu *menu = spice_ctrl_foreign_menu_new();
> -
> - self = g_object_new(REMOTE_VIEWER_TYPE,
> - "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);
> -
> - return self;
> -}
> -
> static void
> spice_ctrl_do_connect(SpiceCtrlController *ctrl G_GNUC_UNUSED,
> VirtViewerApp *self)
> @@ -1187,6 +1172,98 @@ cleanup:
> return ret;
> }
>
> +static const gchar*
> +remote_viewer_version_string(void)
> +{
> +#ifdef REMOTE_VIEWER_OS_ID
> + return " (OS ID: " REMOTE_VIEWER_OS_ID ")";
> +#endif
> + return "";
> +}
> +
> +static const char OPT_TITLE[] = "title";
> +static const char OPT_CONTROLLER[] = "spice-controller";
> +
> +static void
> +remote_viewer_add_main_options(VirtViewerApp *self)
> +{
> + GApplication *app = G_APPLICATION (self);
> + const GOptionEntry options [] = {
> + { 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", &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).
> +
> +#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
> + g_object_set(app, "title", title, NULL);
> + }
> +
> +end:
> + g_strfreev(args);
> + g_free(title);
> + return ret;
> +}
> +
> /*
> * Local variables:
> * c-indent-level: 4
> diff --git a/src/virt-viewer.c b/src/virt-viewer.c
> index 10f624d..4fab44a 100644
> --- a/src/virt-viewer.c
> +++ b/src/virt-viewer.c
> @@ -975,34 +982,84 @@ virt_viewer_start(VirtViewerApp *app, GError **error)
> return VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->start(app, error);
> }
>
> -VirtViewer *
> -virt_viewer_new(const char *uri,
> - const char *name,
> - gboolean direct,
> - gboolean attach,
> - gboolean waitvm,
> - gboolean reconnect)
> +static const char OPT_DIRECT[] = "direct";
> +static const char OPT_ATTACH[] = "attach";
> +static const char OPT_CONNECT[] = "connect";
> +static const char OPT_WAIT[] = "wait";
> +static const char OPT_RECONNECT[] = "reconnect";
> +
> +static void
> +virt_viewer_add_main_options(VirtViewerApp *self)
> {
> - VirtViewer *self;
> - VirtViewerApp *app;
> - VirtViewerPrivate *priv;
> + GApplication *app = G_APPLICATION (self);
> + const GOptionEntry options [] = {
> + { OPT_DIRECT, 'd', 0, G_OPTION_ARG_NONE, NULL,
> + N_("Direct connection with no automatic tunnels"), NULL },
> + { OPT_ATTACH, 'a', 0, G_OPTION_ARG_NONE, NULL,
> + N_("Attach to the local display using libvirt"), NULL },
> + { OPT_CONNECT, 'c', 0, G_OPTION_ARG_STRING, NULL,
> + N_("Connect to hypervisor"), "URI"},
> + { OPT_WAIT, 'w', 0, G_OPTION_ARG_NONE, NULL,
> + N_("Wait for domain to start"), NULL },
> + { OPT_RECONNECT, 'r', 0, G_OPTION_ARG_NONE, NULL,
> + N_("Reconnect to domain upon restart"), NULL },
> + { G_OPTION_REMAINING, '\0', 0, G_OPTION_ARG_STRING_ARRAY, NULL,
> + NULL, "-- DOMAIN-NAME|ID|UUID" },
> + { NULL },
> + };
>
> - self = g_object_new(VIRT_VIEWER_TYPE,
> - "guest-name", name,
> - NULL);
> - app = VIRT_VIEWER_APP(self);
> - priv = self->priv;
> + g_application_add_main_option_entries(app, options);
> +}
>
> - virt_viewer_app_set_direct(app, direct);
> - virt_viewer_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?
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20151202/72c6800d/attachment.sig>
More information about the virt-tools-list
mailing list