[virt-tools-list] [PATCH 3/4] Port to GtkApplication API's
Eduardo Lima (Etrunko)
etrunko at redhat.com
Thu Dec 3 14:14:14 UTC 2015
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 at redhat.com
More information about the virt-tools-list
mailing list