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 792064 - [PATCH] gsettings list-schemas --print-paths
[PATCH] gsettings list-schemas --print-paths
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gsettings
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Allison Karlitskaya (desrt)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-12-30 11:32 UTC by Arnaud B.
Modified: 2018-01-05 13:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add ‘gsettings list-schemas --print-paths’ option. (4.00 KB, patch)
2017-12-30 11:32 UTC, Arnaud B.
none Details | Review
Add ‘gsettings list-schemas --print-paths’ option. (4.68 KB, patch)
2018-01-03 16:34 UTC, Arnaud B.
none Details | Review
Add ‘gsettings list-schemas --print-paths’ option. (4.76 KB, patch)
2018-01-04 23:51 UTC, Arnaud B.
committed Details | Review

Description Arnaud B. 2017-12-30 11:32:33 UTC
Created attachment 366110 [details] [review]
Add ‘gsettings list-schemas --print-paths’ option.

Paths remain a quite central concept of GSettings, but there’s no way for now to get from the `gsettings` tool the path where a (non-relocatable, of course) schema is mapped.

Here is a patch that adds the ‘gsettings list-schemas --print-paths’ option. This will be useful for proposing path completion to `dconf-editor` command-line and similar tools.

Usual warning, my C skills are bad, so double-check the memory freeing (notably).
Comment 1 Philip Withnall 2018-01-03 11:17:44 UTC
Review of attachment 366110 [details] [review]:

The man page for gsettings-tool needs updating to mention the new option too: docs/reference/gio/gsettings.xml.

::: gio/gsettings-tool.c
@@ +145,3 @@
+{
+  gchar **schemas;
+  gint i;

Use gsize instead of gint, since this is an array index.

@@ +149,3 @@
+  g_settings_schema_source_list_schemas (global_schema_source, TRUE, &schemas, NULL);
+
+  for (i = 0; schemas[i]; i++)

Nitpick: Make the NULL comparison more explicit: `schemas[i] != NULL`. That makes it more obvious that schemas[i] isn’t a boolean.

@@ +155,3 @@
+      gchar *schema_path;
+
+      schema_name = g_strdup (schemas[i]);

No need to g_strdup() this.

@@ +159,3 @@
+      schema = g_settings_schema_source_lookup (global_schema_source, schema_name, TRUE);
+      if (schema == NULL)
+        continue; // should never happen

If this should never happen, why put in code to handle it? Use `g_assert (schema != NULL)` if you want to double-check that something will never happen.

@@ +161,3 @@
+        continue; // should never happen
+
+      schema_path = g_strdup (g_settings_schema_get_path (schema));

No need to g_strdup() this.

@@ +163,3 @@
+      schema_path = g_strdup (g_settings_schema_get_path (schema));
+      if (schema_path == NULL)
+        continue; // should never happen

Similarly here.

@@ +165,3 @@
+        continue; // should never happen
+
+      schemas[i] = g_strconcat (schema_name, " ", schema_path, NULL);

This leaks the old value of schemas[i].

@@ +846,3 @@
     return gsettings_help (FALSE, argv[1]);
 
+  if (argc > 2 && skip_third_arg_test == FALSE)

Nitpick: Use `!skip_third_arg_test` rather than `skip_third_arg_test == FALSE`, to fit in with the coding style.
Comment 2 Arnaud B. 2018-01-03 16:34:45 UTC
Created attachment 366250 [details] [review]
Add ‘gsettings list-schemas --print-paths’ option.

Thanks for this review. I tried to correct my patch in regard to the issues you found in it, and added some lines in the documentation. I’m not exactly sure of the “<arg choice="plain">” I used in it, but I don’t find a similar case in other documentations, as I don’t think “<option>” would be appropriated here (it’d be rendered in bold in the manpage, for example).
Comment 3 Philip Withnall 2018-01-04 10:59:01 UTC
Review of attachment 366250 [details] [review]:

Thanks. Two more minor comments, then I think this should be ready to merge.

::: docs/reference/gio/gsettings.xml
@@ +200,3 @@
 Lists the installed, non-relocatable schemas.
 See <option>list-relocatable-schemas</option> if you are interested in
+relocatable schemas. If <arg choice="plain">--print-paths</arg> is given,

Use <optional><option>--print-paths</option></optional> instead of <arg>.

::: gio/gsettings-tool.c
@@ +155,3 @@
+      const gchar *schema_path;
+
+      schema_name = schemas[i];

Use `g_steal_pointer (&schemas[i])` here to make it more obvious that ownership is being transferred to `schema_name`. It also means that if the code is changed to bail early in future, there’s no chance of double-freeing this schema name.
Comment 4 Philip Withnall 2018-01-04 11:01:18 UTC
Review of attachment 366250 [details] [review]:

::: docs/reference/gio/gsettings.xml
@@ +200,3 @@
 Lists the installed, non-relocatable schemas.
 See <option>list-relocatable-schemas</option> if you are interested in
+relocatable schemas. If <arg choice="plain">--print-paths</arg> is given,

(See http://tdg.docbook.org/tdg/4.5/optional.html.)
Comment 5 Arnaud B. 2018-01-04 23:51:37 UTC
Created attachment 366340 [details] [review]
Add ‘gsettings list-schemas --print-paths’ option.

I find the rendering of the “<optional><option>” tags in the manpage a little strange, but didn’t found a better way to do as it’s semantically correct. So, applied the two changes you requested, and added a link to this bug in the description.
Comment 6 Philip Withnall 2018-01-05 13:00:33 UTC
Review of attachment 366340 [details] [review]:

++