feat: Add video browsing support and harden image-only analysis flows#65
Conversation
- Updated AppController to utilize new helper methods for managing image paths and data. - Introduced media type handling in AppState to differentiate between images and videos. - Enhanced PreviewController to filter and display only image paths. - Modified MainWindow to accommodate video files, including loading and displaying video previews. - Added video metadata handling in MetadataSidebar, including properties like resolution, duration, and bitrate. - Updated LoadingOverlay to support floating behavior based on video loading. - Adjusted worker manager signals to reflect changes in media type handling. - Refactored tests to align with new media type constants and ensure proper functionality.
- Added `classify_orientation` function to determine image orientation (portrait, landscape, square) using PIL and EXIF data. - Implemented `build_orientation_map` to create a mapping of file paths to their orientations. - Refactored date handling in `AppState` and related classes to use `datetime` instead of `date`. - Improved cluster management in `MenuManager` with the ability to move selected images to new clusters. - Enabled drag-and-drop functionality in `DroppableTreeView` for managing clusters in similarity mode. - Updated metadata display to show creation dates from EXIF data or filesystem attributes. - Enhanced unit tests to cover new datetime handling and cluster management features.
There was a problem hiding this comment.
Pull request overview
This PR adds mixed media (image + video) browsing support across PhotoSort while explicitly keeping analysis/rating flows image-only, and includes related robustness improvements (orientation-aware clustering + cache/logging hardening).
Changes:
- Add video scanning/browsing, first-frame thumbnails, basic video metadata, and in-viewer playback support.
- Harden “image-only” analysis flows (similarity/best-shot/blur/auto-rotate/ratings) by filtering videos and providing user feedback.
- Improve clustering determinism (RAW-aware orientation classification), manual cluster override safety, and cache/logging behavior.
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| todo.md | Adds internal TODO notes (non-runtime). |
| tests/test_similarity_prepare_clusters.py | Updates tests to use datetime-based date cache values. |
| tests/test_similarity_engine_helpers.py | Adds tests for RAW orientation classification + HF cache clearing. |
| tests/test_preview_controller.py | Ensures preview preloading skips videos. |
| tests/test_navigation_preferences.py | Adds navigation behavior tests for cluster entry points. |
| tests/test_metadata_processor_video.py | Verifies video metadata path skips pyexiv2 and writes cache payload. |
| tests/test_metadata_processor_helpers.py | Updates helper tests for datetime parsing behavior. |
| tests/test_image_pipeline_video_thumbnails.py | Tests first-frame video thumbnail + overlay and fallback behavior. |
| tests/test_file_scanner_automatic_raw.py | Updates scanner tests for renamed supported-extension constant. |
| tests/test_cluster_utils.py | Updates timestamp tests to datetime and adds filesystem fallback coverage. |
| tests/test_app_state_best_shots.py | Updates AppState cache-clearing expectations and introduces AnalysisCache patching. |
| tests/test_app_controller_video_support.py | Adds controller-level tests for mixed-media behavior and image-only operations. |
| src/workers/thumbnail_preload_worker.py | Updates docstrings to “media thumbnails”. |
| src/workers/rating_loader_worker.py | Adds throttled debug logging and updates wording to “media”. |
| src/ui/worker_manager.py | Updates signal comment to include media_type. |
| src/ui/ui_components.py | Enables drag/drop cluster management and adds loading-overlay positioning/flags. |
| src/ui/metadata_sidebar.py | Adds video-aware title + video properties card and creation-date handling. |
| src/ui/menu_manager.py | Adds context-menu cluster actions and parsing helpers. |
| src/ui/main_window.py | Adds video selection handling, viewer integration, timeline presentation, and throttled filter logging. |
| src/ui/left_panel.py | Enables drag/drop in the tree view for similarity cluster management. |
| src/ui/helpers/navigation_utils.py | Adjusts cluster navigation to enter at tail when moving “up”. |
| src/ui/helpers/cluster_utils.py | Adds cluster-id parsing, datetime resolution fallback, and datetime-based sorting. |
| src/ui/controllers/similarity_controller.py | Switches date cache type to datetime and parses cluster ids defensively. |
| src/ui/controllers/preview_controller.py | Filters previews to images only. |
| src/ui/app_state.py | Switches date_cache to datetime and adds optional disk-cache clearing. |
| src/ui/app_controller.py | Centralizes image/media filtering helpers and enforces image-only analysis ops with user messaging. |
| src/ui/advanced_image_viewer.py | Adds video playback UI and QMediaPlayer integration in the viewer. |
| src/main.py | Adds --last-folder startup option and adjusts model-cache initialization behavior. |
| src/core/video_metadata.py | Adds OpenCV-based basic video metadata extraction. |
| src/core/similarity_utils.py | Adds RAW-aware orientation classification and orientation map helper. |
| src/core/similarity_engine.py | Adds orientation-aware clustering flow + HF cache cleanup and explicit cache folder setting. |
| src/core/metadata_processor.py | Skips pyexiv2 for video, returns datetime, adds throttled logging, and caches basic video metadata. |
| src/core/media_utils.py | Introduces shared media extension sets and type inference helpers. |
| src/core/image_processing/raw_image_processor.py | Adds throttled summary logging for RAW preview/thumbnail paths. |
| src/core/image_pipeline.py | Adds first-frame video thumbnail extraction + play overlay and throttled preview-generation logging. |
| src/core/file_scanner.py | Adds mixed-media scanning with media_type and avoids blur detection for videos. |
| src/core/caching/exif_cache.py | Adds throttled ARW metadata caching logs. |
| src/core/caching/analysis_cache.py | Adds manual cluster override persistence utilities. |
| src/core/app_settings.py | Adds sentence-transformers cache dir constant and adjusts preview size estimation factor. |
| README.md | Documents video browsing support + --last-folder CLI option. |
| CLAUDE.md | Adds repository workflow/architecture guidance. |
| .gitignore | Adds .DS_Store. |
Comments suppressed due to low confidence (1)
src/main.py:29
_ensure_local_model_cache()now runs unconditionally, even in non-frozen/source runs, which will create amodels/directory under the project root and overridePHOTOSORT_MODELS_DIR/PYIQA_CACHE_DIRin development. If this is intended only for PyInstaller bundles (per the docstring), reintroduce theis_frozen_runtime()guard or update the function name/docstring and verify dev-mode cache paths won’t be redirected unexpectedly.
def _ensure_local_model_cache():
"""Point PyInstaller builds to the bundled models directory."""
base_dir = resolve_runtime_root(PROJECT_ROOT)
models_dir = os.path.abspath(os.path.join(base_dir, "models"))
os.makedirs(models_dir, exist_ok=True)
os.environ.setdefault("PHOTOSORT_MODELS_DIR", models_dir)
os.environ.setdefault("PYIQA_CACHE_DIR", models_dir)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ui/ui_components.py
Outdated
| # Clear previous highlight if different | ||
| if self.highlighted_drop_target_index != proxy_index: | ||
| self._clear_drop_highlight() | ||
|
|
||
| if not proxy_index.isValid(): | ||
| return | ||
|
|
||
| source_index = self.main_window.proxy_model.mapToSource(proxy_index) | ||
| item = self.main_window.file_system_model.itemFromIndex(source_index) | ||
| if not item: | ||
| return | ||
|
|
||
| item_data = item.data(Qt.ItemDataRole.UserRole) | ||
| if isinstance(item_data, str) and item_data.startswith("cluster_header_"): | ||
| if self.highlighted_drop_target_index != source_index: | ||
| self.original_item_brush = item.background() | ||
| self.highlighted_drop_target_index = source_index |
There was a problem hiding this comment.
highlighted_drop_target_index stores a source index (from mapToSource) but it’s compared against a proxy index (proxy_index) and later passed to self.model().itemFromIndex(...) (where self.model() is the proxy model). This can cause highlight flicker (always clearing) and can also crash because the proxy model typically doesn’t implement itemFromIndex. Consider storing proxy indices consistently, or store the QStandardItem / source index and always fetch via main_window.file_system_model.itemFromIndex(...) (and compare using the same index type).
| media_type = "video" if is_video_extension(ext) else "image" | ||
| file_info = { | ||
| "path": full_path, | ||
| "is_blurred": None, | ||
| "media_type": media_type, | ||
| } | ||
| all_file_data.append(file_info) | ||
| if media_type == "image": | ||
| image_paths_for_blur.append(full_path) | ||
|
|
||
| # Emit found file immediately (with no blur status yet) | ||
| file_info = {"path": full_path, "is_blurred": None} | ||
| self.files_found.emit([file_info]) | ||
| logger.debug(f"Found: {os.path.basename(full_path)}") |
There was a problem hiding this comment.
file_info dicts are emitted via files_found and then later mutated during blur detection (file_data["is_blurred"] = ...). Because signals cross threads and Python objects are passed by reference, this creates cross-thread mutation of objects that the UI thread may already be iterating over, which can lead to racey behavior. Safer pattern is to emit immutable copies during discovery and then emit a final results payload (or per-file blur updates) that the UI consumes, without mutating already-emitted objects.
| # Cluster Management (only in similarity mode) | ||
| if main_win.group_by_similarity_mode and self.app_state.cluster_results: | ||
| menu.addSeparator() | ||
| selected_paths = main_win._get_selected_file_paths_from_view() | ||
| num_selected = len(selected_paths) if len(selected_paths) > 1 else 1 | ||
| label_suffix = f" ({num_selected} images)" if num_selected > 1 else "" | ||
|
|
||
| move_to_new_cluster = QAction( | ||
| f"Move to New Cluster{label_suffix}", main_win | ||
| ) |
There was a problem hiding this comment.
The context-menu label suffix counts all selected paths, but the action only operates on is_image_extension(...) paths. This can show misleading text like “(2 images)” when one selection is a video. Consider counting only image selections (or renaming the label to “items”) to match what will actually be moved.
src/ui/menu_manager.py
Outdated
| """ | ||
| if isinstance(value, int): | ||
| return value | ||
| if isinstance(value, str): | ||
| # Try to extract the cluster ID from "1 - 87.34%" format | ||
| try: | ||
| return int(value.split(" - ")[0]) | ||
| except (ValueError, IndexError): | ||
| try: | ||
| return int(value) | ||
| except ValueError: | ||
| return None | ||
| return None |
There was a problem hiding this comment.
_parse_cluster_id duplicates parsing logic that now exists as ClusterUtils.parse_cluster_id. Duplicating this in multiple UI locations increases the risk of divergence if formats change again. Prefer calling the shared helper to keep parsing behavior consistent.
| """ | |
| if isinstance(value, int): | |
| return value | |
| if isinstance(value, str): | |
| # Try to extract the cluster ID from "1 - 87.34%" format | |
| try: | |
| return int(value.split(" - ")[0]) | |
| except (ValueError, IndexError): | |
| try: | |
| return int(value) | |
| except ValueError: | |
| return None | |
| return None | |
| Delegates to ClusterUtils.parse_cluster_id to keep parsing behavior | |
| consistent across the application. | |
| """ | |
| try: | |
| # Use the shared helper to avoid duplicating parsing logic. | |
| return ClusterUtils.parse_cluster_id(value) # type: ignore[name-defined] | |
| except Exception: | |
| logger.error("Failed to parse cluster ID from value %r", value, exc_info=True) | |
| return None |
todo.md
Outdated
| - Notify if there's changes to the folder (but do not notify if it was the user that deleted an image) and refresh if there is and keep similar position in view | ||
| - Scrolling up with the option to skip single clusters will not go to the last item of a group but instead go to the first item and is wrong | ||
|
|
||
| Local similarity ssave model, work without internet |
There was a problem hiding this comment.
Typo in todo item: "ssave" → "save".
todo.md
Outdated
| 2025-12-24 12:05:25 - DEBUG - [ui.main_window] - [main_window.py:3328] - eventFilter: key=16777249, modifiers=['Ctrl'], search_has_focus=False | ||
| 2025-12-24 12:05:25 - DEBUG - [ui.main_window] - [main_window.py:3417] - Key with other modifiers detected (passing to default handler): 16777249, modifiers: KeyboardModifier.ControlModifier | ||
|
|
||
| Whenever we search in clusters and delete the search thje clusters are all minimized. |
There was a problem hiding this comment.
Typo in todo item: "thje" → "the".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/main.py:27
_ensure_local_model_cache()now runs unconditionally, but its docstring still says it's for PyInstaller builds. Either restore the frozen-runtime guard or update the docstring/commentary to reflect that this is intended for source runs too (and why setting these env vars by default is safe).
def _ensure_local_model_cache():
"""Point PyInstaller builds to the bundled models directory."""
base_dir = resolve_runtime_root(PROJECT_ROOT)
models_dir = os.path.abspath(os.path.join(base_dir, "models"))
os.makedirs(models_dir, exist_ok=True)
os.environ.setdefault("PHOTOSORT_MODELS_DIR", models_dir)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Skipping pyexiv2 extraction for video: %s", | ||
| os.path.basename(operational_path), | ||
| ) | ||
| video_metadata = _build_basic_video_metadata(operational_path) |
There was a problem hiding this comment.
For videos, the cached metadata payload is built from operational_path, but the cache key used everywhere else is the NFC-normalized cache_key_path. If the file is only found via an NFD/NFC variant, this can store a payload whose file_path doesn't match the dictionary key / cache key, which is inconsistent for callers. Consider building video metadata using the canonical cache path (or storing both canonical + operational paths explicitly).
| video_metadata = _build_basic_video_metadata(operational_path) | |
| video_metadata = _build_basic_video_metadata(operational_path) | |
| # Ensure cached metadata uses the canonical cache key path as its primary file_path. | |
| if isinstance(video_metadata, dict): | |
| # Preserve any existing operational file path separately. | |
| if "file_path" in video_metadata and "operational_path" not in video_metadata: | |
| video_metadata["operational_path"] = video_metadata["file_path"] | |
| video_metadata["file_path"] = cache_key_path |
todo.md
Outdated
| # todo: | ||
|
|
||
| - Notify if there's changes to the folder (but do not notify if it was the user that deleted an image) and refresh if there is and keep similar position in view | ||
| - Scrolling up with the option to skip single clusters will not go to the last item of a group but instead go to the first item and is wrong | ||
|
|
||
| Local similarity save model, work without internet | ||
|
|
||
|
|
||
| stop logging this kinds of stuff | ||
| 2025-12-24 12:05:25 - DEBUG - [ui.main_window] - [main_window.py:3328] - eventFilter: key=16777249, modifiers=['Ctrl'], search_has_focus=False | ||
| 2025-12-24 12:05:25 - DEBUG - [ui.main_window] - [main_window.py:3417] - Key with other modifiers detected (passing to default handler): 16777249, modifiers: KeyboardModifier.ControlModifier | ||
|
|
There was a problem hiding this comment.
This looks like a local scratchpad (personal TODOs + pasted debug log lines) being added to the repo. Consider removing this file from the PR (or moving actionable items into issues/docs) to avoid committing transient debug output into version control.
src/ui/ui_components.py
Outdated
| item_data = item.data(Qt.ItemDataRole.UserRole) | ||
| return isinstance(item_data, str) and item_data.startswith("cluster_header_") |
There was a problem hiding this comment.
_is_cluster_drop_valid() treats any UserRole starting with cluster_header_ as a valid drop target, which will also match the "cluster_header_timeline" header. That causes the UI to accept/highlight the Timeline row during drag, but dropEvent() can't parse a numeric cluster id so the drop is ignored. Tighten the validity check to require a parseable numeric cluster id (or explicitly exclude the timeline header).
src/core/similarity_utils.py
Outdated
| getattr(sizes, "height", 0) or getattr(sizes, "iheight", 0) or 0 | ||
| ) | ||
| flip = int(getattr(sizes, "flip", 0) or 0) | ||
| if flip in {5, 6}: |
There was a problem hiding this comment.
RAW orientation handling swaps width/height only for flip values {5, 6}. Other rotated orientations (e.g. 7/8 in libraw/rawpy) also require swapping; otherwise portrait/landscape classification will be wrong for some RAW files.
| if flip in {5, 6}: | |
| if flip in {5, 6, 7, 8}: |
This PR adds first-class video browsing support across PhotoSort and tightens the boundaries between mixed-media browsing and image-only analysis.
What changed:
Added support for scanning and displaying common video formats.
Added video thumbnails using the first decodable frame with a play overlay.
Added video playback support in the advanced viewer.
Added basic video metadata handling and sidebar presentation.
Updated folder loading, thumbnail preload, and metadata loading to handle mixed image/video folders.
Kept similarity analysis, best-shot analysis, blur detection, auto-rotation, and ratings image-only, with explicit video filtering and user feedback.
Added manual-cluster safety so video files cannot be moved into image-analysis clusters.
Improved orientation-aware similarity clustering by using RAW-capable orientation detection instead of relying only on Pillow.
Fixed cache-clearing so Hugging Face cache cleanup works even when the embeddings cache directory is missing.