refactor: Remove jpegtran dependency and update ImageRotator for metadata-first rotation approach #44
Conversation
…data-first rotation approach
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the image rotation system by removing the external jpegtran dependency and implementing a metadata-first rotation approach for JPEG files. The changes simplify the rotation logic while improving cross-platform compatibility and maintaining lossless rotation capabilities through metadata manipulation.
- Removed all
jpegtran-related code including availability checks, subprocess calls, and timeout configurations - Implemented metadata-first rotation for JPEG files with fallback to pixel-based rotation
- Updated UI components to use faster extension-based rotation support checks instead of PyExiv2 operations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/image_processing/image_rotator.py | Removed jpegtran logic and implemented new metadata-first JPEG rotation approach |
| src/ui/menu_manager.py | Updated context menu to use ImageRotator's fast extension-based rotation support check |
| src/core/app_settings.py | Removed jpegtran timeout configuration constant |
| tests/test_image_rotator.py | Removed jpegtran tests and added new tests for metadata-first rotation behavior |
| README.md | Removed jpegtran installation instructions and prerequisites |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def __init__(self): | ||
| self.jpegtran_available = self._check_jpegtran_availability() | ||
| logger.info(f"jpegtran availability: {self.jpegtran_available}") | ||
|
|
||
| def _check_jpegtran_availability(self) -> bool: | ||
| """Check if jpegtran is available in the system PATH.""" | ||
| try: | ||
| result = subprocess.run( | ||
| ["jpegtran", "-version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=JPEGTRAN_TIMEOUT_SECONDS, | ||
| ) | ||
| return result.returncode == 0 | ||
| except ( | ||
| subprocess.SubprocessError, | ||
| FileNotFoundError, | ||
| subprocess.TimeoutExpired, | ||
| ): | ||
| return False | ||
| pass | ||
|
|
There was a problem hiding this comment.
[nitpick] The __init__ method contains only a pass statement but has no actual initialization logic. Consider removing this method entirely since Python provides a default constructor, or add a docstring if this is intentionally left for future initialization code.
tests/test_image_rotator.py
Outdated
| assert isinstance(self.rotator, ImageRotator) | ||
| # jpegtran availability might vary by system | ||
| assert isinstance(self.rotator.jpegtran_available, bool) | ||
| # ImageRotator should initialize without issues |
There was a problem hiding this comment.
[nitpick] This comment is too generic and doesn't explain what specific behavior is being tested. Consider updating it to describe what aspects of initialization are being verified, such as 'ImageRotator should initialize without requiring external dependencies'.
| # ImageRotator should initialize without issues | |
| # Verify that ImageRotator initializes successfully without requiring external dependencies or configuration |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Fallback to pixel rotation | ||
| success = self._rotate_image_standard(image_path, direction) | ||
| method_used = "standard (lossy fallback)" | ||
| method_used = "standard JPEG (quality=95, lossy fallback)" |
There was a problem hiding this comment.
The hardcoded "quality=95" in the method description string doesn't match the actual quality setting used in _rotate_image_standard(). Either remove the specific quality value from the string or ensure it matches the actual implementation.
| method_used = "standard JPEG (quality=95, lossy fallback)" | |
| method_used = "standard JPEG (lossy fallback)" |
| """ | ||
| Handles image rotation with support for: | ||
| 1. Standard rotation for JPEG, PNG and other formats | ||
| 2. Metadata-only rotation for RAW formats (ARW, CR2, NEF, etc.) | ||
| 3. Metadata-only rotation for HEIF/HEIC formats | ||
| 4. XMP orientation metadata updates for all supported formats | ||
| """ |
There was a problem hiding this comment.
This module-level docstring is positioned after imports and before the class definition. It should be moved to the top of the file, right after the imports, to follow Python docstring conventions for module documentation.
duartebarbosadev
deleted the
fix-release-build-context-menu-on-images-and-rotation-doesn't-show-up
branch
This pull request refactors the image rotation logic to remove dependency on the external
jpegtrantool, streamlining rotation support and simplifying both the codebase and usage. JPEG rotation now prioritizes metadata-only (lossless) rotation, with standard pixel rotation as a fallback. The UI and tests are updated to reflect these changes, improving reliability and cross-platform compatibility.Image Rotation Logic Refactor:
jpegtranfor lossless JPEG rotation, including checks for its availability and the associated timeout constant inapp_settings.py. JPEG rotation now uses metadata-only rotation first, falling back to pixel rotation if necessary. (src/core/image_processing/image_rotator.py,src/core/app_settings.py) [1] [2] [3] [4]src/core/image_processing/image_rotator.py) [1] [2]User Interface Updates:
ImageRotatorfor rotation support checks, replacing the previous use ofMetadataProcessor. This allows for faster extension-based checks and avoids unnecessary PyExiv2 operations. (src/ui/menu_manager.py) [1] [2] [3]Testing Improvements:
jpegtranand to validate the new rotation logic, including metadata-first rotation for JPEGs and metadata-only rotation for RAW formats. New tests confirm correct method reporting and ensure file integrity for RAW formats. (tests/test_image_rotator.py) [1] [2]Documentation Updates:
jpegtranas a prerequisite, reflecting the simplified rotation support. (README.md)