pax/booru-viewer
1
0
Fork 0
Code Issues Releases 18 Activity

Popout/grid: stop calling _on_post_activated twice per keyboard nav

Browse Source 
Pre-existing bug in `_navigate_preview` that surfaced after the
preceding perf round shifted timing enough to expose the race. For
every tab, `_navigate_preview` was calling `grid._select(idx)`
followed by an explicit activate-handler call:

    self._grid._select(idx)
    self._on_post_activated(idx)   # ← redundant

`grid._select(idx)` ends with `self.post_selected.emit(index)`,
which is wired to `_on_post_selected` (or the bookmark/library
equivalents), which already calls `_on_post_activated` after a
multi-select length check that's always 1 here because `_select`
calls `_clear_multi` first. So the activation handler ran TWICE per
keyboard nav.

Each `_on_post_activated` schedules an async `_load`, which fires
`image_done` → `_on_image_done` → `_update_fullscreen` →
`set_media` → `_video.stop()` + `_video.play_file(path)`. Two
activations produced two `set_media` cycles in quick succession.

The stale-eof suppression race:

  1. First `play_file` opens window A: `_eof_ignore_until = T+250ms`
  2. Second `play_file` runs ~10-50ms later
  3. Inside the second `play_file`: `_eof_pending = False` runs
     BEFORE `_eof_ignore_until` is reset
  4. Window A may have already expired by this point if the load
     was slow
  5. An async `eof-reached=True` event from the second
     `_video.stop()` lands in the un-armed gap
  6. The gate check `monotonic() < _eof_ignore_until` fails (window A
     expired, window B not yet open)
  7. `_eof_pending = True` sticks
  8. Next `_poll` cycle: `_handle_eof` sees Loop=Next, emits
     `play_next` → `_on_video_end_next` → `_navigate_preview(1, wrap=True)`
     → ANOTHER post advance
  9. User pressed Right once, popout skipped a post

Random and timing-dependent. Hard to reproduce manually but happens
often enough to be visible during normal browsing.

Fix: stop calling the activation handler directly after `_select`.
The signal chain handles it. Applied to all five sites in
`_navigate_preview`:

  - browse view (line 2046-2047)
  - bookmarks view normal nav (line 2024-2025)
  - bookmarks view wrap-edge (line 2028-2029)
  - library view normal nav (line 2036-2037)
  - library view wrap-edge (line 2040-2041)

The wrap-edge cases were called out in the original plan as "leave
alone for scope creep" but they have the same duplicate-call shape
and the same race exposure during auto-advance from EOF. Fixing
them keeps the code consistent and removes a latent bug from a
less-traveled path.

Verified by reading: `_grid._select(idx)` calls `_clear_multi()`
first, so by the time `post_selected` fires, `selected_indices`
returns `[idx]` (length 1), `_on_post_selected`'s multi-select
early-return doesn't fire, and `_on_post_activated(index)` is
always called. Same for the bookmark/library `_on_selected` slots
which have no early-return at all.

Net: ~5 lines deleted, ~25 lines of comments added explaining the
race and the trust-the-signal-chain rule for future contributors.
This commit is contained in:
pax 2026-04-08 02:34:09 -05:00
parent fda3b10beb
commit 31d02d3c7b
1 changed files with 24 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
booru_viewer/gui/app.py
View File

@ -2015,6 +2015,30 @@ class BooruApp(QMainWindow):
when running off the edge — used for the video-end "Next" auto-advance
on tabs that don't have pagination.
"""
# Note on the missing explicit activate calls below: every
# tab's `grid._select(idx)` already chains through to the
# activation handler via the `post_selected` signal, which is
# wired (per tab) to a slot that ultimately calls
# `_on_post_activated` / `_on_bookmark_activated` /
# `_show_library_post`. The previous version of this method
# called the activate handler directly *after* `_select`,
# which fired the activation TWICE per keyboard navigation.
#
# The second activation scheduled a second async `_load`,
# which fired a second `set_media` → second `_video.stop()` →
# second `play_file()` cycle. The two `play_file`'s 250ms
# stale-eof ignore windows leave a brief un-armed gap between
# them (between the new `_eof_pending = False` reset and the
# new `_eof_ignore_until` set). An async `eof-reached=True`
# event from one of the stops landing in that gap would stick
# `_eof_pending = True`, get picked up by `_poll`'s
# `_handle_eof`, fire `play_next` in Loop=Next mode, and
# cause `_navigate_preview(1, wrap=True)` to advance ANOTHER
# post. End result: pressing Right once sometimes advanced
# two posts. Random skip bug, observed on keyboard nav.
#
# Stop calling the activation handlers directly. Trust the
# signal chain.
if self._stack.currentIndex() == 1:
# Bookmarks view
grid = self._bookmarks_view._grid
@ -2022,11 +2046,9 @@ class BooruApp(QMainWindow):
idx = grid.selected_index + direction
if 0 <= idx < len(favs):
grid._select(idx)
self._on_bookmark_activated(favs[idx])
elif wrap and favs:
idx = 0 if direction > 0 else len(favs) - 1
grid._select(idx)
self._on_bookmark_activated(favs[idx])
elif self._stack.currentIndex() == 2:
# Library view
grid = self._library_view._grid
@ -2034,17 +2056,14 @@ class BooruApp(QMainWindow):
idx = grid.selected_index + direction
if 0 <= idx < len(files):
grid._select(idx)
self._library_view.file_activated.emit(str(files[idx]))
elif wrap and files:
idx = 0 if direction > 0 else len(files) - 1
grid._select(idx)
self._library_view.file_activated.emit(str(files[idx]))
else:
idx = self._grid.selected_index + direction
log.info(f"Navigate: direction={direction} current={self._grid.selected_index} next={idx} total={len(self._posts)}")
if 0 <= idx < len(self._posts):
self._grid._select(idx)
self._on_post_activated(idx)
elif idx >= len(self._posts) and direction > 0 and len(self._posts) > 0 and not self._infinite_scroll:
self._search.nav_page_turn = "first"
self._next_page()