From 3ade3a71c116fb63cafe7f76df976f2343f3071d Mon Sep 17 00:00:00 2001 From: pax Date: Wed, 8 Apr 2026 19:40:05 -0500 Subject: [PATCH] popout/state: implement illegal transition handler (env-gated) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the structural alternative to "wait for a downstream symptom and bisect to find the bad dispatch": catch illegal transitions at the dispatch boundary instead of letting them silently no-op. In release mode (default — no env var set): - Illegal events are dropped silently - A `log.debug` line is emitted with the state and event type - dispatch returns [] - state is unchanged - This is what production runs In strict mode (BOORU_VIEWER_STRICT_STATE=1): - Illegal events raise InvalidTransition(state, event) - The exception carries both fields for the diagnostic - This is for development and the test suite — it makes programmer errors loud and immediate instead of silently cascading into a downstream symptom The legality map (`_LEGAL_EVENTS_BY_STATE`) is per-state. Most events (NavigateRequested / Mute / Volume / LoopMode / Fullscreen / window events / Close / ContentArrived) are globally legal in any non-Closing state. State-specific events are listed per state. Closing has an empty legal set; the dispatch entry already drops everything from Closing before the legality check runs. The map distinguishes "legal-but-no-op" from "structurally invalid": - VideoEofReached in LoadingVideo: LEGAL. The state machine intentionally accepts and drops this event. It's the EOF race fix — the event arriving in LoadingVideo is the race scenario, and dropping is the structural cure. Strict mode does NOT raise. - VideoEofReached in SeekingVideo: LEGAL. Same reasoning — eof during a seek is stale. - VideoEofReached in AwaitingContent / DisplayingImage: ILLEGAL. No video is loaded; an eof event arriving here is a real bug in either mpv or the adapter. Strict mode raises. The strict-mode read happens per-dispatch (`os.environ.get`), not cached at module load, so monkeypatch.setenv in tests works correctly. The cost is microseconds per dispatch — negligible. Tests passing after this commit (65 total → 65 pass): Newly added (3): - test_strict_mode_raises_invalid_transition - test_strict_mode_does_not_raise_for_legal_events - test_strict_mode_legal_but_no_op_does_not_raise Plus the existing 62 still pass — the legality check is non- invasive in release mode (existing tests run without BOORU_VIEWER_STRICT_STATE set, so they see release-mode behavior). Phase A (16 tests in tests/core/) still green. The state machine logic is now COMPLETE. Every state, every event, every effect is implemented with both happy-path transitions and illegal-transition handling. The remaining commits (12-16) carve the implementation into the planned file layout (effects.py split, hyprland.py extraction) and rewire the Qt adapter. Test cases for commit 12 (effects split): - Re-import after the file split still works - All 65 tests still pass after `from .effects import ...` change --- booru_viewer/gui/popout/state.py | 119 ++++++++++++++++++++++++++++++- tests/gui/popout/test_state.py | 36 ++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/booru_viewer/gui/popout/state.py b/booru_viewer/gui/popout/state.py index 6cbdbf7..2bcabd7 100644 --- a/booru_viewer/gui/popout/state.py +++ b/booru_viewer/gui/popout/state.py @@ -26,6 +26,8 @@ commits 4-11 of `docs/POPOUT_REFACTOR_PLAN.md`. from __future__ import annotations +import logging +import os from dataclasses import dataclass from enum import Enum from typing import Optional, Union @@ -33,6 +35,41 @@ from typing import Optional, Union from .viewport import Viewport +log = logging.getLogger("booru.popout.state") + + +class InvalidTransition(Exception): + """Raised by `StateMachine.dispatch()` when an event arrives in a + state that doesn't accept it. + + Only raised when `BOORU_VIEWER_STRICT_STATE` is set in the + environment. In release mode (the default), illegal transitions + are dropped silently and a `log.debug` line is emitted instead. + Production runs in release mode; development and the test suite + can opt into strict mode to catch programmer errors at the + dispatch boundary instead of letting them silently no-op. + + The strict-mode raise is the structural alternative to "wait for + a downstream symptom and then bisect to find the bad dispatch." + """ + + def __init__(self, state, event): + super().__init__( + f"Invalid event {type(event).__name__} in state {state.name}" + ) + self.state = state + self.event = event + + +def _strict_mode_enabled() -> bool: + """Read the strict-mode env var at dispatch time. + + Per-dispatch read (not cached at import) so monkeypatch in tests + works correctly. Cheap — `os.environ.get` is microseconds. + """ + return bool(os.environ.get("BOORU_VIEWER_STRICT_STATE")) + + # ---------------------------------------------------------------------- # States # ---------------------------------------------------------------------- @@ -444,6 +481,68 @@ Effect = Union[ ] +# ---------------------------------------------------------------------- +# Legality map: which events are valid in which states +# ---------------------------------------------------------------------- +# +# Used by `StateMachine.dispatch()` for the env-gated strict-mode +# `InvalidTransition` raise. In release mode, illegal events are +# dropped silently (log.debug + return []). In strict mode, they raise +# to catch programmer errors at the dispatch boundary. +# +# A few events are GLOBALLY legal in any non-Closing state: +# - NavigateRequested +# - MuteToggleRequested / VolumeSet / LoopModeSet +# - FullscreenToggled +# - WindowMoved / WindowResized / HyprlandDriftDetected +# - CloseRequested +# - ContentArrived (the adapter can replace media at any time) +# +# State-specific events are listed per state. Some events are +# "legal-but-no-op" — most importantly VideoEofReached in LoadingVideo +# and SeekingVideo (the EOF race fix accepts these and drops them +# without acting). Those count as legal because the state machine +# intentionally observes them; the dropping IS the behavior. + +_GLOBAL_NON_CLOSING_EVENTS: frozenset[type] = frozenset({ + ContentArrived, + NavigateRequested, + MuteToggleRequested, + VolumeSet, + LoopModeSet, + FullscreenToggled, + WindowMoved, + WindowResized, + HyprlandDriftDetected, + CloseRequested, +}) + +_LEGAL_EVENTS_BY_STATE: dict[State, frozenset[type]] = { + State.AWAITING_CONTENT: _GLOBAL_NON_CLOSING_EVENTS | frozenset({Open}), + State.DISPLAYING_IMAGE: _GLOBAL_NON_CLOSING_EVENTS, + State.LOADING_VIDEO: _GLOBAL_NON_CLOSING_EVENTS | frozenset({ + VideoStarted, + VideoEofReached, # legal-but-no-op (EOF race fix) + VideoSizeKnown, + }), + State.PLAYING_VIDEO: _GLOBAL_NON_CLOSING_EVENTS | frozenset({ + VideoEofReached, + VideoSizeKnown, + SeekRequested, + TogglePlayRequested, + }), + State.SEEKING_VIDEO: _GLOBAL_NON_CLOSING_EVENTS | frozenset({ + VideoEofReached, # legal-but-no-op (drops during seek) + VideoSizeKnown, + SeekRequested, + SeekCompleted, + }), + # Closing is terminal — every event drops at the dispatch entry, + # so the legal set is empty (no event reaches the legality check). + State.CLOSING: frozenset(), +} + + # ---------------------------------------------------------------------- # StateMachine # ---------------------------------------------------------------------- @@ -562,7 +661,24 @@ class StateMachine: if self.state == State.CLOSING: return [] - # Skeleton routing. Real handlers land in later commits. + # Legality check: env-gated strict mode (BOORU_VIEWER_STRICT_STATE) + # raises InvalidTransition; release mode drops + logs at debug. + # The legality map distinguishes "intentionally legal-but-no-op" + # (e.g. VideoEofReached in LoadingVideo — the EOF race fix) from + # "structurally invalid" (e.g. SeekRequested in DisplayingImage — + # no video to seek into). + legal_events = _LEGAL_EVENTS_BY_STATE.get(self.state, frozenset()) + if type(event) not in legal_events: + if _strict_mode_enabled(): + raise InvalidTransition(self.state, event) + log.debug( + "Dropping illegal event %s in state %s", + type(event).__name__, + self.state.name, + ) + return [] + + # Routing. match event: case Open(): return self._on_open(event) @@ -1075,4 +1191,5 @@ __all__ = [ "Effect", # Machine "StateMachine", + "InvalidTransition", ] diff --git a/tests/gui/popout/test_state.py b/tests/gui/popout/test_state.py index 930fe28..8c891ba 100644 --- a/tests/gui/popout/test_state.py +++ b/tests/gui/popout/test_state.py @@ -28,6 +28,7 @@ import pytest from booru_viewer.gui.popout.state import ( # Enums + InvalidTransition, LoopMode, MediaKind, State, @@ -515,6 +516,41 @@ def test_invariant_pending_mute_replayed_into_video(): # At commits 3-10 they return [] (the skeleton's default). +def test_strict_mode_raises_invalid_transition(monkeypatch): + """When BOORU_VIEWER_STRICT_STATE is set, illegal events raise + InvalidTransition instead of dropping silently. This is the + development/debug mode that catches programmer errors at the + dispatch boundary.""" + monkeypatch.setenv("BOORU_VIEWER_STRICT_STATE", "1") + m = _new_in(State.PLAYING_VIDEO) + with pytest.raises(InvalidTransition) as exc_info: + m.dispatch(VideoStarted()) + assert exc_info.value.state == State.PLAYING_VIDEO + assert isinstance(exc_info.value.event, VideoStarted) + + +def test_strict_mode_does_not_raise_for_legal_events(monkeypatch): + """Legal events go through dispatch normally even under strict mode.""" + monkeypatch.setenv("BOORU_VIEWER_STRICT_STATE", "1") + m = _new_in(State.PLAYING_VIDEO) + # SeekRequested IS legal in PlayingVideo — no raise + effects = m.dispatch(SeekRequested(target_ms=5000)) + assert m.state == State.SEEKING_VIDEO + + +def test_strict_mode_legal_but_no_op_does_not_raise(monkeypatch): + """The 'legal-but-no-op' events (e.g. VideoEofReached in + LoadingVideo, the EOF race fix) must NOT raise in strict mode. + They're intentionally accepted and dropped — that's the + structural fix, not a programmer error.""" + monkeypatch.setenv("BOORU_VIEWER_STRICT_STATE", "1") + m = _new_in(State.LOADING_VIDEO) + # VideoEofReached in LoadingVideo is legal-but-no-op + effects = m.dispatch(VideoEofReached()) + assert effects == [] + assert m.state == State.LOADING_VIDEO + + @pytest.mark.parametrize("source_state, illegal_event", [ (State.AWAITING_CONTENT, VideoEofReached()), (State.AWAITING_CONTENT, VideoStarted()),