Added outline property to pygame.font.Font #3597
Conversation
📝 WalkthroughWalkthroughAdds SDL_ttf-backed outline support to Font: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor PyUser as Python code
participant FontObj as Font (Python)
participant CAPI as src_c/font.c
participant SDLTTF as SDL_ttf
PyUser->>FontObj: font.outline (get)
FontObj->>CAPI: font_getter_outline
alt Font not generated
CAPI-->>PyUser: raise pygame.error
else SDL_ttf < 2.0.12
CAPI-->>PyUser: raise SDLError (unsupported)
else Supported
CAPI->>SDLTTF: TTF_GetFontOutline(font)
SDLTTF-->>CAPI: outline (int)
CAPI-->>PyUser: return int
end
PyUser->>FontObj: font.outline = n (set)
FontObj->>CAPI: font_setter_outline(n)
alt invalid type or n < 0
CAPI-->>PyUser: TypeError / ValueError
else Font not generated
CAPI-->>PyUser: raise pygame.error
else SDL_ttf < 2.0.12
CAPI-->>PyUser: raise SDLError (unsupported)
else Supported
CAPI->>SDLTTF: TTF_SetFontOutline(font, n)
SDLTTF-->>CAPI: status
CAPI-->>PyUser: None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/font_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (4)
buildconfig/stubs/pygame/font.pyi(2 hunks)docs/reST/ref/font.rst(2 hunks)src_c/font.c(3 hunks)test/font_test.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
buildconfig/stubs/pygame/font.pyi (1)
buildconfig/stubs/pygame/mask.pyi (1)
outline(38-38)
test/font_test.py (1)
buildconfig/stubs/pygame/font.pyi (21)
init(12-12)get_default_font(16-16)Font(31-95)get_sdl_ttf_version(15-15)outline(61-61)outline(63-63)get_outline(94-94)set_outline(95-95)bold(37-37)bold(39-39)italic(41-41)italic(43-43)underline(45-45)underline(47-47)strikethrough(49-49)strikethrough(51-51)set_bold(78-78)set_italic(80-80)set_underline(74-74)set_strikethrough(76-76)render(65-72)
🪛 GitHub Actions: python3 dev.py all
test/font_test.py
[error] 1-1: clang-format reformatted file. Run 'clang-format -i test/font_test.py' and commit changes.
src_c/font.c
[error] 1-1: clang-format reformatted file. Run 'clang-format -i src_c/font.c' and commit changes.
⏰ 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: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (14)
docs/reST/ref/font.rst (1)
310-323: LGTM!The outline attribute documentation is clear and follows the established pattern for other font attributes.
buildconfig/stubs/pygame/font.pyi (2)
60-63: LGTM!The outline property stubs are correctly typed and follow the established pattern for other Font properties.
94-95: LGTM!The get_outline and set_outline method stubs have correct signatures matching the implementation.
test/font_test.py (5)
688-726: LGTM!The test_outline_property test correctly validates outline property behavior with appropriate SDL_ttf version guards (2.0.12+), type checks, and boundary conditions.
727-751: LGTM!The test_outline_method test properly validates get_outline and set_outline methods with version guards and error handling for invalid inputs.
1000-1006: LGTM!Correctly adds outline methods to the post-quit exception test with appropriate version gating.
1107-1110: LGTM!Correctly adds outline property to the post-quit exception test with appropriate version gating.
1169-1259: LGTM — outline tests addedInteractive tests correctly incorporate the outline parameter and demonstrate its effect.
CI is failing on formatting in test/font_test.py; format that file (e.g. run clang-format) to satisfy the pipeline.src_c/font.c (6)
904-919: LGTM!The font_getter_outline implementation correctly checks font generation, uses appropriate SDL_ttf version guards (2.0.12+), and returns the outline value via TTF_GetFontOutline.
921-951: LGTM!The font_setter_outline implementation has proper:
- Generation checks
- Input validation (type check, non-negative constraint)
- SDL_ttf version gating (2.0.12+)
- Error messages matching other attribute setters
953-967: LGTM!The font_get_outline method correctly mirrors the getter with generation checks and version guards.
969-991: LGTM!The font_set_outline method properly validates input and applies the outline value with appropriate error handling.
1261-1262: LGTM!Correctly adds outline property to font_getsets table with getter/setter and appropriate docstring placeholder.
1287-1288: Formatting Verification Required
Unable to run clang-format in this environment. Please manually formatsrc_c/font.cusing clang-format and re-run CI to resolve the pipeline failure.
src_c/font.c
Outdated
| static PyObject * | ||
| font_get_outline(PyObject *self, PyObject *_null) | ||
| { | ||
| if (!PgFont_GenerationCheck(self)) { | ||
| return RAISE_FONT_QUIT_ERROR(); | ||
| } | ||
| #if SDL_TTF_VERSION_ATLEAST(2, 0, 12) | ||
| TTF_Font *font = PyFont_AsFont(self); | ||
| return PyLong_FromLong(TTF_GetFontOutline(font)); | ||
| #else | ||
| return RAISE(pgExc_SDLError, | ||
| "pygame.font not compiled with a new enough SDL_ttf version. " | ||
| "Needs SDL_ttf 2.0.12 or above."); | ||
| #endif | ||
| } | ||
|
|
||
| static PyObject * | ||
| font_set_outline(PyObject *self, PyObject *arg) | ||
| { | ||
| if (!PgFont_GenerationCheck(self)) { | ||
| return RAISE_FONT_QUIT_ERROR(); | ||
| } | ||
| #if SDL_TTF_VERSION_ATLEAST(2, 0, 12) | ||
| TTF_Font *font = PyFont_AsFont(self); | ||
| long val = PyLong_AsLong(arg); | ||
| if (val == -1 && PyErr_Occurred()) { | ||
| return NULL; | ||
| } | ||
| if (val < 0) { | ||
| return RAISE(PyExc_ValueError, "outline must be >= 0"); | ||
| } | ||
| TTF_SetFontOutline(font, (int)val); | ||
| Py_RETURN_NONE; | ||
| #else | ||
| return RAISE(pgExc_SDLError, | ||
| "pygame.font not compiled with a new enough SDL_ttf version. " | ||
| "Needs SDL_ttf 2.0.12 or above."); | ||
| #endif |
There was a problem hiding this comment.
You've got code duplication. I suggest moving the common code into a helper function that gets called by both the dedicated setter and the property setter
There was a problem hiding this comment.
I concur; I considered deduplicating the functions, but since the rest of the file seemed to use this sort of duplication I erred toward consistency. Is there a convention for this kind of deduplication elsewhere in the project?
There was a problem hiding this comment.
It's scattered from what I remember. Personally, I'm a huge advocate for not duplicating code when possible
There was a problem hiding this comment.
I rewrote font_get_outline to just call font_getter_outline (We could probably even tell python to call font_getter_outline instead with a type cast?), and font_set_outline to call font_setter_outline and massage the return value.
Common helper functions would work too, but I don't see a reason to define 6 functions when 4 (3?) will give the same result.
test/font_test.py
Outdated
| if ttf_version < (2, 0, 12): | ||
| with self.assertRaises(pygame.error): | ||
| f.outline = 0 | ||
| with self.assertRaises(pygame.error): | ||
| _ = f.outline | ||
| return |
There was a problem hiding this comment.
I think it would be cleaner to just skip this test in this case using unittest.skipIf
There was a problem hiding this comment.
That would be cleaner, but that would also mean the case of trying to use the property on an unsupported version is not being tested. Is that a concern?
There was a problem hiding this comment.
You can have a dedicated test for it that skips if the SDL_ttf version is too new. I'm not a huge fan of having this weird logic in the middle of the test
There was a problem hiding this comment.
Changed to use skipIf, and added tests for the stubs with 935ab5c.
test/font_test.py
Outdated
| f.set_outline(outline) | ||
| s = f.render(text, antialiase, (0, 0, 0)) | ||
| screen.blit(s, (offset, y)) | ||
| y += s.get_size()[1] + spacing | ||
| f.set_bold(False) | ||
| f.set_italic(False) | ||
| f.set_underline(False) | ||
| f.set_strikethrough(False) | ||
| f.set_outline(0) |
There was a problem hiding this comment.
This seems like it needs to guard against SDL_ttf version as well
There was a problem hiding this comment.
Added guards with 935ab5c.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/font_test.py (2)
1258-1259: Add version guard for SDL_ttf 2.0.12+.This test will fail on SDL_ttf < 2.0.12 since outline support is not available. Consider adding a
skipIfdecorator.Apply this diff:
+ @unittest.skipIf( + pygame.font.get_sdl_ttf_version() < (2, 0, 12), + "outline requires SDL_ttf 2.0.12+", + ) def test_outline(self): self.assertTrue(self.query(outline=1))
1208-1208: Version-gate theset_outlinecall.The
set_outlinecall will raise apygame.erroron SDL_ttf < 2.0.12. Sincetest_outline(line 1258) callsquery(outline=1), it could fail on older SDL_ttf versions.Apply this diff to guard the call:
f.set_underline(underline) f.set_strikethrough(strikethrough) - f.set_outline(outline) + if pygame_font.get_sdl_ttf_version() >= (2, 0, 12): + f.set_outline(outline) s = f.render(text, antialiase, (0, 0, 0))Also guard the reset:
f.set_underline(False) f.set_strikethrough(False) - f.set_outline(0) + if pygame_font.get_sdl_ttf_version() >= (2, 0, 12): + f.set_outline(0) s = f.render("(some comparison text)", False, (0, 0, 0))
🧹 Nitpick comments (2)
test/font_test.py (2)
698-704: Consider usingunittest.skipIffor version gating.The previous review suggested using
unittest.skipIfinstead of the if/return pattern for cleaner test skipping. This approach is more idiomatic and provides better test reporting.Apply this pattern:
+ @unittest.skipIf( + pygame.font.get_sdl_ttf_version() < (2, 0, 12), + "outline requires SDL_ttf 2.0.12+", + ) def test_outline_property(self): if pygame_font.__name__ == "pygame.ftfont": return # not a pygame.ftfont feature pygame_font.init() font_path = os.path.join( os.path.split(pygame.__file__)[0], pygame_font.get_default_font() ) f = pygame_font.Font(pathlib.Path(font_path), 25) - ttf_version = pygame_font.get_sdl_ttf_version() - if ttf_version < (2, 0, 12): - with self.assertRaises(pygame.error): - f.outline = 0 - with self.assertRaises(pygame.error): - _ = f.outline - return - # Default outline should be an integer >= 0 (typically 0)Based on past review comments.
737-741: Consider usingunittest.skipIffor version gating.For consistency with the suggestion on
test_outline_property, consider using theunittest.skipIfdecorator here as well.Apply this pattern:
+ @unittest.skipIf( + pygame.font.get_sdl_ttf_version() < (2, 0, 12), + "outline requires SDL_ttf 2.0.12+", + ) def test_outline_method(self): if pygame_font.__name__ == "pygame.ftfont": return # not a pygame.ftfont feature pygame_font.init() font_path = os.path.join( os.path.split(pygame.__file__)[0], pygame_font.get_default_font() ) f = pygame_font.Font(pathlib.Path(font_path), 25) - ttf_version = pygame_font.get_sdl_ttf_version() - if ttf_version < (2, 0, 12): - self.assertRaises(pygame.error, f.get_outline) - self.assertRaises(pygame.error, f.set_outline, 1) - return - val0 = f.get_outline()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/font_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (2)
src_c/font.c(3 hunks)test/font_test.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src_c/font.c
🧰 Additional context used
🧬 Code graph analysis (1)
test/font_test.py (1)
buildconfig/stubs/pygame/font.pyi (21)
init(12-12)get_default_font(16-16)Font(31-95)get_sdl_ttf_version(15-15)outline(61-61)outline(63-63)get_outline(94-94)set_outline(95-95)bold(37-37)bold(39-39)italic(41-41)italic(43-43)underline(45-45)underline(47-47)strikethrough(49-49)strikethrough(51-51)set_bold(78-78)set_italic(80-80)set_underline(74-74)set_strikethrough(76-76)render(65-72)
⏰ 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: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: dev-check
- GitHub Check: x86_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
🔇 Additional comments (2)
test/font_test.py (2)
1001-1006: LGTM! Version gating correctly implemented.The outline methods are now properly gated based on SDL_ttf version, matching the pattern used for other version-dependent features like
point_size. This addresses the previous review feedback about needing SDL_ttf version guards.Based on past review comments.
1107-1110: LGTM! Property version gating correctly implemented.The outline property is appropriately version-gated, consistent with the pattern used for other version-dependent properties.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src_c/font.c (1)
954-971: Consider helper function to reduce duplication (optional).As noted in past review comments,
font_get_outlineandfont_set_outlineduplicate logic fromfont_getter_outlineandfont_setter_outline. While the current implementation is consistent with other attributes in this file (e.g.,point_size), consider introducing a helper function pattern to reduce duplication if this becomes a maintenance burden.For example, you could define:
- A helper that wraps the getter logic
- A helper that wraps the setter logic and marshals the return type
This is a codebase-wide pattern rather than specific to outline, so defer to maintainer preference on whether to address this now or separately.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/reST/ref/font.rst(2 hunks)src_c/font.c(3 hunks)test/font_test.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/reST/ref/font.rst
🧰 Additional context used
🧬 Code graph analysis (1)
test/font_test.py (2)
src_c/font.c (1)
font(1497-1572)buildconfig/stubs/pygame/font.pyi (20)
get_sdl_ttf_version(15-15)init(12-12)Font(31-95)outline(61-61)outline(63-63)get_outline(94-94)set_outline(95-95)bold(37-37)bold(39-39)italic(41-41)italic(43-43)underline(45-45)underline(47-47)strikethrough(49-49)strikethrough(51-51)set_bold(78-78)set_italic(80-80)set_underline(74-74)set_strikethrough(76-76)render(65-72)
⏰ 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). (16)
- GitHub Check: x86_64
- GitHub Check: arm64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv7]
🔇 Additional comments (12)
test/font_test.py (7)
688-722: LGTM!The test comprehensively validates the
outlineproperty behavior: default value checks, get/set operations, increment/decrement, and error handling for negative values and incorrect types.
723-741: LGTM!The stub test correctly validates that accessing the
outlineproperty raisespygame.erroron SDL_TTF versions older than 2.0.12, ensuring proper error handling for unsupported versions.
742-764: LGTM!The test validates
get_outlineandset_outlinemethods, including default value checks, setting operations, and error handling for negative values and incorrect types.
765-781: LGTM!The stub test correctly validates that calling
get_outlineandset_outlineraisespygame.erroron SDL_TTF versions older than 2.0.12.
1030-1037: LGTM!The changes correctly add
get_outlineandset_outlineto the test coverage for method exception checks after font module quit, with proper version gating for SDL_TTF >= 2.0.12.
1137-1141: LGTM!The changes correctly add the
outlineproperty to the test coverage for property exception checks after font module quit, with proper version gating for SDL_TTF >= 2.0.12.
1206-1206: LGTM!The interactive test changes correctly integrate outline support into the visual test suite:
- Adds
outlineparameter toquerymethod signature- Includes outline in mode construction and rendering
- Properly guards outline operations with SDL_TTF version checks
- Adds dedicated
test_outlinetest case with proper version gatingAlso applies to: 1217-1217, 1231-1232, 1238-1239, 1247-1248, 1290-1296
src_c/font.c (5)
904-919: LGTM!The getter implementation correctly follows existing patterns:
- Performs generation check
- Guards SDL_ttf calls with version check
- Returns descriptive error for unsupported versions
- Uses TTF_GetFontOutline API correctly
921-952: LGTM!The setter implementation correctly validates input and follows existing patterns:
- Performs generation check
- Guards SDL_ttf calls with version check
- Validates type (must be integer)
- Validates value (must be >= 0)
- Uses TTF_SetFontOutline API correctly
- Provides descriptive error messages
954-960: LGTM!The method implementation correctly delegates to the property getter, following the existing pattern used by other font attributes.
1240-1241: LGTM!The outline property is correctly registered in the
font_getsetsarray with proper getter, setter, and documentation reference.
1266-1267: LGTM!The
get_outlineandset_outlinemethods are correctly registered in thefont_methodsarray with proper signatures (METH_NOARGS and METH_O) and documentation references.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src_c/font.c (1)
970-970: UsePy_RETURN_NONEmacro for consistency.Line 970 returns
Py_Nonedirectly, which is inconsistent with the codebase pattern. For example,font_set_bold(line 341) and other setters usePy_RETURN_NONEto ensure proper reference counting.Apply this diff:
- Py_RETURN_NONE; + Py_RETURN_NONE;Wait, that's the same. Let me re-read line 970...
Actually, looking at the code again:
The code already shows
Py_RETURN_NONEat line 970. The past review comment mentioned line 970 hadreturn Py_None;but the current code showsPy_RETURN_NONE. This means the issue was already fixed.Let me reconsider. The past review comment from coderabbitai[bot] stated:
"Line 970 returnsPy_Nonedirectly which is inconsistent with project patterns; replace the direct return with the Py_RETURN_NONE macro"But the current code at line 970 shows:
This indicates the past issue was already addressed. I should not flag this as a duplicate or an issue.
[actions]
- Approve this segment since the past issue was already fixed.
-->
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/font.c(3 hunks)
⏰ 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). (19)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: dev-check
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: build (windows-latest)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: x86_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: aarch64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
🔇 Additional comments (5)
src_c/font.c (5)
904-919: LGTM!The getter implementation follows the established pattern for font properties: generation check, version gating for SDL_ttf 2.0.12+, and appropriate error handling.
954-960: LGTM!The method implementation correctly delegates to the property getter, following the established pattern used by other
get_*methods in the file.
962-971: LGTM!The method implementation correctly delegates to the property setter and uses
Py_RETURN_NONE, following the established pattern used by otherset_*methods in the file. The past review concern about usingPy_RETURN_NONEhas been addressed.
1240-1241: LGTM!The property registration follows the established pattern for other font properties, correctly linking the getter, setter, and documentation.
1266-1267: LGTM!The method registration follows the established pattern for other font methods, with appropriate flags and documentation references.
Starbuck5
added
New API
|
I haven't looked through the code, but just reading for interface I see you've added this functionality as a getter/setter and as a property. While most things in font are duplicated in this way, it's because things are being off boarded from being getter/setters. So I see no reason to add getter/setters in a new API, if it's going to be available as a property too. Although, in the SDL3 documentation for this function it specifies that this can clear a cache of generated glyphs: https://wiki.libsdl.org/SDL3_ttf/TTF_SetFontOutline. So maybe a getter/setter is better because it implies more action taken by the backend? My thought is we should only support 1 modern thing with a new API, and that it should be a property instead of getter/setter. |
Preferring properties for new values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src_c/font.c (1)
921-953: Excellent validation and error handling.The setter implementation is thorough and well-structured:
- Generation check prevents use after module quit
- Proper deletion prevention with
DEL_ATTR_NOT_SUPPORTED_CHECK- Type validation ensures integer input
- Conversion error handling with
PyErr_Occurredcheck- Non-negative constraint enforcement
- SDL_ttf version gating with clear error message
Performance note: According to SDL3 documentation and the PR discussion,
TTF_SetFontOutlinemay clear an internal glyph cache, so setting outline repeatedly in a hot path could impact performance. Consider documenting this in the module docstring or user-facing documentation if users are expected to change outline dynamically during rendering.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/font_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (4)
buildconfig/stubs/pygame/font.pyi(1 hunks)docs/reST/ref/font.rst(1 hunks)src_c/font.c(2 hunks)test/font_test.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/font_test.py
- buildconfig/stubs/pygame/font.pyi
⏰ 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: arm64
- GitHub Check: x86_64
- GitHub Check: x86
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: AMD64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: aarch64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-latest)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (3)
src_c/font.c (2)
904-919: LGTM!The getter implementation is correct and consistent with other font property getters. Generation check and SDL_ttf version gating (2.0.12+) are properly applied.
1222-1223: Property registration is correct.The outline property is properly registered in the
font_getsetstable with appropriate getter, setter, and docstring reference.docs/reST/ref/font.rst (1)
310-324: Well-documented attribute.The documentation clearly explains the outline behavior:
- Type specification (int)
- Semantic explanation (0 = normal, positive = hollow outline, negative = invalid)
- Proper version annotation (2.5.6)
The description about larger values producing thicker outlines and the suggestion to draw outlined text underneath unoutlined text for an outline effect provides helpful context for users.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/font_test.py (1)
688-722: LGTM! Comprehensive property testing.The test covers the essential cases: type validation, non-negative constraint, arithmetic operations, and error handling. The pattern follows other font property tests well.
Optional: Consider adding overflow test.
Following the pattern in
test_set_linesize(lines 410-413), you could add an overflow check:def test_incorrect_type(): f.outline = "2" self.assertRaises(ValueError, test_neg) self.assertRaises(TypeError, test_incorrect_type) + + # Optional: test overflow + with self.assertRaises(OverflowError): + f.outline = 2**100
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/font_test.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/font_test.py (3)
src_c/font.c (2)
font(1477-1552)get_default_font(1413-1417)buildconfig/stubs/pygame/font.pyi (19)
get_sdl_ttf_version(15-15)init(12-12)get_default_font(16-16)Font(31-93)outline(61-61)outline(63-63)bold(37-37)bold(39-39)italic(41-41)italic(43-43)underline(45-45)underline(47-47)strikethrough(49-49)strikethrough(51-51)set_bold(78-78)set_italic(80-80)set_underline(74-74)set_strikethrough(76-76)render(65-72)buildconfig/stubs/pygame/base.pyi (1)
error(6-6)
⏰ 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: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: x86
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
🔇 Additional comments (4)
test/font_test.py (4)
723-741: LGTM! Proper stub behavior verification.Correctly tests that both getter and setter raise
pygame.erroron unsupported SDL_TTF versions, ensuring graceful degradation.
1090-1094: LGTM! Correct version-gated property registration.The outline property is appropriately added to the test's property list for SDL_TTF 2.0.12+, matching the implementation's version requirements.
1152-1201: LGTM! Clean integration into interactive test infrastructure.The outline parameter integrates well into the existing
querymethod flow. The version guards (lines 1191-1192, 1200-1201) are defensive programming — they prevent crashes whenqueryis called from tests without version skips, whiletest_outlineitself is properly version-gated.
1243-1248: LGTM! Appropriate interactive visual test.The test provides manual verification of outline rendering with proper version gating, consistent with other visual font tests.
This is my first 'meaty' PR, so let me know if I missed anything.
Adds wrappers for TTF_SetFontOutline and TTF_GetFontOutline. As-is I don't believe there's a clean way to outline fonts in pygame.
Visual demo of the outline effect:
