GNOME Bugzilla – Bug 727455
Command line option parsing from bindings
Last modified: 2014-08-20 14:03:58 UTC
I recently wanted to add command line handling to a Gjs application and found that it really wasn't possible. The first hurdle was that GEntryOption wanted arg_data to be a pointer to where the argument should be stored. Which wasn't possible from Gjs. With the new add_main_option_entries function of GApplication that hurdle was removed. Then there is the hurdle of not being able to instantiate a GOptionEntry since it was not a boxed object. That could be implemented. But the third hurdle then becomes that add_main_option_entries expect a NULL terminated array of GOptionEntry, declared as const GOptionEntry *, I'm guessing that won't work well. Is there a way forward in enabling use of this API from bindings that can not instantiate a GOptionEntry?
Created attachment 277127 [details] [review] Make GOptionEntry a boxed type This is needed in order for bindings to do command line options parsing. If the bindings can handle null terminated flat arrays and can instanciate GOptionEntries then they can use the GApplication command line options API.
Created attachment 277128 [details] [review] GOptionGroup: Copy entries to entry list GOptionGroup needs to be in ownership of at least the string fields of the GOptionEntry. If the GOptionEntry has been temporarily instanciated in a GI based language like: application.add_main_option_entries([GLib.OptionEntry.new(...)])
Created attachment 277726 [details] [review] Make GOptionEntry a boxed type This is needed in order for bindings to do command line options parsing. If the bindings can handle null terminated flat arrays and can instanciate GOptionEntries then they can use the GApplication command line options API.
In bug #725614 Matthias Clasen said: "Since I've commented on irc, I'll repeat my opinion here: The entire GOptionEntry API smells of C convenience API to me. I'm not convinced that 1-1 translating it to bindings is a great idea." So does anyone have any idea how to avoid this? The current API of GOptionContext and the GApplication API wants a zero-terminated flat array of GOptionEntries to process command line options. Do we want to add some other method (in both senses) to specify the options to those APIs? Any ideas how that might look in that case?
I'm sorry for not replying on this bug, but I sort of have the same opinion -- the entire situation just strikes me as very undesirable. The fact that we have GApplication tied up in the handling of these arguments, however, means that we probably should have a way for bindings to deal with this. In my opinion, the best way to go is with the GVariant route. What's left, then, is a way to add option entries with NULL arg_data to the list. I think we could just have a new API on GApplication for that. Something like void add_option(string long_name, char short_name, int flags, GOptionArg arg, string description, string arg_description); ie: basically mirror the fields of the optionentry struct into the arguments of the function, minus the arg_data. I don't consider this to be particularly beautiful, but I think it would get the job done. Would that be good enough?
Created attachment 283266 [details] [review] GApplication: Add g_application_add_option Do you mean something like this? It lets me add options from gjs code at least and have it work with the VariantDict from handle-command-line. It is a bit hacky. Since it is not string literals for the long_name/description/arg_description they need to be strdup'd and freed. Whatcha think?
Maybe the function name should be g_application_add_main_option to be more in line with g_application_add_main_option_entries?
Review of attachment 283266 [details] [review]: ::: gio/gapplication.c @@ +453,3 @@ g_variant_dict_insert_value (dict, entry->long_name, value); + + g_free ((char *) entry->long_name); This is a bit awful. As much as I normally hate this sort of thing, I'd almost prefer a "should free" flag here which would let us avoid the dup in the (currently) normal case. I consider the cast to be at least as awful as that... Another option would be to intern the strings... @@ +695,3 @@ + * g_application_add_main_option_entries() with a single #GOptionEntry + * that has its arg_data field set to %NULL. + * Please add a note here mentioning how to collect the result. @@ +701,3 @@ + **/ +void +g_application_add_option(GApplication *application, Small whitespace nit: there should be a space between the function name and the '('. Also: indeed please rename this as suggested.
Review of attachment 283266 [details] [review]: ::: gio/gapplication.c @@ +453,3 @@ g_variant_dict_insert_value (dict, entry->long_name, value); + + g_free ((char *) entry->long_name); I agree, it is awful :) I'm having a hard time coming up with a non-awful way though. Where do you propose the should-free flag should be? How would that help us avoid the cast? The strings need to be freed somewhere. I have not used the intern_strings-api and I'm not sure how I would use it here. @@ +695,3 @@ + * g_application_add_main_option_entries() with a single #GOptionEntry + * that has its arg_data field set to %NULL. + * Sure will add a bit, I didn't want to duplicate to much from add_main_option_entries @@ +701,3 @@ + **/ +void +g_application_add_option(GApplication *application, Sure.
Created attachment 283864 [details] [review] GApplication: Add g_application_add_main_options This way is not pretty either. But avoids the cast. If you feel this is worse, could you expand a bit more on the 'should-free' flag or whether we should intern the strings?
Created attachment 283866 [details] [review] GApplication: Add g_application_add_main_option Borked the generation of that patch, resend.
Review of attachment 283866 [details] [review]: For the record, when one speaks of "interned strings" they are talking about g_intern_string(). This patch looks good enough to go in as-is, though. ::: gio/gapplication.c @@ +242,3 @@ + + /* Allocated option strings, from g_application_add_main_option() */ + GSList *option_strings; Given the constraints, this isn't bad at all. I like it.
Attachment 283866 [details] pushed as bf9c862 - GApplication: Add g_application_add_main_option