GNOME Bugzilla – Bug 653462
Improve PyGTK compatibility
Last modified: 2012-03-16 19:42:49 UTC
This patch series improve the PyGTK compatibility, it extends the current Gdk and Gtk overrides to cover a few more parts from PyGTK. It is part of my project to write a wrapper mode which aim to keep enough compatibility to be able to run the same app against both PyGTK and introspection, which will help: * porting applications to introspection, as it can be done step by step * application that wish to support both PyGTK/introspection
Created attachment 190723 [details] [review] Add an override for Gdk.color_parse() Change Gdk.color_parse() to not return a tuple, instead just return the created color or None if it wasn't possible to parse the name into a color. This keeps compatibility with PyGTK but breaks the current API.
Created attachment 190724 [details] [review] Add a default detail value for Widget.render_icon
Created attachment 190725 [details] [review] Do not always pass in user_data to callbacks. This keeps API compatibility with PyGTK and avoids sending in user_data if it's None.
Created attachment 190726 [details] [review] Add a couple of constructors This is for PyGTK compatibility, so that gtk.HBox(True, 2) etc works.
Created attachment 190727 [details] [review] Add vbox/action_area properties Accessing vbox/action_area directly creates segmentation fault, avoid that by mapping the fields to their getters for PyGTK API compatibility
Created attachment 190728 [details] [review] Add a default parameter to GtkTreeModel.filter_new
Created attachment 190729 [details] gicompat.py This is the compatibility layer which extends the API (keysyms, enums/flags, module names) to be essentially 100% compatible. It is a work in progress and I'm not proposing it for inclusion at this point. When this is closer to being finished - when I can run most of Stoq on top of it, I'll clean it up and submit it for inclusion in pygobject.
Comment on attachment 190723 [details] [review] Add an override for Gdk.color_parse() Hmm, perhaps we can keep ABI by leaving Gdk.color_parse untouched and instead add a Gdk.Color.parse API which the converter script will use.
Comment on attachment 190724 [details] [review] Add a default detail value for Widget.render_icon This can be committed to the master, invoke-rewrite and pygobject-2-28 branches
Comment on attachment 190725 [details] [review] Do not always pass in user_data to callbacks. This might break apps so please only commit to master and invoke-rewrite branches. Also please create an ABI_BREAK_NOTES file and add an entry noting that in future version behaviour has changed for action group callbacks
Comment on attachment 190726 [details] [review] Add a couple of constructors This can be committed to the master, invoke-rewrite and pygobject-2-28 branches
Comment on attachment 190727 [details] [review] Add vbox/action_area properties Please commit to the master, invoke-rewrite and pygobject-2-28 branches
Comment on attachment 190727 [details] [review] Add vbox/action_area properties BTW this does need a unit test also so we don't regress
Comment on attachment 190728 [details] [review] Add a default parameter to GtkTreeModel.filter_new Please commit to the master, invoke-rewrite and pygobject-2-28 branches
Comment on attachment 190729 [details] gicompat.py Why isn't this just part of the conversion script? I don't want people to end up not porting their apps to the newer versions of the API.
(In reply to comment #15) > (From update of attachment 190729 [details]) > Why isn't this just part of the conversion script? I don't want people to end > up not porting their apps to the newer versions of the API. It's the opposite of the conversion script. I want to keep on using the older API until I know that my app works fine with gi. Eg there will be changes for apis that are not supported. I'm not going to stick around with the old api forever, it would be okay to show big warnings that cannot be removed.
(In reply to comment #8) > (From update of attachment 190723 [details] [review]) > Hmm, perhaps we can keep ABI by leaving Gdk.color_parse untouched and instead > add a Gdk.Color.parse API which the converter script will use. I don't think it makes sense to keep a broken ABI, we should just fix it. It's worth it in my opinion.
(In reply to comment #17) > (In reply to comment #8) > > (From update of attachment 190723 [details] [review] [details]) > > Hmm, perhaps we can keep ABI by leaving Gdk.color_parse untouched and instead > > add a Gdk.Color.parse API which the converter script will use. > > I don't think it makes sense to keep a broken ABI, we should just fix it. It's > worth it in my opinion. Fair enough, but can you also add a static method to the Gdk.Color overrkide. Also what about gdk_rgba_parse? It looks like that needs to be fixed also.
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 190729 [details] [details]) > > Why isn't this just part of the conversion script? I don't want people to end > > up not porting their apps to the newer versions of the API. > > It's the opposite of the conversion script. I want to keep on using the older > API until I know that my app works fine with gi. Eg there will be changes for > apis that are not supported. > > I'm not going to stick around with the old api forever, it would be okay to > show big warnings that cannot be removed. I would accept the compat module if deprecation warnings are printed for all API used that would need to change. We should then run through them and add suggestions on the right API to use in the deprecation warning. That will annoy app authors enough to eventually fix their apps. I just don't want people to use it as examples on how to properly write a PyGObject app.
Attachment 190723 [details] pushed as 5b1c875 - Add an override for Gdk.color_parse() Attachment 190724 [details] pushed as 7914d81 - Add a default detail value for Widget.render_icon Attachment 190725 [details] pushed as af8bc9d - Do not always pass in user_data to callbacks. Attachment 190726 [details] pushed as 017fdfc - Add a couple of constructors Attachment 190727 [details] pushed as 583d0b3 - Add vbox/action_area properties Attachment 190728 [details] pushed as f8de9b8 - Add a default parameter to GtkTreeModel.filter_new
Created attachment 195479 [details] [review] Add override for Gtk.ToggleButton This patch removes the stock_id parameter from the Gtk.ToggleButton constructor
Created attachment 195480 [details] [review] Override Gtk.RadioButton This patch changes the constructor of Gtk.RadioButton to expect the group as first argument (instead of label).
Comment on attachment 190726 [details] [review] Add a couple of constructors This patch got committed a while ago.
Comment on attachment 190727 [details] [review] Add vbox/action_area properties This patch got committed a while ago.
Comment on attachment 190728 [details] [review] Add a default parameter to GtkTreeModel.filter_new This patch got committed a while ago.
Review of attachment 195480 [details] [review]: If you currently do Gtk.RadioButton('foo'), "foo" becomes the label. So this would change API. I'm not sure whether Gtk.RadioButton('foo') could even be considered valid, as one should either use Gtk.RadioButton.new_with_label(group, "foo") or Gtk.RadioButton(label="foo") (the GObject constructor). Do we need to be concerned about this?
Comment on attachment 195479 [details] [review] Add override for Gtk.ToggleButton I don't quite understand this patch. Why do you filter out all properties except label and use_underline? buttons have a lot more properties which might be useful to set. Also, why should the stock_id not be set?
It was not intentional to filter out properties, re comment 27. I still think gicompat.py is the way to go, it'll make it feasible to port a large gtk+ applications. It will be much improved, but I'll try to continue that work. I imagine it working in the following way: import gi.pygtkcompat (or so) < warning message printed once > gi.pygtkcompat.enable_warnings() < now we get warnings on each use of an old api, modules, methods etc > It's essential to separate both of these above, as the migration should probably happen in several steps: * Port to PyGTK 2.24 * Port to pygtkcompat * Enable extra warnings on pygtkcompat * Continue porting until there are no warnings * Disable pygtkcompat
(In reply to comment #28) > It was not intentional to filter out properties, re comment 27. > > I still think gicompat.py is the way to go, it'll make it feasible to port a > large gtk+ applications. It will be much improved, but I'll try to continue > that work. > > I imagine it working in the following way: > > import gi.pygtkcompat (or so) > < warning message printed once > > > gi.pygtkcompat.enable_warnings() > < now we get warnings on each use of an old api, modules, methods etc > > > It's essential to separate both of these above, as the migration should > probably happen in several steps: > > * Port to PyGTK 2.24 > * Port to pygtkcompat > * Enable extra warnings on pygtkcompat > * Continue porting until there are no warnings > * Disable pygtkcompat I'd be interested in something like this. I have a large pygtk codebase at university to port.
(In reply to comment #28) > It was not intentional to filter out properties, re comment 27. > > I still think gicompat.py is the way to go, it'll make it feasible to port a > large gtk+ applications. It will be much improved, but I'll try to continue > that work. > > I imagine it working in the following way: > > import gi.pygtkcompat (or so) > < warning message printed once > > > gi.pygtkcompat.enable_warnings() > < now we get warnings on each use of an old api, modules, methods etc > > > It's essential to separate both of these above, as the migration should > probably happen in several steps: > > * Port to PyGTK 2.24 > * Port to pygtkcompat > * Enable extra warnings on pygtkcompat > * Continue porting until there are no warnings > * Disable pygtkcompat This sounds great, specially if we move out most of the overrides to such a compatibility module. Otherwise, I don't think we are going to get good docs for the APIs exposed through PyGObject.
I'll develop this for my own project to make it somewhat feature complete before upstreaming it. You can check out the progress here: http://bazaar.launchpad.net/~stoq-dev/stoq/master/view/head:/stoq/lib/gicompat.py
Created attachment 209065 [details] [review] Require Python 2.6
Created attachment 209066 [details] [review] Remove compatibility macros for Python < 2.6
Created attachment 209067 [details] [review] Use class decorators
Created attachment 209068 [details] [review] Remove unused imports
Created attachment 209069 [details] [review] Remove trailing whitespace
Created attachment 209070 [details] [review] Add a PyGTK compatibility layer This is a module which will make it easier to port large applications such as Stoq to g-i.
I have no problem requiring 2.6, but on the other hand if it is just for "@override" it does not seem an extremely urgen issue. What do other people think? Given that we are almost frozen maybe we can land that next cycle? Feel free to push immediately the cleanup patches (trailing spaces, unused imports)
Review of attachment 209070 [details] [review]: I forgot to mention that I also agree with including this upstream. Maybe let's add a note to the docs
Created attachment 209872 [details] [review] Add a PyGTK compatibility layer This module tries quite a bit harder to maintain compatibility with PyGTK, module names, enums, flags and some API.
Review of attachment 209872 [details] [review]: Nice work on this! This replaces the previous set of uncommitted patches, so would be nice to mark those as obsolete. I definitively like them much better in this separate compat module, as it's easier to deprecate in the future. Some suggestions for the patch: - In the top-level docstring it would be nice to point out that you explicitly have to enable it with .enable_*() - I'd recommend renaming enable() to enable_gobject(), and perhaps adding an enable() which calls all enable_*() functions. The latter might lead to some ImportErrors of course, if e. g. python-gudev is not available; so perhaps the non-GTK ones should not be automatically enabled. - First comment in enable() says "enable gobject" twice, the first one is for glib, though. - + from gi.repository import GObject + sys.modules['gobject'] = GObject This looks more hackish than necessary. How about + from gi.repository import GObject as gobject ? - In the tests, what does this do? + def testButtons(self): + self.assertEquals(Gdk._2BUTTON_PRESS, 5) + self.assertEquals(Gdk.BUTTON_PRESS, 4) This does not seem to test anything in the compat module? - gi/Makefile.am should ship this new module in pygi_PYTHON. As this is a new module, it poses no risk to existing code/clients, so I consider this acceptable at this point of the release cycle.
(In reply to comment #41) > Review of attachment 209872 [details] [review]: > > Nice work on this! This replaces the previous set of uncommitted patches, so > would be nice to mark those as obsolete. I definitively like them much better > in this separate compat module, as it's easier to deprecate in the future. > Done. > Some suggestions for the patch: > > - In the top-level docstring it would be nice to point out that you explicitly > have to enable it with .enable_*() Sure, I'll add some more documentation. > > - I'd recommend renaming enable() to enable_gobject(), and perhaps adding an > enable() which calls all enable_*() functions. The latter might lead to some > ImportErrors of course, if e. g. python-gudev is not available; so perhaps the > non-GTK ones should not be automatically enabled. enable_gobject() wouldn't be quite true though, since it adds compat layer for glib/gobject/gio, thus I selected enable() > > - First comment in enable() says "enable gobject" twice, the first one is for > glib, though. Will fix. > > - + from gi.repository import GObject > + sys.modules['gobject'] = GObject > > This looks more hackish than necessary. How about Well, I have code that does "import gobject", so I don't think there's a way around that is it? Or are you just telling me that I should do a s/GObject/gobject/ ? > - In the tests, what does this do? > > + def testButtons(self): > + self.assertEquals(Gdk._2BUTTON_PRESS, 5) > + self.assertEquals(Gdk.BUTTON_PRESS, 4) > > This does not seem to test anything in the compat module? These were added, but I should probably do a s/Gdk/gdk/ there. > - gi/Makefile.am should ship this new module in pygi_PYTHON. Will do. > > As this is a new module, it poses no risk to existing code/clients, so I > consider this acceptable at this point of the release cycle. Great
(In reply to comment #42) > enable_gobject() wouldn't be quite true though, since it adds compat layer for > glib/gobject/gio, thus I selected enable() Fair enough. > > - + from gi.repository import GObject > > + sys.modules['gobject'] = GObject > > > > This looks more hackish than necessary. How about > > Well, I have code that does "import gobject", so I don't think there's a way > around that is it? Ah, so the intention of this is to make a subsequent "import gobject" work? Then "import gi.r.GObject as gobject" would not work indeed. However, it seems your approach doesn't work here either, at least for me: $ python -c 'import sys; from gi.repository import GObject; sys.modules['gobject'] = GObject; import gobject' Traceback (most recent call last):
+ Trace 229893
Now, your test case does exactly that, so I wonder what I'm doing wrong here. Anyway, I see what this is supposed to achieve, so that looks fine to me.
Attachment 209872 [details] pushed as c60d5ee - Add a PyGTK compatibility layer