From c34d0bdd6b0a9ff36675715898d237d4bd19470e Mon Sep 17 00:00:00 2001 From: Andy Arijs Date: Tue, 17 Mar 2026 19:27:08 +0100 Subject: [PATCH 1/8] Fix sidecar metadata lookup to use stable image path keys --- faststack/app.py | 85 +++++++------- faststack/io/sidecar.py | 105 ++++++++++++++++-- faststack/logging_setup.py | 51 ++++++++- faststack/tests/test_jump_to_last_uploaded.py | 16 ++- faststack/tests/test_sidecar.py | 93 ++++++++++++++-- .../tests/thumbnail_view/test_folder_stats.py | 12 ++ faststack/thumbnail_view/folder_stats.py | 4 +- faststack/thumbnail_view/model.py | 20 ++-- 8 files changed, 304 insertions(+), 82 deletions(-) diff --git a/faststack/app.py b/faststack/app.py index 37efddc..304bd33 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -1040,13 +1040,9 @@ def _apply_filter_to_cached_list(self): # Apply flag-based filtering (AND logic: image must have ALL checked flags) if self._filter_enabled and self._filter_flags: flags = self._filter_flags - # Optimize: access sidecar entries directly to avoid get_metadata overhead - entries = self.sidecar.data.entries result = [] for img in filtered: - # Direct dict lookup is faster than get_metadata() which might create objects - stem = img.path.stem - meta = entries.get(stem) + meta = self.sidecar.get_metadata(img.path, create=False) if not meta: continue @@ -1810,7 +1806,7 @@ def jump_to_last_uploaded(self): for idx in range(len(self.image_files) - 1, -1, -1): img = self.image_files[idx] # Dynamic look-up of self.sidecar as requested (important for mocks in tests) - meta = self.sidecar.get_metadata(img.path.stem, create=False) + meta = self.sidecar.get_metadata(img.path, create=False) uploaded = meta.uploaded if meta else False @@ -2126,10 +2122,10 @@ def _on_thumbnail_ready_gui(self, thumbnail_id: str): if self._thumbnail_model: self._thumbnail_model.thumbnailReady.emit(thumbnail_id) - def _get_metadata_dict(self, stem: str) -> dict: - """Get metadata for a file stem as a dict for thumbnail model.""" + def _get_metadata_dict(self, image_path: Path | str) -> dict: + """Get metadata for an image path as a dict for thumbnail model.""" try: - meta = self.sidecar.get_metadata(stem, create=False) + meta = self.sidecar.get_metadata(image_path, create=False) if meta is None: return { "stacked": False, @@ -2148,7 +2144,7 @@ def _get_metadata_dict(self, stem: str) -> dict: "todo": meta.todo, } except Exception as e: # Broad catch for UI plumbing - don't crash grid view - log.debug("Failed to get metadata for %s: %s", stem, e) + log.debug("Failed to get metadata for %s: %s", image_path, e) return { "stacked": False, "uploaded": False, @@ -2162,9 +2158,11 @@ def _get_bulk_metadata_map(self) -> Dict[str, dict]: """Get flattened metadata map for all images (for efficient grid refresh).""" bulk_map = {} try: - # sidecar.data.entries is a dict of stem -> EntryMetadata - for stem, meta in self.sidecar.data.entries.items(): - bulk_map[stem] = { + for img in self.image_files: + meta = self.sidecar.get_metadata(img.path, create=False) + if meta is None: + continue + bulk_map[normalize_path_key(img.path)] = { "stacked": getattr(meta, "stacked", False), "uploaded": getattr(meta, "uploaded", False), "edited": getattr(meta, "edited", False), @@ -2214,8 +2212,8 @@ def toggle_uploaded(self): return today = datetime.now().strftime("%Y-%m-%d") - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem) + image_path = self.image_files[self.current_index].path + meta = self.sidecar.get_metadata(image_path) meta.uploaded = not meta.uploaded if meta.uploaded: @@ -2229,7 +2227,7 @@ def toggle_uploaded(self): self.sync_ui_state() status = "uploaded" if meta.uploaded else "not uploaded" self.update_status_message(f"Marked as {status}") - log.info("Toggled uploaded flag to %s for %s", meta.uploaded, stem) + log.info("Toggled uploaded flag to %s for %s", meta.uploaded, image_path) def toggle_todo(self): """Toggle todo flag for current image.""" @@ -2237,8 +2235,8 @@ def toggle_todo(self): return today = datetime.now().strftime("%Y-%m-%d") - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem) + image_path = self.image_files[self.current_index].path + meta = self.sidecar.get_metadata(image_path) meta.todo = not getattr(meta, "todo", False) if meta.todo: @@ -2252,7 +2250,7 @@ def toggle_todo(self): self.sync_ui_state() status = "todo" if meta.todo else "not todo" self.update_status_message(f"Marked as {status}") - log.info("Toggled todo flag to %s for %s", meta.todo, stem) + log.info("Toggled todo flag to %s for %s", meta.todo, image_path) def toggle_edited(self): """Toggle edited flag for current image.""" @@ -2260,8 +2258,8 @@ def toggle_edited(self): return today = datetime.now().strftime("%Y-%m-%d") - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem) + image_path = self.image_files[self.current_index].path + meta = self.sidecar.get_metadata(image_path) meta.edited = not meta.edited if meta.edited: @@ -2275,7 +2273,7 @@ def toggle_edited(self): self.sync_ui_state() status = "edited" if meta.edited else "not edited" self.update_status_message(f"Marked as {status}") - log.info("Toggled edited flag to %s for %s", meta.edited, stem) + log.info("Toggled edited flag to %s for %s", meta.edited, image_path) def toggle_restacked(self): """Toggle restacked flag for current image.""" @@ -2283,8 +2281,8 @@ def toggle_restacked(self): return today = datetime.now().strftime("%Y-%m-%d") - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem) + image_path = self.image_files[self.current_index].path + meta = self.sidecar.get_metadata(image_path) meta.restacked = not meta.restacked if meta.restacked: @@ -2298,15 +2296,15 @@ def toggle_restacked(self): self.sync_ui_state() status = "restacked" if meta.restacked else "not restacked" self.update_status_message(f"Marked as {status}") - log.info("Toggled restacked flag to %s for %s", meta.restacked, stem) + log.info("Toggled restacked flag to %s for %s", meta.restacked, image_path) def toggle_favorite(self): """Toggle favorite flag for current image.""" if not self.image_files or self.current_index >= len(self.image_files): return - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem) + image_path = self.image_files[self.current_index].path + meta = self.sidecar.get_metadata(image_path) meta.favorite = not meta.favorite @@ -2316,7 +2314,7 @@ def toggle_favorite(self): self.sync_ui_state() status = "Favorited" if meta.favorite else "Unfavorited" self.update_status_message(status) - log.info("Toggled favorite flag to %s for %s", meta.favorite, stem) + log.info("Toggled favorite flag to %s for %s", meta.favorite, image_path) def toggle_stacked(self): """Toggle stacked flag for current image.""" @@ -2324,8 +2322,8 @@ def toggle_stacked(self): return today = datetime.now().strftime("%Y-%m-%d") - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem) + image_path = self.image_files[self.current_index].path + meta = self.sidecar.get_metadata(image_path) meta.stacked = not meta.stacked if meta.stacked: @@ -2339,7 +2337,7 @@ def toggle_stacked(self): self.sync_ui_state() status = "stacked" if meta.stacked else "not stacked" self.update_status_message(f"Marked as {status}") - log.info("Toggled stacked flag to %s for %s", meta.stacked, stem) + log.info("Toggled stacked flag to %s for %s", meta.stacked, image_path) def get_current_metadata(self) -> Dict: if not self.image_files or self.current_index >= len(self.image_files): @@ -2357,8 +2355,8 @@ def get_current_metadata(self) -> Dict: return self._metadata_cache # Compute and cache - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem, create=False) + image_path = self.image_files[self.current_index].path + meta = self.sidecar.get_metadata(image_path, create=False) stack_info = self._get_stack_info(self.current_index) batch_info = self._get_batch_info(self.current_index) @@ -2619,7 +2617,7 @@ def add_favorites_to_batch(self): # Find indices of all favorited images indices_to_add = [] for i, img in enumerate(self.image_files): - meta = self.sidecar.get_metadata(img.path.stem, create=False) + meta = self.sidecar.get_metadata(img.path, create=False) if meta and meta.favorite: indices_to_add.append(i) @@ -2674,7 +2672,7 @@ def add_uploaded_to_batch(self): # Find indices of all uploaded images indices_to_add = [] for i, img in enumerate(self.image_files): - meta = self.sidecar.get_metadata(img.path.stem, create=False) + meta = self.sidecar.get_metadata(img.path, create=False) if meta and meta.uploaded: indices_to_add.append(i) @@ -3099,8 +3097,7 @@ def _launch_helicon_with_files(self, files: List[Path]) -> bool: for img_file in self.image_files: # Match by either RAW pair or JPG path if img_file.raw_pair == file_path or img_file.path == file_path: - stem = img_file.path.stem - meta = self.sidecar.get_metadata(stem) + meta = self.sidecar.get_metadata(img_file.path) meta.stacked = True meta.stacked_date = today break @@ -5448,8 +5445,7 @@ def edit_in_photoshop(self): # Mark as edited on successful launch today = datetime.now().strftime("%Y-%m-%d") - stem = image_file.path.stem - meta = self.sidecar.get_metadata(stem) + meta = self.sidecar.get_metadata(image_file.path) meta.edited = True meta.edited_date = today self.sidecar.save() @@ -5583,8 +5579,7 @@ def start_drag_current_image(self): today = datetime.now().strftime("%Y-%m-%d") for idx in existing_indices: - stem = self.image_files[idx].path.stem - meta = self.sidecar.get_metadata(stem) + meta = self.sidecar.get_metadata(self.image_files[idx].path) meta.uploaded = True meta.uploaded_date = today @@ -6456,8 +6451,7 @@ def stack_source_raws(self): if success: # Mark as restacked on success today = datetime.now().strftime("%Y-%m-%d") - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem) + meta = self.sidecar.get_metadata(self.image_files[self.current_index].path) meta.restacked = True meta.restacked_date = today self.sidecar.save() @@ -7363,8 +7357,9 @@ def get_stack_summary(self) -> str: def is_stacked(self) -> bool: if not self.image_files or self.current_index >= len(self.image_files): return False - stem = self.image_files[self.current_index].path.stem - meta = self.sidecar.get_metadata(stem, create=False) + meta = self.sidecar.get_metadata( + self.image_files[self.current_index].path, create=False + ) return meta.stacked if meta else False def _update_cache_stats(self): diff --git a/faststack/io/sidecar.py b/faststack/io/sidecar.py index c1adcf0..7371d5f 100644 --- a/faststack/io/sidecar.py +++ b/faststack/io/sidecar.py @@ -2,9 +2,10 @@ import json import logging +import os import time from pathlib import Path -from typing import Literal, Optional, overload +from typing import Literal, Optional, Union, overload from faststack.models import Sidecar, EntryMetadata @@ -37,6 +38,7 @@ def _entrymetadata_from_json(meta: dict) -> EntryMetadata: class SidecarManager: def __init__(self, directory: Path, watcher, debug: bool = False): + self.directory = directory self.path = directory / "faststack.json" self.watcher = watcher self.debug = debug @@ -71,8 +73,8 @@ def load(self) -> Sidecar: # Reconstruct nested objects entries = { - stem: _entrymetadata_from_json(meta) - for stem, meta in data.get("entries", {}).items() + key: _entrymetadata_from_json(meta) + for key, meta in data.get("entries", {}).items() } return Sidecar( version=data.get("version", 2), @@ -103,7 +105,7 @@ def save(self): "version": self.data.version, "last_index": self.data.last_index, "entries": { - stem: meta.__dict__ for stem, meta in self.data.entries.items() + key: meta.__dict__ for key, meta in self.data.entries.items() }, "stacks": self.data.stacks, } @@ -121,21 +123,21 @@ def save(self): @overload def get_metadata( - self, image_stem: str, *, create: Literal[True] = True + self, image_ref: Union[str, Path], *, create: Literal[True] = True ) -> EntryMetadata: ... @overload def get_metadata( - self, image_stem: str, *, create: Literal[False] + self, image_ref: Union[str, Path], *, create: Literal[False] ) -> Optional[EntryMetadata]: ... @overload def get_metadata( - self, image_stem: str, *, create: bool + self, image_ref: Union[str, Path], *, create: bool ) -> Optional[EntryMetadata]: ... def get_metadata( - self, image_stem: str, *, create: bool = True + self, image_ref: Union[str, Path], *, create: bool = True ) -> Optional[EntryMetadata]: """Get metadata for an image, optionally creating a persistent entry. @@ -143,11 +145,94 @@ def get_metadata( and storing one if it doesn't exist). When create=False, returns None if no entry exists — callers must handle the None case explicitly. """ - meta = self.data.entries.get(image_stem) + stable_key, candidate_keys = self._lookup_keys(image_ref) + + meta = self.data.entries.get(stable_key) + if meta is None: + for candidate_key in candidate_keys: + if candidate_key == stable_key: + continue + candidate_meta = self.data.entries.get(candidate_key) + if candidate_meta is None: + continue + meta = candidate_meta + if stable_key not in self.data.entries: + self.data.entries[stable_key] = candidate_meta + if candidate_key in self.data.entries and candidate_key != stable_key: + del self.data.entries[candidate_key] + break + if meta is None: + for existing_key, existing_meta in list(self.data.entries.items()): + if existing_key == stable_key: + continue + if self._stable_key_from_key(existing_key) != stable_key: + continue + meta = existing_meta + self.data.entries[stable_key] = existing_meta + del self.data.entries[existing_key] + break + if meta is None and create: meta = EntryMetadata() - self.data.entries[image_stem] = meta + self.data.entries[stable_key] = meta return meta + def metadata_key_for_path(self, image_path: Union[str, Path]) -> str: + """Return the stable sidecar key for a concrete image path.""" + path = Path(image_path) + if not path.is_absolute(): + path = self.directory / path + base_dir = Path(os.path.abspath(str(self.directory))) + abs_path = Path(os.path.abspath(str(path))) + + try: + relative = abs_path.relative_to(base_dir) + stable_path = relative.parent / relative.stem + return str(stable_path).replace("\\", "/") + except ValueError: + stable_path = abs_path.parent / abs_path.stem + return str(stable_path).replace("\\", "/") + + def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: + """Return (stable_key, migration_candidate_keys) for a metadata lookup.""" + if isinstance(image_ref, Path): + stable_key = self.metadata_key_for_path(image_ref) + full_name_key = self._metadata_filename_key(image_ref) + return stable_key, [full_name_key, image_ref.stem] + + value = str(image_ref) + if not value: + return "", [] + + if os.path.sep in value or "/" in value or "\\" in value or "." in Path(value).name: + path = Path(value) + stable_key = self.metadata_key_for_path(path) + full_name_key = self._metadata_filename_key(path) + return stable_key, [full_name_key, path.stem] + + return value, [value] + + def _metadata_filename_key(self, image_path: Union[str, Path]) -> str: + """Return the extension-preserving key used by the regressed patch.""" + path = Path(image_path) + if not path.is_absolute(): + path = self.directory / path + base_dir = Path(os.path.abspath(str(self.directory))) + abs_path = Path(os.path.abspath(str(path))) + + try: + relative = abs_path.relative_to(base_dir) + return str(relative).replace("\\", "/") + except ValueError: + return str(abs_path).replace("\\", "/") + + def _stable_key_from_key(self, key: str) -> str: + """Convert any historical sidecar key form into today's stable key.""" + if not key: + return "" + if os.path.sep in key or "/" in key or "\\" in key or "." in Path(key).name: + return self.metadata_key_for_path(Path(key)) + return key + def set_last_index(self, index: int): self.data.last_index = index diff --git a/faststack/logging_setup.py b/faststack/logging_setup.py index ab684a4..e783d94 100644 --- a/faststack/logging_setup.py +++ b/faststack/logging_setup.py @@ -6,12 +6,57 @@ from pathlib import Path +def _is_writable_dir(path: Path) -> bool: + """Return True when an existing directory accepts file writes.""" + if not path.is_dir(): + return False + + try: + probe = path / ".write_test" + with probe.open("w", encoding="utf-8") as f: + f.write("ok") + probe.unlink(missing_ok=True) + return True + except OSError: + return False + + +def _can_create_dir(path: Path) -> bool: + """Return True when the nearest existing parent is writable.""" + parent = path + while not parent.exists(): + next_parent = parent.parent + if next_parent == parent: + return False + parent = next_parent + + return parent.is_dir() and os.access(parent, os.W_OK) def get_app_data_dir() -> Path: - """Returns the application data directory.""" + """Return a writable application data directory, with fallbacks.""" + candidates = [] + app_data = os.getenv("APPDATA") if app_data: - return Path(app_data) / "faststack" - return Path.home() / ".faststack" + candidates.append(Path(app_data) / "faststack") + + local_app_data = os.getenv("LOCALAPPDATA") + if local_app_data: + candidates.append(Path(local_app_data) / "faststack") + + candidates.append(Path.home() / ".faststack") + candidates.append(Path.cwd() / "var" / "appdata") + + for candidate in candidates: + if _is_writable_dir(candidate): + return candidate + + for candidate in candidates: + if _can_create_dir(candidate): + return candidate + + # Final fallback: return the first candidate even if unwritable so callers + # still get a deterministic location for error reporting. + return candidates[0] if candidates else Path.cwd() / "var" / "appdata" def setup_logging(debug: bool = False): diff --git a/faststack/tests/test_jump_to_last_uploaded.py b/faststack/tests/test_jump_to_last_uploaded.py index 524d8a3..80e21da 100644 --- a/faststack/tests/test_jump_to_last_uploaded.py +++ b/faststack/tests/test_jump_to_last_uploaded.py @@ -66,8 +66,12 @@ def test_jump_to_last_uploaded_success(mock_controller): meta2 = EntryMetadata(uploaded=False) meta3 = EntryMetadata(uploaded=True) - def side_effect(stem, **kwargs): - return {"img1": meta1, "img2": meta2, "img3": meta3}.get(stem, EntryMetadata()) + def side_effect(path, **kwargs): + return { + Path("img1.jpg"): meta1, + Path("img2.jpg"): meta2, + Path("img3.jpg"): meta3, + }.get(Path(path), EntryMetadata()) mock_controller.sidecar.get_metadata.side_effect = side_effect @@ -137,8 +141,12 @@ def test_jump_to_last_uploaded_one(mock_controller): mock_controller.image_files = [img1, img2, img3] mock_controller.current_index = 0 - def side_effect(stem, **kwargs): - return {"img1": meta1, "img2": meta2, "img3": meta3}.get(stem, EntryMetadata()) + def side_effect(path, **kwargs): + return { + Path("img1.jpg"): meta1, + Path("img2.jpg"): meta2, + Path("img3.jpg"): meta3, + }.get(Path(path), EntryMetadata()) mock_controller.sidecar.get_metadata.side_effect = side_effect diff --git a/faststack/tests/test_sidecar.py b/faststack/tests/test_sidecar.py index 2c97eae..1334dac 100644 --- a/faststack/tests/test_sidecar.py +++ b/faststack/tests/test_sidecar.py @@ -61,7 +61,7 @@ def test_sidecar_save(mock_sidecar_dir): # Modify data sm.set_last_index(10) - meta = sm.get_metadata("IMG_TEST") + meta = sm.get_metadata(Path("IMG_TEST.jpg")) # Modify a valid field meta.stack_id = 99 @@ -78,8 +78,8 @@ def test_sidecar_get_metadata_creates_new(mock_sidecar_dir): """Tests that get_metadata creates a new entry if one doesn't exist.""" d = mock_sidecar_dir() sm = SidecarManager(d, None) - assert "NEW_IMG" not in sm.data.entries - meta = sm.get_metadata("NEW_IMG") + assert "NEW_IMG.jpg" not in sm.data.entries + meta = sm.get_metadata(Path("NEW_IMG.jpg")) # EntryMetadata may be a runtime class OR a typing alias, depending on refactors. if isinstance(EntryMetadata, type): @@ -96,7 +96,7 @@ def test_favorite_toggle_sets_json(mock_sidecar_dir): """Tests that toggling favorite writes true/false to JSON.""" d = mock_sidecar_dir() sm = SidecarManager(d, None) - meta = sm.get_metadata("IMG_FAV") + meta = sm.get_metadata(Path("IMG_FAV.jpg")) # Initially false assert meta.favorite is False @@ -120,12 +120,12 @@ def test_favorite_loads_from_sidecar(mock_sidecar_dir): "version": 2, "last_index": 0, "entries": { - "IMG_FAV": {"favorite": True}, + "IMG_FAV.jpg": {"favorite": True}, }, } d = mock_sidecar_dir(content) sm = SidecarManager(d, None) - meta = sm.get_metadata("IMG_FAV") + meta = sm.get_metadata(Path("IMG_FAV.jpg")) assert meta.favorite is True @@ -133,7 +133,7 @@ def test_favorite_toggle_roundtrip(mock_sidecar_dir): """Tests that toggling twice restores original JSON (round-trip).""" d = mock_sidecar_dir() sm = SidecarManager(d, None) - meta = sm.get_metadata("IMG_FAV") + meta = sm.get_metadata(Path("IMG_FAV.jpg")) # Capture original state assert meta.favorite is False @@ -145,5 +145,82 @@ def test_favorite_toggle_roundtrip(mock_sidecar_dir): # Reload and verify sm2 = SidecarManager(d, None) - meta2 = sm2.get_metadata("IMG_FAV") + meta2 = sm2.get_metadata(Path("IMG_FAV.jpg")) assert meta2.favorite is False + + +def test_legacy_stem_entry_migrates_to_path_key(mock_sidecar_dir): + """Legacy stem-keyed entries should migrate on first concrete path lookup.""" + content = { + "version": 2, + "entries": { + "IMG_0001": {"uploaded": True}, + }, + } + d = mock_sidecar_dir(content) + sm = SidecarManager(d, None) + + meta = sm.get_metadata(Path("IMG_0001.jpg"), create=False) + + assert meta is not None + assert meta.uploaded is True + assert "IMG_0001" in sm.data.entries + assert "IMG_0001.jpg" not in sm.data.entries + assert sm.metadata_key_for_path(Path("IMG_0001.jpg")) == "IMG_0001" + + +def test_raw_only_entry_survives_transition_to_visible_jpg(mock_sidecar_dir): + """RAW-only metadata must remain visible after a matching JPG appears.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + + raw_meta = sm.get_metadata(Path("photo.CR2")) + raw_meta.favorite = True + raw_meta.uploaded = True + + jpg_meta = sm.get_metadata(Path("photo.jpg"), create=False) + + assert jpg_meta is raw_meta + assert jpg_meta.favorite is True + assert jpg_meta.uploaded is True + assert list(sm.data.entries) == ["photo"] + + +def test_regressed_filename_key_migrates_to_stable_stem_key(mock_sidecar_dir): + """Entries created by the filename-key regression should migrate back.""" + content = { + "version": 2, + "entries": { + "photo.CR2": {"favorite": True}, + }, + } + d = mock_sidecar_dir(content) + sm = SidecarManager(d, None) + + meta = sm.get_metadata(Path("photo.jpg"), create=False) + + assert meta is not None + assert meta.favorite is True + assert "photo" in sm.data.entries + assert "photo.CR2" not in sm.data.entries + assert "photo.jpg" not in sm.data.entries + + +def test_same_stem_in_different_subfolders_do_not_collide(mock_sidecar_dir): + """Relative parent path must namespace same-stem files in different folders.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + + first = sm.get_metadata(Path("a") / "photo.CR2") + first.favorite = True + + second = sm.get_metadata(Path("b") / "photo.jpg") + second.uploaded = True + + assert first is not second + assert sm.metadata_key_for_path(Path("a") / "photo.CR2") == "a/photo" + assert sm.metadata_key_for_path(Path("b") / "photo.jpg") == "b/photo" + assert sm.data.entries["a/photo"].favorite is True + assert sm.data.entries["a/photo"].uploaded is False + assert sm.data.entries["b/photo"].favorite is False + assert sm.data.entries["b/photo"].uploaded is True diff --git a/faststack/tests/thumbnail_view/test_folder_stats.py b/faststack/tests/thumbnail_view/test_folder_stats.py index 90866e9..fb36a10 100644 --- a/faststack/tests/thumbnail_view/test_folder_stats.py +++ b/faststack/tests/thumbnail_view/test_folder_stats.py @@ -344,6 +344,18 @@ def test_coverage_buckets_in_stats(self, temp_folder): # With 2 files and default 40 buckets, should have 2 buckets assert len(stats.coverage_buckets) == 2 + def test_coverage_buckets_support_path_keys(self, temp_folder): + """Path-aware sidecar keys should still contribute to sparkline coverage.""" + (temp_folder / "a.jpg").touch() + json_path = temp_folder / "faststack.json" + data = {"entries": {"a.jpg": {"uploaded": True}}} + json_path.write_text(json.dumps(data)) + + stats = read_folder_stats(temp_folder) + + assert stats is not None + assert stats.coverage_buckets == [(1.0, 0.0, 0.0)] + class TestCountImagesInFolder: """Tests for count_images_in_folder function.""" diff --git a/faststack/thumbnail_view/folder_stats.py b/faststack/thumbnail_view/folder_stats.py index 8711f06..de19a7e 100644 --- a/faststack/thumbnail_view/folder_stats.py +++ b/faststack/thumbnail_view/folder_stats.py @@ -247,7 +247,9 @@ def _compute_coverage_buckets( # Efficient stem extraction and metadata lookup stem, _ = os.path.splitext(filename) - meta = entries.get(stem) + meta = entries.get(filename) + if meta is None: + meta = entries.get(stem) if isinstance(meta, dict): if meta.get("uploaded", False): diff --git a/faststack/thumbnail_view/model.py b/faststack/thumbnail_view/model.py index fd08620..f26ee02 100644 --- a/faststack/thumbnail_view/model.py +++ b/faststack/thumbnail_view/model.py @@ -128,7 +128,7 @@ def __init__( self, base_directory: Path, current_directory: Path, - get_metadata_callback: Optional[Callable[[str], dict]] = None, + get_metadata_callback: Optional[Callable[[Path | str], dict]] = None, get_batch_indices_callback: Optional[Callable[[], Set[int]]] = None, get_current_index_callback: Optional[Callable[[], int]] = None, thumbnail_size: int = 200, @@ -406,12 +406,10 @@ def refresh(self): filtered = [] for img in images: try: - meta = self._get_metadata(img.path.stem) + meta = self._get_metadata(img.path) if not isinstance(meta, dict): # Ensure it's a dict before .get() - log.debug( - "Metadata for %s is not a dict: %r", img.path.stem, meta - ) + log.debug("Metadata for %s is not a dict: %r", img.path, meta) continue if all(meta.get(flag, False) for flag in flags): @@ -550,15 +548,15 @@ def refresh_from_controller( for img in images: try: if metadata_map: - meta = metadata_map.get(img.path.stem, {}) + meta = metadata_map.get(normalize_path_key(img.path), {}) elif self._get_metadata: - meta = self._get_metadata(img.path.stem) + meta = self._get_metadata(img.path) else: continue if not isinstance(meta, dict): log.debug( - "Metadata for %s is not a dict: %r", img.path.stem, meta + "Metadata for %s is not a dict: %r", img.path, meta ) continue @@ -612,7 +610,7 @@ def _add_images_to_entries( is_todo = False if metadata_map: - meta = metadata_map.get(img.path.stem, {}) + meta = metadata_map.get(normalize_path_key(img.path), {}) is_stacked = meta.get("stacked", False) is_uploaded = meta.get("uploaded", False) is_edited = meta.get("edited", False) @@ -621,7 +619,7 @@ def _add_images_to_entries( is_todo = meta.get("todo", False) elif self._get_metadata: try: - meta = self._get_metadata(img.path.stem) + meta = self._get_metadata(img.path) if isinstance(meta, dict): is_stacked = meta.get("stacked", False) is_uploaded = meta.get("uploaded", False) @@ -631,7 +629,7 @@ def _add_images_to_entries( is_todo = meta.get("todo", False) else: log.debug( - "Metadata for %s is not a dict: %r", img.path.stem, meta + "Metadata for %s is not a dict: %r", img.path, meta ) except Exception as e: log.debug("Error getting metadata for %s: %s", img.path, e) From c5a5abbced4d8d54bd4c9fff7bf1f03818bb7765 Mon Sep 17 00:00:00 2001 From: Andy Arijs Date: Wed, 18 Mar 2026 09:04:27 +0100 Subject: [PATCH 2/8] Fix-sidecar-dotted-key-migration-and-logging-fallbacks --- faststack/io/sidecar.py | 43 +++++++++++++++++--- faststack/logging_setup.py | 20 ++++++---- faststack/tests/test_sidecar.py | 71 +++++++++++++++++++++++++++++---- 3 files changed, 114 insertions(+), 20 deletions(-) diff --git a/faststack/io/sidecar.py b/faststack/io/sidecar.py index 7371d5f..a46f2c2 100644 --- a/faststack/io/sidecar.py +++ b/faststack/io/sidecar.py @@ -7,9 +7,11 @@ from pathlib import Path from typing import Literal, Optional, Union, overload +from faststack.io.indexer import JPG_EXTENSIONS, RAW_EXTENSIONS from faststack.models import Sidecar, EntryMetadata log = logging.getLogger(__name__) +KNOWN_IMAGE_EXTENSIONS = frozenset(ext.lower() for ext in JPG_EXTENSIONS | RAW_EXTENSIONS) def _entrymetadata_from_json(meta: dict) -> EntryMetadata: @@ -146,6 +148,10 @@ def get_metadata( if no entry exists — callers must handle the None case explicitly. """ stable_key, candidate_keys = self._lookup_keys(image_ref) + if not stable_key: + if create: + raise ValueError(f"image_ref must not be empty: {image_ref!r}") + return None meta = self.data.entries.get(stable_key) if meta is None: @@ -180,10 +186,12 @@ def get_metadata( def metadata_key_for_path(self, image_path: Union[str, Path]) -> str: """Return the stable sidecar key for a concrete image path.""" path = Path(image_path) + if not path.name: + return "" if not path.is_absolute(): path = self.directory / path - base_dir = Path(os.path.abspath(str(self.directory))) - abs_path = Path(os.path.abspath(str(path))) + base_dir = Path(os.path.normcase(os.path.abspath(str(self.directory)))) + abs_path = Path(os.path.normcase(os.path.abspath(str(path)))) try: relative = abs_path.relative_to(base_dir) @@ -196,6 +204,8 @@ def metadata_key_for_path(self, image_path: Union[str, Path]) -> str: def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: """Return (stable_key, migration_candidate_keys) for a metadata lookup.""" if isinstance(image_ref, Path): + if not image_ref.name: + return "", [] stable_key = self.metadata_key_for_path(image_ref) full_name_key = self._metadata_filename_key(image_ref) return stable_key, [full_name_key, image_ref.stem] @@ -204,21 +214,34 @@ def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: if not value: return "", [] - if os.path.sep in value or "/" in value or "\\" in value or "." in Path(value).name: + if ( + os.path.sep in value + or "/" in value + or "\\" in value + or Path(value).suffix.lower() in KNOWN_IMAGE_EXTENSIONS + ): path = Path(value) stable_key = self.metadata_key_for_path(path) full_name_key = self._metadata_filename_key(path) return stable_key, [full_name_key, path.stem] + candidate_path = self.directory / value + if candidate_path.exists(): + stable_key = self.metadata_key_for_path(candidate_path) + full_name_key = self._metadata_filename_key(candidate_path) + return stable_key, [full_name_key, value] + return value, [value] def _metadata_filename_key(self, image_path: Union[str, Path]) -> str: """Return the extension-preserving key used by the regressed patch.""" path = Path(image_path) + if not path.name: + return "" if not path.is_absolute(): path = self.directory / path - base_dir = Path(os.path.abspath(str(self.directory))) - abs_path = Path(os.path.abspath(str(path))) + base_dir = Path(os.path.normcase(os.path.abspath(str(self.directory)))) + abs_path = Path(os.path.normcase(os.path.abspath(str(path)))) try: relative = abs_path.relative_to(base_dir) @@ -230,8 +253,16 @@ def _stable_key_from_key(self, key: str) -> str: """Convert any historical sidecar key form into today's stable key.""" if not key: return "" - if os.path.sep in key or "/" in key or "\\" in key or "." in Path(key).name: + if ( + os.path.sep in key + or "/" in key + or "\\" in key + or Path(key).suffix.lower() in KNOWN_IMAGE_EXTENSIONS + ): return self.metadata_key_for_path(Path(key)) + candidate_path = self.directory / key + if candidate_path.exists(): + return self.metadata_key_for_path(candidate_path) return key def set_last_index(self, index: int): diff --git a/faststack/logging_setup.py b/faststack/logging_setup.py index e783d94..6dda08f 100644 --- a/faststack/logging_setup.py +++ b/faststack/logging_setup.py @@ -3,6 +3,7 @@ import logging import logging.handlers import os +import tempfile from pathlib import Path @@ -12,10 +13,10 @@ def _is_writable_dir(path: Path) -> bool: return False try: - probe = path / ".write_test" - with probe.open("w", encoding="utf-8") as f: + with tempfile.NamedTemporaryFile( + mode="w", encoding="utf-8", dir=path, prefix="faststack-write-", delete=True + ) as f: f.write("ok") - probe.unlink(missing_ok=True) return True except OSError: return False @@ -31,6 +32,8 @@ def _can_create_dir(path: Path) -> bool: parent = next_parent return parent.is_dir() and os.access(parent, os.W_OK) + + def get_app_data_dir() -> Path: """Return a writable application data directory, with fallbacks.""" candidates = [] @@ -54,9 +57,8 @@ def get_app_data_dir() -> Path: if _can_create_dir(candidate): return candidate - # Final fallback: return the first candidate even if unwritable so callers - # still get a deterministic location for error reporting. - return candidates[0] if candidates else Path.cwd() / "var" / "appdata" + # Final fallback: system temp is the most reliable writable location. + return Path(tempfile.gettempdir()) / "faststack" def setup_logging(debug: bool = False): @@ -66,7 +68,11 @@ def setup_logging(debug: bool = False): debug: If True, sets log level to DEBUG. Otherwise, sets to WARNING to reduce noise. """ log_dir = get_app_data_dir() / "logs" - log_dir.mkdir(parents=True, exist_ok=True) + try: + log_dir.mkdir(parents=True, exist_ok=True) + except OSError: + log_dir = Path(tempfile.gettempdir()) / "faststack" / "logs" + log_dir.mkdir(parents=True, exist_ok=True) log_file = log_dir / "app.log" # File handler diff --git a/faststack/tests/test_sidecar.py b/faststack/tests/test_sidecar.py index 1334dac..46f696f 100644 --- a/faststack/tests/test_sidecar.py +++ b/faststack/tests/test_sidecar.py @@ -1,6 +1,7 @@ """Tests for the SidecarManager.""" import json +import os from pathlib import Path import pytest @@ -70,15 +71,17 @@ def test_sidecar_save(mock_sidecar_dir): # Verify file content saved_data = json.loads((d / "faststack.json").read_text()) + expected_key = sm.metadata_key_for_path(Path("IMG_TEST.jpg")) assert saved_data["last_index"] == 10 - assert saved_data["entries"]["IMG_TEST"]["stack_id"] == 99 + assert saved_data["entries"][expected_key]["stack_id"] == 99 def test_sidecar_get_metadata_creates_new(mock_sidecar_dir): """Tests that get_metadata creates a new entry if one doesn't exist.""" d = mock_sidecar_dir() sm = SidecarManager(d, None) - assert "NEW_IMG.jpg" not in sm.data.entries + expected_key = sm.metadata_key_for_path(Path("NEW_IMG.jpg")) + assert expected_key not in sm.data.entries meta = sm.get_metadata(Path("NEW_IMG.jpg")) # EntryMetadata may be a runtime class OR a typing alias, depending on refactors. @@ -89,7 +92,7 @@ def test_sidecar_get_metadata_creates_new(mock_sidecar_dir): assert meta.__class__.__name__ == "EntryMetadata" assert hasattr(meta, "stack_id") - assert "NEW_IMG" in sm.data.entries + assert expected_key in sm.data.entries def test_favorite_toggle_sets_json(mock_sidecar_dir): @@ -105,13 +108,14 @@ def test_favorite_toggle_sets_json(mock_sidecar_dir): meta.favorite = True sm.save() saved = json.loads((d / "faststack.json").read_text()) - assert saved["entries"]["IMG_FAV"]["favorite"] is True + expected_key = sm.metadata_key_for_path(Path("IMG_FAV.jpg")) + assert saved["entries"][expected_key]["favorite"] is True # Toggle off meta.favorite = False sm.save() saved = json.loads((d / "faststack.json").read_text()) - assert saved["entries"]["IMG_FAV"]["favorite"] is False + assert saved["entries"][expected_key]["favorite"] is False def test_favorite_loads_from_sidecar(mock_sidecar_dir): @@ -161,12 +165,16 @@ def test_legacy_stem_entry_migrates_to_path_key(mock_sidecar_dir): sm = SidecarManager(d, None) meta = sm.get_metadata(Path("IMG_0001.jpg"), create=False) + expected_key = sm.metadata_key_for_path(Path("IMG_0001.jpg")) assert meta is not None assert meta.uploaded is True - assert "IMG_0001" in sm.data.entries + assert expected_key in sm.data.entries assert "IMG_0001.jpg" not in sm.data.entries - assert sm.metadata_key_for_path(Path("IMG_0001.jpg")) == "IMG_0001" + if os.name == "nt": + assert expected_key == "img_0001" + else: + assert expected_key == "IMG_0001" def test_raw_only_entry_survives_transition_to_visible_jpg(mock_sidecar_dir): @@ -224,3 +232,52 @@ def test_same_stem_in_different_subfolders_do_not_collide(mock_sidecar_dir): assert sm.data.entries["a/photo"].uploaded is False assert sm.data.entries["b/photo"].favorite is False assert sm.data.entries["b/photo"].uploaded is True + + +def test_semantic_dot_key_is_not_treated_as_path(mock_sidecar_dir): + """Semantic keys like sample.v1 must not be truncated as filenames.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + legacy = EntryMetadata(todo=True) + sm.data.entries["sample.v1"] = legacy + + meta = sm.get_metadata("sample.v1", create=False) + + assert meta is legacy + assert "sample.v1" in sm.data.entries + assert "sample" not in sm.data.entries + + +def test_empty_string_create_false_returns_none(mock_sidecar_dir): + """Empty string refs must not create a sidecar entry.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + + assert sm.get_metadata("", create=False) is None + assert "" not in sm.data.entries + + +def test_empty_string_create_true_raises(mock_sidecar_dir): + """Empty string refs should fail fast when asked to create metadata.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + + with pytest.raises(ValueError, match="image_ref must not be empty"): + sm.get_metadata("", create=True) + + assert "" not in sm.data.entries + + +def test_metadata_key_for_path_normalizes_case_on_windows(mock_sidecar_dir): + """Windows path casing should not create duplicate stable keys.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + + key_upper = sm.metadata_key_for_path(Path("Mixed/Case/IMG_0001.JPG")) + key_lower = sm.metadata_key_for_path(Path("mixed/case/img_0001.jpg")) + + if os.name == "nt": + assert key_upper == key_lower + else: + assert key_upper != "" + assert key_lower != "" From bc52c392a760d48cfd08dbc0e20fe32381fb6497 Mon Sep 17 00:00:00 2001 From: Andy Arijs Date: Wed, 18 Mar 2026 09:13:01 +0100 Subject: [PATCH 3/8] Fix-logging-fallback-to-console-only --- faststack/logging_setup.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/faststack/logging_setup.py b/faststack/logging_setup.py index 6dda08f..7a52794 100644 --- a/faststack/logging_setup.py +++ b/faststack/logging_setup.py @@ -68,21 +68,22 @@ def setup_logging(debug: bool = False): debug: If True, sets log level to DEBUG. Otherwise, sets to WARNING to reduce noise. """ log_dir = get_app_data_dir() / "logs" + log_file = None try: log_dir.mkdir(parents=True, exist_ok=True) except OSError: log_dir = Path(tempfile.gettempdir()) / "faststack" / "logs" - log_dir.mkdir(parents=True, exist_ok=True) - log_file = log_dir / "app.log" + try: + log_dir.mkdir(parents=True, exist_ok=True) + except OSError: + log_dir = None + + if log_dir is not None: + log_file = log_dir / "app.log" - # File handler - file_handler = logging.handlers.RotatingFileHandler( - log_file, maxBytes=10 * 1024 * 1024, backupCount=5 - ) formatter = logging.Formatter( "%(asctime)s - %(name)s - %(levelname)s - %(message)s" ) - file_handler.setFormatter(formatter) # Console handler (for seeing logs in terminal) console_handler = logging.StreamHandler() @@ -92,9 +93,15 @@ def setup_logging(debug: bool = False): # Set log level based on debug flag root_logger.setLevel(logging.DEBUG if debug else logging.WARNING) root_logger.handlers.clear() - root_logger.addHandler(file_handler) root_logger.addHandler(console_handler) + if log_file is not None: + file_handler = logging.handlers.RotatingFileHandler( + log_file, maxBytes=10 * 1024 * 1024, backupCount=5 + ) + file_handler.setFormatter(formatter) + root_logger.addHandler(file_handler) + # Configure logging for key modules if debug: logging.getLogger("faststack.imaging.cache").setLevel(logging.DEBUG) From 32614872effb2aa49fd6ea38811dc370c49797e4 Mon Sep 17 00:00:00 2001 From: Andy Arijs Date: Wed, 18 Mar 2026 09:19:55 +0100 Subject: [PATCH 4/8] Add-tempdir-warning-and-empty-Path-tests --- faststack/logging_setup.py | 10 +++++++++- faststack/tests/test_sidecar.py | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/faststack/logging_setup.py b/faststack/logging_setup.py index 7a52794..74122c4 100644 --- a/faststack/logging_setup.py +++ b/faststack/logging_setup.py @@ -6,6 +6,8 @@ import tempfile from pathlib import Path +log = logging.getLogger(__name__) + def _is_writable_dir(path: Path) -> bool: """Return True when an existing directory accepts file writes.""" @@ -58,7 +60,13 @@ def get_app_data_dir() -> Path: return candidate # Final fallback: system temp is the most reliable writable location. - return Path(tempfile.gettempdir()) / "faststack" + fallback = Path(tempfile.gettempdir()) / "faststack" + log.warning( + "No writable app-data directory found; falling back to temp directory %s. " + "Configuration and logs may not persist across restarts.", + fallback, + ) + return fallback def setup_logging(debug: bool = False): diff --git a/faststack/tests/test_sidecar.py b/faststack/tests/test_sidecar.py index 46f696f..dbf358f 100644 --- a/faststack/tests/test_sidecar.py +++ b/faststack/tests/test_sidecar.py @@ -268,6 +268,26 @@ def test_empty_string_create_true_raises(mock_sidecar_dir): assert "" not in sm.data.entries +def test_empty_path_create_false_returns_none(mock_sidecar_dir): + """Path('') should be treated the same as an empty string.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + + assert sm.get_metadata(Path(""), create=False) is None + assert "" not in sm.data.entries + + +def test_empty_path_create_true_raises(mock_sidecar_dir): + """Path('') should fail fast when asked to create metadata.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + + with pytest.raises(ValueError, match="image_ref must not be empty"): + sm.get_metadata(Path(""), create=True) + + assert "" not in sm.data.entries + + def test_metadata_key_for_path_normalizes_case_on_windows(mock_sidecar_dir): """Windows path casing should not create duplicate stable keys.""" d = mock_sidecar_dir() From 3eef80314ca3d80635a5d65036c695b22394e75a Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Thu, 19 Mar 2026 18:14:13 -0700 Subject: [PATCH 5/8] Fix sidecar key handling and hot-path metadata lookups --- faststack/app.py | 16 ++++++--- faststack/io/sidecar.py | 17 +++------- faststack/tests/test_sidecar.py | 59 +++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 16 deletions(-) diff --git a/faststack/app.py b/faststack/app.py index 304bd33..7053754 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -1040,9 +1040,11 @@ def _apply_filter_to_cached_list(self): # Apply flag-based filtering (AND logic: image must have ALL checked flags) if self._filter_enabled and self._filter_flags: flags = self._filter_flags + entries = self.sidecar.data.entries result = [] for img in filtered: - meta = self.sidecar.get_metadata(img.path, create=False) + key = self.sidecar.metadata_key_for_path(img.path) + meta = entries.get(key) if not meta: continue @@ -2157,9 +2159,11 @@ def _get_metadata_dict(self, image_path: Path | str) -> dict: def _get_bulk_metadata_map(self) -> Dict[str, dict]: """Get flattened metadata map for all images (for efficient grid refresh).""" bulk_map = {} + entries = self.sidecar.data.entries try: for img in self.image_files: - meta = self.sidecar.get_metadata(img.path, create=False) + key = self.sidecar.metadata_key_for_path(img.path) + meta = entries.get(key) if meta is None: continue bulk_map[normalize_path_key(img.path)] = { @@ -2615,9 +2619,11 @@ def add_favorites_to_batch(self): return # Find indices of all favorited images + entries = self.sidecar.data.entries indices_to_add = [] for i, img in enumerate(self.image_files): - meta = self.sidecar.get_metadata(img.path, create=False) + key = self.sidecar.metadata_key_for_path(img.path) + meta = entries.get(key) if meta and meta.favorite: indices_to_add.append(i) @@ -2670,9 +2676,11 @@ def add_uploaded_to_batch(self): return # Find indices of all uploaded images + entries = self.sidecar.data.entries indices_to_add = [] for i, img in enumerate(self.image_files): - meta = self.sidecar.get_metadata(img.path, create=False) + key = self.sidecar.metadata_key_for_path(img.path) + meta = entries.get(key) if meta and meta.uploaded: indices_to_add.append(i) diff --git a/faststack/io/sidecar.py b/faststack/io/sidecar.py index a46f2c2..f98289d 100644 --- a/faststack/io/sidecar.py +++ b/faststack/io/sidecar.py @@ -214,23 +214,16 @@ def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: if not value: return "", [] - if ( - os.path.sep in value - or "/" in value - or "\\" in value - or Path(value).suffix.lower() in KNOWN_IMAGE_EXTENSIONS - ): + # Only treat string as a path if it contains explicit path separators. + # Dotted strings (even with image extensions like "photo.CR2") are + # treated as exact keys — migration of legacy filename keys is handled + # by the _stable_key_from_key scan in get_metadata. + if os.path.sep in value or "/" in value or "\\" in value: path = Path(value) stable_key = self.metadata_key_for_path(path) full_name_key = self._metadata_filename_key(path) return stable_key, [full_name_key, path.stem] - candidate_path = self.directory / value - if candidate_path.exists(): - stable_key = self.metadata_key_for_path(candidate_path) - full_name_key = self._metadata_filename_key(candidate_path) - return stable_key, [full_name_key, value] - return value, [value] def _metadata_filename_key(self, image_path: Union[str, Path]) -> str: diff --git a/faststack/tests/test_sidecar.py b/faststack/tests/test_sidecar.py index dbf358f..3ac8c49 100644 --- a/faststack/tests/test_sidecar.py +++ b/faststack/tests/test_sidecar.py @@ -248,6 +248,65 @@ def test_semantic_dot_key_is_not_treated_as_path(mock_sidecar_dir): assert "sample" not in sm.data.entries +def test_dotted_string_with_image_extension_stays_exact(mock_sidecar_dir): + """String key 'photo.CR2' must not be path-normalized to 'photo'.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + legacy = EntryMetadata(favorite=True) + sm.data.entries["photo.CR2"] = legacy + + meta = sm.get_metadata("photo.CR2", create=False) + + assert meta is legacy + assert "photo.CR2" in sm.data.entries + assert "photo" not in sm.data.entries + + +def test_dotted_string_with_jpg_extension_stays_exact(mock_sidecar_dir): + """String key 'IMG_0001.jpg' must not be path-normalized when used as string.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + legacy = EntryMetadata(uploaded=True) + sm.data.entries["IMG_0001.jpg"] = legacy + + meta = sm.get_metadata("IMG_0001.jpg", create=False) + + assert meta is legacy + assert "IMG_0001.jpg" in sm.data.entries + + +def test_string_with_path_separator_is_path_normalized(mock_sidecar_dir): + """String containing a path separator should be treated as a path.""" + d = mock_sidecar_dir() + sm = SidecarManager(d, None) + + meta = sm.get_metadata("subdir/photo.CR2") + expected_key = sm.metadata_key_for_path(Path("subdir/photo.CR2")) + + assert expected_key in sm.data.entries + assert expected_key == "subdir/photo" + + +def test_legacy_filename_key_migrates_via_path_lookup(mock_sidecar_dir): + """Legacy 'photo.CR2' entry migrates when looked up via Path('photo.jpg').""" + content = { + "version": 2, + "entries": { + "photo.CR2": {"favorite": True}, + }, + } + d = mock_sidecar_dir(content) + sm = SidecarManager(d, None) + + # Path-based lookup should find and migrate the legacy key + meta = sm.get_metadata(Path("photo.jpg"), create=False) + + assert meta is not None + assert meta.favorite is True + assert "photo" in sm.data.entries + assert "photo.CR2" not in sm.data.entries + + def test_empty_string_create_false_returns_none(mock_sidecar_dir): """Empty string refs must not create a sidecar entry.""" d = mock_sidecar_dir() From 14674423be03a184bb870dc52b691dca5eb22cbf Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Thu, 19 Mar 2026 19:40:23 -0700 Subject: [PATCH 6/8] Fix issues identified by coderabbit --- faststack/app.py | 3 ++- faststack/io/sidecar.py | 20 ++++++++++++++------ faststack/io/utils.py | 2 +- faststack/logging_setup.py | 8 +++++++- faststack/tests/test_sidecar.py | 3 +-- faststack/thumbnail_view/model.py | 10 ++++++---- 6 files changed, 31 insertions(+), 15 deletions(-) diff --git a/faststack/app.py b/faststack/app.py index 7053754..e741647 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -357,6 +357,7 @@ def __init__( get_metadata_callback=self._get_metadata_dict, get_batch_indices_callback=self._get_batch_indices, get_current_index_callback=self._get_current_loupe_index, + metadata_key_fn=self.sidecar.metadata_key_for_path, thumbnail_size=200, parent=self, # Ensure proper Qt ownership to prevent GC issues ) @@ -2166,7 +2167,7 @@ def _get_bulk_metadata_map(self) -> Dict[str, dict]: meta = entries.get(key) if meta is None: continue - bulk_map[normalize_path_key(img.path)] = { + bulk_map[key] = { "stacked": getattr(meta, "stacked", False), "uploaded": getattr(meta, "uploaded", False), "edited": getattr(meta, "edited", False), diff --git a/faststack/io/sidecar.py b/faststack/io/sidecar.py index f98289d..4daba69 100644 --- a/faststack/io/sidecar.py +++ b/faststack/io/sidecar.py @@ -171,7 +171,7 @@ def get_metadata( for existing_key, existing_meta in list(self.data.entries.items()): if existing_key == stable_key: continue - if self._stable_key_from_key(existing_key) != stable_key: + if self._stable_key_from_key(existing_key, check_fs=True) != stable_key: continue meta = existing_meta self.data.entries[stable_key] = existing_meta @@ -242,8 +242,15 @@ def _metadata_filename_key(self, image_path: Union[str, Path]) -> str: except ValueError: return str(abs_path).replace("\\", "/") - def _stable_key_from_key(self, key: str) -> str: - """Convert any historical sidecar key form into today's stable key.""" + def _stable_key_from_key(self, key: str, check_fs: bool = False) -> str: + """Convert any historical sidecar key form into today's stable key. + + Args: + key: The sidecar key to normalize. + check_fs: If True, check the filesystem for bare-stem keys that + match an existing file. Set to True during one-time migration + scans; leave False on hot paths to avoid filesystem I/O. + """ if not key: return "" if ( @@ -253,9 +260,10 @@ def _stable_key_from_key(self, key: str) -> str: or Path(key).suffix.lower() in KNOWN_IMAGE_EXTENSIONS ): return self.metadata_key_for_path(Path(key)) - candidate_path = self.directory / key - if candidate_path.exists(): - return self.metadata_key_for_path(candidate_path) + if check_fs: + candidate_path = self.directory / key + if candidate_path.exists(): + return self.metadata_key_for_path(candidate_path) return key def set_last_index(self, index: int): diff --git a/faststack/io/utils.py b/faststack/io/utils.py index 6c0d11f..e130440 100644 --- a/faststack/io/utils.py +++ b/faststack/io/utils.py @@ -20,7 +20,7 @@ def normalize_path_key(path: Union[Path, str]) -> str: # os.path.normcase on Linux: returns as-is # os.path.abspath: ensures absolute path and collapses .. # Note: abspath does NOT resolve symlinks (that's realpath). - return os.path.normcase(os.path.abspath(p_str)) + return os.path.normcase(os.path.abspath(p_str)).replace("\\", "/") def compute_path_hash(path: Union[Path, str]) -> str: diff --git a/faststack/logging_setup.py b/faststack/logging_setup.py index 74122c4..03bb9ee 100644 --- a/faststack/logging_setup.py +++ b/faststack/logging_setup.py @@ -3,6 +3,7 @@ import logging import logging.handlers import os +import sys import tempfile from pathlib import Path @@ -33,7 +34,7 @@ def _can_create_dir(path: Path) -> bool: return False parent = next_parent - return parent.is_dir() and os.access(parent, os.W_OK) + return parent.is_dir() def get_app_data_dir() -> Path: @@ -86,6 +87,11 @@ def setup_logging(debug: bool = False): except OSError: log_dir = None + if log_dir is None: + sys.stderr.write( + "WARNING: Could not create log directory; logs will not be persisted.\n" + ) + if log_dir is not None: log_file = log_dir / "app.log" diff --git a/faststack/tests/test_sidecar.py b/faststack/tests/test_sidecar.py index 3ac8c49..322ac42 100644 --- a/faststack/tests/test_sidecar.py +++ b/faststack/tests/test_sidecar.py @@ -358,5 +358,4 @@ def test_metadata_key_for_path_normalizes_case_on_windows(mock_sidecar_dir): if os.name == "nt": assert key_upper == key_lower else: - assert key_upper != "" - assert key_lower != "" + assert key_upper != key_lower diff --git a/faststack/thumbnail_view/model.py b/faststack/thumbnail_view/model.py index f26ee02..ebd198a 100644 --- a/faststack/thumbnail_view/model.py +++ b/faststack/thumbnail_view/model.py @@ -131,6 +131,7 @@ def __init__( get_metadata_callback: Optional[Callable[[Path | str], dict]] = None, get_batch_indices_callback: Optional[Callable[[], Set[int]]] = None, get_current_index_callback: Optional[Callable[[], int]] = None, + metadata_key_fn: Optional[Callable[[Path], str]] = None, thumbnail_size: int = 200, parent=None, ): @@ -141,6 +142,7 @@ def __init__( self._get_metadata = get_metadata_callback self._get_batch_indices = get_batch_indices_callback self._get_current_index = get_current_index_callback + self._metadata_key_fn = metadata_key_fn self._thumbnail_size = thumbnail_size self._entries: List[ThumbnailEntry] = [] self._folder_count: int = 0 # cached; updated on mutation @@ -547,8 +549,8 @@ def refresh_from_controller( filtered = [] for img in images: try: - if metadata_map: - meta = metadata_map.get(normalize_path_key(img.path), {}) + if metadata_map and self._metadata_key_fn: + meta = metadata_map.get(self._metadata_key_fn(img.path), {}) elif self._get_metadata: meta = self._get_metadata(img.path) else: @@ -609,8 +611,8 @@ def _add_images_to_entries( is_favorite = False is_todo = False - if metadata_map: - meta = metadata_map.get(normalize_path_key(img.path), {}) + if metadata_map and self._metadata_key_fn: + meta = metadata_map.get(self._metadata_key_fn(img.path), {}) is_stacked = meta.get("stacked", False) is_uploaded = meta.get("uploaded", False) is_edited = meta.get("edited", False) From 234ed9b4189836e3bf3040ae22e0dfd6aa82d4bd Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Tue, 24 Mar 2026 14:21:38 -0700 Subject: [PATCH 7/8] Fix bulk metadata access to use get_metadata and ensure migration of legacy keys --- faststack/app.py | 17 +++------ faststack/logging_setup.py | 2 +- faststack/tests/test_sidecar.py | 68 +++++++++++++++++++++++++++++---- 3 files changed, 67 insertions(+), 20 deletions(-) diff --git a/faststack/app.py b/faststack/app.py index e741647..1f77d70 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -357,7 +357,7 @@ def __init__( get_metadata_callback=self._get_metadata_dict, get_batch_indices_callback=self._get_batch_indices, get_current_index_callback=self._get_current_loupe_index, - metadata_key_fn=self.sidecar.metadata_key_for_path, + metadata_key_fn=lambda p: self.sidecar.metadata_key_for_path(p), thumbnail_size=200, parent=self, # Ensure proper Qt ownership to prevent GC issues ) @@ -1041,11 +1041,9 @@ def _apply_filter_to_cached_list(self): # Apply flag-based filtering (AND logic: image must have ALL checked flags) if self._filter_enabled and self._filter_flags: flags = self._filter_flags - entries = self.sidecar.data.entries result = [] for img in filtered: - key = self.sidecar.metadata_key_for_path(img.path) - meta = entries.get(key) + meta = self.sidecar.get_metadata(img.path, create=False) if not meta: continue @@ -2160,11 +2158,10 @@ def _get_metadata_dict(self, image_path: Path | str) -> dict: def _get_bulk_metadata_map(self) -> Dict[str, dict]: """Get flattened metadata map for all images (for efficient grid refresh).""" bulk_map = {} - entries = self.sidecar.data.entries try: for img in self.image_files: key = self.sidecar.metadata_key_for_path(img.path) - meta = entries.get(key) + meta = self.sidecar.get_metadata(img.path, create=False) if meta is None: continue bulk_map[key] = { @@ -2620,11 +2617,9 @@ def add_favorites_to_batch(self): return # Find indices of all favorited images - entries = self.sidecar.data.entries indices_to_add = [] for i, img in enumerate(self.image_files): - key = self.sidecar.metadata_key_for_path(img.path) - meta = entries.get(key) + meta = self.sidecar.get_metadata(img.path, create=False) if meta and meta.favorite: indices_to_add.append(i) @@ -2677,11 +2672,9 @@ def add_uploaded_to_batch(self): return # Find indices of all uploaded images - entries = self.sidecar.data.entries indices_to_add = [] for i, img in enumerate(self.image_files): - key = self.sidecar.metadata_key_for_path(img.path) - meta = entries.get(key) + meta = self.sidecar.get_metadata(img.path, create=False) if meta and meta.uploaded: indices_to_add.append(i) diff --git a/faststack/logging_setup.py b/faststack/logging_setup.py index 03bb9ee..435010d 100644 --- a/faststack/logging_setup.py +++ b/faststack/logging_setup.py @@ -34,7 +34,7 @@ def _can_create_dir(path: Path) -> bool: return False parent = next_parent - return parent.is_dir() + return _is_writable_dir(parent) def get_app_data_dir() -> Path: diff --git a/faststack/tests/test_sidecar.py b/faststack/tests/test_sidecar.py index 322ac42..bd0a28e 100644 --- a/faststack/tests/test_sidecar.py +++ b/faststack/tests/test_sidecar.py @@ -154,27 +154,81 @@ def test_favorite_toggle_roundtrip(mock_sidecar_dir): def test_legacy_stem_entry_migrates_to_path_key(mock_sidecar_dir): - """Legacy stem-keyed entries should migrate on first concrete path lookup.""" + """Legacy filename-keyed entries should migrate on first concrete path lookup.""" + # Use a legacy key ("IMG_0001.jpg") that differs from the stable key + # returned by metadata_key_for_path (which strips the extension), so that + # get_metadata must actually perform migration. + legacy_key = "IMG_0001.jpg" content = { "version": 2, "entries": { - "IMG_0001": {"uploaded": True}, + legacy_key: {"uploaded": True}, }, } d = mock_sidecar_dir(content) sm = SidecarManager(d, None) - meta = sm.get_metadata(Path("IMG_0001.jpg"), create=False) expected_key = sm.metadata_key_for_path(Path("IMG_0001.jpg")) + assert expected_key != legacy_key, "legacy key must differ from stable key" + + meta = sm.get_metadata(Path("IMG_0001.jpg"), create=False) assert meta is not None assert meta.uploaded is True + # Entry now lives under the stable key assert expected_key in sm.data.entries + # Legacy key was removed during migration + assert legacy_key not in sm.data.entries + + +def test_bulk_iteration_finds_legacy_keyed_entries(mock_sidecar_dir): + """Bulk get_metadata(path, create=False) must find and migrate legacy keys. + + Regression test: hot paths like flag filtering and batch selection iterate + over all images calling get_metadata(path, create=False). If they bypassed + migration (e.g. via direct entries dict access), legacy-keyed entries would + be silently missed. + """ + content = { + "version": 2, + "entries": { + # Legacy filename-keyed entry (has extension — differs from stable key) + "IMG_0001.jpg": {"favorite": True}, + # Already-stable entry (bare stem, matches metadata_key_for_path) + "IMG_0002": {"uploaded": True}, + # Another legacy filename-keyed entry + "IMG_0003.jpg": {"favorite": True, "uploaded": True}, + }, + } + d = mock_sidecar_dir(content) + sm = SidecarManager(d, None) + + # Simulate the bulk iteration pattern used by _apply_filter_to_cached_list, + # _get_bulk_metadata_map, _batch_add_favorites, _batch_add_uploaded + image_paths = [Path("IMG_0001.jpg"), Path("IMG_0002.jpg"), Path("IMG_0003.jpg")] + found = {} + for p in image_paths: + meta = sm.get_metadata(p, create=False) + if meta is not None: + found[p.stem] = meta + + # All three entries must be found — none silently dropped + assert "IMG_0001" in found, "legacy filename-keyed entry must be found" + assert "IMG_0002" in found, "already-stable entry must be found" + assert "IMG_0003" in found, "legacy filename-keyed entry must be found" + + assert found["IMG_0001"].favorite is True + assert found["IMG_0002"].uploaded is True + assert found["IMG_0003"].favorite is True + assert found["IMG_0003"].uploaded is True + + # Legacy keys should have been migrated to stable keys + stable_1 = sm.metadata_key_for_path(Path("IMG_0001.jpg")) + stable_3 = sm.metadata_key_for_path(Path("IMG_0003.jpg")) + assert stable_1 in sm.data.entries + assert stable_3 in sm.data.entries assert "IMG_0001.jpg" not in sm.data.entries - if os.name == "nt": - assert expected_key == "img_0001" - else: - assert expected_key == "IMG_0001" + assert "IMG_0003.jpg" not in sm.data.entries def test_raw_only_entry_survives_transition_to_visible_jpg(mock_sidecar_dir): From 437efb1def3fd6d6733ea91abc23eaa236ed24dc Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Tue, 24 Mar 2026 16:15:38 -0700 Subject: [PATCH 8/8] fix issues identified by coderabbit --- faststack/thumbnail_view/model.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/faststack/thumbnail_view/model.py b/faststack/thumbnail_view/model.py index ebd198a..50c175a 100644 --- a/faststack/thumbnail_view/model.py +++ b/faststack/thumbnail_view/model.py @@ -268,9 +268,9 @@ def _get_loupe_index_for_entry(self, entry: ThumbnailEntry) -> Optional[int]: # We'll use the parent (AppController) to look this up parent = self.parent() if parent and hasattr(parent, "_path_to_index"): - # Must use the same key format as _rebuild_path_to_index (abspath, - # not realpath/resolve) so the lookup hits on Windows and Linux. - key = os.path.normcase(os.path.abspath(str(entry.path))) + # Must use the same key format as _rebuild_path_to_index + # (normalize_path_key) so the lookup hits on Windows and Linux. + key = normalize_path_key(entry.path) return parent._path_to_index.get(key) return None @@ -454,10 +454,10 @@ def remove_rows_by_path(self, paths: List[Path]) -> None: return # 1. Map paths to rows (normalized for Windows case/separator consistency) - path_keys = {os.path.normcase(os.path.abspath(str(p))) for p in paths} + path_keys = {normalize_path_key(p) for p in paths} indices_to_remove = [] for i, entry in enumerate(self._entries): - if os.path.normcase(os.path.abspath(str(entry.path))) in path_keys: + if normalize_path_key(entry.path) in path_keys: indices_to_remove.append(i) if not indices_to_remove: