Add Window.handle
#3219
Add Window.handle
#3219
Conversation
damusss
added
New API
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
SDL_syswm.h header is removed in SDL3. The way to access the wm info has also changed, and I believe that also changes what values are exported by the SDL side.
I think that going forward any new API added should be SDL3 style, and any thing in the grey area should not be exported at all.
Alright, good to know. I'll study the new system that SDL3 uses and adjust the Window code for it. I believe I should keep the SDL2 version (unless it has more dictionary keys than sdl3 will support) so it can be added before pygame 3. |
|
@ankith26 @Starbuck5
|
|
Perhaps this PR should wait for the "port window to SDL3" PR to be made and merged first |
I suppose that makes sense. Do you need special competence to pull off one of those PRs, or is it just about following the migration guide (and making different codepaths/creating compat macros)? (if so, I could even try to make it, otherwise I'll wait for you or starbuck). :) Almost forgot, do you guys test your PR to actually compile with SDL3 locally, or do you just trust yourselves? About the first option (more likely), how do you do it? My WSL technically counts as linux if that helps |
|
We do test that the patch compiles on SDL3. It can be done locally, but you have to compile SDL3 from source if you are gonna go the WSL way (don't worry, it's not that hard). BTW, we also have SDL3 CI now that can do the testing on windows+mac+linux. Basically the thing is we are yet to figure how to properly deal with the pixelformat changes uniformly, once that is done we can apply the same strategy everywhere and port the next wave of modules |
|
Well actually, |
|
Alright then, nice, thanks |
|
So even if this code works great on SDL3, why should we expose new API in the SDL2 style? |
We don't have to expose it in a SDL2 style, but the window manager information is very dependent on the os so how would you provide the info without a dictionary? |
|
Having something called "wm info" is clearly an SDL2 construction. Well, looking at the SDL code you have it looks like they expose it now as properties on the window object itself. |
|
I have marked as a draft, because with uncertainty about API as well as merge conflicts this is not ready to merge. |
That's true, but each property is going to exist or not based on the OS and window manager. They are not generic properties that belong to almost every window like a position and a size. The dictionary reflects how uncertain the attributes are to exist. plus they are all very low level, so putting them besides common attributes would look "ugly" imo. |
|
Ok, this API was kinda ugly so I replaced it with import pygame
DISPLAY = True
if DISPLAY:
screen = pygame.display.set_mode((500, 500))
else:
win = pygame.Window(size=(500, 500))
screen = win.get_surface()
clock = pygame.Clock()
while True:
for e in pygame.event.get():
if e.type == pygame.QUIT or e.type == pygame.WINDOWCLOSE:
pygame.quit()
raise SystemExit
if e.type == pygame.KEYDOWN:
if DISPLAY:
print(pygame.display.get_wm_info()["window"])
else:
print(win.handle)
clock.tick(60)
if DISPLAY:
pygame.display.flip()
else:
win.flip() |
|
I fixed the merge conflict and I also made sure that Window.handle compiles with SDL3 and returns the proper window handle (I modified window.c to be able to create a window with SDL3 so I could call the property). This is now ready to review. |
|
Is there any other usecase for this apart from diagnostics? We probably discussed this already but just so that it's on github - these handle numbers could potentially be useful for embedding foreign windows in pygame-ce or vice-versa. It is probably going to be very platform/configuration specific though. |
|
@ankith26 I would say the usecase is using low level functionality of the OS that can do something if they know the window pointer. If you want, I can rename this to |
|
Looks like there are people that desire the handle: |
MyreMylar
left a comment
MyreMylar
left a comment
There was a problem hiding this comment.
This seems to make sense to me based on long ago fiddling with window embedding. I'm happy to allow it and then see if there is demand for any other platform specific window properties with defined use cases as we go along.
The way windows are handled is very platform specific in the details and each platform has it's own selection of identifiers that we are unlikely to be able to abstract perfectly to interface with other library code. You can see SDLs SDL_GetWindowProperties() just returns a group of identifiers that differ per platform.
Anyway, approved. 👍
zoldalma999
left a comment
zoldalma999
left a comment
There was a problem hiding this comment.
Added a small comment, but apart from that both API-wise and implementation wise it looks good to me. However I would maybe wait for one or two more approvals, because it is new API.
WalkthroughAdds a read-only Window.handle: int property that returns the native window manager handle when available, otherwise 0. Changes include a type stub, a documentation macro, a C getter with SDL3/SDL2 code paths, and a unit test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python
participant WinObj as Window object
participant C as C getter (window_get_handle)
participant SDL as SDL
rect rgb(0.95,0.98,1)
Note over Py,WinObj: Access Window.handle (public read-only)
end
Py->>WinObj: read .handle
WinObj->>C: call window_get_handle(self)
C->>SDL: SDL_GetCurrentVideoDriver()
alt SDL >= 3.0
C->>SDL: SDL_GetWindowProperties(window, props)
SDL-->>C: driver-specific pointer/number
C-->>WinObj: integer handle or 0
else SDL < 3.0
C->>SDL: SDL_GetWindowWMInfo(window, &wminfo)
SDL-->>C: platform-specific struct
C-->>WinObj: extracted handle or 0
end
WinObj-->>Py: return int handle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-17T08:53:32.421ZApplied to files:
🧬 Code graph analysis (1)src_c/window.c (1)
⏰ 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)
🔇 Additional comments (3)
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: 1
🧹 Nitpick comments (1)
src_c/doc/window_doc.h (1)
20-20: Minor: Add punctuation for consistency.The documentation string should end with a period for consistency with other property documentation in this file.
Apply this diff:
-#define DOC_WINDOW_HANDLE "handle -> int\nGet the window handle provided by the window manager if supported otherwise 0" +#define DOC_WINDOW_HANDLE "handle -> int\nGet the window handle provided by the window manager if supported otherwise 0."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
buildconfig/stubs/pygame/window.pyi(1 hunks)src_c/doc/window_doc.h(1 hunks)src_c/window.c(3 hunks)test/window_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/window_test.py (2)
buildconfig/stubs/pygame/window.pyi (2)
Window(11-503)handle(316-326)src_py/__init__.py (1)
Window(332-333)
src_c/window.c (1)
buildconfig/stubs/pygame/window.pyi (1)
handle(316-326)
⏰ 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: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Pyodide build
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
🔇 Additional comments (6)
test/window_test.py (1)
461-463: LGTM!The test correctly verifies that the
handleattribute exists and returns an integer type.src_c/window.c (4)
11-13: LGTM!Correct conditional include for SDL2 compatibility.
963-990: Note the inconsistency between SDL2 and SDL3 platform handling.The SDL2 code path uses compile-time checks (
#if defined(SDL_VIDEO_DRIVER_WINDOWS)), meaning the handle is only available for drivers that were compiled into SDL. The SDL3 code path uses runtime checks (!strcmp(driver, "windows")), which is more flexible.This means behavior differs between SDL versions: on SDL2, an unsupported platform returns 0 even if the driver name matches, while on SDL3, the driver name determines the property to fetch.
Consider documenting this nuance, or verify that the SDL2 compile-time approach is intentional and aligns with how other properties are handled in the codebase.
1512-1512: LGTM!Correctly exposes the handle as a read-only property.
929-929: Based on verification against official SDL3 documentation, the version requirement in the code is correct.SDL_GetCurrentVideoDriver() is available since SDL 3.2.0, as confirmed by both the official SDL Wiki and the SDL3 source code. The version check at line 929 is therefore accurate and necessary.
The review comment's concern about the function causing incorrect behavior in SDL 3.0.0-3.1.x builds is valid from a defensive coding perspective—attempting to call SDL_GetCurrentVideoDriver() on those versions would indeed fail. However, the
#if SDL_VERSION_ATLEAST(3, 2, 0)guard correctly prevents this.buildconfig/stubs/pygame/window.pyi (1)
315-326: LGTM!The stub is well-documented with clear platform availability notes and the correct version annotation (2.5.7).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/window.c(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src_c/window.c (1)
buildconfig/stubs/pygame/window.pyi (1)
handle(316-326)
⏰ 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: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Pyodide build
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: AMD64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: x86
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: dev-check
🔇 Additional comments (1)
src_c/window.c (1)
1512-1512: Stub correctly exposeshandleas a read‑only property.The verification confirms that
buildconfig/stubs/pygame/window.pyicorrectly declareshandlewith the@propertydecorator at line 315, followed by the method signaturedef handle(self) -> int:at line 316. The property is properly typed and matches the getter implementation in the C code.
5 participants
Window manager information and especially the window handle were only accessible to the display module. I feel like you should know the window handle of any window, so I added the property. This PR changed so now only the window handle is returned. Check the comments ( especially this comment ) for more.
Summary by CodeRabbit
New Features
Documentation
Tests