On Mon, Nov 30, 2015 at 03:29:37PM -0600, Jonathon Jongsma wrote:
> > + app_class->add_main_options = remote_viewer_add_main_options;
> > + 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));
> > }
>
>
> 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 these properties could be either removed or made read-only as we
now set them from inside the class if I'm not mistaken.
> > + { 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.
Agreed, these vfunc could hopefully be improved.
>
> 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.
My understanding is that this is optional. If GOptionEntry::arg_data is
NULL, then the argument will appear in the GVariantDict. If it's not
NULL, then the GOptionEntry::arg_data will be used.
> 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.
Kind of. If you add a new option, you would add whatever variable you
need to set in your GOptionEntry, then you'd have to do something with
this variable in your code. That code would now go in
handle_local_options() rather than in an arbitary place. If you want to
avoid having to look at the GVariantDict to get the option value, you
can.
> 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'?
In general (when you have a single process model), you need to handle
both. Options like --version, --help need to be handled in your local
process (the one you just spawned), some other options are better
handled in the main instance (the long-running process which may have
been started before). For example files to open in a text editor. So in
the usual GApplication use-case, I'd say you'd implement both.
In our case, we don't have a single process model, so we can handle our
options in either places. My reading of
https://developer.gnome.org/gio/unstable/GApplication.html#g-application-add-main-option-entries
however is that it's better to use handle-local-options (or some
variation of it):
« In general, it is recommended that all commandline arguments are
parsed locally. [..] For local options, it is possible
to either use arg_data in the usual way, or to consult (and potentially
remove) the option from the options dictionary. »
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
