Repository navigation
Major improvements to pygame.event introduction documentation - #3738
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRewrites and restructures the event reference: clarifies the event queue dependency on the video/window system and per-frame processing, makes event objects attribute-centric, adds per-event attribute tables, modernizes window/controller/IME/file-drop/Android events, and consolidates versioning notes. (50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reST/ref/event.rst`:
- Line 224: Fix the spelling in the versionadded directive: replace the
misspelled word "Adroid" with "Android" in the line beginning with "..
versionadded:: 2.1.3" (the text containing "Adroid events, ``precise_x`` and
``precise_y`` to ``MOUSEWHEEL`` events") so the directive reads "..
versionadded:: 2.1.3 Android events, ``precise_x`` and ``precise_y`` to
``MOUSEWHEEL`` events".
- Line 60: Correct the spelling of "instantianting" to "instantiating" in the
documentation sentence that references the Event constructor (the phrase
containing ":func:`pygame.event.Event()`"); update that occurrence so the
sentence reads "instantiating :func:`pygame.event.Event()` themselves, usually
paired" (or equivalent grammatical adjustment) to fix the typo.
- Around line 94-133: Update the event attribute listings to indicate the
presence of a window attribute for events whose implementation sets it: include
"window" in the attribute list (or add a parenthetical note) for KEYDOWN, KEYUP,
MOUSEMOTION, MOUSEBUTTONUP, MOUSEBUTTONDOWN, MOUSEWHEEL, TEXTEDITING, TEXTINPUT,
DROPFILE, DROPBEGIN, DROPCOMPLETE, DROPTEXT, FINGERMOTION, FINGERDOWN, and
FINGERUP so the docs explicitly state the window that received the event.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6520271-3286-47bf-8dfd-c7fb4b72833d
📒 Files selected for processing (1)
docs/reST/ref/event.rst
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reST/ref/event.rst`:
- Around line 187-190: Remove the incorrect "(x, y attributes)" annotation from
the WINDOWMOVED and WINDOWRESIZED entries so only WINDOWSIZECHANGED retains the
"(x, y attributes)" note; update the event list block that contains WINDOWMOVED,
WINDOWSIZECHANGED, and WINDOWRESIZED to match the source comment in
src_c/event.c (only WINDOWSIZECHANGED should show x,y attributes).
- Line 92: The docs line listing a `code` attribute for USEREVENT is misleading;
update the USEREVENT entry to either remove the `code` mention or reword it to
state that USEREVENT has no standard system attributes and that users may attach
arbitrary attributes as needed (referencing the USEREVENT symbol and the `code`
attribute in the entry so the maintainer knows which token to change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4846e5c9-0093-4f9c-914e-813cc7d1aa28
📒 Files selected for processing (1)
docs/reST/ref/event.rst
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/reST/ref/event.rst (1)
92-93:⚠️ Potential issue | 🟡 MinorClarify that
USEREVENThas no standard attributes.Removing the legacy
codeimplication helps, but this row still reads like a normal system-event entry. I’d explicitly say thatUSEREVENTis just the first value in the user-defined range and has no guaranteed attributes, so readers do not infer an omitted default attribute set from the table.📝 Suggested wording
- USEREVENT (Starting type of the user defined - event types) + USEREVENT (No guaranteed attributes; starting value + of the user-defined event type range, + ``USEREVENT`` to ``NUMEVENTS - 1``)Based on learnings, "USEREVENT has no guaranteed system attributes. It is only the starting value of the user-defined event type range (USEREVENT to NUMEVENTS-1). The
codeattribute listed in older documentation was erroneous/legacy and does not apply."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reST/ref/event.rst` around lines 92 - 93, Update the table row for USEREVENT to explicitly state it has no guaranteed system attributes and is only the starting value of the user-defined event range; mention the range (USEREVENT to NUMEVENTS-1) and that the older implication of a `code` attribute was legacy/erroneous and does not apply, so readers won't infer any default attribute set for USEREVENT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/reST/ref/event.rst`:
- Around line 92-93: Update the table row for USEREVENT to explicitly state it
has no guaranteed system attributes and is only the starting value of the
user-defined event range; mention the range (USEREVENT to NUMEVENTS-1) and that
the older implication of a `code` attribute was legacy/erroneous and does not
apply, so readers won't infer any default attribute set for USEREVENT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62da0181-3de9-43a8-b366-3ecd5713f20a
📒 Files selected for processing (1)
docs/reST/ref/event.rst
routines -> functions, clearer "If it has not been initialized" - what is "it"? "and process them" unnecessary and not good grammar in context We wouldn't recommend filtering, so why does it need a paragraph? Clarify bit about events still needing to be pumped Events are not in "stack", they are on a queue. __dict__ is irrelevant Simplify wording of custom_type() bit. Why does event_name need a sentence to be hyped? DROPTEXT works on Windows, tested.
|
@damusss I love stuff like this. I had a couple extra improvements on a read through, but rather than telling you them I just made my own commit. Please take a look. Change descriptions, in order, very concise: |
|
@Starbuck5 thank you for the commit! The introductory part needed an extra improvement. |
The current version is one of the most confusing documentations I've ever read. This PR properly organizes the information in a readable and concise manner. Main issues:
The current version uses a "history" approach, as if it was narrating how events changed over time, which is not useful for someone learning how to handle events. I've slightly edited and separated with headers the text walls and organized the event types in 3 lists (the first one separates events by category). I only added descriptions for the events that really needed them, connecting them to the respecting types/attributes. The documentation about
TEXT*events is completely new. I genuinely had no idea what those events were, and I honestly don't expect most people to know, therefore I think the notes are useful.Detailed breakdown of decisions:
DROP*/MIDI*/KEYMAPCHANGED/CLIPBOARDUPDATEevent descriptions can be made concise next to the event typeinstance_id..." versionchanged tags exist to replace thisLOCALECHANGEDis an event sent when user locale changes" is like saying the QUIT event is sent when the app quitsWINDOWRESIZED/WINDOWSIZECHANGEDambiguity was important to clarify