From 2cfbc1e9692536788c56fa454d5afaabbaccaa30 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 10 Sep 2026 11:03:17 -0400 Subject: [PATCH] fix(element-style): a wrong-size BDF now keeps its font, not its size BDF fonts are fixed-size bitmap strikes: FreeType accepts only the pixel size baked into the file and raises for anything else. 32 of the 35 shipped fonts are BDF, so a size picked in the web UI usually is not a valid strike -- and load_font caught that failure with its generic "unloadable font" handler, which substitutes PressStart2P. Asking for 5x7.bdf at size 10 therefore rendered a completely different typeface, silently. It now falls back to the file's own native size instead, which is what SportsCore._load_custom_font_from_element_config has always done. The native size is read via FontManager._read_bdf_native_size rather than a fourth copy of that parser, matching how core.py already delegates. Also here, because they are the same code path: - native_bdf_size() is exposed for the web UI, which needs to know when a size field can take effect at all. None means "free choice". - ElementStyle.font_size now reports the size actually realised rather than the one requested. Callers lay out from it, and reserving space for a size nothing was drawn at is how this surfaces. - The module font cache is a bounded LRU (256) instead of an unbounded dict. The display process runs for weeks and every config save can add a (font, size) pair; every other hot cache in the codebase is bounded this way. Untouched configs are unaffected: the shipped classic fonts are the three TTFs, so nothing was hitting the substitution path by default. Co-Authored-By: Claude Opus 5 --- src/element_style.py | 148 ++++++++++++++++++++++++++++++------- test/test_element_style.py | 96 ++++++++++++++++++++++++ 2 files changed, 219 insertions(+), 25 deletions(-) diff --git a/src/element_style.py b/src/element_style.py index 12b1abb2..12bcdfef 100644 --- a/src/element_style.py +++ b/src/element_style.py @@ -47,6 +47,7 @@ import copy import json import logging import os +from collections import OrderedDict from dataclasses import dataclass from typing import Any, Dict, Optional, Tuple, Union @@ -69,9 +70,25 @@ _FONTS_SUBDIR = os.path.join('assets', 'fonts') # Last-resort font when a requested file can't be found or loaded. _FALLBACK_FONT_NAME = 'PressStart2P-Regular.ttf' -# (resolved absolute path, size) -> loaded font face. BDF faces are stateful -# in principle, but the core's own FontManager shares faces the same way. -_font_cache: Dict[Tuple[str, int], Any] = {} +# (resolved absolute path, requested size) -> (font face, realised size). +# BDF faces are stateful in principle, but the core's own FontManager shares +# faces the same way. +# +# Bounded LRU rather than the unbounded dict this started as: the display +# process runs for weeks, and every config save can introduce a new +# (font, size) pair. 256 is far above the working set -- a panel draws from a +# handful of faces -- while still having a ceiling. Matches the house style of +# every other hot cache (display_manager, font_manager, adaptive_layout). +_FONT_CACHE_MAX = 256 +_font_cache: 'OrderedDict[Tuple[str, int], Tuple[Any, int]]' = OrderedDict() + + +def _cache_put(key: Tuple[str, int], value: Tuple[Any, int]) -> None: + """Insert, evicting the least recently used entry past the bound.""" + _font_cache[key] = value + _font_cache.move_to_end(key) + while len(_font_cache) > _FONT_CACHE_MAX: + _font_cache.popitem(last=False) # Config keys a style element block carries, in schema/UI order. _STYLE_KEYS = ('font', 'font_size', 'text_color') @@ -124,13 +141,91 @@ def resolve_font_path(font_name: str) -> Optional[str]: return None +def native_bdf_size(font_name: str) -> Optional[int]: + """The one pixel size a BDF font can render at, or None. + + None means "not a BDF, not found, or unreadable" — i.e. the size is a + free choice. The web UI uses this to lock the size field for a bitmap + font instead of offering a number that cannot take effect. + """ + path = resolve_font_path(font_name) + if path is None or not path.lower().endswith('.bdf'): + return None + return _read_bdf_native_size(path) + + +def _read_bdf_native_size(path: str) -> Optional[int]: + """A BDF file's own pixel size, delegated to FontManager. + + Deliberately not reimplemented: FontManager's reader prefers PIXEL_SIZE + over the SIZE line's point-size (they differ on the several bundled + fonts defined at 75dpi) and stops at the first STARTCHAR. Core always + ships it; the guard is for the plugin test harnesses that stub the + module out. + """ + try: + from src.font_manager import FontManager + return FontManager._read_bdf_native_size(path) + except Exception: # pragma: no cover - defensive + return None + + +def _load_bdf(path: str, size: int) -> Tuple[Any, int]: + """A ``freetype.Face`` for a BDF file at the closest size it can do. + + BDF fonts are fixed-size bitmap strikes, not scalable outlines: + FreeType accepts only the exact pixel size baked into the file and + raises for anything else. 32 of the 35 shipped fonts are BDF, so a + size the user picked in the web UI usually is not a valid strike. + + Retrying at the file's native size is the behaviour SportsCore already + has (``_load_custom_font_from_element_config``). Without it this + function fell through to the generic except below and returned + *PressStart2P* — so choosing 5x7.bdf at size 10 silently rendered a + completely different typeface rather than 5x7 at 7px. + """ + if freetype is None: + raise RuntimeError("freetype not available for BDF fonts") + + def _face_at(px: int) -> Any: + face = freetype.Face(path) + # Character size in 1/64th points at 72dpi == pixel size. + face.set_char_size(px * 64, px * 64, 72, 72) + return face + + try: + return _face_at(size), size + except Exception: + native = _read_bdf_native_size(path) + if not native or native == size: + raise + # A fresh Face: the first one already took a failed set_char_size. + face = _face_at(native) + logger.debug("BDF font %s loaded at its native size %s " + "(requested %s is not a strike in this file)", + path, native, size) + return face, native + + def load_font(font_name: str, size: int) -> Any: """Load a font by filename at a pixel size, with caching and fallback. ``.bdf`` files load as ``freetype.Face`` (matching FontManager), other - files through ``PIL.ImageFont.truetype``. A missing or unloadable font - degrades to ``PressStart2P-Regular.ttf`` at the requested size, then to - PIL's built-in default — this function never raises. + files through the pinned ``load_truetype``. A BDF asked for a size it + has no strike for falls back to its own native size (see + :func:`_load_bdf`), not to a different font. A missing or unloadable + font degrades to ``PressStart2P-Regular.ttf`` at the requested size, + then to PIL's built-in default — this function never raises. + """ + return _load_font_sized(font_name, size)[0] + + +def _load_font_sized(font_name: str, size: int) -> Tuple[Any, int]: + """``load_font`` plus the pixel size actually realised. + + The two differ only for a BDF snapped to its native strike. Callers + that lay out by size (line heights, ladders) need the realised value, + or they reserve space for a size nothing was drawn at. """ try: size = max(1, int(size)) @@ -140,47 +235,44 @@ def load_font(font_name: str, size: int) -> Any: path = resolve_font_path(font_name) if path is None: logger.warning("Font file not found: %s, using fallback", font_name) - return _load_fallback_font(size) + return _load_fallback_font(size), size cache_key = (path, size) cached = _font_cache.get(cache_key) if cached is not None: + _font_cache.move_to_end(cache_key) return cached try: if path.lower().endswith('.bdf'): - if freetype is None: - raise RuntimeError("freetype not available for BDF fonts") - face = freetype.Face(path) - # Character size in 1/64th points at 72dpi == pixel size. - face.set_char_size(size * 64, size * 64, 72, 72) - font: Any = face + font, effective = _load_bdf(path, size) else: - font = load_truetype(path, size) + font, effective = load_truetype(path, size), size except Exception as e: logger.warning("Error loading font %s at %spx: %s, using fallback", path, size, e) return _load_fallback_font(size) - _font_cache[cache_key] = font - return font + _cache_put(cache_key, (font, effective)) + return font, effective -def _load_fallback_font(size: int) -> Any: +def _load_fallback_font(size: int) -> Tuple[Any, int]: """PressStart2P at the requested size, else PIL's built-in default.""" path = resolve_font_path(_FALLBACK_FONT_NAME) if path is not None: cache_key = (path, size) cached = _font_cache.get(cache_key) if cached is not None: + _font_cache.move_to_end(cache_key) return cached try: - font = load_truetype(path, size) - _font_cache[cache_key] = font - return font + entry = (load_truetype(path, size), size) + _cache_put(cache_key, entry) + return entry except Exception as e: logger.error("Error loading fallback font: %s", e) - return ImageFont.load_default() + return ImageFont.load_default(), size # --------------------------------------------------------------------------- @@ -592,12 +684,17 @@ class ElementStyleResolver: color = (_normalize_color(classic_color) or default_color or (255, 255, 255)) + # font_size reports what was actually realised, which differs from + # the request only when a BDF snapped to its native strike. Callers + # lay out from this value; reporting the request would reserve space + # for a size nothing was drawn at. + font, realised_size = _load_font_sized(font_name, font_size) return ElementStyle( - font=load_font(font_name, font_size), + font=font, color=color, offset=self.offset(element_key), font_name=font_name, - font_size=font_size, + font_size=realised_size, user_forced=user_forced, user_forced_color=bool(color_forced), ) @@ -607,12 +704,13 @@ class ElementStyleResolver: """The untouched fallback style — used when resolution itself fails, so ``style()`` can keep its never-raises promise.""" size = self._coerce_size(classic_size, 8) + font, realised_size = _load_font_sized(classic_font, size) return ElementStyle( - font=load_font(classic_font, size), + font=font, color=_normalize_color(classic_color) or (255, 255, 255), offset=(0, 0), font_name=classic_font, - font_size=size, + font_size=realised_size, user_forced=False, user_forced_color=False, ) diff --git a/test/test_element_style.py b/test/test_element_style.py index 8d0535ab..39f2ce3a 100644 --- a/test/test_element_style.py +++ b/test/test_element_style.py @@ -24,6 +24,8 @@ from PIL import ImageFont from src.element_style import ( ElementStyleResolver, + _load_font_sized, + native_bdf_size, defaults_from_schema, defaults_from_schema_file, expand_style_elements, @@ -410,3 +412,97 @@ class TestResolverPlumbing: cust = schema["properties"]["customization"]["properties"] assert cust["title_text"]["x-style-managed"] is True assert "title_text" in cust["layout"]["properties"] +class TestBdfSizing: + """A BDF is a fixed-size bitmap strike, not a scalable outline. + + FreeType accepts only the exact pixel size baked into the file and raises + for anything else, and 32 of the 35 shipped fonts are BDF -- so a size + picked in the web UI usually is not a valid strike. This used to fall + through to the generic font-load except and return *PressStart2P*, so + asking for 5x7.bdf at size 10 silently rendered a different typeface. + It now falls back to the file's own size instead, matching what + SportsCore._load_custom_font_from_element_config already did. + """ + + def test_a_valid_strike_loads_at_that_size(self): + import freetype + font, realised = _load_font_sized("5x7.bdf", 7) + assert isinstance(font, freetype.Face) + assert realised == 7 + + @pytest.mark.parametrize("requested", [4, 10, 16, 32]) + def test_a_wrong_size_keeps_the_font_and_snaps_the_size(self, requested): + """The regression: the face must still be 5x7, not a substitute.""" + import freetype + font, realised = _load_font_sized("5x7.bdf", requested) + assert isinstance(font, freetype.Face), ( + "a BDF asked for a bad size used to come back as a PIL " + "PressStart2P face -- a different font entirely") + assert realised == 7 + + def test_the_reported_size_is_what_was_realised(self, style_schema_path): + """ElementStyle.font_size drives caller layout, so it must not report + a size nothing was drawn at.""" + config = {"customization": {"title_text": {"font": "5x7.bdf", + "font_size": 20}}} + style = ElementStyleResolver( + config, defaults_from_schema_file(style_schema_path) + ).style("title_text", classic_font="PressStart2P-Regular.ttf", + classic_size=8) + assert style.font_name == "5x7.bdf" + assert style.font_size == 7 + + def test_native_bdf_size_reads_the_file(self): + assert native_bdf_size("5x7.bdf") == 7 + + @pytest.mark.parametrize("name", ["PressStart2P-Regular.ttf", + "no-such-font.bdf", "", None]) + def test_native_bdf_size_is_none_when_size_is_a_free_choice(self, name): + """None is the web UI's signal that the size field stays editable.""" + assert native_bdf_size(name) is None + + def test_a_scalable_font_realises_the_requested_size(self): + for size in (6, 8, 13): + font, realised = _load_font_sized("PressStart2P-Regular.ttf", size) + assert realised == size + assert not isinstance(font, type(None)) + + +class TestFontCacheIsBounded: + """The display process runs for weeks and every config save can add a new + (font, size) pair; this cache was unbounded, against the house style of + every other hot cache in the codebase.""" + + def test_the_cache_evicts_past_its_bound(self): + import src.element_style as es + es._font_cache.clear() + try: + for size in range(1, es._FONT_CACHE_MAX + 40): + load_font("PressStart2P-Regular.ttf", size) + assert len(es._font_cache) <= es._FONT_CACHE_MAX + finally: + es._font_cache.clear() + + def test_a_repeated_load_is_the_same_object(self): + import src.element_style as es + es._font_cache.clear() + try: + first = load_font("PressStart2P-Regular.ttf", 8) + assert load_font("PressStart2P-Regular.ttf", 8) is first + finally: + es._font_cache.clear() + + def test_eviction_is_least_recently_used(self): + import src.element_style as es + es._font_cache.clear() + try: + oldest = load_font("PressStart2P-Regular.ttf", 1) + for size in range(2, es._FONT_CACHE_MAX + 1): + load_font("PressStart2P-Regular.ttf", size) + # Touch the oldest so it is no longer the eviction candidate, + # then overflow by one. + assert load_font("PressStart2P-Regular.ttf", 1) is oldest + load_font("PressStart2P-Regular.ttf", es._FONT_CACHE_MAX + 1) + assert load_font("PressStart2P-Regular.ttf", 1) is oldest + finally: + es._font_cache.clear()