Created attachment 183996 [details] [review]
Add test for extended schemas

The test checks that the keys of the base schema are all present in
the extended schema.

Bug #645453.

I forget what my intent was here but I can easily imagine that I expected the client to traverse the chain of schemas searching for the key.  That's certainly he most reasonable behaviour when you consider that a particular large schema may have many other schemas extending it by only overriding one or two keys.

This doesn't just affect g_settings_list_keys(), but g_settings_set/get as well. 

IMO this should behave like GObject properties in derived classes: if Bar is derived from Foo, you can just g_object_get/set the properties from Foo even if you have a Bar instance. 

I don't think your scheme is workable. Note that if key K is overridden, it will be present in Bar, but if not, it won't. Also when you consider child schemas, which you must get with g_settings_get_child(), there is no function to turn the returned Bar schema into a Foo schema to 'search' for the key. Add in the inefficiency of having to construct all the gsettings for each schema from most-derived Bar to the base Foo, I think it's pretty clear what the 'intent' should be here :)

No -- I mean that GSettings should deal with this internally, but in the library, not the schema compiler.

Created attachment 258049 [details] [review]
GSettings: properly support 'extends'

Support the 'extends' attribute that has been supported by the compiler
for a long time by doing three things:

 - when creating a schema that extends another schema, lookup that other
   schema

 - when looking up keys and we can't find them in the schema, check
   (recursively) in the 'extends' schema

 - when listing all keys in a schema, also visit the extends schemas,
   but take care to avoid duplicates caused by overrides

Extend the testsuite to verify that it works.

Review of attachment 258049 [details] [review]:

::: gio/gsettingsschema.c
@@ +446,3 @@
+      if (schema->extends == NULL)
+        g_warning ("Schema '%s' extends schema '%s' but we could not find it", schema_id, extends);
+    }

Shouldn't this |return NULL;| here ? Otherwise if you use the GSettingsSchema to construct a GSettings, the original bug is still there.

The problem is that this will result in an abort.  That's fine, of course, but this abort didn't used to exist.  It's theoretically possible that this would break existing code...

Review of attachment 258049 [details] [review]:

Looks good to me. I agree with Ryan that we shouldn't return NULL there in the case that the base schema doesn't exist. It might break existing apps and the warning should be enough to catch those.

Attachment 183996 [details] pushed as ef57996 - Add test for extended schemas
Attachment 258049 [details] pushed as cbf8cf8 - GSettings: properly support 'extends'

I missed that Christian included a (better) testcase already, so I went with
his instead.

FYI, this is causing a hard crash in the PyGI test suite:
https://git.gnome.org/browse/pygobject/tree/tests/test_gio.py?id=3.10.1#n117

g_settings_schema_list() at gsettingsschema.c:1,015 0x7ffff4fbe878	
g_settings_list_keys() at gsettings.c:2,138 0x7ffff4fc5695	

It looks like "list" is NULL in some situation for this test and accessing it in the for loop crashes:

-      list = gvdb_table_list (schema->table, "");
-      len = list ? g_strv_length (list) : 0;
+          list = gvdb_table_list (s->table, "");
 
-      schema->items = g_new (GQuark, len);
-      j = 0;
+          for (i = 0; list[i]; i++)