GNOME Bugzilla – Bug 674351
[PATCH] Handle GType properties correctly
Last modified: 2012-04-20 13:10:24 UTC
For example, >>> Clutter.Inteval(value_type=GObject.TYPE_INT) TypeError: could not convert value for property `value_type' from gobject.GType to GType
Created attachment 212312 [details] [review] Handle GType properties correctly Fix conversion from/to properties of type G_TYPE_GTYPE
Thanks for the patch! This needs a test case, I'll try to come up with one.
Created attachment 212331 [details] [review] tests: Add test for GType properties Needs an additional property in Regress.TestObject https://bugzilla.gnome.org/show_bug.cgi?id=674366
It's unfortunately more tricky than that, your test case succeeds even without your patch. I have added a test case (not committed) to tests/test_properties.py which installs a new "gtype" property, but this requires a lot more fixes to gi/_gobject/propertyhelper.py and gi/_gobject/pygtype.c. I am currently working my way through the code. But while that is useful, I have a hunch that property initializers in constructor calls are handled in a different code path (the one you sent a patch for), so neither your nor my current test case is sufficient for the patch. A good test case might be to find a GType property in GObject, Gio, or Gtk (i. e. one of the libraries that the test suite already depends on anyway), and supply a value to this in a constructor call.
Grepping my system's girs for a GType property does not turn up any in the Girs that we have available in the pygobject test suite, though: $ grep -A5 '<property' /usr/share/gir-1.0/* | grep -B4 '<type.*GType' yields Folks-0.6.gir, Gee-1.0.gir, Gladeui-2.0.gir, and Peas-1.0.gir. Anyway, I think I'll finish my patch for general GType property support first, and will then add a test case to supply a value for that in the constructor call, which should match your original problem.
Comment on attachment 212331 [details] [review] tests: Add test for GType properties Your new test is useful anyway, so it's good to commit, it just isn't related to the patch.
Hmmm, you're right, the test even works without the patch. It only breaks with: >>> Everything.TestObj(gtype=int) and >>> object_.set_property('gtype', int) Both should be covered by the patch And yes, situation with different code paths is a bit messy. I recognized this while implementing http://bugzilla-attachments.gnome.org/attachment.cgi?id=212221 (which is open for your review, BTW ;))
Comment on attachment 212331 [details] [review] tests: Add test for GType properties Thanks! I updated the test case to include the constructor property setting (which previously failed), fixed the tab damage, and committed both patches under one commit: http://git.gnome.org/browse/pygobject/commit/?id=84e3471ba4595534cbe6875f1c8b77776e1d1814
Created attachment 212349 [details] [review] Support defining GType properties from Python This is the patch I was working on when creating a test case for your bug. This test case also reproduces the problem and confirms that your patch fixes it. It also exposes that creating GType properties from Python does not work. With this patch it does. I'd appreciate a review, as it's non-trivial. In particular, I'm not very happy about the change to _default_getter() in gi/_gobject/propertyhelper.py. It would perhaps be better to actually set property default values through g_param_value_set_default() so that the defaults are available in the C source as well, instead of only returning the default value in _default_getter()? If so, this should become a separate patch, and I'll rework mine on top of that.
Ah, now I know why I had so much trouble with the default values -- I read gobject/gparamspecs.h, for some reason GType properties do not actually support a default value, unlike most other types. g_param_value_set_default() does not do what I thought it did. So I'll rework the patch to drop default values for GType properties.
Created attachment 212370 [details] [review] Support defining GType properties from Python This patch drops default values for GTypes (as GLib does not support them), and thus also the previous hack. This patch looks much better now.
Was reviewed on IRC, pushed.