feat: Implement AI-powered Best Shot selection feature#56
feat: Implement AI-powered Best Shot selection feature#56duartebarbosadev wants to merge 1 commit intomainfrom
Conversation
- Added BestShotPicker and ClusterBestShotWorker to handle AI analysis for selecting the best images from user selections and similarity clusters. - Introduced new UI components in MainWindow and MenuManager for triggering best shot analysis. - Enhanced DeletionPresentation to include a "best shot" indicator. - Updated WorkerManager to manage the lifecycle of best shot analysis threads and workers. - Created tests for BestShotPicker functionality to ensure reliability and correctness.
There was a problem hiding this comment.
Pull Request Overview
This PR implements an AI-powered best shot selection feature that leverages vision-language models through OpenAI-compatible APIs (primarily LM Studio). Users can now select multiple images and have the application automatically identify the best shot based on technical quality criteria like sharpness, composition, and exposure.
Key Changes:
- New AI module with
BestShotPickerclass for vision model integration - Settings infrastructure for API configuration (URL, key, model, timeout)
- UI controllers and workers for background processing of single selections and entire similarity clusters
- Visual presentation layer showing AI-selected best shots with
[BEST]labels
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 28 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/ai/best_shot_picker.py |
Core AI logic for image analysis and best shot selection via OpenAI API |
src/core/ai/__init__.py |
Module exports for AI components |
src/core/app_settings.py |
Added persistent settings for API URL, key, model, and timeout |
src/workers/best_shot_picker_worker.py |
Background worker for analyzing single image selections |
src/workers/cluster_best_shot_worker.py |
Background worker for batch-processing similarity clusters |
src/ui/worker_manager.py |
Worker lifecycle management and preview cache integration |
src/ui/controllers/best_shot_picker_controller.py |
Controller for single-selection best shot workflow |
src/ui/controllers/cluster_best_shot_controller.py |
Controller for cluster-wide best shot analysis |
src/ui/menu_manager.py |
Added menu actions and keyboard shortcuts for best shot features |
src/ui/main_window.py |
Integration points for controllers and best shot label management |
src/ui/dialog_manager.py |
Added AI settings section to preferences dialog |
src/ui/helpers/ai_best_shot_settings.py |
Reusable settings widget for AI configuration |
src/ui/helpers/deletion_utils.py |
Enhanced text display to include [BEST] prefix |
src/ui/controllers/deletion_mark_controller.py |
Updated presentation logic to handle best shot state |
src/ui/app_state.py |
Added best_shot_paths tracking and path update handling |
src/ui/app_controller.py |
Menu state management for cluster best shot action |
tests/test_best_shot_picker.py |
Comprehensive test suite for BestShotPicker functionality |
README.md |
Documentation for AI Best Shot Picker setup and usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Analyze each image based on the following criteria: | ||
| - **Sharpness and Focus**: Is the subject in focus? Are there motion blur or focus issues? | ||
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. |
There was a problem hiding this comment.
Corrected spelling of 'Lightining' to 'Lighting'.
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | |
| - **Color/Lighting**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. |
| "Connection Error", | ||
| f"An error occurred while testing the connection:\n\n{str(e)}", | ||
| ) | ||
| logger.error(f"AI connection test error: {e}") |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.error('AI connection test error: %s', e) to avoid unnecessary string formatting when logging is disabled.
| logger.error(f"AI connection test error: {e}") | |
| logger.error("AI connection test error: %s", e) |
| f"AI Best Shot Picker settings saved: " | ||
| f"url={api_url}, model={model}, timeout={timeout}" |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.info('AI Best Shot Picker settings saved: url=%s, model=%s, timeout=%s', api_url, model, timeout) to avoid unnecessary string formatting when logging is disabled.
| f"AI Best Shot Picker settings saved: " | |
| f"url={api_url}, model={model}, timeout={timeout}" | |
| "AI Best Shot Picker settings saved: url=%s, model=%s, timeout=%s", | |
| api_url, model, timeout |
| f"Starting best shot analysis for {len(image_paths)} images " | ||
| f"using API: {api_url}" |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.info('Starting best shot analysis for %d images using API: %s', len(image_paths), api_url) to avoid unnecessary string formatting when logging is disabled.
| f"Starting best shot analysis for {len(image_paths)} images " | |
| f"using API: {api_url}" | |
| "Starting best shot analysis for %d images using API: %s", | |
| len(image_paths), | |
| api_url, |
| # Log the order of images being analyzed | ||
| for idx, path in enumerate(image_paths, 1): | ||
| from pathlib import Path | ||
| logger.info(f" Image {idx}: {Path(path).name}") |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.info(' Image %d: %s', idx, Path(path).name) to avoid unnecessary string formatting when logging is disabled.
| logger.info(f" Image {idx}: {Path(path).name}") | |
| logger.info(" Image %d: %s", idx, Path(path).name) |
|
|
||
| # Make API call | ||
| try: | ||
| logger.debug(f"Sending request to {self.base_url}") |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.debug('Sending request to %s', self.base_url) to avoid unnecessary string formatting when logging is disabled.
| logger.debug(f"Sending request to {self.base_url}") | |
| logger.debug("Sending request to %s", self.base_url) |
| ) | ||
|
|
||
| response_text = completion.choices[0].message.content | ||
| logger.debug(f"Received response: {response_text[:200]}...") |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.debug('Received response: %s...', response_text[:200]) to avoid unnecessary string formatting when logging is disabled.
| logger.debug(f"Received response: {response_text[:200]}...") | |
| logger.debug("Received response: %s...", response_text[:200]) |
| return result | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"API call failed: {e}") |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.error('API call failed: %s', e) to avoid unnecessary string formatting when logging is disabled.
| ], | ||
| max_tokens=10, | ||
| ) | ||
| logger.debug(f"Connection test successful: {response.choices[0].message.content if response.choices else 'No response'}") |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.debug('Connection test successful: %s', response.choices[0].message.content if response.choices else 'No response') to avoid unnecessary string formatting when logging is disabled.
| logger.debug(f"Connection test successful: {response.choices[0].message.content if response.choices else 'No response'}") | |
| logger.debug( | |
| "Connection test successful: %s", | |
| response.choices[0].message.content if response.choices else 'No response' | |
| ) |
| logger.debug(f"Connection test successful: {response.choices[0].message.content if response.choices else 'No response'}") | ||
| return True | ||
| except Exception as e: | ||
| logger.error(f"Connection test failed: {e}") |
There was a problem hiding this comment.
Use lazy string formatting with logger methods. Replace f-string with logger.error('Connection test failed: %s', e) to avoid unnecessary string formatting when logging is disabled.
| logger.error(f"Connection test failed: {e}") | |
| logger.error("Connection test failed: %s", e) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _get_image_url(self, image_path: str) -> str: | ||
| """ | ||
| Convert an image file path to a file:// URL or return existing URL. | ||
|
|
||
| Args: | ||
| image_path: Path to the image file or URL | ||
|
|
||
| Returns: | ||
| URL string for the image | ||
|
|
||
| Raises: | ||
| FileNotFoundError: If the image file doesn't exist | ||
| """ | ||
| # Check if it's already a URL (http/https) | ||
| if image_path.startswith(("http://", "https://")): | ||
| return image_path | ||
|
|
||
| # Convert local file path to file:// URL | ||
| try: | ||
| path = Path(image_path) | ||
| if not path.exists(): | ||
| raise FileNotFoundError(f"Image file not found: {image_path}") | ||
|
|
||
| # Convert to absolute path and create file:// URL | ||
| absolute_path = path.resolve() | ||
| return absolute_path.as_uri() | ||
| except Exception as e: | ||
| raise BestShotPickerError(f"Failed to create URL for image {image_path}: {e}") | ||
|
|
There was a problem hiding this comment.
The _get_image_url method is defined but never used in the codebase. Consider removing it to reduce code complexity, or add a comment explaining its purpose for future use.
| def _get_image_url(self, image_path: str) -> str: | |
| """ | |
| Convert an image file path to a file:// URL or return existing URL. | |
| Args: | |
| image_path: Path to the image file or URL | |
| Returns: | |
| URL string for the image | |
| Raises: | |
| FileNotFoundError: If the image file doesn't exist | |
| """ | |
| # Check if it's already a URL (http/https) | |
| if image_path.startswith(("http://", "https://")): | |
| return image_path | |
| # Convert local file path to file:// URL | |
| try: | |
| path = Path(image_path) | |
| if not path.exists(): | |
| raise FileNotFoundError(f"Image file not found: {image_path}") | |
| # Convert to absolute path and create file:// URL | |
| absolute_path = path.resolve() | |
| return absolute_path.as_uri() | |
| except Exception as e: | |
| raise BestShotPickerError(f"Failed to create URL for image {image_path}: {e}") |
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | ||
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | ||
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | ||
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | ||
| - **Overall Appeal**: Which image is most visually appealing? | ||
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. | ||
| - **Subject Sharpness** – focus quality, motion blur, and clarity of the main subject. |
There was a problem hiding this comment.
[nitpick] Inconsistent punctuation in the prompt criteria list. Line 280 uses a colon followed by an em dash, line 285 uses a colon followed by an em dash, but line 286 uses only an em dash. Consider standardizing to use colon followed by description for consistency.
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | |
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | |
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | |
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | |
| - **Overall Appeal**: Which image is most visually appealing? | |
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. | |
| - **Subject Sharpness** – focus quality, motion blur, and clarity of the main subject. | |
| - **Color/Lightining**: Color & Lighting accuracy, contrast, saturation, white balance, and visual appeal. | |
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | |
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | |
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | |
| - **Overall Appeal**: Which image is most visually appealing? | |
| - **Editing Potential**: How well the image could respond to color grading, retouching, or enhancement. | |
| - **Subject Sharpness**: Focus quality, motion blur, and clarity of the main subject. |
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | ||
| - **Overall Appeal**: Which image is most visually appealing? | ||
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. | ||
| - **Subject Sharpness** – focus quality, motion blur, and clarity of the main subject. |
There was a problem hiding this comment.
[nitpick] Inconsistent punctuation in the prompt criteria list. Line 280 uses a colon followed by an em dash, line 285 uses a colon followed by an em dash, but line 286 uses only an em dash. Consider standardizing to use colon followed by description for consistency.
| - **Subject Sharpness** – focus quality, motion blur, and clarity of the main subject. | |
| - **Subject Sharpness**: focus quality, motion blur, and clarity of the main subject. |
| - **Sharpness and Focus**: Is the subject in focus? Are there motion blur or focus issues? | ||
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | ||
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | ||
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | ||
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | ||
| - **Overall Appeal**: Which image is most visually appealing? | ||
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. | ||
| - **Subject Sharpness** – focus quality, motion blur, and clarity of the main subject. |
There was a problem hiding this comment.
[nitpick] The criteria list has redundant entries for sharpness/focus. Line 279 covers 'Sharpness and Focus' and line 286 covers 'Subject Sharpness' with overlapping concepts (focus quality, motion blur). Consider consolidating these into a single, comprehensive criterion to avoid repetition.
| - **Sharpness and Focus**: Is the subject in focus? Are there motion blur or focus issues? | |
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | |
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | |
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | |
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | |
| - **Overall Appeal**: Which image is most visually appealing? | |
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. | |
| - **Subject Sharpness** – focus quality, motion blur, and clarity of the main subject. | |
| - **Sharpness and Focus**: Is the image sharp overall, with the main subject in clear focus? Consider focus quality, motion blur, and clarity of the main subject. | |
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | |
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | |
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | |
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | |
| - **Overall Appeal**: Which image is most visually appealing? | |
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. |
| - **Sharpness and Focus**: Is the subject in focus? Are there motion blur or focus issues? | ||
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | ||
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | ||
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | ||
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | ||
| - **Overall Appeal**: Which image is most visually appealing? | ||
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. | ||
| - **Subject Sharpness** – focus quality, motion blur, and clarity of the main subject. |
There was a problem hiding this comment.
[nitpick] The criteria list has redundant entries for sharpness/focus. Line 279 covers 'Sharpness and Focus' and line 286 covers 'Subject Sharpness' with overlapping concepts (focus quality, motion blur). Consider consolidating these into a single, comprehensive criterion to avoid repetition.
| - **Sharpness and Focus**: Is the subject in focus? Are there motion blur or focus issues? | |
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | |
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | |
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | |
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | |
| - **Overall Appeal**: Which image is most visually appealing? | |
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. | |
| - **Subject Sharpness** – focus quality, motion blur, and clarity of the main subject. | |
| - **Sharpness and Focus**: Is the image and its main subject in clear focus? Consider overall sharpness, focus quality, motion blur, and clarity of the main subject. | |
| - **Color/Lightining**: Color & Lighting – accuracy, contrast, saturation, white balance, and visual appeal. | |
| - **Composition**: Does the image follow framing, subject placement, use of space principles? | |
| - **Subject Expression**: For portraits, does the subject have a good expression (eyes open, natural smile)? | |
| - **Technical Quality**: Are there any artifacts, noise, or technical issues? | |
| - **Overall Appeal**: Which image is most visually appealing? | |
| - **Editing Potential**: – how well the image could respond to color grading, retouching, or enhancement. |
| """ | ||
|
|
||
| import logging | ||
| from PyQt6.QtCore import Qt, pyqtSignal |
There was a problem hiding this comment.
Import of 'Qt' is not used.
| from PyQt6.QtCore import Qt, pyqtSignal | |
| from PyQt6.QtCore import pyqtSignal |
|
|
||
| from PyQt6.QtCore import QObject, pyqtSignal | ||
|
|
||
| from core.ai.best_shot_picker import BestShotPicker, BestShotPickerError, BestShotResult |
There was a problem hiding this comment.
Import of 'BestShotResult' is not used.
| from core.ai.best_shot_picker import BestShotPicker, BestShotPickerError, BestShotResult | |
| from core.ai.best_shot_picker import BestShotPicker, BestShotPickerError |
| Tests for AI Best Shot Picker functionality | ||
| """ | ||
|
|
||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| import os |
|
|
||
| import os | ||
| import pytest | ||
| from unittest.mock import Mock, patch, MagicMock |
There was a problem hiding this comment.
Import of 'Mock' is not used.
| from unittest.mock import Mock, patch, MagicMock | |
| from unittest.mock import patch, MagicMock |
| import os | ||
| import pytest | ||
| from unittest.mock import Mock, patch, MagicMock | ||
| from pathlib import Path |
There was a problem hiding this comment.
Import of 'Path' is not used.
| from pathlib import Path |
This pull request introduces the new AI-powered "Best Shot Picker" feature to PhotoSort, enabling users to automatically select the best image from a group using a vision-language model via an OpenAI-compatible API. The changes span documentation, core logic, app settings, UI state, and controller integration to provide a seamless user experience for configuring and running best shot analysis.
AI Best Shot Picker Feature:
BestShotPickerController), progress/result dialogs, and integration with the main window and worker manager.core.aimodule withBestShotPicker,BestShotResult, and error handling, encapsulating the AI logic and API interaction for the best shot feature.Settings and Configuration:
UI and State Management:
AppState) to track which images are flagged as AI-selected best shots, and ensured this state is correctly cleared or updated as needed (e.g., when files are removed or renamed). [1] [2] [3] [4]Documentation:
README.mdto document the new AI Best Shot Picker feature, including setup instructions, configuration options, and usage steps for end users. [1] [2]