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

Simplify c-side of events sub-module. (Part 1) #3000

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

gresm
Copy link
Contributor

@gresm gresm commented Jul 16, 2024

This PR moves some of functionality from c to python.

@damusss
Copy link
Contributor

damusss commented Jul 17, 2024

First of all, I'd like to say that I love where this is going :)
I have a few modifications that I (personally) would prefer:

  • is there a reason why _NAMES_MAPPING has raw integers instead of pygame.X event constants? I tried doing that, and it worked perfectly fine. In cursors.py, the dictionary also has pygame constants
  • I think your stub situation can be improved: IMO there should not be _event.pyi, rather, there should be only event.pyi containing both from _event.c and from event.py (there is a stub file for cursors.py, even tho it's made in python!). The stubs are fake code, we don't need to tell the user that event is actually split between C and py, what matters is what is accessible in it. Again I tried that myself, added init, quit and modified a signature and the stubcheck.py with mypy passes for me, that's why I'm suggesting it.

Tell me what you think about this

@gresm
Copy link
Contributor Author

gresm commented Jul 17, 2024

@damusss Thanks for the comment - I'll update _NAMES_MAPPING shortly. As for _event.pyi, I'm using it as a roadmap of what's left to do, but I'm not going to completely remove pygame._event, just change some of the functionality, so probably this file will be included.

@damusss
Copy link
Contributor

damusss commented Jul 17, 2024

@damusss Thanks for the comment - I'll update _NAMES_MAPPING shortly. As for _event.pyi, I'm using it as a roadmap of what's left to do, but I'm not going to completely remove pygame._event, just change some of the functionality, so probably this file will be included.

Alright, I'll check the stub situation when you are closer to finish then. Nice work so far :>

@gresm
Copy link
Contributor Author

gresm commented Jul 19, 2024

(Why the commits duplicated after I resolved merge conflict?)

@damusss
Copy link
Contributor

damusss commented Jul 19, 2024

No actually why are part of the commits not yours?? what even happened? It looks like you are adding the deprecator decorator

@gresm
Copy link
Contributor Author

gresm commented Jul 19, 2024

I merged commits from upstream using git pull origin main --rebase and likely this --rebase is the cause of inclusion of these commits... I'll try to discard this merge and re-attempt doing it.

@gresm gresm force-pushed the events-in-python branch 2 times, most recently from b808ac9 to 931cf5e Compare July 19, 2024 20:24
@gresm
Copy link
Contributor Author

gresm commented Jul 19, 2024

Done, even though they still appear in the conversation, I removed them from the commits tab (I guess that conversation side just logs every change and not exactly those that truly will be merged?).

@gresm
Copy link
Contributor Author

gresm commented Jul 19, 2024

I think I'll split this PR in two parts:

  1. Migrating stuff related to Event class. (nearly done, c-api documentation still needed to be updated)
  2. Migrating the monstrosity of events filtering. (not touched yet)

@gresm gresm changed the title Simplify c-side of events sub-module. Simplify c-side of events sub-module. (Part 1) Jul 19, 2024
@gresm gresm marked this pull request as ready for review July 19, 2024 21:28
@gresm gresm requested a review from a team as a code owner July 19, 2024 21:28
@yunline yunline added Code quality/robustness Code quality and resilience to changes event pygame.event labels Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality/robustness Code quality and resilience to changes event pygame.event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants