From cec93545ad06ad3af62c38707b1ad15a9f105cfd Mon Sep 17 00:00:00 2001 From: pax Date: Wed, 15 Apr 2026 17:47:36 -0500 Subject: [PATCH] popout: drop in-flight-refactor language from docstrings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During the state machine extraction every comment that referenced a specific commit in the plan (skeleton / 14a / 14b / 'future commit') was useful — it told you which commit a line appeared in and what was about to change. Once the refactor landed those notes became noise: they describe history nobody needs while reading the current code. Rewrites keep the rationale (no-op handlers still explain WHY they're no-ops, Loop=Next / video auto-fit still have their explanations) and preserves the load-bearing commit 14b reference in _dispatch_and_apply's docstring — that one actually does protect future-you from reintroducing the bug-by-typo pattern. --- booru_viewer/gui/popout/effects.py | 2 +- booru_viewer/gui/popout/hyprland.py | 10 +- booru_viewer/gui/popout/state.py | 37 ++----- booru_viewer/gui/popout/window.py | 165 +++++++++++++--------------- 4 files changed, 89 insertions(+), 125 deletions(-) diff --git a/booru_viewer/gui/popout/effects.py b/booru_viewer/gui/popout/effects.py index 1c278ac..92a57bf 100644 --- a/booru_viewer/gui/popout/effects.py +++ b/booru_viewer/gui/popout/effects.py @@ -114,7 +114,7 @@ class FitWindowToContent: """Compute the new window rect for the given content aspect using `state.viewport` and dispatch it to Hyprland (or `setGeometry()` on non-Hyprland). The adapter delegates the rect math + dispatch - to `popout/hyprland.py`'s helper, which lands in commit 13. + to the helpers in `popout/hyprland.py`. """ content_w: int diff --git a/booru_viewer/gui/popout/hyprland.py b/booru_viewer/gui/popout/hyprland.py index 8faee0a..7467c96 100644 --- a/booru_viewer/gui/popout/hyprland.py +++ b/booru_viewer/gui/popout/hyprland.py @@ -11,11 +11,11 @@ behind the same `HYPRLAND_INSTANCE_SIGNATURE` env var check the legacy code used. Off-Hyprland systems no-op or return None at every entry point. -The legacy `FullscreenPreview._hyprctl_*` methods become 1-line -shims that call into this module — see commit 13's changes to -`popout/window.py`. The shims preserve byte-for-byte call-site -compatibility for the existing window.py code; commit 14's adapter -rewrite drops them in favor of direct calls. +The popout adapter calls these helpers directly; there are no +`FullscreenPreview._hyprctl_*` shims anymore. Every env-var gate +for opt-out (`BOORU_VIEWER_NO_HYPR_RULES`, popout-specific aspect +lock) is implemented inside these functions so every call site +gets the same behavior. """ from __future__ import annotations diff --git a/booru_viewer/gui/popout/state.py b/booru_viewer/gui/popout/state.py index 9cabb0c..23f1668 100644 --- a/booru_viewer/gui/popout/state.py +++ b/booru_viewer/gui/popout/state.py @@ -16,12 +16,6 @@ becomes the forcing function that keeps this module pure. The architecture, state diagram, invariant→transition mapping, and event/effect lists are documented in `docs/POPOUT_ARCHITECTURE.md`. This module's job is to be the executable form of that document. - -This is the **commit 2 skeleton**: every state, every event type, every -effect type, and the `StateMachine` class with all fields initialized. -The `dispatch` method routes events to per-event handlers that all -currently return empty effect lists. Real transitions land in -commits 4-11 of `docs/POPOUT_REFACTOR_PLAN.md`. """ from __future__ import annotations @@ -423,10 +417,6 @@ class StateMachine: The state machine never imports Qt or mpv. It never calls into the adapter. The communication is one-directional: events in, effects out. - - **This is the commit 2 skeleton**: all state fields are initialized, - `dispatch` is wired but every transition handler is a stub that - returns an empty effect list. Real transitions land in commits 4-11. """ def __init__(self) -> None: @@ -511,14 +501,7 @@ class StateMachine: # and reads back the returned effects + the post-dispatch state. def dispatch(self, event: Event) -> list[Effect]: - """Process one event and return the effect list. - - **Skeleton (commit 2):** every event handler currently returns - an empty effect list. Real transitions land in commits 4-11. - Tests written in commit 3 will document what each transition - is supposed to do; they fail at this point and progressively - pass as the transitions land. - """ + """Process one event and return the effect list.""" # Closing is terminal — drop everything once we're done. if self.state == State.CLOSING: return [] @@ -577,13 +560,13 @@ class StateMachine: case CloseRequested(): return self._on_close_requested(event) case _: - # Unknown event type. Returning [] keeps the skeleton - # safe; the illegal-transition handler in commit 11 - # will replace this with the env-gated raise. + # Unknown event type — defensive fall-through. The + # legality check above is the real gate; in release + # mode illegal events log and drop, strict mode raises. return [] # ------------------------------------------------------------------ - # Per-event stub handlers (commit 2 — all return []) + # Per-event handlers # ------------------------------------------------------------------ def _on_open(self, event: Open) -> list[Effect]: @@ -594,8 +577,7 @@ class StateMachine: on the state machine instance for the first ContentArrived handler to consume. After Open the machine is still in AwaitingContent — the actual viewport seeding from saved_geo - happens inside the first ContentArrived (commit 8 wires the - actual viewport math; this commit just stashes the inputs). + happens inside the first ContentArrived. No effects: the popout window is already constructed and showing. The first content load triggers the first fit. @@ -610,12 +592,11 @@ class StateMachine: Snapshot the content into `current_*` fields regardless of kind so the rest of the state machine can read them. Then - transition to LoadingVideo (video) or DisplayingImage (image, - commit 10) and emit the appropriate load + fit effects. + transition to LoadingVideo (video) or DisplayingImage (image) + and emit the appropriate load + fit effects. The first-content-load one-shot consumes `saved_geo` to seed - the viewport before the first fit (commit 8 wires the actual - seeding). After this commit, every ContentArrived flips + the viewport before the first fit. Every ContentArrived flips `is_first_content_load` to False — the saved_geo path runs at most once per popout open. """ diff --git a/booru_viewer/gui/popout/window.py b/booru_viewer/gui/popout/window.py index a315ea8..54c374f 100644 --- a/booru_viewer/gui/popout/window.py +++ b/booru_viewer/gui/popout/window.py @@ -68,9 +68,8 @@ from .viewport import Viewport, _DRIFT_TOLERANCE, anchor_point # the dispatch trace to the Ctrl+L log panel — useful but invisible # from the shell. We additionally attach a stderr StreamHandler to # the adapter logger so `python -m booru_viewer.main_gui 2>&1 | -# grep POPOUT_FSM` works during the commit-14a verification gate. -# The handler is tagged with a sentinel attribute so re-imports -# don't stack duplicates. +# grep POPOUT_FSM` works from the terminal. The handler is tagged +# with a sentinel attribute so re-imports don't stack duplicates. import sys as _sys _fsm_log = logging.getLogger("booru.popout.adapter") _fsm_log.setLevel(logging.DEBUG) @@ -146,25 +145,20 @@ class FullscreenPreview(QMainWindow): self._stack.addWidget(self._viewer) self._video = VideoPlayer() - # Note: two legacy VideoPlayer signal connections removed in - # commits 14b and 16: + # Two legacy VideoPlayer forwarding connections were removed + # during the state machine extraction — don't reintroduce: # - # - `self._video.play_next.connect(self.play_next_requested)` - # (removed in 14b): the EmitPlayNextRequested effect now - # emits play_next_requested via the state machine dispatch - # path. Keeping the forwarding would double-emit the signal - # and cause main_window to navigate twice on every video - # EOF in Loop=Next mode. + # - `self._video.play_next.connect(self.play_next_requested)`: + # the EmitPlayNextRequested effect emits play_next_requested + # via the state machine dispatch path. Keeping the forward + # would double-emit on every video EOF in Loop=Next mode. # - # - `self._video.video_size.connect(self._on_video_size)` - # (removed in 16): the dispatch path's VideoSizeKnown - # handler emits FitWindowToContent which the apply path - # delegates to _fit_to_content. The legacy direct call to - # _on_video_size → _fit_to_content was a parallel duplicate - # that the same-rect skip in _fit_to_content made harmless, - # but it muddied the trace. The dispatch lambda below is - # wired in the same __init__ block (post state machine - # construction) and is now the sole path. + # - `self._video.video_size.connect(self._on_video_size)`: + # the dispatch path's VideoSizeKnown handler produces + # FitWindowToContent which the apply path delegates to + # _fit_to_content. The direct forwarding was a parallel + # duplicate that same-rect-skip in _fit_to_content masked + # but that muddied the dispatch trace. self._stack.addWidget(self._video) self.setCentralWidget(central) @@ -374,17 +368,15 @@ class FullscreenPreview(QMainWindow): else: self.showFullScreen() - # ---- State machine adapter wiring (commit 14a) ---- + # ---- State machine adapter wiring ---- # Construct the pure-Python state machine and dispatch the # initial Open event with the cross-popout-session class state - # the legacy code stashed above. The state machine runs in - # PARALLEL with the legacy imperative code: every Qt event - # handler / mpv signal / button click below dispatches a state - # machine event AND continues to run the existing imperative - # action. The state machine's returned effects are LOGGED at - # DEBUG, not applied to widgets. The legacy path stays - # authoritative through commit 14a; commit 14b switches the - # authority to the dispatch path. + # the legacy code stashed above. Every Qt event handler, mpv + # signal, and button click below dispatches a state machine + # event via `_dispatch_and_apply`, which applies the returned + # effects to widgets. The state machine is the authority for + # "what to do next"; the imperative helpers below are the + # implementation the apply path delegates into. # # The grid_cols field is used by the keyboard nav handlers # for the Up/Down ±cols stride. @@ -403,20 +395,17 @@ class FullscreenPreview(QMainWindow): monitor=monitor, )) - # Wire VideoPlayer's playback_restart Signal (added in commit 1) - # to the adapter's dispatch routing. mpv emits playback-restart - # once after each loadfile and once after each completed seek; - # the adapter distinguishes by checking the state machine's - # current state at dispatch time. + # Wire VideoPlayer's playback_restart Signal to the adapter's + # dispatch routing. mpv emits playback-restart once after each + # loadfile and once after each completed seek; the adapter + # distinguishes by checking the state machine's current state + # at dispatch time. self._video.playback_restart.connect(self._on_video_playback_restart) # Wire VideoPlayer signals to dispatch+apply via the - # _dispatch_and_apply helper. NOTE: every lambda below MUST - # call _dispatch_and_apply, not _fsm_dispatch directly. Calling - # _fsm_dispatch alone produces effects that never reach - # widgets — the bug that landed in commit 14b and broke - # video auto-fit (FitWindowToContent never applied) and - # Loop=Next play_next (EmitPlayNextRequested never applied) - # until the lambdas were fixed in this commit. + # _dispatch_and_apply helper. Every lambda below MUST call + # _dispatch_and_apply, not _fsm_dispatch directly — see the + # docstring on _dispatch_and_apply for the historical bug that + # explains the distinction. self._video.play_next.connect( lambda: self._dispatch_and_apply(VideoEofReached()) ) @@ -465,8 +454,8 @@ class FullscreenPreview(QMainWindow): Adapter-internal helper. Centralizes the dispatch + log path so every wire-point is one line. Returns the effect list for - callers that want to inspect it (commit 14a doesn't use the - return value; commit 14b will pattern-match and apply). + callers that want to inspect it; prefer `_dispatch_and_apply` + at wire-points so the apply step can't be forgotten. The hasattr guard handles edge cases where Qt events might fire during __init__ (e.g. resizeEvent on the first show()) @@ -488,10 +477,10 @@ class FullscreenPreview(QMainWindow): return effects def _on_video_playback_restart(self) -> None: - """mpv `playback-restart` event arrived (via VideoPlayer's - playback_restart Signal added in commit 1). Distinguish - VideoStarted (after load) from SeekCompleted (after seek) by - the state machine's current state. + """mpv `playback-restart` event arrived via VideoPlayer's + playback_restart Signal. Distinguish VideoStarted (after load) + from SeekCompleted (after seek) by the state machine's current + state. This is the ONE place the adapter peeks at state to choose an event type — it's a read, not a write, and it's the price of @@ -508,42 +497,35 @@ class FullscreenPreview(QMainWindow): # round trip. # ------------------------------------------------------------------ - # Commit 14b — effect application + # Effect application # ------------------------------------------------------------------ # # The state machine's dispatch returns a list of Effect descriptors # describing what the adapter should do. `_apply_effects` is the - # single dispatch point: every wire-point that calls `_fsm_dispatch` - # follows it with `_apply_effects(effects)`. The pattern-match by - # type is the architectural choke point — if a new effect type is - # added in state.py, the type-check below catches the missing - # handler at runtime instead of silently dropping. + # single dispatch point: `_dispatch_and_apply` dispatches then calls + # this. The pattern-match by type is the architectural choke point + # — a new Effect type in state.py triggers the TypeError branch at + # runtime instead of silently dropping the effect. # - # Several apply handlers are deliberate no-ops in commit 14b: + # A few apply handlers are intentional no-ops: # # - ApplyMute / ApplyVolume / ApplyLoopMode: the legacy slot - # connections on the popout's VideoPlayer are still active and - # handle the user-facing toggles directly. The state machine - # tracks these values for the upcoming SyncFromEmbedded path - # (future commit) but doesn't push them to widgets — pushing - # would create a sync hazard with the embedded preview's mute - # state, which main_window pushes via direct attribute writes. + # connections on the popout's VideoPlayer handle the user-facing + # toggles directly. The state machine tracks these values as the + # source of truth for sync with the embedded preview; pushing + # them back here would create a double-write hazard. # - # - SeekVideoTo: the legacy `_ClickSeekSlider.clicked_position → - # VideoPlayer._seek` connection still handles both the mpv.seek - # call and the legacy 500ms `_seek_pending_until` pin window. - # The state machine's SeekingVideo state tracks the seek for - # future authority, but the slider rendering and the seek call - # itself stay legacy. Replacing this requires either modifying - # VideoPlayer's _poll loop (forbidden by the no-touch rule) or - # building a custom poll loop in the adapter. + # - SeekVideoTo: `_ClickSeekSlider.clicked_position → _seek` on the + # VideoPlayer handles both the mpv.seek call and the legacy + # 500ms pin window. The state machine's SeekingVideo state + # tracks the seek; the slider rendering and the seek call itself + # live on VideoPlayer. # - # The other effect types (LoadImage, LoadVideo, StopMedia, + # Every other effect (LoadImage, LoadVideo, StopMedia, # FitWindowToContent, EnterFullscreen, ExitFullscreen, # EmitNavigate, EmitPlayNextRequested, EmitClosed, TogglePlay) - # delegate to existing private helpers in this file. The state - # machine becomes the official entry point for these operations; - # the helpers stay in place as the implementation. + # delegates to a private helper in this file. The state machine + # is the entry point; the helpers are the implementation. def _apply_effects(self, effects: list) -> None: """Apply a list of Effect descriptors returned by dispatch. @@ -560,18 +542,19 @@ class FullscreenPreview(QMainWindow): elif isinstance(e, StopMedia): self._apply_stop_media() elif isinstance(e, ApplyMute): - # No-op in 14b — legacy slot handles widget update. - # State machine tracks state.mute for future authority. + # No-op — VideoPlayer's legacy slot owns widget update; + # the state machine keeps state.mute as the sync source + # for the embedded-preview path. pass elif isinstance(e, ApplyVolume): - pass # same — no-op in 14b + pass # same — widget update handled by VideoPlayer elif isinstance(e, ApplyLoopMode): - pass # same — no-op in 14b + pass # same — widget update handled by VideoPlayer elif isinstance(e, SeekVideoTo): - # No-op in 14b — legacy `_seek` slot handles both - # mpv.seek (now exact) and the pin window. Replacing - # this requires touching VideoPlayer._poll which is - # out of scope. + # No-op — `_seek` slot on VideoPlayer handles both + # mpv.seek and the pin window. The state's SeekingVideo + # fields exist so the slider's read-path still returns + # the clicked position during the seek. pass elif isinstance(e, TogglePlay): self._video._toggle_play() @@ -687,14 +670,14 @@ class FullscreenPreview(QMainWindow): self._save_btn.setToolTip("Unsave from library" if saved else "Save to library (S)") # ------------------------------------------------------------------ - # Public method interface (commit 15) + # Public method interface # ------------------------------------------------------------------ # - # The methods below replace direct underscore access from - # main_window.py. They wrap the existing private fields so - # main_window doesn't have to know about VideoPlayer / ImageViewer - # / QStackedWidget internals. The legacy private fields stay in - # place — these are clean public wrappers, not a re-architecture. + # The methods below are the only entry points main_window.py uses + # to drive the popout. They wrap the private fields so main_window + # doesn't have to know about VideoPlayer / ImageViewer / + # QStackedWidget internals. The private fields stay in place; these + # are clean public wrappers, not a re-architecture. def is_video_active(self) -> bool: """True if the popout is currently showing a video (vs image). @@ -1489,11 +1472,11 @@ class FullscreenPreview(QMainWindow): return True elif key == Qt.Key.Key_Period and self._stack.currentIndex() == 1: # +/- keys are seek-relative, NOT slider-pin seeks. The - # state machine's SeekRequested is for slider-driven - # seeks. The +/- keys go straight to mpv via the - # legacy path; the dispatch path doesn't see them in - # 14a (commit 14b will route them through SeekRequested - # with a target_ms computed from current position). + # state machine's SeekRequested models slider-driven + # seeks (target_ms known up front); relative seeks go + # straight to mpv. If we ever want the dispatch path to + # own them, compute target_ms from current position and + # route through SeekRequested. self._video._seek_relative(1800) return True elif key == Qt.Key.Key_Comma and self._stack.currentIndex() == 1: