From e0bfeedf28570790eb498f35454c3aca287b9679 Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Thu, 26 Mar 2026 15:34:18 -0700 Subject: [PATCH 1/3] fix race condition in histogram display --- faststack/app.py | 51 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/faststack/app.py b/faststack/app.py index 0be6dd4..da6f687 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -198,8 +198,12 @@ def __init__( max_workers=1, thread_name_prefix="Histogram" ) self._hist_inflight = False + self._hist_inflight_since: float = 0.0 # monotonic time when inflight was set + self._hist_inflight_token: int = 0 # token of the currently inflight job self._hist_pending = None self._hist_token = 0 + self._hist_null_retries: int = 0 # consecutive None-result retries + self._hist_last_args = (1.0, 0.0, 0.0, 1.0) # last zoom/pan/scale args used self._hist_lock = threading.Lock() self.histogramReady.connect(self._apply_histogram_result) self.previewReady.connect(self._apply_preview_result) @@ -5938,6 +5942,7 @@ def update_histogram( with self._hist_lock: self._hist_pending = (zoom, pan_x, pan_y, image_scale) + self._hist_null_retries = 0 # Fresh request resets retry counter inflight = self._hist_inflight if not self.histogram_timer.isActive() and not inflight: @@ -5949,20 +5954,35 @@ def _kick_histogram_worker(self): with self._hist_lock: if self._hist_inflight: - return + # Safety: if inflight has been stuck for >10s, force-reset it. + # This is a defensive fallback against unforeseen edge cases + # where the done-callback or signal delivery fails to clear it. + elapsed = time.monotonic() - self._hist_inflight_since + if elapsed < 10.0: + return + log.warning( + "Histogram inflight stuck for %.1fs, force-resetting", elapsed + ) + self._hist_inflight = False if self._hist_pending is None: return args = self._hist_pending self._hist_pending = None + self._hist_last_args = args # Preserve for retry on None result self._hist_token += 1 token = self._hist_token # Mark as inflight while holding the lock to prevent others from entering self._hist_inflight = True + self._hist_inflight_token = token + self._hist_inflight_since = time.monotonic() - # Snap the currently known preview data to avoid racing with the editor + # Snap the currently known preview data to avoid racing with the editor. + # Only use cached preview if it matches the current image to prevent stale histograms. preview_data = self._last_rendered_preview + if preview_data and self._last_rendered_preview_index != self.current_index: + preview_data = None if not preview_data: # Fallback for initial load if no edit preview yet (could use get_decoded_image?) # But histogram is mostly for edits. If preview_data is None, we likely can't compute anyway. @@ -6004,7 +6024,9 @@ def _kick_histogram_worker(self): self, target_index, ) - fut.add_done_callback(self._on_histogram_done) + fut.add_done_callback( + functools.partial(self._on_histogram_done, submitted_token=token) + ) except Exception as e: log.error("Histogram executor failed to submit task: %s", e) with self._hist_lock: @@ -6099,14 +6121,16 @@ def _compute_histogram_worker( except Exception: return token, None - def _on_histogram_done(self, fut): + def _on_histogram_done(self, fut, submitted_token): if getattr(self, "_shutting_down", False): return try: token, hist = fut.result() except Exception: - token, hist = None, None + # Use the submitted_token so _apply_histogram_result can still + # match this completion to the correct inflight job. + token, hist = submitted_token, None # bounce back to UI thread via signal self.histogramReady.emit((token, hist)) @@ -6119,15 +6143,28 @@ def _apply_histogram_result(self, payload): token, hist = payload with self._hist_lock: - self._hist_inflight = False + # Only clear inflight if this result is from the current job. + # After a force-reset in _kick_histogram_worker, a stale completion + # must not clear inflight for a newer job that is already running. + if token == self._hist_inflight_token: + self._hist_inflight = False if hist is not None: if token == self._hist_token: self.ui_state.histogramData = hist self.ui_state.highlightStateChanged.emit() + self._hist_null_retries = 0 - # If more updates arrived while we computed, run again soon + # If more updates arrived while we computed, run again soon. + # Also retry (up to 3 times) if the computation returned None + # (image may not have been cached yet after the 50ms timer). pending = self._hist_pending is not None + if not pending and hist is None and self._hist_null_retries < 3 and ( + self.ui_state.isHistogramVisible or self.ui_state.isEditorOpen + ): + self._hist_null_retries += 1 + self._hist_pending = self._hist_last_args + pending = True if pending: self.histogram_timer.start() From b0545d5ed2f53bed00855a3727ac5cbb70553d50 Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Thu, 26 Mar 2026 17:09:27 -0700 Subject: [PATCH 2/3] Make it so the user can still use keyboard shortcuts when the histogram is open, and if a batch is selected, drag and drop just moves the batch instead of the current image too --- faststack/app.py | 37 +++++++++++++++++++++++++++---- faststack/qml/HistogramWindow.qml | 4 ++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/faststack/app.py b/faststack/app.py index da6f687..d1065fe 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -198,8 +198,9 @@ def __init__( max_workers=1, thread_name_prefix="Histogram" ) self._hist_inflight = False - self._hist_inflight_since: float = 0.0 # monotonic time when inflight was set + self._hist_inflight_since: float = 0.0 # monotonic time when worker started self._hist_inflight_token: int = 0 # token of the currently inflight job + self._hist_inflight_future: Optional[concurrent.futures.Future] = None self._hist_pending = None self._hist_token = 0 self._hist_null_retries: int = 0 # consecutive None-result retries @@ -774,6 +775,14 @@ def zoom_400(self): self.sync_ui_state() self.ui_state.isZoomedChanged.emit() + @Slot(int, int) + def handle_key_from_histogram(self, key: int, modifiers: int): + """Forward key presses from the histogram window to the keybinder.""" + from PySide6.QtGui import QKeyEvent + + event = QKeyEvent(QKeyEvent.Type.KeyPress, key, Qt.KeyboardModifier(modifiers)) + self.keybinder.handle_key_press(event) + def eventFilter(self, watched, event) -> bool: # Don't handle key events when a dialog is open if self._dialog_open: @@ -5490,9 +5499,8 @@ def start_drag_current_image(self): if not self.image_files or self.current_index >= len(self.image_files): return - # Collect all files: current + any in defined batches + # Collect files to drag: batch files if any batches exist, otherwise current image files_to_drag = set() - files_to_drag.add(self.current_index) # Add all files from defined batches for start, end in self.batches: @@ -5500,6 +5508,10 @@ def start_drag_current_image(self): if 0 <= idx < len(self.image_files): files_to_drag.add(idx) + # Only include current image if no batch files were selected + if not files_to_drag: + files_to_drag.add(self.current_index) + # Convert to sorted list and get only existing paths file_indices = sorted(files_to_drag) existing_indices = [ @@ -5957,13 +5969,22 @@ def _kick_histogram_worker(self): # Safety: if inflight has been stuck for >10s, force-reset it. # This is a defensive fallback against unforeseen edge cases # where the done-callback or signal delivery fails to clear it. + if self._hist_inflight_since == 0.0: + return # Job is queued but hasn't started yet elapsed = time.monotonic() - self._hist_inflight_since if elapsed < 10.0: return + # Only force-reset if the Future is actually done (callback was + # lost). If the worker is still running, resetting would just + # queue a duplicate behind it on the single-worker executor. + fut = self._hist_inflight_future + if fut is not None and not fut.done(): + return # Worker is genuinely still running, not lost log.warning( "Histogram inflight stuck for %.1fs, force-resetting", elapsed ) self._hist_inflight = False + self._hist_inflight_future = None if self._hist_pending is None: return @@ -5976,7 +5997,7 @@ def _kick_histogram_worker(self): # Mark as inflight while holding the lock to prevent others from entering self._hist_inflight = True self._hist_inflight_token = token - self._hist_inflight_since = time.monotonic() + self._hist_inflight_since = 0.0 # Set when worker actually starts # Snap the currently known preview data to avoid racing with the editor. # Only use cached preview if it matches the current image to prevent stale histograms. @@ -6024,6 +6045,7 @@ def _kick_histogram_worker(self): self, target_index, ) + self._hist_inflight_future = fut fut.add_done_callback( functools.partial(self._on_histogram_done, submitted_token=token) ) @@ -6031,12 +6053,18 @@ def _kick_histogram_worker(self): log.error("Histogram executor failed to submit task: %s", e) with self._hist_lock: self._hist_inflight = False + self._hist_inflight_future = None @staticmethod def _compute_histogram_worker( token, args, decoded, controller=None, target_index=-1 ): # IMPORTANT: do not touch QObjects here except thread-safe plain data + # Record when the worker actually starts executing (not when it was queued) + if controller: + with controller._hist_lock: + controller._hist_inflight_since = time.monotonic() + zoom, pan_x, pan_y, image_scale = args # If data wasn't provided, try to fetch it safely using the controller @@ -6148,6 +6176,7 @@ def _apply_histogram_result(self, payload): # must not clear inflight for a newer job that is already running. if token == self._hist_inflight_token: self._hist_inflight = False + self._hist_inflight_future = None if hist is not None: if token == self._hist_token: diff --git a/faststack/qml/HistogramWindow.qml b/faststack/qml/HistogramWindow.qml index 944d11d..c3af857 100644 --- a/faststack/qml/HistogramWindow.qml +++ b/faststack/qml/HistogramWindow.qml @@ -21,6 +21,10 @@ Window { if (event.key === Qt.Key_H && controller) { controller.toggle_histogram() event.accepted = true + } else if (controller) { + // Forward unhandled keys (e.g. arrow keys) to controller + controller.handle_key_from_histogram(event.key, event.modifiers) + event.accepted = true } } } From 10a6618102d8d440a4e6f3b1f1f8d4a8d225b109 Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Thu, 26 Mar 2026 17:44:48 -0700 Subject: [PATCH 3/3] Clean up histogram fix --- faststack/app.py | 22 +++++++++++++++------- faststack/qml/HistogramWindow.qml | 2 +- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/faststack/app.py b/faststack/app.py index d1065fe..36aae2b 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -775,13 +775,15 @@ def zoom_400(self): self.sync_ui_state() self.ui_state.isZoomedChanged.emit() - @Slot(int, int) - def handle_key_from_histogram(self, key: int, modifiers: int): - """Forward key presses from the histogram window to the keybinder.""" + @Slot(int, int, str) + def handle_key_from_histogram(self, key: int, modifiers: int, text: str): + """Forward key presses from the histogram window through eventFilter.""" from PySide6.QtGui import QKeyEvent - event = QKeyEvent(QKeyEvent.Type.KeyPress, key, Qt.KeyboardModifier(modifiers)) - self.keybinder.handle_key_press(event) + event = QKeyEvent( + QKeyEvent.Type.KeyPress, key, Qt.KeyboardModifier(modifiers), text + ) + self.eventFilter(self.main_window, event) def eventFilter(self, watched, event) -> bool: # Don't handle key events when a dialog is open @@ -6187,9 +6189,15 @@ def _apply_histogram_result(self, payload): # If more updates arrived while we computed, run again soon. # Also retry (up to 3 times) if the computation returned None # (image may not have been cached yet after the 50ms timer). + # Only retry for the current token — stale completions should not + # trigger retries or consume the retry budget. pending = self._hist_pending is not None - if not pending and hist is None and self._hist_null_retries < 3 and ( - self.ui_state.isHistogramVisible or self.ui_state.isEditorOpen + if ( + not pending + and hist is None + and token == self._hist_token + and self._hist_null_retries < 3 + and (self.ui_state.isHistogramVisible or self.ui_state.isEditorOpen) ): self._hist_null_retries += 1 self._hist_pending = self._hist_last_args diff --git a/faststack/qml/HistogramWindow.qml b/faststack/qml/HistogramWindow.qml index c3af857..2829434 100644 --- a/faststack/qml/HistogramWindow.qml +++ b/faststack/qml/HistogramWindow.qml @@ -23,7 +23,7 @@ Window { event.accepted = true } else if (controller) { // Forward unhandled keys (e.g. arrow keys) to controller - controller.handle_key_from_histogram(event.key, event.modifiers) + controller.handle_key_from_histogram(event.key, event.modifiers, event.text) event.accepted = true } }