The previous FlowLayout._do_layout walked each thumb summing
`widget.width() + THUMB_SPACING` and wrapped on `x + item_w >
self.width()`. This was vulnerable to two issues that conspired to
produce the "grid collapses by a column when switching to a post"
bug:
1. **Per-widget width drift**: ThumbnailWidget calls
`setFixedSize(THUMB_SIZE, THUMB_SIZE)` in __init__, capturing the
constant at construction time. If `THUMB_SIZE` is later mutated
via `_apply_settings` (main_window.py:2953 writes
`grid_mod.THUMB_SIZE = new_size`), existing thumbs keep their old
fixed size while new ones (e.g. from infinite-scroll backfill via
`append_posts`) get the new value. Mixed widths break the
width-summing wrap loop.
2. **Off-by-one in the columns property**: `w // (THUMB_SIZE +
THUMB_SPACING)` overcounted by 1 at column boundaries because it
omitted the leading THUMB_SPACING margin. A row that fits N
thumbs needs `THUMB_SPACING + N * step` pixels, not `N * step`.
For width=1135 with step=188, the formula returned 6 columns
while `_do_layout` only fit 5 — the two diverged whenever the
grid sat in the boundary range.
Both are fixed by using a deterministic position formula:
cols = max(1, (width - THUMB_SPACING) // step)
for each thumb i:
col = i % cols
row = i // cols
x = THUMB_SPACING + col * step
y = THUMB_SPACING + row * step
The layout is now a function of `self.width()` and the constants
only — no per-widget reads, no width-summing accumulator. The
columns property uses the EXACT same formula so callers (e.g.
main_window's keyboard Up/Down nav step) always get the value the
visual layout actually used.
Standardizing on the constant means existing thumbs that were
created with an old `THUMB_SIZE` value still position correctly
(they sit in the cells positioned by the new step), and any future
mutation of THUMB_SIZE only affects newly-created thumbs without
breaking the layout of the surviving ones.
Affects all three tabs (Browse / Bookmarks / Library) since they
all use ThumbnailGrid from grid.py.
Verification:
- Phase A test suite (16 tests) still passes
- Popout state machine tests (65 tests) still pass
- Total: 81 / 81 automated tests green
- Imports clean
- Manual: open the popout to a column boundary (resize window
width such that the grid is exactly N columns wide), switch
between posts — column count should NOT flip to N-1 anymore.
Also verify keyboard Up/Down nav steps by exactly the column
count visible on screen (was off-by-one before at boundaries).
The ThumbnailGrid was setting horizontal scrollbar to AlwaysOff
explicitly but leaving the vertical scrollbar at the default
AsNeeded. When content first overflowed enough to summon the
vertical scrollbar, the viewport width dropped by ~14-16px
(scrollbar width), and FlowLayout's column count flipped down by 1
because the integer-division formula sat right at a boundary.
columns = max(1, w // (THUMB_SIZE + THUMB_SPACING))
For THUMB_SIZE=180 + THUMB_SPACING=6 (per-column step = 186):
- viewport 1122 → 6 columns
- viewport 1108 (1122 - 14 scrollbar) → 5 columns
If the popout/main window happened to sit anywhere in the range
where `viewport_width % 186 < scrollbar_width`, the column count
flipped when the scrollbar appeared. The user saw "the grid
collapses by a column when switching to a post" — the actual
trigger isn't post selection, it's the grid scrolling enough to
bring the selected thumbnail into view, which makes content
visibly overflow and summons the scrollbar. From the user's
perspective the two events looked correlated.
Fix: setVerticalScrollBarPolicy(Qt.ScrollBarAlwaysOn). The
scrollbar is now always visible, its width is always reserved in
the viewport, and FlowLayout's column count is stable across the
scrollbar visibility transition.
Trade-off: a slim grey scrollbar strip is always visible on the
right edge of the grid, even when content fits on one screen and
would otherwise have no scrollbar. For an image grid that almost
always overflows in practice, this is the standard behavior (most
file browsers / image viewers do the same) and the cost is
invisible after the first few thumbnails load.
Affects all three tabs (Browse / Bookmarks / Library) since they
all use ThumbnailGrid from grid.py.
Verification:
- Phase A test suite (16 tests) still passes
- Popout state machine tests (65 tests) still pass
- Total: 81 / 81 automated tests green
- Imports clean
- Manual: open the popout to a column boundary (resize window
width such that the grid is exactly N columns wide before any
scrolling), then scroll down — column count should NOT flip to
N-1 anymore.
The signal-connection lambdas in __init__ added by commit 14a only
called _fsm_dispatch — they never followed up with _apply_effects.
Commit 14b added the apply layer and updated the keyboard event
handlers in eventFilter to dispatch+apply, but missed the lambdas.
Result: every effect produced by an mpv-driven signal was silently
dropped.
Two user-visible regressions:
1. Video auto-fit (and aspect ratio lock) broken in popout. The
mpv `video-params` observer fires when mpv reports video
dimensions, and the chain is:
_on_video_params (mpv thread) → _pending_video_size set
→ _poll → video_size.emit(w, h)
→ connected lambda → dispatch VideoSizeKnown(w, h)
→ state machine emits FitWindowToContent(w, h)
→ adapter SHOULD apply by calling _fit_to_content
The lambda dropped the effects, so _fit_to_content never ran
for video loads. Image loads were unaffected because they go
through set_media's ContentArrived dispatch (which DOES apply
via _dispatch_and_apply in this commit) with API-known
dimensions.
2. Loop=Next play_next broken. The mpv eof → VideoPlayer.play_next
→ connected lambda → dispatch VideoEofReached chain produces an
EmitPlayNextRequested effect in PlayingVideo + Loop=Next, but
the lambda dropped the effect, so self.play_next_requested was
never emitted, and main_window's _on_video_end_next never fired.
The user reported the auto-fit breakage; the play_next breakage
was the silent twin that no one noticed because Loop=Next isn't
the default.
Both bugs landed in commit 14b. The seek pin removal in d48435d
didn't cause them but exposed the auto-fit one because the user
was paying attention to popout sizing during the slider verification.
Fix:
- Add `_dispatch_and_apply(event)` helper. The single line of
documentation in its docstring tells future-pax: "if you're
going to dispatch an event, go through this helper, not bare
_fsm_dispatch." This makes the apply step impossible to forget
for any new wire-point.
- Update all 6 signal-connection lambdas to call _dispatch_and_apply:
play_next → VideoEofReached
video_size → VideoSizeKnown
clicked_position → SeekRequested
_mute_btn.clicked → MuteToggleRequested
_vol_slider.valueChanged → VolumeSet
_loop_btn.clicked → LoopModeSet
- Update the rest of the dispatch sites (keyboard event handlers in
eventFilter, the wheel-tilt navigation, the wheel-vertical volume
scroll, _on_video_playback_restart, set_media, closeEvent, the
Open dispatch in __init__, and the WindowResized/WindowMoved
dispatches in resizeEvent/moveEvent) to use _dispatch_and_apply
for consistency. The keyboard handlers were already calling
dispatch+apply via the two-line `effects = ...; self._apply_effects(effects)`
pattern; switching to the helper is just deduplication. The
Open / Window* dispatches were bare _fsm_dispatch but their
handlers return [] anyway so the apply was a no-op.
After this commit, every dispatch site in the popout adapter goes
through one helper. The only remaining `self._fsm_dispatch(...)` call
is inside the helper itself (line 437) and one reference in the
helper's docstring.
Verification:
- Phase A test suite (16 tests) still passes
- State machine tests (65 tests) still pass — none of them touch
the adapter wiring
- 81 / 81 tests green at HEAD
Manual verification needed:
- Click an uncached video in browse → popout opens, video loads,
popout auto-fits to video aspect, Hyprland aspect lock applies
- Click cached video → same
- Loop=Next mode + video reaches EOF → popout advances to next post
(was silently broken since 14b)
- Image load still auto-fits (regression check — image path was
already working via ContentArrived's immediate FitWindowToContent)
The 500ms `_seek_pending_until` pin window in `VideoPlayer._poll`
became redundant after `609066c` switched the slider seek from
`'absolute'` to `'absolute+exact'`. With exact seek, mpv decodes
from the previous keyframe forward to the click position before
reporting it via `time_pos`, so `_poll`'s read-and-write loop
naturally lands the slider at the click position without any
pinning. The pin was defense in depth for keyframe-rounding latency
that no longer exists.
Removed:
- `_seek_target_ms`, `_seek_pending_until`, `_seek_pin_window_secs`
fields from `__init__`
- The `_time.monotonic() < _seek_pending_until` branch in `_poll`
(now unconditionally `setValue(pos_ms)` after the isSliderDown
check)
- The pin-arming logic from `_seek` (now just calls `mpv.seek`
directly)
Net diff: ~30 lines removed, ~10 lines of explanatory comments
added pointing future-pax at the `609066c` commit body for the
"why" of the cleanup.
The popout's state machine SeekingVideo state continues to track
seeks via the dispatch path (seek_target_ms is held on the state
machine, not on VideoPlayer) for the future when the adapter's
SeekVideoTo apply handler grows past its current no-op. The
removal here doesn't affect that — it only drops dead defense-in-
depth code from the legacy slider rendering path.
Verification:
- Phase A test suite (16 tests) still passes
- State machine tests (65 tests) still pass — none of them touch
VideoPlayer fields
- Both surfaces (embedded preview + popout) still seek correctly
per the post-609066c verification (commit 14a/14b sweep)
Followup target from docs/POPOUT_FINAL.md "What's NOT done"
section. The other listed followup (replace self._viewport with
self._state_machine.viewport in popout/window.py) is bigger and
filed for a future session.
Removes the tests/ folder from git tracking and adds it to .gitignore.
The 81 tests (16 Phase A core + 65 popout state machine) stay on
disk as local-only working notes, the same way docs/ and project.md
are gitignored. Running them is `pytest tests/` from the project
root inside .venv as before — nothing about the tests themselves
changed, just whether they're version-controlled.
Reverts the related additions in pyproject.toml and README.md from
commit bf14466 (Phase A baseline) so the public surface doesn't
reference a tests/ folder that no longer ships:
- pyproject.toml: drops [project.optional-dependencies] test extra
and [tool.pytest.ini_options]. pytest + pytest-asyncio are still
installed in the local .venv via the previous pip install -e ".[test]"
so the suite keeps running locally; new clones won't get them
automatically.
- README.md: drops the "Run tests:" section from the Linux install
block. The README's install instructions return to their pre-
Phase-A state.
- .gitignore: adds `tests/` alongside the existing `docs/` and
`project.md` lines (the same convention used for the refactor
inventory / plan / notes / final report docs).
The 12 test files removed from tracking (`git rm -r --cached`):
tests/__init__.py
tests/conftest.py
tests/core/__init__.py
tests/core/test_cache.py
tests/core/test_concurrency.py
tests/core/test_config.py
tests/core/test_db.py
tests/core/api/__init__.py
tests/core/api/test_base.py
tests/gui/__init__.py
tests/gui/popout/__init__.py
tests/gui/popout/test_state.py
Verification:
- tests/ still exists on disk
- `pytest tests/` still runs and passes 81 / 81 in 0.11s
- `git ls-files tests/` returns nothing
- `git status` is clean
Removes the last vestiges of the legacy compatibility layer that
commits 13-15 left in place to keep the app runnable across the
authority transfer:
1. Three `_hyprctl_*` shim methods on FullscreenPreview that
delegated to the popout/hyprland module-level functions. Commit
13 added them to preserve byte-for-byte call-site compatibility
while window.py still had its old imperative event handling.
After commit 14b switched authority to the dispatch+apply path
and commit 15 cleaned up main_window's interface, every remaining
call site in window.py is updated to call hyprland.* directly:
self._hyprctl_get_window() → hyprland.get_window(self.windowTitle())
self._hyprctl_resize(0, 0) → hyprland.resize(self.windowTitle(), 0, 0)
self._hyprctl_resize_and_move(...) → hyprland.resize_and_move(self.windowTitle(), ...)
8 internal call sites updated, 3 shim methods removed.
2. The legacy `self._video.video_size.connect(self._on_video_size)`
parallel-path connection plus the dead `_on_video_size` method.
The dispatch lambda wired in __init__ already handles
VideoSizeKnown → FitWindowToContent → _fit_to_content via the
apply path. The legacy direct connection was a duplicate that
the same-rect skip in _fit_to_content made harmless, but it
muddied the dispatch trace and was dead weight after 14b.
A new `from . import hyprland` at the top of window.py imports the
module once at load time instead of inline-importing on every shim
call (the legacy shims used `from . import hyprland` inside each
method body to avoid import order issues during the commit-13
extraction).
After this commit, FullscreenPreview's interaction with Hyprland is:
- Single import: `from . import hyprland`
- Direct calls: `hyprland.get_window(self.windowTitle())` etc
- No shim layer
- The popout/hyprland module is the single source of Hyprland IPC
for the popout
Tests passing after this commit: 81 / 81 (16 Phase A + 65 state).
Phase A still green.
Final state of the popout state machine refactor:
- 6 states / 17 events / 14 effects (within budget 10/20/15)
- 6 race-fix invariants enforced structurally (no timestamp windows
in state.py, no guards, no fall-throughs)
- popout/state.py + popout/effects.py: pure Python, no PySide6, no
mpv, no httpx — verifiable via the meta_path import blocker
- popout/hyprland.py: isolated subprocess wrappers
- popout/window.py: thin Qt adapter — translates Qt events into
state machine dispatches, applies returned effects to widgets via
the existing private helpers
- main_window.py: zero direct popout._underscore access; all
interaction goes through the public method surface defined in
commit 15
Test cases / followups: none. The refactor is complete.
Drops every direct popout._underscore access from main_window in favor
of nine new public methods on FullscreenPreview. The legacy private
fields (_video, _viewer, _stack, _bookmark_btn, etc.) stay in place —
this is a clean public wrapper layer, not a re-architecture. Going
through public methods makes the popout's interface explicit and
prevents future code from reaching into popout internals.
New public methods on FullscreenPreview:
is_video_active() -> bool
Replaces popout._stack.currentIndex() == 1 checks. Used to gate
video-only operations.
set_toolbar_visibility(*, bookmark, save, bl_tag, bl_post)
Replaces 4-line popout._bookmark_btn.setVisible(...) etc block.
Per-tab toolbar gating.
sync_video_state(*, volume, mute, autoplay, loop_state)
Replaces 4-line popout._video.volume = ... etc block. Called by
main_window's _open_fullscreen_preview to push embedded preview
state into the popout.
get_video_state() -> dict
Returns volume / mute / autoplay / loop_state / position_ms in
one read. Replaces 5 separate popout._video.* attribute reads
in main_window's _on_fullscreen_closed reverse sync.
seek_video_to(ms)
Wraps VideoPlayer.seek_to_ms (which uses 'absolute+exact' since
the 609066c drag-back fix). Used by the seek-after-load pattern.
connect_media_ready_once(callback)
One-shot callback wiring with auto-disconnect. Replaces the
manual lambda + try/except disconnect dance in main_window.
pause_media()
Wraps VideoPlayer.pause(). Replaces 3 sites of direct
popout._video.pause() calls in privacy-screen / external-open
paths.
force_mpv_pause()
Direct mpv.pause = True without button text update. Replaces
the legacy popout._video._mpv.pause = True deep attribute access
in main_window's _on_post_activated. Used to prevent the OLD
video from reaching natural EOF during the new post's async
download.
stop_media()
Stops the video and clears the image viewer. Replaces 2 sites
of the popout._viewer.clear() + popout._video.stop() sequence
in blacklist-removal flow.
main_window.py call sites updated:
Line 1122-1130 (_on_post_activated):
popout._video._mpv.pause = True → popout.force_mpv_pause()
Line 1339-1342 (_update_fullscreen):
4 popout._*.setVisible(...) → popout.set_toolbar_visibility(...)
Line 1798, 1811, 2731:
popout._video.pause() → popout.pause_media()
Line 2151-2166 (_open_fullscreen_preview sync block):
sv = popout._video; sv.volume = ...; ...
+ manual seek-when-ready closure
→ popout.sync_video_state(...) + popout.connect_media_ready_once(...)
Line 2196-2207 (_on_fullscreen_closed reverse sync):
sv = popout._video; pv.volume = sv.volume; ...; popout._stack.currentIndex...
→ popout.get_video_state() returning a dict
Line 2393-2394, 2421-2423 (blacklist removal):
popout._viewer.clear() + popout._video.stop()
→ popout.stop_media()
After this commit, main_window has ZERO direct popout._underscore
accesses. The popout's public method surface is the only way for
main_window to interact with the popout's internals.
The popout's public method surface is now:
Lifecycle:
- set_media (existing — keeps the kind, info, width, height contract)
- update_state (existing — bookmarked/saved button labels)
- close (Qt builtin — triggers closeEvent)
Wiring:
- set_post_tags
- set_bookmark_folders_callback
- set_folders_callback
Privacy:
- privacy_hide / privacy_show (existing)
New in commit 15:
- is_video_active
- set_toolbar_visibility
- sync_video_state
- get_video_state
- seek_video_to
- connect_media_ready_once
- pause_media
- force_mpv_pause
- stop_media
Outbound signals (unchanged from refactor start):
- navigate / play_next_requested / closed
- bookmark_requested / bookmark_to_folder
- save_to_folder / unsave_requested
- blacklist_tag_requested / blacklist_post_requested
- privacy_requested
Tests passing after this commit: 81 / 81 (16 Phase A + 65 state).
Phase A still green.
Verification:
- Imports clean
- Pure-Python state machine + tests unchanged
- main_window's popout interaction goes through public methods only
Test cases for commit 16 (final shim cleanup):
- Drop the hyprland re-export shim methods from popout/window.py
- Have callers use popout.hyprland directly
**Commit 14b of the pre-emptive 14a/14b split.**
Adds the effect application path. The state machine becomes the
single source of truth for the popout's media transitions, navigation,
fullscreen toggle, and close lifecycle. The legacy imperative paths
that 14a left in place are removed where the dispatch+apply chain
now produces the same side effects.
Architectural shape:
Qt event → _fsm_dispatch(Event) → list[Effect] → _apply_effects()
↓
pattern-match by type
↓
calls existing private helpers
(_fit_to_content, _enter_fullscreen,
_video.play_file, etc.)
The state machine doesn't try to reach into Qt or mpv directly; it
returns descriptors and the adapter dispatches them to the existing
implementation methods. The private helpers stay in place as the
implementation; the state machine becomes their official caller.
What's fully authoritative via dispatch+apply:
- Navigate keys + wheel tilt → NavigateRequested → EmitNavigate
- F11 → FullscreenToggled → EnterFullscreen / ExitFullscreen
- Space → TogglePlayRequested → TogglePlay
- closeEvent → CloseRequested → StopMedia + EmitClosed
- set_media → ContentArrived → LoadImage|LoadVideo + FitWindowToContent
- mpv playback-restart → VideoStarted | SeekCompleted (state-aware)
- mpv eof-reached + Loop=Next → VideoEofReached → EmitPlayNextRequested
- mpv video-params → VideoSizeKnown → FitWindowToContent
What's deliberately no-op apply in 14b (state machine TRACKS but
doesn't drive):
- ApplyMute / ApplyVolume / ApplyLoopMode: legacy slot connections
on the popout's VideoPlayer still handle the user-facing toggles.
Pushing state.mute/volume/loop_mode would create a sync hazard
with the embedded preview's mute state, which main_window pushes
via direct attribute writes at popout open. The state machine
fields are still updated for the upcoming SyncFromEmbedded path
in a future commit; the apply handlers are intentionally empty.
- SeekVideoTo: the legacy `_ClickSeekSlider.clicked_position →
VideoPlayer._seek` connection still handles both the mpv.seek
call (now exact, per the 609066c drag-back fix) and the legacy
500ms `_seek_pending_until` pin window. Replacing this requires
modifying VideoPlayer._poll which is forbidden by the state
machine refactor's no-touch rule on media/video_player.py.
Removed duplicate legacy emits (would have caused real bugs):
- self.navigate.emit(±N) in eventFilter arrow keys + wheel tilt
→ EmitNavigate effect
- self.closed.emit() and self._video.stop() in closeEvent
→ StopMedia + EmitClosed effects
- self._video.play_next.connect(self.play_next_requested)
signal-to-signal forwarding → EmitPlayNextRequested effect
- self._enter_fullscreen() / self._exit_fullscreen() direct calls
→ EnterFullscreen / ExitFullscreen effects
- self._video._toggle_play() direct call → TogglePlay effect
- set_media body's load logic → LoadImage / LoadVideo effects
The Esc/Q handler now only calls self.close() and lets closeEvent
do the dispatch + apply. Two reasons:
1. Geometry persistence (FullscreenPreview._saved_geometry /
_saved_fullscreen) is adapter-side concern and must run BEFORE
self.closed is emitted, because main_window's
_on_fullscreen_closed handler reads those class fields. Saving
geometry inside closeEvent before dispatching CloseRequested
gets the order right.
2. The state machine sees the close exactly once. Two-paths
(Esc/Q → dispatch + close() → closeEvent → re-dispatch) would
require the dispatch entry's CLOSING-state guard to silently
absorb the second event — works but more confusing than just
having one dispatch site.
The closeEvent flow now is:
1. Save FullscreenPreview._saved_fullscreen and _saved_geometry
(adapter-side, before dispatch)
2. Remove the QApplication event filter
3. Dispatch CloseRequested → effects = [StopMedia, EmitClosed]
4. Apply effects → stop media, emit self.closed
5. super().closeEvent(event) → Qt window close
Verification:
- Phase A test suite (16 tests in tests/core/) still passes
- State machine tests (65 in tests/gui/popout/test_state.py) still pass
- Total: 81 / 81 automated tests green
- Imports clean
**The 11 manual scenarios are NOT verified by automated tests.**
The user must run the popout interactively and walk through each
scenario before this commit can be considered fully verified:
1. P↔L navigation cycles drift toward corner
2. Super+drag externally then nav
3. Corner-resize externally then nav
4. F11 same-aspect round-trip
5. F11 across-aspect round-trip
6. First-open from saved geometry
7. Restart persistence across app sessions
8. Rapid Right-arrow spam
9. Uncached video click
10. Mute toggle before mpv exists
11. Seek mid-playback (already verified by the 14a + drag-back-fix
sweep)
**If ANY scenario fails after this commit:** immediate `git revert
HEAD`, do not fix in place. The 14b apply layer is bounded enough
that any regression can be diagnosed by inspecting the apply handler
for the relevant effect type, but the in-place-fix temptation should
be resisted — bisect-safety requires a clean revert.
Test cases for commit 15:
- main_window.popout calls become method calls instead of direct
underscore access (open_post / sync_video_state / get_video_state /
set_toolbar_visibility)
- Method-cluster sweep from REFACTOR_INVENTORY.md still passes
The slider's _seek used plain 'absolute' (keyframe seek), which made
mpv land on the nearest keyframe at-or-before the click position.
For sparse-keyframe videos (1-5s GOP) the actual position landed
1-5s behind where the user clicked. The 500ms _seek_pending_until
pin window from c4061b0 papered over this for half a second, but
afterwards the slider visibly dragged back to mpv's keyframe-rounded
position and crawled forward. Observed in BOTH the embedded preview
and the popout slider — the bug lives in the shared VideoPlayer
class, so fixing it once fixes both surfaces.
Empirically verified by the pre-commit-1 mpv probe in
docs/POPOUT_REFACTOR_PLAN.md: seek(7.0, 'absolute') landed at
time_pos=5.000 (2s back); seek(12.0, 'absolute') landed at 10.033
(also 2s back). Both match the user's reported drag-back symptom.
The other seek paths in VideoPlayer already use exact mode:
- seek_to_ms (line 318): 'absolute+exact'
- _seek_relative (line 430): 'relative+exact'
The slider's _seek was the only outlier. The original c4061b0 commit
chose plain 'absolute' for "responsiveness" and added the pin window
to hide the keyframe rounding. This commit removes the underlying
cause: the seek now decodes from the previous keyframe forward to
the EXACT target position before mpv emits playback-restart, costing
~30-100ms more per seek depending on GOP density (well under the
500ms pin window) but landing time_pos at the click position
exactly. The slider doesn't need any pin window to mask a
discrepancy that no longer exists.
The _seek_pending_until pin remains in place as defense in depth —
it's now redundant for keyframe rounding but still smooths over the
sub-100ms decode latency between the click and the first _poll
tick that reads the new time_pos. Commit 14b will remove the legacy
pin code as part of the imperative-path cleanup.
This also unblocks the popout state machine's design (commits 6, 11,
14a). The state machine's SeekingVideo state lasts until mpv's
playback-restart event arrives — empirically 14-34ms in the user's
verification log of commit 14a. Without exact seek, commit 14b
would visibly REGRESS slider behavior because compute_slider_display_ms
returns mpv.time_pos after 30ms instead of the legacy 500ms — the
drag-back would surface immediately on every seek. With exact seek,
mpv.time_pos == seek_target_ms after the seek completes, so the
state machine's slider pin is correct without needing any extra
window.
Found during commit 14a verification gate. Pre-existing bug — the
state machine refactor revealed it but didn't introduce it.
Tests passing after this commit: 81 / 81 (16 Phase A + 65 state).
Phase A still green. Phase B regression target met (the dispatch
trace from the verification run shows correct SeekRequested →
SeekCompleted round-trips with no spurious state transitions).
Verification:
- Click slider mid-playback in embedded preview → no drag-back
- Click slider mid-playback in popout → no drag-back
- Drag the slider continuously → still works (isSliderDown path
unchanged)
- Period/Comma keys (relative seek) → still work (already use
'relative+exact')
Follow-up to commit 14a. The booru logger has only the in-app
QTextEdit LogHandler attached (main_window.py:436-440), so the
POPOUT_FSM dispatch trace from the state machine adapter only
reaches the Ctrl+L log panel — invisible from the shell.
Adds a stderr StreamHandler attached directly to the
`booru.popout.adapter` logger so:
python -m booru_viewer.main_gui 2>&1 | grep POPOUT_FSM
works during the commit-14a verification gate. The user can capture
the dispatch trace per scenario and compare it to the legacy path's
actions before commit 14b switches authority.
The handler is tagged with a `_is_popout_fsm_stderr` sentinel
attribute so re-imports of window.py don't stack duplicate
handlers (defensive — module-level code only runs once per process,
but the check costs nothing).
Format: `[HH:MM:SS.mmm] POPOUT_FSM <event> | <old> -> <new> | effects=[...]`
The millisecond precision matters for the seek scenario where the
race window is sub-100ms.
Propagation to the parent booru logger is left enabled, so dispatch
trace lines also continue to land in the in-app log panel for the
user who prefers Ctrl+L.
Tests still pass (81 / 81). No behavior change to widgets — this
only affects log output routing.
**Commit 14a of the pre-emptive 14a/14b split.**
Adds the popout's pure-Python state machine as a parallel side
channel to the legacy imperative event handling. The state machine
runs alongside the existing 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.**
This is the bisect-safe-by-construction split the refactor plan
called for. 197 lines added, 0 removed. No widget side effects from
the dispatch path. App is byte-identical from the user's perspective.
Wired wire-points (every Qt event the state machine cares about):
__init__:
- Constructs StateMachine, sets grid_cols
- Dispatches Open(saved_geo, saved_fullscreen, monitor) using
the class-level cross-popout-session state
- Connects VideoPlayer.playback_restart Signal (added in
commit 1) to _on_video_playback_restart, which routes to
VideoStarted (LoadingVideo) or SeekCompleted (SeekingVideo)
based on current state machine state
- Connects VideoPlayer.play_next → VideoEofReached dispatch
- Connects VideoPlayer.video_size → VideoSizeKnown dispatch
- Connects VideoPlayer._seek_slider.clicked_position → SeekRequested
- Connects VideoPlayer._mute_btn.clicked → MuteToggleRequested
- Connects VideoPlayer._vol_slider.valueChanged → VolumeSet
- Connects VideoPlayer._loop_btn.clicked → LoopModeSet
set_media:
- Detects MediaKind from is_video / .gif suffix
- Builds referer for streaming URLs
- Dispatches ContentArrived(path, info, kind, width, height, referer)
BEFORE the legacy imperative load path runs
eventFilter (key + wheel):
- Esc/Q → CloseRequested
- Left/H → NavigateRequested(-1)
- Right/L → NavigateRequested(+1)
- Up/K → NavigateRequested(-grid_cols)
- Down/J → NavigateRequested(+grid_cols)
- F11 → FullscreenToggled
- Space (video) → TogglePlayRequested
- Wheel horizontal tilt → NavigateRequested(±1)
- Wheel vertical (video) → VolumeSet(new_value)
- Period/Comma keys (relative seek) explicitly NOT dispatched —
they go straight to mpv via the legacy path. The state
machine's SeekRequested is for slider-driven seeks; commit 14b
will route the relative-seek keys through SeekRequested with
a target_ms computed from current position.
resizeEvent (non-Hyprland branch):
- WindowResized(rect) dispatched after the legacy viewport update
moveEvent (non-Hyprland branch):
- WindowMoved(rect) dispatched after the legacy viewport update
closeEvent:
- CloseRequested dispatched at entry
The _fsm_dispatch helper centralizes the dispatch + log path so every
wire-point is one line. Logs at DEBUG level via a new
`booru.popout.adapter` logger:
POPOUT_FSM <event_name> | <old_state> -> <new_state> | effects=[...]
Filter the log output by `POPOUT_FSM` substring to see only the
state machine activity during the manual sweep.
The _on_video_playback_restart helper is the ONE place the adapter
peeks at state machine state to choose between two event types
(VideoStarted vs SeekCompleted from the same mpv playback-restart
event). It's a read, not a write — the state machine's dispatch
remains the only mutation point.
Tests passing after this commit: 81 / 81 (16 Phase A + 65 state).
Phase A still green.
**Verification gate (next):**
Before commit 14b lands, the user runs the popout in their own
interactive Hyprland session and walks through the 11 race scenarios:
1. P↔L navigation cycles drift toward corner
2. Super+drag externally then nav
3. Corner-resize externally then nav
4. F11 same-aspect round-trip
5. F11 across-aspect round-trip
6. First-open from saved geometry
7. Restart persistence across app sessions
8. Rapid Right-arrow spam
9. Uncached video click
10. Mute toggle before mpv exists
11. Seek mid-playback
For each scenario, capture the POPOUT_FSM log lines and verify the
state machine's dispatch sequence matches what the legacy path
actually did. Any discrepancy is a state machine logic bug that
must be fixed in state.py BEFORE 14b lands and switches authority
to the dispatch path. Fix in state.py, not in window.py — state.py
is still the source of truth.
The bisect-safe property: even if the user finds a discrepancy
during the sweep, this commit DOES NOT change app behavior. App is
fully functional through the legacy path. The dispatch path is
diagnostic-only.
Test cases for commit 14b:
- Each effect type pattern-matches to a real widget action
- Manual 11-scenario sweep with the dispatch path authoritative
Pure refactor: moves the three Hyprland IPC helpers
(_hyprctl_get_window, _hyprctl_resize, _hyprctl_resize_and_move)
out of FullscreenPreview's class body and into a new sibling
hyprland.py module. The class methods become 1-line shims that
call the module functions, preserving byte-for-byte call-site
compatibility for the existing window.py code (_fit_to_content,
_enter_fullscreen, closeEvent all keep using self._hyprctl_*).
The module-level functions take the window title as a parameter
instead of reading it from self.windowTitle(), so they're cleanly
testable without a Qt instance.
Two reasons for the split:
1. **Architecture target.** docs/POPOUT_ARCHITECTURE.md calls for
popout/hyprland.py as a separate module so the upcoming Qt
adapter rewrite (commit 14) can call the helpers through a clean
import surface — no FullscreenPreview self-reference required.
2. **Single source of Hyprland IPC.** Both the legacy window.py
methods and (soon) the adapter's effect handler can call the same
functions. The state machine refactor's FitWindowToContent effect
resolves to a hyprland.resize_and_move call without going through
the legacy class methods.
The shims live in window.py for one commit only — commit 14's
adapter rewrite drops them in favor of direct calls to
popout.hyprland.* from the effect application path.
Files changed:
- NEW: booru_viewer/gui/popout/hyprland.py (~180 lines)
- MOD: booru_viewer/gui/popout/window.py (~120 lines removed,
~20 lines of shims added)
Tests passing after this commit: 81 / 81 (16 Phase A + 65 state).
Phase A still green.
Smoke test:
- FullscreenPreview class still imports cleanly
- All three _hyprctl_* shim methods present
- Shim source code references hyprland module
- App expected to launch without changes (popout open / fit / close
all go through the shims, which delegate to the module functions
with the same byte-for-byte semantics as the legacy methods)
Test cases for commit 14 (window.py adapter rewrite):
- Replace eventFilter imperative branches with dispatch calls
- Apply effects from dispatch returns to widgets
- Manual 11-scenario sweep
Pure refactor: moves the 14 effect dataclasses + the Effect union type
from `state.py` into a new sibling `effects.py` module. `state.py`
imports them at the top and re-exports them via `__all__`, so the
public API of `state.py` is unchanged — every existing import in the
test suite (and any future caller) keeps working without modification.
Two reasons for the split:
1. **Conceptual clarity.** State.py is the dispatch + transition
logic; effects.py is the data shapes the adapter consumes.
Splitting matches the architecture target in
docs/POPOUT_ARCHITECTURE.md and makes the effect surface
discoverable in one file.
2. **Import-purity gate stays in place for both modules.**
effects.py inherits the same hard constraint as state.py: no
PySide6, mpv, httpx, or any module that imports them. Verified
by running both modules through a meta_path import blocker that
refuses those packages — both import cleanly.
State.py still imports from effects.py via the standard
`from .effects import LoadImage, LoadVideo, ...` block. The dispatch
handlers continue to instantiate effect descriptors inline; only the
class definitions moved.
Files changed:
- NEW: booru_viewer/gui/popout/effects.py (~190 lines)
- MOD: booru_viewer/gui/popout/state.py (effect dataclasses
removed, import block added — net ~150 lines removed)
Tests passing after this commit: 65 / 65 (no change).
Phase A (16 tests in tests/core/) still green.
Test cases for commit 13 (hyprland.py extraction):
- import popout.hyprland and call helpers
- app launches with the shimmed window.py still using the helpers
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
Two final transition handlers complete the state machine surface:
ContentArrived(IMAGE | GIF) in any state →
DisplayingImage, [LoadImage(path, is_gif), FitWindowToContent(w, h)]
Same path as the video branch but routes to ImageViewer instead
of mpv. The is_gif flag tells the adapter which ImageViewer
method to call (set_gif vs set_image — current code at
popout/window.py:411-417).
CloseRequested from any non-Closing state →
Closing, [StopMedia, EmitClosed]
Closing is terminal. Every subsequent event returns [] regardless
of type (handled at the dispatch entry, which has been in place
since the skeleton). The adapter handles geometry persistence and
Qt cleanup outside the state machine — those are not popout
state machine concerns.
Tests passing after this commit: 62 / 62 (100%).
Newly green:
- test_awaiting_content_arrived_image_loads_and_transitions
- test_awaiting_content_arrived_gif_loads_as_animated
- test_displaying_image_content_replace_with_image
- test_close_from_each_state_transitions_to_closing[5 states]
Phase A (16 tests in tests/core/) still green.
State machine complete. The 6-state / 17-event / 14-effect design
is fully implemented:
States (6, budget ≤10):
AwaitingContent / DisplayingImage / LoadingVideo /
PlayingVideo / SeekingVideo / Closing
Events (17, budget ≤20):
Open / ContentArrived / NavigateRequested
VideoStarted / VideoEofReached / VideoSizeKnown
SeekRequested / SeekCompleted
MuteToggleRequested / VolumeSet / LoopModeSet / TogglePlayRequested
FullscreenToggled
WindowMoved / WindowResized / HyprlandDriftDetected
CloseRequested
Effects (14, budget ≤15):
LoadImage / LoadVideo / StopMedia
ApplyMute / ApplyVolume / ApplyLoopMode
SeekVideoTo / TogglePlay
FitWindowToContent / EnterFullscreen / ExitFullscreen
EmitNavigate / EmitPlayNextRequested / EmitClosed
Six race-fix invariants all enforced structurally — no timestamp
suppression windows in state.py, no guards, no fall-throughs:
1. EOF race: VideoEofReached only valid in PlayingVideo
2. Double-load: Navigate from media → AwaitingContent never
re-emits Load until ContentArrived
3. Persistent viewport: viewport is a state field, only mutated
by user-action events
4. F11 round-trip: pre_fullscreen_viewport snapshot/restore
5. Seek pin: SeekingVideo state + compute_slider_display_ms read
6. Pending mute: state.mute owned by machine, ApplyMute on
PlayingVideo entry
Test cases for commit 11 (illegal transition handler):
- dispatch invalid event in strict mode raises InvalidTransition
- dispatch invalid event in release mode returns [] (current behavior)
- BOORU_VIEWER_STRICT_STATE env var gates the raise
Three event handlers, all updating state fields and emitting the
corresponding Apply effect:
MuteToggleRequested:
Flip state.mute unconditionally — independent of which media state
we're in, independent of whether mpv exists. Emit ApplyMute. The
persistence-on-load mechanism in _on_video_started already replays
state.mute into the freshly-loaded video, so toggling mute before
any video is loaded survives the load cycle.
VolumeSet:
Set state.volume (clamped 0-100), emit ApplyVolume. Same
persistence-on-load behavior.
LoopModeSet:
Set state.loop_mode, emit ApplyLoopMode. Also affects what
happens at the next EOF (PlayingVideo + VideoEofReached branches
on state.loop_mode), so changing it during playback takes effect
on the next eof without any other state mutation.
This commit makes the 0a68182 pending mute fix structural at the
popout layer. The state machine owns mute / volume / loop_mode as
the source of truth. The current VideoPlayer._pending_mute field
stays as defense in depth — the state machine refactor's prompt
forbids touching media/video_player.py beyond the playback_restart
Signal addition. The popout layer no longer depends on the lazy
replay because the state machine emits ApplyMute on every
PlayingVideo entry.
All four persistent fields (mute, volume, loop_mode, viewport)
are now state machine fields with single-writer ownership through
dispatch().
Tests passing after this commit (62 total → 54 pass, 8 fail):
- test_state_field_mute_persists_across_video_loads
- test_state_field_volume_persists_across_video_loads
- test_state_field_loop_mode_persists
- test_invariant_pending_mute_replayed_into_video (RACE FIX!)
Phase A (16 tests) still green.
Tests still failing (8, scheduled for commit 10):
- DisplayingImage content arrived branch (commit 10)
- Closing transitions (commit 10)
- Open + first content with image kind (commit 10)
Test cases for commit 10 (DisplayingImage + Closing):
- ContentArrived(IMAGE) → DisplayingImage + LoadImage(is_gif=False)
- ContentArrived(GIF) → DisplayingImage + LoadImage(is_gif=True)
- DisplayingImage + ContentArrived(IMAGE) replaces media
- CloseRequested from each state → Closing + StopMedia + EmitClosed
Three event handlers, all updating state.viewport from rect data:
WindowMoved (Qt moveEvent, non-Hyprland only):
Move-only update — preserve existing long_side, recompute center.
Moves don't change size, so the viewport's "how big does the user
want it" intent stays put while its "where does the user want it"
intent updates.
WindowResized (Qt resizeEvent, non-Hyprland only):
Full rebuild — long_side becomes new max(w, h), center becomes
the rect center. Resizes change both intents.
HyprlandDriftDetected (adapter, fit-time hyprctl drift check):
Full rebuild from rect. This is the ONLY path that captures
Hyprland Super+drag — Wayland's xdg-toplevel doesn't expose
absolute window position to clients, so Qt's moveEvent never
fires for external compositor-driven movement. The adapter's
_derive_viewport_for_fit equivalent will dispatch this event when
it sees the current Hyprland rect drifting from the last
dispatched rect by more than _DRIFT_TOLERANCE.
All three handlers gate on (not fullscreen) and (not Closing).
Drifts and moves while in fullscreen aren't meaningful for the
windowed viewport.
This makes the 7d19555 persistent viewport structural. The
viewport is a state field. It's only mutated by WindowMoved /
WindowResized / HyprlandDriftDetected (user action) — never by
FitWindowToContent reading and writing back its own dispatch.
The drift accumulation that the legacy code's "recompute from
current state" shortcut suffered cannot happen here because there's
no read-then-write path; viewport is the source of truth, not
derived from current rect.
Tests passing after this commit (62 total → 50 pass, 12 fail):
- test_window_moved_updates_viewport_center_only
- test_window_resized_updates_viewport_long_side
- test_hyprland_drift_updates_viewport_from_rect
(The persistent-viewport-no-drift invariant test was already
passing because the previous transition handlers don't write to
viewport — the test was checking the absence of drift via the
absence of writes, which the skeleton already satisfied.)
Phase A (16 tests) still green.
Tests still failing (12, scheduled for commits 9-11):
- mute/volume/loop persistence events (commit 9)
- DisplayingImage content arrived branch (commit 10)
- Closing transitions (commit 10)
Test cases for commit 9 (mute/volume/loop persistence):
- MuteToggleRequested flips state.mute, emits ApplyMute
- VolumeSet sets state.volume, emits ApplyVolume
- LoopModeSet sets state.loop_mode, emits ApplyLoopMode
FullscreenToggled in any non-Closing state flips state.fullscreen.
Enter (fullscreen=False → True):
- Snapshot state.viewport into state.pre_fullscreen_viewport
- Emit EnterFullscreen effect (adapter calls self.showFullScreen())
Exit (fullscreen=True → False):
- Restore state.viewport from state.pre_fullscreen_viewport
- Clear state.pre_fullscreen_viewport
- Emit ExitFullscreen effect (adapter calls self.showNormal() then
defers a FitWindowToContent on the next event-loop tick — matching
the current QTimer.singleShot(0, ...) pattern)
This makes the 705e6c6 F11 round-trip viewport preservation
structural. The fix in the legacy code wrote the current Hyprland
window state into _viewport inside _enter_fullscreen so the F11
exit could restore it. The state machine version is equivalent: the
viewport snapshot at the moment of entering is the source of truth
for restoration. Whether the user got there via Super+drag (no Qt
moveEvent on Wayland), nav, or external resize, the snapshot
captures the viewport AS IT IS RIGHT NOW.
The interaction with HyprlandDriftDetected (commit 8): the adapter
will dispatch a HyprlandDriftDetected event before FullscreenToggled
during enter, so any drift between the last dispatched rect and
current Hyprland geometry is absorbed into viewport BEFORE the
snapshot. That's how the state machine handles the "user dragged
the popout, then immediately pressed F11" case.
Tests passing after this commit (62 total → 47 pass, 15 fail):
- test_invariant_f11_round_trip_restores_pre_fullscreen_viewport
Phase A (16 tests) still green.
Tests still failing (15, scheduled for commits 8-11):
- Persistent viewport / drift events (commit 8)
- mute/volume/loop persistence events (commit 9)
- DisplayingImage content arrived branch (commit 10)
- Closing transitions (commit 10)
Test cases for commit 8 (persistent viewport + drift events):
- WindowMoved updates viewport center, preserves long_side
- WindowResized updates viewport long_side from new max(w,h)
- HyprlandDriftDetected rebuilds viewport from rect
- Persistent viewport doesn't drift across navs (already passing)
PlayingVideo + SeekRequested → SeekingVideo, stash target_ms, emit
SeekVideoTo. SeekingVideo + SeekRequested replaces the target (user
clicked again, latest seek wins). SeekingVideo + SeekCompleted →
PlayingVideo.
The slider pin behavior is the read-path query
`compute_slider_display_ms(mpv_pos_ms)` already implemented at the
skeleton stage: while in SeekingVideo, returns `seek_target_ms`;
otherwise returns `mpv_pos_ms`. The Qt-side adapter's poll timer
asks the state machine for the slider display value on every tick
and writes whatever it gets back to the slider widget.
**This replaces 96a0a9d's 500ms _seek_pending_until timestamp window
at the popout layer.** The state machine has no concept of wall-clock
time. The SeekingVideo state lasts exactly until mpv signals the seek
is done, via the playback_restart Signal added in commit 1. The
adapter distinguishes load-restart from seek-restart by checking
the state machine's current state (LoadingVideo → VideoStarted;
SeekingVideo → SeekCompleted).
The pre-commit-1 probe verified that mpv emits playback-restart
exactly once per load and exactly once per seek (3 events for 1
load + 2 seeks), so the dispatch routing is unambiguous.
VideoPlayer's internal _seek_pending_until field stays in place as
defense in depth — the state machine refactor's prompt explicitly
forbids touching media/video_player.py beyond the playback_restart
Signal addition. The popout layer no longer depends on it.
Tests passing after this commit (62 total → 46 pass, 16 fail):
- test_playing_video_seek_requested_transitions_and_pins
- test_seeking_video_completed_returns_to_playing
- test_seeking_video_seek_requested_replaces_target
- test_invariant_seek_pin_uses_compute_slider_display_ms (RACE FIX!)
Phase A (16 tests) still green.
Tests still failing (16, scheduled for commits 7-11):
- F11 round-trip (commit 7)
- Persistent viewport / drift events (commit 8)
- mute/volume/loop persistence events (commit 9)
- DisplayingImage content arrived branch (commit 10)
- Closing transitions (commit 10)
Test cases for commit 7 (Fullscreen flag + F11 round-trip):
- dispatch FullscreenToggled in any media state, assert flag flipped
- F11 enter snapshots viewport into pre_fullscreen_viewport
- F11 exit restores viewport from pre_fullscreen_viewport
NavigateRequested in any media state (DisplayingImage / LoadingVideo /
PlayingVideo / SeekingVideo) transitions to AwaitingContent and emits
[StopMedia, EmitNavigate]. NavigateRequested in AwaitingContent itself
(rapid Right-arrow spam, second nav before main_window has delivered
the next post) emits EmitNavigate alone — no StopMedia, because
there's nothing to stop.
This is the structural fix for the double-load race that 31d02d3c
fixed upstream by removing the explicit _on_post_activated call after
_grid._select. The popout-layer fix is independent and stronger: even
if upstream signal chains misfire, the state machine never produces
two Load effects for the same navigation cycle. The state machine's
LoadVideo / LoadImage effects only fire from ContentArrived, and
ContentArrived is delivered exactly once per main_window-side post
activation.
The Open event handler also lands here. Stashes saved_geo,
saved_fullscreen, monitor on the state machine instance for the
first ContentArrived to consume. The actual viewport seeding from
saved_geo lives in commit 8 — this commit just stores the inputs.
Tests passing after this commit (62 total → 42 pass, 20 fail):
- test_awaiting_open_stashes_saved_geo
- test_awaiting_navigate_emits_navigate_only
- test_displaying_image_navigate_stops_and_emits
- test_loading_video_navigate_stops_and_emits
- test_playing_video_navigate_stops_and_emits
- test_seeking_video_navigate_stops_and_emits
- test_invariant_double_navigate_no_double_load (RACE FIX!)
Plus several illegal-transition cases for nav-from-now-valid-states.
Phase A (16 tests in tests/core/) still green.
Tests still failing (20, scheduled for commits 6-11):
- SeekingVideo entry/exit (commit 6)
- F11 round-trip (commit 7)
- Persistent viewport / drift events (commit 8)
- mute/volume/loop persistence events (commit 9)
- DisplayingImage content arrived branch (commit 10)
- Closing transitions (commit 10)
Test cases for commit 6 (SeekingVideo + slider pin):
- PlayingVideo + SeekRequested → SeekingVideo + SeekVideoTo effect
- SeekingVideo + SeekRequested replaces seek_target_ms
- SeekingVideo + SeekCompleted → PlayingVideo
- test_invariant_seek_pin_uses_compute_slider_display_ms
First batch of real transitions. The EOF race fix is the headline —
this commit replaces fda3b10b's 250ms _eof_ignore_until timestamp
window with a structural transition that drops VideoEofReached in
every state except PlayingVideo.
Transitions implemented:
ContentArrived(VIDEO) in any state →
LoadingVideo, [LoadVideo, FitWindowToContent]
Snapshots current_path/info/kind/width/height. Flips
is_first_content_load to False (the saved_geo seeding lands in
commit 8). Image and GIF kinds are still stubbed — they get
DisplayingImage in commit 10.
LoadingVideo + VideoStarted →
PlayingVideo, [ApplyMute, ApplyVolume, ApplyLoopMode]
The persistence effects fire on PlayingVideo entry, pushing the
state machine's persistent values into mpv. This is the
structural replacement for VideoPlayer._pending_mute's lazy-mpv
replay (the popout layer now owns mute as truth; VideoPlayer's
internal _pending_mute stays as defense in depth, untouched).
PlayingVideo + VideoEofReached →
Loop=NEXT: [EmitPlayNextRequested]
Loop=ONCE: [] (mpv keep_open=yes pauses naturally)
Loop=LOOP: [] (mpv loop-file=inf handles internally)
*Anything* + VideoEofReached (not in PlayingVideo) →
[], state unchanged
**THIS IS THE EOF RACE FIX.** The fda3b10b commit added a 250ms
timestamp window inside VideoPlayer to suppress eof events
arriving from a previous file's stop. The state machine subsumes
that by only accepting eof in PlayingVideo. In LoadingVideo
(where the race lives), VideoEofReached is structurally invalid
and gets dropped at the dispatch boundary. No window. No
timestamp. No race.
LoadingVideo / PlayingVideo + VideoSizeKnown →
[FitWindowToContent(w, h)]
mpv reports new dimensions; refit. Same effect for both states
because the only difference is "did the user see a frame yet"
(which doesn't matter for window sizing).
PlayingVideo + TogglePlayRequested →
[TogglePlay]
Space key / play button. Only valid in PlayingVideo — toggling
play during a load or seek would race with mpv's own state
machine.
Tests passing after this commit (62 total → 35 pass, 27 fail):
- test_loading_video_started_transitions_to_playing
- test_loading_video_eof_dropped (RACE FIX!)
- test_loading_video_size_known_emits_fit
- test_playing_video_eof_loop_next_emits_play_next
- test_playing_video_eof_loop_once_pauses
- test_playing_video_eof_loop_loop_no_op
- test_playing_video_size_known_refits
- test_playing_video_toggle_play_emits_toggle
- test_invariant_eof_race_loading_video_drops_stale_eof (RACE FIX!)
- test_awaiting_content_arrived_video_transitions_to_loading
- test_awaiting_content_arrived_video_emits_persistence_effects
Plus several illegal-transition cases for the (LoadingVideo, *)
events that this commit makes meaningfully invalid.
Phase A (16 tests in tests/core/) still green.
Tests still failing (27, scheduled for commits 5-11):
- Open / NavigateRequested handlers (commit 5)
- DisplayingImage transitions (commit 10)
- SeekingVideo transitions (commit 6)
- Closing transitions (commit 10)
- Persistent viewport / drift events (commit 8)
- mute/volume/loop persistence events (commit 9)
- F11 round-trip (commit 7)
Test cases for commit 5 (Navigating + AwaitingContent + double-load):
- dispatch NavigateRequested in PlayingVideo → AwaitingContent
- second NavigateRequested in AwaitingContent doesn't re-stop
- test_invariant_double_navigate_no_double_load
Lays down the full test surface for the popout state machine ahead of
any transition logic. 62 collected tests across the four categories
from docs/POPOUT_REFACTOR_PLAN.md "Test plan":
1. Read-path queries (4 tests, all passing at commit 3 — these
exercise the parts of the skeleton that are already real:
compute_slider_display_ms, the terminal Closing guard, the
initial state defaults)
2. Per-state transition tests (~22 tests, all failing at commit 3
because the per-event handlers in state.py are stubs returning
[]. Each documents the expected new state and effects for one
specific (state, event) pair. These pass progressively as
commits 4-11 land.)
3. Race-fix invariant tests (6 tests — one for each of the six
structural fixes from the prior fix sweep: EOF race, double-
navigate, persistent viewport, F11 round-trip, seek pin,
pending mute replay. The EOF race test already passes because
dropping VideoEofReached in LoadingVideo is just "stub returns
[]", which is the right behavior for now. The others fail
until their transitions land.)
4. Illegal transition tests (17 parametrized cases — at commit 11
these become BOORU_VIEWER_STRICT_STATE-gated raises. At commits
3-10 they pass trivially because the stubs return [], which is
the release-mode behavior.)
All 62 tests are pure Python:
- Import only `booru_viewer.gui.popout.state` and `popout.viewport`
- Construct StateMachine() directly
- Use direct field mutation (`m.state = State.PLAYING_VIDEO`) for
setup, dispatch the event under test, assert the new state +
returned effects
- No QApplication, no mpv, no httpx, no filesystem outside tmp_path
- Sub-100ms total runtime (currently 0.31s including test discovery)
The forcing function: if state.py grows a PySide6/mpv/httpx import,
this test file fails to collect and the suite breaks. That's the
guardrail that keeps state.py pure as transitions land.
Test count breakdown (62 total):
- 4 trivially-passing (read-path queries + initial state)
- 22 transition tests (one per (state, event) pair)
- 6 invariant tests (mapped to the six race fixes)
- 17 illegal transition cases (parametrized over (state, event) pairs)
- 5 close-from-each-state cases (parametrized)
- 8 misc (state field persistence, window events)
Result at commit 3:
35 failed, 27 passed in 0.31s
The 27 passing are exactly the predicted set: trivial reads + the
illegal-transition pass-throughs (which work today because the stubs
return [] just like release-mode strict-state would). The 35 failing
are the transition handlers that need real implementations.
Phase A test suite (16 tests in tests/core/) still passes — this
commit only adds new tests, no existing test changed.
Test cases for state machine implementation (commits 4-11):
- Each failing test is its own commit acceptance criterion
- Commit N "passes" when the relevant subset of tests turns green
- Final state machine sweep (commit 11): all 62 tests pass
Lays down the data shapes for the popout state machine ahead of any
transition logic. Pure Python — does not import PySide6, mpv, httpx,
subprocess, or any module that does. The Phase B test suite (commit 3)
will exercise this purity by importing it directly without standing
up a QApplication; the test suite is the forcing function that keeps
the file pure as transitions land in commits 4-11.
Module structure follows docs/POPOUT_ARCHITECTURE.md exactly.
States (6, target ≤10):
AwaitingContent — popout exists, no current media (initial OR mid-nav)
DisplayingImage — static image or GIF on screen
LoadingVideo — set_media called for video, awaiting first frame
PlayingVideo — video active (paused or playing)
SeekingVideo — user-initiated seek pending
Closing — closeEvent received, terminal
Events (17, target ≤20):
Open / ContentArrived / NavigateRequested
VideoStarted / VideoEofReached / VideoSizeKnown
SeekRequested / SeekCompleted
MuteToggleRequested / VolumeSet / LoopModeSet / TogglePlayRequested
FullscreenToggled
WindowMoved / WindowResized / HyprlandDriftDetected
CloseRequested
Effects (14, target ≤15):
LoadImage / LoadVideo / StopMedia
ApplyMute / ApplyVolume / ApplyLoopMode
SeekVideoTo / TogglePlay
FitWindowToContent / EnterFullscreen / ExitFullscreen
EmitNavigate / EmitPlayNextRequested / EmitClosed
Frozen dataclasses for events and effects, Enum for State / MediaKind /
LoopMode. Dispatch uses Python 3.10+ structural pattern matching to
route by event type.
StateMachine fields cover the full inventory:
- Lifecycle: state, is_first_content_load
- Persistent (orthogonal): fullscreen, mute, volume, loop_mode
- Geometry: viewport, pre_fullscreen_viewport, last_dispatched_rect
- Seek: seek_target_ms
- Content snapshot: current_path, current_info, current_kind,
current_width, current_height
- Open-event payload: saved_geo, saved_fullscreen, monitor
- Nav: grid_cols
Read-path query implemented even at the skeleton stage:
compute_slider_display_ms(mpv_pos_ms) returns seek_target_ms while
in SeekingVideo, mpv_pos_ms otherwise. This is the structural
replacement for the 500ms _seek_pending_until timestamp window —
no timestamp, just the SeekingVideo state.
Every per-event handler is a stub that returns []. Real transitions
land in commits 4-11 (priority order: PlayingVideo + LoadingVideo +
EOF race fix → Navigating + AwaitingContent + double-load fix →
SeekingVideo + slider pin → Fullscreen + F11 → viewport + drift →
mute/volume/loop persistence → DisplayingImage + Closing → illegal
transition handler).
Closing is treated as terminal at the dispatch entry — once we're
there, every event returns [] regardless of type. Same property the
current closeEvent has implicitly.
Verification:
- Phase A test suite (16 tests) still passes
- state.py imports cleanly with PySide6/mpv/httpx blocked at the
meta_path level (purity gate)
- StateMachine() constructs with all fields initialized to sensible
defaults
- Stub dispatch returns [] for every event type
- 6 states / 17 events / 14 effects all under budget (≤10/≤20/≤15)
Test cases for state machine tests (Prompt 3 commit 3):
- Construct StateMachine, assert initial state == AwaitingContent
- Assert is_first_content_load is True at construction
- Assert all stub dispatches return []
- Assert compute_slider_display_ms returns mpv_pos when not seeking
Adds a Qt Signal that mirrors mpv's `playback-restart` event. The
upcoming popout state machine refactor (Prompt 3) needs a clean,
event-driven "seek/load completed" edge to drive its SeekingVideo →
PlayingVideo and LoadingVideo → PlayingVideo transitions, replacing
the current 500ms timestamp suppression window in `_seek_pending_until`.
mpv's `playback-restart` fires once after each loadfile (when playback
actually starts producing frames) and once after each completed seek.
Empirically verified by the pre-commit-1 probe in
docs/POPOUT_REFACTOR_PLAN.md: a load + 2 seeks produces exactly 3
events, with `seeking=False` at every event (the event represents the
completion edge, not the in-progress state).
The state machine adapter will distinguish "load done" from "seek done"
by checking the state machine's current state at dispatch time:
- `playback-restart` while in LoadingVideo → VideoStarted event
- `playback-restart` while in SeekingVideo → SeekCompleted event
Implementation:
- One Signal added near the existing play_next / media_ready /
video_size definitions, with a doc comment explaining what fires
it and which state machine consumes it.
- One event_callback registration in `_ensure_mpv` (alongside the
existing observe_property calls). The callback runs on mpv's
event thread; emitting a Qt Signal is thread-safe and the
receiving slot runs on the GUI thread via Qt's default
AutoConnection (sender and receiver in the same thread by the
time the popout adapter wires the connection).
- The decorator-based `@self._mpv.event_callback(...)` form is used
to match the rest of the python-mpv idioms in the file. The inner
function name `_emit_playback_restart` is local-scoped — mpv keeps
its own reference, so there's no leak from re-creation across
popout open/close cycles (each popout gets a fresh VideoPlayer
with its own _ensure_mpv call).
This is the only commit in the popout state machine refactor that
touches `media/video_player.py`. All subsequent commits land in
`popout/` (state.py, effects.py, hyprland.py, window.py) or
`gui/main_window.py` interface updates. 21 lines added, 0 removed.
Verification:
- Phase A test suite (16 tests) still passes
- Module imports cleanly with the new Signal in place
- App launches without errors (smoke test)
Test cases for state machine adapter (Prompt 3 popout/state.py):
- VideoPlayer.playback_restart fires once on play_file completion
- VideoPlayer.playback_restart fires once per _seek call
First regression-test layer for booru-viewer. Pure Python — no Qt, no
mpv, no network, no real filesystem outside tmp_path. Locks in the
security and concurrency invariants from the 54ccc40 + eb58d76 hardening
commits ahead of the upcoming popout state machine refactor (Prompt 3),
which needs a stable baseline to refactor against.
16 tests across five files mirroring the source layout under
booru_viewer/core/:
- tests/core/test_db.py (4):
- _validate_folder_name rejection rules (.., /foo, \\foo, .hidden,
~user, empty) and acceptance categories (unicode, spaces, parens)
- add_bookmark INSERT OR IGNORE collision returns the existing row
id (locks in the lastrowid=0 fix)
- get_bookmarks LIKE escaping (literal cat_ear does not match catear)
- tests/core/test_cache.py (7):
- _referer_for hostname suffix matching (gelbooru.com / donmai.us
apex rewrite, both exact-match and subdomain)
- _referer_for rejects substring attackers
(imgblahgelbooru.attacker.com does NOT pick up the booru referer)
- ugoira frame-count and uncompressed-size caps refuse zip bombs
before any decompression
- _do_download MAX_DOWNLOAD_BYTES enforced both at the
Content-Length pre-check AND in the chunk-loop running total
- _is_valid_media returns True on OSError (no delete + redownload
loop on transient EBUSY)
- tests/core/test_config.py (2):
- saved_folder_dir rejects literal .. and ../escape
- find_library_files walks root + 1 level, filters by
MEDIA_EXTENSIONS, exact post-id stem match
- tests/core/test_concurrency.py (2):
- get_app_loop raises RuntimeError before set_app_loop is called
- run_on_app_loop round-trips a coroutine result from a worker
thread loop back to the test thread
- tests/core/api/test_base.py (1):
- BooruClient._shared_client lazy singleton constructor-once under
10-thread first-call race
Plus tests/conftest.py with fixtures: tmp_db, tmp_library,
reset_app_loop, reset_shared_clients. All fixtures use tmp_path or
reset module-level globals around the test so the suite is parallel-
safe.
pyproject.toml:
- New [project.optional-dependencies] test extra: pytest>=8.0,
pytest-asyncio>=0.23
- New [tool.pytest.ini_options]: asyncio_mode = "auto",
testpaths = ["tests"]
README.md:
- Linux install section gains "Run tests" with the
pip install -e ".[test]" + pytest tests/ invocation
Phase B (post-sweep VideoPlayer regression tests for the seek slider
pin, _pending_mute lazy replay, and volume replay) is deferred to
Prompt 3's state machine work — VideoPlayer cannot be instantiated
without QApplication and a real mpv, which is out of scope for a
unit test suite. Once the state machine carves the pure-Python state
out of VideoPlayer, those tests become trivial against the helper
module.
Suite runs in 0.07s (16 tests). Independent of Qt/mpv/network/ffmpeg.
Test cases for Prompt 3:
- (already covered) — this IS the test suite Prompt 3 builds on top of
The popout's VideoPlayer is constructed with no mpv attached — mpv
gets wired up in _ensure_mpv on the first set_media call. main_window's
_open_fullscreen_preview syncs preview→popout state right after the
popout is constructed, so it writes is_muted *before* mpv exists. The
old setter only forwarded to mpv if mpv was set:
@is_muted.setter
def is_muted(self, val: bool) -> None:
if self._mpv:
self._mpv.mute = val
self._mute_btn.setText("Unmute" if val else "Mute")
For the popout's pre-mpv VideoPlayer this updated the button text but
silently dropped the value. _ensure_mpv then created the mpv instance
later with default mute=False, so the popout always opened unmuted
even when the embedded preview was muted (or when a previous popout
session had muted and then closed).
Fix: introduce a Python-side _pending_mute field that survives the
lazy mpv creation. The setter writes to _pending_mute unconditionally
and forwards to mpv if it exists. The getter returns _mpv.mute when
mpv is set, otherwise _pending_mute. _ensure_mpv replays _pending_mute
into the freshly-created mpv instance after applying the volume from
the slider, mirroring the existing volume-from-slider replay pattern
that already worked because the slider widget exists from construction
and acts as the volume's persistent storage.
Also threaded _pending_mute through _toggle_mute so the click-driven
toggle path stays consistent with the setter path — without it, a
mute toggle inside the popout would update mpv but not _pending_mute,
and the next sync round-trip via the setter would clobber it.
Verified manually:
- popout video, click mute, close popout, reopen on same video →
mute persisted (button shows "Unmute", audio silent)
- toggle to unmute, close, reopen → unmuted persisted
- embedded preview video mute → close popout → state propagates
correctly via _on_fullscreen_closed's reverse sync
Note #3 in REFACTOR_NOTES.md (search result count + end-of-results
flag mismatch) reproduced once during the refactor verification sweep
and not again at later commits, so it's intermittent — likely
scenario-dependent (specific tag, blacklist hit rate, page-size /
limit interaction). The bug is real but not reliably repro-able, so
the right move is to add logging now and capture real data on the
next reproduction instead of guessing at a fix.
Both _do_search (paginated) and _on_reached_bottom (infinite scroll
backfill) now log a `do_search:` / `on_reached_bottom:` line with the
following fields:
- limit the configured page_size
- api_returned_total raw count of posts the API returned across all
fetched pages (sum of every batch the loop saw)
- kept post-filter, post-clamp count actually emitted
- drops_bl_tags posts dropped by the blacklist-tags filter
- drops_bl_posts posts dropped by the blacklist-posts filter
- drops_dedup posts dropped by the dedup-against-seen filter
- api_short_signal (do_search only) whether the LAST batch came
back smaller than limit — the implicit "API ran
out" hint
- api_exhausted (on_reached_bottom only) the explicit
api_exhausted flag the loop sets when len(batch)
falls short
- last_page (on_reached_bottom only) the highest page index
the backfill loop touched
_on_search_done also gets a one-liner with displayed_count, limit,
and the at_end decision so the user-visible "(end)" flag can be
correlated with the upstream numbers.
Implementation note: the per-filter drop counters live in a closure-
captured `drops` dict that the `_filter` closure mutates as it walks
its three passes (bl_tags → bl_posts → dedup). Same dict shape in
both `_do_search` and `_on_reached_bottom` so the two log lines are
directly comparable. Both async closures also accumulate `raw_total`
across the loop iterations to capture the API's true return count,
since the existing locals only kept the last batch's length.
All logging is `log.debug` so it's off at default INFO level. To
capture: bump booru_viewer logger level (or run with debug logging
enabled in main_window.py:440 — already DEBUG by default per the
existing setLevel call).
This commit DOES NOT fix#3 — the symptom is still intermittent and
the root cause is unknown. It just makes the next reproduction
diagnosable in one shot instead of requiring a second instrumented
run.
Two related improvements to the Ctrl+P privacy screen flow.
1. Resume video on un-hide
Pre-fix: Ctrl+P paused any playing video in the embedded preview and
the popout, but the second Ctrl+P only hid the privacy overlay — the
videos stayed paused. The user had to manually click Play to resume.
Fix: in _toggle_privacy's privacy-off branch, mirror the privacy-on
pause logic with resume() calls on the embedded preview's video player
and the popout's video. Unconditional resume — if the user manually
paused before Ctrl+P, the auto-resume on un-hide is a tiny annoyance,
but the common case (privacy hides → user comes back → video should
be playing again) wins.
2. Popout privacy uses an in-place overlay instead of hide()
Pre-fix attempt: privacy-on called self._fullscreen_window.hide() and
privacy-off called .show(). On Wayland (Hyprland) the hide→show round
trip drops the window's position because the compositor unmaps the
window on hide and remaps it at the default tile position on show.
A first attempt at restoring the position via a deferred
hyprctl_resize_and_move dispatch in privacy_show didn't take — by
the time the dispatch landed, the window had already been re-tiled
and the move was gated by `if not win.get("floating"): return`.
Cleaner fix: don't hide the popout window at all. FullscreenPreview
gains its own _privacy_overlay (a black QWidget child of central,
parallel to the existing toolbar / controls bar children) that
privacy_hide raises over the media stack. The popout window stays
mapped, position is preserved automatically because nothing moves,
and the overlay covers the content visually.
privacy_hide / privacy_show methods live in FullscreenPreview, not
in main_window — popout-internal state belongs to the popout module.
_toggle_privacy in main_window just calls them. This also makes
adding more popout-side privacy state later (e.g. fullscreen save)
a one-method change inside the popout class.
Also added a _popout_was_visible flag in BooruApp._toggle_privacy so
privacy-off only restores the popout if it was actually visible at
privacy-on time. Without the gate, privacy-off would inappropriately
re-show a popout the user had closed before triggering privacy.
Verified manually:
- popout open + drag to non-default pos + Ctrl+P + Ctrl+P → popout
still at the dragged position, content visible again
- popout open + video playing + Ctrl+P + Ctrl+P → video resumes
- popout closed + Ctrl+P + Ctrl+P → popout stays closed
- embedded preview video + Ctrl+P + Ctrl+P → resumes
- Ctrl+P with no video on screen → no errors
Pax requested the keyboard shortcut be removed — too easy to fat-finger
when navigating with the keyboard, and "Open in Default App" still
ships an external process that may steal focus from the app. The
right-click menu's Open in Default App action stays, both on browse
thumbnails and in the preview pane right-click — only the bare-key
shortcut goes away.
The deleted block was the only Key_O handler in BooruApp.keyPressEvent,
so no other behavior changes.
Verified manually:
- press O on a selected thumbnail → nothing happens
- right-click thumbnail → "Open in Default App" still present and opens
- right-click preview pane → same
Two related preservation bugs around the popout's F11 fullscreen
toggle, both surfaced during the post-refactor verification sweep.
1. ImageViewer zoom/pan loss on resize
ImageViewer.resizeEvent unconditionally called _fit_to_view() on every
resize event. F11 enter resizes the widget to the full screen, F11
exit resizes it back to the windowed size — both fired _fit_to_view,
clobbering any explicit user zoom and offset. Same problem for manual
window drags and splitter moves.
Fix: in resizeEvent, compute the previous-size fit-to-view zoom from
event.oldSize() and compare to current _zoom. Only re-fit if the user
was at fit-to-view at the previous size (within a 0.001 epsilon —
tighter than any wheel/key zoom step). Otherwise leave _zoom and
_offset alone.
The first-resize case (no valid oldSize, e.g. initial layout) still
defaults to fit, matching the original behavior for fresh widgets.
2. Popout window position lost on F11 round-trip
FullscreenPreview._enter_fullscreen captured _windowed_geometry but
the F11-exit restore goes through `_viewport` (the persistent center +
long_side that drives _fit_to_content). The drift detection in
_derive_viewport_for_fit only updates _viewport when
_last_dispatched_rect is set AND a fit is being computed — neither
path catches the "user dragged the popout with Super+drag and then
immediately pressed F11" sequence:
- Hyprland Super+drag does NOT fire Qt's moveEvent (xdg-toplevel
doesn't expose absolute screen position to clients on Wayland),
so Qt-side drift detection is dead on Hyprland.
- The Hyprland-side drift detection in _derive_viewport_for_fit
only fires inside a fit, and no fit is triggered between a drag
and F11.
- Result: _viewport still holds whatever it had before the drag —
typically the saved-from-last-session geometry seeded by the
first-fit one-shot at popout open.
When F11 exits, the deferred _fit_to_content reads the stale viewport
and restores the popout to the *previously seeded* position instead of
where the user actually had it.
Fix: in _enter_fullscreen, after capturing _windowed_geometry, also
write the current windowed state into self._viewport directly. The
viewport then holds the actual pre-fullscreen position regardless of
how it got there (drag, drag+nav, drag+F11, etc.), and F11 exit's
restore reads it correctly.
Bundled into one commit because both fixes are "F11 round-trip should
preserve where the user was" — the image fix preserves content state
(zoom/pan), the popout fix preserves window state (position). Same
theme, related root cause class. Bisecting one without the other
would be misleading.
Verified manually:
- image: scroll-zoom + drag pan + F11 + F11 → zoom and pan preserved
- image: untouched zoom + F11 + F11 → still fits to view
- image: scroll-zoom + manual window resize → zoom preserved
- popout: Super+drag to a new position + F11 + F11 → lands at the
dragged position, not at the saved-from-last-session position
- popout: same sequence on a video post → same result (videos don't
have zoom/pan, but the window-position fix applies to all media)
The seek slider snapped visually backward after a click for the first
~tens to hundreds of ms — long enough to be obvious. Race trace:
user clicks slider at target T
→ _ClickSeekSlider.mousePressEvent fires
→ setValue(T) lands the visual at the click position
→ clicked_position emits → _seek dispatches mpv.seek(T) async
→ mpv processes the seek on its own thread
meanwhile the 100ms _poll timer keeps firing
→ reads mpv.time_pos (still the OLD position, mpv hasn't caught up)
→ calls self._seek_slider.setValue(pos_ms)
→ slider visually snaps backward to the pre-seek position
→ mpv finishes seeking, next poll tick writes the new position
→ slider jumps forward to settle near T
The isSliderDown() guard at the existing setValue site (around line
425) only suppresses writebacks during a *drag* — fast clicks never
trigger isSliderDown, so the guard didn't help here.
Fix: pin the slider to the user's target throughout a 500ms post-seek
window. Mirror the existing _eof_ignore_until pattern (stale-eof
suppression in play_file → _on_eof_reached) — it's the same shape:
"after this dispatch, ignore poll-driven writebacks for N ms because
mpv hasn't caught up yet."
- _seek now records _seek_target_ms and arms _seek_pending_until
- _poll forces _seek_slider.setValue(_seek_target_ms) on every tick
inside the window, instead of mpv's lagging time_pos
- After the window expires, normal mpv-driven writes resume
Pin window is 500ms (vs the eof window's 250ms) because network and
streaming seeks take noticeably longer than local-cache seeks. Inside
the window the slider is forced to the target every tick, so mpv lag
is invisible no matter how long it takes within the window.
First attempt used a smaller 250ms window with a "close enough"
early-release condition (release suppression once mpv reports a
position within 250ms of the target). That still showed minor
track-back because the "close enough" threshold permitted writing
back a position slightly less than the target, producing a small
visible jump. The pin-to-target approach is robust against any
mpv interim position.
The time_label keeps updating to mpv's actual position throughout —
only the slider value is pinned, so the user can still see the
seek progressing in the time text.
Verified manually: clicks at start / middle / end of a video slider
all hold position cleanly. Drag still works (the isSliderDown path
is untouched). Normal playback advances smoothly (the pin window
only affects the post-seek window, not steady-state playback).
Two related fixes for the File → Batch Download Page (Ctrl+D) flow.
1. Saved-dot refresh
Pre-fix: when the user picked a destination inside the library, the
batch wrote files to disk but the browse grid's saved-dots stayed dark
until the next refresh. The grid was lying about local state.
Fix: stash the chosen destination as self._batch_dest at the dispatch
site, then in _on_batch_progress (which already fires per-file via
the existing batch_progress signal) check whether dest is inside
saved_dir(); if so, find the just-finished post in self._posts by id
and light its grid thumb's saved-locally dot. Dots appear incrementally
as each file lands, not all at once at the end.
The batch_progress signal grew a third int param (post_id of the
just-finished item). It's a single-consumer signal — only
_on_batch_progress connects to it — so the shape change is local.
Both batch download paths (the file menu's _batch_download and the
multi-select menu's _batch_download_posts) pass post.id through.
When the destination is OUTSIDE the library, dots stay dark — the
saved-dot means "in library", not "downloaded somewhere". The check
uses Path.is_relative_to (Python 3.11+).
self._batch_dest is cleared in _on_batch_done after the batch finishes
so a subsequent non-batch save doesn't accidentally see a stale dest.
2. Tab gating
Pre-fix: File → Batch Download Page... was enabled on Bookmarks and
Library tabs, where it makes no sense (those tabs already show local
files). Ctrl+D fired regardless of active tab.
Fix: store the QAction as self._batch_action instead of a local var
in _setup_menu, then toggle setEnabled(index == 0) from _switch_view.
Disabling the QAction also disables its keyboard shortcut, so Ctrl+D
becomes a no-op on non-browse tabs without a separate guard.
Verified manually:
- Browse tab → menu enabled, Ctrl+D works
- Bookmarks/Library tabs → menu grayed out, Ctrl+D no-op
- Batch dl into ~/.local/share/booru-viewer/saved → dots light up
one-by-one as files land
- Batch dl into /tmp → files written, dots stay dark
The infinite-scroll backfill loop in _on_reached_bottom accumulates
results from up to 9 follow-up API pages until len(collected) >= limit,
but the break condition is >= not ==, so the very last full batch
would push collected past the configured page_size. The non-infinite
search path in _do_search already slices collected[:limit] before
emitting search_done at line 805 — the infinite path was emitting the
unclamped list. Result: a single backfill round occasionally appended
more than page_size posts, producing irregular batch sizes the user
could see.
Fix: one-character change at the search_append.emit call site to mirror
the non-infinite path's slice.
Why collected[:limit] over the alternative break-early-with-clamp:
1. Consistency — the non-infinite path in _do_search already does
the same slice before emit. One pattern, both branches.
2. Trivially fewer lines than restructuring the loop break.
3. The slight wasted download work (the over-fetched final batch is
already on disk by the time we slice) is acceptable. It's at most
one extra page's worth, only happens at the boundary, only on
infinite scroll, and the next backfill round picks up from where
the visible slice ends — nothing is *lost*, just briefly redundant.
Verified manually on a high-volume tag with infinite scroll enabled
and page_size=40: pre-fix appended >40 posts in one round, post-fix
appended exactly 40.
The browse grid's multi-select right-click menu collapsed library and
bookmark actions into a single "Remove All Bookmarks" entry that did
*both* — it called delete_from_library and remove_bookmark per post,
and was unconditionally visible regardless of selection state. Two
problems:
1. There was no way to bulk-unsave files from the library without
also stripping the bookmarks. Saved-but-not-bookmarked posts had
no bulk-unsave path at all.
2. The single misleadingly-named action didn't match the single-post
right-click menu's clean separation of "Save to Library / Unsave
from Library" vs. "Bookmark as / Remove Bookmark".
Reshape: split into four distinct actions, each with symmetric
conditional visibility:
- Save All to Library → shown only if any post is unsaved
- Unsave All from Library → shown only if any post is saved (NEW)
- Bookmark All → shown only if any post is unbookmarked
- Remove All Bookmarks → shown only if any post is bookmarked
Mixed selections show whichever subset of the four is relevant. The
new Unsave All from Library calls a new _bulk_unsave method that
mirrors the _bulk_save shape but synchronously (delete_from_library
is a filesystem op, no httpx round-trip). Remove All Bookmarks now
*only* removes bookmarks — it no longer touches the library, matching
the single-post Remove Bookmark action's scope.
Always-shown actions (Download All, Copy All URLs) stay below a
separator at the bottom.
Verified:
- Multi-select unbookmarked+unsaved posts → only Save All / Bookmark All
- Multi-select saved-not-bookmarked → only Unsave All / Bookmark All
- Multi-select bookmarked+saved → only Unsave All / Remove All Bookmarks
- Mixed selection → all four appear
- Unsave All from Library removes files, leaves bookmarks
- Remove All Bookmarks removes bookmarks, leaves files
Final commit of the gui/app.py + gui/preview.py structural refactor.
Updates the four call sites that were importing through the
preview.py / app.py shims to import from each entity's canonical
sibling module instead, then deletes the now-empty shim files.
Edits:
- main_gui.py:38 from booru_viewer.gui.app import run
→ from booru_viewer.gui.app_runtime import run
- main_window.py:44 from .preview import ImagePreview
→ from .preview_pane import ImagePreview
- main_window.py:1133 from .preview import VIDEO_EXTENSIONS
→ from .media.constants import VIDEO_EXTENSIONS
- main_window.py:2061 from .preview import FullscreenPreview
→ from .popout.window import FullscreenPreview
- main_window.py:2135 from .preview import FullscreenPreview
→ from .popout.window import FullscreenPreview
Deleted:
- booru_viewer/gui/app.py
- booru_viewer/gui/preview.py
Final gui/ tree:
gui/
__init__.py (unchanged, empty)
app_runtime.py entry point + style loader
main_window.py BooruApp QMainWindow
preview_pane.py ImagePreview embedded preview
info_panel.py InfoPanel widget
log_handler.py LogHandler (Qt-aware logger adapter)
async_signals.py AsyncSignals signal hub
search_state.py SearchState dataclass
media/
__init__.py
constants.py VIDEO_EXTENSIONS, _is_video
image_viewer.py ImageViewer (zoom/pan)
mpv_gl.py _MpvGLWidget, _MpvOpenGLSurface
video_player.py VideoPlayer + _ClickSeekSlider
popout/
__init__.py
viewport.py Viewport NamedTuple, _DRIFT_TOLERANCE
window.py FullscreenPreview popout window
grid.py, bookmarks.py, library.py, search.py, sites.py,
settings.py, dialogs.py (all untouched)
Net result for the refactor: 2 god-files (app.py 3608 lines +
preview.py 2273 lines = 5881 lines mixing every concern) replaced
by 12 small clean modules + 2 oversize-by-design god-class files
(main_window.py and popout/window.py — see docs/REFACTOR_PLAN.md
for the indivisible-class rationale).
Followups discovered during execution are recorded in
docs/REFACTOR_NOTES.md (gitignored, local-only).
Step 13 of the gui/app.py + gui/preview.py structural refactor —
final move out of app.py. The four entry-point helpers move together
because they're a tightly-coupled cluster: run() calls all three of
the others (_apply_windows_dark_mode, _load_user_qss,
_BASE_POPOUT_OVERLAY_QSS). Splitting them across commits would just
add bookkeeping overhead with no bisect benefit.
app_runtime.py imports BooruApp from main_window for run()'s
instantiation site, plus Qt at module level (the nested
_DarkArrowStyle class inside run() needs Qt.PenStyle.NoPen at call
time). Otherwise the four helpers are byte-identical to their
app.py originals.
After this commit app.py is just the original imports header + log
+ the shim block — every entity that used to live in it now lives
in its canonical module. main_gui.py still imports from
booru_viewer.gui.app via the shim (`from .app_runtime import run`
re-exports it). Commit 14 swaps main_gui.py to the canonical path
and deletes app.py.
Step 12 of the gui/app.py + gui/preview.py structural refactor — the
biggest single move out of app.py. The entire ~3020-line BooruApp
QMainWindow class moves to its own module under gui/. The class body
is byte-identical: every method, every signal connection, every
private attribute access stays exactly as it was.
main_window.py imports the helper classes that already moved out of
app.py (SearchState, LogHandler, AsyncSignals, InfoPanel) directly
from their canonical sibling modules at the top of the file, so the
bare-name lookups inside BooruApp method bodies (`SearchState(...)`,
`LogHandler(self._log_text)`, `AsyncSignals()`, `InfoPanel()`) keep
resolving to the same class objects. Same package depth as app.py
was, so no relative-import depth adjustment is needed for any of
the lazy `..core.X` or `.preview` imports inside method bodies —
they keep working through the preview.py shim until commit 14
swaps them to canonical paths.
app.py grows the BooruApp re-export shim line. After this commit
app.py is just imports + log + the four helpers (run,
_apply_windows_dark_mode, _load_user_qss, _BASE_POPOUT_OVERLAY_QSS)
+ the shim block. Commit 13 carves the helpers out, commit 14
deletes the shims and the file.
VERIFICATION: full method-cluster sweep (see docs/REFACTOR_PLAN.md
"Commit 12 expanded verification" section), not the 7-item smoke test.
Step 11 of the gui/app.py + gui/preview.py structural refactor. Pure
copy: the toggleable info panel widget with category-coloured tag
list moves to its own module. The new module gets its own
`log = logging.getLogger("booru")` at module level — same logger
instance the rest of the app uses (logging.getLogger is idempotent
by name), matching the existing per-module convention used by
grid.py / bookmarks.py / library.py. All six tag-color Qt Properties
preserved verbatim. app.py grows another shim line. Shim removed
in commit 14.
Step 10 of the gui/app.py + gui/preview.py structural refactor. Pure
copy: the QObject signal hub that BooruApp uses to marshal async
worker results back to the GUI thread moves to its own module. All
14 signals are preserved verbatim. app.py grows another shim line
so internal `AsyncSignals()` references in BooruApp keep working.
Shim removed in commit 14.
Step 9 of the gui/app.py + gui/preview.py structural refactor. Pure
copy: the Qt-aware logging.Handler that bridges the booru logger to
the in-app QTextEdit log panel moves to its own module. app.py grows
another shim line so any internal `LogHandler(...)` reference (the
single one in BooruApp._setup_ui) keeps resolving through the module
namespace. Shim removed in commit 14.
Step 8 of the gui/app.py + gui/preview.py structural refactor — first
move out of app.py. Pure copy: the SearchState dataclass moves to its
own module. app.py grows its first re-export shim block at the bottom
so any internal `SearchState(...)` reference in BooruApp keeps working
through the module-namespace lookup. Shim removed in commit 14.
Step 7 of the gui/app.py + gui/preview.py structural refactor. Pure
move: the embedded preview pane class (the one that lives in the
right column of the main window and combines image+video+toolbar)
is now in its own module. preview_pane.py is at the same package
depth as preview.py was, so no relative-import depth adjustment is
needed inside the class body.
preview.py grows the final preview-side re-export shim line. After
this commit preview.py is just the original imports + _log + shim
block — every class that used to live in it now lives in its
canonical module under media/ or popout/ or as preview_pane. The
file gets deleted entirely in commit 14.
Step 6 of the gui/app.py + gui/preview.py structural refactor — the
biggest single move in the sequence. The entire 1046-line popout
window class moves to its own module under popout/, alongside the
viewport NamedTuple it depends on. The popout overlay styling
documentation comment that lived above the class moves with it
since it's about the popout, not about ImagePreview.
Address-only adjustment: the lazy `from ..core.config import` lines
inside `_hyprctl_resize` and `_hyprctl_resize_and_move` become
`from ...core.config import` because the new module sits one package
level deeper. Same target module, different relative-import depth —
no behavior change.
preview.py grows another re-export shim so app.py's two lazy
`from .preview import FullscreenPreview` call sites (in
_open_fullscreen_preview and _on_fullscreen_closed) keep working
unchanged. Shim removed in commit 14, where the call sites move
to the canonical `from .popout.window import FullscreenPreview`.
Step 5 of the gui/app.py + gui/preview.py structural refactor. Moves
the click-to-seek QSlider variant and the mpv-backed transport-control
widget into their own module under media/. The new module imports
_MpvGLWidget from .mpv_gl (sibling) instead of relying on the bare
name in the old preview.py namespace.
Address-only adjustment: the lazy `from ..core.cache import _referer_for`
inside `play_file` becomes `from ...core.cache import _referer_for`
because the new module sits one package level deeper. Same target
module, different relative-import depth — no behavior change.
preview.py grows another re-export shim line so ImagePreview (still
in preview.py) and FullscreenPreview can keep constructing
VideoPlayer unchanged. Shim removed in commit 14.
