Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

post keydown events, closes #1580 #1584

Merged
merged 10 commits into from Sep 5, 2020

Conversation

robertpfeiffer
Copy link
Contributor

@robertpfeiffer robertpfeiffer commented Feb 19, 2020

This obviously (if you look at the code) only covers my own use case, but it should serve as a rough sketch for #1238. The other event types should be analogous.

For #1580

Copy link
Contributor

@charlesej charlesej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few minor comments on the code, but overall this doesn't seem to be working on my system (windows 10/python 3.8.1/pygame 2.0.0.dev7 (SDL: 2.0.10)).

I'm testing using the code in #1238. When posting a key down event, the SDL_PushEvent(&event) on line 1685 is returning a 0 which means the event was filtered (ref: https://wiki.libsdl.org/SDL_PushEvent)

The filtering in pg_event_filter() seems to be happening due to the key.repeat being set.

    else if (type == SDL_KEYDOWN) {
#ifdef WIN32
        SDL_Event inputEvent[2];
#endif /* WIN32 */

        if (event->key.repeat) {
            return 0;
        }

src_c/event.c Outdated Show resolved Hide resolved
src_c/event.c Outdated Show resolved Hide resolved
@robertpfeiffer
Copy link
Contributor Author

robertpfeiffer commented Feb 19, 2020

I'll look into it some more, but I disagree with at least one of the tests.

pygame.event.post(pygame.event.Event(pygame.KEYDOWN)) should fail

@illume
Copy link
Member

illume commented Feb 21, 2020 via email

@robertpfeiffer
Copy link
Contributor Author

I added error handling. Now the tests are broken because they post invalid events.

@illume
Copy link
Member

illume commented Apr 9, 2020

What's the status of this one?

@illume illume mentioned this pull request Apr 9, 2020
@robertpfeiffer
Copy link
Contributor Author

robertpfeiffer commented Apr 9, 2020

@illume The status is that I won't be fixing this for all event types, because 1) that would be a lot of typing and 2) the bug is somewhere else. There is code in event.c already to mangle USEREVENTs into the right type with an event filter, but it's not loaded properly, except when you run the test suite. Forcing keydown events to have all the propertied of SDL2 keydown events ironically breaks the test cases about posting events based on empty python dicts.

If you think a stop-gap is better than nothing, I can clean this up a bit. After all, I use pygame.event.post in my twitch integration tutorial and would love it to work with pygame 2.0. Or maybe you see right away why putting

pygame.display.init()
pygame.display.quit()
pygame.init()

at the beginning works as a workaround. I don't. Maybe it's a corner case in SDL. I just didn't finish this because I thought "there has to be a better way" but then there wasn't.

@illume
Copy link
Member

illume commented Apr 11, 2020

If you think a stop-gap is better than nothing, I can clean this up a bit.

Yeah, thanks. I think a stop-gap is better than nothing.

Forcing keydown events to have all the propertied of SDL2 keydown events ironically breaks the test cases about posting events based on empty python dicts.

I guess if posting events worked before without filling in all the fields we should still allow that?

Here's a checklist for remaining stuff, feel free to edit it or ask me to do some bits.

  • Moving this code into a function?
  • a bit of doc
  • event_test.test_get__event_sequence is failing on appveyor Windows.
======================================================================
FAIL: test_get__event_sequence (pygame.tests.event_test.EventModuleTest)
Ensure get() can handle a sequence of event types.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python27\lib\site-packages\pygame\tests\event_test.py", line 366, in test_get__event_sequence
    self._assertCountEqual(retrieved_events, expected_events)
  File "C:\Python27\lib\site-packages\pygame\tests\event_test.py", line 208, in _assertCountEqual
    self.assertItemsEqual(*args, **kwargs)
AssertionError: Element counts were not equal:
First has 1, Second has 0:  <Event(768-KeyDown {'scancode': 1964920650, 'window': None, 'key': 32, 'unicode': '', 'mod': 8096})>
First has 0, Second has 1:  <Event(768-KeyDown {'key': 32})>
----------------------------------------------------------------------
Ran 36 tests in 0.040s
FAILED (failures=1, skipped=1)
loading pygame.tests.fastevent_test

@robertpfeiffer
Copy link
Contributor Author

Has anybody had any luck fixing the root cause in event.c? Otherwise we need to generate proper SDL events instead of Python-side event dicts.

@robertpfeiffer
Copy link
Contributor Author

I think I made it work. What do the other two mean? documenting that event.post may or may not fail if you post SDL events without the right fields?

@robertpfeiffer
Copy link
Contributor Author

robertpfeiffer commented Aug 24, 2020

The docs are a bit vague on purpose. I don't want to rigorously describe edge case behaviour, because I don't want to support this stopgap indefinitely.
Moving any of this into a function would only make sense if this wasn't a stopgap (which is worth considering, exposing all pygame posted events to SDL2 event filtering).

@robertpfeiffer
Copy link
Contributor Author

I don't see a way to trigger another build...

Copy link
Contributor

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this seems fine, little bit of seemingly unneeded code in there, but it doesn't seem to break anything and it fills in more key data when key events are manually posted to the queue with out all their data. At least so long as you do the display.quit() and display.init() dance beforehand.

Tested it with this:

import pygame

pygame.display.init()  # This is still weird   
pygame.display.quit()  # This is still weird   
pygame.init()

pygame.event.clear()

new_event = pygame.event.Event(pygame.KEYDOWN, key=pygame.K_a)
pygame.event.post(new_event)
print(new_event)
print(pygame.event.poll())

Output on dev10:

<Event(768-KeyDown {'key': 97})>
<Event(768-KeyDown {'key': 97})>

Output with this PR:

<Event(768-KeyDown {'key': 97})>
<Event(768-KeyDown {'unicode': '', 'key': 97, 'mod': 33824, 'scancode': 59098328, 'window': None})>

I think that was the eventual goal of this PR after all the back and forth? I'll approve once the above is resolved one way or the other.

src_c/event.c Outdated Show resolved Hide resolved
@robertpfeiffer
Copy link
Contributor Author

@MyreMylar

You shouldn't need any additional inits and quits to post KEYDOWN events now. Post just uses the SDL2 event queue with the proper event type for keydown events now.

@illume
Copy link
Member

illume commented Sep 5, 2020

LGTM. Doesn't require the init dance anymore.

14:04:28-rene~/dev/pygame/pygame (robertpfeiffer-postkeydownevents)$ python3 postevent_test.py 
pygame 2.0.0.dev11 (SDL 2.0.12, python 3.8.2)
Hello from the pygame community. https://www.pygame.org/contribute.html
<Event(768-KeyDown {'key': 97})>
<Event(768-KeyDown {'unicode': '', 'key': 97, 'mod': 45200, 'scancode': 107738736, 'window': None})>

14:04:37-rene~/dev/pygame/pygame (robertpfeiffer-postkeydownevents)$ python postevent_test.py 
pygame 2.0.0.dev8 (SDL 2.0.12, python 2.7.16)
Hello from the pygame community. https://www.pygame.org/contribute.html
<Event(768-KeyDown {'key': 97})>
<Event(0-NoEvent {})>

are missing or have incompatible values, and unexpected properties may
be silently omitted. In order to avoid this behaviour, custom event
properties should be used with custom event types.
This behaviour is not guaranteed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is confusing to me. What behavior is not guaranteed?

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@illume illume merged commit e40d00d into pygame:master Sep 5, 2020
@illume illume added event pygame.event key pygame.key critical labels Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants