GNOME Bugzilla – Bug 792064
[PATCH] gsettings list-schemas --print-paths
Last modified: 2018-01-05 13:05:34 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).
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.
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).
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.
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.)
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.
Review of attachment 366340 [details] [review]: ++