From 8103275a1fb6b3ab9763e467d5f172d848b64a9b Mon Sep 17 00:00:00 2001 From: Marcin Zawalski Date: Mon, 6 Jul 2026 18:26:36 +0200 Subject: [PATCH] Fix Next/Prev enable state (#407) and unbind Cyan shortcut by default (#406) #407: the toolbar's Next/Prev enable check ran in raw uploaded_files (load-order) index space while navigation walks the sidebar's sorted/ filtered display order. On rolls not loaded alphabetically the two disagree: Next greys out on the last-loaded frame (mid-list) and stays enabled-but-no-op on the display-last frame. Compute enable state in display space via asset_model.actual_to_display, matching next_file/ prev_file, and refresh on asset_model.layoutChanged (sort/filter changes emit that, not state_changed). #406: ship Cyan (cyan_inc/cyan_dec) with empty default bindings. Empty keys are already handled end-to-end (apply_bindings and the editor's duplicate check skip them), so Cyan is unbound out of the box yet stays assignable via the shortcut editor. --- negpy/desktop/view/canvas/toolbar.py | 7 +++-- negpy/desktop/view/shortcut_registry.py | 4 +-- tests/test_desktop_session.py | 41 +++++++++++++++++++++++++ tests/test_shortcut_registry.py | 11 +++++++ 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/negpy/desktop/view/canvas/toolbar.py b/negpy/desktop/view/canvas/toolbar.py index d7e485b3..e1263c55 100644 --- a/negpy/desktop/view/canvas/toolbar.py +++ b/negpy/desktop/view/canvas/toolbar.py @@ -316,6 +316,7 @@ def _connect_signals(self) -> None: self.controller.zoom_changed.connect(self._on_zoom_changed) self.session.state_changed.connect(self._update_ui_state) + self.session.asset_model.layoutChanged.connect(self._update_ui_state) # Overflow menu action connections self._ov_hq_action.triggered.connect(self.controller.toggle_hq_preview) @@ -415,8 +416,10 @@ def _show_shortcuts(self) -> None: def _update_ui_state(self) -> None: state = self.session.state - self.btn_prev.setEnabled(state.selected_file_idx > 0) - self.btn_next.setEnabled(state.selected_file_idx < len(state.uploaded_files) - 1) + model = self.session.asset_model + display_idx = model.actual_to_display(state.selected_file_idx) + self.btn_prev.setEnabled(display_idx > 0) + self.btn_next.setEnabled(0 <= display_idx < model.rowCount() - 1) self.btn_hq.setChecked(state.hq_preview) self._ov_hq_action.setChecked(state.hq_preview) diff --git a/negpy/desktop/view/shortcut_registry.py b/negpy/desktop/view/shortcut_registry.py index 61de1df3..5d916d7f 100644 --- a/negpy/desktop/view/shortcut_registry.py +++ b/negpy/desktop/view/shortcut_registry.py @@ -26,8 +26,8 @@ class ShortcutEntry: "auto_crop": ShortcutEntry("Shift+A", "Toggle autocrop", "Geometry"), "pick_dust": ShortcutEntry("Shift+D", "Toggle heal tool", "Tools"), "cancel_tool": ShortcutEntry("Esc", "Cancel active tool", "Tools"), - "cyan_dec": ShortcutEntry("Alt+Shift+C", "Cyan down", "Exposure"), - "cyan_inc": ShortcutEntry("Alt+C", "Cyan up", "Exposure"), + "cyan_dec": ShortcutEntry("", "Cyan down", "Exposure"), + "cyan_inc": ShortcutEntry("", "Cyan up", "Exposure"), "magenta_down": ShortcutEntry("D", "Magenta down", "Exposure"), "magenta_up": ShortcutEntry("E", "Magenta up", "Exposure"), "yellow_down": ShortcutEntry("F", "Yellow down", "Exposure"), diff --git a/tests/test_desktop_session.py b/tests/test_desktop_session.py index c520a559..6ba8e32d 100644 --- a/tests/test_desktop_session.py +++ b/tests/test_desktop_session.py @@ -494,5 +494,46 @@ def test_clearing_filter_restores_full_list(self): self.assertEqual(len(self.model._sorted_indices), 5) +class TestNavButtonBoundaries(unittest.TestCase): + """Regression for #407: Next/Prev enable must be computed in display space + (sorted/filtered order), matching session.next_file/prev_file — not raw + uploaded_files (load-order) index space.""" + + def setUp(self): + self.state = AppState() + # Loaded reverse-alphabetically; default sort is name-ascending, so + # display order is the reverse of load order. + self.state.uploaded_files = [ + {"name": "c.dng", "path": "/tmp/c.dng", "hash": "h1"}, + {"name": "b.dng", "path": "/tmp/b.dng", "hash": "h2"}, + {"name": "a.dng", "path": "/tmp/a.dng", "hash": "h3"}, + ] + self.model = AssetListModel(self.state) + + def _enabled(self, actual_idx): + display_idx = self.model.actual_to_display(actual_idx) + prev_enabled = display_idx > 0 + next_enabled = 0 <= display_idx < self.model.rowCount() - 1 + return prev_enabled, next_enabled + + def test_first_display_file_is_last_loaded(self): + # a.dng (actual 2) is the last-loaded but first in display order. + prev_enabled, next_enabled = self._enabled(2) + self.assertFalse(prev_enabled) + self.assertTrue(next_enabled) + + def test_last_display_file_is_first_loaded(self): + # c.dng (actual 0) is the first-loaded but last in display order. + prev_enabled, next_enabled = self._enabled(0) + self.assertTrue(prev_enabled) + self.assertFalse(next_enabled) + + def test_filtered_out_selection_disables_both(self): + self.model.set_filter("a.dng", regex=False) + prev_enabled, next_enabled = self._enabled(0) # c.dng no longer visible + self.assertFalse(prev_enabled) + self.assertFalse(next_enabled) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_shortcut_registry.py b/tests/test_shortcut_registry.py index 609b235a..860bbf03 100644 --- a/tests/test_shortcut_registry.py +++ b/tests/test_shortcut_registry.py @@ -39,6 +39,17 @@ def test_load_bindings_merges_saved_overrides(): assert bindings["grade_up"] == default_bindings()["grade_up"] +def test_cyan_defaults_empty_but_bindable(): + # #406: Cyan ships with no default binding, yet stays assignable via the editor. + defaults = default_bindings() + assert defaults["cyan_inc"] == "" + assert defaults["cyan_dec"] == "" + + bindings = merge_bindings({"cyan_inc": "Alt+C", "cyan_dec": "Alt+Shift+C"}) + assert bindings["cyan_inc"] == "Alt+C" + assert bindings["cyan_dec"] == "Alt+Shift+C" + + def test_tooltip_with_multiple_shortcuts_renders_all_keys(): tooltip = tooltip_with_shortcut("Density", ["density_up", "density_down"], {"density_up": "Q", "density_down": "A"})