[Part 4] SDL3: mouse+key+rect: runtime fixes #3579
Conversation
📝 WalkthroughWalkthroughIntroduces SDL-version-aware behavior across input and rect compatibility: adds deprecation note in a stub, updates keycode mapping for AC Back depending on SDL version, revises mouse relative-mode and visibility handling with explicit NULL-window logic, broadens rect intersection compatibility for SDL3, and adjusts tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Py as Python
participant PG as pygame.mouse (C)
participant SDL as SDL
rect rgba(200,230,255,0.3)
note right of Py: set_relative_mode(mode)
Py->>PG: mouse_set_relative_mode(mode)
alt SDL >= 3.0.0
PG->>PG: win = default window
alt win is NULL
PG-->>Py: raise SDLError ("display.set_mode not called")
else win present
PG->>SDL: SDL_SetWindowRelativeMouseMode(win, mode)
SDL-->>PG: status / error
PG-->>Py: return / raise on error
end
else SDL < 3.0.0
PG->>PG: win may be NULL
alt win is NULL
PG-->>Py: DeprecationWarning
end
PG->>SDL: SDL_SetRelativeMouseMode(mode)
SDL-->>PG: status / error
PG-->>Py: return / raise on error
end
end
rect rgba(230,255,230,0.3)
note right of Py: get_visible()
Py->>PG: mouse_get_visible()
PG->>PG: visible = PG_CursorVisible()
PG->>PG: win = default window (may be NULL)
PG->>SDL: SDL_GetWindowRelativeMouseMode(win or NULL)
SDL-->>PG: false if NULL
PG-->>Py: visible && !relative_mode
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
buildconfig/stubs/pygame/mouse.pyi (1)
295-296: Clarify SDL2 vs SDL3 behavior in versionchanged note.Docs say “deprecated and may error in the future” but runtime already errors on SDL3 and warns on SDL2. Suggest making this explicit.
- .. versionchanged:: 2.5.6 calling this function before calling - :func:`pygame.display.set_mode` is deprecated and may error in the future. + .. versionchanged:: 2.5.6 + On SDL 2.x, calling this function before :func:`pygame.display.set_mode` + emits a ``DeprecationWarning``. On SDL 3.x, it raises ``pygame.error``.test/key_test.py (2)
11-11: Name this to reflect intent and make it immutable.SKIPPED_KEYS_NEW doesn’t convey scope; also make it a frozenset.
-SKIPPED_KEYS_NEW = {"K_MODE"} +# Keys whose alt-name roundtrip (use_compat=False) is layout/SDL-version dependent. +SKIPPED_ALTNAME_ROUNDTRIP = frozenset({"K_MODE"})And update references accordingly (see below).
- if const_name not in SKIPPED_KEYS_NEW: + if const_name not in SKIPPED_ALTNAME_ROUNDTRIP: self.assertEqual(pygame.key.key_code(alt_name), const_val)
173-175: Explain the quit-before-init pattern.Add a short comment so future readers know we’re ensuring a clean slate before init.
- pygame.quit() + # Ensure a fully clean SDL/Pygame state before initializing for this suite. + pygame.quit()src_c/key.c (1)
392-396: Version-gated K_AC_BACK mapping looks correct.Using SDL_VERSION_ATLEAST to select 1073742106 (SDL3) vs 1073742094 (SDL2) preserves stable name while tracking keycode changes.
Consider adding a brief comment citing the SDL3 remap for future maintainers:
#if SDL_VERSION_ATLEAST(3, 0, 0) - {1073742106, "AC Back"}, /* K_AC_BACK */ + {1073742106, "AC Back"}, /* K_AC_BACK (SDL3 moved from 2094 -> 2106) */ #else {1073742094, "AC Back"}, /* K_AC_BACK */ #endifsrc_c/mouse.c (1)
327-331: Changed no-window semantics for get_visible; confirm intended.Now returns PG_CursorVisible() when no window (SDL3 path), whereas older logic returned 0. Looks sensible, but please confirm tests cover get_visible without a window.
Consider an inline note to document this behavior:
- /* If win is NULL, SDL_GetWindowRelativeMouseMode returns false */ + /* If win is NULL, SDL_GetWindowRelativeMouseMode(NULL) returns false, + so result reduces to PG_CursorVisible() (works pre-display.set_mode). */test/mouse_test.py (1)
387-395: Version-gated assertions look right; consider subTest for clearer failures.Wrapping the loop body in subTest(val=val) improves diagnostics if one of the two cases fails without changing behavior.
- for val in (True, False): - if pygame.version.SDL >= (3, 0, 0): - with self.assertRaises(pygame.error): - pygame.mouse.set_relative_mode(val) - else: - with self.assertWarns(DeprecationWarning): - pygame.mouse.set_relative_mode(val) + for val in (True, False): + with self.subTest(val=val): + if pygame.version.SDL >= (3, 0, 0): + with self.assertRaises(pygame.error): + pygame.mouse.set_relative_mode(val) + else: + with self.assertWarns(DeprecationWarning): + pygame.mouse.set_relative_mode(val)src_c/pgcompat_rect.h (1)
11-18: Broadened guard to include SDL3 is correct; define a shared feature macro to avoid drift with .c.Use a single PG_NEEDS_FRECT_COMPAT macro to centralize the condition used here and in pgcompat_rect.c.
-/* SDL 2.0.22 provides some utility functions for FRects */ -#if !(SDL_VERSION_ATLEAST(2, 0, 22)) || SDL_VERSION_ATLEAST(3, 0, 0) +/* SDL 2.0.22 provides some utility functions for FRects. + * For SDL < 2.0.22 and SDL >= 3.0.0 we provide our own for SDL2-compat. */ +#if !(SDL_VERSION_ATLEAST(2, 0, 22)) || SDL_VERSION_ATLEAST(3, 0, 0) +#define PG_NEEDS_FRECT_COMPAT 1 +#endif + +#ifdef PG_NEEDS_FRECT_COMPAT SDL_bool PG_IntersectFRectAndLine(SDL_FRect *rect, float *X1, float *Y1, float *X2, float *Y2); #else #define PG_IntersectFRectAndLine SDL_IntersectFRectAndLine -#endif /* !(SDL_VERSION_ATLEAST(2, 0, 22)) || SDL_VERSION_ATLEAST(3, 0, 0) */ +#endif /* PG_NEEDS_FRECT_COMPAT */src_c/pgcompat_rect.c (1)
4-9: Good rationale on SDL3 edge semantics; switch to the shared guard to keep h/c in sync.Adopt PG_NEEDS_FRECT_COMPAT (defined in the header) here to prevent conditional mismatches between files.
-/* SDL3 changed how the edges are handled. Previously right/bottom edges were +/* SDL3 changed how the edges are handled. Previously right/bottom edges were * considered excluded from the FRect but now they aren't. * For now do SDL2 compat, but consider changing this in the future. * See: https://github.com/pygame-community/pygame-ce/issues/3571 */ -#if !(SDL_VERSION_ATLEAST(2, 0, 22)) || SDL_VERSION_ATLEAST(3, 0, 0) +#ifdef PG_NEEDS_FRECT_COMPAT ... -#endif /* !(SDL_VERSION_ATLEAST(2, 0, 22)) || SDL_VERSION_ATLEAST(3, 0, 0) */ +#endif /* PG_NEEDS_FRECT_COMPAT */Optional: add a focused unit test that hits lines exactly on the right/bottom edges to lock the intended SDL2 semantics while SDL3 fallback is active.
Also applies to: 183-183
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
buildconfig/stubs/pygame/mouse.pyi(1 hunks)src_c/key.c(1 hunks)src_c/mouse.c(3 hunks)src_c/pgcompat_rect.c(2 hunks)src_c/pgcompat_rect.h(1 hunks)test/key_test.py(3 hunks)test/mouse_test.py(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.196Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
📚 Learning: 2025-08-30T21:11:00.196Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.196Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
Applied to files:
src_c/pgcompat_rect.csrc_c/pgcompat_rect.h
📚 Learning: 2025-08-14T02:59:43.863Z
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#0
File: :0-0
Timestamp: 2025-08-14T02:59:43.863Z
Learning: In pygame-ce's C unit testing framework using Unity, the RUN_TEST_PG_INTERNAL macro automatically handles setUp() and tearDown() calls for each test execution, providing automatic test isolation without requiring explicit reset calls between tests.
Applied to files:
test/key_test.py
🔇 Additional comments (3)
test/key_test.py (1)
291-292: Good, targeted skip for layout-dependent key.Conditionally skipping only the alt-name roundtrip for K_MODE keeps coverage strong without fragile assertions.
src_c/mouse.c (1)
623-645: SDL3: error on missing window; SDL2: deprecate-and-continue — LGTM.The split matches the docs intent and provides clear guidance via DeprecationWarning on SDL2.
Ensure test/mouse_test.py asserts:
- SDL3: set_relative_mode(True) without display raises pygame.error.
- SDL2: same call emits DeprecationWarning and succeeds.
test/mouse_test.py (1)
410-410: Initializing a window before toggling relative mode on SDL3 is correct.This avoids the NULL-window error and aligns tests with the new runtime behavior.
set_relative_modewithoutset_modeon SDL2, on SDL3 it errors.PG_IntersectFRectAndLinecompat code on SDL3 for now, for complete SDL2FRectcompat (this can be changed in the future)