popout/state: implement illegal transition handler (env-gated)
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
This commit is contained in:
parent
4fb17151b1
commit
3ade3a71c1
@ -26,6 +26,8 @@ commits 4-11 of `docs/POPOUT_REFACTOR_PLAN.md`.
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import logging
|
||||||
|
import os
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
from enum import Enum
|
from enum import Enum
|
||||||
from typing import Optional, Union
|
from typing import Optional, Union
|
||||||
@ -33,6 +35,41 @@ from typing import Optional, Union
|
|||||||
from .viewport import Viewport
|
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
|
# 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
|
# StateMachine
|
||||||
# ----------------------------------------------------------------------
|
# ----------------------------------------------------------------------
|
||||||
@ -562,7 +661,24 @@ class StateMachine:
|
|||||||
if self.state == State.CLOSING:
|
if self.state == State.CLOSING:
|
||||||
return []
|
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:
|
match event:
|
||||||
case Open():
|
case Open():
|
||||||
return self._on_open(event)
|
return self._on_open(event)
|
||||||
@ -1075,4 +1191,5 @@ __all__ = [
|
|||||||
"Effect",
|
"Effect",
|
||||||
# Machine
|
# Machine
|
||||||
"StateMachine",
|
"StateMachine",
|
||||||
|
"InvalidTransition",
|
||||||
]
|
]
|
||||||
|
|||||||
@ -28,6 +28,7 @@ import pytest
|
|||||||
|
|
||||||
from booru_viewer.gui.popout.state import (
|
from booru_viewer.gui.popout.state import (
|
||||||
# Enums
|
# Enums
|
||||||
|
InvalidTransition,
|
||||||
LoopMode,
|
LoopMode,
|
||||||
MediaKind,
|
MediaKind,
|
||||||
State,
|
State,
|
||||||
@ -515,6 +516,41 @@ def test_invariant_pending_mute_replayed_into_video():
|
|||||||
# At commits 3-10 they return [] (the skeleton's default).
|
# 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", [
|
@pytest.mark.parametrize("source_state, illegal_event", [
|
||||||
(State.AWAITING_CONTENT, VideoEofReached()),
|
(State.AWAITING_CONTENT, VideoEofReached()),
|
||||||
(State.AWAITING_CONTENT, VideoStarted()),
|
(State.AWAITING_CONTENT, VideoStarted()),
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user