GNOME Bugzilla – Bug 693618
set_properties() on Gtk.Widget doesn't work
Last modified: 2013-02-13 08:32:25 UTC
Created attachment 235741 [details] [review] Patch with fix + test for the problems related in this bug report Calling `.set_properties()` on a Gtk.Widget currently fails with the following message: SystemError: error return without exception set I bissected the faulty commit to http://git.gnome.org/browse/pygobject/commit/?id=fd32acdd97f49f086a8ad5cf2b65862c4e6ccc44 While fixing the problem, I noticed that calling the method with several properties to set would only set one of the property. The first problem is due to the fact that `result` is not always set to Py_None upon completion (if pygi_set_property_value() returns 0 straight away, it jumps to exit: and returns NULL). The second problem is related: it exits the while loop as soon as a property has been set by pygi_set_property_value(). Strangely, the test in test_gi.TestPropertyObject.test_multi doesn't show this problem, and I couldn't reproduce it using GObject.GObject directly. That's why I'm reporting against Gtk.Widget. I made a patch to fix both problem, with one test showing the expected, correct result. Thanks, Jonathan
Review of attachment 235741 [details] [review]: Thanks for this! a few comments. ::: gi/_gobject/pygobject.c @@ +1457,3 @@ + if (ret != 0) { + if (PyErr_Occurred()) + goto exit; This seems right. It would be helpful if there was a comment stating that non-zero can mean an error occurred or the property was not found (in which case we try the default set_property_from_pspec). ::: tests/test_overrides_gtk.py @@ +617,1 @@ Rather than using Gtk for the test we might be able to use GIMarshalingTests.PropertiesObject which has a large amount of properties. The reason you couldn't get the bug to show up with a regular GObject is likely because dynamically created properties don't have introspection information.
Created attachment 235821 [details] [review] Correctly set properties on object with statically defined properties
Thanks for the review. * I added the comments in pygobject.c * I changed the test to use GIMarshalingTests.PropertiesObject instead, and moved it to the corresponding test_gi.py file * I also changed the commit message, in relation with the informations you gave me in #1 Note that I'm brand new to PyGObject, so the comment or the commit message might be wrong (:
Created attachment 235822 [details] [review] Correctly set properties on object with statically defined properties My bad, I uploaded an incomplete patch.
Also, note that setting "some_char" instead of "some_uchar" to 54 raises this error here: (process:5572): GLib-GObject-CRITICAL **: g_value_set_uchar: assertion `G_VALUE_HOLDS_UCHAR (value)' failed
Review of attachment 235822 [details] [review]: Thanks for the updated patch. It seems like the new test would not exercise the first problem mentioned. As that occurs when setting a dynamically defined property with set_properties? So perhaps derive a class from GIMarshallingTests.PropertiesObject with an additional dynamic property and test a mixture of the two with set_properties?
Commited with a few commit message tweaks.