After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 696054 - Rework the way command line parameters are passed to panels
Rework the way command line parameters are passed to panels
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on:
Blocks: 694315 695885
 
 
Reported: 2013-03-18 12:29 UTC by Emanuele Aina
Modified: 2013-04-03 16:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
shell: Use GVariant to convey panel arguments instead of a string array (19.08 KB, patch)
2013-03-18 12:36 UTC, Emanuele Aina
committed Details | Review
shell: Let panels have their own commandline flags (8.90 KB, patch)
2013-03-18 12:36 UTC, Emanuele Aina
none Details | Review
shell: Let panels have their own commandline flags (9.02 KB, patch)
2013-03-19 17:28 UTC, Emanuele Aina
committed Details | Review
shell: Expose panel launching with DBus-activation (4.91 KB, patch)
2013-03-19 17:30 UTC, Emanuele Aina
committed Details | Review
shell: Expose panel launching with DBus-activation (4.91 KB, patch)
2013-04-03 16:13 UTC, Emanuele Aina
committed Details | Review
shell: Let panels have their own commandline flags (9.13 KB, patch)
2013-04-03 16:13 UTC, Emanuele Aina
committed Details | Review
shell: Use GVariant to convey panel arguments instead of a string array (19.37 KB, patch)
2013-04-03 16:13 UTC, Emanuele Aina
committed Details | Review

Description Emanuele Aina 2013-03-18 12:29: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.
Comment 1 Emanuele Aina 2013-03-18 12:36:11 UTC
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.
Comment 2 Emanuele Aina 2013-03-18 12:36:21 UTC
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.
Comment 3 Emanuele Aina 2013-03-19 17:28:01 UTC
Created attachment 239276 [details] [review]
shell: Let panels have their own commandline flags

Fix the keyboard panel parameter indexes.
Comment 4 Emanuele Aina 2013-03-19 17:30:52 UTC
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
Comment 5 Bastien Nocera 2013-04-03 13:57:35 UTC
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?
Comment 6 Bastien Nocera 2013-04-03 13:59:36 UTC
Review of attachment 239276 [details] [review]:

Looks fine at first glance.
Comment 7 Bastien Nocera 2013-04-03 14:00:05 UTC
Review of attachment 239276 [details] [review]:

(missed changing the status)
Comment 8 Bastien Nocera 2013-04-03 14:00:55 UTC
Review of attachment 239277 [details] [review]:

Looks good!
Comment 9 Emanuele Aina 2013-04-03 15:55:54 UTC
(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().
Comment 10 Emanuele Aina 2013-04-03 16:12:37 UTC
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
Comment 11 Emanuele Aina 2013-04-03 16:13:10 UTC
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);
Comment 12 Emanuele Aina 2013-04-03 16:13:20 UTC
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.
Comment 13 Emanuele Aina 2013-04-03 16:13:30 UTC
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.