GNOME Bugzilla – Bug 640838
Provide comfortable GSettings API
Last modified: 2011-02-08 14:29:50 UTC
I just discussed with Ryan Lortie about providing a comfortable dictionary-like GSettings API. Since we already have a nice GVariant API now, using the native interface does not look too bad actually (see the test cases I just committed [1]), but it could be made even more pythonic. I prepared a new test case how it could look like: ----------- 8< --------------- def test_gsettings_override(self): settings = Gio.Settings.new('org.gnome.test') # dictionary interface self.assertEqual(len(settings), 4) self.assert_('test-array' in settings) self.assert_('test-array' in settings.keys()) # get various types self.assertEqual(settings['test-boolean'], True) self.assertEqual(settings['test-string'], 'Hello') self.assertEqual(settings['test-array'], [1, 2]) self.assertEqual(settings['test-tuple'], (1, 2)) # set a value settings['test-string'] = 'Goodbye' self.assertEqual(settings['test-string'], 'Goodbye') settings['test-array'] = [3, 4, 5] self.assertEqual(settings['test-array'], [3, 4, 5]) ----------- 8< --------------- I have the __getitem__() and keys() parts implemented and working (those are easy), but I'm still unsure how to provide __setitem__(), as this would need to query the type signature of a value. Ryan mentioned that this would be easy and cheap, but I don't see anything in the GSettings API. Ryan, you mentioned that you would be happy to add some C API to do that? Something like const gchar* g_settings_get_type(GSettings *settings, const gchar* key) ? [1] http://git.gnome.org/browse/pygobject/commit/?id=69207910209ebfe450df616aeb8fa4cc2e7eccf3
hi Martin. One option you have now (and has been abused elsewhere) is that you could use the get() call. It will give you a GVariant, and by definition, the type of value for this key is the type of the value you are given. You may also find g_settings_get_range() to be helpful here.
Ah, g_settings_get_range() is indeed what I was looking for, thanks! Also, is there a way to avoid GVariant crashing with a SIGABRT each time you specify an unknown key? You can't intercept a C assert() failure in Python, so in order to produce proper KeyErrors instead of a program crash I need to do an expensive "name in settings.list_keys()", as there is no g_settings_has_key() method.
Created attachment 179580 [details] [review] first working patch This now provides a working implementation of a dictionary-like Gio.Settings with test cases. The thing that I don't particularly like yet is the rather expensive check if a key is known. I don't think it's a major issue as a program usually doesn't set/query hundreds of settings values, but perhaps this can still be improved by either not making GVariant not abort, or providing a more efficient key validity check.
Review of attachment 179580 [details] [review]: ::: gi/overrides/Gio.py @@ +51,3 @@ +class Settings(Gio.Settings, UserDict.DictMixin): + '''Provide dictionary-like access to GLib.Settings.''' + I'm not sure DictMixin is a good idea here. __repr__ should be overridden to show that it's a Settings instance and not a dict, eg: def __repr__(self): return '<Gio.Settings %s>' % (repr(self.items()), ) __nonzero__ should always return True. I don't think __cmp__ makes sense, when would you compare one Settings instance to another? Gio.Settings.get() conflicts with dict.get(), they expect different values. Since there are no python users yet I guess the python version should win. Constructor should be overridden so that: def __init__(self, schema, path=None, backend=None) does the right thing. @@ +54,3 @@ + def __getitem__(self, key): + # get_value() aborts the program on an unknown key + if not key in self.list_keys(): "if not key in self:" should be enough @@ +55,3 @@ + # get_value() aborts the program on an unknown key + if not key in self.list_keys(): + raise KeyError('unknown key ' + key) "unknown key: %r" % (key, ) is better here. @@ +61,3 @@ + def __setitem__(self, key, value): + # set_value() aborts the program on an unknown key + if not key in self.list_keys(): See above. @@ +62,3 @@ + # set_value() aborts the program on an unknown key + if not key in self.list_keys(): + raise KeyError('unknown key ' + key) See above. @@ +66,3 @@ + # determine type string of this key + range = self.get_range(key) + _type = range.get_child_value(0).get_string() Normally type_
Thanks Johan for the review! I'll get these fixed, but I still have one question: > I'm not sure DictMixin is a good idea here. Because it's not python 3 compatible? Right, I should move that to collections.MutableMapping. Or do you mean we shouldn't use these interfaces at all?
Since 2.2 you can just subclass from dict.
Created attachment 180326 [details] [review] patch (In reply to comment #4) > I'm not sure DictMixin is a good idea here. On second thought it might indeed provide too much magic. I removed it. > __repr__ should be overridden to show that it's a Settings instance and not a > dict, eg: repr() now (i. e. after dropping the mixin) says <Settings object at 0x100ccd0 (GSettings at 0x10aeca0)> which is consistent with other introspected classes. I'd leave it like that. > __nonzero__ should always return True. Fixed and test case added. > I don't think __cmp__ makes sense, when would you compare one Settings instance to another? I agree. Not having the dict mixin got rid of that, too. > Gio.Settings.get() conflicts with dict.get(), they expect different values. > Since there are no python users yet I guess the python version should win. Gio.Settings.get() doesn't really exist, as g_settings_get() is not introspectable (it's a vararg method). > Constructor should be overridden so that: > > def __init__(self, schema, path=None, backend=None) > > does the right thing. Done, test case added. Other remarks (KeyError etc.) fixed as well, with added test cases.
(In reply to comment #5) > Thanks Johan for the review! I'll get these fixed, but I still have one > question: > > > I'm not sure DictMixin is a good idea here. > > Because it's not python 3 compatible? Right, I should move that to > collections.MutableMapping. Or do you mean we shouldn't use these interfaces at > all? (In reply to comment #6) > Since 2.2 you can just subclass from dict. The point is that GSettings is not just a data type, it's also an instance, it's tricky to mix both of them. > repr() now (i. e. after dropping the mixin) says > > <Settings object at 0x100ccd0 (GSettings at 0x10aeca0)> > > which is consistent with other introspected classes. I'd leave it like that. __repr__ could still be implemented to show something useful about the content, some of the Gtk+ classes does that. Unsure what though, better to leave it like that now. > > > __nonzero__ should always return True. > > Fixed and test case added. This is still wrong. The test should test a gsettings instance which has a length of 0.
Actually inheriting from MutableMapping seems *much* saner than DictMixin. http://docs.python.org/library/collections.html#abcs-abstract-base-classes But then we'd need to bump the required python version to 2.6.
Created attachment 180384 [details] [review] patch against git head > > > __nonzero__ should always return True. > > > > Fixed and test case added. > > This is still wrong. The test should test a gsettings instance which has a > length of 0. Test case added. The default __nonzero__() is implemented in terms of len(), so this requires no code changes. This is fairly academic anyway, as Settings dictionaries have a fairly static structure (you don't add or change keys at runtime, they are defined in the .xml).
Created attachment 180385 [details] [review] patch against git head Discussed the bool() semantics on IRC, and it's actually supposed to always be True. If you write "if mysettings:", you usually just want to test that it's not None, not whether it's empty (Settings are not enough like a dictionary for that to make sense). For this reason, and because you can't change/add/delete keys, I also didn't subclass from MutableMapping.
Review of attachment 180385 [details] [review]: Looks good, thanks!
Pushed, thanks for the review!