[virt-tools-list] [PATCH v4 3/5] Port to GtkApplication API's
Eduardo Lima (Etrunko)
etrunko at redhat.com
Tue Feb 16 19:22:33 UTC 2016
On 02/16/2016 03:25 PM, Jonathon Jongsma wrote:
> Looks pretty good. Comments inline.
>
>> diff --git a/src/remote-viewer-main.c b/src/remote-viewer-main.c
>> index 81cf736..b05f27b 100644
>> --- a/src/remote-viewer-main.c
>> +++ b/src/remote-viewer-main.c
>> @@ -22,184 +22,29 @@
>>
>> #include <config.h>
>> #include <locale.h>
>> +#include <gio/gio.h>
>> #include <gtk/gtk.h>
>> #include <glib/gi18n.h>
>> #include <stdlib.h>
>> -#ifdef G_OS_WIN32
>> -#include <windows.h>
>> -#include <io.h>
>> -#endif
>> -
>> -#ifdef HAVE_GTK_VNC
>> -#include <vncdisplay.h>
>> -#endif
>> -#ifdef HAVE_SPICE_GTK
>> -#include <spice-option.h>
>> -#endif
>> -#ifdef HAVE_OVIRT
>> -#include <govirt/ovirt-options.h>
>> -#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;
>> - GtkRecentData meta = {
>> - .app_name = (char*)"remote-viewer",
>> - .app_exec = (char*)"remote-viewer %u",
>> - .mime_type = (char*)mime_type,
>> - };
>> -
>> - if (uri == NULL)
>> - return;
>> -
>> - recent = gtk_recent_manager_get_default();
>> - meta.display_name = uri;
>> - if (!gtk_recent_manager_add_full(recent, uri, &meta))
>> - g_warning("Recent item couldn't be added");
>> -}
>> -
>> -static void connected(VirtViewerSession *session,
>> - VirtViewerApp *self G_GNUC_UNUSED)
>> -{
>> - gchar *uri = virt_viewer_session_get_uri(session);
>> - const gchar *mime = virt_viewer_session_mime_type(session);
>> -
>> - recent_add(uri, mime);
>> - g_free(uri);
>> -}
>>
>> 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, &title,
>> - N_("Set window title"), NULL },
>> -#ifdef HAVE_SPICE_GTK
>> - { "spice-controller", '\0', 0, G_OPTION_ARG_NONE, &controller,
>> - N_("Open connection using Spice controller communication"), NULL },
>> -#endif
>> - { G_OPTION_REMAINING, '\0', 0, G_OPTION_ARG_STRING_ARRAY, &args,
>> - 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, &argc, &argv, &error);
>> - 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 (controller) {
>> - viewer = remote_viewer_new_with_controller();
>> - g_object_set(viewer, "guest-name", "defined by Spice controller",
>> NULL);
>> - } else {
>> -#endif
>> - viewer = remote_viewer_new(uri);
>> - if (title)
>> - g_object_set(viewer, "title", title, NULL);
>> -#ifdef HAVE_SPICE_GTK
>> - }
>> -#endif
>> + viewer = remote_viewer_new();
>> if (viewer == NULL)
>
> Unrelated to your patch, but viewer cannot actually be NULL here.
> remote_viewer_new() just calls g_object_new(), and glib always aborts if it
> can't allocate memory.
>
Thanks for the clarification, and by looking at it again, the
{remote,virt}_viewer_new() functions are completely useless, and I think
that we can move the g_object_new() calls to the main function directly.
>> - goto cleanup;
>> -
>> - app = VIRT_VIEWER_APP(viewer);
>> -
>> - if (!virt_viewer_app_start(app, &error)) {
>> - if (g_error_matches(error, VIRT_VIEWER_ERROR,
>> VIRT_VIEWER_ERROR_CANCELLED))
>> - ret = 0;
>> - else if (error) {
>> - virt_viewer_app_simple_message_dialog(app, error->message);
>> - }
>> - goto cleanup;
>> - }
>> -
>> - g_signal_connect(virt_viewer_app_get_session(app), "session-connected",
>> - G_CALLBACK(connected), app);
>> -
>> - gtk_main();
>> -
>> - ret = 0;
>> + goto end;
>>
>> - cleanup:
>> - g_free(uri);
>> - if (viewer)
>> - g_object_unref(viewer);
>> - g_strfreev(args);
>> - g_clear_error(&error);
>> + ret = g_application_run(G_APPLICATION(viewer), argc, argv);
>> + g_object_unref(viewer);
>>
>> +end:
>> return ret;
>> }
>>
>> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
>> index e712d61..2703a24 100644
>> --- a/src/remote-viewer.c
>> +++ b/src/remote-viewer.c
[snip]
>> @@ -634,9 +737,11 @@ remote_viewer_activate(VirtViewerApp *app, GError
>> **error)
>> }
>>
>> static void
>> -remote_viewer_window_added(VirtViewerApp *app,
>> - VirtViewerWindow *win)
>> +remote_viewer_window_added(GtkApplication *app,
>> + GtkWindow *w)
>> {
>> + VirtViewerWindow *win = VIRT_VIEWER_WINDOW(
>> + g_object_get_data(G_OBJECT(w), "virt-viewer
>> -window"));
>
> I really dislike using GObject data this way. If there's no better way to handle
> it, I suppose it's OK, but I'd prefer a different design.
>
>> spice_menu_update(REMOTE_VIEWER(app), win);
>> spice_foreign_menu_update(REMOTE_VIEWER(app), win);
>> }
>> @@ -742,8 +847,10 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED
>> RestProxyAuth *auth,
>> }
>>
>> static void
>> -ovirt_foreign_menu_update(RemoteViewer *app, VirtViewerWindow *win)
>> +ovirt_foreign_menu_update(GtkApplication *gtkapp, GtkWindow *gtkwin,
>> G_GNUC_UNUSED gpointer data)
>> {
>> + RemoteViewer *app = REMOTE_VIEWER(gtkapp);
>> + VirtViewerWindow *win = g_object_get_data(G_OBJECT(gtkwin), "virt-viewer
>> -window");
>
> Same comment about GObject data.
>
It is indeed a workaround, I think the right wait to do it would to make
VirtViewerWindow inherit from GtkApplicationWindow. I did not dedicated
too much time on this idea, because I thought there were too many
changes in the code already. Maybe this could be attacked in a follow up
patch.
>> GtkWidget *menu = g_object_get_data(G_OBJECT(win), "foreign-menu");
>> GtkWidget *submenu;
>> GtkMenuShell *shell =
>> GTK_MENU_SHELL(gtk_builder_get_object(virt_viewer_window_get_builder(win),
>> "top-menu"));
>> @@ -776,8 +883,9 @@ static void
>> ovirt_foreign_menu_update_each(gpointer value,
>> gpointer user_data)
>> {
>> - ovirt_foreign_menu_update(REMOTE_VIEWER(user_data),
>> - VIRT_VIEWER_WINDOW(value));
>> + ovirt_foreign_menu_update(GTK_APPLICATION(user_data),
>> +
>> virt_viewer_window_get_window(VIRT_VIEWER_WINDOW(value)),
>> + NULL);
>> }
>>
>> static void
[snip]
>> @@ -1142,6 +1280,9 @@ retry_dialog:
>> goto cleanup;
>> }
>>
>> + g_signal_connect(virt_viewer_app_get_session(app), "session
>> -connected",
>> + G_CALLBACK(remote_viewer_session_connected), app);
>> +
>
> This session-connected change (as well as the two static functions directly
> above seem unrelated to the GtkApplication change. Can they be moved into a
> separate commit, or are they inter-related somehow?
>
They are related, because with these changes we delay the creation of
the session object. With those two functions in main, we were getting
some warnings noticed in earlier version of this patchset.
>> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
>> index 60157e9..8120272 100644
>> --- a/src/virt-viewer-app.c
>> +++ b/src/virt-viewer-app.c
[snip]
>> @@ -1788,8 +1781,6 @@ virt_viewer_app_init(VirtViewerApp *self)
>> self->priv = GET_PRIVATE(self);
>>
>> gtk_window_set_default_icon_name("virt-viewer");
>> - virt_viewer_app_set_debug(opt_debug);
>> - virt_viewer_app_set_fullscreen(self, opt_fullscreen);
>>
>> self->priv->displays = g_hash_table_new_full(g_direct_hash,
>> g_direct_equal, NULL, g_object_unref);
>> self->priv->config = g_key_file_new();
>> @@ -1805,14 +1796,7 @@ virt_viewer_app_init(VirtViewerApp *self)
>>
>> g_clear_error(&error);
>>
>> - if (opt_zoom < MIN_ZOOM_LEVEL || opt_zoom > MAX_ZOOM_LEVEL) {
>> - g_printerr(_("Zoom level must be within %d-%d\n"), MIN_ZOOM_LEVEL,
>> MAX_ZOOM_LEVEL);
>> - opt_zoom = NORMAL_ZOOM_LEVEL;
>> - }
>> -
>> self->priv->initial_display_map =
>> virt_viewer_app_get_monitor_mapping_for_section(self, "fallback");
>> - self->priv->verbose = opt_verbose;
>> - self->priv->quit_on_disconnect = opt_kiosk ? opt_kiosk_quit : TRUE;
>> g_signal_connect(self, "notify::guest-name",
>> G_CALLBACK(title_maybe_changed), NULL);
>> g_signal_connect(self, "notify::title", G_CALLBACK(title_maybe_changed),
>> NULL);
>> g_signal_connect(self, "notify::guri", G_CALLBACK(title_maybe_changed),
>> NULL);
>> @@ -1871,9 +1855,18 @@ virt_viewer_update_smartcard_accels(VirtViewerApp
>> *self)
>> }
>>
>> static void
>> -virt_viewer_app_constructed(GObject *object)
>> +virt_viewer_app_startup(GApplication *app)
>
> consider renaming this function to something like
> virt_viewer_app_on_application_startup() to prevent confusion with
> virt_viewer_app_start()
>
Done.
>> {
>> - VirtViewerApp *self = VIRT_VIEWER_APP(object);
>> + VirtViewerApp *self = VIRT_VIEWER_APP(app);
>> + GError *error = NULL;
>> +
>> + G_APPLICATION_CLASS(virt_viewer_app_parent_class)->startup(app);
>> +
>> + virt_viewer_app_set_debug(opt_debug);
>> + virt_viewer_app_set_fullscreen(self, opt_fullscreen);
>> +
>> + self->priv->verbose = opt_verbose;
>> + self->priv->quit_on_disconnect = opt_kiosk ? opt_kiosk_quit : TRUE;
>>
>> self->priv->main_window = virt_viewer_app_window_new(self,
>>
>> virt_viewer_app_get_first_monitor(self));
>> @@ -1881,6 +1874,12 @@ virt_viewer_app_constructed(GObject *object)
>>
>> virt_viewer_app_set_kiosk(self, opt_kiosk);
>> virt_viewer_app_set_hotkeys(self, opt_hotkeys);
>> +
>> + if (opt_zoom < MIN_ZOOM_LEVEL || opt_zoom > MAX_ZOOM_LEVEL) {
>> + g_printerr(_("Zoom level must be within %d-%d\n"), MIN_ZOOM_LEVEL,
>> MAX_ZOOM_LEVEL);
>> + opt_zoom = NORMAL_ZOOM_LEVEL;
>> + }
>> +
>> virt_viewer_window_set_zoom_level(self->priv->main_window, opt_zoom);
>>
>> virt_viewer_set_insert_smartcard_accel(self, GDK_F8, GDK_SHIFT_MASK);
>> @@ -1891,25 +1890,82 @@ virt_viewer_app_constructed(GObject *object)
>> gtk_accel_map_add_entry("<virt-viewer>/view/zoom-out", GDK_minus,
>> GDK_CONTROL_MASK);
>> gtk_accel_map_add_entry("<virt-viewer>/view/zoom-in", GDK_plus,
>> GDK_CONTROL_MASK);
>> gtk_accel_map_add_entry("<virt-viewer>/send/secure-attention", GDK_End,
>> GDK_CONTROL_MASK | GDK_MOD1_MASK);
>> +
>> + if (!virt_viewer_app_start(self, &error)) {
>> + if (error && !g_error_matches(error, VIRT_VIEWER_ERROR,
>> VIRT_VIEWER_ERROR_CANCELLED)) {
>> + virt_viewer_app_simple_message_dialog(self, error->message);
>> + }
>> +
>> + g_clear_error(&error);
>> + g_application_quit(app);
>
> I would add a return here since it doesn't really make sense to call
> g_application_hold() after we try to quit (even though it won't have any
> impact).
Sure, Fabiano also suggested this change, so Done.
>
>> + }
>> +
>> + g_application_hold(app);
>> +}
>> +
>> +static gboolean
>> +virt_viewer_app_local_command_line (GApplication *gapp,
>> + gchar ***args,
>> + int *status)
>> +{
>> + VirtViewerApp *self = VIRT_VIEWER_APP(gapp);
>> + gboolean ret = FALSE;
>> + gint argc = g_strv_length(*args);
>> + GError *error = NULL;
>> + GOptionContext *context = g_option_context_new(NULL);
>> + GOptionGroup *group = g_option_group_new("virt-viewer", NULL, NULL, NULL,
>> NULL);
>> +
>> + *status = 0;
>> + g_option_context_set_main_group(context, group);
>> + VIRT_VIEWER_APP_GET_CLASS(self)->add_option_entries(self, context,
>> group);
>> +
>> + 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
>> +
>> + if (!g_option_context_parse(context, &argc, args, &error))
>> + {
>> + if (error && !g_error_matches(error, VIRT_VIEWER_ERROR,
>> VIRT_VIEWER_VERSION)) {
>> + g_printerr(_("%s\n"), error->message);
>> + *status = 1;
>
> I don't like this approach. A version is not an error. It should set a status of
> 0 (not 1), but should return TRUE to indicate that we've fully handled the
> commandline and the application should exit. Also, I don't see any reason that
> the version handling couldn't be done in the common VirtViewerApp class. I don't
> think there's any problem with printing the OS ID in the virt-viewer version
> output.
>
This will make stuff a lot easier from my side. Again, it was workaround
I put in place to deal with different outputs according to the program
running.
>> diff --git a/src/virt-viewer-util.h b/src/virt-viewer-util.h
>> index f1cb08b..a9496a4 100644
>> --- a/src/virt-viewer-util.h
>> +++ b/src/virt-viewer-util.h
>> @@ -31,6 +31,7 @@ extern gboolean doDebug;
>> enum {
>> VIRT_VIEWER_ERROR_FAILED,
>> VIRT_VIEWER_ERROR_CANCELLED,
>> + VIRT_VIEWER_VERSION,
>
> As mentioned above, this is not an error, so I don't think it should be handled
> this way.
>
I will fix it. Thanks a lot for the review.
>
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
>
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com
More information about the virt-tools-list
mailing list