Fix ascii scrap on sdl2 backend, docs/tests fixes #3473
Conversation
ankith26
force-pushed
the
ankith26-scrap-fixes
branch
from
7e91c13 to
7e8d9e5
Compare
ankith26
force-pushed
the
ankith26-scrap-fixes
branch
from
7e8d9e5 to
43bdb43
Compare
|
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src_c/scrap_sdl2.c (1)
107-114: Usesrclenand ensure NUL-termination before callingSDL_SetClipboardText
SDL_SetClipboardTextexpects a NUL-terminated UTF-8 string. Passing the rawsrcignoressrclenand can read past the buffer or truncate if there are embedded NULs. Allocate a temporary buffer, copy, and terminate.- if (strcmp(type, pygame_scrap_plaintext_type) == 0 || - strcmp(type, pygame_scrap_utf8text_type) == 0) { - if (SDL_SetClipboardText(src) == 0) { - return 1; - } - } + if (strcmp(type, pygame_scrap_plaintext_type) == 0 || + strcmp(type, pygame_scrap_utf8text_type) == 0) { + char *tmp = (char *)malloc((size_t)srclen + 1); + if (!tmp) { + PyErr_NoMemory(); + return 0; + } + memcpy(tmp, src, (size_t)srclen); + tmp[srclen] = '\0'; + if (SDL_SetClipboardText(tmp) == 0) { + free(tmp); + return 1; + } + free(tmp); + }Optional: if strict ASCII is desired when
type == "text/plain", validate with_pg_is_ascii_str(tmp)and fail early, mirroring theget()behavior.
🧹 Nitpick comments (8)
test/scrap_test.py (5)
2-2: Prefer a single, reusable Windows-gating constant over importingplatformUse a module-level decorator alias to avoid duplicating the skip condition and to keep the reason consistent across tests.
-import platform +import platform +WINDOWS_ONLY = unittest.skipIf( + platform.system() != "Windows", + "Non-text clipboard types and user-defined scrap types are only supported on Windows with the SDL2 backend", +)
55-58: Unify the skip condition and clarify the reasonReplace the inline decorator with the shared alias so changes to the gating/wording happen in one place.
- @unittest.skipIf( - platform.system() != "Windows", - "scrap features are broken on non windows platforms", - ) + @WINDOWS_ONLY
103-106: Same here: use the sharedWINDOWS_ONLYdecoratorKeeps the tests DRY and the skip rationale accurate.
- @unittest.skipIf( - platform.system() != "Windows", - "scrap features are broken on non windows platforms", - ) + @WINDOWS_ONLY
115-118: Same here: consolidate the platform gateReduces duplication and makes future edits easier.
- @unittest.skipIf( - platform.system() != "Windows", - "scrap features are broken on non windows platforms", - ) + @WINDOWS_ONLY
67-73: Add SDL2-specific context to the clipboard‐ownership skipVerification of
pygame_scrap_lost()on the SDL2 backend (insrc_c/scrap_sdl2.cat line 94) shows it always returns1(“lost”) unconditionally. As a result, this test will unconditionally hit the secondskipTestbranch under SDL2. To avoid confusion, the skip reason should note that behavior.• File:
test/scrap_test.py(around lines 67–73)
• Change the skip message from:- self.skipTest("requires the pygame application to own the clipboard") + self.skipTest("requires the pygame application to own the clipboard (SDL2 backend always loses ownership)")src_c/scrap_sdl2.c (1)
11-13: String literals should beconst-qualifiedThese globals point to string literals; marking them
constprevents accidental writes and avoids compiler warnings on some toolchains.-char *pygame_scrap_plaintext_type = "text/plain"; -char *pygame_scrap_utf8text_type = "text/plain;charset=utf-8"; +const char *pygame_scrap_plaintext_type = "text/plain"; +const char *pygame_scrap_utf8text_type = "text/plain;charset=utf-8";Note: fully const-correctness would also change
pygame_scrap_typesand any prototypes that expose it toconst char **. If that ripples too far for this PR, the above tweak is optional.docs/reST/ref/scrap.rst (2)
80-82: Clarify ASCII vs UTF-8 behavior for text typesMake it explicit that
"text/plain"is ASCII-only in the SDL2 backend, and non-ASCII should use the UTF-8 variant. This will save users time debugging None returns fromscrap.get("text/plain")."text/plain" Plain text (also accessible via the SCRAP_TEXT constant) "text/plain;charset=utf-8" UTF-8 encoded text + +.. note:: + When requesting ``"text/plain"`` via :func:`pygame.scrap.get`, only ASCII content + is considered valid; if the clipboard contains non-ASCII characters the call + returns ``None``. For Unicode text use ``"text/plain;charset=utf-8"``.
105-107: Tighten wording about platform supportConsider adding “with the SDL2 backend” to be explicit, since older backends (e.g., X11) historically handled more. Optional.
- Non-text data is only supported on Windows. On other platforms only text is supported. + Non-text data is only supported on Windows with the SDL2 backend. On other platforms only text is supported.
📜 Review details
Configuration used: CodeRabbit UI
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 (4)
docs/reST/ref/scrap.rst(3 hunks)src_c/scrap_sdl2.c(4 hunks)test/scrap_tags.py(0 hunks)test/scrap_test.py(4 hunks)
💤 Files with no reviewable changes (1)
- test/scrap_tags.py
🔇 Additional comments (4)
src_c/scrap_sdl2.c (1)
80-88: LGTM: types array updated for ASCII + UTF-8 and terminatorThe allocation and terminator update matches the new two-text-type behavior.
docs/reST/ref/scrap.rst (3)
88-88: LGTM: Windows-only non-text MIME types are clearly scopedThe explicit listing helps set accurate expectations for cross-platform behavior.
93-95: Good clarification on user-defined typesNoting Windows-only accessibility for custom types aligns with the current SDL2 backend.
200-202: Put() predefined types list matches implementationLimiting to
SCRAP_BMPandSCRAP_TEXTreflects the code and avoids implying broader support. Looks good.
ankith26
force-pushed
the
ankith26-scrap-fixes
branch
2 times, most recently
from
be6d5e9 to
1a0b982
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src_c/scrap_sdl2.c (2)
81-81: Type-safe allocation style (and header requirement)Minor: prefer
3 * sizeof *pygame_scrap_typesto avoid repeating the pointed-to type. Also, this requires<stdlib.h>(covered in the includes suggestion above).- pygame_scrap_types = malloc(sizeof(char *) * 3); + pygame_scrap_types = malloc(3 * sizeof *pygame_scrap_types);
50-57: Null-dereference of count when caller passes NULLYou unconditionally assign to
*counton Line 55, but callers may passNULL(you already handle this at entry). Guard the assignment.- if (is_utf8 || _pg_validate_ascii_str(clipboard)) { - *count = strlen(clipboard); - retval = strdup(clipboard); - } + if (is_utf8 || _pg_validate_ascii_str(clipboard)) { + if (count) { + *count = strlen(clipboard); + } + retval = strdup(clipboard); + }Additionally, this function (and others below) use
strcmp,strlen,strdup, andmalloc. Please include the proper headers to avoid implicit declarations and build differences across platforms:#endif + +/* C runtime headers needed by this translation unit */ +#include <string.h> /* strcmp, strlen, strdup */ +#include <stdlib.h> /* malloc, free */Note: sticking with
strdupis fine here because the returned buffer is later freed withfree()(see PYGAME_SCRAP_FREE_STRING). If you switch toSDL_strdup, you must also switch the deallocator toSDL_freeto avoid mismatched alloc/free pairs.
🧹 Nitpick comments (3)
src_c/scrap_sdl2.c (3)
11-12: Consider internal linkage and const-correctness for MIME type literalsThese are file-local constants; expose them as
staticand make themconstto avoid accidental modification and symbol export.-char *pygame_scrap_plaintext_type = "text/plain"; -char *pygame_scrap_utf8text_type = "text/plain;charset=utf-8"; +static const char *const pygame_scrap_plaintext_type = "text/plain"; +static const char *const pygame_scrap_utf8text_type = "text/plain;charset=utf-8";If other compilation units rely on these symbols, keep external linkage but at least make them
const.
23-34: Scope this helper; minor tighteningMake
_pg_validate_ascii_strstaticto limit external visibility. Implementation is fine and correctly usesunsigned charfor the range check.-int +static int _pg_validate_ascii_str(const char *str) { while (*str) { if ((unsigned char)*str > 127) { return 0; } str++; } return 1; }
108-109: LGTM with a note: srclen is ignoredAccepting both MIME types and delegating to
SDL_SetClipboardTextis correct.srclenis currently unused; that matches SDL’s API expectation of a NUL-terminated UTF-8 string. If callers can provide non-NUL-terminated buffers, either validate here or document thatsrcmust be NUL-terminated.Would you like me to add a brief comment to the function header documenting that
srcmust be NUL-terminated when using the SDL2 backend?
📜 Review details
Configuration used: CodeRabbit UI
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 (5)
docs/reST/ref/scrap.rst(3 hunks)src_c/scrap_sdl2.c(3 hunks)test/meson.build(0 hunks)test/scrap_tags.py(0 hunks)test/scrap_test.py(4 hunks)
💤 Files with no reviewable changes (2)
- test/meson.build
- test/scrap_tags.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/reST/ref/scrap.rst
- test/scrap_test.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src_c/scrap_sdl2.c (1)
src_c/scrap.c (1)
pygame_scrap_initialized(92-96)
⏰ 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: x86_64
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- 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: dev-check
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: x86
- GitHub Check: AMD64
🔇 Additional comments (3)
src_c/scrap_sdl2.c (3)
18-20: LGTM: contains() now correctly recognizes both MIME typesThe logic is straightforward and defers availability to
SDL_HasClipboardText(), which is appropriate for a quick capability check across platforms.
42-44: Good: initialize count to 0 on entryConsistent API behavior even on early returns. Nice.
87-88: LGTM: advertise both text types in the returned listOrdering and NULL-termination look correct.
ankith26
force-pushed
the
ankith26-scrap-fixes
branch
from
1a0b982 to
7e05d3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src_c/scrap_sdl2.c (2)
42-44: Good: initialize out-parameter count to 0This ensures consistent behavior on all early returns and addresses earlier feedback.
11-12: Add missing standard headers for strcmp/strlen/strdup and mallocThis file uses strcmp, strlen, strdup, and malloc without including the required headers. This will fail on strict compilers (C99+ disallows implicit declarations) and is a portability hazard. Also aligns with a prior note to include <string.h>.
Apply:
#endif +#include <string.h> /* strcmp, strlen, strdup */ +#include <stdlib.h> /* malloc, free */ + +/* MSVC exposes strdup as _strdup; guard to keep builds portable */ +#if defined(_MSC_VER) && !defined(strdup) +#define strdup _strdup +#endifAlso applies to: 50-51, 56-58, 83-83
🧹 Nitpick comments (5)
src_c/scrap_sdl2.c (5)
18-20: Consider aligning contains() semantics with get() for text/plain (ASCII)contains() may return true for "text/plain" even when the clipboard holds non-ASCII characters, while get() will subsequently refuse to return that data for the ASCII type. If you want strict type-consistent behavior, optionally validate ASCII here when type == text/plain. Trade-off: this calls SDL_GetClipboardText() and briefly allocates/frees.
Possible refinement:
-int -pygame_scrap_contains(char *type) +int +pygame_scrap_contains(const char *type) { - return (strcmp(type, pygame_scrap_plaintext_type) == 0 || - strcmp(type, pygame_scrap_utf8text_type) == 0) && - SDL_HasClipboardText(); + int is_plain = strcmp(type, pygame_scrap_plaintext_type) == 0; + int is_utf8 = strcmp(type, pygame_scrap_utf8text_type) == 0; + if (!(is_plain || is_utf8) || !SDL_HasClipboardText()) { + return 0; + } + if (is_utf8) { + return 1; + } + /* ASCII requested: verify that current clipboard text is ASCII-only */ + char *text = SDL_GetClipboardText(); + if (!text) { + return 0; + } + int ok = _pg_validate_ascii_str(text); + SDL_free(text); + return ok; }If you prefer to keep contains() cheap, at least document the current behavior difference between contains() and get().
11-12: Make MIME type globals and function signatures const-correctThese are string literals and read-only parameters; mark them const to improve type-safety and prevent accidental writes. This is internal C code, but verify no external symbol constraints rely on non-const types.
-char *pygame_scrap_plaintext_type = "text/plain"; -char *pygame_scrap_utf8text_type = "text/plain;charset=utf-8"; +const char *pygame_scrap_plaintext_type = "text/plain"; +const char *pygame_scrap_utf8text_type = "text/plain;charset=utf-8"; @@ -int -pygame_scrap_contains(char *type) +int +pygame_scrap_contains(const char *type) @@ -char * -pygame_scrap_get(char *type, size_t *count) +char * +pygame_scrap_get(const char *type, size_t *count) @@ -int -pygame_scrap_put(char *type, Py_ssize_t srclen, char *src) +int +pygame_scrap_put(const char *type, Py_ssize_t srclen, const char *src)If adjusting exported symbols is risky, keep the existing public prototypes and add internal const casts locally.
Also applies to: 15-17, 37-37, 102-104
83-90: Avoid magic numbers and use sizeof on the pointer targetMinor maintainability nit: using sizeof(*ptr) is safer if the type changes.
- pygame_scrap_types = malloc(sizeof(char *) * 3); + pygame_scrap_types = malloc(sizeof(*pygame_scrap_types) * 3);Optionally define a constant for the count (2 entries + NULL):
enum { PYGAME_SCRAP_TYPE_COUNT = 2 }; /* malloc(sizeof(*pygame_scrap_types) * (PYGAME_SCRAP_TYPE_COUNT + 1)) */
81-81: Handle SDL_Init failure and surface the SDL errorDefensive, cheap, and improves diagnostics in rare failure modes.
- SDL_Init(SDL_INIT_VIDEO); + if (SDL_Init(SDL_INIT_VIDEO) < 0) { + PyErr_SetString(pgExc_SDLError, SDL_GetError()); + return 0; + }
110-112: Enforce ASCII on put() when type == text/plain to mirror get() semanticsRight now, put() accepts non-ASCII even for text/plain, but a subsequent get("text/plain") will refuse to return it. Consider enforcing ASCII on writes for the plain type to keep types consistent end-to-end. Also, srclen is currently unused; either use it for validation or explicitly mark it unused to avoid warnings.
-int -pygame_scrap_put(char *type, Py_ssize_t srclen, char *src) +int +pygame_scrap_put(const char *type, Py_ssize_t srclen, const char *src) { + (void)srclen; /* currently unused; kept for API parity */ @@ - if (strcmp(type, pygame_scrap_plaintext_type) == 0 || - strcmp(type, pygame_scrap_utf8text_type) == 0) { + if (strcmp(type, pygame_scrap_plaintext_type) == 0 || + strcmp(type, pygame_scrap_utf8text_type) == 0) { + if (strcmp(type, pygame_scrap_plaintext_type) == 0 && + !_pg_validate_ascii_str(src)) { + /* Keep behavior quiet like get(): fail without raising */ + return 0; + } if (SDL_SetClipboardText(src) == 0) { return 1; } }
📜 Review details
Configuration used: CodeRabbit UI
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 (5)
docs/reST/ref/scrap.rst(3 hunks)src_c/scrap_sdl2.c(3 hunks)test/meson.build(0 hunks)test/scrap_tags.py(0 hunks)test/scrap_test.py(4 hunks)
💤 Files with no reviewable changes (2)
- test/scrap_tags.py
- test/meson.build
🚧 Files skipped from review as they are similar to previous changes (2)
- test/scrap_test.py
- docs/reST/ref/scrap.rst
🧰 Additional context used
🧬 Code graph analysis (1)
src_c/scrap_sdl2.c (1)
src_c/scrap.c (1)
pygame_scrap_initialized(92-96)
⏰ 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
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (macos-14)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-latest)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: dev-check
🔇 Additional comments (2)
src_c/scrap_sdl2.c (2)
23-34: ASCII validator: correct and safe implementationCasting to unsigned char avoids sign-extension pitfalls; loop is clear and correct for 7-bit ASCII. Good addition.
50-61: get(): logic looks correct; memory ownership is consistent with PYGAME_SCRAP_FREE_STRING
- For UTF-8 requests, you pass through the text.
- For ASCII requests, you filter non-ASCII safely.
- You duplicate with strdup so callers can free() as implied by PYGAME_SCRAP_FREE_STRING.
LGTM.
As we all know scrap is deprecated, why this PR?
Well, my initial goal was to simply skip all the failing tests on non-windows platforms and call it a day, but then I realised that we broke
"text/plain"AKApygame.SCRAP_TEXTsupport in pygame-ce when it is trivial to support in our current SDL2 scrap backend (which already supports"text/plain;charset=utf-8").Of the 5 failing tests, simple updates to our sdl2 scrap backend fixes 2 tests on all non windows platforms. I added skips to the rest on non windows platforms, and also removed an interactive test that was testing for removed x11 backend functionality.
And finally, I updated the docs to reflect reality. The docs currently claim a lot of things we don't support, and this may tempt a few users to use it ignoring the deprecation warnings. Our current scrap status is simply "plain text is supported on all platforms, images/music may work on windows".
Summary by CodeRabbit