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 640838 - Provide comfortable GSettings API
Provide comfortable GSettings API
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal enhancement
: ---
Assigned To: Martin Pitt
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2011-01-28 17:00 UTC by Martin Pitt
Modified: 2011-02-08 14:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
first working patch (5.78 KB, patch)
2011-01-29 11:24 UTC, Martin Pitt
reviewed Details | Review
patch (7.10 KB, patch)
2011-02-07 18:40 UTC, Martin Pitt
none Details | Review
patch against git head (7.39 KB, patch)
2011-02-08 14:10 UTC, Martin Pitt
none Details | Review
patch against git head (7.63 KB, patch)
2011-02-08 14:20 UTC, Martin Pitt
committed Details | Review

Description Martin Pitt 2011-01-28 17:00:45 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
Comment 1 Allison Karlitskaya (desrt) 2011-01-28 18:38:32 UTC
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.
Comment 2 Martin Pitt 2011-01-29 11:18:35 UTC
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.
Comment 3 Martin Pitt 2011-01-29 11:24:25 UTC
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.
Comment 4 Johan (not receiving bugmail) Dahlin 2011-02-07 16:20:43 UTC
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_
Comment 5 Martin Pitt 2011-02-07 16:30:14 UTC
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?
Comment 6 johnp 2011-02-07 18:39:29 UTC
Since 2.2 you can just subclass from dict.
Comment 7 Martin Pitt 2011-02-07 18:40:09 UTC
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.
Comment 8 Johan (not receiving bugmail) Dahlin 2011-02-08 12:55:51 UTC
(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.
Comment 9 Johan (not receiving bugmail) Dahlin 2011-02-08 13:02:44 UTC
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.
Comment 10 Martin Pitt 2011-02-08 14:10:39 UTC
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).
Comment 11 Martin Pitt 2011-02-08 14:20:40 UTC
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.
Comment 12 Johan (not receiving bugmail) Dahlin 2011-02-08 14:23:09 UTC
Review of attachment 180385 [details] [review]:

Looks good, thanks!
Comment 13 Martin Pitt 2011-02-08 14:29:40 UTC
Pushed, thanks for the review!