GNOME Bugzilla – Bug 696054
Rework the way command line parameters are passed to panels
Last modified: 2013-04-03 16:13:30 UTC
For bug #694315 (Add a way to request the creation of new online accounts) we need a way to pass parameters in a DBus-friendly way and be able to pass more structured data. It would also help to be able to have panel-specific command-line --flags to e.g. filter online accounts providers by features provides as requested in bug #695885. This issue is to collect the patches related to parameters-passing in a single place, to make tracking their status easier.
Created attachment 239119 [details] [review] shell: Use GVariant to convey panel arguments instead of a string array By using a GVariant of type "av" we can potentially pass more structured data to panels, which will become relevant with the ability to invoke them by GAction-based DBus-activation introduced in the following patch.
Created attachment 239120 [details] [review] shell: Let panels have their own commandline flags Add a class method to CcPanel to get a GOptionGroup which will be added to the main commandline parser. This gives panels the chance to have commandline "--flags" in addition to the already available parameters. This changes changes the way parameters are passed to panels: the first entry in the GVariant array is always the a{sv} dictionary of commandline flags, followed by the remaining free-form arguments.
Created attachment 239276 [details] [review] shell: Let panels have their own commandline flags Fix the keyboard panel parameter indexes.
Created attachment 239277 [details] [review] shell: Expose panel launching with DBus-activation Turn Control Center in a DBus-activable service and export a 'launch-panel' GAction which accepts a tuple containing the id of the desired panel and its parameters as a GVariant array. The snippet below show how the custom shortcuts section of the keyboard panel can be invoked by a external programs through DBus: GVariantBuilder *flags = g_variant_builder_new (G_VARIANT_TYPE_VARDICT); GVariantBuilder *params = g_variant_builder_new (G_VARIANT_TYPE ("av")); g_variant_builder_add (params, "v", g_variant_builder_end (flags)); g_variant_builder_add (params, "v", g_variant_new_string ("shortcuts")); g_variant_builder_add (params, "v", g_variant_new_string ("custom")); GVariant *v = g_variant_new ("(s@av)", "keyboard", g_variant_builder_end (params)); GApplication *gnomecc = g_application_new (id, G_APPLICATION_IS_LAUNCHER); if (!g_application_register (gnomecc, NULL, &error)) g_error ("Failed to register launcher for %s: %s", id, error->message); g_action_group_activate_action (G_ACTION_GROUP (gnomecc), "launch-panel", v); Updated the example above after the introduction of the commandline flags patch. Original attachment: https://bugzilla.gnome.org/attachment.cgi?id=239121
Review of attachment 239119 [details] [review]: Looks good to commit after those changes. ::: panels/keyboard/cc-keyboard-panel.c @@ +95,3 @@ + g_variant_get_child (parameters, 1, "v", &v); + section = g_variant_get_string (v, NULL); + g_variant_unref (v); Mention the fall-through in a comment. @@ +100,3 @@ + page = g_variant_get_string (v, NULL); + g_variant_unref (v); + cc_keyboard_panel_set_page (panel, page, section); Ditto. ::: panels/network/cc-network-panel.c @@ +172,3 @@ + gsize i = 0, n; + n = g_variant_iter_init (&iter, array); + strv = g_new0 (const gchar*, n + 1); I'd prefer if you use g_ptr_array() instead. ::: shell/cc-application.c @@ +196,3 @@ + g_variant_builder_add (builder, "v", g_variant_new_string (start_panels[i])); + parameters = g_variant_builder_end (builder); + if (!cc_shell_set_active_panel_from_id (CC_SHELL (self->priv->window), start_id, parameters, &err)) Are we leaking the parameters here?
Review of attachment 239276 [details] [review]: Looks fine at first glance.
Review of attachment 239276 [details] [review]: (missed changing the status)
Review of attachment 239277 [details] [review]: Looks good!
(In reply to comment #5) > ::: shell/cc-application.c > @@ +196,3 @@ > + g_variant_builder_add (builder, "v", g_variant_new_string > (start_panels[i])); > + parameters = g_variant_builder_end (builder); > + if (!cc_shell_set_active_panel_from_id (CC_SHELL (self->priv->window), > start_id, parameters, &err)) > > Are we leaking the parameters here? I had to chase this down to the innards of GValue: value_collect_variant() calls g_variant_ref_sink(), thus no, we don't need to release floating GVariant arguments passed to g_object_new().
The following fixes have been pushed: ab0576f shell: Expose panel launching with DBus-activation 31a8a99 shell: Let panels have their own commandline flags 9977bb2 shell: Use GVariant to convey panel arguments instead of a string array
Created attachment 240499 [details] [review] shell: Expose panel launching with DBus-activation Turn Control Center in a DBus-activable service and export a 'launch-panel' GAction which accepts a tuple containing the id of the desired panel and its parameters as a GVariant array. The snippet below show how the custom shortcuts section of the keyboard panel can be invoked by a external programs through DBus: GVariantBuilder *flags = g_variant_builder_new (G_VARIANT_TYPE_VARDICT); GVariantBuilder *params = g_variant_builder_new (G_VARIANT_TYPE ("av")); g_variant_builder_add (params, "v", g_variant_builder_end (flags)); g_variant_builder_add (params, "v", g_variant_new_string ("shortcuts")); g_variant_builder_add (params, "v", g_variant_new_string ("custom")); GVariant *v = g_variant_new ("(s@av)", "keyboard", g_variant_builder_end (params)); GApplication *gnomecc = g_application_new (id, G_APPLICATION_IS_LAUNCHER); if (!g_application_register (gnomecc, NULL, &error)) g_error ("Failed to register launcher for %s: %s", id, error->message); g_action_group_activate_action (G_ACTION_GROUP (gnomecc), "launch-panel", v);
Created attachment 240500 [details] [review] shell: Let panels have their own commandline flags Add a class method to CcPanel to get a GOptionGroup which will be added to the main commandline parser. This gives panels the chance to have commandline "--flags" in addition to the already available parameters. This changes changes the way parameters are passed to panels: the first entry in the GVariant array is always the a{sv} dictionary of commandline flags, followed by the remaining free-form arguments.
Created attachment 240501 [details] [review] shell: Use GVariant to convey panel arguments instead of a string array By using a GVariant of type "av" we can potentially pass more structured data to panels, which will become relevant with the ability to invoke them by GAction-based DBus-activation introduced in the following patch.