diff --git a/src/display_controller.py b/src/display_controller.py index 4ebe580a..da70a5e9 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -24,7 +24,7 @@ import os import json from pathlib import Path -from typing import Dict, Any, List, Optional +from typing import Dict, Any, List, Optional, Callable from datetime import datetime from concurrent.futures import ThreadPoolExecutor, as_completed # pylint: disable=no-name-in-module import pytz @@ -163,6 +163,13 @@ def _follower_gated_update(): self.plugin_modes = {} # mode -> plugin_instance mapping for plugin-first dispatch self.mode_to_plugin_id: Dict[str, str] = {} self.plugin_display_modes: Dict[str, List[str]] = {} + # Per-plugin config-change callbacks, kept so we can unsubscribe a + # plugin when it is disabled live. + self._plugin_config_callbacks: Dict[str, Callable] = {} + # Set by the config-watcher thread when the enabled-plugin set changes; + # the main run loop reconciles (loads/unloads) on its own thread so + # mutating available_modes never races with rendering. + self._pending_plugin_reconcile = False self.on_demand_active = False self.on_demand_mode: Optional[str] = None self.on_demand_modes: List[str] = [] # All modes for the on-demand plugin @@ -331,47 +338,10 @@ def load_single_plugin(plugin_id): logger.info("✓ Loaded plugin %s in %.3f seconds (%d/%d)", plugin_id, result['load_time'], loaded_count, enabled_count) - # Get plugin instance and manifest - plugin_instance = self.plugin_manager.get_plugin(plugin_id) - manifest = self.plugin_manager.plugin_manifests.get(plugin_id, {}) - - # Prefer plugin's modes attribute if available (dynamic based on enabled leagues) - # Fall back to manifest display_modes if plugin doesn't provide modes - if plugin_instance and hasattr(plugin_instance, 'modes') and plugin_instance.modes: - display_modes = list(plugin_instance.modes) - logger.debug("Using plugin.modes for %s: %s", plugin_id, display_modes) - else: - display_modes = manifest.get('display_modes', [plugin_id]) - logger.debug("Using manifest display_modes for %s: %s", plugin_id, display_modes) - - if isinstance(display_modes, list) and display_modes: - self.plugin_display_modes[plugin_id] = list(display_modes) - else: - display_modes = [plugin_id] - self.plugin_display_modes[plugin_id] = list(display_modes) - - # Subscribe plugin to config changes for hot-reload - if hasattr(self, 'config_service') and hasattr(plugin_instance, 'on_config_change'): - def config_change_callback(old_config: Dict[str, Any], new_config: Dict[str, Any]) -> None: - """Callback for plugin config changes.""" - try: - plugin_instance.on_config_change(new_config) - logger.debug("Plugin %s notified of config change", plugin_id) - except Exception as e: - logger.error("Error in plugin %s config change handler: %s", plugin_id, e, exc_info=True) - - self.config_service.subscribe(config_change_callback, plugin_id=plugin_id) - logger.debug("Subscribed plugin %s to config changes", plugin_id) - - # Add plugin modes to available modes - for mode in display_modes: - self.available_modes.append(mode) - self.plugin_modes[mode] = plugin_instance - self.mode_to_plugin_id[mode] = plugin_id - logger.debug(" Added mode: %s", mode) - # Invalidate signature cache so the new instance is re-inspected - self._plugin_accepts_display_mode.pop(plugin_id, None) - + # Register the loaded plugin's modes, config subscription + # and dispatch maps (shared with live enable hot-reload). + self._register_loaded_plugin(plugin_id) + # Show progress progress_pct = int((loaded_count / enabled_count) * 100) elapsed = time.time() - plugin_time @@ -447,6 +417,10 @@ def config_change_callback(old_config: Dict[str, Any], new_config: Dict[str, Any # when the user saves settings via the web UI. def _controller_config_change(old_config: Dict[str, Any], new_config: Dict[str, Any]) -> None: self._refresh_config_cache(new_config) + # If a plugin was enabled/disabled, flag a reconcile for the main + # loop to apply (loading/unloading off the watcher thread is unsafe). + if self._enabled_set_changed(old_config, new_config): + self._pending_plugin_reconcile = True self.config_service.subscribe(_controller_config_change) @@ -1582,6 +1556,15 @@ def run(self): logger.info(f"Initial mode set to: {self.current_display_mode} (index: {self.current_mode_index}, total modes: {len(self.available_modes)})") while True: + # Apply plugin enable/disable edits saved via the web UI. The + # config-watcher thread only sets the flag; loading/unloading and + # rebuilding available_modes happens here on the render thread so + # it can't race with rendering. Deferred while on-demand is active + # (the flag stays set) so we don't fight its temporary-enable. + if self._pending_plugin_reconcile and not self.on_demand_active: + self._pending_plugin_reconcile = False + self._reconcile_enabled_plugins() + # Handle on-demand commands before rendering self._poll_on_demand_requests() self._check_on_demand_expiration() @@ -2507,6 +2490,153 @@ def _cleanup_expired_wifi_status(self): self.wifi_status_active = False self.wifi_status_expires_at = None + def _register_loaded_plugin(self, plugin_id: str) -> List[str]: + """Register an already-loaded plugin's display modes, config-change + subscription and dispatch maps with the controller. + + Shared by startup loading and live enable hot-reload so both paths + build identical controller state. Returns the registered modes. + """ + plugin_instance = self.plugin_manager.get_plugin(plugin_id) + manifest = self.plugin_manager.plugin_manifests.get(plugin_id, {}) + + # Prefer the plugin's dynamic modes attribute (e.g. based on enabled + # leagues), else fall back to manifest display_modes, else the id. + if plugin_instance is not None and getattr(plugin_instance, 'modes', None): + display_modes = list(plugin_instance.modes) + logger.debug("Using plugin.modes for %s: %s", plugin_id, display_modes) + else: + display_modes = manifest.get('display_modes', [plugin_id]) + logger.debug("Using manifest display_modes for %s: %s", plugin_id, display_modes) + if not (isinstance(display_modes, list) and display_modes): + display_modes = [plugin_id] + self.plugin_display_modes[plugin_id] = list(display_modes) + + # Subscribe to config changes for per-plugin hot-reload. Bind plugin_id + # and instance as defaults so each plugin's callback targets its own + # instance (avoids late-binding when registering many plugins), and + # remember the callback so we can unsubscribe on disable. + if hasattr(self, 'config_service') and hasattr(plugin_instance, 'on_config_change'): + def config_change_callback(old_config: Dict[str, Any], new_config: Dict[str, Any], + _pid: str = plugin_id, _plugin: Any = plugin_instance) -> None: + """Callback for plugin config changes.""" + try: + _plugin.on_config_change(new_config) + logger.debug("Plugin %s notified of config change", _pid) + except Exception as e: + logger.error("Error in plugin %s config change handler: %s", _pid, e, exc_info=True) + + self.config_service.subscribe(config_change_callback, plugin_id=plugin_id) + self._plugin_config_callbacks[plugin_id] = config_change_callback + logger.debug("Subscribed plugin %s to config changes", plugin_id) + + # Add modes to the dispatch maps. + for mode in display_modes: + if mode not in self.available_modes: + self.available_modes.append(mode) + self.plugin_modes[mode] = plugin_instance + self.mode_to_plugin_id[mode] = plugin_id + logger.debug(" Added mode: %s", mode) + # Invalidate signature cache so the new instance is re-inspected. + self._plugin_accepts_display_mode.pop(plugin_id, None) + return display_modes + + def _unregister_plugin(self, plugin_id: str) -> None: + """Remove a plugin's modes, config subscription and instance, then + unload it. Used by live disable hot-reload.""" + modes = self.plugin_display_modes.pop(plugin_id, []) + for mode in modes: + if mode in self.available_modes: + self.available_modes.remove(mode) + self.plugin_modes.pop(mode, None) + self.mode_to_plugin_id.pop(mode, None) + + # Unsubscribe the plugin's config-change callback. + callback = self._plugin_config_callbacks.pop(plugin_id, None) + if callback is not None and hasattr(self, 'config_service'): + try: + self.config_service.unsubscribe(callback, plugin_id=plugin_id) + except Exception as e: + logger.debug("Error unsubscribing plugin %s from config changes: %s", plugin_id, e) + + self._plugin_accepts_display_mode.pop(plugin_id, None) + + # Tear down the instance (cleanup + on_disable + module unload). + try: + self.plugin_manager.unload_plugin(plugin_id) + except Exception as e: + logger.error("Error unloading plugin %s: %s", plugin_id, e, exc_info=True) + + logger.info("Disabled plugin %s live (removed modes: %s)", plugin_id, modes) + + def _enabled_set_changed(self, old_config: Dict[str, Any], new_config: Dict[str, Any]) -> bool: + """True if any top-level section's ``enabled`` flag differs between two + configs. A cheap watcher-thread check that gates the full reconcile. + Non-plugin sections (e.g. schedule) may match too; the reconcile + no-ops for anything that isn't a discovered plugin.""" + def enabled_map(cfg: Dict[str, Any]) -> Dict[str, bool]: + return { + key: bool(value.get('enabled', False)) + for key, value in cfg.items() + if isinstance(value, dict) + } + return enabled_map(old_config) != enabled_map(new_config) + + def _reconcile_enabled_plugins(self) -> None: + """Load/unload plugins so the running set matches the enabled set in + config. Runs on the main display thread (never the config-watcher + thread) so mutating available_modes is race-free against rendering.""" + if self.plugin_manager is None: + return + try: + config = self.config_service.get_config() + except Exception: + config = self.config + try: + discovered = set(self.plugin_manager.discover_plugins()) + except Exception as e: + logger.error("Plugin reconcile: discovery failed: %s", e, exc_info=True) + return + + desired = {p for p in discovered if config.get(p, {}).get('enabled', False)} + current = set(self.plugin_display_modes.keys()) + to_add = desired - current + to_remove = current - desired + if not to_add and not to_remove: + return + + previous_mode = self.current_display_mode + + for plugin_id in to_remove: + self._unregister_plugin(plugin_id) + + for plugin_id in to_add: + try: + if self.plugin_manager.load_plugin(plugin_id): + modes = self._register_loaded_plugin(plugin_id) + logger.info("Enabled plugin %s live (modes: %s)", plugin_id, modes) + else: + logger.warning("Plugin reconcile: failed to load %s", plugin_id) + except Exception as e: + logger.error("Plugin reconcile: error enabling %s: %s", plugin_id, e, exc_info=True) + + self._resync_mode_index_after_change(previous_mode) + logger.info("Plugin reconcile complete: +%s -%s (%d modes)", + sorted(to_add), sorted(to_remove), len(self.available_modes)) + + def _resync_mode_index_after_change(self, previous_mode: Optional[str]) -> None: + """Clamp rotation state after available_modes changed. Stays on the + previous mode if it survived, otherwise restarts cleanly within range.""" + if not self.available_modes: + self.current_mode_index = 0 + self.current_display_mode = None + return + if previous_mode in self.available_modes: + self.current_mode_index = self.available_modes.index(previous_mode) + else: + self.current_mode_index %= len(self.available_modes) + self.current_display_mode = self.available_modes[self.current_mode_index] + def _refresh_config_cache(self, new_config: Dict[str, Any]) -> None: """Refresh all config-derived caches when a hot-reload fires. diff --git a/test/test_display_controller_plugin_toggle.py b/test/test_display_controller_plugin_toggle.py new file mode 100644 index 00000000..b729a03d --- /dev/null +++ b/test/test_display_controller_plugin_toggle.py @@ -0,0 +1,157 @@ +"""Tests for live plugin enable/disable hot-reload in DisplayController. + +Enabling or disabling a plugin in config used to require a full display +restart because the plugin list and available_modes were built once at init. +These tests cover the reconcile path that loads/unloads plugins and rebuilds +the dispatch maps on the main thread when the enabled set changes. +""" + +from unittest.mock import MagicMock + + +def _make_plugin(modes): + plugin = MagicMock() + plugin.modes = list(modes) + return plugin + + +def _wire_plugin_manager(controller, plugins, discovered=None): + """Point the controller's mock plugin_manager at a set of fake plugins. + + `plugins` maps plugin_id -> mock instance (with a .modes list). + """ + pm = controller.plugin_manager + pm.discover_plugins.return_value = list(discovered if discovered is not None else plugins.keys()) + pm.load_plugin.return_value = True + pm.unload_plugin.return_value = True + pm.plugin_manifests = {} + pm.get_plugin.side_effect = lambda pid: plugins.get(pid) + return pm + + +def _set_config(controller, cfg): + controller.config_service.get_config = lambda: cfg + + +class TestPluginEnableDisableHotReload: + def test_enable_plugin_live(self, test_display_controller): + controller = test_display_controller + assert controller.available_modes == [] + + plugin = _make_plugin(["foo"]) + _wire_plugin_manager(controller, {"foo": plugin}) + _set_config(controller, {"foo": {"enabled": True}}) + + controller._reconcile_enabled_plugins() + + assert "foo" in controller.plugin_display_modes + assert "foo" in controller.available_modes + assert controller.plugin_modes["foo"] is plugin + assert controller.mode_to_plugin_id["foo"] == "foo" + controller.plugin_manager.load_plugin.assert_any_call("foo") + + def test_disable_plugin_live(self, test_display_controller): + controller = test_display_controller + plugin = _make_plugin(["live", "recent"]) + _wire_plugin_manager(controller, {"sports": plugin}, discovered=["sports"]) + + # Enable, then disable. + _set_config(controller, {"sports": {"enabled": True}}) + controller._reconcile_enabled_plugins() + assert "sports" in controller.plugin_display_modes + assert "live" in controller.available_modes and "recent" in controller.available_modes + assert "sports" in controller._plugin_config_callbacks + + _set_config(controller, {"sports": {"enabled": False}}) + controller._reconcile_enabled_plugins() + + assert "sports" not in controller.plugin_display_modes + assert "live" not in controller.available_modes + assert "recent" not in controller.available_modes + assert "live" not in controller.plugin_modes + assert "recent" not in controller.mode_to_plugin_id + controller.plugin_manager.unload_plugin.assert_any_call("sports") + assert "sports" not in controller._plugin_config_callbacks + + def test_disable_clamps_current_mode_index(self, test_display_controller): + controller = test_display_controller + p1 = _make_plugin(["a"]) + p2 = _make_plugin(["b"]) + _wire_plugin_manager(controller, {"p1": p1, "p2": p2}, discovered=["p1", "p2"]) + + _set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": True}}) + controller._reconcile_enabled_plugins() + # Add order across multiple plugins is set-driven (as at init), so + # compare membership, not order. + assert set(controller.available_modes) == {"a", "b"} + + # Pretend we're currently showing p2's mode. + controller.current_mode_index = controller.available_modes.index("b") + controller.current_display_mode = "b" + + _set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": False}}) + controller._reconcile_enabled_plugins() + + assert controller.available_modes == ["a"] + # Index must be back in range and the display mode no longer the removed one. + assert 0 <= controller.current_mode_index < len(controller.available_modes) + assert controller.current_display_mode == "a" + + def test_enable_keeps_current_mode(self, test_display_controller): + controller = test_display_controller + p1 = _make_plugin(["a"]) + p2 = _make_plugin(["b"]) + _wire_plugin_manager(controller, {"p1": p1, "p2": p2}, discovered=["p1", "p2"]) + + _set_config(controller, {"p1": {"enabled": True}}) + controller._reconcile_enabled_plugins() + controller.current_mode_index = 0 + controller.current_display_mode = "a" + + # Enabling p2 should not disturb the currently-showing mode. + _set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": True}}) + controller._reconcile_enabled_plugins() + + assert "b" in controller.available_modes + assert controller.current_display_mode == "a" + assert controller.available_modes[controller.current_mode_index] == "a" + + def test_noop_when_enabled_set_unchanged(self, test_display_controller): + controller = test_display_controller + plugin = _make_plugin(["foo"]) + _wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"]) + _set_config(controller, {"foo": {"enabled": True}}) + controller._reconcile_enabled_plugins() + + load_calls = controller.plugin_manager.load_plugin.call_count + unload_calls = controller.plugin_manager.unload_plugin.call_count + + # Reconcile again with no change — must not load/unload anything. + controller._reconcile_enabled_plugins() + assert controller.plugin_manager.load_plugin.call_count == load_calls + assert controller.plugin_manager.unload_plugin.call_count == unload_calls + + +class TestEnabledSetChanged: + def test_detects_toggle(self, test_display_controller): + c = test_display_controller + assert c._enabled_set_changed({"a": {"enabled": True}}, {"a": {"enabled": False}}) is True + + def test_no_change(self, test_display_controller): + c = test_display_controller + cfg = {"a": {"enabled": True}, "b": {"enabled": False}} + assert c._enabled_set_changed(cfg, dict(cfg)) is False + + def test_new_enabled_section(self, test_display_controller): + c = test_display_controller + assert c._enabled_set_changed( + {"a": {"enabled": True}}, + {"a": {"enabled": True}, "b": {"enabled": True}}, + ) is True + + def test_ignores_non_enabled_value_edits(self, test_display_controller): + c = test_display_controller + assert c._enabled_set_changed( + {"a": {"enabled": True, "duration": 30}}, + {"a": {"enabled": True, "duration": 45}}, + ) is False