From 46a568f326d9136e43fe3b2bb72b711a01d5fa7d Mon Sep 17 00:00:00 2001 From: Sean Harding Date: Tue, 11 Aug 2026 18:45:06 -0500 Subject: [PATCH] Never file a render under a frame it did not come from #800 stopped a finished render being attributed to whatever frame was selected, but only closed half the hole: it checked whether the render's source_hash matched an open asset and, on a miss, still fell back to selected_file_idx. That fallback fires whenever the list has moved on -- a different folder opened, or a merge swapping frames for a composite -- and _update_thumbnail_from_state runs on file switch, on save and after an export, not only from the render that produced the buffer. So last_metrics can hold a render whose frame is no longer open, and its picture is filed under whichever frame is selected now. The write is persisted, so it outlives the session and stays wrong until that frame is rendered again. Found on disk rather than reasoned about: an audit of 68 raw files under one shoot compared every cached thumbnail against its own file by structural correlation, robust to a rendered positive differing in tone from the source preview. One frame scored 0.10 -- slide-07's beach sunset carrying a sideways slide-09-style render -- and the mis-filed image matched, at 1.00, the thumbnail legitimately owned by a frame in slide-08. One image, two keys. A render that cannot be attributed to an open frame now updates nothing. The cost is a skipped refresh in exactly the case where the alternative is writing someone else's picture; the next render of that frame fills it in. The no-source_hash fallback goes too. It was justified on the grounds that active_file_changing snapshots the outgoing frame, which is still selected -- but that cannot be told apart from a different folder being open now, which is the failure above. The render worker sets source_hash unconditionally on a required task field, so refusing costs nothing real. Three existing fixtures omitted source_hash and leaned on the fallback to attribute; they now set the hash their asset carries, which is what the worker always does. --- docs/CHANGELOG.md | 1 + negpy/desktop/controller.py | 19 +++++--- tests/test_controller.py | 4 +- tests/test_thumbnail_attribution.py | 12 +++-- tests/test_thumbnail_attribution_fallback.py | 49 ++++++++++++++++++++ tests/test_thumbnail_readback_thread.py | 4 +- 6 files changed, 74 insertions(+), 15 deletions(-) create mode 100644 tests/test_thumbnail_attribution_fallback.py diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 99c0a51bb..96adfc257 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased +- Fix: **A frame no longer wears another frame's thumbnail after switching folders.** #800 stopped a finished render being filed under whatever frame was selected, but only when the session had no frames at all. With a folder open, a render whose own frame had since left the list — a different folder opened, or a merge swapping frames for a composite — still fell back to the selection, and the write is persisted, so the wrong picture survived restarts until that frame was rendered again. Found on disk: a slide-08 frame's render cached under a slide-07 frame, correlating 0.10 with its own file. A render that cannot be attributed to an open frame now updates nothing; the next render of that frame writes it. - Fix: **The session panel's toolbar measures its own contents correctly.** It sized every item by `sizeHint()`, which is only a preference — `setFixedWidth` does not change it, and a bare `QFrame` has none at all and reports -1. So the 1 px dividers were each given a 3 px slot that Qt then clamped the ink back into, leaving 2 px of dead space beside them and pushing buttons into the **»** menu sooner than the width required. The bar also reserved room for the **»** button before knowing one was needed, which on a bar ending in a divider evicted a real button and so conjured the very chevron it was reserving for. Both now measure what will actually be laid out. This is what `tests/test_overflow_bar.py::test_overflow_button_stays_hidden_when_only_a_separator_would_spill` has been failing on since #701 — on macOS, where a QToolButton's natural hint is wider than the width it is fixed to; CI runs Linux, where it is not. - Fix: **Dialog buttons on macOS no longer underline a letter that does nothing.** Qt puts the mnemonic in its own standard-button text, so a Yes/No box asks for `&Yes`, and the Fusion style NegPy uses for the dark theme draws the underline. macOS has no mnemonic convention, though, so Qt binds no key there — the native style answers that hint the other way and draws nothing. The underline is now suppressed on macOS only; Alt+letter still works, and is still shown, on Windows and Linux. - Fix: **The filmstrip no longer shows one frame's picture on another frame's thumbnail.** A render carries the frame it was started for, but the finished pixels were filed under whichever frame happened to be selected when they arrived — so clicking to the next frame before a full-res decode finished cached the previous frame's image on the new one, where it stayed until that cell was clicked and re-rendered. The thumbnail now follows the render's own frame, as the render memo already did. diff --git a/negpy/desktop/controller.py b/negpy/desktop/controller.py index 69d7aab73..ef40f5b3b 100644 --- a/negpy/desktop/controller.py +++ b/negpy/desktop/controller.py @@ -4489,13 +4489,18 @@ def _asset_for_render(self, metrics: Dict[str, Any]) -> Optional[Dict[str, Any]] active_file_changing caller — there the outgoing file is still selected. """ source_hash = metrics.get("source_hash") - files = self.state.uploaded_files - if source_hash: - for asset in files: - if asset.get("hash") == source_hash: - return asset - idx = self.state.selected_file_idx - return files[idx] if 0 <= idx < len(files) else None + if not source_hash: + return None + for asset in self.state.uploaded_files: + if asset.get("hash") == source_hash: + return asset + # No fallback to the selected frame. This runs on file switch, on save and after an + # export as well as from the render itself, so last_metrics can hold a render whose + # frame has since left the list — a different folder was opened, or a merge swapped + # frames for a composite. Guessing files that buffer under whatever is selected now, + # and persists it, so one frame wears another's picture until it is rendered again. + # A skipped refresh costs nothing; the next render of that frame writes it. + return None def _update_thumbnail_from_state(self, persist: bool = True) -> None: if not self.state.current_file_path or not self.state.current_file_hash: diff --git a/tests/test_controller.py b/tests/test_controller.py index 50397a2bc..98066ea05 100644 --- a/tests/test_controller.py +++ b/tests/test_controller.py @@ -260,7 +260,7 @@ def test_rendered_thumbnail_keys_by_asset_identity_not_filename(self): state.config = dc_replace( state.config, rgbscan=RgbScanConfig(enabled=True, green_path="/tmp/_DSC1317.NEF", blue_path="/tmp/_DSC1318.NEF") ) - state.last_metrics = {"base_positive": np.zeros((2, 2, 3), dtype=np.float32)} + state.last_metrics = {"base_positive": np.zeros((2, 2, 3), dtype=np.float32), "source_hash": "h1"} captured = {} self.controller.thumbnail_update_requested.connect(lambda task: captured.setdefault("task", task)) @@ -1957,7 +1957,7 @@ def test_thumbnail_task_carries_the_same_params_as_the_canvas(self): state.selected_file_idx = 0 state.current_file_path = "/tmp/frame.cr2" state.current_file_hash = "hash-1" - state.last_metrics = {"base_positive": np.zeros((4, 4, 3), dtype=np.float32)} + state.last_metrics = {"base_positive": np.zeros((4, 4, 3), dtype=np.float32), "source_hash": "hash-1"} emitted = [] # Drop the real worker connection first: emitting would otherwise hand the diff --git a/tests/test_thumbnail_attribution.py b/tests/test_thumbnail_attribution.py index f3b1c7969..105de71ef 100644 --- a/tests/test_thumbnail_attribution.py +++ b/tests/test_thumbnail_attribution.py @@ -57,15 +57,19 @@ def test_the_ordinary_case_is_unchanged(self): self.assertEqual(_emitted_key(controller), asset_thumbnail_key(B)) - def test_without_a_source_hash_it_falls_back_to_the_selection(self): - """The active_file_changing caller snapshots the outgoing frame, which is still - the selected one, so the selection is the right answer there.""" + def test_without_a_source_hash_nothing_is_written(self): + """Originally this fell back to the selection, on the grounds that the + active_file_changing caller snapshots the outgoing frame, which is still selected. + That cannot tell "outgoing frame still selected" from "a different folder is open + now", and the second case files one frame's picture under another's — found on + disk, correlating 0.10 with its own file. The render worker always sets + source_hash, so refusing here costs nothing real.""" metrics = {"base_positive": np.zeros((4, 4, 3), np.float32)} controller = _controller(selected_idx=0, metrics=metrics) AppController._update_thumbnail_from_state(controller) - self.assertEqual(_emitted_key(controller), asset_thumbnail_key(A)) + controller.thumbnail_update_requested.emit.assert_not_called() def test_a_render_of_an_unloaded_frame_updates_nothing(self): """Its asset is gone from the session — better to skip than to key by the diff --git a/tests/test_thumbnail_attribution_fallback.py b/tests/test_thumbnail_attribution_fallback.py new file mode 100644 index 000000000..073e4e39a --- /dev/null +++ b/tests/test_thumbnail_attribution_fallback.py @@ -0,0 +1,49 @@ +"""A render whose frame is no longer open must not be filed under whatever is selected now. + +`_update_thumbnail_from_state` runs on file switch, on save, and after an export — not only +from the render that produced the buffer. So `state.last_metrics` can hold a render whose +frame has since left `uploaded_files`: opening a different folder replaces the list, and a +merge or unmerge swaps frames for a composite. Resolving that by falling back to +`selected_file_idx` files one frame's picture under another frame's key, and the write is +persisted, so it survives restarts until that frame is rendered again. +""" + +from unittest.mock import MagicMock + +from negpy.desktop.controller import AppController + + +def _controller(files, selected): + ctrl = MagicMock() + ctrl.state.uploaded_files = files + ctrl.state.selected_file_idx = selected + return ctrl + + +A = {"hash": "hash-a", "path": "/x/a.nef"} +B = {"hash": "hash-b", "path": "/x/b.nef"} + + +def test_a_render_is_attributed_to_its_own_frame(): + ctrl = _controller([A, B], 1) + assert AppController._asset_for_render(ctrl, {"source_hash": "hash-a"}) is A + + +def test_a_render_whose_frame_has_closed_is_not_attributed_to_anyone(): + """The reported bug: a slide-08 frame's render cached under a slide-07 frame's key, + correlation 0.10 against its own file, persisted to disk.""" + ctrl = _controller([A, B], 1) + assert AppController._asset_for_render(ctrl, {"source_hash": "hash-of-a-closed-folder"}) is None + + +def test_metrics_without_an_identity_are_not_attributed(): + ctrl = _controller([A, B], 1) + assert AppController._asset_for_render(ctrl, {}) is None + + +def test_a_composite_is_matched_by_its_own_suffixed_hash(): + """Merges and half-frames carry suffixed hashes; source_hash is current_file_hash, so + they match directly and must not be mistaken for a miss.""" + composite = {"hash": "digest#hdr", "path": "/x/a.nef", "hdr_paths": ("/x/b.nef",)} + ctrl = _controller([composite], 0) + assert AppController._asset_for_render(ctrl, {"source_hash": "digest#hdr"}) is composite diff --git a/tests/test_thumbnail_readback_thread.py b/tests/test_thumbnail_readback_thread.py index 3c11703fb..bf8c012b3 100644 --- a/tests/test_thumbnail_readback_thread.py +++ b/tests/test_thumbnail_readback_thread.py @@ -50,7 +50,7 @@ def _controller_stub(self, metrics): def test_gpu_texture_is_not_read_back_on_the_ui_thread(self): tex = _FakeTexture(np.zeros((8, 8, 4), dtype=np.float32)) - fn, stub = self._controller_stub({"base_positive": tex}) + fn, stub = self._controller_stub({"base_positive": tex, "source_hash": "h1"}) with patch("negpy.desktop.controller.GPUTexture", _FakeTexture): fn(stub) self.assertEqual(tex.readbacks, 0, "the UI thread must not read back the render texture") @@ -59,7 +59,7 @@ def test_gpu_texture_is_not_read_back_on_the_ui_thread(self): def test_uses_the_host_copy_the_worker_attached(self): tex = _FakeTexture(np.zeros((8, 8, 4), dtype=np.float32)) host = np.full((8, 8, 3), 0.25, dtype=np.float32) - fn, stub = self._controller_stub({"base_positive": tex, "thumbnail_source": host}) + fn, stub = self._controller_stub({"base_positive": tex, "thumbnail_source": host, "source_hash": "h1"}) with patch("negpy.desktop.controller.GPUTexture", _FakeTexture): fn(stub) self.assertEqual(tex.readbacks, 0)