On 12/02/2015 09:21 AM, Christophe Fergeau wrote: > 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. Good idea. I will take a look at it. >> >> >> 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. As I answered in my previous email, I had a bit different flow in my mind, but I can rework the logic to use the existing vfuncs. > >> >> 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 > Thanks a lot for the explanation about handle-local-options Christophe. :) -- 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
