Merged
Conversation
… navigation - Implemented `compute_next_after_rotation` in `rotation_utils.py` to determine the next path after rotations. - Created `StatusBarInfo` class and `build_status_bar_info` function in `statusbar_utils.py` for structured status messages. - Refactored `_group_images_by_cluster` in `main_window.py` to use `ClusterUtils`. - Updated navigation methods in `main_window.py` to utilize new `navigate_group_cyclic` and `navigate_linear` functions. - Removed obsolete navigation methods `_index_below` and `_index_above` from `main_window.py`. - Added tests for rotation utilities, status bar utilities, cluster utilities, and navigation utilities. - Ensured proper handling of file paths and metadata in the new utility functions.
…bosadev/PhotoSort into refactor-encapsulation
…t runs without an X server
duartebarbosadev
changed the title
… similarity controllers - Implement tests for FileDeletionController to verify single and multi-selection deletion behavior, including header pruning. - Create tests for FilterController to ensure search and cluster filtering functionality works as expected. - Add deferred initialization tests for FilterController to check behavior when proxy models are set after controller creation. - Update tests for finding proxy indices to avoid pytest collection warnings by renaming test classes. - Introduce integration tests for HotkeyController to validate skip_deleted flag propagation. - Develop tests for MetadataController to confirm refresh behavior on selection changes. - Implement navigation controller tests to validate group and linear navigation, including handling of deleted images. - Add tests for PreviewController to ensure correct preload behavior and status messaging. - Create tests for RotationController to verify acceptance and refusal of rotation paths. - Implement selection controller tests to confirm correct retrieval of selected file paths. - Add similarity controller tests to validate clustering and embedding functionality. - Introduce tests for smart navigation to ensure correct cycling behavior within groups.
- Implemented tests for finding first and last visible items in both list and tree contexts. - Created a new test suite for smart down navigation, ensuring sequential navigation through image items. - Added tests for smart up navigation, verifying sequential behavior and preventing wrap-around within groups. - Introduced simple and fixed test cases for both down and up navigation scenarios. - Refactored navigation logic to work without proxy models, simplifying the selection process.
…ate management and selection preservation
- Introduced `test_image_rotator.py` to validate the functionality of the ImageRotator class, including rotation support, orientation calculations, and error handling. - Enhanced `test_metadata_controller.py` with parameterized tests to ensure metadata refresh behavior on selection changes and sidebar visibility. - Created `test_metadata_processor_constants.py` to verify the integrity of metadata-related constants. - Developed `test_metadata_processor_core.py` for extensive testing of the MetadataProcessor class, covering batch metadata extraction, caching, and error handling. - Added `test_metadata_processor_helpers.py` to validate helper functions used in metadata processing. - Implemented `test_metadata_processor_rotation.py` to test rotation methods and their integration with caching. - Updated `test_navigation_controller.py` to improve navigation tests, ensuring correct behavior with deleted items. - Introduced `test_rotation_integration.py` to validate the complete rotation system through integration tests.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the main window by extracting UI logic into dedicated controller classes to improve code organization, testability, and maintainability. The refactor separates complex behaviors from MainWindow, introduces comprehensive test coverage for the new controllers, and makes minor improvements to CI configuration and code cleanup.
Key changes:
- Introduction of 10 new controller classes to encapsulate UI behaviors previously embedded in
MainWindow - Addition of 33 comprehensive test files covering controller logic, helpers, and integration scenarios
- CI improvements including headless PyQt6 support and better artifact handling
- Minor code cleanups removing outdated comments and improving readability
Reviewed Changes
Copilot reviewed 59 out of 60 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/*.py (33 files) | Comprehensive test coverage for new controllers, helpers, and integration testing |
| src/ui/controllers/*.py (9 files) | New controller classes extracting navigation, deletion, similarity, and other UI behaviors |
| src/ui/helpers/*.py (7 files) | Utility functions supporting the extracted controller logic |
| DEVELOPER_GUIDE.md | Documentation of new controller layer and extension patterns |
| Various core files | Minor comment cleanups removing outdated annotations |
Comments suppressed due to low confidence (3)
tests/test_metadata_processor_core.py:1
- The removal of helper function and constant imports suggests these have been moved to separate test files, but this could break existing tests that depend on these imports. Ensure all references to the removed imports are handled in the new separate test files.
import pyexiv2 # noqa: F401 # This must be the first import or else it will cause a silent crash on windows
tests/test_metadata_processor_core.py:1
- Good refactor to use parametrized testing for rating values. This improves test coverage by testing multiple rating values in a single test method rather than looping within the test.
import pyexiv2 # noqa: F401 # This must be the first import or else it will cause a silent crash on windows
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| images_by_cluster: Dict[int, List[Dict[str, Any]]], | ||
| embeddings_cache: Dict[str, List[float]], | ||
| date_cache: Dict[str, Optional[date_obj]], | ||
| ) -> List[int]: |
There was a problem hiding this comment.
Excellent documentation in the docstring explaining the PCA approach and fallback strategy. The use of date_obj.max as a sentinel value for missing timestamps is well-documented and provides clear rationale for the implementation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a significant documentation update and codebase refactor focused on encapsulating UI logic into dedicated controller classes, as well as some improvements to CI configuration and minor code cleanups. The main goal is to reduce the complexity of
MainWindow, separate destructive and non-destructive deletion logic, and make the codebase easier to test and maintain. Additionally, there are minor fixes and improvements in caching modules and CI workflow for better headless test support.Documentation and Refactor (Encapsulation Phase)
DEVELOPER_GUIDE.mdto document the new controller layer, detailing each controller's responsibilities and the extension pattern for future additions. This clarifies the separation of concerns and guides developers on maintaining consistency during future refactors.DEVELOPER_GUIDE.md, listing extracted behaviors, split responsibilities, protocol-driven design, and deterministic sorting strategies.DeletionMarkControllerinsrc/ui/controllers/deletion_mark_controller.py, which encapsulates non-destructive deletion mark and blur presentation logic, separating it from destructive filesystem operations. This controller provides batch operations and UI integration helpers for marking/unmarking files and updating their presentation.CI Workflow Improvements
libegl1to system dependencies in.github/workflows/ci.ymlto support headless PyQt6 usage.QT_QPA_PLATFORM: offscreenin test environment to ensure Qt runs without an X server, improving CI reliability for GUI tests..github/workflows/ci.ymlby specifying directory paths and handling missing files gracefully.Code Cleanups and Minor Fixes
importstatements in core caching modules and image processing, cleaning up unnecessary annotations. [1] [2] [3] [4] [5] [6]scan_directoryto improve code readability.start_auto_rotation_analysisfor clarity.