Skip to content

fix: stop caching fonts while no display mode is set - #67

Merged
dmccoystephenson merged 2 commits into
mainfrom
fix/font-cache-without-display
Nov 6, 2026
Merged

dmccoystephenson merged 2 commits into
mainfrom
fix/font-cache-without-display

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

drawText was found to segfault after pygame.quit()/pygame.init() when no display mode is set, and drawButton was found to carry two undocumented requirements. All three concern a Graphik used without a display mode, or with arguments the docs did not cover.

  • _getFont (bug fix): the font cache detects a restart by comparing pygame.display.get_surface(). That value is None on both sides of a restart when no display mode is set, so a freed Font was 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 raises pygame.error when the video system is not initialized, even for an off-screen surface, and that sizeText must be an int. The class docstring's list of fractional-argument exceptions is corrected to match.
  • Tests: four are added (142 passed, 138 before).
    • test_draw_text_skips_the_font_cache_while_no_display_mode_is_set
    • test_draw_text_survives_a_pygame_restart_with_no_display_mode, which runs in a subprocess so a segfault cannot take the test run down
    • test_draw_button_rejects_a_fractional_text_size
    • test_draw_button_requires_the_video_system_even_for_an_off_screen_surface

No public signature is changed, so the Phase 3 consumer-impact gate does not apply. The one performance trade-off is limited to drawText with no display mode, which now rebuilds its font on each call. A crash is judged worse than that cost, and pygame.font.Font was what every call did before fonts were cached.

Closes #65
Closes #66

Test plan

  • python3 -m py_compile src/main/python/preponderous/graphik/graphik.py
  • Import smoke test: from preponderous.graphik import Graphik
  • python3 -m pytest: 142 passed. The conftest forces the dummy SDL drivers, so this is headless.
  • Stash-and-run: with graphik.py at main, the two font tests fail (assert 1 == 2, and Fatal Python error: (pygame parachute) Segmentation Fault). With the fix they pass.
  • CI matrix (3.9 to 3.13)

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:

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

dmccoystephenson and others added 2 commits November 6, 2026 01:55
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>
@dmccoystephenson

Copy link
Copy Markdown
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 (Could not resolve user with login 'copilot').

  • Scope: PASS. Three files changed: _getFont plus docstrings, the README's drawButton and fractional-argument statements, and four tests. Every hunk maps to drawText segfaults after a pygame restart when no display mode is set #65 or docs: drawButton's video-system and integer sizeText requirements are undocumented #66.
  • Tests-new: PASS (not applicable). No public method was added.
  • Tests-fix: PASS. Run empirically with graphik.py at main: test_draw_text_skips_the_font_cache_while_no_display_mode_is_set failed with assert 1 == 2 and test_draw_text_survives_a_pygame_restart_with_no_display_mode failed with Fatal Python error: (pygame parachute) Segmentation Fault. Both pass with the fix.
  • Sibling structure: PASS. The tests sit beside their counterparts in the font-cache, fractional-size and window-coordinate groups, reuse _count_font_constructions, _make_graphik and _rgb, and follow the subprocess pattern of test_package_imports_without_pygame.
  • Sibling renames: PASS (not applicable). Nothing was renamed.
  • Docs: PASS. The README, class docstring and method docstrings agree. The README's Dependencies section is unaffected, and no version statement changes. The font cache is not mentioned in the README, so only the drawText docstring needed the new caveat.
  • Issue resolution: PASS. drawText segfaults after a pygame restart when no display mode is set #65's crash no longer reproduces, and both requirements named in docs: drawButton's video-system and integer sizeText requirements are undocumented #66 are documented and pinned.
  • Manual validation: PASS. py_compile, the import smoke test and pytest (142 passed) are green locally, and all five CI jobs are green on 96d19ab.
  • camelCase: PASS. No public method was added or renamed.
  • Backward-compat: PASS. No signature changed, and no call that succeeded before now fails, so the consumer-impact gate does not apply. gh search code was not run, for that reason.
  • Headless: PASS. The new tests ran under the dummy driver locally and in CI. The subprocess test inherits the conftest's environment.
  • Version sync: PASS. No version string was touched.
  • No new deps: PASS. No import was added.

Do-not-auto-merge paths: none matched. .github/workflows/, LICENSE and the version source are untouched, and no file deletes more than 50 lines.

Finding fixed during this review:

  • src/test/python/preponderous/graphik/test_graphik.py:1596 — a comment said the other draw methods never touch the video system, which is false for drawImage (convert_alpha). It was reworded in 96d19ab.

Findings accepted without change:

  • src/main/python/preponderous/graphik/graphik.py:131 — drawText on an off-screen surface with no display mode now builds a Font on every call, which is slower. This is judged acceptable: the alternative is a crash, and setting a display mode restores caching.
  • src/main/python/preponderous/graphik/graphik.py:131 — pygame.font.quit() followed by pygame.font.init() with a display still set remains uncovered. It is the limitation already recorded in the _getFont comment and is unrelated to this change.
  • src/test/python/preponderous/graphik/test_graphik.py:770 — the subprocess test was only seen to fail on pygame 2.1.2. How a stale Font behaves on the pygame versions CI installs was not observed, so this test may not discriminate there. test_draw_text_skips_the_font_cache_while_no_display_mode_is_set counts Font constructions and is independent of that.
  • The PR description's CI checkbox was written before CI finished and is left unticked. CI's result is recorded here.

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

@dmccoystephenson
dmccoystephenson merged commit 745e35d into main Nov 6, 2026
5 checks passed
@dmccoystephenson
dmccoystephenson deleted the fix/font-cache-without-display branch November 6, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant