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 653462 - Improve PyGTK compatibility
Improve PyGTK compatibility
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
unspecified
Other All
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2011-06-27 03:49 UTC by Johan (not receiving bugmail) Dahlin
Modified: 2012-03-16 19:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add an override for Gdk.color_parse() (1.04 KB, patch)
2011-06-27 03:49 UTC, Johan (not receiving bugmail) Dahlin
committed Details | Review
Add a default detail value for Widget.render_icon (791 bytes, patch)
2011-06-27 03:49 UTC, Johan (not receiving bugmail) Dahlin
committed Details | Review
Do not always pass in user_data to callbacks. (2.13 KB, patch)
2011-06-27 03:49 UTC, Johan (not receiving bugmail) Dahlin
committed Details | Review
Add a couple of constructors (1.95 KB, patch)
2011-06-27 03:49 UTC, Johan (not receiving bugmail) Dahlin
committed Details | Review
Add vbox/action_area properties (1018 bytes, patch)
2011-06-27 03:49 UTC, Johan (not receiving bugmail) Dahlin
committed Details | Review
Add a default parameter to GtkTreeModel.filter_new (747 bytes, patch)
2011-06-27 03:49 UTC, Johan (not receiving bugmail) Dahlin
committed Details | Review
gicompat.py (4.15 KB, text/plain)
2011-06-27 03:54 UTC, Johan (not receiving bugmail) Dahlin
  Details
Add override for Gtk.ToggleButton (1.04 KB, patch)
2011-09-02 13:59 UTC, Sebastian Pölsterl
none Details | Review
Override Gtk.RadioButton (1.25 KB, patch)
2011-09-02 14:00 UTC, Sebastian Pölsterl
none Details | Review
Require Python 2.6 (723 bytes, patch)
2012-03-06 11:29 UTC, Johan (not receiving bugmail) Dahlin
none Details | Review
Remove compatibility macros for Python < 2.6 (2.69 KB, patch)
2012-03-06 11:30 UTC, Johan (not receiving bugmail) Dahlin
none Details | Review
Use class decorators (13.47 KB, patch)
2012-03-06 11:30 UTC, Johan (not receiving bugmail) Dahlin
none Details | Review
Remove unused imports (897 bytes, patch)
2012-03-06 11:30 UTC, Johan (not receiving bugmail) Dahlin
none Details | Review
Remove trailing whitespace (950 bytes, patch)
2012-03-06 11:30 UTC, Johan (not receiving bugmail) Dahlin
none Details | Review
Add a PyGTK compatibility layer (10.62 KB, patch)
2012-03-06 11:30 UTC, Johan (not receiving bugmail) Dahlin
none Details | Review
Add a PyGTK compatibility layer (12.79 KB, patch)
2012-03-15 18:44 UTC, Johan (not receiving bugmail) Dahlin
committed Details | Review

Description Johan (not receiving bugmail) Dahlin 2011-06-27 03:49:33 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
Comment 1 Johan (not receiving bugmail) Dahlin 2011-06-27 03:49:37 UTC
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.
Comment 2 Johan (not receiving bugmail) Dahlin 2011-06-27 03:49:41 UTC
Created attachment 190724 [details] [review]
Add a default detail value for Widget.render_icon
Comment 3 Johan (not receiving bugmail) Dahlin 2011-06-27 03:49:44 UTC
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.
Comment 4 Johan (not receiving bugmail) Dahlin 2011-06-27 03:49:48 UTC
Created attachment 190726 [details] [review]
Add a couple of constructors

This is for PyGTK compatibility, so that gtk.HBox(True, 2) etc
works.
Comment 5 Johan (not receiving bugmail) Dahlin 2011-06-27 03:49:51 UTC
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
Comment 6 Johan (not receiving bugmail) Dahlin 2011-06-27 03:49:55 UTC
Created attachment 190728 [details] [review]
Add a default parameter to GtkTreeModel.filter_new
Comment 7 Johan (not receiving bugmail) Dahlin 2011-06-27 03:54:59 UTC
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 8 johnp 2011-06-27 18:53:28 UTC
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 9 johnp 2011-06-27 18:54:46 UTC
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 10 johnp 2011-06-27 18:59:14 UTC
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 11 johnp 2011-06-27 19:00:25 UTC
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 12 johnp 2011-06-27 19:01:41 UTC
Comment on attachment 190727 [details] [review]
Add vbox/action_area properties

