Add exif data to status bar#44
Merged
AlanRockefeller merged 7 commits intomainfrom Feb 19, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded asynchronous, debounced EXIF-brief extraction and UI plumbing; overhauled ByteLRUCache eviction/callback and size estimation for thread-safety; migrated thumbnail provider to QImage and optional timing, deferred thumbnail-model source-reason clearing; many tests and minor QML/UI text/behavior updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as QML / UIState
participant AC as AppController
participant Exec as EXIF WorkerPool
participant Meta as imaging.metadata
participant Cache as App EXIF Cache
UI->>AC: navigation/selection change
AC->>AC: debounce -> _read_exif_deferred()
AC->>Exec: submit get_exif_brief(path)
Exec->>Meta: open file & extract brief
Meta-->>Exec: exif brief string
Exec-->>AC: emit _exifBriefReady(key, brief)
AC->>Cache: store _exif_brief_cache[key]=brief
AC->>UI: emit _exifBriefReady signal
UI->>AC: request metadata
AC-->>UI: metadata includes "exif_brief"
UI->>UI: render exifBrief in status bar
sequenceDiagram
participant App as Application
participant Cache as ByteLRUCache
participant CB as on_evict callback
App->>Cache: __setitem__(key, value)
Cache->>Cache: acquire lock, detect replacement/overflow, collect pending_callbacks
Cache->>Cache: release lock
Cache->>CB: execute pending_callbacks outside lock
CB-->>App: on_evict notified with evicted items
App->>Cache: evict_paths / __delitem__
Cache->>Cache: lock, remove entries, gather callbacks
Cache->>Cache: release lock
Cache->>CB: run callbacks after lock released
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/thumbnail_view/provider.py (1)
126-141:⚠️ Potential issue | 🟠 MajorDead
requestPixmapmethod — remove this leftover.This first
requestPixmapdefinition (lines 126–141) has no executable body — just a docstring and comments. It gets silently shadowed by the secondrequestPixmapat line 186. This appears to be leftover from introducing_parse_id. Ruff F811 confirms the redefinition.Proposed fix: remove the dead method
- def requestPixmap(self, id_str: str, size: QSize, requestedSize: QSize) -> QPixmap: - """Request a pixmap for the given ID. - - This method is O(1) - returns immediately with cached data or placeholder. - - Args: - id_str: URL path after "image://thumbnail/" - size: Output size reference (set by us) - requestedSize: Requested size from QML - - Returns: - QPixmap of the thumbnail or placeholder - """ - # Parse the ID - # Format: {size}/{path_hash}/{mtime_ns}?r={rev} - # Or: folder/{path_hash}/{mtime_ns}?r={rev} - def _parse_id(🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/thumbnail_view/provider.py` around lines 126 - 141, Remove the dead duplicate requestPixmap method that contains only a docstring and comments and is shadowed by the real implementation; locate the first definition of requestPixmap (the one that documents parsing format and mentions _parse_id) and delete that entire method so there is only the single working requestPixmap implementation remaining (the later definition that performs the actual logic).
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@ChangeLog.md`:
- Around line 5-11: Add a missing changelog bullet under version 1.5.9
describing the primary PR feature "Add exif data to status bar": mention EXIF
brief caching, background retrieval, and UI exposure to the status bar (e.g.,
"Show EXIF metadata in status bar with brief caching and background retrieval"),
so users see the core change; reference the PR title "Add exif data to status
bar" and include a concise single-line bullet summarizing EXIF caching, async
fetching, and status bar display.
- Line 7: Change the phrase "Full Screen Mode: Press F11 to toggle fullscreen in
loupe view" to use the compound-adjective form "Full-Screen Mode" so the line
reads "Full-Screen Mode: Press F11 to toggle fullscreen in loupe view"; update
any other occurrences of the standalone "Full Screen" used adjectivally in
ChangeLog.md to "Full-Screen" for consistency.
In `@faststack/app.py`:
- Around line 2337-2351: The debounce handler can leave a stale
_exif_pending_path when it fires for a different image or hits the early
non-JPEG return; update the handler around _exif_source_key(self.current_index)
to clear self._exif_pending_path whenever the computed exif_key doesn't match
the pending key or when you take the early-return path (e.g., when
source_path.suffix not in self._JPEG_SUFFIXES), and ensure you still
update/short-circuit self._exif_brief_cache as before; reference
_exif_pending_path, _exif_source_key, _exif_brief_cache, current_index,
image_files, and _JPEG_SUFFIXES to locate the code to modify.
In `@faststack/imaging/metadata.py`:
- Around line 189-191: The code in get_exif_data uses the private img._getexif()
while get_exif_brief uses the public img.getexif(), which is fragile; update
get_exif_data to call the public img.getexif() instead and normalize its return
to the expected dict-like structure (e.g., convert the returned Exif object to a
plain dict or iterate .items()) so the rest of get_exif_data logic that expects
key/value pairs works the same as before; reference img.getexif(),
get_exif_data, and get_exif_brief when making the change.
- Around line 64-85: The three bare "except Exception: pass" blocks in
_exif_rational_to_seconds silently swallow conversion errors; modify each to
catch the exception as e (e.g. "except Exception as e:") and call a module-level
logger.debug with context (include the input x, which branch failed like
"rational object" or "tuple/list", and the exception message) before continuing
to preserve current behavior (still return None on failure); if no logger
exists, create a module-level one via logging.getLogger(__name__).
In `@faststack/qml/Main.qml`:
- Around line 996-1006: The status bar Label shows an inconsistent fallback
string; in the first Label (rendering uiState.currentFilename) replace the old
fallback " | N/A" with "N/A" so it matches the separator-free style used when
uiState.imageCount > 0; locate the Label that uses uiState.currentFilename (and
its sibling Label using uiState.exifBrief) and update the fallback text
accordingly.
In `@faststack/qml/ThumbnailTile.qml`:
- Around line 262-292: The Backups and Developed badge rectangles are hardcoding
colors ("#9C27B0" and "#009688"); add new tile-level color properties (e.g.,
backupsColor and developedColor alongside existing properties like
uploadedColor, editedColor) in the same location where those other color
properties are defined, then replace the hardcoded color values in the Rectangle
elements that use tile.tileHasBackups and tile.tileHasDeveloped to reference
tile.backupsColor and tile.developedColor respectively so theming remains
consistent and discoverable.
In `@faststack/tests/debug_app_init.py`:
- Around line 30-33: The assignment to qapp is only to ensure a QApplication
exists and is unused; change the assignments to use an underscore to indicate
intentional discard (e.g., replace "qapp = QApplication(sys.argv)" and "qapp =
QApplication.instance()" with "_ = QApplication(sys.argv)" and "_ =
QApplication.instance()") so the variable is not flagged as unused while keeping
the same behavior in the QApplication initialization logic.
In `@faststack/tests/debug_exif.py`:
- Around line 27-31: The debug test contains dead-end scaffold code: in
faststack/tests/debug_exif.py the ImageEditor instance's float_image is being
set to the dict returned by ImageEditor._initial_edits() (which is not image
data) and the flow is left unfinished; either remove this file or convert it
into a proper unit test by creating a real ImageEditor workflow that
produces/loads image data, exercises the methods under test (e.g., create or
load an image, apply edits, then assert EXIF-related outcomes), and replace the
placeholder assignment to float_image with a real image buffer or object; locate
references to ImageEditor, float_image, and _initial_edits to implement the fix.
In `@faststack/tests/test_cache_replacement_callback.py`:
- Around line 28-42: The test test_replacement_plus_lru_eviction currently
converts evicted (a list of (k,v) tuples) to dict(evicted), which silently drops
earlier entries if a key appears more than once; change the assertions to
preserve multiple evictions by collecting values per key (e.g., build a
dict-of-lists from the evicted list or use a collections.defaultdict(list)) and
then assert that evicted_keys include "a" and "b" and that the list for each key
contains the expected values (or assert on specific tuple positions in evicted)
instead of using dict(evicted).
In `@faststack/tests/test_deletion_perf_structure.py`:
- Around line 20-29: The test file uses patch context managers that bind unused
mock names causing F841 warnings; update the with blocks that currently use "as"
for ByteLRUCache, ThumbnailModel, Prefetcher, PathResolver (the context with
patch("faststack.app.ByteLRUCache") as MockCache,
patch("faststack.app.ThumbnailModel") as MockModel,
patch("faststack.app.Prefetcher") as MockPrefetcher,
patch("faststack.app.PathResolver") as MockResolver, ...) to drop the "as
<name>" parts so the patches remain active but no unused variables are created,
and apply the same change to the other with block in
test_delete_uses_targeted_eviction.
In `@faststack/tests/test_feedback_fixes.py`:
- Around line 69-74: The test function test_image_editor_save_exception includes
an unused fixture parameter tmp_path; remove the tmp_path argument from the
function signature so the test only declares def
test_image_editor_save_exception(): and keep the rest of the body unchanged—this
removes the unused pytest fixture while still testing ImageEditor.save_image()
raising RuntimeError when no file path is set.
- Line 109: The test unpacks two values from find_images_with_variants (visible,
vmap) but never uses vmap; change the unpack to (visible, _vmap) or (visible, _)
in the call site where find_images_with_variants(tdp) is invoked so the unused
second return value is explicitly marked as intended; update the occurrence in
faststack/tests/test_feedback_fixes.py that currently reads "visible, vmap =
find_images_with_variants(tdp)" to use the underscore-prefixed variable.
In `@faststack/tests/test_loupe_delete.py`:
- Around line 86-90: The conditional that builds arg0 is redundant because both
branches call list(args[0]); replace the entire conditional expression with a
single direct normalization like assigning arg0 = list(args[0]) (reference the
arg0 variable and the args[0] operand in the test_loupe_delete.py snippet) to
simplify the code.
In `@faststack/tests/test_metadata.py`:
- Around line 94-102: The test test_get_exif_data_no_exif is meant to exercise
get_exif_data when the file exists but has no EXIF, however Path.exists is not
patched so the function may return early; update the test to patch
pathlib.Path.exists to return True (e.g., patch Path.exists or
patch("pathlib.Path.exists", return_value=True)) so the mocked Image.open and
mock_img._getexif() are actually invoked and the assertions on result["summary"]
and result["full"] validate the no-EXIF code path.
In `@faststack/tests/test_variants_logic.py`:
- Around line 92-108: The test test_path_normalization currently only exercises
Windows behavior; add a POSIX branch so the test also asserts norm_path
preserves case on non-Windows systems: inside test_path_normalization, keep the
existing Windows guard and assertions for p1/p2/n1/n2 and add an else branch
(for os.name != "nt") that computes n1/n2 via norm_path with the same inputs and
asserts they are NOT equal (or that their string forms differ), referencing
norm_path and the local variables p1, p2, n1, n2 to ensure cross-platform
behavior is verified.
In `@faststack/tests/thumbnail_view/test_provider_logic.py`:
- Around line 8-21: The provider fixture contains dead scaffolding; remove
unused variables and the no-op context manager and simplify the fixture to only
create a ThumbnailProvider instance via
ThumbnailProvider.__new__(ThumbnailProvider) and set p._default_size = 200
before returning it. Keep the fixture name provider and preserve using
ThumbnailProvider.__new__ so the constructor is bypassed and only the minimal
state (_default_size) is initialized.
- Around line 49-59: The two tests test_parse_id_invalid_format and
test_parse_id_invalid_number unpack all return values from provider._parse_id
but never use most of them; update the unpacking to discard unused values using
the starred-underscore idiom (e.g., replace the long tuple unpack with id_clean,
*_, is_valid = provider._parse_id(id_str) or similar) so only id_clean and
is_valid (or the needed symbols) are kept and Ruff RUF059 warnings are silenced.
In `@faststack/tests/thumbnail_view/test_reason_tracking.py`:
- Around line 95-110: Add a one-line comment to the
test_refresh_sets_deferred_clear test documenting that it only verifies
refresh() does not synchronously clear model._next_source_reason and that the
deferred clear firing is not tested here (since it requires an event-loop
iteration), e.g. reference model.set_filter / refresh() and
model._next_source_reason to clarify the limitation for future readers.
In `@faststack/thumbnail_view/model.py`:
- Line 516: The assertion in ThumbnailModel's refresh thread uses an unnecessary
f-string: replace the f-string assertion in the refresh/thread check (assert cur
== own, f"ThumbnailModel refresh thread mismatch") with a plain string literal
(assert cur == own, "ThumbnailModel refresh thread mismatch") to remove the
redundant interpolation; locate this in the ThumbnailModel refresh thread code
path and update the assertion message accordingly.
In `@faststack/thumbnail_view/prefetcher.py`:
- Around line 262-263: The _decode_worker function currently accepts a priority:
int = PRIO_MED parameter (passed from submit()) but never uses it; either remove
the priority parameter from _decode_worker and stop passing it from submit(), or
thread it into the worker logic—e.g., accept and store the priority, include it
in debug/log messages (use thumb_debug.ThumbTimer or process logger) or use it
to select different decode behavior/queue ordering; update submit() call sites
to match the chosen approach and remove any unused PRIO_MED import if you delete
the parameter.
- Around line 256-320: Duplicate timer/no-timer blocks in _decode_worker make
the orientation and encode steps repeated; replace them with a context manager
that is either timer.stage(...) or a no-op (like contextlib.nullcontext) and
wrap the shared logic once. Specifically, for orientation use a ctx =
timer.stage("orientation") if timer else nullcontext() and then within with ctx:
call get_exif_orientation(path) and apply_orientation_to_np(rgb_array,
orientation); do the same pattern for the "encode" stage and any other
duplicated blocks (e.g., decode/resize/io) where you currently have identical
code paths. Ensure you import and use nullcontext and keep the same behavior for
setting result via Image.fromarray(...).save(...) into a BytesIO buffer and
honoring cancel checks and _stop_event exactly where they are now.
In `@faststack/thumbnail_view/provider.py`:
- Around line 327-329: Replace the eager f-string log.debug call with lazy
%-style formatting so the message is only formatted when debug is enabled;
locate the debug call in the PathResolver update flow (the log.debug invocation
that prints "PathResolver update took ...ms for ... items") and change it to use
a format string with positional arguments (e.g., "%.2f" and "%d") passing
dt*1000 and model.rowCount() as separate parameters to log.debug.
- Around line 126-141: Remove the dead duplicate requestPixmap method that
contains only a docstring and comments and is shadowed by the real
implementation; locate the first definition of requestPixmap (the one that
documents parsing format and mentions _parse_id) and delete that entire method
so there is only the single working requestPixmap implementation remaining (the
later definition that performs the actual logic).
🧹 Nitpick comments (16)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@ChangeLog.md`: - Line 7: Change the phrase "Full Screen Mode: Press F11 to toggle fullscreen in loupe view" to use the compound-adjective form "Full-Screen Mode" so the line reads "Full-Screen Mode: Press F11 to toggle fullscreen in loupe view"; update any other occurrences of the standalone "Full Screen" used adjectivally in ChangeLog.md to "Full-Screen" for consistency. In `@faststack/imaging/metadata.py`: - Around line 189-191: The code in get_exif_data uses the private img._getexif() while get_exif_brief uses the public img.getexif(), which is fragile; update get_exif_data to call the public img.getexif() instead and normalize its return to the expected dict-like structure (e.g., convert the returned Exif object to a plain dict or iterate .items()) so the rest of get_exif_data logic that expects key/value pairs works the same as before; reference img.getexif(), get_exif_data, and get_exif_brief when making the change. - Around line 64-85: The three bare "except Exception: pass" blocks in _exif_rational_to_seconds silently swallow conversion errors; modify each to catch the exception as e (e.g. "except Exception as e:") and call a module-level logger.debug with context (include the input x, which branch failed like "rational object" or "tuple/list", and the exception message) before continuing to preserve current behavior (still return None on failure); if no logger exists, create a module-level one via logging.getLogger(__name__). In `@faststack/qml/ThumbnailTile.qml`: - Around line 262-292: The Backups and Developed badge rectangles are hardcoding colors ("#9C27B0" and "#009688"); add new tile-level color properties (e.g., backupsColor and developedColor alongside existing properties like uploadedColor, editedColor) in the same location where those other color properties are defined, then replace the hardcoded color values in the Rectangle elements that use tile.tileHasBackups and tile.tileHasDeveloped to reference tile.backupsColor and tile.developedColor respectively so theming remains consistent and discoverable. In `@faststack/tests/debug_app_init.py`: - Around line 30-33: The assignment to qapp is only to ensure a QApplication exists and is unused; change the assignments to use an underscore to indicate intentional discard (e.g., replace "qapp = QApplication(sys.argv)" and "qapp = QApplication.instance()" with "_ = QApplication(sys.argv)" and "_ = QApplication.instance()") so the variable is not flagged as unused while keeping the same behavior in the QApplication initialization logic. In `@faststack/tests/debug_exif.py`: - Around line 27-31: The debug test contains dead-end scaffold code: in faststack/tests/debug_exif.py the ImageEditor instance's float_image is being set to the dict returned by ImageEditor._initial_edits() (which is not image data) and the flow is left unfinished; either remove this file or convert it into a proper unit test by creating a real ImageEditor workflow that produces/loads image data, exercises the methods under test (e.g., create or load an image, apply edits, then assert EXIF-related outcomes), and replace the placeholder assignment to float_image with a real image buffer or object; locate references to ImageEditor, float_image, and _initial_edits to implement the fix. In `@faststack/tests/test_cache_replacement_callback.py`: - Around line 28-42: The test test_replacement_plus_lru_eviction currently converts evicted (a list of (k,v) tuples) to dict(evicted), which silently drops earlier entries if a key appears more than once; change the assertions to preserve multiple evictions by collecting values per key (e.g., build a dict-of-lists from the evicted list or use a collections.defaultdict(list)) and then assert that evicted_keys include "a" and "b" and that the list for each key contains the expected values (or assert on specific tuple positions in evicted) instead of using dict(evicted). In `@faststack/tests/test_deletion_perf_structure.py`: - Around line 20-29: The test file uses patch context managers that bind unused mock names causing F841 warnings; update the with blocks that currently use "as" for ByteLRUCache, ThumbnailModel, Prefetcher, PathResolver (the context with patch("faststack.app.ByteLRUCache") as MockCache, patch("faststack.app.ThumbnailModel") as MockModel, patch("faststack.app.Prefetcher") as MockPrefetcher, patch("faststack.app.PathResolver") as MockResolver, ...) to drop the "as <name>" parts so the patches remain active but no unused variables are created, and apply the same change to the other with block in test_delete_uses_targeted_eviction. In `@faststack/tests/test_feedback_fixes.py`: - Line 109: The test unpacks two values from find_images_with_variants (visible, vmap) but never uses vmap; change the unpack to (visible, _vmap) or (visible, _) in the call site where find_images_with_variants(tdp) is invoked so the unused second return value is explicitly marked as intended; update the occurrence in faststack/tests/test_feedback_fixes.py that currently reads "visible, vmap = find_images_with_variants(tdp)" to use the underscore-prefixed variable. In `@faststack/tests/test_metadata.py`: - Around line 94-102: The test test_get_exif_data_no_exif is meant to exercise get_exif_data when the file exists but has no EXIF, however Path.exists is not patched so the function may return early; update the test to patch pathlib.Path.exists to return True (e.g., patch Path.exists or patch("pathlib.Path.exists", return_value=True)) so the mocked Image.open and mock_img._getexif() are actually invoked and the assertions on result["summary"] and result["full"] validate the no-EXIF code path. In `@faststack/tests/test_variants_logic.py`: - Around line 92-108: The test test_path_normalization currently only exercises Windows behavior; add a POSIX branch so the test also asserts norm_path preserves case on non-Windows systems: inside test_path_normalization, keep the existing Windows guard and assertions for p1/p2/n1/n2 and add an else branch (for os.name != "nt") that computes n1/n2 via norm_path with the same inputs and asserts they are NOT equal (or that their string forms differ), referencing norm_path and the local variables p1, p2, n1, n2 to ensure cross-platform behavior is verified. In `@faststack/tests/thumbnail_view/test_provider_logic.py`: - Around line 49-59: The two tests test_parse_id_invalid_format and test_parse_id_invalid_number unpack all return values from provider._parse_id but never use most of them; update the unpacking to discard unused values using the starred-underscore idiom (e.g., replace the long tuple unpack with id_clean, *_, is_valid = provider._parse_id(id_str) or similar) so only id_clean and is_valid (or the needed symbols) are kept and Ruff RUF059 warnings are silenced. In `@faststack/tests/thumbnail_view/test_reason_tracking.py`: - Around line 95-110: Add a one-line comment to the test_refresh_sets_deferred_clear test documenting that it only verifies refresh() does not synchronously clear model._next_source_reason and that the deferred clear firing is not tested here (since it requires an event-loop iteration), e.g. reference model.set_filter / refresh() and model._next_source_reason to clarify the limitation for future readers. In `@faststack/thumbnail_view/prefetcher.py`: - Around line 262-263: The _decode_worker function currently accepts a priority: int = PRIO_MED parameter (passed from submit()) but never uses it; either remove the priority parameter from _decode_worker and stop passing it from submit(), or thread it into the worker logic—e.g., accept and store the priority, include it in debug/log messages (use thumb_debug.ThumbTimer or process logger) or use it to select different decode behavior/queue ordering; update submit() call sites to match the chosen approach and remove any unused PRIO_MED import if you delete the parameter. - Around line 256-320: Duplicate timer/no-timer blocks in _decode_worker make the orientation and encode steps repeated; replace them with a context manager that is either timer.stage(...) or a no-op (like contextlib.nullcontext) and wrap the shared logic once. Specifically, for orientation use a ctx = timer.stage("orientation") if timer else nullcontext() and then within with ctx: call get_exif_orientation(path) and apply_orientation_to_np(rgb_array, orientation); do the same pattern for the "encode" stage and any other duplicated blocks (e.g., decode/resize/io) where you currently have identical code paths. Ensure you import and use nullcontext and keep the same behavior for setting result via Image.fromarray(...).save(...) into a BytesIO buffer and honoring cancel checks and _stop_event exactly where they are now. In `@faststack/thumbnail_view/provider.py`: - Around line 327-329: Replace the eager f-string log.debug call with lazy %-style formatting so the message is only formatted when debug is enabled; locate the debug call in the PathResolver update flow (the log.debug invocation that prints "PathResolver update took ...ms for ... items") and change it to use a format string with positional arguments (e.g., "%.2f" and "%d") passing dt*1000 and model.rowCount() as separate parameters to log.debug.faststack/tests/debug_exif.py (1)
27-31: Incomplete debug test with dead-end code.This test file appears to be a scratch/debug artifact. Line 29 assigns a dict from
_initial_edits()tofloat_image(which normally holds image data), and lines 30–31 acknowledge the flow is unfinished. Consider either completing this into a meaningful test or removing it from the repository — debug scaffolding like this tends to bit-rot.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/debug_exif.py` around lines 27 - 31, The debug test contains dead-end scaffold code: in faststack/tests/debug_exif.py the ImageEditor instance's float_image is being set to the dict returned by ImageEditor._initial_edits() (which is not image data) and the flow is left unfinished; either remove this file or convert it into a proper unit test by creating a real ImageEditor workflow that produces/loads image data, exercises the methods under test (e.g., create or load an image, apply edits, then assert EXIF-related outcomes), and replace the placeholder assignment to float_image with a real image buffer or object; locate references to ImageEditor, float_image, and _initial_edits to implement the fix.ChangeLog.md (1)
7-7: Nit: "Full Screen" → "Full-Screen" when used as a compound adjective.-- Full Screen Mode: Press F11 to toggle fullscreen in loupe view +- Full-Screen Mode: Press F11 to toggle fullscreen in loupe view.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@ChangeLog.md` at line 7, Change the phrase "Full Screen Mode: Press F11 to toggle fullscreen in loupe view" to use the compound-adjective form "Full-Screen Mode" so the line reads "Full-Screen Mode: Press F11 to toggle fullscreen in loupe view"; update any other occurrences of the standalone "Full Screen" used adjectivally in ChangeLog.md to "Full-Screen" for consistency.faststack/tests/test_feedback_fixes.py (1)
109-109: Unpackedvmapis never used.Prefix with
_to signal intent.Proposed fix
- visible, vmap = find_images_with_variants(tdp) + visible, _vmap = find_images_with_variants(tdp)🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/test_feedback_fixes.py` at line 109, The test unpacks two values from find_images_with_variants (visible, vmap) but never uses vmap; change the unpack to (visible, _vmap) or (visible, _) in the call site where find_images_with_variants(tdp) is invoked so the unused second return value is explicitly marked as intended; update the occurrence in faststack/tests/test_feedback_fixes.py that currently reads "visible, vmap = find_images_with_variants(tdp)" to use the underscore-prefixed variable.faststack/tests/debug_app_init.py (1)
30-33: Unused variableqapp(Ruff F841).The variable is assigned solely to ensure a
QApplicationinstance exists. Consider using_to signal intent.Suggested fix
if not QApplication.instance(): - qapp = QApplication(sys.argv) - else: - qapp = QApplication.instance() + QApplication(sys.argv)🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/debug_app_init.py` around lines 30 - 33, The assignment to qapp is only to ensure a QApplication exists and is unused; change the assignments to use an underscore to indicate intentional discard (e.g., replace "qapp = QApplication(sys.argv)" and "qapp = QApplication.instance()" with "_ = QApplication(sys.argv)" and "_ = QApplication.instance()") so the variable is not flagged as unused while keeping the same behavior in the QApplication initialization logic.faststack/qml/ThumbnailTile.qml (1)
262-292: Consider extracting badge colors into properties for consistency.The existing badges (U, E, R, F, B, S) use tile-level color properties (e.g.,
uploadedColor,editedColor), but the new Backups and Developed badges hardcode their colors ("#9C27B0","#009688"). This makes future theming or color adjustments less discoverable.♻️ Suggested: add color properties alongside existing ones (around line 53)
property color cursorColor: "#00BFFF" // Cyan for keyboard cursor +property color backupsColor: "#9C27B0" // Purple for backups (Bk) +property color developedColor: "#009688" // Teal for developed (D)Then reference them:
- color: "#9C27B0" + color: backupsColor- color: "#009688" + color: developedColor🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/qml/ThumbnailTile.qml` around lines 262 - 292, The Backups and Developed badge rectangles are hardcoding colors ("#9C27B0" and "#009688"); add new tile-level color properties (e.g., backupsColor and developedColor alongside existing properties like uploadedColor, editedColor) in the same location where those other color properties are defined, then replace the hardcoded color values in the Rectangle elements that use tile.tileHasBackups and tile.tileHasDeveloped to reference tile.backupsColor and tile.developedColor respectively so theming remains consistent and discoverable.faststack/tests/test_variants_logic.py (1)
92-108: Windows-only normalization test — looks correct.The
skipTestguard and assertions are appropriate. Consider also testing a non-Windows scenario (e.g., verifying that on POSIX, differently-cased paths remain distinct) to ensurenorm_pathbehaves as expected across platforms.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/test_variants_logic.py` around lines 92 - 108, The test test_path_normalization currently only exercises Windows behavior; add a POSIX branch so the test also asserts norm_path preserves case on non-Windows systems: inside test_path_normalization, keep the existing Windows guard and assertions for p1/p2/n1/n2 and add an else branch (for os.name != "nt") that computes n1/n2 via norm_path with the same inputs and asserts they are NOT equal (or that their string forms differ), referencing norm_path and the local variables p1, p2, n1, n2 to ensure cross-platform behavior is verified.faststack/tests/test_deletion_perf_structure.py (1)
20-29: Unusedasbindings in patch context managers.
MockCache,MockModel,MockPrefetcher, andMockResolverare captured but never referenced. Since you only need the patches active (not the mock objects), you can drop theasclauses to silence the F841 warnings.♻️ Suggested cleanup
with ( - patch("faststack.app.ByteLRUCache") as MockCache, - patch("faststack.app.ThumbnailModel") as MockModel, - patch("faststack.app.Prefetcher") as MockPrefetcher, - patch("faststack.app.PathResolver") as MockResolver, + patch("faststack.app.ByteLRUCache"), + patch("faststack.app.ThumbnailModel"), + patch("faststack.app.Prefetcher"), + patch("faststack.app.PathResolver"), patch("faststack.app.Watcher"), patch("faststack.app.uuid"), patch("faststack.app.QTimer"), patch("faststack.app.concurrent.futures.ThreadPoolExecutor"), ):The same applies to the
withblock intest_delete_uses_targeted_eviction(lines 51-60).🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/test_deletion_perf_structure.py` around lines 20 - 29, The test file uses patch context managers that bind unused mock names causing F841 warnings; update the with blocks that currently use "as" for ByteLRUCache, ThumbnailModel, Prefetcher, PathResolver (the context with patch("faststack.app.ByteLRUCache") as MockCache, patch("faststack.app.ThumbnailModel") as MockModel, patch("faststack.app.Prefetcher") as MockPrefetcher, patch("faststack.app.PathResolver") as MockResolver, ...) to drop the "as <name>" parts so the patches remain active but no unused variables are created, and apply the same change to the other with block in test_delete_uses_targeted_eviction.faststack/thumbnail_view/prefetcher.py (2)
262-263:priorityparameter is accepted but unused.The
priorityparameter in_decode_workeris never referenced in the method body. It's passed fromsubmit()but doesn't influence execution. Either use it (e.g., for logging) or remove it from the signature.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/thumbnail_view/prefetcher.py` around lines 262 - 263, The _decode_worker function currently accepts a priority: int = PRIO_MED parameter (passed from submit()) but never uses it; either remove the priority parameter from _decode_worker and stop passing it from submit(), or thread it into the worker logic—e.g., accept and store the priority, include it in debug/log messages (use thumb_debug.ThumbTimer or process logger) or use it to select different decode behavior/queue ordering; update submit() call sites to match the chosen approach and remove any unused PRIO_MED import if you delete the parameter.
256-320: Significant code duplication in timer/no-timer branches.The orientation, encode, decode, resize, and IO operations are each duplicated with the only difference being the
with timer.stage(...)wrapper. Consider a small helper to reduce this, e.g.:from contextlib import nullcontext ctx = timer.stage("orientation") if timer else nullcontext() with ctx: orientation = get_exif_orientation(path) rgb_array = apply_orientation_to_np(rgb_array, orientation)This would halve the line count for these sections and eliminate the duplication risk when updating logic.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/thumbnail_view/prefetcher.py` around lines 256 - 320, Duplicate timer/no-timer blocks in _decode_worker make the orientation and encode steps repeated; replace them with a context manager that is either timer.stage(...) or a no-op (like contextlib.nullcontext) and wrap the shared logic once. Specifically, for orientation use a ctx = timer.stage("orientation") if timer else nullcontext() and then within with ctx: call get_exif_orientation(path) and apply_orientation_to_np(rgb_array, orientation); do the same pattern for the "encode" stage and any other duplicated blocks (e.g., decode/resize/io) where you currently have identical code paths. Ensure you import and use nullcontext and keep the same behavior for setting result via Image.fromarray(...).save(...) into a BytesIO buffer and honoring cancel checks and _stop_event exactly where they are now.faststack/thumbnail_view/provider.py (1)
327-329: Use%-style formatting forlog.debuginstead of f-string.The rest of the codebase consistently uses
log.debug("...", arg1, arg2)format for lazy evaluation. This f-string eagerly formats even when debug logging is disabled.Suggested fix
- log.debug( - f"PathResolver update took {dt*1000:.2f}ms for {model.rowCount()} items" - ) + log.debug( + "PathResolver update took %.2fms for %d items", + dt * 1000, + model.rowCount(), + )🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/thumbnail_view/provider.py` around lines 327 - 329, Replace the eager f-string log.debug call with lazy %-style formatting so the message is only formatted when debug is enabled; locate the debug call in the PathResolver update flow (the log.debug invocation that prints "PathResolver update took ...ms for ... items") and change it to use a format string with positional arguments (e.g., "%.2f" and "%d") passing dt*1000 and model.rowCount() as separate parameters to log.debug.faststack/tests/thumbnail_view/test_reason_tracking.py (1)
95-110: Good test of deferred-clear semantics, but consider adding a brief comment about what's not tested.The test correctly validates that
refresh()doesn't synchronously clear the reason. However, it doesn't (and can't easily, without an event loop) verify that the deferred clear eventually fires. This is fine as-is, but a one-line comment noting this limitation would help future readers understand the boundary.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/thumbnail_view/test_reason_tracking.py` around lines 95 - 110, Add a one-line comment to the test_refresh_sets_deferred_clear test documenting that it only verifies refresh() does not synchronously clear model._next_source_reason and that the deferred clear firing is not tested here (since it requires an event-loop iteration), e.g. reference model.set_filter / refresh() and model._next_source_reason to clarify the limitation for future readers.faststack/tests/thumbnail_view/test_provider_logic.py (1)
49-59: Use*_to discard unused unpacked variables.Per Ruff RUF059, the unpacked variables on lines 51 and 57 are never used. Using the starred-underscore idiom is more idiomatic and silences the warnings:
Suggested fix
def test_parse_id_invalid_format(self, provider): id_str = "invalid/id" - id_clean, parts, thumb_size, path_hash, mtime_ns, reason, is_folder, is_valid = provider._parse_id(id_str) + *_, is_valid = provider._parse_id(id_str) assert is_valid is False def test_parse_id_invalid_number(self, provider): id_str = "abc/pathhash/123" - id_clean, parts, thumb_size, path_hash, mtime_ns, reason, is_folder, is_valid = provider._parse_id(id_str) + *_, is_valid = provider._parse_id(id_str) assert is_valid is False🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/thumbnail_view/test_provider_logic.py` around lines 49 - 59, The two tests test_parse_id_invalid_format and test_parse_id_invalid_number unpack all return values from provider._parse_id but never use most of them; update the unpacking to discard unused values using the starred-underscore idiom (e.g., replace the long tuple unpack with id_clean, *_, is_valid = provider._parse_id(id_str) or similar) so only id_clean and is_valid (or the needed symbols) are kept and Ruff RUF059 warnings are silenced.faststack/tests/test_metadata.py (1)
94-102: Pre-existing:test_get_exif_data_no_exifdoesn't patchPath.exists.This test intends to cover the "file exists but no EXIF" path, but without patching
Path.exists,Path("dummy.jpg").exists()returnsFalseand the function returns early before reaching_getexif(). The mock update on line 98 is correct, but the test may not actually exercise the intended code path. This is pre-existing, not introduced by this PR.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/test_metadata.py` around lines 94 - 102, The test test_get_exif_data_no_exif is meant to exercise get_exif_data when the file exists but has no EXIF, however Path.exists is not patched so the function may return early; update the test to patch pathlib.Path.exists to return True (e.g., patch Path.exists or patch("pathlib.Path.exists", return_value=True)) so the mocked Image.open and mock_img._getexif() are actually invoked and the assertions on result["summary"] and result["full"] validate the no-EXIF code path.faststack/imaging/metadata.py (2)
189-191:get_exif_datauses private_getexif()whileget_exif_briefuses publicgetexif().Line 191 uses
img._getexif()(private, returns raw dict) while the newget_exif_brief(line 123) usesimg.getexif()(public, returnsExifobject). This is pre-existing and the two functions intentionally work with different return formats, but consolidating on the public API would reduce fragility across Pillow upgrades.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/imaging/metadata.py` around lines 189 - 191, The code in get_exif_data uses the private img._getexif() while get_exif_brief uses the public img.getexif(), which is fragile; update get_exif_data to call the public img.getexif() instead and normalize its return to the expected dict-like structure (e.g., convert the returned Exif object to a plain dict or iterate .items()) so the rest of get_exif_data logic that expects key/value pairs works the same as before; reference img.getexif(), get_exif_data, and get_exif_brief when making the change.
64-85: Silentexcept Exception: passswallows conversion failures without trace.
_exif_rational_to_secondshas three bareexcept Exception: passblocks. For a status-bar feature, silent failures are fine from a UX perspective, but alog.debugwould help diagnose EXIF quirks from unusual cameras without cluttering output.♻️ Suggested improvement (example for one block)
try: n, d = int(x.numerator), int(x.denominator) if d != 0: return float(Fraction(n, d)) - except Exception: - pass + except Exception: + log.debug("Failed to convert rational %r to seconds", x)🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/imaging/metadata.py` around lines 64 - 85, The three bare "except Exception: pass" blocks in _exif_rational_to_seconds silently swallow conversion errors; modify each to catch the exception as e (e.g. "except Exception as e:") and call a module-level logger.debug with context (include the input x, which branch failed like "rational object" or "tuple/list", and the exception message) before continuing to preserve current behavior (still return None on failure); if no logger exists, create a module-level one via logging.getLogger(__name__).faststack/tests/test_cache_replacement_callback.py (1)
28-42:dict(evicted)loses data if a key is evicted more than once.If future changes cause
"a"to appear inevictedtwice (e.g. replacement + LRU in the same operation),dict(evicted)would silently keep only the last value. This won't bite you today, but a dict-of-lists or explicit index assertions would be more future-proof.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@faststack/tests/test_cache_replacement_callback.py` around lines 28 - 42, The test test_replacement_plus_lru_eviction currently converts evicted (a list of (k,v) tuples) to dict(evicted), which silently drops earlier entries if a key appears more than once; change the assertions to preserve multiple evictions by collecting values per key (e.g., build a dict-of-lists from the evicted list or use a collections.defaultdict(list)) and then assert that evicted_keys include "a" and "b" and that the list for each key contains the expected values (or assert on specific tuple positions in evicted) instead of using dict(evicted).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/thumbnail_view/prefetcher.py (1)
381-395:⚠️ Potential issue | 🟡 MinorPIL fallback path does not close the image — potential file-handle leak.
Image.open(path)at line 384 keeps the file handle open until GC collects thepil_img. In a threaded thumbnail decoder, many concurrent fallback decodes could exhaust file descriptors. Use a context manager as in the TurboJPEG path.Proposed fix
try: with timer.stage("decode") if timer else nullcontext(): - pil_img = Image.open(path) - # Convert to RGB if needed - if pil_img.mode != "RGB": - pil_img = pil_img.convert("RGB") - - # Resize - pil_img.thumbnail((target_size, target_size), Image.Resampling.LANCZOS) - return np.array(pil_img) + with Image.open(path) as pil_img: + # Convert to RGB if needed + if pil_img.mode != "RGB": + pil_img = pil_img.convert("RGB") + + # Resize + pil_img.thumbnail((target_size, target_size), Image.Resampling.LANCZOS) + return np.array(pil_img)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 381 - 395, The PIL fallback is leaving file handles open by calling Image.open(path) without a context manager; wrap the open in a with Image.open(path) as pil_img: block (inside the existing timer.stage("decode") context) so the file is closed promptly, perform mode conversion and thumbnail while inside that with, then return a copied array (e.g., np.array(pil_img.copy())) so the returned data isn't tied to the closed image; keep the existing exception handling/logging around this block.
🧹 Nitpick comments (9)
faststack/tests/test_deletion_perf_structure.py (1)
117-117: Use_for unused lambda parameter.The
size_oflambda doesn't use its argument. Convention is to name unused parameters_.♻️ Proposed fix
- cache = ByteLRUCache(1000, size_of=lambda x: 1) + cache = ByteLRUCache(1000, size_of=lambda _: 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/test_deletion_perf_structure.py` at line 117, The size_of lambda passed to ByteLRUCache is ignoring its parameter and should use the conventional unused-parameter name; update the call to ByteLRUCache(1000, size_of=...) to supply a lambda that names the unused argument "_" (e.g., replace the current size_of lambda's parameter name with "_") so it's clear the parameter is intentionally unused (refer to ByteLRUCache and the size_of argument).faststack/thumbnail_view/provider.py (2)
125-166: Consider aNamedTuplefor the_parse_idreturn type.An 8-element plain tuple is error-prone at call sites (easy to mix up positional fields). A
NamedTuplewould make both the return and destructuring self-documenting:♻️ Suggested refactor
from typing import NamedTuple class ParsedId(NamedTuple): id_clean: str parts: list[str] thumb_size: Optional[int] path_hash: Optional[str] mtime_ns: Optional[int] reason: str is_folder: bool is_valid: boolThis keeps backward-compatible tuple unpacking while adding
.is_valid,.path_hash, etc. access at call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 125 - 166, Replace the 8-element plain tuple return of _parse_id with a NamedTuple to make field access explicit: define a typing.NamedTuple (e.g., ParsedId) with fields id_clean, parts, thumb_size, path_hash, mtime_ns, reason, is_folder, is_valid; change _parse_id's return annotation to -> ParsedId and return ParsedId(...) instead of raw tuples in all return paths; update any call sites that rely on positional unpacking to either continue to unpack (will remain compatible) or — preferably — access attributes like parsed.path_hash and parsed.is_valid where clarity is needed (search for usages of _parse_id and replace tuple index references with attribute access).
262-270:_bytes_to_image— consider returning the placeholder instead ofNone.Returning
Nonehere forces the caller (line 232) to checkimage and not image.isNull()and then fall through to the prefetcher/placeholder path. If_bytes_to_imagereturnedNone, the code path works, but the caller silently re-submits a prefetch for a cached entry that failed to decode — this could loop if the cached bytes are persistently corrupt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 262 - 270, The _bytes_to_image function currently returns None on decode failure which causes callers to re-submit prefetches for corrupted cached bytes; change _bytes_to_image (in thumbnail_view/provider.py) to return a non-null placeholder QImage (e.g., the same placeholder used by the prefetcher/placeholder path) when loadFromData fails or an exception occurs, and keep the debug log of the failure; ensure the returned placeholder is not isNull() so callers won't requeue the entry and add a clear log message indicating bytes were replaced with a placeholder.faststack/tests/debug_exif.py (1)
5-6: Imports should be grouped before intra-package imports.
tempfileandshutilare stdlib modules but appear after the third-party and local imports on lines 1–4. This is a minor style nit — PEP 8 / isort convention groups stdlib first.Suggested reorder
import unittest +import tempfile +import shutil from pathlib import Path from PIL import Image, ExifTags from faststack.imaging.editor import ImageEditor, sanitize_exif_orientation -import tempfile -import shutil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/debug_exif.py` around lines 5 - 6, Move the standard-library imports tempfile and shutil so they appear before any third-party or intra-package imports in faststack/tests/debug_exif.py; specifically, place the tempfile and shutil import statements at the top of the import block (stdlib group) and then follow with the existing third-party and local imports to comply with PEP8/isort ordering.faststack/thumbnail_view/prefetcher.py (2)
456-476:cancel_allassumes all timers in_inflightare non-None.Line 469 sets
timer.cancelled = Truewithout a None check. This is currently safe becausesubmit()always ensures a timer is created (line 168–169), but ifsubmit()logic ever changes, this would crash. A minor defensive guard would be prudent.Add guard
for timer in inflight_timers: - timer.cancelled = True - thumb_debug.log_trace("cancel_requested", rid=timer.rid) + if timer: + timer.cancelled = True + thumb_debug.log_trace("cancel_requested", rid=timer.rid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 456 - 476, cancel_all assumes timers from self._inflight are non-None and sets timer.cancelled and reads timer.rid; make it defensive by checking each timer is not None before touching it (in cancel_all) so future changes to submit() won't cause AttributeError—iterate inflight_timers and for each do an explicit if timer is not None: set timer.cancelled = True and call thumb_debug.log_trace(..., rid=timer.rid).
397-402: Type hint fortimershould beOptionalto match the guards inside the method.The parameter is typed as
"thumb_debug.ThumbTimer"(non-Optional), yet the method body guards withif timer:on lines 412, 417, 424, and 440. While currentlytimeris always non-None by the time this callback fires (becausesubmit()creates one at line 168–169), the annotation should reflect the defensive guards or the guards should be removed for clarity.Align type hint with usage
def _on_decode_done( self, future: Future, job_key: Tuple[int, str, int], cache_key: str, - timer: "thumb_debug.ThumbTimer", + timer: Optional["thumb_debug.ThumbTimer"], ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 397 - 402, The _on_decode_done callback’s timer parameter is annotated as "thumb_debug.ThumbTimer" but the method checks `if timer:` defensively; update the signature to timer: Optional["thumb_debug.ThumbTimer"] and import Optional from typing (or adjust existing typing imports) so the annotation matches usage (alternatively, remove the defensive `if timer:` checks if you decide timer will never be None); reference the _on_decode_done function and the timer parameter when making the change.faststack/imaging/metadata.py (2)
125-135:get_exif_briefonly supports JPEG — consider documenting this limitation or extending to TIFF/HEIF.The suffix check at line 133 restricts to
.jpg/.jpeg/.jpe. Many cameras also produce.tifor.heiffiles with EXIF data. If this is intentional (e.g., performance constraint from the status bar hot path), a brief comment explaining why would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` around lines 125 - 135, get_exif_brief currently bails out for any file whose suffix isn't in {".jpg", ".jpeg", ".jpe"} which silently excludes TIFF/HEIF files that may contain EXIF; either document this limitation or extend the suffix check to include common EXIF-bearing types (e.g., ".tif", ".tiff", ".heif", ".heic") and ensure downstream code (e.g., the early Path.exists() check and any Pillow usage) still works for those formats — update the docstring to state supported formats if you keep the restriction, or expand the set in the path.suffix check inside get_exif_brief and add a brief comment about performance/hot-path considerations if relevant.
163-168: Silentexcept: passswallows conversion failures — add debug logging.Per Ruff S110,
try-except-passhides potentially useful diagnostic info for unusual EXIF values. The same applies to lines 188–189.Proposed fix
f_number = tags.get(0x829D) if f_number is not None: try: val = float(f_number) parts.append(f"f/{val:.1f}") - except Exception: - pass + except Exception: + log.debug("Could not parse FNumber %r", f_number)try: cleaned = clean_exif_value(dt) # Format is "YYYY:MM:DD HH:MM:SS" — extract time portion if " " in cleaned: parts.append(cleaned.split(" ", 1)[1]) else: parts.append(cleaned) - except Exception: - pass + except Exception: + log.debug("Could not parse DateTime %r", dt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` around lines 163 - 168, The f-number conversion block swallows errors with a bare except; replace it by catching specific exceptions (ValueError, TypeError) and log the failure instead of silently passing: when converting f_number to float in the block that appends to parts (variables f_number, val, parts), log a debug message including the raw f_number and exception (use the module logger or logging.getLogger(__name__) with exc_info=True) so malformed EXIF values are visible; apply the same change to the analogous try/except handling the other EXIF conversion around lines 188–189 (the similar block that parses and appends that exposure-related value).faststack/app.py (1)
2357-2368: Consider a separate executor (or >1 worker) for EXIF reads.EXIF reads now share the single-worker histogram executor, so slow disk I/O can stall histogram updates. A small dedicated executor (or increasing
max_workers) would keep histogram responsiveness stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 2357 - 2368, The EXIF read task is currently submitted to the single-worker histogram executor (_hist_executor) via the local _worker (calling get_exif_brief) which can block histogram updates; create and use a dedicated executor (e.g., _exif_executor with >1 worker) or increase _hist_executor.max_workers, then submit _worker to that dedicated executor instead of _hist_executor and ensure the new executor is created once (e.g., as a member) and shut down appropriately; update references from submitting to _hist_executor to submitting to the new _exif_executor so _exifBriefReady emissions don't contend with histogram tasks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/imaging/metadata.py`:
- Around line 138-149: The call to exif.get_ifd(ExifTags.IFD.Exif) is executed
after the Image.open(path) context closes which can trigger "I/O on closed
file"; move the exif.get_ifd(...) call into the with Image.open(path) as img:
block and immediately convert its result to a dict (e.g., exif_ifd =
dict(exif.get_ifd(ExifTags.IFD.Exif) if hasattr(ExifTags, "IFD") else {})) while
the file is open, keep the rest (if not exif: return "" and tags = dict(exif);
tags.update(exif_ifd)) unchanged so lazy-loaded IFD data is read before the file
is closed.
In `@faststack/tests/test_deletion_perf_structure.py`:
- Line 85: The test assigns the return of app._delete_indices([0], "test") to
summary but never uses it; either remove the unused variable or assert on it.
Edit the test in test_deletion_perf_structure.py to call
app._delete_indices([0], "test") without assigning to summary, or replace the
assignment with an assertion that validates the expected return value from
_delete_indices (reference the _delete_indices function name and the summary
variable to locate the line).
---
Outside diff comments:
In `@faststack/thumbnail_view/prefetcher.py`:
- Around line 381-395: The PIL fallback is leaving file handles open by calling
Image.open(path) without a context manager; wrap the open in a with
Image.open(path) as pil_img: block (inside the existing timer.stage("decode")
context) so the file is closed promptly, perform mode conversion and thumbnail
while inside that with, then return a copied array (e.g.,
np.array(pil_img.copy())) so the returned data isn't tied to the closed image;
keep the existing exception handling/logging around this block.
---
Duplicate comments:
In `@faststack/app.py`:
- Around line 2337-2340: The early return in the block comparing exif_key to
self._exif_pending_path can leave self._exif_pending_path stuck; update the code
in the method that computes exif_key (using _exif_source_key and current_index)
so that when self._exif_pending_path is not None and != exif_key you first
clear/reset self._exif_pending_path (e.g., set to None) before returning; ensure
you reference the existing symbols _exif_source_key, _exif_pending_path and
current_index so the pending flag is always cleared on debounce mismatch.
In `@faststack/qml/Main.qml`:
- Around line 996-1008: Ensure the status bar fallbacks are consistent and
remove the duplicated review marker: verify the first Label (displaying
currentFilename) uses "N/A" as the fallback and the second Label (exifBrief)
intentionally uses "—" only when uiState exists; if you intended both to be
identical change exifBrief to "N/A". Also remove the stray "[duplicate_comment]"
marker from the review/meta so there aren’t duplicate approval comments.
In `@faststack/tests/test_feedback_fixes.py`:
- Around line 69-74: The test test_image_editor_save_exception expects
ImageEditor.save_image to raise RuntimeError with message "No file path set"
when no image is loaded; implement or adjust ImageEditor.save_image (and related
initializer/state) so that if float_image is None or no output file path is set
it raises RuntimeError("No file path set") to satisfy the test assertion and
keep the test name unchanged.
---
Nitpick comments:
In `@faststack/app.py`:
- Around line 2357-2368: The EXIF read task is currently submitted to the
single-worker histogram executor (_hist_executor) via the local _worker (calling
get_exif_brief) which can block histogram updates; create and use a dedicated
executor (e.g., _exif_executor with >1 worker) or increase
_hist_executor.max_workers, then submit _worker to that dedicated executor
instead of _hist_executor and ensure the new executor is created once (e.g., as
a member) and shut down appropriately; update references from submitting to
_hist_executor to submitting to the new _exif_executor so _exifBriefReady
emissions don't contend with histogram tasks.
In `@faststack/imaging/metadata.py`:
- Around line 125-135: get_exif_brief currently bails out for any file whose
suffix isn't in {".jpg", ".jpeg", ".jpe"} which silently excludes TIFF/HEIF
files that may contain EXIF; either document this limitation or extend the
suffix check to include common EXIF-bearing types (e.g., ".tif", ".tiff",
".heif", ".heic") and ensure downstream code (e.g., the early Path.exists()
check and any Pillow usage) still works for those formats — update the docstring
to state supported formats if you keep the restriction, or expand the set in the
path.suffix check inside get_exif_brief and add a brief comment about
performance/hot-path considerations if relevant.
- Around line 163-168: The f-number conversion block swallows errors with a bare
except; replace it by catching specific exceptions (ValueError, TypeError) and
log the failure instead of silently passing: when converting f_number to float
in the block that appends to parts (variables f_number, val, parts), log a debug
message including the raw f_number and exception (use the module logger or
logging.getLogger(__name__) with exc_info=True) so malformed EXIF values are
visible; apply the same change to the analogous try/except handling the other
EXIF conversion around lines 188–189 (the similar block that parses and appends
that exposure-related value).
In `@faststack/tests/debug_exif.py`:
- Around line 5-6: Move the standard-library imports tempfile and shutil so they
appear before any third-party or intra-package imports in
faststack/tests/debug_exif.py; specifically, place the tempfile and shutil
import statements at the top of the import block (stdlib group) and then follow
with the existing third-party and local imports to comply with PEP8/isort
ordering.
In `@faststack/tests/test_deletion_perf_structure.py`:
- Line 117: The size_of lambda passed to ByteLRUCache is ignoring its parameter
and should use the conventional unused-parameter name; update the call to
ByteLRUCache(1000, size_of=...) to supply a lambda that names the unused
argument "_" (e.g., replace the current size_of lambda's parameter name with
"_") so it's clear the parameter is intentionally unused (refer to ByteLRUCache
and the size_of argument).
In `@faststack/thumbnail_view/prefetcher.py`:
- Around line 456-476: cancel_all assumes timers from self._inflight are
non-None and sets timer.cancelled and reads timer.rid; make it defensive by
checking each timer is not None before touching it (in cancel_all) so future
changes to submit() won't cause AttributeError—iterate inflight_timers and for
each do an explicit if timer is not None: set timer.cancelled = True and call
thumb_debug.log_trace(..., rid=timer.rid).
- Around line 397-402: The _on_decode_done callback’s timer parameter is
annotated as "thumb_debug.ThumbTimer" but the method checks `if timer:`
defensively; update the signature to timer: Optional["thumb_debug.ThumbTimer"]
and import Optional from typing (or adjust existing typing imports) so the
annotation matches usage (alternatively, remove the defensive `if timer:` checks
if you decide timer will never be None); reference the _on_decode_done function
and the timer parameter when making the change.
In `@faststack/thumbnail_view/provider.py`:
- Around line 125-166: Replace the 8-element plain tuple return of _parse_id
with a NamedTuple to make field access explicit: define a typing.NamedTuple
(e.g., ParsedId) with fields id_clean, parts, thumb_size, path_hash, mtime_ns,
reason, is_folder, is_valid; change _parse_id's return annotation to -> ParsedId
and return ParsedId(...) instead of raw tuples in all return paths; update any
call sites that rely on positional unpacking to either continue to unpack (will
remain compatible) or — preferably — access attributes like parsed.path_hash and
parsed.is_valid where clarity is needed (search for usages of _parse_id and
replace tuple index references with attribute access).
- Around line 262-270: The _bytes_to_image function currently returns None on
decode failure which causes callers to re-submit prefetches for corrupted cached
bytes; change _bytes_to_image (in thumbnail_view/provider.py) to return a
non-null placeholder QImage (e.g., the same placeholder used by the
prefetcher/placeholder path) when loadFromData fails or an exception occurs, and
keep the debug log of the failure; ensure the returned placeholder is not
isNull() so callers won't requeue the entry and add a clear log message
indicating bytes were replaced with a placeholder.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/thumbnail_view/provider.py (1)
183-272:⚠️ Potential issue | 🟠 Major
sizeoutput parameter not set — QMLImageauto-sizing will breakQt's
QQuickImageProvider::requestImage()documentation requires: "In all cases,sizemust be set to the original size of the image." This is used by QML to set theImageelement'swidthandheightif those values are not set explicitly. The current implementation never sets thesizeparameter, causing any QMLImagethat relies on implicit sizing to render as 0×0 and appear invisible or misaligned.All four return paths need to set
sizebefore returning:
- Line 19 (
_error_placeholder): Set size to error placeholder dimensions- Line 22 (
_folder_placeholder): Set size to folder placeholder dimensions- Line 72 (cached image): Set size to decoded image dimensions
- Line 90 (
_placeholder): Set size to placeholder dimensions🔧 Proposed fix
if image and not image.isNull(): + size.setWidth(image.width()) + size.setHeight(image.height()) if timer: ... return image + size.setWidth(self._placeholder.width()) + size.setHeight(self._placeholder.height()) # Return placeholder immediately (non-blocking) return self._placeholderAlso apply size-setting to early returns at lines 19 and 22.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 183 - 272, The requestImage method never sets the out-parameter size (QSize) before returning, causing QML implicit sizing to break; update requestImage to assign the proper QSize on every return: when returning self._error_placeholder set size to that placeholder's dimensions, when returning self._folder_placeholder set size to the folder placeholder dimensions, when returning a decoded image from self._bytes_to_image set size to the decoded QImage's width/height, and when returning self._placeholder set size to the placeholder dimensions; locate these in requestImage (use symbols cache_key, cached_bytes, _bytes_to_image, _error_placeholder, _folder_placeholder, _placeholder) and ensure size is assigned to the original image size immediately prior to each return.
🧹 Nitpick comments (8)
faststack/tests/test_deletion_perf_structure.py (1)
17-45: Remove unusedmock_appfixture or refactor tests to use it.The
mock_appfixture (lines 17–45) is defined but never used—none of the three test functions declare it as a parameter.test_delete_uses_targeted_eviction(lines 48–109) duplicates the same 8-patch block inline instead of reusing the fixture. Either wire the fixture into the tests that need it and remove the inline patch blocks, or delete the fixture entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/test_deletion_perf_structure.py` around lines 17 - 45, The mock_app pytest fixture is defined but unused; either remove it or have tests reuse it to avoid duplicate patch setup. Fix by either deleting the mock_app fixture (remove function mock_app) if not needed, or refactor tests like test_delete_uses_targeted_eviction to accept the mock_app parameter and drop the in-function repeated patch block (replace the inline patch/patches and local mock_engine/AppController setup with the provided mock_app). Ensure references to app.image_cache.evict_paths, app.prefetcher, app._thumbnail_model, app._path_resolver, app._path_to_index, app.sidecar, and app._delete_executor continue to work with the reused mock_app.faststack/qml/Main.qml (1)
1002-1006: Consider hiding the exifBrief label when empty instead of showing "N/A".For images without EXIF data (e.g., PNGs, screenshots), the label will permanently display "N/A", adding visual noise to the status bar. It would be cleaner to hide it when there's nothing meaningful to display, consistent with how other status labels (Stacked, Uploaded, Favorite) use
visible:to conditionally show.Suggested approach
Label { - text: (uiState && uiState.imageCount > 0) - ? (uiState.exifBrief || "N/A") - : "N/A" + text: (uiState && uiState.exifBrief) ? uiState.exifBrief : "" color: root.currentTextColor + visible: uiState ? (uiState.imageCount > 0 && uiState.exifBrief !== "") : false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/Main.qml` around lines 1002 - 1006, The EXIF label should be hidden when there's no meaningful data: update the Label (the component with text currently using uiState.exifBrief || "N/A" and color root.currentTextColor) to set visible to a condition like (uiState && uiState.imageCount > 0 && uiState.exifBrief && uiState.exifBrief.length > 0) and change the text to use uiState.exifBrief (or an empty string) rather than "N/A", so the label is only shown when exifBrief has content.faststack/imaging/metadata.py (2)
155-163: ISO value not validated as displayable after unwrapping from list.After unwrapping a list/tuple ISO value at line 162, the raw value is directly interpolated into the f-string at line 163. If the value happens to be a non-integer type (e.g., a Pillow
IFDRational), the display could look odd. Consider casting tointfor a clean display like"ISO 800"instead of potentially"ISO IFDRational(800, 1)".Suggested fix
if iso is not None: # Some cameras return a list/tuple for ISO if isinstance(iso, (list, tuple)) and len(iso) > 0: iso = iso[0] - parts.append(f"ISO {iso}") + try: + parts.append(f"ISO {int(iso)}") + except (ValueError, TypeError): + parts.append(f"ISO {iso}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` around lines 155 - 163, The ISO handling block that reads iso = tags.get(0x8827) and unwraps list/tuple should cast the unwrapped value to an integer for display; after the list/tuple unwrap in the ISO branch convert the value via int(iso) (with a safe fallback to str(iso) if int() raises TypeError/ValueError) before doing parts.append(f"ISO {iso}") so the appended string becomes a clean numeric display like "ISO 800" and not a representation like "IFDRational(800, 1)".
185-198: Silentexcept: passon date parsing swallows errors without any trace.The static analysis tool (S110) correctly flags lines 195–196. If
clean_exif_valueor the string split logic fails here, there's zero diagnostic output. This makes debugging EXIF quirks unnecessarily hard. Alog.debugwould maintain the defensive posture while preserving observability.Suggested fix
except Exception: - pass + log.debug("Failed to parse capture time from %r", dt, exc_info=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` around lines 185 - 198, The silent bare except around the EXIF date parsing should be replaced with an explicit exception handler that logs the failure so issues aren't swallowed: change the try/except to catch Exception as e, call a debug log including the offending dt and exception (e.g., logger.debug("failed parsing EXIF date %r: %s", dt, e)), and continue to append nothing on failure; ensure a module-level logger (logger = logging.getLogger(__name__)) is present or imported if needed; keep behavior of not raising so parsing remains defensive while preserving observability for clean_exif_value, dt, and parts.faststack/thumbnail_view/prefetcher.py (3)
506-538:ThumbnailCachehas O(n) LRU operations on everygetandput
_order.remove(key)(lines 512, 523) and_order.pop(0)(line 535) are all O(n) on alist. Withmax_items=5000and this cache sitting on the hot path during grid scrolling, each access can scan thousands of entries. Replace the_cache/_orderpair withcollections.OrderedDictfor O(1) move-to-end and O(1) popitem eviction.♻️ Proposed refactor using OrderedDict
+from collections import OrderedDict class ThumbnailCache: def __init__(self, max_bytes: int = 256 * 1024 * 1024, max_items: int = 5000): self._max_bytes = max_bytes self._max_items = max_items - self._cache: Dict[str, bytes] = {} - self._order: list = [] # LRU order (oldest first) + self._cache: OrderedDict[str, bytes] = OrderedDict() self._current_bytes = 0 self._lock = Lock() def get(self, key: str) -> Optional[bytes]: with self._lock: if key not in self._cache: return None - self._order.remove(key) - self._order.append(key) + self._cache.move_to_end(key) return self._cache[key] def put(self, key: str, value: bytes): with self._lock: if key in self._cache: old_value = self._cache[key] self._current_bytes -= len(old_value) - self._order.remove(key) + del self._cache[key] self._cache[key] = value - self._order.append(key) + self._cache.move_to_end(key) self._current_bytes += len(value) while ( self._current_bytes > self._max_bytes or len(self._cache) > self._max_items - ) and self._order: - oldest = self._order.pop(0) - if oldest in self._cache: - self._current_bytes -= len(self._cache[oldest]) - del self._cache[oldest] + ) and self._cache: + oldest_key, oldest_val = self._cache.popitem(last=False) + self._current_bytes -= len(oldest_val) def clear(self): with self._lock: self._cache.clear() - self._order.clear() self._current_bytes = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 506 - 538, The current ThumbnailCache implementation uses a list _order and dict _cache causing O(n) operations in ThumbnailCache.get and ThumbnailCache.put due to _order.remove(key) and _order.pop(0); replace the pair with a single collections.OrderedDict (or dict+collections.deque with bookkeeping) so move-to-end and eviction become O(1). Modify ThumbnailCache to store bytes in an OrderedDict (e.g., self._cache: OrderedDict[str, bytes]), update get to use move_to_end(key, last=True) and return the value, update put to set/overwrite entries with move_to_end and use popitem(last=False) to evict while updating _current_bytes, and remove all usages of _order; ensure thread safety around the same self._lock.
168-212: Deadif timer:branches after unconditional timer assignment at line 169After line 168–169,
timeris guaranteed non-Nonefor the rest ofsubmit. The guards at lines 185, 193, and 200 are always-true, and the ternarytimer.rid if timer else 0at line 212 always resolves totimer.rid. These add noise without safety value.♻️ Proposed cleanup
if job_key in self._inflight: existing_rid, existing_timer = self._inflight[job_key] if existing_timer: existing_timer.coalesced_from = timer.reason if ( existing_timer.prio_effective is not None and priority < existing_timer.prio_effective ): existing_timer.prio_effective = priority - if timer: - thumb_debug.log_trace( - "prio_bump", - rid=existing_timer.rid, - new_prio=priority, - triggered_by_rid=timer.rid, - ) + thumb_debug.log_trace( + "prio_bump", + rid=existing_timer.rid, + new_prio=priority, + triggered_by_rid=timer.rid, + ) - if timer: - thumb_debug.inc("decode_coalesced") - thumb_debug.log_trace( - "coalesced", rid=timer.rid, existing_rid=existing_rid - ) + thumb_debug.inc("decode_coalesced") + thumb_debug.log_trace( + "coalesced", rid=timer.rid, existing_rid=existing_rid + ) return False - if timer: - timer.t_queued = time.perf_counter() - ... + timer.t_queued = time.perf_counter() + ... - self._inflight[job_key] = (timer.rid if timer else 0, timer) + self._inflight[job_key] = (timer.rid, timer)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 168 - 212, In submit (prefetcher.py) the local timer is always set (timer = thumb_debug.ThumbTimer(...)) before the inflight logic, so remove redundant conditional branches that check "if timer:" around the coalescing/metrics/logging blocks and simplify the inflight insertion to use timer.rid directly instead of the ternary; specifically, delete the unnecessary "if timer:" guards around the decoders' coalesced/queued metrics and log_trace calls and change self._inflight[job_key] = (timer.rid if timer else 0, timer) to self._inflight[job_key] = (timer.rid, timer) to reduce dead code while preserving the existing behavior.
99-101: Type annotation inconsistency:_inflightvalue declaresThumbTimer(non-Optional), butcancel_allguards againstNoneThe annotation at lines 99–101 says
Tuple[int, "thumb_debug.ThumbTimer"](non-Optional timer), yetcancel_all(line 471) hasif timer is not None. Sincesubmitnow unconditionally creates a timer at line 168–169, the stored timer is always non-None. Either:
- Keep the annotation as-is and remove the dead guard in
cancel_all, or- Widen the annotation to
Optional["thumb_debug.ThumbTimer"]if you want to preserve the defensiveness.♻️ Option A — annotation is correct, remove dead guard in cancel_all
for timer in inflight_timers: - if timer is not None: - timer.cancelled = True - thumb_debug.log_trace("cancel_requested", rid=timer.rid) + timer.cancelled = True + thumb_debug.log_trace("cancel_requested", rid=timer.rid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 99 - 101, The _inflight dict's value type is declared as Tuple[int, "thumb_debug.ThumbTimer"] but cancel_all checks for timer is not None; to keep the code defensive, change the type annotation on self._inflight to Tuple[int, Optional["thumb_debug.ThumbTimer"]] (and import typing.Optional if not already imported), leaving cancel_all's guard intact; ensure any other references that assume a non-None timer handle Optional accordingly (look for uses in cancel_all, submit, and any timer cancellation paths).faststack/thumbnail_view/provider.py (1)
215-261:_path_resolvercalled twice for the samepath_hashon a cache missLine 215 resolves the path for trace logging, and line 261 resolves it again unconditionally on a cache miss. Since
_path_resolveris a dict lookup it's cheap, but the duplication is unnecessary. Hoist the resolved path before the cache check and reuse it.♻️ Proposed fix
+ path = self._path_resolver(parsed.path_hash) if self._path_resolver else None if thumb_debug.timing_enabled or thumb_debug.trace_enabled: - path = self._path_resolver(parsed.path_hash) if self._path_resolver else None timer = thumb_debug.ThumbTimer( key=cache_key, path=path, reason=parsed.reason ) ... ... - path = self._path_resolver(parsed.path_hash) if self._path_resolver else None if path: self._prefetcher.submit(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 215 - 261, Compute path once from self._path_resolver(parsed.path_hash) (or None if no resolver) before the cache check and reuse that variable for the ThumbTimer construction and later on cache miss instead of calling _path_resolver twice; update the code around the timer creation (where path is passed to thumb_debug.ThumbTimer) and remove the second unconditional call to self._path_resolver near the cache-miss handling so the single resolved path variable is used for logging and any further logic (references: _path_resolver, parsed.path_hash, timer, thumb_debug.ThumbTimer, self._cache.get, self._bytes_to_image).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/app.py`:
- Around line 2317-2361: The early-return in _read_exif_deferred gates EXIF work
to _JPEG_SUFFIXES (so TIFF/HEIF never get a brief) while get_exif_brief()
supports more formats; update _read_exif_deferred to either remove the suffix
check entirely and let get_exif_brief(source_path) decide supported formats, or
expand _JPEG_SUFFIXES to include the TIFF/HEIF extensions used by
get_exif_brief(); ensure you still set self._exif_brief_cache[exif_key] and
clear self._exif_pending_path in the unsupported case (same behavior as today)
and keep the existing resolution logic
(get_group_key_for_path/self._variant_map) intact.
In `@faststack/imaging/metadata.py`:
- Around line 213-220: get_exif_data currently only converts exif_obj to dict
which yields IFD0 tags and misses the Exif sub-IFD (where ISO, FNumber,
ExposureTime, LensModel, DateTimeOriginal live); while the image is still open,
call exif_obj.get_ifd(ExifTags.IFD.Exif) (like get_exif_brief does), merge that
returned mapping into the exif dict (ensuring keys from the sub-IFD override or
supplement IFD0 as appropriate) before you run the existing tag-decoding logic
in get_exif_data so the summary fields populate correctly; also ensure ExifTags
is available/imported where get_exif_data is defined.
In `@faststack/tests/test_deletion_perf_structure.py`:
- Around line 83-86: Remove the two debug print statements surrounding the call
to app._delete_indices in the test (the lines printing "DEBUG: Calling
_delete_indices" and "DEBUG: Returned from _delete_indices"); leave the
Arrange/Act/Assert flow intact and only call app._delete_indices([0], "test")
without emitting prints.
In `@faststack/thumbnail_view/prefetcher.py`:
- Line 137: The parameter annotation "size: int = None" violates PEP 484; change
the parameter type to an explicit optional (e.g., "size: Optional[int]" or
"size: int | None") and add the required import ("from typing import Optional")
if using Optional[...] so the signature in prefetcher.py uses an explicit
Optional for the size parameter.
In `@faststack/thumbnail_view/provider.py`:
- Around line 280-290: The except branch in _bytes_to_image currently logs the
same failure twice (log.debug and log.warning); remove the redundant
log.debug("Failed to convert bytes to image: %s", e) and keep the single
log.warning that includes the exception details so the failure is only logged
once while retaining the error context in the warning message.
---
Outside diff comments:
In `@faststack/thumbnail_view/provider.py`:
- Around line 183-272: The requestImage method never sets the out-parameter size
(QSize) before returning, causing QML implicit sizing to break; update
requestImage to assign the proper QSize on every return: when returning
self._error_placeholder set size to that placeholder's dimensions, when
returning self._folder_placeholder set size to the folder placeholder
dimensions, when returning a decoded image from self._bytes_to_image set size to
the decoded QImage's width/height, and when returning self._placeholder set size
to the placeholder dimensions; locate these in requestImage (use symbols
cache_key, cached_bytes, _bytes_to_image, _error_placeholder,
_folder_placeholder, _placeholder) and ensure size is assigned to the original
image size immediately prior to each return.
---
Nitpick comments:
In `@faststack/imaging/metadata.py`:
- Around line 155-163: The ISO handling block that reads iso = tags.get(0x8827)
and unwraps list/tuple should cast the unwrapped value to an integer for
display; after the list/tuple unwrap in the ISO branch convert the value via
int(iso) (with a safe fallback to str(iso) if int() raises TypeError/ValueError)
before doing parts.append(f"ISO {iso}") so the appended string becomes a clean
numeric display like "ISO 800" and not a representation like "IFDRational(800,
1)".
- Around line 185-198: The silent bare except around the EXIF date parsing
should be replaced with an explicit exception handler that logs the failure so
issues aren't swallowed: change the try/except to catch Exception as e, call a
debug log including the offending dt and exception (e.g., logger.debug("failed
parsing EXIF date %r: %s", dt, e)), and continue to append nothing on failure;
ensure a module-level logger (logger = logging.getLogger(__name__)) is present
or imported if needed; keep behavior of not raising so parsing remains defensive
while preserving observability for clean_exif_value, dt, and parts.
In `@faststack/qml/Main.qml`:
- Around line 1002-1006: The EXIF label should be hidden when there's no
meaningful data: update the Label (the component with text currently using
uiState.exifBrief || "N/A" and color root.currentTextColor) to set visible to a
condition like (uiState && uiState.imageCount > 0 && uiState.exifBrief &&
uiState.exifBrief.length > 0) and change the text to use uiState.exifBrief (or
an empty string) rather than "N/A", so the label is only shown when exifBrief
has content.
In `@faststack/tests/test_deletion_perf_structure.py`:
- Around line 17-45: The mock_app pytest fixture is defined but unused; either
remove it or have tests reuse it to avoid duplicate patch setup. Fix by either
deleting the mock_app fixture (remove function mock_app) if not needed, or
refactor tests like test_delete_uses_targeted_eviction to accept the mock_app
parameter and drop the in-function repeated patch block (replace the inline
patch/patches and local mock_engine/AppController setup with the provided
mock_app). Ensure references to app.image_cache.evict_paths, app.prefetcher,
app._thumbnail_model, app._path_resolver, app._path_to_index, app.sidecar, and
app._delete_executor continue to work with the reused mock_app.
In `@faststack/thumbnail_view/prefetcher.py`:
- Around line 506-538: The current ThumbnailCache implementation uses a list
_order and dict _cache causing O(n) operations in ThumbnailCache.get and
ThumbnailCache.put due to _order.remove(key) and _order.pop(0); replace the pair
with a single collections.OrderedDict (or dict+collections.deque with
bookkeeping) so move-to-end and eviction become O(1). Modify ThumbnailCache to
store bytes in an OrderedDict (e.g., self._cache: OrderedDict[str, bytes]),
update get to use move_to_end(key, last=True) and return the value, update put
to set/overwrite entries with move_to_end and use popitem(last=False) to evict
while updating _current_bytes, and remove all usages of _order; ensure thread
safety around the same self._lock.
- Around line 168-212: In submit (prefetcher.py) the local timer is always set
(timer = thumb_debug.ThumbTimer(...)) before the inflight logic, so remove
redundant conditional branches that check "if timer:" around the
coalescing/metrics/logging blocks and simplify the inflight insertion to use
timer.rid directly instead of the ternary; specifically, delete the unnecessary
"if timer:" guards around the decoders' coalesced/queued metrics and log_trace
calls and change self._inflight[job_key] = (timer.rid if timer else 0, timer) to
self._inflight[job_key] = (timer.rid, timer) to reduce dead code while
preserving the existing behavior.
- Around line 99-101: The _inflight dict's value type is declared as Tuple[int,
"thumb_debug.ThumbTimer"] but cancel_all checks for timer is not None; to keep
the code defensive, change the type annotation on self._inflight to Tuple[int,
Optional["thumb_debug.ThumbTimer"]] (and import typing.Optional if not already
imported), leaving cancel_all's guard intact; ensure any other references that
assume a non-None timer handle Optional accordingly (look for uses in
cancel_all, submit, and any timer cancellation paths).
In `@faststack/thumbnail_view/provider.py`:
- Around line 215-261: Compute path once from
self._path_resolver(parsed.path_hash) (or None if no resolver) before the cache
check and reuse that variable for the ThumbTimer construction and later on cache
miss instead of calling _path_resolver twice; update the code around the timer
creation (where path is passed to thumb_debug.ThumbTimer) and remove the second
unconditional call to self._path_resolver near the cache-miss handling so the
single resolved path variable is used for logging and any further logic
(references: _path_resolver, parsed.path_hash, timer, thumb_debug.ThumbTimer,
self._cache.get, self._bytes_to_image).
| self, | ||
| path: Path, | ||
| mtime_ns: int, | ||
| size: int = None, |
There was a problem hiding this comment.
size: int = None violates PEP 484 — fix the implicit Optional
Ruff RUF013 flags this correctly.
🔧 Proposed fix
- size: int = None,
+ size: Optional[int] = None,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| size: int = None, | |
| size: Optional[int] = None, |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 137-137: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/thumbnail_view/prefetcher.py` at line 137, The parameter annotation
"size: int = None" violates PEP 484; change the parameter type to an explicit
optional (e.g., "size: Optional[int]" or "size: int | None") and add the
required import ("from typing import Optional") if using Optional[...] so the
signature in prefetcher.py uses an explicit Optional for the size parameter.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
faststack/imaging/metadata.py (1)
337-370:⚠️ Potential issue | 🟠 MajorGPS coordinates are silently never extracted under Pillow ≥ 8.2 (confirmed on 10.0).
In Pillow 8.2.0+, direct access to sub-IFD dictionaries via
im.getexif()[tag_offset]was removed; they must now be accessed viaim.getexif().get_ifd(tag_constant). Whendict(exif_obj)is called at line 228, the GPSInfo tag (0x8825) is included as a raw IFD offset integer—not a dict. Line 235 then maps this to the name "GPSInfo" indecoded_exif. Soget_val("GPSInfo")returns an integer. The guardif gps_info:passes (the integer is truthy), butif 2 in gps_infoat line 354 raisesTypeError: argument of type 'int' is not iterable, which theexcept Exceptionat line 364 silently swallows withpass. GPS coordinates are never extracted.Fix: Fetch the GPS sub-IFD using the same pattern already applied to the Exif sub-IFD at line 223. Add the following at line 224:
exif_ifd = dict(exif_obj.get_ifd(ExifTags.IFD.Exif) if hasattr(ExifTags, "IFD") else {}) + gps_ifd = dict(exif_obj.get_ifd(ExifTags.IFD.GPSInfo) if hasattr(ExifTags, "IFD") else {})Then replace the GPS extraction block (lines 336–370) to use the fetched dict:
- # GPS - gps_info = get_val("GPSInfo") - if gps_info: + # GPS (fetched via get_ifd inside the with block above) + if gps_ifd: try: + gps_info = gps_ifd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` around lines 337 - 370, The GPS extraction fails because get_val("GPSInfo") can be an integer IFD offset under Pillow ≥8.2 instead of a dict; update the code to fetch the GPS sub-IFD via exif_obj.get_ifd(0x8825) (same pattern used for the Exif sub-IFD) before using get_val("GPSInfo"), then change the GPS parsing block that references get_val("GPSInfo") and checks membership (e.g., "if 2 in gps_info") to operate on the fetched dict (or skip parsing if get_ifd returned None); ensure you keep the convert_to_degrees helper and the latitude/longitude sign handling but only run them when gps_info is an actual mapping.faststack/app.py (1)
4913-4921:⚠️ Potential issue | 🟡 MinorShutdown the EXIF executor with the other pools.
Line 4913-4921:
shutdown_nonqt()stops hist/preview/save/delete executors, but_exif_executorremains live. To avoid late emits during shutdown, close it here as well (or delegate to the consolidated shutdown helper).🛠️ Suggested fix
try: log.info("Shutting down background executors...") self._hist_executor.shutdown(wait=False, cancel_futures=True) self._preview_executor.shutdown(wait=False, cancel_futures=True) + self._exif_executor.shutdown(wait=False, cancel_futures=True) # wait=True ensures pending saves/deletes complete to avoid data loss/corruption self._save_executor.shutdown(wait=True, cancel_futures=False) self._delete_executor.shutdown(wait=True, cancel_futures=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 4913 - 4921, The shutdown block in shutdown_nonqt() currently stops _hist_executor, _preview_executor, _save_executor and _delete_executor but omits _exif_executor; add a shutdown call for self._exif_executor (e.g., matching the short-lived background pools like _hist_executor/_preview_executor using shutdown(wait=False, cancel_futures=True) or otherwise delegate to the shared shutdown helper) so the EXIF worker is closed during application shutdown and cannot emit late events.
🧹 Nitpick comments (4)
faststack/app.py (1)
3491-3499: Clear pending EXIF state on directory switch.Line 3491-3499: You clear
_exif_brief_cachebut keep_exif_pending_path. Clearing the pending key here avoids stale state if the same key is reused after switching folders.♻️ Suggested tweak
self._metadata_cache = {} self._metadata_cache_index = (-1, -1) self._exif_brief_cache.clear() + self._exif_pending_path = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 3491 - 3499, When resetting state on directory switch (the block that clears last_displayed_image, image_cache, prefetcher, display_generation, _metadata_cache/_index, and _exif_brief_cache), also clear the pending EXIF key by resetting _exif_pending_path (e.g., set it to None or an empty value) to avoid retaining stale pending EXIF state; update the same reset block that references _exif_brief_cache to also reset _exif_pending_path.faststack/thumbnail_view/provider.py (2)
294-301: Consider narrowing the broadexcept Exception(RuffBLE001).
QImage.loadFromDatareturnsFalseon JPEG parse failure rather than raising, so theexceptblock only fires for unexpected Qt-binding-level errors. CatchingExceptionsilently swallows anything thrown, including programming bugs. If the intent is purely defensive against C++ interop exceptions, a comment documenting that is at minimum warranted; narrowing to(RuntimeError,)or re-raising after logging is preferable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 294 - 301, The broad except in the JPEG decode block (in provider.py around the QImage.loadFromData use) should be narrowed and documented: replace "except Exception as e" with a targeted exception type (e.g., "except RuntimeError as e") or log and re-raise after logging so genuine programming errors aren't silently swallowed; add a short comment explaining this is only to guard against Qt/C++ interop runtime errors and keep the existing fallback return None when a benign interop error occurs.
183-184: Suppress RuffARG002on the unusedrequestedSizeparameter.Qt calls this method positionally, so the parameter must remain in the signature, but Python has no issue with an
_prefix to signal intentional non-use.♻️ Proposed fix
- def requestImage(self, id_str: str, size: QSize, requestedSize: QSize) -> QImage: + def requestImage(self, id_str: str, size: QSize, _requestedSize: QSize) -> QImage:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 183 - 184, Rename the unused parameter in the requestImage method to suppress Ruff ARG002: change the signature of requestImage to use _requestedSize instead of requestedSize so Qt can still call it positionally but the underscore signals intentional non-use; update any internal references (if present) and adjust the docstring or parameter name mentions to match the new _requestedSize identifier.faststack/tests/thumbnail_view/test_provider_logic.py (1)
109-135: Rename unusedprefetcherto suppress RuffRUF059.
prefetcheris unpacked but never referenced in this test; prefix it with an underscore.♻️ Proposed fix
- provider, cache, prefetcher = wired_provider + provider, cache, _prefetcher = wired_providerAdditionally, the
PySide6.QtCoreandPLACEHOLDER_COLOR/ERROR_COLORimports inside the method (lines 116–117) could be moved to the module level —ThumbnailProvideralready requires PySide6 at import time, so the deferred imports give no benefit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/thumbnail_view/test_provider_logic.py` around lines 109 - 135, In test_bad_cached_bytes_returns_placeholder_not_error: rename the unused unpacked variable prefetcher to _prefetcher to silence Ruff RUF059, and move the in-function imports of PySide6.QtCore and PLACEHOLDER_COLOR/ERROR_COLOR to the top of the test module (they're already required by ThumbnailProvider at import time), so the test only references provider.requestImage and the color constants without deferred imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/app.py`:
- Around line 2367-2372: The EXIF worker function _worker should guard against
exceptions from get_exif_brief so a thrown error doesn't leave
_exif_pending_path uncleared; wrap the call to get_exif_brief(p) in a
try/except, and in the except branch call signal.emit(key, {}) (or an empty
brief value) to ensure the pending key is cleared and the UI updates, and
optionally log the exception via the existing logger; modify the _worker
function (which currently calls get_exif_brief and signal.emit) to always emit
for the key even on error.
In `@faststack/imaging/metadata.py`:
- Around line 198-199: Replace the silent "except Exception: pass" in
faststack.imaging.metadata with proper error handling: catch the exception as
"e", log the error (e.g., logger.exception or logger.error with the field name
and the offending datetime string) and then handle the failure explicitly (e.g.,
set the parsed datetime variable to None or raise a ValueError) instead of
swallowing it; ensure you reference the existing module logger (or create one
via logging.getLogger(__name__)) and include the original exception and raw
input in the log message so failures in the datetime parsing/formatting code
path are observable.
In `@faststack/thumbnail_view/prefetcher.py`:
- Around line 529-535: The loop unpacks (oldest_key, oldest_val) but never uses
oldest_key, triggering Ruff RUF059; change the unused variable name to start
with an underscore (e.g., _oldest_key or _) in the unpacking of
self._cache.popitem(last=False) inside the method that adjusts
self._current_bytes/self._cache, leaving oldest_val as is and keeping the
decrement logic using len(oldest_val).
In `@faststack/thumbnail_view/provider.py`:
- Around line 215-225: The inline comment "# Resolve path only if needed for
trace" is stale because path is resolved unconditionally earlier; update or
remove it to match the current logic: either move the path resolution into the
conditional that checks thumb_debug.timing_enabled or thumb_debug.trace_enabled
(so path = self._path_resolver(parsed.path_hash) happens only when constructing
thumb_debug.ThumbTimer), or simply delete/replace the comment to state that path
is resolved unconditionally before creating timer; adjust around variables
timer, cache_key, path, parsed and the conditional that constructs
thumb_debug.ThumbTimer accordingly.
---
Outside diff comments:
In `@faststack/app.py`:
- Around line 4913-4921: The shutdown block in shutdown_nonqt() currently stops
_hist_executor, _preview_executor, _save_executor and _delete_executor but omits
_exif_executor; add a shutdown call for self._exif_executor (e.g., matching the
short-lived background pools like _hist_executor/_preview_executor using
shutdown(wait=False, cancel_futures=True) or otherwise delegate to the shared
shutdown helper) so the EXIF worker is closed during application shutdown and
cannot emit late events.
In `@faststack/imaging/metadata.py`:
- Around line 337-370: The GPS extraction fails because get_val("GPSInfo") can
be an integer IFD offset under Pillow ≥8.2 instead of a dict; update the code to
fetch the GPS sub-IFD via exif_obj.get_ifd(0x8825) (same pattern used for the
Exif sub-IFD) before using get_val("GPSInfo"), then change the GPS parsing block
that references get_val("GPSInfo") and checks membership (e.g., "if 2 in
gps_info") to operate on the fetched dict (or skip parsing if get_ifd returned
None); ensure you keep the convert_to_degrees helper and the latitude/longitude
sign handling but only run them when gps_info is an actual mapping.
---
Nitpick comments:
In `@faststack/app.py`:
- Around line 3491-3499: When resetting state on directory switch (the block
that clears last_displayed_image, image_cache, prefetcher, display_generation,
_metadata_cache/_index, and _exif_brief_cache), also clear the pending EXIF key
by resetting _exif_pending_path (e.g., set it to None or an empty value) to
avoid retaining stale pending EXIF state; update the same reset block that
references _exif_brief_cache to also reset _exif_pending_path.
In `@faststack/tests/thumbnail_view/test_provider_logic.py`:
- Around line 109-135: In test_bad_cached_bytes_returns_placeholder_not_error:
rename the unused unpacked variable prefetcher to _prefetcher to silence Ruff
RUF059, and move the in-function imports of PySide6.QtCore and
PLACEHOLDER_COLOR/ERROR_COLOR to the top of the test module (they're already
required by ThumbnailProvider at import time), so the test only references
provider.requestImage and the color constants without deferred imports.
In `@faststack/thumbnail_view/provider.py`:
- Around line 294-301: The broad except in the JPEG decode block (in provider.py
around the QImage.loadFromData use) should be narrowed and documented: replace
"except Exception as e" with a targeted exception type (e.g., "except
RuntimeError as e") or log and re-raise after logging so genuine programming
errors aren't silently swallowed; add a short comment explaining this is only to
guard against Qt/C++ interop runtime errors and keep the existing fallback
return None when a benign interop error occurs.
- Around line 183-184: Rename the unused parameter in the requestImage method to
suppress Ruff ARG002: change the signature of requestImage to use _requestedSize
instead of requestedSize so Qt can still call it positionally but the underscore
signals intentional non-use; update any internal references (if present) and
adjust the docstring or parameter name mentions to match the new _requestedSize
identifier.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/app.py (1)
3494-3504:⚠️ Potential issue | 🟡 MinorStop the EXIF debounce timer on directory switches to avoid stale reads.
If a debounce timer from the previous directory fires after
_switch_to_directory, it can enqueue an EXIF read for a no‑longer‑relevant path and repopulate the cleared cache. Stopping the timer here avoids unnecessary I/O and stale cache entries (Line 3499).🧹 Suggested fix
self._metadata_cache = {} self._metadata_cache_index = (-1, -1) + self._exif_debounce_timer.stop() self._exif_brief_cache.clear() self._exif_pending_path = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/app.py` around lines 3494 - 3504, In _switch_to_directory, after clearing caches and before returning, stop and clear the EXIF debounce timer so any previously scheduled EXIF read for the old directory cannot fire and repopulate _exif_brief_cache; specifically, check for the timer instance (e.g. self._exif_brief_timer or self._exif_debounce_timer), call its cancel()/stop() method if present, and set that attribute to None (also ensure _exif_pending_path is cleared as shown) to fully prevent stale callbacks from executing.
🧹 Nitpick comments (14)
faststack/tests/test_metadata.py (1)
39-41: Optional: use_for intentionally unused stub parameters.Four Ruff
ARG002warnings point to the same pattern — stub implementations and patched-but-unreferenced fixtures. The idiomatic fix is to prefix with_(or rename to_tag/_mock_exists).♻️ Proposed fix
class MockExif(dict): - def get_ifd(self, tag): + def get_ifd(self, _tag): return {}- def test_get_exif_data_no_exif(self, mock_open, mock_exists): + def test_get_exif_data_no_exif(self, mock_open, _mock_exists):- def test_get_exif_brief_failure(self, mock_open, mock_log, mock_exists): + def test_get_exif_brief_failure(self, mock_open, mock_log, _mock_exists):class MockExif(dict): - def get_ifd(self, tag): return {} + def get_ifd(self, _tag): return {}Also applies to: 100-100, 118-118, 123-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/test_metadata.py` around lines 39 - 41, The stubbed parameters and patched-but-unreferenced fixtures (e.g., MockExif.get_ifd(tag)) should be renamed to show they are intentionally unused; update signatures and fixture argument names to use a leading underscore (for example change get_ifd(self, tag) to get_ifd(self, _tag) and rename any unused test fixture parameters like mock_exists to _mock_exists) so Ruff ARG002 warnings are resolved while leaving implementations unchanged.faststack/imaging/metadata.py (4)
155-155:list[str](PEP 585) is inconsistent with thetypingimports and requires Python ≥ 3.9.The module already imports
Dict,Optional, andUnionfromtyping(Python 3.8-compatible spelling). Mixing in PEP 585 built-in generics annotation likelist[str]at runtime is fine only if Python 3.9+ is guaranteed. If 3.8 is a supported target, this raisesTypeErrorat runtime.♻️ Proposed fix (option A — keep old-style typing for consistency)
-from typing import Any, Dict, Optional, Union +from typing import Any, Dict, List, Optional, Union- parts: list[str] = [] + parts: List[str] = []♻️ Proposed fix (option B — migrate all typing imports to PEP 585/604 if Python 3.9+ is confirmed)
-from typing import Any, Dict, Optional, Union +from typing import AnyThen use
dict[str, Any],list[str],X | None,X | Ythroughout the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` at line 155, The annotation for the variable parts uses PEP 585-style list[str] which is inconsistent with the module's typing imports and may break on Python 3.8; change the annotation to use the typing module (List[str]) and add List to the existing typing imports (or, if the project targets Python 3.9+, migrate all annotations to PEP 585 forms consistently across the file); update the declaration of parts in metadata.py (the parts variable) accordingly so it matches the rest of the file's typing style.
223-227: UseExifTags.IFD.GPSInfoinstead of the raw integer0x8825for consistency.Line 223 reads the Exif sub-IFD via the named constant
ExifTags.IFD.Exif; line 227 falls back to the bare integer0x8825for GPSInfo. Using the named constant improves readability and aligns with the rest of the file.♻️ Proposed fix
- gps_ifd = dict(exif_obj.get_ifd(0x8825)) if hasattr(ExifTags, "IFD") else {} + gps_ifd = dict(exif_obj.get_ifd(ExifTags.IFD.GPSInfo)) if hasattr(ExifTags, "IFD") else {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` around lines 223 - 227, Replace the hard-coded GPS IFD numeric literal with the named ExifTags constant for consistency: when constructing gps_ifd (in the same block that builds exif_ifd from exif_obj.get_ifd), use ExifTags.IFD.GPSInfo instead of 0x8825 and keep the same hasattr(ExifTags, "IFD") guard and dict(...) wrapping so behavior and compatibility with Pillow versions remain unchanged.
367-369: Redundantpassafterlog.warning.
passis a no-op here; removing it reduces visual noise.♻️ Proposed fix
except Exception as e: log.warning(f"Failed to parse GPS info: {e}") - pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` around lines 367 - 369, Remove the redundant no-op "pass" in the except block that logs GPS parse failures; specifically, inside the exception handler where log.warning(f"Failed to parse GPS info: {e}") is called (in the GPS parsing logic in faststack.imaging.metadata.py), delete the trailing "pass" so the except block only contains the warning call.
169-175:get_exif_briefhandles FNumber inconsistently vsget_exif_data.
get_exif_data(lines 296–297) explicitly handles tuple-format FNumber viaf_number[0] / f_number[1], whileget_exif_briefonly callsfloat(f_number). With Pillow 10.xIFDRationalvalues this is fine, but older or non-standard EXIF data may store it as a bare(n, d)tuple, silently dropping aperture from the brief.♻️ Proposed fix for consistency
- try: - val = float(f_number) - parts.append(f"f/{val:.1f}") - except (ValueError, TypeError) as e: - log.debug(f"Failed to convert f_number {f_number!r}: {e}", exc_info=True) + try: + if isinstance(f_number, (tuple, list)) and len(f_number) == 2: + val = f_number[0] / f_number[1] + else: + val = float(f_number) + parts.append(f"f/{val:.1f}") + except (ValueError, TypeError, ZeroDivisionError) as e: + log.debug("Failed to convert f_number %r: %s", f_number, e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/imaging/metadata.py` around lines 169 - 175, get_exif_brief currently converts the FNumber tag by calling float(f_number) which loses aperture when the tag is a bare (numerator, denominator) tuple; update get_exif_brief (the block reading tags.get(0x829D) / variable f_number) to mirror get_exif_data's handling: if f_number is a 2‑element tuple/list, compute val = f_number[0] / f_number[1]; otherwise attempt float(f_number) in a try/except and append f/{val:.1f}; on failure keep the existing debug log path. This ensures tuple-format FNumber values are handled consistently with get_exif_data.faststack/thumbnail_view/prefetcher.py (6)
556-563:sizeandbytes_usedread shared state without holding_lockAll mutations to
_current_bytesand_cacheare guarded by_lock, but these properties bypass it. CPython's GIL makes this safe today, but it's inconsistent with the class's own locking discipline and would be a data race under Python 3.13+ free-threaded builds.♻️ Proposed fix
`@property` def size(self) -> int: """Current number of items.""" - return len(self._cache) + with self._lock: + return len(self._cache) `@property` def bytes_used(self) -> int: """Current bytes used.""" - return self._current_bytes + with self._lock: + return self._current_bytes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 556 - 563, The properties size and bytes_used read shared state without holding the class lock; update the size and bytes_used property getters to acquire self._lock before accessing self._cache and self._current_bytes (and release it afterwards) so they follow the same locking discipline as mutators; specifically, wrap the reads of self._cache in size and of self._current_bytes in bytes_used with the existing self._lock to avoid a data race.
556-563:sizeandbytes_usedproperties read shared state without the lockAll mutations to
_current_bytesand_cacheare lock-guarded, but these properties read them without acquiring_lock. CPython's GIL makes this safe today, but it's inconsistent with the rest of the class and would be a data race in free-threaded Python 3.13+.♻️ Proposed fix
`@property` def size(self) -> int: """Current number of items.""" - return len(self._cache) + with self._lock: + return len(self._cache) `@property` def bytes_used(self) -> int: """Current bytes used.""" - return self._current_bytes + with self._lock: + return self._current_bytes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 556 - 563, The properties size and bytes_used read shared state (_cache and _current_bytes) without holding the instance lock, causing a race; update the size and bytes_used property implementations to acquire the existing _lock while reading those fields (e.g., use the lock as a context manager around len(self._cache) and returning self._current_bytes) so reads are consistent with the lock-guarded mutations performed elsewhere in the class.
169-170: Timer always created insubmiteven when timing is disabled
ThumbnailProvider.requestImagegates timer creation onthumb_debug.timing_enabled or trace_enabled, butsubmitunconditionally creates a fallback timer. This allocates aThumbTimeron every cache-miss even when all debug instrumentation is off.♻️ Proposed fix
- if timer is None: - timer = thumb_debug.ThumbTimer(key=cache_key, path=path, reason="prefetch") + if timer is None and (thumb_debug.timing_enabled or thumb_debug.trace_enabled): + timer = thumb_debug.ThumbTimer(key=cache_key, path=path, reason="prefetch")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 169 - 170, The submit method unconditionally creates a ThumbTimer on cache misses; change submit so it only instantiates thumb_debug.ThumbTimer(key=cache_key, path=path, reason="prefetch") when debug timing is enabled (use thumb_debug.timing_enabled or thumb_debug.trace_enabled) — keep timer as None otherwise so no allocation occurs; mirror the gating logic used in ThumbnailProvider.requestImage and ensure any later logic that expects timer handles a None timer safely.
519-524: Misleading comment inputThe comment
"(OrderedDict handles key existence)"implies the dict automatically handles the byte-count bookkeeping, which it does not — the code explicitly does it. The phrasing contradicts the next line ("But we must update _current_bytes").♻️ Proposed fix
- # If already present, remove old entry logic by just updating (OrderedDict handles key existence) - # But we must update _current_bytes first if it exists if key in self._cache: + # Update existing entry: adjust byte count and refresh LRU position self._current_bytes -= len(self._cache[key]) self._cache.move_to_end(key, last=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 519 - 524, The comment in the put method is misleading: OrderedDict only handles ordering, not byte-count bookkeeping; update the comment above the if-block that checks "if key in self._cache" to clearly state that we must subtract the old entry size from self._current_bytes before moving the key, e.g. note that OrderedDict.move_to_end handles ordering but we explicitly adjust _current_bytes when replacing an existing key (referencing _cache, _current_bytes, move_to_end, and key to locate the block). Ensure the new comment does not imply OrderedDict updates sizes automatically and succinctly describes the exact steps taken (subtract old bytes, then move_to_end).
169-170:ThumbTimerunconditionally created insubmitwhen timing is disabled
ThumbnailProvider.requestImageguards timer creation withthumb_debug.timing_enabled or trace_enabled, butsubmitalways creates a fallback timer for every cache-miss submit, even when all instrumentation is off.♻️ Proposed fix
- if timer is None: - timer = thumb_debug.ThumbTimer(key=cache_key, path=path, reason="prefetch") + if timer is None and (thumb_debug.timing_enabled or thumb_debug.trace_enabled): + timer = thumb_debug.ThumbTimer(key=cache_key, path=path, reason="prefetch")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 169 - 170, The submit method creates a ThumbTimer unconditionally on cache miss; change it to only instantiate the fallback timer when instrumentation is enabled by guarding the creation with the same condition used in ThumbnailProvider.requestImage (i.e. check thumb_debug.timing_enabled or thumb_debug.trace_enabled) so that ThumbTimer(key=cache_key, path=path, reason="prefetch") is only created when timer is None AND (thumb_debug.timing_enabled or thumb_debug.trace_enabled); keep existing variables (timer, cache_key, path) and behavior unchanged otherwise.
519-524: Misleading comment inputcontradicts itself
"(OrderedDict handles key existence)"implies the byte-count bookkeeping is automatic, which is false — the very next line says"But we must update _current_bytes first". The two sentences contradict each other.♻️ Proposed fix
if key in self._cache: - # If already present, remove old entry logic by just updating (OrderedDict handles key existence) - # But we must update _current_bytes first if it exists + # Update existing entry: adjust byte count and refresh LRU position self._current_bytes -= len(self._cache[key]) self._cache.move_to_end(key, last=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 519 - 524, The comment in put() is contradictory about OrderedDict handling key existence; update it to clearly state that OrderedDict only manages ordering but does not track byte sizes, so when a key exists in self._cache you must subtract len(self._cache[key]) from self._current_bytes before replacing or moving the entry; specifically revise the comment around the if key in self._cache: block to remove the misleading "(OrderedDict handles key existence)" text and explain that ordering is automatic but manual bookkeeping of self._current_bytes is required for correct byte-counting when updating _cache and calling _cache.move_to_end(key, last=True).faststack/thumbnail_view/provider.py (3)
217-219: Path resolved unconditionally before cache lookup — wasted call on every cache hitWhen timing/tracing is disabled,
pathis only consumed by the prefetchsubmitcall on cache misses. Moving the resolver call to after the cache check avoids calling it on hits.♻️ Proposed restructuring
- # Resolve path - we already have path_hash and mtime_ns - path = self._path_resolver(parsed.path_hash) if self._path_resolver else None - if thumb_debug.timing_enabled or thumb_debug.trace_enabled: + path = self._path_resolver(parsed.path_hash) if self._path_resolver else None timer = thumb_debug.ThumbTimer( key=cache_key, path=path, reason=parsed.reason ) ... cached_bytes = self._cache.get(cache_key) if cached_bytes: ... return image # or fall through to evict + # Resolve path only needed on miss (for prefetch submit) + path = self._path_resolver(parsed.path_hash) if self._path_resolver else None + if path: self._prefetcher.submit(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 217 - 219, The code currently calls self._path_resolver(parsed.path_hash) unconditionally and assigns it to path before checking the cache, causing an unnecessary resolve on cache hits; change the logic in the thumbnail provider so the cache lookup runs first (using parsed.path_hash and mtime_ns) and only if the cache misses and you need to prefetch, call self._path_resolver(parsed.path_hash) to produce path for the prefetch submit call; reference the existing symbols _path_resolver, parsed.path_hash, path, and the prefetch submit call so you only resolve the path on cache misses.
217-219: Path resolved unconditionally before cache lookup — wasted resolver call on every hit
_path_resolveris invoked before the cache check. On a cache hit with timing/tracing disabled,pathis never consumed. Moving the call to after the cache miss is confirmed should save the lookup on the hot path.♻️ Proposed restructuring
- # Resolve path - we already have path_hash and mtime_ns - path = self._path_resolver(parsed.path_hash) if self._path_resolver else None - if thumb_debug.timing_enabled or thumb_debug.trace_enabled: + path = self._path_resolver(parsed.path_hash) if self._path_resolver else None timer = thumb_debug.ThumbTimer( key=cache_key, path=path, reason=parsed.reason ) ... cached_bytes = self._cache.get(cache_key) if cached_bytes: ... return image # cache hit — path never needed + # Resolve path for prefetch submission (cache miss path only) + path = self._path_resolver(parsed.path_hash) if self._path_resolver else None + if path: self._prefetcher.submit(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 217 - 219, The code calls self._path_resolver(parsed.path_hash) unconditionally and assigns to path before checking the cache, causing unnecessary resolver calls on cache hits; change the logic in the thumbnail provider so that path is only resolved after confirming a cache miss (i.e., move the call to self._path_resolver into the cache-miss branch and only compute path when tracing/timing needs it or when the cache lookup fails), referencing the existing symbols _path_resolver, parsed.path_hash, and the variable path to locate where to adjust the flow.
83-85: Returning the sameQImageinstance from concurrentrequestImagecalls is unsafeImage providers that support QImage loading automatically include support for asynchronous loading of images. When async loading is enabled, the image request to the provider is run in a low priority thread. This means
requestImagecan be invoked concurrently from multiple non-GUI threads.Qt's
QImageis documented as not thread-safe. The rendering pipeline internally callsdetach()on the received image (to get a writable copy), which mutates the shared reference count. Performing that operation on the same instance from two threads simultaneously is a data race.Returning a shallow COW copy is O(1) and eliminates the hazard:
♻️ Proposed fix
- size.setWidth(self._error_placeholder.width()) - size.setHeight(self._error_placeholder.height()) - return self._error_placeholder + size.setWidth(self._error_placeholder.width()) + size.setHeight(self._error_placeholder.height()) + return QImage(self._error_placeholder)- size.setWidth(self._folder_placeholder.width()) - size.setHeight(self._folder_placeholder.height()) - return self._folder_placeholder + size.setWidth(self._folder_placeholder.width()) + size.setHeight(self._folder_placeholder.height()) + return QImage(self._folder_placeholder)- size.setWidth(self._placeholder.width()) - size.setHeight(self._placeholder.height()) - return self._placeholder + size.setWidth(self._placeholder.width()) + size.setHeight(self._placeholder.height()) + return QImage(self._placeholder)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 83 - 85, The placeholders (_placeholder, _folder_placeholder, _error_placeholder) are stored as single QImage instances but requestImage can be called concurrently; returning the same QImage causes data races when Qt calls detach(). Modify requestImage (the provider method that returns images) to return a shallow COW copy instead of the stored instance—e.g., construct a new QImage from the stored one (using QImage(self._placeholder) or equivalent) or otherwise perform a shallow copy before returning; ensure creation via _create_placeholder/_create_folder_placeholder stays the same but always return a copied QImage to callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/imaging/metadata.py`:
- Around line 140-150: The except block in get_exif_brief silently returns "" on
any Image.open()/EXIF read failure; update the exception handler in the
get_exif_brief function to log the failure (use the module logger, e.g.
log.debug) including the exception information and the image path (and
optionally the exception type/message) before returning "" so we retain graceful
degradation but preserve observability; reference the Image.open(path) / exif /
exif_ifd code area and ensure the logger call includes exc_info or the exception
string for debugging.
In `@faststack/thumbnail_view/provider.py`:
- Around line 83-85: The placeholders self._placeholder,
self._folder_placeholder, and self._error_placeholder are shared instances but
requestImage can be called from worker threads; change requestImage (in the
thumbnail image provider class) to return per-call QImage copies (e.g.
QImage(self._placeholder), QImage(self._folder_placeholder),
QImage(self._error_placeholder)) instead of returning the stored objects
directly so each thread gets its own reentrant QImage instance via copy-on-write
semantics.
---
Outside diff comments:
In `@faststack/app.py`:
- Around line 3494-3504: In _switch_to_directory, after clearing caches and
before returning, stop and clear the EXIF debounce timer so any previously
scheduled EXIF read for the old directory cannot fire and repopulate
_exif_brief_cache; specifically, check for the timer instance (e.g.
self._exif_brief_timer or self._exif_debounce_timer), call its cancel()/stop()
method if present, and set that attribute to None (also ensure
_exif_pending_path is cleared as shown) to fully prevent stale callbacks from
executing.
---
Nitpick comments:
In `@faststack/imaging/metadata.py`:
- Line 155: The annotation for the variable parts uses PEP 585-style list[str]
which is inconsistent with the module's typing imports and may break on Python
3.8; change the annotation to use the typing module (List[str]) and add List to
the existing typing imports (or, if the project targets Python 3.9+, migrate all
annotations to PEP 585 forms consistently across the file); update the
declaration of parts in metadata.py (the parts variable) accordingly so it
matches the rest of the file's typing style.
- Around line 223-227: Replace the hard-coded GPS IFD numeric literal with the
named ExifTags constant for consistency: when constructing gps_ifd (in the same
block that builds exif_ifd from exif_obj.get_ifd), use ExifTags.IFD.GPSInfo
instead of 0x8825 and keep the same hasattr(ExifTags, "IFD") guard and dict(...)
wrapping so behavior and compatibility with Pillow versions remain unchanged.
- Around line 367-369: Remove the redundant no-op "pass" in the except block
that logs GPS parse failures; specifically, inside the exception handler where
log.warning(f"Failed to parse GPS info: {e}") is called (in the GPS parsing
logic in faststack.imaging.metadata.py), delete the trailing "pass" so the
except block only contains the warning call.
- Around line 169-175: get_exif_brief currently converts the FNumber tag by
calling float(f_number) which loses aperture when the tag is a bare (numerator,
denominator) tuple; update get_exif_brief (the block reading tags.get(0x829D) /
variable f_number) to mirror get_exif_data's handling: if f_number is a
2‑element tuple/list, compute val = f_number[0] / f_number[1]; otherwise attempt
float(f_number) in a try/except and append f/{val:.1f}; on failure keep the
existing debug log path. This ensures tuple-format FNumber values are handled
consistently with get_exif_data.
In `@faststack/tests/test_metadata.py`:
- Around line 39-41: The stubbed parameters and patched-but-unreferenced
fixtures (e.g., MockExif.get_ifd(tag)) should be renamed to show they are
intentionally unused; update signatures and fixture argument names to use a
leading underscore (for example change get_ifd(self, tag) to get_ifd(self, _tag)
and rename any unused test fixture parameters like mock_exists to _mock_exists)
so Ruff ARG002 warnings are resolved while leaving implementations unchanged.
In `@faststack/thumbnail_view/prefetcher.py`:
- Around line 556-563: The properties size and bytes_used read shared state
without holding the class lock; update the size and bytes_used property getters
to acquire self._lock before accessing self._cache and self._current_bytes (and
release it afterwards) so they follow the same locking discipline as mutators;
specifically, wrap the reads of self._cache in size and of self._current_bytes
in bytes_used with the existing self._lock to avoid a data race.
- Around line 556-563: The properties size and bytes_used read shared state
(_cache and _current_bytes) without holding the instance lock, causing a race;
update the size and bytes_used property implementations to acquire the existing
_lock while reading those fields (e.g., use the lock as a context manager around
len(self._cache) and returning self._current_bytes) so reads are consistent with
the lock-guarded mutations performed elsewhere in the class.
- Around line 169-170: The submit method unconditionally creates a ThumbTimer on
cache misses; change submit so it only instantiates
thumb_debug.ThumbTimer(key=cache_key, path=path, reason="prefetch") when debug
timing is enabled (use thumb_debug.timing_enabled or thumb_debug.trace_enabled)
— keep timer as None otherwise so no allocation occurs; mirror the gating logic
used in ThumbnailProvider.requestImage and ensure any later logic that expects
timer handles a None timer safely.
- Around line 519-524: The comment in the put method is misleading: OrderedDict
only handles ordering, not byte-count bookkeeping; update the comment above the
if-block that checks "if key in self._cache" to clearly state that we must
subtract the old entry size from self._current_bytes before moving the key, e.g.
note that OrderedDict.move_to_end handles ordering but we explicitly adjust
_current_bytes when replacing an existing key (referencing _cache,
_current_bytes, move_to_end, and key to locate the block). Ensure the new
comment does not imply OrderedDict updates sizes automatically and succinctly
describes the exact steps taken (subtract old bytes, then move_to_end).
- Around line 169-170: The submit method creates a ThumbTimer unconditionally on
cache miss; change it to only instantiate the fallback timer when
instrumentation is enabled by guarding the creation with the same condition used
in ThumbnailProvider.requestImage (i.e. check thumb_debug.timing_enabled or
thumb_debug.trace_enabled) so that ThumbTimer(key=cache_key, path=path,
reason="prefetch") is only created when timer is None AND
(thumb_debug.timing_enabled or thumb_debug.trace_enabled); keep existing
variables (timer, cache_key, path) and behavior unchanged otherwise.
- Around line 519-524: The comment in put() is contradictory about OrderedDict
handling key existence; update it to clearly state that OrderedDict only manages
ordering but does not track byte sizes, so when a key exists in self._cache you
must subtract len(self._cache[key]) from self._current_bytes before replacing or
moving the entry; specifically revise the comment around the if key in
self._cache: block to remove the misleading "(OrderedDict handles key
existence)" text and explain that ordering is automatic but manual bookkeeping
of self._current_bytes is required for correct byte-counting when updating
_cache and calling _cache.move_to_end(key, last=True).
In `@faststack/thumbnail_view/provider.py`:
- Around line 217-219: The code currently calls
self._path_resolver(parsed.path_hash) unconditionally and assigns it to path
before checking the cache, causing an unnecessary resolve on cache hits; change
the logic in the thumbnail provider so the cache lookup runs first (using
parsed.path_hash and mtime_ns) and only if the cache misses and you need to
prefetch, call self._path_resolver(parsed.path_hash) to produce path for the
prefetch submit call; reference the existing symbols _path_resolver,
parsed.path_hash, path, and the prefetch submit call so you only resolve the
path on cache misses.
- Around line 217-219: The code calls self._path_resolver(parsed.path_hash)
unconditionally and assigns to path before checking the cache, causing
unnecessary resolver calls on cache hits; change the logic in the thumbnail
provider so that path is only resolved after confirming a cache miss (i.e., move
the call to self._path_resolver into the cache-miss branch and only compute path
when tracing/timing needs it or when the cache lookup fails), referencing the
existing symbols _path_resolver, parsed.path_hash, and the variable path to
locate where to adjust the flow.
- Around line 83-85: The placeholders (_placeholder, _folder_placeholder,
_error_placeholder) are stored as single QImage instances but requestImage can
be called concurrently; returning the same QImage causes data races when Qt
calls detach(). Modify requestImage (the provider method that returns images) to
return a shallow COW copy instead of the stored instance—e.g., construct a new
QImage from the stored one (using QImage(self._placeholder) or equivalent) or
otherwise perform a shallow copy before returning; ensure creation via
_create_placeholder/_create_folder_placeholder stays the same but always return
a copied QImage to callers.
Comment on lines
+140
to
+150
| try: | ||
| with Image.open(path) as img: | ||
| exif = img.getexif() | ||
| # getexif() nests EXIF sub-IFD tags; merge them for flat access | ||
| # Read them while file is open to avoid "I/O on closed file" | ||
| exif_ifd = dict(exif.get_ifd(ExifTags.IFD.Exif) if hasattr(ExifTags, "IFD") else {}) | ||
|
|
||
| if not exif: | ||
| return "" | ||
| except Exception: | ||
| return "" |
There was a problem hiding this comment.
Add a debug log when the broad exception fires in get_exif_brief.
A silent return "" on any Image.open / EXIF-read failure makes it impossible to distinguish "format not supported" (expected) from "Pillow decode error" or a code bug. A single log.debug call preserves the graceful degradation while retaining observability.
♻️ Proposed fix
- except Exception:
+ except Exception:
+ log.debug("get_exif_brief: could not read EXIF from %s", path, exc_info=True)
return ""🧰 Tools
🪛 Ruff (0.15.1)
[warning] 149-149: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/imaging/metadata.py` around lines 140 - 150, The except block in
get_exif_brief silently returns "" on any Image.open()/EXIF read failure; update
the exception handler in the get_exif_brief function to log the failure (use the
module logger, e.g. log.debug) including the exception information and the image
path (and optionally the exception type/message) before returning "" so we
retain graceful degradation but preserve observability; reference the
Image.open(path) / exif / exif_ifd code area and ensure the logger call includes
exc_info or the exception string for debugging.
Comment on lines
83
to
85
| self._placeholder = self._create_placeholder(default_size, PLACEHOLDER_COLOR) | ||
| self._folder_placeholder = self._create_folder_placeholder(default_size) | ||
| self._error_placeholder = self._create_placeholder(default_size, ERROR_COLOR) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Qt6 QImage thread safety documentation
💡 Result:
Qt 6 documents QImage as reentrant (not explicitly “thread-safe”):
- QImage: “All functions in this class are reentrant.” This means you can call QImage APIs from multiple threads as long as each thread uses its own QImage instance (its own data). [1]
- Implicit sharing across threads: Qt notes that implicitly shared classes (including QImage) use atomic reference counting, so you can copy/pass QImage between threads safely (e.g., via queued signal/slot). However, atomic ref-counting does not make sharing the same instance thread-safe—if multiple threads access the same QImage object and any thread may modify it, you must synchronize (mutex, etc.). [2]
- Meaning of “reentrant” in Qt docs: reentrant allows concurrent calls only when each invocation uses unique data; only “thread-safe” allows shared-instance use without extra locking. [3]
References
[1] Qt 6 QImage Class documentation. (doc.qt.io)
[2] Qt 6 “Thread-Support in Qt Modules” → “Threads and Implicitly Shared Classes”. (doc.qt.io)
[3] Qt 6 “Reentrancy and Thread-Safety”. (doc.qt.io)
Citations:
- 1: https://doc.qt.io/qt-6/qimage.html?utm_source=openai
- 2: https://doc.qt.io/qt-6.9/threads-modules.html?utm_source=openai
- 3: https://doc.qt.io/qt-6/threads-reentrancy.html?utm_source=openai
🌐 Web query:
QQuickImageProvider requestImage thread safety Qt6
💡 Result:
In Qt 6, QQuickImageProvider::requestImage() is not guaranteed to run on the GUI thread:
- If you enable async loading (either by passing
QQmlImageProviderBase::ForceAsynchronousImageLoadingwhen constructing the provider, or when QML loads the image asynchronously), Qt will run provider requests in a separate thread so the UI won’t block. ForQImageproviders this is supported; forQPixmapproviders it depends on platform support (“ThreadedPixmaps”), otherwise it may still run synchronously. [1][2] - For non-
ImageResponseproviders (requestImage/requestPixmap/requestTexture) Qt notes that async requests are executed on a single worker thread per engine, so one slow provider call can block other provider requests (even though it’s off the UI thread). [1]
Thread-safety implications
- Implement
requestImage()(and your derived destructor) as thread-safe (no access to GUI-only objects, protect shared state with locks, etc.). The docs explicitly call out that the derived provider’s destructor must be thread safe. [1]
If you need real concurrency / scalable loading
- Use
QQuickAsyncImageProvider+QQuickImageResponse. Qt explicitly warnsrequestImageResponse()may be called by multiple threads, so it must be reentrant, and you can fan out work viaQThreadPool/similar. [1][3]
Sources: Qt docs for QQuickImageProvider, QQmlImageProviderBase, and QQuickAsyncImageProvider. [1][2][3]
🏁 Script executed:
# Locate and inspect the provider.py file
find . -name "provider.py" -path "*/thumbnail_view/*" | head -20Repository: AlanRockefeller/faststack
Length of output: 107
🏁 Script executed:
# Read the provider.py file to verify the actual code
cat -n ./faststack/thumbnail_view/provider.py | head -300Repository: AlanRockefeller/faststack
Length of output: 12736
🏁 Script executed:
# Verify the exact lines mentioned in the review (201-208, 283-285)
sed -n '199,210p' ./faststack/thumbnail_view/provider.py
sed -n '280,286p' ./faststack/thumbnail_view/provider.pyRepository: AlanRockefeller/faststack
Length of output: 773
🌐 Web query:
Qt6 QImage shallow copy COW thread safety performance requestImage
💡 Result:
QImage shallow copy / COW (Qt 6)
QImageis implicitly shared (“copy-on-write”): assignment/copy is a shallow copy (pointer + atomic refcount inc), and a deep copy happens only when a non-const operation would modify the data while it’s shared. This makes pass-by-value typically cheap. [1][2]- Read-only pixel accessors like
constScanLine()/constBits()do not detach (no deep copy) because they returnconstdata. [3]
Thread-safety implications
- Qt’s implicitly shared classes (including
QImage) use atomic reference counting, so copying across threads is safe as a value type (reentrant). [4] - But atomic refcount ≠ “thread-safe object”: you still must not have multiple threads access the same
QImageinstance concurrently without locking (even if only one thread “might write”). The safe pattern is: publish/copy theQImageto the worker thread, then each thread works on its own instance; any later modification will detach as needed. [4][5]
Performance takeaways
- If you only produce an image and then hand it off read-only, implicit sharing is usually ideal (near-zero copy cost). [1][4]
- If multiple stages will mutate their local copies, you’ll pay the deep-copy cost at the first write per stage; that’s expected COW behavior. [1][4]
QQuickImageProvider::requestImage() and threads
- If you construct the provider with
QQmlImageProviderBase::ForceAsynchronousImageLoading, Qt will run image requests in a separate thread (sorequestImage()can do heavier work without blocking the UI). [6][7] - Qt also notes that for providers other than
ImageResponse, async execution is effectively single-threaded per engine, so one slow provider can serialize other loads; for higher throughput, Qt suggestsQQuickAsyncImageProviderand doing your own concurrency (e.g.,QThreadPool). [7]
Rule of thumb: return a QImage built entirely within requestImage() (thread-local), and avoid sharing/mutating the same QImage instance across threads; let implicit sharing handle the cheap handoff and detach-on-write when needed. [4][7]
Sources
[1] Qt Docs — Implicit Sharing (Qt 6.10) https://doc.qt.io/qt-6/implicit-sharing.html
[2] Qt Docs — Threads and Implicitly Shared Classes (Qt 6.9) https://doc.qt.io/qt-6.9/threads-modules.html
[3] Qt Docs — QImage::constScanLine (Qt 6.10) https://doc.qt.io/qt-6/qimage.html
[4] Qt Docs — Threads and Implicitly Shared Classes (atomic refcount + locking requirement) https://doc.qt.io/qt-6.9/threads-modules.html
[5] Qt Docs — Reentrancy and Thread-Safety (definitions) https://doc.qt.io/qt-6/threads-reentrancy.html
[6] Qt Docs — QQmlImageProviderBase::ForceAsynchronousImageLoading https://doc.qt.io/qt-6/qqmlimageproviderbase.html
[7] Qt Docs — QQuickImageProvider (async behavior, caching, single-thread-per-engine note) https://doc.qt.io/qt-6/qquickimageprovider.html
🌐 Web query:
Qt6 QQuickImageProvider requestImage concurrent calls multiple threads
💡 Result:
In Qt 6, QQuickImageProvider::requestImage() (and requestPixmap() / requestTexture()) may be called from multiple threads, so your implementation must be reentrant / thread-safe (no unsynchronized shared mutable state, protect caches/singletons with a mutex, avoid using thread-affine QObjects incorrectly, etc.).[1]
Also note the important nuance about “async” loading:
- If you set
QQmlImageProviderBase::ForceAsynchronousImageLoading, Qt will run requests off the GUI thread.[1][2] - But for providers that are not
ImageResponse(i.e., plainQImage/QPixmap/Textureproviders), Qt’s async execution is on a single worker thread perQQmlEngine, so requests won’t necessarily run in parallel; one slow request can block others for that engine.[1] - If you need true parallelism (multiple concurrent in-flight loads), use
QQuickAsyncImageProvider(ImageResponsetype) and do your own threading (e.g.,QThreadPool).[1][2]
Sources: Qt 6 QQuickImageProvider docs (threading + single-thread-per-engine async behavior + recommendation to use QQuickAsyncImageProvider), and QQmlImageProviderBase docs (ForceAsynchronousImageLoading).[1][2]
References
[1] (doc.qt.io)
[2] (doc.qt.io)
Citations:
- 1: https://doc.qt.io/qt-6/qquickimageprovider.html?utm_source=openai
- 2: https://doc.qt.io/qt-6/qqmlimageproviderbase.html?utm_source=openai
Return per-call copies of placeholder QImages — requestImage runs off the GUI thread
Qt documents QImage as reentrant, requiring each thread to use its own instance. QQuickImageProvider::requestImage may be called from Qt's image-loading worker thread(s) concurrently; returning the same pre-created self._placeholder, self._folder_placeholder, and self._error_placeholder instances violates this contract.
QImage(other) creates a shallow copy sharing the pixel buffer via copy-on-write — effectively O(1) and safe. Each caller receives its own instance, eliminating contention over refcount and metadata.
♻️ Proposed fix
- return self._error_placeholder
+ return QImage(self._error_placeholder)- return self._folder_placeholder
+ return QImage(self._folder_placeholder)- return self._placeholder
+ return QImage(self._placeholder)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/thumbnail_view/provider.py` around lines 83 - 85, The placeholders
self._placeholder, self._folder_placeholder, and self._error_placeholder are
shared instances but requestImage can be called from worker threads; change
requestImage (in the thumbnail image provider class) to return per-call QImage
copies (e.g. QImage(self._placeholder), QImage(self._folder_placeholder),
QImage(self._error_placeholder)) instead of returning the stored objects
directly so each thread gets its own reentrant QImage instance via copy-on-write
semantics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Quality