Misc bug fixes (display, window, image, pixelarray) #3591
Conversation
📝 WalkthroughWalkthroughUpdates adjust control-flow conditions across display and window handling, correct RGB channel loss/shift in 32bpp packing, and fix surface lock/unlock logic. No public API signatures changed. Warnings on OS-forced resize are triggered on size mismatch only. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SDL as SDL Event Loop
participant EV as pg_ResizeEventWatch (SDL3)
participant DISP as display.c handlers
SDL->>EV: SDL_Event e
alt e.type NOT in SDL_WINDOW_EVENT range
EV-->>SDL: return 0 (no processing)
else e.type in window event range
EV->>DISP: handle window event
DISP-->>EV: processed
EV-->>SDL: return 1
end
note over EV,DISP: Condition inverted to early-return on non-window events
sequenceDiagram
autonumber
participant PA as _make_surface
participant S as SDL_Surface
PA->>S: SDL_MUSTLOCK(S)?
alt Must lock == true
PA->>S: SDL_LockSurface(S)
PA->>S: ... write pixels ...
PA->>S: SDL_UnlockSurface(S)
else Must lock == false
PA->>S: ... write pixels without lock ...
end
note over PA,S: Lock/unlock performed only when required
sequenceDiagram
autonumber
participant API as window_set_modal_for
participant CORE as PG_SetWindowModalFor
API->>CORE: set modal relationship
CORE-->>API: int rc
alt rc < 0
API-->>API: treat as error
else rc >= 0
API-->>API: success path
end
note over API: Error condition changed to (rc < 0)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src_c/display.c (1)
1687-1695: SDL3 vsync check is inverted — triggers error when vsync is actually enabled
SDL_GetRenderVSync(pg_renderer, &has_vsync)setshas_vsyncnon‑zero when vsync is ON. The code errors whenvsync && has_vsync, which is backwards. It should error when vsync was requested but is NOT enabled.Apply:
- if (vsync && has_vsync) { + if (vsync && !has_vsync) { PyErr_SetString(pgExc_SDLError, "could not enable vsync"); _display_state_cleanup(state); goto DESTROY_WINDOW; }
🧹 Nitpick comments (1)
src_c/pixelarray_methods.c (1)
161-164: Lock only when required: inversion fix is correctLock/unlock now guard on
SDL_MUSTLOCK(new_surf); that’s the right behavior.Consider checking the return value of
SDL_LockSurface(new_surf)and raising on failure to avoid copying into an unlocked surface on error.Also applies to: 240-242
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src_c/display.c(2 hunks)src_c/image.c(1 hunks)src_c/pixelarray_methods.c(2 hunks)src_c/window.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#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/display.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: x86_64
- GitHub Check: arm64
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: x86
- GitHub Check: dev-check
🔇 Additional comments (4)
src_c/image.c (1)
701-703: Fix G/B channel loss application in 32bpp RGB pathUsing
GlossandBloss(notRloss) fordata[1]anddata[2]is correct. Approving.Please add/adjust a unit test that exercises 32bpp RGB surfaces and verifies green/blue bytes match the format masks/shift/loss.
src_c/display.c (2)
897-901: Event watch early‑return condition is now correctInverting the check to return early on non‑window events is right and avoids extra branching.
1832-1846: Resize warning gate: width/height mismatch check is fineThe simplified mismatch check is reasonable under the existing gating. Approved.
src_c/window.c (1)
476-478: Correct return-code check for PG_SetWindowModalFor — verifiedSwitching to "< 0" correctly treats negative as error (SDL3-style). The provided rg output shows only the call at src_c/window.c:476–478 and no other call sites use truthiness checks.
A random collection of bugfixes. Some of these bugs were introduced fairly recently by me during the SDL3 porting, good thing they didn't make it to a release. These bugs were caught by coderabbit.