Repository navigation
fix: stop caching fonts while no display mode is set - #67
Merged
Merged
Conversation
With no display mode, get_surface() is None on both sides of a pygame restart, so the session check missed it and drawText used a freed Font and segfaulted. Fonts are now built per call until a display exists. Also documents and pins drawButton's need for the video system and an int sizeText. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
drawImage also needs the video system, through convert_alpha, so the comment now names drawRectangle and drawText. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
Self-review rubric, scored against the diff at 96d19ab and CI run 42191332458. This is an inline review by the authoring session, not an independent one. A Copilot review could not be requested (
Do-not-auto-merge paths: none matched. Finding fixed during this review:
Findings accepted without change:
Verdict: ready to merge. This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener). drafted by Claude on behalf of Daniel Stephenson |
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
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.
Summary
drawTextwas found to segfault afterpygame.quit()/pygame.init()when no display mode is set, anddrawButtonwas found to carry two undocumented requirements. All three concern aGraphikused without a display mode, or with arguments the docs did not cover._getFont(bug fix): the font cache detects a restart by comparingpygame.display.get_surface(). That value isNoneon both sides of a restart when no display mode is set, so a freedFontwas reused. Fonts are no longer cached in that state and are built on each call instead. Caching is unchanged once a display mode is set.drawButton(docs): the docstring and README now state that it raisespygame.errorwhen the video system is not initialized, even for an off-screen surface, and thatsizeTextmust be anint. The class docstring's list of fractional-argument exceptions is corrected to match.test_draw_text_skips_the_font_cache_while_no_display_mode_is_settest_draw_text_survives_a_pygame_restart_with_no_display_mode, which runs in a subprocess so a segfault cannot take the test run downtest_draw_button_rejects_a_fractional_text_sizetest_draw_button_requires_the_video_system_even_for_an_off_screen_surfaceNo public signature is changed, so the Phase 3 consumer-impact gate does not apply. The one performance trade-off is limited to
drawTextwith no display mode, which now rebuilds its font on each call. A crash is judged worse than that cost, andpygame.font.Fontwas what every call did before fonts were cached.Closes #65
Closes #66
Test plan
python3 -m py_compile src/main/python/preponderous/graphik/graphik.pyfrom preponderous.graphik import Graphikpython3 -m pytest: 142 passed. The conftest forces the dummy SDL drivers, so this is headless.graphik.pyatmain, the two font tests fail (assert 1 == 2, andFatal Python error: (pygame parachute) Segmentation Fault). With the fix they pass.Local validation used pygame 2.1.2 on Python 3.8, which is below the supported floor. CI is the check against current pygame versions. The segfault itself was only observed on 2.1.2.
Backlog
Each open issue not picked was skipped for a stated reason:
.github/workflows/edits, which are on the do-not-auto-merge list. Opening them as held PRs would block later dispatches.This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).
🤖 Generated with Claude Code
drafted by Claude on behalf of Daniel Stephenson