feat: Implement navigation preferences for skipping singleton clusters and rating-based navigation#61
Conversation
…s and rating-based navigation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces advanced navigation features for the photo sorting application, enabling users to navigate images based on similarity clusters or star ratings. The implementation includes new navigation helper functions, menu options for configuring navigation preferences, and integration throughout the application.
Key Changes:
- Added navigation utility functions to jump between images by rating or multi-image similarity clusters
- Implemented menu actions for "Skip Single-Image Clusters" and "Jump to Rating" navigation modes
- Integrated navigation preferences into the main window with dynamic enable/disable logic based on view mode and available data
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_navigation_preferences.py | New test suite for navigation helper functions with comprehensive coverage of edge cases |
| src/ui/helpers/navigation_utils.py | Added three new navigation functions with iterator helper for finding next rating matches and cluster heads |
| src/ui/menu_manager.py | Added menu actions for navigation preferences with proper state synchronization and tooltips |
| src/ui/main_window.py | Integrated navigation preferences with path lookup utilities and state management for skip-singleton and rating-based navigation |
| src/ui/app_controller.py | Added refresh calls for navigation shortcut actions during folder load and clustering completion |
| README.md | Removed outdated planned features from the roadmap |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lambda checked, value=option: self._handle_cluster_sort_selection(value) | ||
| ) | ||
| self.cluster_sort_actions[option] = action | ||
| self.cluster_sort_actions[option] = action |
There was a problem hiding this comment.
The assignment self.cluster_sort_actions[option] = action is incorrectly indented outside the for loop. This means only the last action from the loop will be stored in the dictionary, potentially losing all previous actions. This line should be indented at the same level as the other statements inside the for loop (lines 449-455).
| self.cluster_sort_actions[option] = action | |
| self.cluster_sort_actions[option] = action |
| def _get_cluster_id_for_path(self, path: str) -> Optional[int]: | ||
| value = self._lookup_path_dict(self.app_state.cluster_results, path, None) | ||
| if isinstance(value, str): | ||
| try: | ||
| return int(value) | ||
| except ValueError: | ||
| return value | ||
| return value |
There was a problem hiding this comment.
The return type annotation Optional[int] is incorrect. When value is a string that cannot be converted to int, line 1715 returns the original string value, making the actual return type Optional[int | str]. This type inconsistency could cause issues when the cluster_lookup callable is passed to navigation_utils functions that expect Callable[[str], Optional[int]].
| try: | ||
| current_index = ( | ||
| visible_paths.index(current_path) | ||
| if current_path in visible_paths | ||
| else -1 | ||
| ) | ||
| except ValueError: | ||
| current_index = -1 |
There was a problem hiding this comment.
The try-except block is unnecessary here. The if current_path in visible_paths condition on line 1752 ensures that visible_paths.index(current_path) will not raise a ValueError. The except clause on lines 1755-1756 is unreachable code. Consider simplifying to just use the conditional expression without the try-except.
| try: | |
| current_index = ( | |
| visible_paths.index(current_path) | |
| if current_path in visible_paths | |
| else -1 | |
| ) | |
| except ValueError: | |
| current_index = -1 | |
| current_index = ( | |
| visible_paths.index(current_path) | |
| if current_path in visible_paths | |
| else -1 | |
| ) |
This pull request introduces advanced navigation features to the photo sorting app, allowing users to jump between images based on similarity clusters or star ratings. It adds new navigation helpers, updates menus for these features, and ensures that navigation shortcuts and menu options are dynamically enabled or disabled based on the current view and data. The changes also include code refactoring for improved maintainability.
Navigation Feature Enhancements:
navigation_utils.pyto support jumping to images by rating and multi-image similarity clusters. These includefind_next_rating_match,find_next_multi_image_cluster_head, andfind_next_in_same_multi_cluster, along with supporting utilities.main_window.pyto use these helpers, allowing users to navigate up/down through images based on star ratings or to skip singleton clusters when in similarity group mode. Preferences can be set via new menu actions, and navigation behavior is coordinated with the current app state.Menu and UI Updates:
menu_manager.pyfor "Skip Single-Image Clusters" and "Jump to Rating" navigation modes, including dynamic enable/disable logic and synchronization with the main window's state.Codebase Refactoring and Maintenance:
navigation_utils.pyfor clarity and extensibility, including the addition ofCounterand more flexible callable signatures.main_window.py, making navigation more robust and consistent.README and Documentation:
README.mdto reflect the current capabilities and roadmap more accurately.