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 <spice-controller.h>
>> -#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*
>> +remote_viewer_version_string(void)
>> +{
>> +#ifdef REMOTE_VIEWER_OS_ID
>> + return " (OS ID: " REMOTE_VIEWER_OS_ID ")";
>> +#endif
>> + return "";
>> +}
>> +
>
> This function seems a little strange. It doesn't really match the name of the
> function. It just returns an OSID rather than a version string.
>
>
It was a way to add OSID to the version string, and prior to this patch,
it is shown only if we are running remote-viewer. I wanted to keep the
behavior as the same as before the patch. If I move the #ifdef block
back to virt-viewer-app, virt-viewer will also show the string.
>> +static const char OPT_TITLE[] = "title";
>> +static const char OPT_CONTROLLER[] = "spice-controller";
>
>
> I'd use
> static const char *VARNAME
>
> rather than
> static const char VARNAME[]
>
>
> However, see my comments about handle_local_options() below. Depending on what
> we do there, we may not need to define constant variables for these option
> names.
>
>
>> +
>> +static void
>> +remote_viewer_add_main_options(VirtViewerApp *self)
>> +{
>> + GApplication *app = G_APPLICATION (self);
>> + const GOptionEntry options [] = {
>
> I know this is just moved code, but perhaps it's worthwhile to fix the coding
> style to remove the spaces before the parentheses and brackets:
>
> G_APPLICATION(self)
> 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);
>> +
>> +#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);
>> + g_object_set(app, "title", title, NULL);
>> + }
>> +
>> +end:
>> + g_strfreev(args);
>> + g_free(title);
>> + return ret;
>> +}
>
>
> 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.
Yes, that was intentional, I could not find a way to control the flow so
that the function on the parent class is called first and also can
access the return value of the one on the child class.
>
> 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.
> 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. 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 g_option_context_parse() in a command-line handler.
>
> I admit that GApplication is designed to enable a slightly more complicated
> approach than we want here (a single-instance unique application), and I don't
> have extensive experience with using this class, but I get the impression that
> handle-local-options is not intended to be a signal that you normally need to
> handle. Is there a reason you chose this signal/vfunc over 'command-line'?
>
The main reason for using it rather than command-line is because
'handle-local-options' is the one called earlier in the process. So if
we receive any invalid option or another one that would make the process
exit, the earlier, the better.
>> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
>> index 653b30c..adabb27 100644
>> --- a/src/virt-viewer-app.c
>> +++ b/src/virt-viewer-app.c
>> @@ -32,6 +32,7 @@
>> #include <string.h>
>> #include <unistd.h>
>> #include <locale.h>
>> +#include <gio/gio.h>
>> #include <glib/gprintf.h>
>> #include <glib/gi18n.h>
>>
>> @@ -60,9 +61,11 @@
>> #endif
>> #ifdef HAVE_SPICE_GTK
>> #include "virt-viewer-session-spice.h"
>> +#include <spice-client.h>
>
> This change is probably not directly related to the GtkApplication stuff so
> could possibly be in a separate commit?
>
Ok.
>> @@ -926,12 +927,11 @@ virt_viewer_app_window_new(VirtViewerApp *self, gint
>> nth)
>> virt_viewer_session_get_has_usbredir(self->priv
>> ->session));
>> }
>>
>> - g_signal_emit(self, signals[SIGNAL_WINDOW_ADDED], 0, window);
>> -
>> if (self->priv->fullscreen)
>> app_window_try_fullscreen(self, window, nth);
>>
>> w = virt_viewer_window_get_window(window);
>> + gtk_application_add_window(GTK_APPLICATION(self), w);
>
> This is a slight change in behavior. It will now emit the window-added signal
> after the call to app_window_try_fullscreen() rather than before. Not sure
> whether this is important, just noting it. Perhaps move this where the original
> signal was emitted?
>
Good catch, it was not intentional change. I will check for results with
try_fullscreen after the signal emission.
>> @@ -1479,7 +1477,7 @@ virt_viewer_app_disconnected(VirtViewerSession *session
>> G_GNUC_UNUSED, const gch
>> virt_viewer_app_hide_all_windows(self);
>>
>> if (priv->quitting)
>> - gtk_main_quit();
>> + g_application_quit(G_APPLICATION(self));
>
>
> I'd need to test to make sure, but I think it's possible that the call to
> virt_viewer_app_hide_all_windows() a few lines above might result in the
> application quitting because the application no longer has any associated
> windows. In other words, even if priv->quitting is false, the application might
> quit here. I could be wrong though. Have you tested this path?
Another one I need to check. Thanks.
>
>
>> @@ -1851,13 +1837,17 @@ static void
>> virt_viewer_app_constructed(GObject *object)
>> {
>> VirtViewerApp *self = VIRT_VIEWER_APP(object);
>> + virt_viewer_app_add_main_options(self);
>> +}
>> +
>> +static void
>> +virt_viewer_app_startup_cb(GApplication *app, G_GNUC_UNUSED gpointer data)
>> +{
>> + VirtViewerApp *self = VIRT_VIEWER_APP(app);
>>
>> self->priv->main_window = virt_viewer_app_window_new(self,
>>
>> virt_viewer_app_get_first_monitor(self));
>> self->priv->main_notebook =
>> GTK_WIDGET(virt_viewer_window_get_notebook(self->priv->main_window));
>> -
>> - virt_viewer_app_set_kiosk(self, opt_kiosk);
>> - virt_viewer_app_set_hotkeys(self, opt_hotkeys);
>> virt_viewer_window_set_zoom_level(self->priv->main_window, opt_zoom);
>>
>> virt_viewer_set_insert_smartcard_accel(self, GDK_F8, GDK_SHIFT_MASK);
>> @@ -1871,9 +1861,26 @@ virt_viewer_app_constructed(GObject *object)
>> }
>
> Do we really need a separate 'startup' handler? Or can we move stuff from here
> into either the "handle-local-options" or "activate" handlers? (or to "command
> -line" if we choose that route)
Yes it is necessary, handle-local-options happens too early in the
process. But one thing it is not completely clear for me it the
difference between providing a signal handler and overriding the vfunc.
In this specific case, when I override, the application crashes, while
with the signal handler it works fine.
>> @@ -2536,48 +2523,122 @@ virt_viewer_app_show_preferences(VirtViewerApp *self,
>> GtkWidget *parent)
>> }
>>
>> static gboolean
>> -option_kiosk_quit(G_GNUC_UNUSED const gchar *option_name,
>> - const gchar *value,
>> - G_GNUC_UNUSED gpointer data, GError **error)
>> +virt_viewer_app_option_kiosk_quit(VirtViewerApp *self, const gchar *value)
>> {
>> - if (g_str_equal(value, "never")) {
>> - opt_kiosk_quit = FALSE;
>> + if (!g_strcmp0(value, "never")) {
>
> I'd prefer an explicit ' == 0' here and in the following test
>
Ok.
>
>> + self->priv->quit_on_disconnect = FALSE;
>> return TRUE;
>> }
>> - if (g_str_equal(value, "on-disconnect")) {
>> - opt_kiosk_quit = TRUE;
>> + if (!g_strcmp0(value, "on-disconnect")) {
>> + self->priv->quit_on_disconnect = TRUE;
>> return TRUE;
>> }
>>
>> - g_set_error(error, G_OPTION_ERROR, G_OPTION_ERROR_FAILED, _("Invalid
>> kiosk-quit argument: %s"), value);
>> + g_printerr(_("Invalid kiosk-quit argument: %s\n\n"), value);
>> +
>> return FALSE;
>> }
>>
>> -GOptionGroup*
>> -virt_viewer_app_get_option_group(void)
>> +static const char OPT_VERSION[] = "version";
>> +static const char OPT_ZOOM[] = "zoom";
>> +static const char OPT_FULLSCREEN[] = "full-screen";
>> +static const char OPT_HOTKEYS[] = "hotkeys";
>> +static const char OPT_KIOSK[] = "kiosk";
>> +static const char OPT_KIOSK_QUIT[] = "kiosk-quit";
>> +static const char OPT_VERBOSE[] = "verbose";
>> +static const char OPT_DEBUG[] = "debug";
>
> Again, I'd prefer string declarations (char*) rather than array declarations
> here if we need these variables.
>
The difference between those two is that using explicit arrays one will
use slight less memory than pointers. See:
http://c-faq.com/decl/strlitinit.html
http://c-faq.com/aryptr/aryptr2.html
>
>> +
>> +static void
>> +virt_viewer_app_add_main_options(VirtViewerApp *self)
>> {
>> - static const GOptionEntry options [] = {
>> - { "zoom", 'z', 0, G_OPTION_ARG_INT, &opt_zoom,
>> + GApplication *app = G_APPLICATION (self);
>> + VirtViewerAppClass *app_class = VIRT_VIEWER_APP_GET_CLASS(self);
>> +
>> + static const GOptionEntry virt_viewer_app_options [] = {
>> + { OPT_VERSION, 'V', 0, G_OPTION_ARG_NONE, NULL,
>> + N_("Display version information"), NULL },
>> + { OPT_ZOOM, 'z', 0, G_OPTION_ARG_INT, NULL,
>> N_("Zoom level of window, in percentage"), "ZOOM" },
>> - { "full-screen", 'f', 0, G_OPTION_ARG_NONE, &opt_fullscreen,
>> + { OPT_FULLSCREEN, 'f', 0, G_OPTION_ARG_NONE, NULL,
>> N_("Open in full screen mode (adjusts guest resolution to fit the
>> client)"), NULL },
>> - { "hotkeys", 'H', 0, G_OPTION_ARG_STRING, &opt_hotkeys,
>> + { OPT_HOTKEYS, 'H', 0, G_OPTION_ARG_STRING, NULL,
>> N_("Customise hotkeys"), NULL },
>> - { "kiosk", 'k', 0, G_OPTION_ARG_NONE, &opt_kiosk,
>> + { OPT_KIOSK, 'k', 0, G_OPTION_ARG_NONE, NULL,
>> N_("Enable kiosk mode"), NULL },
>> - { "kiosk-quit", '\0', 0, G_OPTION_ARG_CALLBACK, option_kiosk_quit,
>> + { OPT_KIOSK_QUIT, '\0', 0, G_OPTION_ARG_STRING, NULL,
>> N_("Quit on given condition in kiosk mode"), N_("<never|on
>> -disconnect>") },
>> - { "verbose", 'v', 0, G_OPTION_ARG_NONE, &opt_verbose,
>> + { OPT_VERBOSE, 'v', 0, G_OPTION_ARG_NONE, NULL,
>> N_("Display verbose information"), NULL },
>> - { "debug", '\0', 0, G_OPTION_ARG_NONE, &opt_debug,
>> + { OPT_DEBUG, '\0', 0, G_OPTION_ARG_NONE, NULL,
>> N_("Display debugging information"), NULL },
>> - { NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }
>> + { NULL },
>> };
>> - GOptionGroup *group;
>> - group = g_option_group_new("virt-viewer", NULL, NULL, NULL, NULL);
>> - g_option_group_add_entries(group, options);
>>
>> - return group;
>> + g_application_add_main_option_entries(app, virt_viewer_app_options);
>> + app_class->add_main_options(self);
>> +
>> +#ifdef HAVE_GTK_VNC
>> + g_application_add_option_group(app, vnc_display_get_option_group());
>> +#endif
>> +#ifdef HAVE_SPICE_GTK
>> + g_application_add_option_group(app, spice_get_option_group());
>> +#endif
>> +}
>> +
>> +static gint
>> +virt_viewer_app_handle_local_options(GApplication *app,
>> + GVariantDict *options)
>> +{
>> + VirtViewerApp *self = VIRT_VIEWER_APP(app);
>> + VirtViewerAppClass *app_class = VIRT_VIEWER_APP_GET_CLASS(self);
>> + gchar *opt_hotkeys = NULL;
>> + gint ret = -1;
>> +
>> + /* Finish program execution if version is supplied. */
>> + if (g_variant_dict_contains(options, OPT_VERSION)) {
>> + gchar *version_str = g_strdup_printf(_("%s version %s%s\n"),
>> + g_get_prgname(),
>> + VERSION BUILDID,
>> + app_class->version_string ?
>> app_class->version_string() : "");
>> + g_print(version_str);
>> + g_free(version_str);
>> + return 0;
>> + }
>> +
>> + virt_viewer_app_set_fullscreen(self, g_variant_dict_contains(options,
>> OPT_FULLSCREEN));
>> + virt_viewer_app_set_debug(g_variant_dict_contains(options, OPT_DEBUG));
>> + self->priv->verbose = g_variant_dict_contains(options, OPT_VERBOSE);
>> +
>> + if (g_variant_dict_contains(options, OPT_KIOSK)) {
>> + gchar *opt_kiosk_quit = NULL;
>> +
>> + virt_viewer_app_set_kiosk(self, TRUE);
>> + self->priv->quit_on_disconnect = TRUE;
>> +
>> + if (g_variant_dict_lookup(options, OPT_KIOSK_QUIT, "s",
>> &opt_kiosk_quit) &&
>> + !virt_viewer_app_option_kiosk_quit(self, opt_kiosk_quit)) {
>> + ret = 0;
>> + goto end;
>> + }
>> + }
>> +
>> + if (g_variant_dict_lookup(options, OPT_ZOOM, "i", &opt_zoom)) {
>> + 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;
>> + }
>> + }
>> +
>> + if (g_variant_dict_lookup(options, OPT_HOTKEYS, "s", &opt_hotkeys))
>> + virt_viewer_app_set_hotkeys(self, opt_hotkeys);
>> +
>> + ret = app_class->handle_options(self, options);
>> +
>> +end:
>> + if (!ret)
>> + g_printerr(_("Run with --help to see a full list of available command
>> line options\n\n"));
>> +
>> + return ret;
>> }
>
>
> Same comment here about handle-local-options vs. command-line
>
Answered above.
>
>>
>> gboolean virt_viewer_app_get_session_cancelled(VirtViewerApp *self)
>> diff --git a/src/virt-viewer-app.h b/src/virt-viewer-app.h
>> index bbbc9b4..3e47d08 100644
>> --- a/src/virt-viewer-app.h
>> +++ b/src/virt-viewer-app.h
>> @@ -24,6 +24,7 @@
>> #define VIRT_VIEWER_APP_H
>>
>> #include <glib-object.h>
>> +#include <gtk/gtk.h>
>> #include "virt-viewer-util.h"
>> #include "virt-viewer-window.h"
>>
>> @@ -39,16 +40,15 @@ G_BEGIN_DECLS
>> typedef struct _VirtViewerAppPrivate VirtViewerAppPrivate;
>>
>> typedef struct {
>> - GObject parent;
>> + GtkApplication parent;
>> VirtViewerAppPrivate *priv;
>> } VirtViewerApp;
>>
>> typedef struct {
>> - GObjectClass parent_class;
>> + GtkApplicationClass parent_class;
>>
>> /* signals */
>> void (*window_added) (VirtViewerApp *self, VirtViewerWindow *window);
>> - void (*window_removed) (VirtViewerApp *self, VirtViewerWindow *window);
>
> I think we need to remove the 'window_added' vfunc here as well, since we
> removed the signal that was associated with this vfunc.
This was a left over, thanks for noticing.
>> @@ -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";
>
>
> string vs. array again.
Same as above.
>
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>
V2 on the way. Thank you for the review.
--
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
