Add Event Watchers and Filters #3637
Conversation
|
Warning Rate limit exceeded@SuperRedingBros has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds four new event APIs (add/remove event watcher and filter): type stubs, docs, C implementation with internal state and synchronization, watcher wrapper and proxy event constructor, and unit tests covering add/remove semantics and runtime behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Pygame as Pygame Event System
participant Filters as User Filters
participant Watchers as User Watchers
rect rgb(230,240,255)
note over App,Pygame: Register handlers
App->>Pygame: add_event_filter(filter_fn)
Pygame->>Filters: store filter_fn (thread-safe)
App->>Pygame: add_event_watcher(watcher_fn)
Pygame->>Watchers: store watcher_fn (thread-safe)
end
rect rgb(220,255,220)
note over App,Pygame: Event dispatch
App->>Pygame: post_event(event)
Pygame->>Pygame: check enabled
alt enabled
loop filters
Pygame->>Filters: filter_fn(event)
alt filter returns true
Pygame->>Pygame: drop event (stop)
else
Pygame->>Pygame: continue
end
end
loop watchers
Pygame->>Watchers: watcher_fn(converted Event)
end
Pygame->>Pygame: deliver event to normal handlers
end
end
rect rgb(255,235,220)
note over App,Pygame: Unregister handlers
App->>Pygame: remove_event_filter(filter_fn)
Pygame->>Filters: remove filter_fn (thread-safe)
App->>Pygame: remove_event_watcher(watcher_fn)
Pygame->>Watchers: remove watcher_fn (thread-safe)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src_c/event.c (1)
718-751: Consider optimizing Event object creation in filter loop.Line 731 creates a new Event object for every filter in every iteration. This could impact performance when multiple filters are registered and many events are processed.
Consider creating the Event object once before the loop and reusing it for all filter calls, unless there's a specific reason each filter needs a fresh Event object.
# Pseudo-code suggestion: PyObject *eventObj = pg_eventNew_no_free_proxy(event); for (Py_ssize_t i = 0; i < totalUserFilters; i++) { PyObject *filter = PyList_GetItem(userFilters, i); PyObject *returnValue = PyObject_CallOneArg(filter, eventObj); # ... error handling and skip logic } Py_DECREF(eventObj);However, verify this is safe - if filters can modify the Event object, creating a new one each time may be intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
buildconfig/stubs/pygame/event.pyi(2 hunks)docs/reST/ref/event.rst(1 hunks)src_c/doc/event_doc.h(1 hunks)src_c/event.c(6 hunks)test/event_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/event_test.py (2)
src_c/event.c (1)
event(2751-2822)buildconfig/stubs/pygame/event.pyi (8)
add_event_watcher(56-56)clear(47-47)post(54-54)Event(28-29)poll(44-44)remove_event_watcher(57-57)add_event_filter(58-58)remove_event_filter(59-59)
src_c/event.c (1)
src_c/include/pythoncapi_compat.h (1)
PyObject_CallOneArg(398-401)
🪛 Ruff (0.14.3)
test/event_test.py
942-942: Unused function argument: event
(ARG001)
958-958: Unused function argument: event
(ARG001)
976-976: Unused function argument: event
(ARG001)
🔇 Additional comments (11)
test/event_test.py (4)
938-970: LGTM!The test correctly verifies that event watchers are called when events are posted. The static analysis warning about unused
eventparameters is a false positive - the test intentionally doesn't use the event parameter, as it's only verifying that the watchers are invoked.
971-990: LGTM!The test correctly verifies that removing an event watcher prevents it from being called on subsequent events.
992-1011: LGTM!The test correctly verifies that event filters can selectively block events from reaching the queue.
1013-1039: LGTM!The test correctly verifies that removing an event filter restores normal event processing and that attempting to remove a non-existent filter raises
ValueError.src_c/event.c (7)
103-104: LGTM!The global
userFilterslist is properly declared and initialized at line 814 inpgEvent_AutoInit.
523-525: LGTM!The forward declaration is necessary since
pg_eventNew_no_free_proxyis used in the event filter before its definition.
2547-2583: LGTM - Complex but correct reference counting.The function correctly manages the
dict_proxyreference counting to prevent double-free issues when multiple parts of the codebase use the same event. The temporary manipulation ofdo_free_at_endis necessary to ensure the proxy isn't freed prematurely.
2585-2625: LGTM!The watcher wrapper correctly handles GIL acquisition, error handling, and SDL2 compatibility. Errors are appropriately printed to stderr and cleared to prevent them from propagating into SDL's event processing.
2627-2648: LGTM!The function correctly validates the callable and registers it with SDL. Returning the argument with INCREF enables decorator usage.
2665-2687: LGTM!The function correctly handles thread safety with GIL and mutex, and returns the argument with INCREF for decorator usage.
2738-2745: LGTM!The method table entries correctly expose the new event watcher and filter functions to Python.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/reST/ref/event.rst (1)
471-490: Consolidate version markers and minor documentation issues.All four new function definitions use
versionaddedold:: todoas a placeholder. These need to be updated to:
- Replace "todo" with the actual version number this PR targets (e.g.,
3.0.0or the next release)- Consider whether
versionaddedoldis the intended directive; elsewhere in the file,versionaddedis used for new features (without the "old" suffix)Additionally, the typos on lines 481 and 515 ("seperate" → "separate") flagged in previous reviews have already been corrected in this revision.
Please verify:
- What version number should replace "todo"?
- Should the directive be
versionadded(to match modern sections) orversionaddedold(for consistency with legacy event docs)?Once confirmed, apply a consolidated diff for all four functions:
- .. versionaddedold:: todo + .. versionadded:: X.X.XAlso applies to: 492-503, 505-523, 525-535
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/reST/ref/event.rst(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-11T10:24:52.717Z
Learnt from: damusss
Repo: pygame-community/pygame-ce PR: 3604
File: buildconfig/stubs/pygame/surface.pyi:298-299
Timestamp: 2025-10-11T10:24:52.717Z
Learning: For the pygame-community/pygame-ce repository, batch similar documentation or terminology improvements together in a single comment rather than pointing them out individually. This helps avoid unnecessary commit spam.
Applied to files:
docs/reST/ref/event.rst
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src_c/event.c (2)
2650-2663: Critical: Missing Py_INCREF for returned Py_None.Line 2662 returns
Py_Nonewithout incrementing its reference count, which will cause reference counting errors and potential crashes.Apply this diff:
- return Py_None; + Py_RETURN_NONE;
2689-2707: Critical: Missing Py_INCREF for returned Py_None.Line 2706 returns
Py_Nonewithout incrementing its reference count, which will cause reference counting errors and potential crashes.Apply this diff:
- Py_RETURN_NONE; + Py_RETURN_NONE;Note: The
Py_RETURN_NONEmacro handles the reference count correctly.
🧹 Nitpick comments (2)
src_c/event.c (2)
2674-2678: Minor: Unnecessary GIL acquisition in Python-called function.
pg_event_add_filteris called from Python via the module method table, so the GIL is already held. ThePyGILState_Ensure()andPyGILState_Release()calls at lines 2674 and 2678 are unnecessary and add overhead.Apply this diff:
if (PyCallable_Check(arg)) { - PyGILState_STATE gstate = PyGILState_Ensure(); PG_LOCK_EVFILTER_MUTEX PyList_Append(userFilters, arg); PG_UNLOCK_EVFILTER_MUTEX - PyGILState_Release(gstate); }
2694-2705: Minor: Unnecessary GIL acquisition in Python-called function.Similar to
pg_event_add_filter,pg_event_remove_filteris called from Python, so the GIL is already held. ThePyGILState_Ensure()andPyGILState_Release()calls are unnecessary.Apply this diff:
VIDEO_INIT_CHECK(); - PyGILState_STATE gstate = PyGILState_Ensure(); PG_LOCK_EVFILTER_MUTEX Py_ssize_t index = PySequence_Index(userFilters, arg); if (index == -1) { PG_UNLOCK_EVFILTER_MUTEX PyErr_SetString(PyExc_ValueError, "event filter not found"); - PyGILState_Release(gstate); return NULL; } PySequence_DelItem(userFilters, index); PG_UNLOCK_EVFILTER_MUTEX - PyGILState_Release(gstate); Py_RETURN_NONE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/event.c(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-13T12:39:19.130Z
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.130Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/event.c
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/event.c
🧬 Code graph analysis (1)
src_c/event.c (1)
src_c/include/pythoncapi_compat.h (1)
PyObject_CallOneArg(398-401)
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/event.c(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-13T12:39:19.130Z
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.130Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/event.c
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/event.c
🧬 Code graph analysis (1)
src_c/event.c (1)
src_c/include/pythoncapi_compat.h (1)
PyObject_CallOneArg(398-401)
🔇 Additional comments (1)
src_c/event.c (1)
815-818: LGTM: Proper initialization and error handling.The initialization of
userFilterscorrectly checks for NULL and raises an appropriate error if allocation fails.
There was a problem hiding this comment.
Hello and welcome, thanks for taking the time to contribute to pygame-ce 🥳 It is really impressive to see a new contributor directly tacking deep C code 😎
Some general notes:
- Ideally it would have been better if you opened a new issue to discuss the API before implementing it.
- SDL3, which is what we are porting pygame-ce to, has the new main callbacks API. Ideally, any new API we add must follow SDL3 style/behaviour even if implemented in the current SDL2 codebase.
- For this PR, I am not opposed to adding event callbacks but I believe there should be only one thing added, and that should be equivalent to SDL3's
SDL_AppEvent. Adding both watchers and filters is exposing too much details to the users and is potentially more confusing IMHO.
This is just my idea but here's what a pygame-ce app using a main callback API could look like in the future (of course, this is not fully fleshed out either)
import pygame
@pygame.app_event
def handle_event(event: pygame.Event):
if event.type == pygame.QUIT:
raise SystemExit
... # handle any other event here
@pygame.app_iterate
def handle_loop(window: pygame.Window):
pygame.draw.xxx(window.get_surface(), ...)
... # do other things
# no need to call pygame.display.update/flip or window flip
pygame.launch([size], [title], [other set_mode params])We were planning to tackle this when we got to using SDL3 completely.
Fair enough, I'll try to keep that in mind for the future. Should I make a new issue to discuss this there?
I do remember reading that somewhere, would you suggest making the event watchers behave more like SDL_AppEvent?
Also pretty fair, I mostly just added the filters because I considered them an adjacent feature that I figured I would add while I was adding the event watchers. If you think that they are making things more confusing I have no problem with removing them. |
yes that is what I would personally prefer.
That would be appreciated, I would also like to hear other contributors opinions on this API. |
2 participants
Adds
pygame.event.add_event_watcher,pygame.event.remove_event_watcher,pygame.event.add_event_filter, andpygame.event.remove_event_filteras loose wrappers around the corresponding SDL functions as a potential new way to handle events.This has two primary rationales.
Firstly, to allow using an event handler paradigm if desired while having no impact on the standard event loop structure.
As this PR does nothing without explicitly opting into it opens up more options while not having any impact on other methods of handling events.
Secondarily, event watchers can be used as an effective solution for the quirk of the Windows version of SDL of the main thread freezing while the OS is sending resize or window movement events. This allows for more advanced users who may be impacted by the issue to resolve it without having to mess with the windows APIs directly. An example of the issue from upstream pygame can be found here.
A simple example that will print out all events that are added to the queue could be done like this:
and an example of a filter that will block only left click events:
I have also created a more complex demo implementation showcasing the event watchers as well as demonstrating both the windows issue and an example solution for the issue here.
Summary by CodeRabbit
New Features
Documentation
Tests