From 31d02d3c7b31d36d17ab891908f09cfb0e5cc178 Mon Sep 17 00:00:00 2001 From: pax Date: Wed, 8 Apr 2026 02:34:09 -0500 Subject: [PATCH] Popout/grid: stop calling _on_post_activated twice per keyboard nav MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- booru_viewer/gui/app.py | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/booru_viewer/gui/app.py b/booru_viewer/gui/app.py index e999b0a..145069a 100644 --- a/booru_viewer/gui/app.py +++ b/booru_viewer/gui/app.py @@ -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()