From b950871ca871281abbd8b5fc1f7a4b7cbc296e97 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:45:21 -0400 Subject: [PATCH] refactor(sports): one reader for element colours and layout offsets There were two copies of the per-element colour read and three of the layout-offset read. They had already drifted -- the scroll-card renderer carries a comment about having ignored offsets its own schema advertised -- and each new capability had to be added to all of them or silently work in some places and not others. All of them now go through src.element_style, which is what carries the alias handling and the per-mode lookup. That lands immediately for the nine plugins importing these modules: a scoreboard asking for `score_text` offsets finds the `layout.score` its users configured, and a Live instance resolves its own colours through SKIN_MODE without any call site passing a mode. _normalize_color learned "#RRGGBB" in the process. The scoreboards' own readers have always accepted it, so the shared one had to, or consolidating would have quietly dropped a form users' configs may hold. _coerce_offset picked up the non-finite guard the scroll-card reader had and the other two did not. _get_layout_offset is promoted onto SportsCoreSharedMixin. Each plugin still carries its own copy in its bundled sports.py, which wins by MRO -- so adopting this is a deletion in the plugin, and until that deletion nothing changes for it. Note for whoever runs the suite next: test_display_dirty_tracking.py is order-dependent. Fifteen of its tests failed in one full run and passed in the next with no change in between, and pass in isolation. Pre-existing, unrelated to this, but it makes a full-run diff untrustworthy until it is fixed. Co-Authored-By: Claude Opus 5 --- src/common/sports_card.py | 24 ++--- src/common/sports_game_renderer.py | 16 +-- src/common/sports_shared.py | 35 +++++-- src/element_style.py | 160 ++++++++++++++++++++--------- test/test_sports_card.py | 50 +++++++++ test/test_sports_game_renderer.py | 45 ++++++++ test/test_sports_shared.py | 40 ++++++++ 7 files changed, 287 insertions(+), 83 deletions(-) diff --git a/src/common/sports_card.py b/src/common/sports_card.py index c8e55082..beda5c9c 100644 --- a/src/common/sports_card.py +++ b/src/common/sports_card.py @@ -99,18 +99,18 @@ def upcoming_center_mode(config: Optional[Dict[str, Any]]) -> str: # --------------------------------------------------------------------------- def element_color(config: Optional[Dict[str, Any]], element: str, - default: Tuple[int, int, int] = (255, 255, 255)): - """Per-element text colour from customization..text_color.""" - try: - cfg = (config or {}).get("customization", {}).get(element, {}) - value = cfg.get("text_color") - if isinstance(value, (list, tuple)) and len(value) == 3: - return tuple(max(0, min(255, int(c))) for c in value) - if isinstance(value, str) and value.startswith("#") and len(value) == 7: - return tuple(int(value[i:i + 2], 16) for i in (1, 3, 5)) - except (TypeError, ValueError): - pass - return default + default: Tuple[int, int, int] = (255, 255, 255), + mode: Optional[str] = None): + """Per-element text colour from customization..text_color. + + Delegated rather than reimplemented: there were two copies of this + read and three of the offset read, and the shared one also resolves + the element under the names plugins actually use (the layout block + says `score` where the style block says `score_text`) and honours a + per-mode override. Hex strings are still accepted. + """ + from src.element_style import element_color as _shared + return _shared(config, element, default, mode) def font_color(config: Optional[Dict[str, Any]], fonts: Optional[Dict[str, Any]], diff --git a/src/common/sports_game_renderer.py b/src/common/sports_game_renderer.py index d42faf3f..c890e45c 100644 --- a/src/common/sports_game_renderer.py +++ b/src/common/sports_game_renderer.py @@ -134,19 +134,9 @@ class SportsGameRendererMixin: the element on the scroll/Vegas card too -- previously the schema advertised these offsets but this renderer ignored them. """ - try: - layout = (self.config or {}).get("customization", {}).get("layout", {}) - value = (layout.get(element) or {}).get(axis, default) - if isinstance(value, bool): - return default - if isinstance(value, (int, float)): - return int(value) if math.isfinite(value) else default - if isinstance(value, str): - parsed = float(value) - return int(parsed) if math.isfinite(parsed) else default - except (TypeError, ValueError, OverflowError): - pass - return default + from src.element_style import layout_offset + return layout_offset(self.config, element, axis, default, + getattr(self, "SKIN_MODE", None)) # ---- upcoming cards ------------------------------------------------ diff --git a/src/common/sports_shared.py b/src/common/sports_shared.py index 3d38196e..a988eaef 100644 --- a/src/common/sports_shared.py +++ b/src/common/sports_shared.py @@ -757,18 +757,31 @@ class SportsCoreSharedMixin: self.logger.debug("Headline font scaling skipped", exc_info=True) return fonts + def _get_layout_offset(self, element: str, axis: str, + default: int = 0) -> int: + """X/Y nudge for one element, from ``customization.layout``. + + Promoted here so every scoreboard reads offsets the same way the + scroll card does. Each plugin still carries its own copy in its + bundled sports.py, which wins by MRO until that copy is deleted -- + deleting it is what buys the alias handling (a plugin asking for + ``score_text`` finds the ``score`` its users configured) and the + per-mode overrides, since this resolves through SKIN_MODE. + """ + from src.element_style import layout_offset + return layout_offset(self.config, element, axis, default, + getattr(self, "SKIN_MODE", None)) + def _element_color(self, element: str, default: Tuple[int, int, int] = (255, 255, 255)): - """Per-element text colour from customization..text_color.""" - try: - cfg = (self.config or {}).get("customization", {}).get(element, {}) - value = cfg.get("text_color") - if isinstance(value, (list, tuple)) and len(value) == 3: - return tuple(max(0, min(255, int(c))) for c in value) - if isinstance(value, str) and value.startswith("#") and len(value) == 7: - return tuple(int(value[i:i + 2], 16) for i in (1, 3, 5)) - except (TypeError, ValueError): - pass - return default + """Per-element text colour from customization..text_color. + + Mode-aware through SKIN_MODE, so Live and Recent instances of the + same scoreboard resolve their own colours without any call site + passing a mode. + """ + from src.element_style import element_color as _shared + return _shared(self.config, element, default, + getattr(self, "SKIN_MODE", None)) def _unshare_element_fonts(self, fonts): """Give each colourable element its own face object. diff --git a/src/element_style.py b/src/element_style.py index 122855b8..d4893362 100644 --- a/src/element_style.py +++ b/src/element_style.py @@ -46,6 +46,7 @@ root derived from this module's own location). import copy import json import logging +import math import os from collections import OrderedDict from dataclasses import dataclass @@ -886,7 +887,21 @@ def defaults_from_schema_file(schema_path: Union[str, os.PathLike]) -> Dict[str, # --------------------------------------------------------------------------- def _normalize_color(value: Any) -> Optional[Tuple[int, int, int]]: - """An (r, g, b) tuple of ints in 0..255, or None for anything else.""" + """An (r, g, b) tuple of ints in 0..255, or None for anything else. + + ``"#RRGGBB"`` is accepted as well as ``[r, g, b]``: the scoreboards' + own colour readers have always taken both, and this is the function + they now share. + """ + if isinstance(value, str): + text = value.strip() + if len(text) == 7 and text.startswith('#'): + try: + return (int(text[1:3], 16), int(text[3:5], 16), + int(text[5:7], 16)) + except ValueError: + return None + return None if isinstance(value, (list, tuple)) and len(value) == 3: try: rgb = tuple(int(c) for c in value) @@ -1005,6 +1020,101 @@ def _coerce_scale(value: Any, default: float) -> float: return min(scale, 10.0) +def _coerce_offset(value: Any, default: int, element_key: str, + axis: str) -> int: + """A pixel offset as an int; anything nonsensical is ``default``. + + A bool degrades rather than counting as 1/0 -- the more correct reading + of a pixel offset, and what the shared resolver has always done + relative to the classic inline read. Non-finite floats degrade too: + the scroll-card reader guarded against those explicitly and this is now + the one implementation. + """ + if isinstance(value, bool): + return int(default) + if isinstance(value, (int, float)): + if isinstance(value, float) and not math.isfinite(value): + return int(default) + return int(value) + if isinstance(value, str): + try: + parsed = float(value) + except (TypeError, ValueError): + logger.warning("Invalid layout offset for %s.%s: %r, using %s", + element_key, axis, value, default) + return int(default) + if not math.isfinite(parsed): + return int(default) + return int(parsed) + return int(default) + + +def layout_offset(config: Any, element_key: str, axis: str, + default: int = 0, mode: Optional[str] = None) -> int: + """One ``customization.layout..`` value, as an int. + + The stateless form of :meth:`ElementStyleResolver.offset_value`, for the + scoreboard helpers that are handed a config rather than holding one. + Both go through here, so the alias handling and the per-mode lookup + cannot drift between them -- there were three separate readers of this + block before, and the scroll-card one had already been found ignoring + offsets the schema advertised. + """ + try: + block = config.get('customization') if isinstance(config, dict) else None + block = block if isinstance(block, dict) else {} + base = _lookup_element(block.get('layout'), element_key).get(axis) + base_value = (int(default) if base is None + else _coerce_offset(base, default, element_key, axis)) + + if mode: + modes = block.get('modes') + mode_block = modes.get(mode) if isinstance(modes, dict) else None + if isinstance(mode_block, dict): + override = _lookup_element(mode_block.get('layout'), + element_key).get(axis) + if override is not None: + return _coerce_offset(override, base_value, + element_key, axis) + return base_value + except Exception as e: + logger.warning("Error reading layout offset %s.%s: %s", + element_key, axis, e) + try: + return int(default) + except (TypeError, ValueError): + return 0 + + +def element_color(config: Any, element_key: str, + default: Tuple[int, int, int] = (255, 255, 255), + mode: Optional[str] = None) -> Tuple[int, int, int]: + """``customization..text_color``, or ``default``. + + The stateless colour lookup the scoreboards share. Unlike + :meth:`ElementStyleResolver.style` this does not compare against a + schema default -- the callers have no schema to hand -- so any + configured colour counts, which is what their own readers did. + """ + try: + block = config.get('customization') if isinstance(config, dict) else None + block = block if isinstance(block, dict) else {} + if mode: + modes = block.get('modes') + mode_block = modes.get(mode) if isinstance(modes, dict) else None + if isinstance(mode_block, dict): + override = _normalize_color( + _lookup_element(mode_block, element_key).get('text_color')) + if override is not None: + return override + value = _normalize_color( + _lookup_element(block, element_key).get('text_color')) + return value if value is not None else default + except Exception as e: + logger.warning("Error reading colour for %s: %s", element_key, e) + return default + + class ElementStyleResolver: """Resolves per-element user styling against schema defaults. @@ -1160,31 +1270,8 @@ class ElementStyleResolver: axis — ``None`` there means inherit the base offset, which is what lets a mode nudge one element without restating the rest. """ - try: - # Base first, so it is the thing a malformed mode value falls - # back to. Resolving the mode first and passing the caller's - # default would let one bad string in a mode block silently - # discard a perfectly good base offset. - base = self._layout_axis(self._customization(), element_key, axis) - base_value = (int(default) if base is None - else self._coerce_offset(base, default, - element_key, axis)) - - effective_mode = self._effective_mode(mode) - if effective_mode: - override = self._layout_axis( - self._mode_block(effective_mode), element_key, axis) - if override is not None: - return self._coerce_offset(override, base_value, - element_key, axis) - return base_value - except Exception as e: - logger.warning("Error reading layout offset %s.%s: %s", - element_key, axis, e) - try: - return int(default) - except (TypeError, ValueError): - return 0 + return layout_offset(self._config, element_key, axis, default, + self._effective_mode(mode)) @staticmethod def _layout_element(block: Dict[str, Any], @@ -1205,27 +1292,6 @@ class ElementStyleResolver: """``block['layout'][element][axis]``, or None if absent anywhere.""" return cls._layout_element(block, element_key).get(axis) - @staticmethod - def _coerce_offset(value: Any, default: int, element_key: str, - axis: str) -> int: - """A pixel offset as an int; anything nonsensical is ``default``. - - A bool degrades rather than counting as 1/0 -- it is the more - correct reading of a pixel offset, and matches what the shared - resolver has always done relative to the classic inline read. - """ - if isinstance(value, bool): - return int(default) - if isinstance(value, (int, float)): - return int(value) - if isinstance(value, str): - try: - return int(float(value)) - except (TypeError, ValueError): - logger.warning("Invalid layout offset for %s.%s: %r, using %s", - element_key, axis, value, default) - return int(default) - return int(default) # -- resolution internals ----------------------------------------------- diff --git a/test/test_sports_card.py b/test/test_sports_card.py index 9a4da303..6c0d854d 100644 --- a/test/test_sports_card.py +++ b/test/test_sports_card.py @@ -262,3 +262,53 @@ class TestUnshareElementFonts: fonts = {"score": a, "time": b} C.unshare_element_fonts(log, fonts) assert fonts["score"] is a and fonts["time"] is b + +class TestSharedElementReaders: + """The scoreboards' colour and offset reads now go through one + implementation. + + There were two copies of the colour read and three of the offset read. + They had already drifted: the scroll-card renderer carries a comment + noting it used to ignore offsets its own schema advertised. Sharing one + reader is what lets a fix (or an alias, or a per-mode override) reach + the full-screen scorebug and the scroll card together. + """ + + CONFIG = { + "customization": { + "score_text": {"text_color": [255, 200, 0]}, + "status": {"text_color": "#00ff00"}, + "layout": {"score": {"y_offset": -3}}, + "modes": { + "live": {"score_text": {"text_color": [255, 0, 0]}, + "layout": {"score": {"y_offset": 7}}}, + }, + }, + } + + def test_colour_reads_the_configured_value(self, log): + assert C.element_color(self.CONFIG, "score_text") == (255, 200, 0) + + def test_hex_colours_are_still_accepted(self, log): + """These readers have always taken '#RRGGBB' as well as [r, g, b], + so the shared one had to learn it rather than the callers losing it.""" + assert C.element_color(self.CONFIG, "status") == (0, 255, 0) + + def test_colour_resolves_through_an_alias(self, log): + """The style block says status_text where this config says status.""" + assert C.element_color(self.CONFIG, "status_text") == (0, 255, 0) + + def test_a_mode_overrides_the_colour(self, log): + assert C.element_color(self.CONFIG, "score_text", + mode="live") == (255, 0, 0) + + def test_an_unset_element_gets_the_default(self, log): + assert C.element_color(self.CONFIG, "nothing", (1, 2, 3)) == (1, 2, 3) + + @pytest.mark.parametrize("config", [ + None, {}, "nonsense", {"customization": "nonsense"}, + {"customization": {"score_text": "nonsense"}}, + {"customization": {"score_text": {"text_color": "not a colour"}}}, + ]) + def test_a_hostile_config_gives_the_default(self, config, log): + assert C.element_color(config, "score_text", (9, 9, 9)) == (9, 9, 9) diff --git a/test/test_sports_game_renderer.py b/test/test_sports_game_renderer.py index 50590005..14046d2c 100644 --- a/test/test_sports_game_renderer.py +++ b/test/test_sports_game_renderer.py @@ -256,3 +256,48 @@ class TestContract: h._draw_upcoming_center(_draw(), {}) h._draw_upcoming_game_status(_draw(), {}) h.set_rankings_cache({}) + + +class TestLayoutOffsetsAreShared: + """The scroll/Vegas card reads offsets through the same function the + full-screen scorebug does. + + This renderer already carries a comment about having ignored offsets its + own schema advertised -- it was the third copy of this read. Sharing one + means the alias handling and per-mode overrides land here too, without + this file knowing about either. + """ + + CONFIG = {"customization": { + "layout": {"score": {"y_offset": -3}}, + "modes": {"live": {"layout": {"score": {"y_offset": 7}}}}, + }} + + def _host(self, config, mode=None): + attrs = {"config": config} + if mode: + attrs["SKIN_MODE"] = mode + return type("Card", (SportsGameRendererMixin,), attrs)() + + def test_it_reads_the_configured_offset(self): + assert self._host(self.CONFIG)._layout_offset("score", "y_offset") == -3 + + def test_it_resolves_through_an_alias(self): + """A caller asking for score_text finds the layout.score the user + configured -- the two namespaces disagree by convention.""" + assert self._host(self.CONFIG)._layout_offset( + "score_text", "y_offset") == -3 + + def test_skin_mode_selects_the_per_mode_offset(self): + assert self._host(self.CONFIG, "live")._layout_offset( + "score", "y_offset") == 7 + + def test_a_host_without_a_skin_mode_gets_the_base(self): + assert self._host(self.CONFIG)._layout_offset("score", "y_offset") == -3 + + @pytest.mark.parametrize("written", [ + float("inf"), float("nan"), True, "bad", None, [], {}, + ]) + def test_nonsense_degrades_to_the_default(self, written): + config = {"customization": {"layout": {"score": {"y_offset": written}}}} + assert self._host(config)._layout_offset("score", "y_offset", 4) == 4 diff --git a/test/test_sports_shared.py b/test/test_sports_shared.py index 1c36b5b7..5ef97d40 100644 --- a/test/test_sports_shared.py +++ b/test/test_sports_shared.py @@ -422,3 +422,43 @@ class TestUnshareKeepsTheLayoutEngine: fonts = {"score": shared, "time": shared} self._host()._unshare_element_fonts(fonts) assert fonts["time"].getlength("88-88") == shared.getlength("88-88") + + +class TestPromotedLayoutOffset: + """_get_layout_offset moved here so every scoreboard reads offsets the + way the scroll card does. + + Each plugin still carries its own copy in its bundled sports.py, which + wins by MRO. That is the migration property: adopting this is a + deletion in the plugin, and until that deletion nothing changes. + """ + + CONFIG = {"customization": { + "layout": {"score": {"y_offset": -3}}, + "modes": {"recent": {"layout": {"score": {"y_offset": 9}}}}, + }} + + def _host(self, **attrs): + return type("H", (SportsCoreSharedMixin,), + dict({"config": self.CONFIG}, **attrs))() + + def test_it_reads_the_configured_offset(self): + assert self._host()._get_layout_offset("score", "y_offset") == -3 + + def test_it_resolves_through_an_alias(self): + """What a plugin gains by deleting its own copy: the style block + says score_text where the layout block says score.""" + assert self._host()._get_layout_offset("score_text", "y_offset") == -3 + + def test_skin_mode_selects_the_per_mode_offset(self): + assert self._host(SKIN_MODE="recent")._get_layout_offset( + "score", "y_offset") == 9 + + def test_a_plugins_own_copy_still_wins(self): + """Until a plugin deletes its copy, this changes nothing for it.""" + host = self._host( + _get_layout_offset=lambda self, element, axis, default=0: 99) + assert host._get_layout_offset("score", "y_offset") == 99 + + def test_an_unset_offset_is_the_default(self): + assert self._host()._get_layout_offset("nothing", "y_offset", 5) == 5