Add pygame.display.get_desktop_usable_bounds
#3530
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
damusss
added
New API
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdded a new display API get_desktop_usable_bounds returning a list of Rects for usable desktop areas. Implemented in C with SDL2/SDL3 paths, exposed in the display module, documented via a new doc macro, typed in stubs, and tested to validate types and size constraints against get_desktop_sizes. Changes
Sequence Diagram(s)sequenceDiagram
participant Py as Python user
participant Mod as pygame.display
participant C as pg_get_desktop_usable_bounds (C)
participant SDL as SDL Video API
Py->>Mod: get_desktop_usable_bounds()
Mod->>C: call (no args)
alt SDL3
C->>SDL: SDL_GetDisplays() / SDL_GetDisplayUsableBounds()
else pre-SDL3
C->>SDL: SDL_GetNumVideoDisplays() / SDL_GetDisplayUsableBounds()
end
SDL-->>C: usable bounds per display
C-->>Mod: list[Rect]
Mod-->>Py: list[Rect]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 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
CodeRabbit Configuration File (
|
![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)
test/display_test.py (1)
700-709: Avoid potential IndexError and assert list alignmentIf, for any reason, the two lists differ in length, this loop will raise IndexError. Assert the lengths match and iterate with zip to keep the test robust.
Apply this diff:
def test_get_desktop_usable_bounds(self): bounds = pygame.display.get_desktop_usable_bounds() sizes = pygame.display.get_desktop_sizes() self.assertIsInstance(bounds, list) - for i, bound in enumerate(bounds): - self.assertIsInstance(bound, pygame.Rect) - size = sizes[i] - self.assertLessEqual(bound.w, size[0]) - self.assertLessEqual(bound.h, size[1]) + self.assertEqual(len(bounds), len(sizes)) + for bound, size in zip(bounds, sizes): + self.assertIsInstance(bound, pygame.Rect) + self.assertLessEqual(bound.w, size[0]) + self.assertLessEqual(bound.h, size[1])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
buildconfig/stubs/pygame/display.pyi(2 hunks)src_c/display.c(2 hunks)src_c/doc/display_doc.h(1 hunks)test/display_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/display_test.py (2)
buildconfig/stubs/pygame/display.pyi (2)
get_desktop_usable_bounds(425-441)get_desktop_sizes(406-423)buildconfig/stubs/pygame/rect.pyi (1)
Rect(303-303)
buildconfig/stubs/pygame/display.pyi (4)
buildconfig/stubs/pygame/typing.pyi (1)
rect(57-57)src_py/typing.py (1)
rect(57-57)buildconfig/stubs/typing_sample_app.py (1)
rect(100-101)buildconfig/stubs/pygame/rect.pyi (1)
Rect(303-303)
🔇 Additional comments (4)
src_c/doc/display_doc.h (1)
14-14: Doc macro addition LGTMThe new macro name and summary align with existing patterns and the API behavior.
buildconfig/stubs/pygame/display.pyi (2)
425-442: Stub signature and docstring LGTMReturn type list[Rect] matches runtime objects. The docstring mirrors the new API semantics and version tag is present.
55-55: No Union usage detected – import removal is safe
- buildconfig/stubs/pygame/display.pyi: scanned for “Union” with ripgrep; no occurrences found.
src_c/display.c (1)
3179-3181: Method registration LGTMCorrectly registered with METH_NOARGS and linked to the doc macro.
Comment on lines
+2299
to
+2344
| static PyObject * | ||
| pg_get_desktop_usable_bounds(PyObject *self, PyObject *_null) | ||
| { | ||
| int display_count, i; | ||
| PyObject *result; | ||
|
|
||
| VIDEO_INIT_CHECK(); | ||
|
|
||
| #if PG_SDL3 | ||
| SDL_DisplayID *displays = SDL_GetDisplays(&display_count); | ||
| if (displays == NULL) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| #else | ||
| display_count = SDL_GetNumVideoDisplays(); | ||
| if (display_count < 0) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| #endif | ||
|
|
||
| result = PyList_New(display_count); | ||
| if (!result) { | ||
| return NULL; | ||
| } | ||
|
|
||
| for (i = 0; i < display_count; i++) { | ||
| SDL_Rect bounds; | ||
| #if PG_SDL3 | ||
| SDL_DisplayID display_id = displays[i]; | ||
| if (SDL_GetDisplayUsableBounds(display_id, &bounds) == SDL_FALSE) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| #else | ||
| if (SDL_GetDisplayUsableBounds(i, &bounds) < 0) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| #endif | ||
| PyObject *pg_rect = pgRect_New(&bounds); | ||
| if (pg_rect == NULL) { | ||
| return NULL; /* exception already set */ | ||
| } | ||
| PyList_SET_ITEM(result, i, pg_rect); | ||
| } | ||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
Memory leaks and missing DECREF on error paths in SDL3 and SDL2 branches
- SDL3 path: SDL_GetDisplays allocates an array that must be freed with SDL_free(). It is never freed.
- Both paths: After allocating result = PyList_New(...), early returns on errors leak the Python list (missing Py_DECREF(result)).
- On pgRect_New failure, result and (in SDL3) displays are leaked.
Fix by freeing displays (SDL3) and DECREF-ing result on all error paths.
Apply this diff:
static PyObject *
pg_get_desktop_usable_bounds(PyObject *self, PyObject *_null)
{
- int display_count, i;
- PyObject *result;
+ int display_count, i;
+ PyObject *result = NULL;
VIDEO_INIT_CHECK();
#if PG_SDL3
- SDL_DisplayID *displays = SDL_GetDisplays(&display_count);
- if (displays == NULL) {
- return RAISE(pgExc_SDLError, SDL_GetError());
- }
+ SDL_DisplayID *displays = SDL_GetDisplays(&display_count);
+ if (displays == NULL) {
+ return RAISE(pgExc_SDLError, SDL_GetError());
+ }
#else
display_count = SDL_GetNumVideoDisplays();
if (display_count < 0) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
#endif
- result = PyList_New(display_count);
- if (!result) {
- return NULL;
- }
+ result = PyList_New(display_count);
+ if (!result) {
+#if PG_SDL3
+ SDL_free(displays);
+#endif
+ return NULL;
+ }
for (i = 0; i < display_count; i++) {
SDL_Rect bounds;
#if PG_SDL3
- SDL_DisplayID display_id = displays[i];
- if (SDL_GetDisplayUsableBounds(display_id, &bounds) == SDL_FALSE) {
- return RAISE(pgExc_SDLError, SDL_GetError());
- }
+ SDL_DisplayID display_id = displays[i];
+ if (SDL_GetDisplayUsableBounds(display_id, &bounds) == SDL_FALSE) {
+ Py_DECREF(result);
+ SDL_free(displays);
+ return RAISE(pgExc_SDLError, SDL_GetError());
+ }
#else
- if (SDL_GetDisplayUsableBounds(i, &bounds) < 0) {
- return RAISE(pgExc_SDLError, SDL_GetError());
- }
+ if (SDL_GetDisplayUsableBounds(i, &bounds) < 0) {
+ Py_DECREF(result);
+ return RAISE(pgExc_SDLError, SDL_GetError());
+ }
#endif
- PyObject *pg_rect = pgRect_New(&bounds);
- if (pg_rect == NULL) {
- return NULL; /* exception already set */
- }
+ PyObject *pg_rect = pgRect_New(&bounds);
+ if (pg_rect == NULL) {
+#if PG_SDL3
+ SDL_free(displays);
+#endif
+ Py_DECREF(result);
+ return NULL; /* exception already set */
+ }
PyList_SET_ITEM(result, i, pg_rect);
}
- return result;
+#if PG_SDL3
+ SDL_free(displays);
+#endif
+ return result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static PyObject * | |
| pg_get_desktop_usable_bounds(PyObject *self, PyObject *_null) | |
| { | |
| int display_count, i; | |
| PyObject *result; | |
| VIDEO_INIT_CHECK(); | |
| #if PG_SDL3 | |
| SDL_DisplayID *displays = SDL_GetDisplays(&display_count); | |
| if (displays == NULL) { | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #else | |
| display_count = SDL_GetNumVideoDisplays(); | |
| if (display_count < 0) { | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #endif | |
| result = PyList_New(display_count); | |
| if (!result) { | |
| return NULL; | |
| } | |
| for (i = 0; i < display_count; i++) { | |
| SDL_Rect bounds; | |
| #if PG_SDL3 | |
| SDL_DisplayID display_id = displays[i]; | |
| if (SDL_GetDisplayUsableBounds(display_id, &bounds) == SDL_FALSE) { | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #else | |
| if (SDL_GetDisplayUsableBounds(i, &bounds) < 0) { | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #endif | |
| PyObject *pg_rect = pgRect_New(&bounds); | |
| if (pg_rect == NULL) { | |
| return NULL; /* exception already set */ | |
| } | |
| PyList_SET_ITEM(result, i, pg_rect); | |
| } | |
| return result; | |
| } | |
| static PyObject * | |
| pg_get_desktop_usable_bounds(PyObject *self, PyObject *_null) | |
| { | |
| int display_count, i; | |
| PyObject *result = NULL; | |
| VIDEO_INIT_CHECK(); | |
| #if PG_SDL3 | |
| SDL_DisplayID *displays = SDL_GetDisplays(&display_count); | |
| if (displays == NULL) { | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #else | |
| display_count = SDL_GetNumVideoDisplays(); | |
| if (display_count < 0) { | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #endif | |
| result = PyList_New(display_count); | |
| if (!result) { | |
| #if PG_SDL3 | |
| SDL_free(displays); | |
| #endif | |
| return NULL; | |
| } | |
| for (i = 0; i < display_count; i++) { | |
| SDL_Rect bounds; | |
| #if PG_SDL3 | |
| SDL_DisplayID display_id = displays[i]; | |
| if (SDL_GetDisplayUsableBounds(display_id, &bounds) == SDL_FALSE) { | |
| Py_DECREF(result); | |
| SDL_free(displays); | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #else | |
| if (SDL_GetDisplayUsableBounds(i, &bounds) < 0) { | |
| Py_DECREF(result); | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #endif | |
| PyObject *pg_rect = pgRect_New(&bounds); | |
| if (pg_rect == NULL) { | |
| #if PG_SDL3 | |
| SDL_free(displays); | |
| #endif | |
| Py_DECREF(result); | |
| return NULL; /* exception already set */ | |
| } | |
| PyList_SET_ITEM(result, i, pg_rect); | |
| } | |
| #if PG_SDL3 | |
| SDL_free(displays); | |
| #endif | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In src_c/display.c around lines 2299 to 2344, the SDL3 branch leaks the displays
array returned by SDL_GetDisplays and both branches leak the Python list and (in
SDL3) displays on early error paths; ensure you free resources on every error
path: after creating result = PyList_New(display_count) call, on any subsequent
error return you must Py_DECREF(result) before returning; in the SDL3 branch
free the displays array with SDL_free(displays) before every return (both on
SDL_GetDisplayUsableBounds failure and on pgRect_New failure), and also free
displays just before the successful return; do not change behavior otherwise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As I said on discord,
pygame.display.get_desktop_sizesexist but doesn't take in consideration the taskbar or any other OS reserved space. The SDL function for it exists, so it makes sense to me to add it to display. It would be very tedious to do this in python cross platform without it. It's a differnt function from the sizes because it returns rects, but the return list uses the same logic so with the same index you get the full size and usable area for the same desktop. Also, it's alredy SDL3 compatible, so there will be no problems when display is ported. There is no conflict with window because this is one of the features that make sense for display, and doesn't concert any windows (display should only hold this functions)Summary by CodeRabbit
New Features
Documentation
Tests