Please commit to the master, invoke-rewrite and pygobject-2-28 branches
Comment 13 johnp 2011-06-27 19:02:34 UTC
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 14 johnp 2011-06-27 19:03:25 UTC
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 15 johnp 2011-06-27 19:05:29 UTC
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.
Comment 16 Johan (not receiving bugmail) Dahlin 2011-06-27 21:39:42 UTC
(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.
Comment 17 Johan (not receiving bugmail) Dahlin 2011-06-27 21:40:33 UTC
(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.
Comment 18 johnp 2011-06-27 21:54:39 UTC
(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.
Comment 19 johnp 2011-06-28 16:09:48 UTC
(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.
Comment 20 johnp 2011-08-13 10:09:05 UTC
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
Comment 21 Sebastian Pölsterl 2011-09-02 13:59:06 UTC
Created attachment 195479 [details] [review]
Add override for Gtk.ToggleButton

This patch removes the stock_id parameter from the Gtk.ToggleButton constructor
Comment 22 Sebastian Pölsterl 2011-09-02 14:00:10 UTC
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 23 Martin Pitt 2012-02-10 09:37:44 UTC
Comment on attachment 190726 [details] [review]
Add a couple of constructors

This patch got committed a while ago.
Comment 24 Martin Pitt 2012-02-10 09:38:24 UTC
Comment on attachment 190727 [details] [review]
Add vbox/action_area properties

This patch got committed a while ago.
Comment 25 Martin Pitt 2012-02-10 09:38:33 UTC
Comment on attachment 190728 [details] [review]
Add a default parameter to GtkTreeModel.filter_new

This patch got committed a while ago.
Comment 26 Martin Pitt 2012-02-10 09:46:08 UTC
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 27 Martin Pitt 2012-02-10 09:48:46 UTC
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?
Comment 28 Johan (not receiving bugmail) Dahlin 2012-02-29 03:00:38 UTC
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
Comment 29 John Stowers 2012-02-29 09:03:34 UTC
(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.
Comment 30 Tomeu Vizoso 2012-02-29 09:07:19 UTC
(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.
Comment 31 Johan (not receiving bugmail) Dahlin 2012-02-29 17:06:15 UTC
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
Comment 32 Johan (not receiving bugmail) Dahlin 2012-03-06 11:29:59 UTC
Created attachment 209065 [details] [review]
Require Python 2.6
Comment 33 Johan (not receiving bugmail) Dahlin 2012-03-06 11:30:04 UTC
Created attachment 209066 [details] [review]
Remove compatibility macros for Python < 2.6
Comment 34 Johan (not receiving bugmail) Dahlin 2012-03-06 11:30:08 UTC
Created attachment 209067 [details] [review]
Use class decorators
Comment 35 Johan (not receiving bugmail) Dahlin 2012-03-06 11:30:12 UTC
Created attachment 209068 [details] [review]
Remove unused imports
Comment 36 Johan (not receiving bugmail) Dahlin 2012-03-06 11:30:16 UTC
Created attachment 209069 [details] [review]
Remove trailing whitespace
Comment 37 Johan (not receiving bugmail) Dahlin 2012-03-06 11:30:20 UTC
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.
Comment 38 Paolo Borelli 2012-03-06 14:49:01 UTC
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)
Comment 39 Paolo Borelli 2012-03-06 14:58:57 UTC
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
Comment 40 Johan (not receiving bugmail) Dahlin 2012-03-15 18:44:07 UTC
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.
Comment 41 Martin Pitt 2012-03-16 12:16:41 UTC
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.
Comment 42 Johan (not receiving bugmail) Dahlin 2012-03-16 12:20:45 UTC
(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
Comment 43 Martin Pitt 2012-03-16 12:41:10 UTC
(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):
  • File "<string>", line 1 in <module>
NameError: name 'gobject' is not defined

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.
Comment 44 Johan (not receiving bugmail) Dahlin 2012-03-16 19:42:44 UTC
Attachment 209872 [details] pushed as c60d5ee - Add a PyGTK compatibility layer