fix: a Vegas static pause survives a non-numeric display duration; aliased store installs ask for a restart

CHANGELOG.md

@@ -885,6 +885,25 @@ policies are unchanged.
   a runtime publisher that stops still goes `stale`, and a subscription that
   goes quiet still falls back to the cache. The cache path's 120 s rule is
   unchanged.
+- A plugin that pauses the Vegas scroll gets its pause when its display
+  duration is not a plain number. Several plugins (clock-simple, calendar,
+  countdown) return `display_duration` as it is in config.json, so a value
+  saved as `"20"` or `null` (the raw config editor, a hand edit) reached the
+  pause as a string or None; comparing it with the clock raised, and the
+  plugin flashed up and the scroll went straight on, at every one of its
+  turns. `inf` held the pause until something interrupted it, and 0, a
+  negative number or NaN ended it at once. The pause now reads the duration
+  as the rotation does (`finite_seconds()` in `base_plugin`): a numeric
+  string counts, anything else that is not a finite number (or a
+  `get_display_duration()` that raises) pauses for 30 s, and a number at or
+  below zero for 15 s, with one warning per plugin.
+- Reinstalling Weather, Music, Stocks or Leaderboard from the Plugin Store
+  while it is enabled asks for a display restart, as reinstalling any other
+  enabled plugin does. `POST /api/v3/plugins/install` looked for the
+  plugin's `enabled` flag under the store id (`weather`), but its config
+  section is under the id its manifest declares (`ledmatrix-weather`), so
+  `restart_required` was always false and the display kept running the
+  copy it had loaded. The check now uses the installed id.
 
 ### Scrolling
 

src/display_controller.py

@@ -25,7 +25,6 @@
 import inspect
 import signal
 import json
-import math
 import threading
 import types
 from collections import deque
@@ -57,6 +56,7 @@
     PluginReloadResult,
 )
 from src.ipc.server import ControlServer, QueuedCommand, StateHub, start_control_server
+from src.plugin_system.base_plugin import finite_seconds
 from src.vegas_mode.render_pipeline import SYNC_SEND_INTERVAL
 
 # Get logger with consistent configuration
@@ -90,19 +90,6 @@
 DEFAULT_DYNAMIC_DURATION_CAP = 180.0
 
 
-def _finite_seconds(value: Any) -> Optional[float]:
-    """``value`` as seconds when it is a finite number or a numeric string,
-    else None. A bool is not a number here, though it is an int: True would
-    read as a one-second screen."""
-    if isinstance(value, bool):
-        return None
-    try:
-        seconds = float(value)
-    except (TypeError, ValueError, OverflowError):
-        return None
-    return seconds if math.isfinite(seconds) else None
-
-
 class _PluginReloadJob:
     """A ``plugin.reload`` whose slow half runs off the render thread.
 
@@ -1392,7 +1379,7 @@ def _get_display_duration(self, mode_key):
         except Exception as err:  # pylint: disable=broad-except
             problem = f"get_display_duration() raised {type(err).__name__}: {err}"
         else:
-            seconds = _finite_seconds(value)
+            seconds = finite_seconds(value)
             if seconds is not None:
                 return seconds
             problem = f"display duration {value!r} is not a number"

src/plugin_system/base_plugin.py

@@ -11,6 +11,7 @@
 from abc import ABC, abstractmethod
 from enum import Enum
 from typing import Dict, Any, Optional, List
+import math
 import os
 import sys
 from src.deprecation import deprecated, warn_deprecated
@@ -240,6 +241,26 @@ def resolve_vegas_participation(plugin: Any, plugin_id: Optional[str] = None) ->
     return legacy_vegas_participation(plugin)
 
 
+def finite_seconds(value: Any) -> Optional[float]:
+    """``value`` as seconds when it is a finite number or a numeric string,
+    else None. A bool is not a number here, though it is an int: True would
+    read as a one-second screen.
+
+    How the core reads a plugin's get_display_duration() -- the rotation
+    (DisplayController._get_display_duration) and the Vegas static pause --
+    which several plugins answer straight from config.json, so a value saved
+    as "20" or null arrives as a string or None. A number at or below zero is
+    returned as it is; each caller has its own rule for that.
+    """
+    if isinstance(value, bool):
+        return None
+    try:
+        seconds = float(value)
+    except (TypeError, ValueError, OverflowError):
+        return None
+    return seconds if math.isfinite(seconds) else None
+
+
 class BasePlugin(ABC):
     """
     Base class that all plugins must inherit from.

src/vegas_mode/coordinator.py

@@ -18,10 +18,11 @@
 import sys
 import time
 import threading
