GNOME Bugzilla – Bug 687487
Replace static GObject constants and functions with those available in GI
Last modified: 2012-11-06 08:04:16 UTC
This is parallel to bug 686765 but for gobject. There are many constants and functions exposed through the gobject static bindings that can easily be replaced by what is already available in gi. Before any of this work can be done, we need to verify there are unittests which cover all the methods moving to gi. This should be committed separately before the move. Once stuff is moved, we should not have to change tests to get them working again with gi. A few simple ones to start with (these should also be marked as deprecated): SIGNAL_* = GObject.SignalFlags.* PARAM_* = GObject.ParamFlags.* The following can be removed from gi/_gobject/__init.py and pulled in as deprecated within the GObject overrides in favor of GLib usage: OPTION_FLAG_* = GLib.OPTION_FLAG_* OPTION_ERROR_* = GLib.OPTION_ERROR_* spawn_async = GLib.spawn_async filename_from_utf8 = GLib.filename_from_utf8 Pid = GLib.Pid GError = GLib.GError glib_version = GLib.glib_version OptionGroup = GLib.OptionGroup OptionContext = GLib.OptionContext GObject module level functions already functioning the same as the static bindings out of the box. These can simply be deleted from the static bindings (with noted exceptions): type_name (1) type_from_name (1) type_is_a (1) type_children type_interfaces signal_list_ids signal_lookup signal_name (1) The static version might still be needed for other python modules within gi/_gobject/* Methods which will need overrides to get them working the same as the static bindings: type_parent - Needs to raise RuntimeError when an invalid type is returned. signal_query - Needs to return a tuple (named tuple is best containing the members of the return) The following cannot be removed for various reasons, but can be explicitly assigned in the GObject overrides instead using gi/_gobject/__init__.py with reliance on DynamicGObjectModule to layer things: add_emission_hook = _gobject.add_emission_hook features = _gobject.features list_properties = _gobject.list_properties pygobject_version = _gobject.pygobject_version new = _gobject.new remove_emission_hook = _gobject.remove_emission_hook signal_list_names = _gobject.signal_list_names signal_new = _gobject.signal_new threads_init = _gobject.threads_init type_register = _gobject.type_register signal_accumulator_true_handled = _gobject.signal_accumulator_true_handled G_MIN/MAX_* = _gobject.G_MIN/MAX_* The constant types found in gi/_gobject/constants.py can be pulled in with "type_from_name" within the overrides and the constants.py file can be deleted. However, some of these are used by signalhelper.py and propertyhelper.py. In these cases, the used constants can be added directly into those modules using the static bindings. Eventually it would be nice if signalhelper and propertyhelper could become part of the overrides and rely on gi instead of the static bindings.
Created attachment 227956 [details] [review] Add unittests for module level type and signal functions Add tests for the following methods: signal_list_ids, signal_name, signal_lookup, signal_query, type_children, type_from_name, type_name, type_is_a, and type_interfaces.
We will need to keep signal_query as a static method until bug 687550, bug 687545, and bug 687541 are fixed.
Created attachment 228074 [details] [review] Make unitests for gobject functions moving to gi more strict Add stricter error condition tests.
Created attachment 228075 [details] [review] Move gobject static functions and constants to gi Replace the following functions with gi and overrides: type_children, type_interfaces, signal_list_ids, signal_list_names, signal_lookup, signal_name, type_parent. Assign SIGNAL_* and PARAM_* from gi SignalFlags and ParamFlags respectively. Move module level assignments of a number of static functions to the GObject.py overrides file.
(In reply to comment #1) > Created an attachment (id=227956) [details] [review] > Add unittests for module level type and signal functions Thanks for writing those! This is committed now, but I have some suggestions for cleanup: - test_type_name() looks a bit excessive (as it requires the same level of maintenance than the actual assignment of the constants), but ok. - test_gi_types_equal_static_type_from_name() checks internal API. I'd prefer if this could use GObject.type_from_name(), as that's what GObject-2.0.gir exports. If that doesn't work, it should be overridden, and then this test case would include the override (or check the GI method directly once we drop the static binding). But test_type_from_name() already tests the GI method (or the override) directly, and as that's the API which we really want, I think test_gi_types_equal_static_type_from_name() should just be dropped, and some of its tests (like for guint64) moved to test_type_from_name(). + def test_signal_list_ids(self): + # This should not raise a SystemError: + # https://bugzilla.gnome.org/show_bug.cgi?id=687492 + self.assertRaises(SystemError, GObject.signal_list_ids, GObject.TYPE_INVALID) - Please let's not assert wrong behaviour -- let's rather check it throws a TypeError, and mark the test as @unittest.expectedFailure. Thanks!
Comment on attachment 228074 [details] [review] Make unitests for gobject functions moving to gi more strict + with self.assertRaisesRegex(SystemError, 'error return without exception set'): + GObject.signal_list_ids(GObject.TYPE_INVALID) This still asserts wrong behaviour; please see my previous comment. This happens again further down.
Comment on attachment 228075 [details] [review] Move gobject static functions and constants to gi Yay! Why does gi/_gobject/gobjectmodule.c drop all SIGNAL_* except SIGNAL_RUN_FIRST, and all PARAM_* except PARAM_READWRITE? The rest looks good to me.
(In reply to comment #6) > (From update of attachment 228074 [details] [review]) > + with self.assertRaisesRegex(SystemError, 'error return without > exception set'): > + GObject.signal_list_ids(GObject.TYPE_INVALID) > > This still asserts wrong behaviour; please see my previous comment. This > happens again further down. Per our IRC discussion, the SystemError emulation will be removed and instead the test will check the default TypeError coming from pygi. (In reply to comment #7) > (From update of attachment 228075 [details] [review]) > Yay! > > Why does gi/_gobject/gobjectmodule.c drop all SIGNAL_* except SIGNAL_RUN_FIRST, > and all PARAM_* except PARAM_READWRITE? I needed to keep these because they are used by signalhelper.py and propertyhelper.py. I hope they can be removed later but this will take some amount of re-structuring to get around circular dependencies. (In reply to comment #5) > - test_type_name() looks a bit excessive (as it requires the same level of > maintenance than the actual assignment of the constants), but ok. > > - test_gi_types_equal_static_type_from_name() checks internal API. I'd prefer > if this could use GObject.type_from_name(), as that's what GObject-2.0.gir > exports. If that doesn't work, it should be overridden, and then this test case > would include the override (or check the GI method directly once we drop the > static binding). But test_type_from_name() already tests the GI method (or the > override) directly, and as that's the API which we really want, I think > test_gi_types_equal_static_type_from_name() should just be dropped, and some of > its tests (like for guint64) moved to test_type_from_name(). We need to keep the static type_from_name method around because it is used in bootstrapping code (via constants.py) to get an introspection module up and running. These tests were validation that the gi method and static method return the same things. So beyond a single verification that, yes the static method returns the same thing as the gi method, the tests are probably not needed and I will remove them in the updated patch.
Comment on attachment 228075 [details] [review] Move gobject static functions and constants to gi Ah, thanks for the explanation!
Created attachment 228077 [details] [review] Make unitests for gobject functions moving to gi more strict Add expected failure test for invalid SystemError's coming from signal_lookup and signal_list_ids. Remove excessive type_name tests and type_from_name tests.
Created attachment 228078 [details] [review] Move gobject static functions and constants to gi Replace the following functions with gi and overrides: type_children, type_interfaces, signal_list_ids, signal_list_names, signal_lookup, signal_name, type_parent. Assign SIGNAL_* and PARAM_* from gi SignalFlags and ParamFlags respectively. Move module level assignments of a number of static functions to the GObject.py overrides file.
Comment on attachment 228077 [details] [review] Make unitests for gobject functions moving to gi more strict Thanks!
Comment on attachment 228078 [details] [review] Move gobject static functions and constants to gi Assuming that you tested this with both py 2.7 and 3.x, this looks good. Please go ahead, thanks!
Attachment 228077 [details] pushed as f4acd6a - Make unitests for gobject functions moving to gi more strict Attachment 228078 [details] pushed as da21069 - Move gobject static functions and constants to gi
Created attachment 228093 [details] [review] Move TYPE and G_MIN/MAX constants from _gobject to GObject Clear out TYPE and G_MIN/MAX constants from _gobject/__init__.py and move them into the GObject overrides. Disperse class imports among modules that use them instead of using _gobject/__init__.py as a staging area (e.g. GInterface). There is still more to do but this is as far as things can be taken without module re-structuring. This should be enough to close this bug and also safely get bug 687488 fixed.
Comment on attachment 228093 [details] [review] Move TYPE and G_MIN/MAX constants from _gobject to GObject Thanks for this! I adjusted it to the changes that I already made to trunk (as discussed on IRC).
Should we keep this open until the remaining constants and wrappers get dropped, or is that "good enough" now? We can't drop the G_MAXINT and friends ones yet as they are not in the .gir. I'm not sure whether we need to keep the static wrappers for signal_new() and signal_query() or whether we can move those to GI.