From 3f920f28991705da65229aa0b2f72b22474611a9 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Mon, 5 Oct 2026 08:26:26 -0400 Subject: [PATCH] fix(fonts): unloading a plugin forgets its manifest fonts (#757) * fix(fonts): unloading a plugin forgets its manifest fonts PluginManager.unload_plugin() and the failed-load cleanup only called FontManager.forget_manager_fonts(), which drops usage data. The plugin's manifest registrations stayed: plugin_fonts / plugin_font_catalogs, its plugin_id::family entries in font_catalog, and cached font objects for them -- so its fonts kept resolving after unload and a family a reinstalled manifest dropped stayed registered. The deprecated unregister_plugin_fonts did this cleanup but nothing called it; it was removed in #708. Add FontManager.forget_plugin_fonts(plugin_id) and call it from both paths alongside forget_manager_fonts (each guarded on its own, so a font manager stub with only one still works). FontManager takes no locks, so like forget_manager_fonts it uses atomic pops over snapshots. A reload (unload + load) registers the manifest fonts again and they resolve. Raised by CodeRabbit on #709. Co-Authored-By: Claude Opus 5.5 * fix(fonts): call the font manager's forget methods without a None-able local Pylint E1102 (Codacy) read getattr(..., None) as possibly not callable. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 8 + docs/FONT_MANAGER.md | 4 +- src/font_manager.py | 33 ++++ src/plugin_system/plugin_manager.py | 29 ++-- test/test_font_manager.py | 143 ++++++++++++++++++ ...test_plugin_manager_failed_load_cleanup.py | 1 + 6 files changed, 206 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4607b5e0..71c3fbe6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -162,6 +162,14 @@ Internal; no behaviour change. Stage 3 of `docs/RUN_LOOP_REDESIGN.md`. ### Fixed +- Unloading a plugin now forgets the fonts its manifest registered, not only + the fonts it reported using. Its `plugin_id::family` entries kept resolving + and their cached font objects stayed alive until a restart, and a family a + reinstalled plugin's manifest dropped stayed registered. The new + `FontManager.forget_plugin_fonts(plugin_id)` does the cleanup; + `PluginManager.unload_plugin()` and a failed load call it alongside + `forget_manager_fonts()`, and a reload registers the manifest's fonts again. + - The web preview and `/api/v3/display/current` no longer stay black for a whole screen that draws its card once and then holds it. The snapshot is written from `update_display()` at most once per write interval, so a frame diff --git a/docs/FONT_MANAGER.md b/docs/FONT_MANAGER.md index 6d2dc61f..9ad0cb17 100644 --- a/docs/FONT_MANAGER.md +++ b/docs/FONT_MANAGER.md @@ -203,6 +203,7 @@ Current methods: | `measure_text(text, font)` | `(width, height, baseline)` | | `get_font_height(font)` | Line height | | `register_plugin_fonts(plugin_id, font_manifest)` | Register a plugin's fonts (core calls it at load) | +| `forget_plugin_fonts(plugin_id)` | Drop a plugin's manifest fonts and their cached objects (core calls it when a plugin unloads) | | `clear_cache()` | Drop cached fonts and metrics | | `font_catalog` (attribute) | Family name → file path | @@ -218,5 +219,6 @@ Removed in 3.8.0, after logging a deprecation warning on first call since | `get_performance_stats()` | — | | `set_override()`, `remove_override()`, `get_overrides()` | a font field in your plugin's config schema | | `get_manager_fonts()`, `get_detected_fonts()` | — | -| `get_plugin_fonts()`, `unregister_plugin_fonts()` | — | +| `get_plugin_fonts()` | — | +| `unregister_plugin_fonts()` | `forget_plugin_fonts()` (core calls it on unload) | | `add_font()`, `remove_font()`, `validate_font()` | the web UI's Fonts tab | diff --git a/src/font_manager.py b/src/font_manager.py index c722c712..17990983 100644 --- a/src/font_manager.py +++ b/src/font_manager.py @@ -222,6 +222,39 @@ class FontManager: logger.error(f"Error registering fonts for plugin {plugin_id}: {e}", exc_info=True) return False + def forget_plugin_fonts(self, plugin_id: str) -> bool: + """Drop the fonts ``plugin_id``'s manifest registered: its manifest + and catalog, its ``plugin_id::family`` entries in font_catalog, and + cached font objects for those families. Called by core when a plugin + is unloaded, so a reload registers from its current manifest and a + removed plugin's fonts stop resolving. + + FontManager takes no locks; like forget_manager_fonts this relies on + single dict operations being atomic and iterates snapshots, so a + render thread calling get_font() meanwhile cannot break it. Returns + True if the plugin had registered fonts. + """ + prefix = f"{plugin_id}::" + manifest = self.plugin_fonts.pop(plugin_id, None) + catalog = self.plugin_font_catalogs.pop(plugin_id, None) + # Every namespaced entry, not just the families in the catalog: one + # whose file failed to load never made it into the catalog, and a + # caller may have added one directly. + for family in list(self.font_catalog): + if family.startswith(prefix): + self.font_catalog.pop(family, None) + # get_font() keys the cache f"{family}_{size_px}". + dropped = [key for key in list(self.font_cache) if key.startswith(prefix)] + for key in dropped: + self.font_cache.pop(key, None) + if dropped: + # Font objects someone may hold were dropped; see cache_generation. + self.cache_generation += 1 + if manifest is None and catalog is None: + return False + logger.info("Forgot fonts of plugin %s", plugin_id) + return True + def _validate_font_manifest(self, font_manifest: Dict[str, Any]) -> bool: """Validate the structure of a plugin's font manifest.""" required_fields = ["fonts"] diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 1d2961fb..2ff6fd1f 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -601,11 +601,21 @@ class PluginManager: self.plugin_loader.unregister_plugin_modules(plugin_id) except Exception as e: # pragma: no cover - defensive self.logger.debug("Could not drop modules of %s: %s", plugin_id, e) - try: - if self.font_manager is not None and hasattr(self.font_manager, 'forget_manager_fonts'): - self.font_manager.forget_manager_fonts(plugin_id) - except Exception as e: - self.logger.debug("Could not forget fonts of %s: %s", plugin_id, e) + self._forget_plugin_fonts(plugin_id) + + def _forget_plugin_fonts(self, plugin_id: str) -> None: + """Drop what the FontManager holds for a plugin: the fonts its + instance reported using (the Fonts tab's "Used by") and the fonts its + manifest registered. Never raises.""" + if self.font_manager is None: + return + for name in ('forget_manager_fonts', 'forget_plugin_fonts'): + if not hasattr(self.font_manager, name): + continue + try: + getattr(self.font_manager, name)(plugin_id) + except Exception as e: + self.logger.debug("Could not forget fonts of %s (%s): %s", plugin_id, name, e) #: Config keys the **core** reads out of a plugin's own config block. The #: plugin never declares them, so a schema with @@ -850,12 +860,9 @@ class PluginManager: # Delegate sub-module and cached-module cleanup to the loader self.plugin_loader.unregister_plugin_modules(plugin_id) - # Its font registrations go with it (the Fonts tab's "Used by"). - try: - if self.font_manager is not None and hasattr(self.font_manager, 'forget_manager_fonts'): - self.font_manager.forget_manager_fonts(plugin_id) - except Exception as e: - self.logger.debug("Could not forget fonts of %s: %s", plugin_id, e) + # Its font registrations go with it: the fonts it reported using + # and the ones its manifest registered. + self._forget_plugin_fonts(plugin_id) # Update state self.state_manager.set_state(plugin_id, PluginState.UNLOADED) diff --git a/test/test_font_manager.py b/test/test_font_manager.py index b377c616..90a3847a 100644 --- a/test/test_font_manager.py +++ b/test/test_font_manager.py @@ -185,6 +185,149 @@ class TestPluginFonts: assert fm.font_catalog["my-plugin::bundled"] == str(plugin_dir / "fonts" / "Bundled.ttf") +class TestForgetPluginFonts: + """forget_plugin_fonts drops what a plugin's manifest registered. Before + it, unloading a plugin left its fonts resolvable and its cached font + objects alive until a restart.""" + + @staticmethod + def _register(fm, root, plugin_id, family="bundled"): + plugin_dir = root / plugin_id + (plugin_dir / "fonts").mkdir(parents=True, exist_ok=True) + font_file = plugin_dir / "fonts" / f"{family}.ttf" + if not font_file.exists(): # a loaded font may hold it open (Windows) + shutil.copy(resolve_asset_path("assets/fonts/PressStart2P-Regular.ttf"), font_file) + manifest = {"fonts": [{"family": family, "source": f"plugin://fonts/{family}.ttf"}]} + assert fm.register_plugin_fonts(plugin_id, manifest, plugin_dir=plugin_dir) + return plugin_dir + + @staticmethod + def _entries_of(fm, plugin_id): + prefix = f"{plugin_id}::" + return { + "plugin_fonts": plugin_id in fm.plugin_fonts, + "plugin_font_catalogs": plugin_id in fm.plugin_font_catalogs, + "font_catalog": [k for k in fm.font_catalog if k.startswith(prefix)], + "font_cache": [k for k in fm.font_cache if k.startswith(prefix)], + } + + NONE = {"plugin_fonts": False, "plugin_font_catalogs": False, + "font_catalog": [], "font_cache": []} + + def test_unload_leaves_no_plugin_entries(self, fm, tmp_path): + self._register(fm, tmp_path, "alpha") + fm.resolve_font("alpha.title", "bundled", 8, plugin_id="alpha") + fm.get_font("alpha::bundled", 10) + assert self._entries_of(fm, "alpha")["font_cache"] # cached before + gen = fm.cache_generation + + assert fm.forget_plugin_fonts("alpha") is True + + assert self._entries_of(fm, "alpha") == self.NONE + assert fm.cache_generation == gen + 1 + # The family no longer resolves to the plugin's file. + assert fm.font_catalog.get("alpha::bundled") is None + + def test_other_plugins_and_core_fonts_are_untouched(self, fm, tmp_path): + self._register(fm, tmp_path, "alpha") + self._register(fm, tmp_path, "beta") + # A plugin whose id is a prefix of another's must not take it along. + self._register(fm, tmp_path, "alpha-two") + for pid in ("alpha", "beta", "alpha-two"): + fm.get_font(f"{pid}::bundled", 8) + core_font = fm.get_font("press_start", 8) + beta_before = self._entries_of(fm, "beta") + alpha_two_before = self._entries_of(fm, "alpha-two") + + fm.forget_plugin_fonts("alpha") + + assert self._entries_of(fm, "beta") == beta_before + assert self._entries_of(fm, "alpha-two") == alpha_two_before + assert fm.get_font("press_start", 8) is core_font + + def test_reload_re_registers_cleanly(self, fm, tmp_path): + plugin_dir = self._register(fm, tmp_path, "alpha") + old = fm.get_font("alpha::bundled", 8) + fm.forget_plugin_fonts("alpha") + + self._register(fm, tmp_path, "alpha") + + assert fm.font_catalog["alpha::bundled"] == str(plugin_dir / "fonts" / "bundled.ttf") + font = fm.resolve_font("alpha.title", "bundled", 8, plugin_id="alpha") + assert isinstance(font, ImageFont.FreeTypeFont) + assert font is not old # loaded fresh, not the dropped cache entry + + def test_a_family_the_new_manifest_drops_stops_resolving(self, fm, tmp_path): + self._register(fm, tmp_path, "alpha", family="old_face") + fm.forget_plugin_fonts("alpha") + self._register(fm, tmp_path, "alpha", family="new_face") + + assert "alpha::old_face" not in fm.font_catalog + assert "alpha::new_face" in fm.font_catalog + + def test_unknown_plugin_is_a_no_op(self, fm): + catalog = dict(fm.font_catalog) + gen = fm.cache_generation + + assert fm.forget_plugin_fonts("never-registered") is False + + assert fm.font_catalog == catalog + assert fm.cache_generation == gen + + +class TestPluginManagerReloadFonts: + """Through PluginManager: unloading a plugin forgets its manifest fonts, + and reload_plugin (unload + load) registers them again so they resolve.""" + + PLUGIN_ID = "font-reload-demo" + MODULE = "plugin_font_reload_demo" + + def test_unload_forgets_and_reload_resolves(self, tmp_path): + import sys + from src.plugin_system.plugin_manager import PluginManager + + plugins_dir = tmp_path / "plugins" + plugin_dir = plugins_dir / self.PLUGIN_ID + (plugin_dir / "fonts").mkdir(parents=True) + shutil.copy(resolve_asset_path("assets/fonts/PressStart2P-Regular.ttf"), + plugin_dir / "fonts" / "Bundled.ttf") + manifest = {"id": self.PLUGIN_ID, "name": "Demo", "class_name": "Demo", + "entry_point": "manager.py", + "fonts": {"fonts": [{"family": "bundled", + "source": "plugin://fonts/Bundled.ttf"}]}} + (plugin_dir / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8") + (plugin_dir / "manager.py").write_text( + "class Demo:\n" + " def __init__(self, plugin_id, config, display_manager, cache_manager, plugin_manager):\n" + " self.enabled = True\n", encoding="utf-8") + + pm = PluginManager(plugins_dir=str(plugins_dir)) + fm = FontManager({}) + pm.font_manager = fm + pm.plugin_manifests[self.PLUGIN_ID] = manifest + key = f"{self.PLUGIN_ID}::bundled" + try: + assert pm.load_plugin(self.PLUGIN_ID) is True + assert key in fm.font_catalog + fm.register_manager_font(self.PLUGIN_ID, "demo.title", "bundled", 8) + old = fm.resolve_font("demo.title", "bundled", 8, plugin_id=self.PLUGIN_ID) + + assert pm.unload_plugin(self.PLUGIN_ID) is True + assert self.PLUGIN_ID not in fm.plugin_fonts + assert self.PLUGIN_ID not in fm.plugin_font_catalogs + assert key not in fm.font_catalog + assert not [k for k in fm.font_cache if k.startswith(f"{self.PLUGIN_ID}::")] + assert self.PLUGIN_ID not in fm.manager_fonts + + assert pm.reload_plugin(self.PLUGIN_ID) is True + assert fm.font_catalog[key] == str(plugin_dir / "fonts" / "Bundled.ttf") + font = fm.resolve_font("demo.title", "bundled", 8, plugin_id=self.PLUGIN_ID) + assert isinstance(font, ImageFont.FreeTypeFont) + assert font is not old + finally: + sys.modules.pop(self.MODULE, None) + + class TestDownloadFont: """_download_font: plugin fonts declared by URL, cached in temp_font_dir.""" diff --git a/test/test_plugin_manager_failed_load_cleanup.py b/test/test_plugin_manager_failed_load_cleanup.py index a5e10824..f6e8fc00 100644 --- a/test/test_plugin_manager_failed_load_cleanup.py +++ b/test/test_plugin_manager_failed_load_cleanup.py @@ -69,6 +69,7 @@ def test_fixed_plugin_loads_new_code_after_failed_load(plugin_env, first_source) assert MODULE_NAME not in sys.modules assert PLUGIN_ID not in pm.plugin_loader._loaded_modules pm.font_manager.forget_manager_fonts.assert_called_with(PLUGIN_ID) + pm.font_manager.forget_plugin_fonts.assert_called_with(PLUGIN_ID) (plugin_dir / "manager.py").write_text(_FIXED, encoding="utf-8") assert pm.load_plugin(PLUGIN_ID) is True