GNOME Bugzilla – Bug 721977
improve split handling of command line arguments
Last modified: 2014-02-17 23:29:18 UTC
After playing around with GOptionContext vs. GApplication for a good part of today I've realised that it's really not possible to have a non-trivial set of options on both the local side (for --version, etc.) and the remote side without a great deal of pain. I think I have a good solution to that: We want to add a new flag: G_OPTION_FLAG_PASSTHROUGH This option is accepted, checked for validity, and included in --help output in the usual way, but when encountered, instead of being processed in the usual way (by having its value written to the @arg_data of #GOptionEntry) it is simply left in the argument list. This is useful if the argument list will be processed again by other parsers or by future uses of #GOptionContext. And then some new API: g_option_context_add_main_passthrough_entries(): This function is the same as g_option_context_add_main_entries() with the exception that each entry has its flags modified by the addition of the G_OPTION_FLAG_PASSTHROUGH flag. This is useful for reusing the same array of GOptionEntry structs with two different uses of #GOptionContext (such as from the local and then the primary instance of a #GApplication). and: g_option_context_add_passthrough_group(): This function is the same as g_option_context_add_group() with the exception that the group is treated as if each entry had G_OPTION_FLAG_PASSTHROUGH set on it. This is useful for allowing arguments from an option group that you intend to use with a future use of #GOptionContext to be passed through to it. The idea is that an application could then split their arguments into two parts: those intended to be handled locally and those intended to be handled remotely. Let's take a simple example of an app that wants to accept these arguments: --help --version The usual, handled in the local instance. --edit Start the app in edit mode (possibly with some files), handled in the primary instance. and assume that it wants to use GApplicationCommandLine for some reason or other. now we can do: static gboolean show_version; static gboolean edit_mode; static GOptionEntry local_options[] = { { "version", ..., &show_version, ... }, { NULL } }; static GOptionEntry primary_options[] = { { "edit", ..., &edit_mode, ... }, { NULL } }; static gboolean myapp_local_command_line (GApplication *application, gchar ***arguments, gint *exit_status) { gboolean handled = FALSE; GOptionContext *context; GError *error = NULL; context = g_option_context_new (NULL); g_option_context_add_main_entries (context, local_options, GETTEXT_PACKAGE); g_option_context_add_main_passthrough_entries (context, primary_options, GETTEXT_PACKAGE); if (!g_option_context_parse_strv (context, arguments, &error)) { g_printerr (_("%s\nUse '%s --help' for help.\n"), error->message, (*arguments)[0]); g_error_free (error); *exit_status = 1; handled = TRUE; } else if (show_version) { g_print ("Version %s\n", VERSION); *exit_status = 0; handled = TRUE; } g_option_context_free (context); if (handled) return TRUE; return G_APPLICATION_CLASS (myapp_parent_class) ->local_command_line (application, arguments, exit_status); } and then: static void my_app_command_line (GApplication *application, GApplicationCommandLine *cmdline) { gboolean handled = FALSE; GOptionContext *context; GError *error = NULL; gchar **args; gint i; context = g_option_context_new (NULL); args = g_application_command_line_get_arguments (cmdline, NULL); g_option_context_add_main_entries (context, primary_options, GETTEXT_PACKAGE); if (!g_option_context_parse_strv (context, &args, &error)) { ... should not happen because arguments are checked on the other side ... g_application_command)line_printerr (cmdline, "unexpected error: %s", error->message); g_application_command_line_set_exit_status (cmdline, 1); g_error_free (error); return; } if (edit_mode) { ... } /* process remaining non-option arguments as filenames */ for (i = 0; args[i]; i++) { file = g_application_command_line_create_file_for_arg (cmdline, args[i]); ... } } We could also add an option group for GApplication designed to be used for passthrough so that the --gapplication-service flag (and future flags we add) get properly passed through to the chain-up handler (as well as getting properly documented in --help). We might even use GOptionContext ourselves from the default handler... All of this would put us into a pretty good state: - --help would work locally, including all arguments (even those processed on the remote side) - we could handle some arguments locally - we could reject arguments that the remote side won't understand from the local side (since we use the same option entries) - we can properly handle arguments that take particular types of values and even verify ahead of time that things like integers have been given correctly Something to think about is to make the 'passthrough' arguments update the state of the arg_data anyway since it might be useful in some cases to know about a flag locally in order to modify the behaviour but also to pass it through. 'gedit --wait' is an interesting example of this because it causes the local_command_line handling to want to change the flags set on the application but is also needed from 'command_line' to know that it needs to keep the remote instance alive. The argument against this is that this is not a common case and that it would require the 'passthrough' user of arguments to clear out their arg_data values in order not to leak them (when meanwhile it doesn't care about the vast majority of them).
Another alternative comes to mind: it may be possible to play a particular set of action entries 'in reverse' in order to rebuild an argument list out of them that could be forwarded to the primary instance for processing. I consider this to be pretty crufty, and I don't like it... but it's theoretically possible.
Or maybe g_option_group_set_passthrough() or g_option_group_add_flags() ?
I have a new idea here, but there's one annoying little detail unsolved. Patches incoming anyway.
Created attachment 266123 [details] [review] Add g_application_register_or_exit() This calls g_application_register(). If it fails, it prints the reason to stderr and calls exit() with a non-zero status. It's unclear what else anyone could want to do in this case, so we may as well provide an API that does the obvious thing. This will make it easier for people to implement their own version of local_command_line without having to chain-up. We can use it internally in a couple of places as well.
Created attachment 266124 [details] [review] Add g_application_command_line() This is really the same sort of function as g_application_activate() and g_application_open(): it emits a command-line signal int he primary instance of a registered GApplication. It's a bit of a historical oddity that we ever decided to deal with this case in a special way (ie: by local_command_line returning FALSE). Let's modernise this a bit (but keep the fallback code there for compatibility). In addition to the usual command line arguments, we also support passing a GVariant a{sv} with "extra data" to the primary instance. If command line argument parsing is done on the local side the idea is that the result of that will be sent via GVariant to the primary instance which can access the values directly instead of using GOptionContext again.
Created attachment 266125 [details] [review] Clean up g_application_real_local_command_line() This function has had a few too many features crufted into it over time. Clean it up a bit. - move the processing of --gapplication-service out of here and into g_application_run() where we look for the argument before calling local_command_line(). This means that applications no longer have to deal with this argument (or allow their GOptionContext to ignore it) - simplify the conditional mess that has grown over time - use g_application_command_line() directly and always return %TRUE The logic reads a lot easier now, and is as follows: - register - if we're a service, ensure we have no arguments and return - else, if we have _HANDLES_COMMAND_LINE, call _command_line() - else, if there are no arguments, activate() - else, open() a list of files (or error out if we don't support that)
Created attachment 266126 [details] [review] GOption: add some GOptionEntry utility functions Add a function to clean up the values that are left in GOptionEntry arg_data pointers after we're done with them. More importantly, add a pair of functions to convert the values in the arg_data of a GOptionEntry array to a GVariant and back again. This GVariant can be sent with the new 'extra_data' of GApplication command line invocations.
Created attachment 266127 [details] [review] app: only parse commandline arguments once Use new GLib functionality to do the parsing only from local_command_line and then ship the results across D-Bus to the primary instance. This removes the need to parse twice.
The only problem remaining here is that the handling of --gapplication-service is still not very nice. If this is given, we set the IS_SERVICE flag and then call into local_command_line(). Most people overriding this will not take --gapplication-service mode into explicit account and they will do things like inspecting the argument list, noticing no arguments, and calling activate() or command_line(), which is not the right thing to do in service mode. My main idea for dealing with this is a separate service_command_line() function that we call instead which defaults to rejecting all arguments. Getting the API of this to 'feel right' is proving to be tricky, though -- I guess probably we want to return TRUE to mean "already handled" just like the local_command_line() case, where "already handled" typically means "OK. Exit now." and the %FALSE case probably should mean "register and start the 10 second wait for the incoming message". The tricky part is that the user's normal local_command_line() function could decide to turn on service mode itself. Do we re-run the service_command_line() function in this case? Would we do that from the local_command_line chainup, or after it returns? It's tricky, and I've yet to find a solution that I am completely happy with...
One more thing worth mentioning: the attached gedit patch is a small functionality change: any commandline arguments with gtk or gobject introspection options will not be forwarded, which is a behaviour change. At least for Gtk (and maybe for gobject-introspection) this is actually no change at all, though. That's because by the time the command line arguments arrive, gtk_init() has already been called in the primary instance. We might decide to be 'more thorough' here by serialising the entire content of a GOptionContext and deserialising it back into a new GOptionContext (with all the groups) on the receiving side, but I don't think this is really necessary.
Review of attachment 266123 [details] [review]: Is it something normal apps will do often? I do not recall calling g_application_register in any of my apps... if it is something one uses seldomly I would not bother making this function public (internally the refactoring looks ok)
Review of attachment 266127 [details] [review]: ::: gedit/gedit-app.c @@ -812,3 @@ - new_document = FALSE; - geometry = NULL; - wait = FALSE; at least "wait" is still reaching the server side, so we still need to reset it otherwise the next time is still set to true. For the others it may be worth adding a comment where they are declared specifying they are only used in the local instance
Review of attachment 266127 [details] [review]: ::: gedit/gedit-app.c @@ -812,3 @@ - new_document = FALSE; - geometry = NULL; - wait = FALSE; g_option_entries_cleanup() takes care of this now
Review of attachment 266125 [details] [review]: ::: gio/gapplication.h @@ +175,3 @@ +GLIB_AVAILABLE_IN_2_40 +void g_application_send_option_entries (GApplication *application, + GOptionEntry *entries); Spurious addition ?
Comment on attachment 266126 [details] [review] GOption: add some GOptionEntry utility functions Why is this API taking GOptionEntry's instead of GOptionGroup or GOptionContext? Is there a case where one would want to reset the entries of only one group instead of the whole context?
Comment on attachment 266123 [details] [review] Add g_application_register_or_exit() I don't really see the need for this either. In g-t I do if (!register (...)) goto out; but I don't like APIs that call exit() behind your back, because you don't get a chance to do cleanup.
Review of attachment 266124 [details] [review]: This makes sense to me. It adds a nice symmetry with the other entry points.
Review of attachment 266126 [details] [review]: How would I use this e.g. with gtk_get_option_group ?
Created attachment 267544 [details] [review] Add g_application_command_line() This is really the same sort of function as g_application_activate() and g_application_open(): it emits a command-line signal int he primary instance of a registered GApplication. It's a bit of a historical oddity that we ever decided to deal with this case in a special way (ie: by local_command_line returning FALSE). Let's modernise this a bit (but keep the fallback code there for compatibility). In addition to the usual command line arguments, we also support passing a GVariantDict with "options" to the primary instance. If command line argument parsing is done on the local side the idea is that the result of that will be sent via GVariant to the primary instance which can access the values directly instead of using GOptionContext again.
Created attachment 267545 [details] [review] GApplication: parse command line options Add support for parsing command line options with GApplication. You can add GOptionGroup and GOptionEntry using two new APIs: g_application_add_option_group() and g_application_add_main_option_entries(). Also add a "handle-local-options" signal that allows handling of commandline arguments in the local process without having to override local_command_line. As a special feature, you can have a %NULL @arg_data in a GOptionEntry which will cause the argument to be stored in a GVariantDict. This dictionary is available for inspection and modification by the "handle-local-options" signal and can be forwarded to the primary instance in cases of command line invocation (where it can be fetched using g_application_command_line_get_options()).
Created attachment 267546 [details] [review] gedit: Clean up command line handling This adjusts to the new "handle-local-options" API in GApplication and removes command line handling from the primary instance.
Changes in this patch set and general summary: - depends on the GVariantDict patches in bug 625408 - register_or_exit() is gone for the reasons that chpe stated - command_line() now takes a GVariantDict instead of GVariant. There's no super-strong reason for that other than avoiding a round-trip from GVariantDict to GVariant back to GVariantDict in the 'local' case. I may change this again because I'm not sure it's worth it (particularly when you consider that it will be a tree-form GVariant that we go through). - we now aim to avoid anyone from ever needing to override local_command_line in any reasonable situation, favouring the new "handle-local-options" signal instead (which should be nicely bindable and usable from C without making a subclass) - --gapplication-service handling is unified through GOptionContext with the rest of the application options. --help works very nicely now. - we will reject unknown commandline arguments as long as the user has called _add_main_option_entries() at least once. This lets us continue to ignore unrecognised commandline options (passing them over to the primary instance) in order to remain backwards compatible with existing usage. - needs docs, including an update to the docs of local_command_line advising people to avoid using it - the weird magic return value of -1 on the signal is because signals cannot return-via-reference. We can therefore not do the TRUE/FALSE pattern with separate exit_status variable. pbor suggested a set_exit_status() function that can be called in case we want a non-zero status, in which case we could go back to using a bool. I don't like the global-state nature of that, but in practice it would likely be rarely used: failing to parse the arguments is already dealt with by GApplication internally and a successful handling of a local option like --version or --list-encodings will typically want to return 0. gedit notes: - there is more room for cleanup. I want to remove the global variables entirely and convert more things to action invocations instead. This patch was made to do the port as directly as possible and the rest can be done in a future patch.
One more thing: there has been a lot of refactoring done and undone and there are still some undebugged crashes here as a result. I'm mostly looking for API feedback at the moment.
Review of attachment 267545 [details] [review]: I see g_application_add_option_group added to the headers and mentioned in the docs...but no implementation.
Review of attachment 267544 [details] [review]: ::: gio/gapplicationcommandline.c @@ +403,3 @@ + * g_application_command_line_get_options: + * @cmdline: a #GApplicationCommandLine + * In the header, the function that is added is called g_application_command_line_get_extra_data - what gives ?
Created attachment 267577 [details] [review] Add g_application_command_line() This is really the same sort of function as g_application_activate() and g_application_open(): it emits a command-line signal int he primary instance of a registered GApplication. It's a bit of a historical oddity that we ever decided to deal with this case in a special way (ie: by local_command_line returning FALSE). Let's modernise this a bit (but keep the fallback code there for compatibility). In addition to the usual command line arguments, we also support passing a GVariantDict with "options" to the primary instance. If command line argument parsing is done on the local side the idea is that the result of that will be sent via GVariant to the primary instance which can access the values directly instead of using GOptionContext again.
Created attachment 267578 [details] [review] GApplication: parse command line options Add support for parsing command line options with GApplication. You can add GOptionGroup and GOptionEntry using two new APIs: g_application_add_option_group() and g_application_add_main_option_entries(). Also add a "handle-local-options" signal that allows handling of commandline arguments in the local process without having to override local_command_line. As a special feature, you can have a %NULL @arg_data in a GOptionEntry which will cause the argument to be stored in a GVariantDict. This dictionary is available for inspection and modification by the "handle-local-options" signal and can be forwarded to the primary instance in cases of command line invocation (where it can be fetched using g_application_command_line_get_options()).
Sorry -- rebase disaster.
Still not quite recovered from the disaster, it seems. 1. I don't see become_service being used anywhere 2. The option group that is set with g_application_add_option_group doesn't get any treatment that would cause its options to make it to the other side. 3. We also seem to have lost the "--gapplication-service must be the sole option given" ?
also, missing docs for some of the new api
(In reply to comment #31) > Still not quite recovered from the disaster, it seems. (In reply to comment #32) > also, missing docs for some of the new api Ya.... was mostly looking for conceptual review at this point. > 1. I don't see become_service being used anywhere That's a bug. I should be setting the service flag in that case. Will fix. > 2. The option group that is set with g_application_add_option_group doesn't get > any treatment that would cause its options to make it to the other side. This is true and also intentional. By the time it gets to the other side, startup() will already have been called and it will be too late to do anything useful with them. Basically, these arguments are only useful in the new hybrid/split mode that we seem to be consolidating around. In the normal case (shell launching and opening files) we will D-Bus activate with no special arguments. This corresponds to existing usage. If someone wants to run from the commandline and enable debugging output or set some gtk flags then they can do that (and then in that case it won't go the D-Bus route). The flags for gtk are really only useful in the case that the local instance becomes the primary instance. In the case that the local instance just sends the message and dies then Gtk will be never initialised, and in the case that we're starting for the second time it will be too late. This could be better documented. > 3. We also seem to have lost the "--gapplication-service must be the sole > option given" ? Yes -- and that's intentional. Now that we have proper commandline parsing, there is no reason that we shouldn't be able to start the service with some extra options (debugging, gtk options, etc). So, meaningful: --gapplication-service --g-fatal-warnings This is something that has given me a bit of pause however. For example, this is of questionable usefulness: --gapplication-service --version and this is completely pointless: --gapplication-service --new-window What will happen in those cases right now is that in the first case the version will be printed and in the second case nothing special will happen (because we will drop the GVariantDict on the floor). I considered the possibility of disabling handling of the 'main' options in the case of service mode and maybe not calling the user's handle-local-options function but it seems like this could cause issues in the case that the application has its own debug options that it might want to use in service mode. I also considered a separate handle-service-options signal that we would call instead of the normal one in case we find ourselves in service mode at the end of the parsing (after dealing with --gapplication-service). In short: this is very tricky. The current solution has some nice properties about it, however: - it avoids surgery to GOptionContext that we'd have to perform in order to explain to it that "this option means that you can't use any other options". This would be necessary because we're already mid-parse by the time we see --gapplication-service, so it's to late to avoid adding the other option entries depending on if we see it or not. - we don't dismiss the idea that the user may have useful flags that they want to deal with, even in service mode - the pointless cases are just that: pointless. I think that's better than creating cases where it's impossible to have certain functionality. - looking around and seeing the sort of things that people tend to handle in local_command_line() it seems mostly like various flags are picked out for special treatment (typically with a quick exit) and -1 will be returned for the other cases. If -1 gets returned, then we do our default handling which will be right in all cases. - overriding local_command_line() is still available to the 'truly weird' cases The main reservation: In the case that more exotic things are desired (for example, if gedit wanted to move its special parsing of +LINE:COLUMN to the local side and send that as an action invocation (interleaved with 'open' calls) then this would involve inspecting the command line in the local handler, including the case of sending 'Activate' when there are no arguments. In the service case, Activate is obviously inappropriate. I expect that the handler would have to check if it is in service mode before doing the activation. Alternatively, the handler could deal with the 'no options' case by returning -1 knowing that they will get the activate if it's appropriate. If there should be arguments given to the service, they will be interpreted in the usual way (ie: they will cause files to be opened, even in service mode). I'm not sure what you'd expect with passing filenames to service mode, though... That's more or less the crux of what took me so long with this patch. I think I'm happy(ish) now, but I do welcome criticism of this approach and am open to other ideas.
Review of attachment 267577 [details] [review]: ::: gio/gapplication.c @@ +257,3 @@ GNotificationBackend *notifications; + + GOptionContext *option_context; Only needed in the next patch. ::: gio/gapplicationcommandline.c @@ +409,3 @@ + * options that were parsed according to the #GOptionEntrys added to the + * application with g_application_add_main_option_entries() and possibly + * modified from your GApplication::handle-local-options handler. This documents behavior that is only added in the next patch.
Review of attachment 267578 [details] [review]: ::: gio/gapplication.c @@ +995,3 @@ + * + * The ::handle-local-options signal is emitted on the local instance + * after the parsing of the command line options has occurred. Should stay consistent: we say 'commandline' everywhere else, no 'command line' @@ +1004,3 @@ + * from the @arg_data of an installed #GOptionEntrys) in order to + * decide to perform certain actions, including premature termination + * of the application (which may be useful for options like --version). 'termination of the application' is a little confusing in this context. We're just talking about the termination about the current process, not a possibly already running instance. @@ +1006,3 @@ + * of the application (which may be useful for options like --version). + * + * The handler should return a non-negative value to prematurely more precisely: a value != -1, I think ?
(In reply to comment #33) [...] > This could be better documented. Yes, please. Reading this patch, I'm left wondering what will appear in the argv and what in the options, when dealing with a commandline. This will doubtlessly confuse many people. > > 3. We also seem to have lost the "--gapplication-service must be the sole > > option given" ? > > Yes -- and that's intentional. [...] Rationale sounds fine to me. I hope --gapplication-service will make an appearance in the docs, together with some of these explanations. Maybe add a chapter about 'Handling commandline options with GApplication'.
Created attachment 268281 [details] [review] GApplication: parse command line options Add support for parsing command line options with GApplication. I ended up hating g_application_command_line() a lot for a small but complicated reason: the signature of the function would have been different than the signature of the signal. This has a lot to do with the strange split personality of GApplication: it functions as both the application and the thing that launches it. I think this is something that we may want to fix some day (by deprecating the 'invoker'-side API of GApplication and introducing a separate launcher API) so I certainly don't want to go adding more things... The approach here gives us all of the abilities we had before with less API exposed. In fact, the only change to a gedit patch is a rename of a function and a bugfix. It's my opinion that the new signal should be sufficiently powerful for almost any use and that overriding local_command_line() should become a thing of the past. As a result, our slightly magical treatment of "options" without exposing it as a public API is probably OK.
Review of attachment 268281 [details] [review]: Please add the docs to the sections file. Looks fine to me otherwise. Thanks!
Comment on attachment 268281 [details] [review] GApplication: parse command line options Attachment 268281 [details] pushed as 0e67128 - GApplication: parse command line options