Library thumbnails are saved by post_id (_copy_library_thumb uses
f"{post.id}.jpg") but the library viewer looked them up by file
stem (f"{filepath.stem}.jpg"). For digit-stem files (12345.jpg)
these are the same. For templated files (artist_12345.jpg) the
stem is "artist_12345" which doesn't match the thumbnail named
"12345.jpg" — wrong or missing thumbnails.
Fix: resolve post_id from the filename via
get_library_post_id_by_filename, then look up the thumbnail as
f"{post_id}.jpg". Generated thumbnails (for files without a
cached browse thumbnail) also store by post_id now, so
everything stays consistent.
The tag search filter in refresh() used f.stem.isdigit() to
extract post_id — templated filenames like artist_12345.jpg
failed the check and got filtered out even when their post_id
matched the search query.
Fix: look up post_id via db.get_library_post_id_by_filename
first (handles templated filenames), fall back to int(stem) for
legacy digit-stem files. Same pattern as the delete and saved-dot
fixes from earlier in this refactor.
Logs when GL render context is actually initialized (not on the no-op
path). Confirms GL init fires once per widget lifetime, not on every
video click. Kept permanently for future debugging.
behavior change: when streaming=True (uncached video handed directly to
mpv), _load now early-returns instead of running download_image in
parallel. mpv's stream-record option (added in the previous commit)
handles cache population, so the parallel httpx download was a second
TCP+TLS connection to the same CDN URL contending with mpv for
bandwidth. Single connection per uncached video after this commit.
Replaces the parallel httpx download with mpv's stream-record per-file
option. When play_file receives an HTTP URL, it passes stream_record
pointing at a .part temp file alongside the URL. mpv writes the incoming
network stream to disk as it decodes, so a single HTTP connection serves
both playback and cache population.
On clean EOF the .part is promoted to the real cache path via os.replace.
Seeks invalidate the recording (mpv may skip byte ranges), so
_seeked_during_record flags it for discard. stop() and rapid-click
cleanup also discard incomplete .part files.
At this commit both pipelines are active — _load still runs the httpx
download in parallel. Whichever finishes second wins os.replace. The
next commit removes the httpx path.
behavior change: mpv now uses explicit cache=yes, cache_pause=no
(stutter over pause for short clips), 50MiB demuxer buffer cap,
20s read-ahead, and 10s network timeout (down from ~60s default).
Improves first-frame latency on uncached video streams and surfaces
stalled-connection errors faster.
behavior change: _apply_load_video now switches the stack to the video
surface BEFORE calling play_file so mpv's first frame lands on a visible
widget instead of a cleared image viewer. Removes the redundant stop()
call — loadfile("replace") atomically replaces the current file.
Also fixes video position not surviving popout close: StopMedia (part of
CloseRequested effects) destroyed mpv's time_pos before get_video_state
could read it. Now closeEvent snapshots position_ms before dispatching
CloseRequested, and get_video_state returns the snapshot.
behavior change: _on_video_stream no longer calls stop() on the
embedded preview's mpv when the popout is the visible target. The
embedded preview is hidden and idle — the synchronous command('stop')
round-trip was wasting ~50-100ms on the click-to-first-frame critical
path with no visible benefit. loadfile("replace") in the popout's
play_file handles the media swap atomically.
Library items' Post objects were constructed without width/height
(library_meta doesn't store them), so the popout got 0/0 and
_fit_to_content returned early without setting keep_aspect_ratio.
Videos were unaffected because mpv reports dimensions later via
VideoSizeKnown. Images had no second chance — the aspect lock
was never set, and manual window resizing stretched them freely.
Fix: new _image_dimensions(path) reads the actual pixel size from
the file via QPixmap before constructing the Post. The Post now
carries real width/height. _update_fullscreen moved to run AFTER
Post construction so cp.width/cp.height are populated when the
popout reads them for pre-fit + aspect lock.
Not a regression from the templates refactor — pre-existing gap
in the library display path.
The flag was set in _ensure_post_categories_async which runs AFTER
_on_post_selected calls info_panel.set_post. By the time the flag
was True, the flat tags had already rendered. The flash persisted.
Fix: check whether a fetch is needed and set the flag in
_on_post_selected, right before set_post. The info panel sees the
flag and skips the flat-tag fallback on its first render.
When a category fetch is about to fire (Rule34/Safebooru.org/
Moebooru on first click), the info panel was rendering the full
flat tag list, then ~200ms later re-rendering with categorized
tags. The re-layout from flat→categorized looked like a visual
hitch.
Fix: new _categories_pending flag on InfoPanel. When True, the
flat-tag fallback branch is skipped — the tags area stays empty
until categories arrive and render in one clean pass.
_ensure_post_categories_async sets _categories_pending = True
before scheduling the fetch (or False if no fetcher = Danbooru)
_on_categories_updated clears _categories_pending = False
Visual result:
Danbooru/e621: instant (inline, no flag)
Gelbooru with auth: instant (background prefetch beat the click)
Rule34/SB.org/Moebooru: empty ~200ms → categories appear cleanly
(no flat→categorized re-layout)
The Library tab's single-delete and multi-delete context menu
actions called .unlink() directly, bypassing delete_from_library
entirely. They only extracted post_id from digit-stem filenames
(int(stem) if stem.isdigit()), so templated files like
artist_12345.jpg got deleted from disk but left orphan
library_meta rows that made get_saved_post_ids lie forever.
Fix: resolve post_id via db.get_library_post_id_by_filename first
(handles templated filenames), fall back to int(stem) for legacy
digit-stem files, then call db.remove_library_meta(post_id) after
unlinking. Both single-delete and multi-delete paths are fixed.
This was the last source of orphan library_meta rows. With this
fix + the earlier delete_from_library cleanup, every deletion
path in the app now cleans up its meta row:
- Library tab single delete (this commit)
- Library tab multi delete (this commit)
- Browse/preview "Unsave from Library" (via delete_from_library)
- Browse multi-select "Unsave All" (via delete_from_library)
- Bookmarks "Unsave from Library" (via delete_from_library)
- Bookmarks multi-select "Unsave All" (via delete_from_library)
The primary search result handler (_on_search_done) was still using
the old filesystem walk + stem.isdigit() filter to build the saved-
post-id set. The two other call sites (_on_load_more and the
blacklist rebuild) were fixed in the earlier saved-dot sweep but
this one was missed. Templated filenames like artist_12345.jpg
were invisible, so the saved-dot disappeared after any grid
rebuild (new search, page change, etc).
Fix: use self._db.get_saved_post_ids() (one indexed SELECT,
format-agnostic) like the other two sites already do. Also drops
the saved_dir import that was only needed for the filesystem walk.
Two places checked `if post.tag_categories: return` before doing
a full cache-coverage check, causing posts with partial cache
composes (e.g. 5/40 tags from the background prefetch) to get
stuck at low coverage forever:
ensure_categories: removed the post.tag_categories early exit.
Now ALWAYS runs try_compose_from_cache first. Only the 100%
coverage return (True) is trusted as "done." Partial composes
return False and fall through to the fetch path.
_ensure_post_categories_async: removed the post.tag_categories
guard. Danbooru/e621 are filtered by the client.category_fetcher
is None check instead (they categorize inline, no fetcher).
For Gelbooru-style sites, always schedules ensure_categories
regardless of current post state.
Root cause: the partial-compose fix (try_compose_from_cache
populates tag_categories even when cache coverage is <100%)
conflicted with the early-exit guards that assumed non-empty
tag_categories = fully categorized. Now the only "fully done"
signal is try_compose_from_cache returning True (100% coverage).
Two bookmark save sites updated for save_post_file's sync→async
signature change:
_save_bookmark_to_library: wraps the save in an async closure
and schedules via run_on_app_loop (already imported for the
thumbnail download path). Fire-and-forget; the source file is
already cached so the save is near-instant.
Save As action: same async wrapper pattern. The dialog runs
synchronously (user picks destination), then the actual file
copy is scheduled on the async loop.
Neither site passes a category_fetcher — bookmarks don't have a
direct reference to the active BooruClient. The save flow's
ensure_categories check in library_save.py short-circuits (the
fetcher is None), so template rendering uses whatever categories
are already on the post object. For bookmark→library saves, the
user typically hasn't clicked the post in the browse grid, so
categories may be empty — the template falls back to %id% for
category tokens, same as before. Full categorization on the
bookmark save path is a future enhancement (would require passing
the client through from main_window).
Four save call sites updated to await save_post_file (now async)
and pass category_fetcher so the template-render ensure check can
fire when needed.
_bulk_save: creates fetcher once at the top of the async closure,
shared across all posts in the batch. Probe state persists
within the batch.
_save_to_library: creates fetcher per invocation (single post).
_save_as: wrapped in an async closure (was sync before) since
save_post_file is now async. Uses bookmark_done/error signals
for status instead of direct showMessage.
_batch_download_to: creates fetcher once at the top, shared
across the batch.
New _get_category_fetcher helper returns the fetcher from a fresh
client (lightweight — shares the global httpx pool) or None if no
site is active.
Three changes:
1. _make_client passes db=self._db, site_id=s.id so Gelbooru and
Moebooru clients get a CategoryFetcher attached via the factory.
2. _on_post_activated calls _ensure_post_categories_async(post)
after setting up the preview. If the post has empty categories
(background prefetch hasn't reached it yet, or cache miss),
this schedules ensure_categories on the async loop. When it
completes, it emits categories_updated via the Qt signal.
3. _on_categories_updated slot re-renders the info panel and
preview pane tag display when the currently-selected post's
categories arrive. Stale updates (user clicked a different post
before the fill completed) are silently dropped by the post.id
check.
Two more digit-stem-only callsites I missed in the saved-dot fix
sweep. _set_library_info and _show_library_post both did
'if not stem.isdigit(): return' before consulting library_meta or
building the toolbar Post. Templated files (post-template-refactor
saves like 12345_hatsune_miku.jpg) bailed out silently — clicking
one in the Library tab left the info panel showing the previous
selection's data and the toolbar actions did nothing.
Extracted a small helper _post_id_from_library_path that resolves
either layout: look up library_meta.filename first (templated),
fall back to int(stem) for legacy digit-stem files. Both call sites
go through the helper now.
Same pattern as the find_library_files / _is_post_in_library
fixes from the earlier saved-dot bug. With this commit there are
no remaining "is templated file in the library?" callsites that
fall back to digit-stem matching alone — every check is
format-agnostic via the DB.
The browse grid had the same digit-stem-only bug as the bookmark
grid: _saved_ids in two places used a root-only iterdir + isdigit
filter, missing both subfolder saves and templated filenames. The
user only reported the bookmark side, but this side has been
silently broken for any save into a subfolder for a while.
Six changes, all driven by the new db-backed helpers:
_on_load_more (browse grid append):
_saved_ids = self._db.get_saved_post_ids()
After-blacklist rebuild:
_saved_ids = self._db.get_saved_post_ids()
_is_post_saved:
return self._db.is_post_in_library(post_id)
Bookmark preview lookup find_library_files:
pass db=self._db so templated names also match
_unsave_from_preview delete_from_library:
pass db=self._db so templated names get unlinked AND meta cleaned
_bulk_unsave delete_from_library:
same fix
The dot on bookmark thumbnails uses set_saved_locally(...) and was
driven by find_library_files(post_id) — a digit-stem filesystem
walk that silently failed for any save with a templated filename
(e.g. 12345_hatsune_miku.jpg). The user reported it broken right
after templating landed.
Switch to db.get_saved_post_ids() for the grid refresh: one indexed
SELECT, set membership in O(1) per thumb. Format-agnostic, sees
both digit-stem and templated saves.
The "Unsave from Library" context menu used the same broken
find_library_files check for visibility. Switched to
db.is_post_in_library(post_id), which is the same idea via a
single-row SELECT 1.
Both delete_from_library call sites (single + bulk Unsave All)
now pass db so templated filenames are matched and the meta row
gets cleaned up. Refresh always runs after Unsave so the dot
clears whether the file was on disk or just an orphan meta row.
Sixth and final Phase 2 site migration. The bookmarks context-menu
Save As action now mirrors main_window._save_as: render the template
to populate the dialog default name, then route the actual save
through save_post_file with explicit_name set to whatever the user
typed. Same behavior change as the browse-side Save As — Save As
into saved_dir() now registers library_meta where v0.2.3 didn't.
After this commit the eight save sites in main_window.py and
bookmarks.py all share one implementation. The net diff of Phase 1 +
Phase 2 (excluding the Phase 0 scaffolding) is a deletion in
main_window.py + bookmarks.py even after adding library_save.py,
which is the test for whether the refactor was the right call.
Fifth Phase 2 site migration. _copy_to_library_unsorted and
_copy_to_library now both delegate to a private
_save_bookmark_to_library helper that walks through save_post_file.
A small _bookmark_to_post adapter constructs a Post from a Bookmark
for the renderer — Bookmark already carries every field the renderer
reads, this is just one place to maintain if Post's shape drifts.
Fixes the latent v0.2.3 bug where bookmark→library copies wrote
files but never registered library_meta rows — those files were on
disk but invisible to Library tag-search until you also re-saved
from the browse side.
Picks up filename templates and sequential collision suffixes for
bookmark→library saves for free, same as the browse-side migrations.
Net add (+32 lines) is from the new helper docstrings + the explicit
_bookmark_to_post adapter; the actual save logic shrinks to a one-
liner per public method.
Fourth Phase 2 site migration. Extracts a shared _batch_download_to
helper that owns the async loop with a per-batch in_flight set, then
makes both _batch_download (the dialog-driven entry) and
_batch_download_posts (the multi-select entry) thin wrappers that
delegate to it.
Fixes the latent v0.2.3 bug where batch downloads landing inside
saved_dir() never wrote library_meta rows — _on_batch_done painted
saved-dots from disk but the search index stayed empty. The
library_meta write is now automatic via save_post_file's
is_relative_to(saved_dir()) check, so any batch into a library folder
gets indexed for free.
Also picks up filename templates and sequential collision suffixes
across batch downloads — collision-prone templates like %artist% on a
page of same-artist posts now produce someartist.jpg, someartist_1.jpg,
someartist_2.jpg instead of clobbering.
Third Phase 2 site migration. Default filename in the dialog now
comes from rendering the library_filename_template against the post,
so users see their templated name and can edit if they want. Drops
the legacy hardcoded "post_" prefix on the default — anyone who wants
the prefix can put it in the template.
The actual save still routes through save_post_file with
explicit_name set to whatever the user typed, so collision resolution
runs even on user-chosen filenames (sequential _1/_2 if the picked
name already belongs to a different post in the library).
behavior change from v0.2.3: Save As into saved_dir() now registers
library_meta. Previously Save As never wrote meta regardless of
destination. If a file is in the library it should be searchable —
this fixes that.
Second Phase 2 site migration. Hoists destination resolution out of
the per-iteration loop, uses a shared in_flight set so collision-prone
templates (%artist% on a page of same-artist posts) get sequential
suffixes instead of clobbering each other, and finally calls
_copy_library_thumb so multi-select bulk saves get library thumbnails
just like single-post saves do.
Drops the dead site_id assignment that nothing read.
Fixes the latent bug where _bulk_save left library thumbnails uncopied
even though _save_to_library always copied them — multi-select saves
were missing thumbnails in the Library tab until you re-saved one at
a time.
First Phase 2 site migration. _save_to_library shrinks from ~80 lines
to ~30 by delegating to core.library_save.save_post_file. The
"find existing copy and rename across folders" block is gone — same-
post idempotency is now handled by the DB-backed filename column via
_same_post_on_disk inside save_post_file. The thumbnail-copy block is
extracted as a new _copy_library_thumb helper so _bulk_save (Phase
2.2) can call it too.
behavior change from v0.2.3: cross-folder re-save is now copy, not
move. Old folder's copy is preserved. The atomic-rename-move was a
workaround for not having a DB-backed filename column; with
_same_post_on_disk the workaround is unnecessary. Users who want
move semantics can manually delete the old copy.
Net diff: -52 lines.
Adds the foundation that the unified save flow refactor builds on. No
behavior change at this commit — empty default template means every save
site still produces {id}{ext} like v0.2.3.
- core/db.py: library_meta.filename column with non-breaking migration
for legacy databases. Index on filename. New
get_library_post_id_by_filename() lookup. filename kwarg on
save_library_meta (defaults to "" for legacy callers).
library_filename_template added to _DEFAULTS.
- core/config.py: render_filename_template() with %id% %md5% %ext%
%rating% %score% %artist% %character% %copyright% %general% %meta%
%species% tokens. Sanitizes filesystem-reserved chars, collapses
whitespace, strips leading dots/.., caps the rendered stem at 200
characters, falls back to post id when sanitization yields empty.
- gui/settings.py: Library filename template input field next to the
Library directory row, with a help label listing tokens and noting
that Gelbooru/Moebooru can only resolve the basic ones.
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 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
