GNOME Bugzilla – Bug 751597
Fix command line handling
Last modified: 2015-07-16 10:10:34 UTC
g-c-c command line handling currently has various issues: - gnome-control-center --help/--list will not exit - gnome-control-center --help/--list will not output anything if another g-c-c instance is already running - gnome-control-center --version will cause already existing g-c-c instances to exit This patch series makes use of GApplication::handle-local-options in order to fix these issues. There is no in-tree implementation of the CcPanel::get_option_group() vfunc, so "94ac9f6 Readd call to cc_panel_loader_add_option_groups()" hasn't been tested at all and is likely to be broken.
Created attachment 306208 [details] [review] shell: Fix GVariantBuilder leak When a GVariantBuilder is created with g_variant_builder_new(), it must be unref'ed with g_variant_builder_unref() when no longer needed. In this case, we can just use a local stack-allocated GVariantBuilder to avoid the leak.
Created attachment 306209 [details] [review] shell: Fix flags_builder leak The GVariantBuilder flags_builder used by cc_application_command_line is allocated using g_variant_builder_new() so it must be freed with g_variant_builder_unref() after use. It could be replaced with a stack-allocated GVariantBuilder, but subsequent commits will actually need a heap-allocated flags_builder, so I'm going with the minimal change here.
Created attachment 306210 [details] [review] Don't handle --help ourselves GOption can handle --help for us, so we don't need to reimplement this ourselves. This causes a small regression as starting a main gnome-control-center instance and then running gnome-control-center --help will cause the main instance-control-center to exit. This will be fixed in the following patches, and this fixes the opposite bug: if gnome-control-center is not running, gnome-control-center --help would not exit after displaying the help before this commit.
Created attachment 306211 [details] [review] Use g_application_add_main_option_entries Since GApplication provides this API, we can as well use it, this will be useful in order to correctly split option handling between the local instance and the main instance. This commit removes the call to cc_panel_loader_add_option_groups() which will be readded in the next commit. Nothing uses this functionality in the current codebase anyway.
Created attachment 306212 [details] [review] Parse command line args into a GVariantDict Since we are using g_application_add_main_option, we can remove the global variable used to parse the arguments into, and get the parsed arguments from the GVariantDict returned by g_application_command_line_get_options_dict(). This is in preparation for handling some command line options in the local gnome-control-center instance, and others in the remote instance.
Created attachment 306213 [details] [review] Connect to "handle-local-options" signal
Created attachment 306214 [details] [review] Handle --list from the local instance This is an help-like parameter, so we want its output to show up in the terminal from which gnome-control-center was just started, not from the terminal from which the main gnome-control-center instance was started.
Created attachment 306215 [details] [review] Handle --version as a regular argument It was handled through a callback calling exit(). Now that we have a handle-local-options callback, we can handle it as a regular argument from there.
Created attachment 306216 [details] [review] Readd call to cc_panel_loader_add_option_groups() This requires to refactor a little bit the way the 'flags_builder' GVariantBuilder is handled, it's now created by the local instance and then the handle-local-options callback passes the flags_builder content to the main instance through the 'options' GVariantDict. This is all untested as nothing upstream uses this feature.
Created attachment 306217 [details] [review] Move cheese_gtk_init() call earlier Now that we handle local command line arguments, the 'command_line' vfunc can no longer get the initial argc/argv passed to the process. I could not find an easy way of calling cheese_gtk_init() only when starting up the main instance, so it's called unconditionally from main() where argc/argv are available.
Created attachment 306218 [details] [review] Don't call gtk_clutter_init() when using cheese cheese_gtk_init() is called as well and will be taking care of that for us.
Some of the patches need the patch from https://bugzilla.gnome.org/show_bug.cgi?id=751598 to be applied to work correctly.
Add Emanuele to the CC: list.
(In reply to Christophe Fergeau from comment #0) > There is no in-tree implementation of the CcPanel::get_option_group() vfunc, > so "94ac9f6 Readd call to cc_panel_loader_add_option_groups()" hasn't been > tested at all and is likely to be broken. (In reply to Christophe Fergeau from comment #9) > Created attachment 306216 [details] [review] [review] > Readd call to cc_panel_loader_add_option_groups() > > This requires to refactor a little bit the way the 'flags_builder' > GVariantBuilder is handled, it's now created by the local instance and > then the handle-local-options callback passes the flags_builder content > to the main instance through the 'options' GVariantDict. > This is all untested as nothing upstream uses this feature. Emanuele, you were cc'ed as you were the author of g-c-c commit 31a8a99 and the bits/questions above are related to it
Review of attachment 306208 [details] [review]: Looks good to me.
Review of attachment 306209 [details] [review]: Looks good to me.
Review of attachment 306210 [details] [review]: Looks good to me.
Review of attachment 306211 [details] [review]: Looks good to me.
Review of attachment 306212 [details] [review]: Looks good to me, just some trivial remarks to prove that I've read it. ;) ::: shell/cc-application.c @@ +150,3 @@ + cc_shell_log_set_debug (TRUE); + else + cc_shell_log_set_debug (FALSE); Keeping the gboolean variable would seem cleaner to me: verbose = g_variant_dict_contains (options, "verbose"); cc_shell_log_set_debug (verbose); @@ +170,3 @@ int i; + g_return_val_if_fail (start_panels[0] != NULL, 1); Any reason for this? The lookup for G_OPTION_REMAINING should have failed already if the resulting string array contains no useful entries, right? Anyway, I'm not opposed to it, I'm just wondering if there's some specific rationale for being extra defensive here. :)
Review of attachment 306213 [details] [review]: Looks good to me.
Review of attachment 306214 [details] [review]: Looks good to me.
Review of attachment 306215 [details] [review]: Looks good to me.
Review of attachment 306213 [details] [review]: I've played a bit with the code, exploring the new shiny features of GOption/GApplication, and it seems that handling the signal and returning a positive integer is not enough to prevent the overview to be activated, probably because CcApplication overrides the GApplication::command_line handler instead of just handling the signal. Being consistent and overriding the GApplication::handle_local_options handler in cc_application_class_init() instead of listening to the signal seems to fix the issue.
Review of attachment 306216 [details] [review]: Honestly I'd just drop the code. It was introduced for bug #695885, but I never found the time to finish it so, unless there's anyone else interested in it, I'd just attach this patch to bug #695885 for reference and drop it from the current patchset.
Review of attachment 306217 [details] [review]: Looks good to me.
Review of attachment 306218 [details] [review]: Looks good to me.
Review of attachment 306213 [details] [review]: « returning a positive integer is not enough to prevent the overview to be activated. Being consistent and overriding the GApplication::handle_local_options handler in cc_application_class_init() instead of listening to the signal seems to fix the issue. » The glib patch in bug https://bugzilla.gnome.org/show_bug.cgi?id=751598 should fix the issue you are describing.
Thanks a lot for the reviews Emanuele!
> The glib patch in bug https://bugzilla.gnome.org/show_bug.cgi?id=751598 should fix the issue you are describing. It sounds like it's that, but in the meantime (and for consistency) overriding the GApplication::handle_local_options() method seems the best choice to me. I hate when there's more than one way to do it. ;)
Created attachment 307088 [details] [review] Revert "shell: Let panels have their own commandline flags" This reverts commit 31a8a99440cce715b2e32ca71a6cbf650eab012a.
Created attachment 307089 [details] [review] shell: Fix GVariantBuilder leak When a GVariantBuilder is created with g_variant_builder_new(), it must be unref'ed with g_variant_builder_unref() when no longer needed. In this case, we can just use a local stack-allocated GVariantBuilder to avoid the leak.
Created attachment 307090 [details] [review] Don't handle --help ourselves GOption can handle --help for us, so we don't need to reimplement this ourselves. This causes a small regression as starting a main gnome-control-center instance and then running gnome-control-center --help will cause the main instance-control-center to exit. This will be fixed in the following patches, and this fixes the opposite bug: if gnome-control-center is not running, gnome-control-center --help would not exit after displaying the help before this commit.
Created attachment 307091 [details] [review] Use g_application_add_main_option_entries Since GApplication provides this API, we can as well use it, this will be useful in order to correctly split option handling between the local instance and the main instance. This commit removes the call to cc_panel_loader_add_option_groups() which will be readded in the next commit. Nothing uses this functionality in the current codebase anyway.
Created attachment 307092 [details] [review] Parse command line args into a GVariantDict Since we are using g_application_add_main_option, we can remove the global variable used to parse the arguments into, and get the parsed arguments from the GVariantDict returned by g_application_command_line_get_options_dict(). This is in preparation for handling some command line options in the local gnome-control-center instance, and others in the remote instance.
Created attachment 307093 [details] [review] Connect to "handle-local-options" signal
Created attachment 307094 [details] [review] Handle --list from the local instance This is an help-like parameter, so we want its output to show up in the terminal from which gnome-control-center was just started, not from the terminal from which the main gnome-control-center instance was started.
Created attachment 307095 [details] [review] Handle --version as a regular argument It was handled through a callback calling exit(). Now that we have a handle-local-options callback, we can handle it as a regular argument from there.
Created attachment 307096 [details] [review] Move cheese_gtk_init() call earlier Now that we handle local command line arguments, the 'command_line' vfunc can no longer get the initial argc/argv passed to the process. I could not find an easy way of calling cheese_gtk_init() only when starting up the main instance, so it's called unconditionally from main() where argc/argv are available.
Created attachment 307097 [details] [review] Don't call gtk_clutter_init() when using cheese cheese_gtk_init() is called as well and will be taking care of that for us.
Review of attachment 306216 [details] [review]: Ok, I've added a patch reverting this.
Review of attachment 306216 [details] [review]: "this" = commit 31a8a99440
Review of attachment 306212 [details] [review]: ::: shell/cc-application.c @@ +150,3 @@ + cc_shell_log_set_debug (TRUE); + else + cc_shell_log_set_debug (FALSE); I've changed this. @@ +170,3 @@ int i; + g_return_val_if_fail (start_panels[0] != NULL, 1); No good reason I think. I probably added that during development as a sanity check and forgot to remove it. It's still there in the new version I just posted, I can drop it if you want.
Review of attachment 307088 [details] [review]: Can you add a short explanation in the commit message about the reason of the revert? Eg. simply stating that the feature was meant for bug #695885, but that no in-tree user has materialized in a long time.
Review of attachment 307088 [details] [review]: Very good point, thanks :) I've changed the log to « Revert "shell: Let panels have their own commandline flags" This reverts commit 31a8a99440cce715b2e32ca71a6cbf650eab012a. This was meant for bgo#695885 which has stalled for a while, so this feature has no in-tree user. This commit removes it for now, this can be readded when users for it materialize. https://bugzilla.gnome.org/show_bug.cgi?id=751597 »
Review of attachment 307088 [details] [review]: The edited commit message looks good, thanks!
Bastien, up to you! :)
Review of attachment 307089 [details] [review]: Sure.
Review of attachment 307090 [details] [review]: Looks good.
Review of attachment 307091 [details] [review]: > This commit removes the call to cc_panel_loader_add_option_groups() I don't see it being removed in this patch.
Review of attachment 307092 [details] [review]: I noticed now that you're missing the "shell: " prefix for all your commits. Please make sure to add those before committing. Looks fine otherwise.
Review of attachment 307093 [details] [review]: Looks fine otherwise. ::: shell/cc-application.c @@ +113,3 @@ +cc_application_handle_local_options (GApplication *application, GVariantDict *options) +{ + Extra line feed here.
Review of attachment 307091 [details] [review]: Indeed, it went away with «Revert "shell: Let panels have their own commandline flags"», the reference to it should be dropped from the commit message.
Review of attachment 307094 [details] [review]: Yes.
Review of attachment 307095 [details] [review]: Yep.
Review of attachment 307096 [details] [review]: That's fine, it was very convoluted anyway. You can remove the "I could not find an easy way of calling cheese_gtk_init() only when starting up the main instance" part of the commit message though.
Review of attachment 307097 [details] [review]: Sure.
Review of attachment 307091 [details] [review]: I've dropped the 2nd paragraph from the log: « shell: Use g_application_add_main_option_entries Since GApplication provides this API, we can as well use it, this will be useful in order to correctly split option handling between the local instance and the main instance. https://bugzilla.gnome.org/show_bug.cgi?id=751597 »
Attachment 307089 [details] pushed as c33ac8b - shell: Fix GVariantBuilder leak