-from typing import Optional, Dict, Any, List, Callable, TYPE_CHECKING
+from typing import Optional, Dict, Any, FrozenSet, List, Callable, TYPE_CHECKING
 
 from src import display_watchdog
 from src.common import render_gate
+from src.plugin_system.base_plugin import finite_seconds
 from src.vegas_mode.config import VegasModeConfig
 from src.vegas_mode.elements import LiveEpochs
 from src.vegas_mode.plugin_adapter import PluginAdapter
@@ -53,6 +54,14 @@
 #: every plugin. Game state doesn't change within a quarter second.
 _LIVE_PRIORITY_CHECK_INTERVAL = 0.25
 
+#: Seconds a static pause shows a plugin whose display duration can't be
+#: used, as long as the rotation shows it: 30 when get_display_duration()
+#: raises or answers something that is not a number
+#: (DisplayController._get_display_duration), 15 when it answers a number at
+#: or below zero (DisplayController._resolve_durations).
+_UNREADABLE_DURATION = 30.0
+_NOT_POSITIVE_DURATION = 15.0
+
 
 def _percentile(ordered: List[float], fraction: float) -> float:
     """Nearest-rank percentile of an already-sorted list.
@@ -92,6 +101,9 @@ class VegasModeCoordinator:
     _live_reason: Optional[str] = None
     # Set only while Vegas has changed the GIL switch interval; read with getattr.
     _saved_switch_interval: Optional[float]
+    #: Plugins already warned about a display duration the pause can't use,
+    #: so a bad setting logs once, not at every turn. Replaced, not mutated.
+    _duration_warned: FrozenSet[str] = frozenset()
 
     def __init__(
         self,
@@ -1010,7 +1022,7 @@ def _handle_static_pause(self, plugin: 'BasePlugin') -> bool:
             # Wait for the plugin's display duration. Monotonic, like the
             # iteration clock: an NTP step on an RTC-less Pi would otherwise
             # end the pause at once or stretch it by the correction.
-            duration = plugin.get_display_duration()
+            duration = self._static_pause_duration(plugin)
             start = time.monotonic()
 
             while time.monotonic() - start < duration:
@@ -1046,6 +1058,42 @@ def _handle_static_pause(self, plugin: 'BasePlugin') -> bool:
 
         return True
 
+    def _static_pause_duration(self, plugin: 'BasePlugin') -> float:
+        """Seconds a static pause shows ``plugin``: its display duration,
+        read the way the rotation reads it.
+
+        Several plugins return their display_duration setting straight from
+        config.json, so one saved as "20" or null came back as a string or
+        None; comparing it with the clock raised, and the pause's broad
+        except ended the pause at every one of the plugin's turns. inf
+        paused until something interrupted it, and NaN, False, 0 or a
+        negative number ended the pause at once. A numeric string counts
+        (finite_seconds); anything else, or a raise, gets
+        _UNREADABLE_DURATION, and a number at or below zero
+        _NOT_POSITIVE_DURATION, logged once per plugin.
+        """
+        try:
+            value = plugin.get_display_duration()
+        except Exception as err:  # pylint: disable=broad-except
+            problem = f"get_display_duration() raised {type(err).__name__}: {err}"
+            seconds = _UNREADABLE_DURATION
+        else:
+            seconds = finite_seconds(value)
+            if seconds is not None and seconds > 0:
+                return seconds
+            if seconds is None:
+                problem = f"display duration {value!r} is not a number"
+                seconds = _UNREADABLE_DURATION
+            else:
+                problem = f"display duration {value!r} is not above zero"
+                seconds = _NOT_POSITIVE_DURATION
+        plugin_id = plugin.plugin_id
+        if plugin_id not in self._duration_warned:
+            self._duration_warned = self._duration_warned | {plugin_id}
+            logger.warning("[%s] %s; its static pause lasts %.0fs (logged once)",
+                           plugin_id, problem, seconds)
+        return seconds
+
     def _end_static_pause(self) -> None:
         """End static pause and restore scroll state."""
         should_resume_scrolling = False

test/test_api_v3_install_restart_installed_id.py

@@ -0,0 +1,123 @@
+"""POST /plugins/install asks for a restart by the id the plugin installed as.
+
+A store install needs a display restart when config.json already enables the
+plugin (a reinstall, or a config carried over): the display loads a plugin
+when its ``enabled`` flag changes, and this flag did not. The route read the
+flag under the registry id it was given. An aliased entry installs under
+another id -- ``weather`` installs a directory whose manifest declares
+``ledmatrix-weather``, and its config section is ``ledmatrix-weather`` -- so
+reinstalling an enabled Weather never reported that a restart was needed,
+and the display kept running the old copy.
+"""
+
+import json
+from unittest.mock import MagicMock
+
+import pytest
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401
+
+INSTALL = "/api/v3/plugins/install"
+
+
+@pytest.fixture
+def store(api_v3_module, tmp_path):
+    """The store installs registry entry ``weather`` as ``installed_id``."""
+    manager = api_v3_module.api_v3.plugin_store_manager
+    manager.install_plugin.return_value = True
+    manager.get_registry_info.return_value = None
+    manager._find_plugin_path.return_value = None
+
+    def installs_as(installed_id):
+        path = tmp_path / installed_id
+        path.mkdir()
+        (path / "manifest.json").write_text(json.dumps({"id": installed_id}),
+                                            encoding="utf-8")
+        manager._find_plugin_path.side_effect = (
+            lambda pid: path if pid == "weather" else None)
+
+    manager.installs_as = installs_as
+    return manager
+
+
+@pytest.fixture
+def config(api_v3_module):
+    """config.json with an ``enabled`` flag for each plugin id given."""
+    def sections(enabled):
+        api_v3_module.api_v3.config_manager.load_config.return_value = {
+            plugin_id: {"enabled": flag} for plugin_id, flag in enabled.items()}
+    return sections
+
+
+@pytest.fixture
+def queued(api_v3_module):
+    queue = MagicMock()
+
+    def enqueue(operation_type, plugin_id, operation_callback=None):
+        queue.callback_result = operation_callback(MagicMock())
+        return "op-1"
+
+    queue.enqueue_operation.side_effect = enqueue
+    api_v3_module.api_v3.operation_queue = queue
+    return queue
+
+
+def _direct(client):
+    return client.post(INSTALL, json={"plugin_id": "weather"}).get_json()
+
+
+def _queued(client, queue):
+    client.post(INSTALL, json={"plugin_id": "weather"})
+    return queue.callback_result
+
+
+class TestDirectInstall:
+    def test_an_aliased_install_enabled_under_its_installed_id_asks_for_a_restart(
+            self, api_v3_client, store, config):
+        store.installs_as("ledmatrix-weather")
+        config({"ledmatrix-weather": True})
+        body = _direct(api_v3_client)
+        assert body["status"] == "success"
+        assert body["restart_required"] is True
+        assert body["restart_message"]
+
+    def test_an_enabled_section_under_the_registry_id_alone_does_not(
+            self, api_v3_client, store, config):
+        """The display knows the plugin as ledmatrix-weather; nothing runs
+        under a section called weather."""
+        store.installs_as("ledmatrix-weather")
+        config({"weather": True})
+        assert _direct(api_v3_client)["restart_required"] is False
+
+    def test_an_aliased_install_that_is_not_enabled_needs_no_restart(
+            self, api_v3_client, store, config):
+        store.installs_as("ledmatrix-weather")
+        config({"ledmatrix-weather": False})
+        assert _direct(api_v3_client)["restart_required"] is False
+
+    def test_an_install_under_its_own_id_is_unchanged(self, api_v3_client, store, config):
+        store.installs_as("weather")
+        config({"weather": True})
+        assert _direct(api_v3_client)["restart_required"] is True
+
+    def test_an_install_that_cannot_be_found_uses_the_requested_id(
+            self, api_v3_client, store, config):
+        config({"weather": True})
+        assert _direct(api_v3_client)["restart_required"] is True
+
+
+class TestQueuedInstall:
+    def test_an_aliased_install_enabled_under_its_installed_id_asks_for_a_restart(
+            self, api_v3_client, store, config, queued):
+        store.installs_as("ledmatrix-weather")
+        config({"ledmatrix-weather": True})
+        result = _queued(api_v3_client, queued)
+        assert result["success"] is True
+        assert result["restart_required"] is True
+        assert result["restart_message"]
+
+    def test_an_enabled_section_under_the_registry_id_alone_does_not(
+            self, api_v3_client, store, config, queued):
+        store.installs_as("ledmatrix-weather")
+        config({"weather": True})
+        assert _queued(api_v3_client, queued)["restart_required"] is False

test/test_vegas_static_mode.py

@@ -219,7 +219,8 @@ def _coord(self, plugin, lock=None):
     def _plugin(self):
         plugin = MagicMock()
         plugin.plugin_id = 'clock'
-        plugin.get_display_duration.return_value = 0
+        # A moment: zero would pause 15 s, as the rotation shows it.
+        plugin.get_display_duration.return_value = 0.01
         return plugin
 
     def test_trigger_comes_from_the_pipeline(self):