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 727810 - Support setattr pass-through based on union discrimination for Gdk.Event
Support setattr pass-through based on union discrimination for Gdk.Event
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
3.10.x
Other Linux
: Normal enhancement
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2014-04-08 10:09 UTC by Marcin Szewczyk
Modified: 2014-08-23 04:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Different approaches on emitting Event/EventKey (2.00 KB, text/x-python)
2014-04-08 10:09 UTC, Marcin Szewczyk
  Details
Gdk.Event: Include GdkEventType in __repr__ (1.45 KB, patch)
2014-04-14 16:27 UTC, Christoph Reiter (lazka)
committed Details | Review
Gdk.Event: Override __setattr__ to set fields base on the (2.28 KB, patch)
2014-04-14 16:30 UTC, Christoph Reiter (lazka)
committed Details | Review
Set deprecation warning if arguments get passed to gi.Boxed.__init__() (5.40 KB, patch)
2014-04-14 16:33 UTC, Christoph Reiter (lazka)
reviewed Details | Review
Raise TypeError if arguments get passed to Boxed.__init__ (3.72 KB, patch)
2014-04-14 21:52 UTC, Christoph Reiter (lazka)
committed Details | Review

Description Marcin Szewczyk 2014-04-08 10:09:29 UTC
Created attachment 273774 [details]
Different approaches on emitting Event/EventKey

I have no idea how to emit a correct event in analogy to PyGTK methods.

I attach an example code with comments to give an idea how different approaches have failed.

The direct reason for this bug report is an error message in one variant:
TypeError: cannot set a structure which has no well-defined ownership transfer rules
Comment 1 Christoph Reiter (lazka) 2014-04-08 10:43:27 UTC
##################################################
def send_key_event(widget, **kwargs):
    """Returns True if the event was handled"""

    ev = Gdk.Event()
    ev.type = Gdk.EventType.KEY_PRESS
    ev.any.window = win.get_window()

    for key, value in kwargs.items():
        setattr(ev.key, key, value)

    return widget.event(ev)

print(send_key_event(entry, keyval=ord("A")))
##################################################
Comment 2 Marcin Szewczyk 2014-04-08 11:24:33 UTC
Thank you. I have missed the possibility of setting those keys directly.
Comment 3 Christoph Reiter (lazka) 2014-04-10 22:04:07 UTC
The reason why setting fields of the Gdk.Event enum is weird is that it does some magic getting the enum field members depending on the type but doesn't do the same for setting values. E.g. getattr(ev, "window") works while setattr(ev, "window", ..) doesn't.

Also Gdk.Event.__new__ ignores all arguments. "Gdk.Event(Gdk.EventType.KEY_PRESS)" does not set the type, only "Gdk.Event.new(Gdk.EventType.KEY_PRESS)" does.

I propose the following changes:

1) The __getattr__ magic of Gdk.Event should be mirrored to __setattr__.

2) Passing arguments to union constructors should result in a deprecation warning since they get just ignored atm. This is handled in _boxed_new()/pygi-boxed.c as far as I can see.
Comment 4 Simon Feltman 2014-04-14 03:34:57 UTC
(In reply to comment #3)
> ...
> I propose the following changes:
> 
> 1) The __getattr__ magic of Gdk.Event should be mirrored to __setattr__.
> 
> 2) Passing arguments to union constructors should result in a deprecation
> warning since they get just ignored atm. This is handled in
> _boxed_new()/pygi-boxed.c as far as I can see.

Sounds good to me. Also note there is a union discriminator API in GI but I don't think it is actually implemented on the scanner/typelib side of things:

https://developer.gnome.org/gi/1.40/gi-GIUnionInfo.html#g-union-info-get-discriminator-type
Comment 5 Simon Feltman 2014-04-14 03:52:33 UTC
Additionally it would be nice to implement __repr__ to return something a bit more descriptive like: Gdk.Event.new(Gdk.EventType.KEY_PRESS)
Comment 6 Christoph Reiter (lazka) 2014-04-14 16:27:41 UTC
Created attachment 274287 [details] [review]
Gdk.Event: Include GdkEventType in __repr__

Adds event type to the repr, but not in the format you proposed, since repr() of GdkEventType would also have to be changed. So I kept it simple and consistent for now.
Comment 7 Christoph Reiter (lazka) 2014-04-14 16:30:10 UTC
Created attachment 274288 [details] [review]
Gdk.Event: Override __setattr__ to set fields base on the

Turned out there was a test for this, but it was buggy and only set Gdk.Event.__dict__

side note: the UNION_MEMBERS list is out of date, there are new event types in gdk..
Comment 8 Christoph Reiter (lazka) 2014-04-14 16:33:45 UTC
Created attachment 274289 [details] [review]
Set deprecation warning if arguments get passed to gi.Boxed.__init__()

This checks on __init__, thus all overrides using __new__ need to filter out __init__ args.

Not 100% sure if this is the best approach, feedback welcome.
Comment 9 Simon Feltman 2014-04-14 18:04:16 UTC
Review of attachment 274287 [details] [review]:

Looks good.
Comment 10 Simon Feltman 2014-04-14 18:05:04 UTC
Review of attachment 274288 [details] [review]:

LGTM.
Comment 11 Simon Feltman 2014-04-14 19:42:33 UTC
Review of attachment 274289 [details] [review]:

This patch is related to bug 705810:
https://git.gnome.org/browse/pygobject/commit/?id=2f2069c9efcd8

Which removed an args/kwargs check from the GIBoxed.__new__ (_boxed_new). Unfortunately the reason for that removal wasn't clearly noted in the commit log (and should have been put in a separate patch). But it allowed removal of various __new__ implementations for things like Gdk.Event and Gdk.Color. I think needing to implement __new__ and __init__ for those boxed overrides annoyed me, but the removal caused this bug to occur. I like your approach of putting the error in _boxed_init rather than in _boxed_new. Similarly we might be able to get away with a RuntimeWarning or even an exception instead of PyGIDeprecationWarning since Boxed creation with extra args used to be an exception pre-3.12. 

The difference between putting the error in _boxed_new vs _boxed_init is with _boxed_new, any overridden __init__ which accepts arguments also needs to override __new__ to ignore args. With _boxed_init giving the error the reverse is true when overriding __new__... it requires an __init__ override which ignores args, but only having an __init__ override does not require a __new__ override. I think the error in _boxed_init (what you've done) seems cleaner as only overriding __init__ is arguably a more common Python idiom.

So this patch seems fine with the addition of a possibly harder error and Pango.Layout not needing the override.

::: gi/overrides/Pango.py
@@ +48,3 @@
 
+    def __init__(self, *args, **kwargs):
+        return super(Layout, self).__init__()

Pango.Layout is a GObject so this shouldn't be needed.
Comment 12 Christoph Reiter (lazka) 2014-04-14 21:52:12 UTC
Created attachment 274308 [details] [review]
Raise TypeError if arguments get passed to Boxed.__init__
Comment 13 Simon Feltman 2014-04-15 02:16:57 UTC
Attachment 274287 [details] pushed as 78a0508 - Gdk.Event: Include GdkEventType in __repr__
Comment 14 Simon Feltman 2014-08-23 04:57:47 UTC
Note I changed this back to use the warning code you added from the patch in comment #8 with the difference that it is a TypeError not a DeprecationWarning. The exception caused some fallout in at least Gramps.

https://git.gnome.org/browse/pygobject/commit/?id=27a1467