Add grid view mode with image previews#35
Conversation
WalkthroughThis pull request introduces a complete thumbnail grid view feature to faststack. New Python modules implement thumbnail caching, prefetching, and a Qt-based model. QML components provide grid UI and tile rendering. The AppController integrates grid navigation, view toggling, and thumbnail management. Configuration files are updated to exclude the .claude directory and remove TIFF binary marking. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QML as QML (Main)
participant UIState
participant Controller as AppController
participant Model as ThumbnailModel
participant Prefetcher
participant Cache as ThumbnailCache
participant Provider as ThumbnailProvider
User->>QML: Press "T" key
QML->>UIState: toggleGridView()
UIState->>Controller: toggle_grid_view()
Controller->>Controller: _set_grid_view_active(True)
Controller->>Model: refresh(filter)
Model->>Controller: _get_metadata_dict() for each image
Controller->>Model: Selection & path-to-index ready
Model-->>QML: dataChanged signal
UIState-->>QML: isGridViewActiveChanged(True)
QML->>QML: StackLayout shows grid (index 1)
QML->>Model: Bind GridView to thumbnailModel
User->>QML: Scroll grid
QML->>QML: ThumbnailGridView onContentYChanged
QML->>UIState: gridPrefetchRange(topIdx, bottomIdx)
UIState->>Controller: (internal prefetch trigger)
Controller->>Prefetcher: prefetch_batch(entries)
Prefetcher->>Cache: Check cache for (size, hash, mtime)
alt Cache miss
Prefetcher->>Prefetcher: _decode_worker() in thread pool
Prefetcher->>Prefetcher: _decode_image(), apply EXIF orientation
Prefetcher->>Cache: put(cache_key, jpeg_bytes)
Prefetcher->>Controller: _on_thumbnail_ready(thumbnail_id)
Controller->>Controller: _on_thumbnail_ready_gui() [GUI thread]
Controller->>Model: _on_thumbnail_ready(thumbnail_id)
Model->>Model: Increment thumb_rev, emit dataChanged
Model-->>QML: dataChanged(ThumbnailSourceRole)
else Cache hit
Prefetcher->>Provider: (thumbnail already available)
end
QML->>Provider: requestPixmap(image://thumbnail/...)
Provider->>Cache: get(cache_key)
Provider-->>QML: QPixmap from cached JPEG
QML->>QML: Display thumbnail image
sequenceDiagram
participant User
participant Grid as ThumbnailGridView
participant UIState
participant Controller as AppController
participant Model as ThumbnailModel
participant LoupeView as Loupe View
User->>Grid: Click folder entry or parent (..)
Grid->>Grid: MouseArea.onClicked()
alt Click is folder
Grid->>UIState: gridNavigateTo(folder_path)
UIState->>Controller: grid_navigate_to(path)
Controller->>Controller: _switch_to_directory(folder_path, update_base_directory=False)
Controller->>Model: refresh() for new directory
Model-->>Grid: dataChanged, row count updated
Grid->>Grid: Reset contentY, refocus, trigger prefetch
else Click is image with Ctrl/Shift
Grid->>Model: select_index(idx, shift, ctrl)
Model->>Model: Update selection set
Model-->>Grid: dataChanged(IsSelectedRole)
Grid->>Grid: Update visual selection highlight
else Click is image (plain)
Grid->>UIState: gridOpenIndex(index)
UIState->>Controller: grid_open_index(index)
Controller->>Controller: _get_current_loupe_index() resolve to loupe view
Controller->>LoupeView: Load image at resolved index
UIState-->>Grid: isGridViewActiveChanged(False)
Grid-->>LoupeView: (StackLayout shows loupe)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 12
🤖 Fix all issues with AI agents
In `@faststack/app.py`:
- Around line 1084-1095: The _get_metadata_dict method currently swallows all
Exceptions from sidecar.get_metadata; change it to catch only expected
exceptions (e.g., AttributeError, KeyError, IOError or a specific SidecarError
if one exists) and log the error once before returning the default dict. Locate
_get_metadata_dict and replace the broad except Exception with targeted except
clauses (or except (AttributeError, KeyError, IOError) as e) and call the
module/class logger (or self.logger) to record a concise message including the
stem and exception details, then return the default {"stacked": False,
"uploaded": False, "edited": False, "restacked": False}.
- Around line 1032-1066: Replace the direct assignment self.current_index =
loupe_index in grid_open_index with a call to
self._set_current_index(loupe_index) so edit-mode reset, crop reset, histogram
refresh and navigation-specific prefetch behavior occur; keep the subsequent
view switch call self._set_grid_view_active(False) and UI sync, but remove or
avoid duplicating prefetcher.update_prefetch if _set_current_index already
triggers prefetching to prevent double updates.
In `@faststack/qml/Main.qml`:
- Around line 920-944: The Button click handlers call uiState methods directly
and lack null checks; update each handler to guard against a null/undefined
uiState before invoking methods: for the Clear button check uiState then call
uiState.gridClearSelection(), for Refresh check uiState then call
uiState.gridRefresh(), and for Single Image check uiState then call
uiState.toggleGridView(); locate these handlers by the Button blocks referencing
uiState, gridClearSelection, gridRefresh, and toggleGridView and wrap the calls
in simple if (uiState) { ... } guards.
In `@faststack/qml/ThumbnailGridView.qml`:
- Around line 11-16: The code references an undefined root object (used as
root.isDarkTheme) but the component id is gridViewRoot; update any
root.isDarkTheme usages to gridViewRoot.isDarkTheme, and add a new property bool
isDarkTheme (default false) on the component (e.g., property bool isDarkTheme:
false) so parents can bind into it; ensure the parent binds to
gridViewRoot.isDarkTheme when instantiating this ThumbnailGridView.qml to avoid
QML ReferenceErrors at runtime.
In `@faststack/qml/ThumbnailTile.qml`:
- Around line 26-33: Add a local property to ThumbnailTile.qml (e.g., property
bool isDarkTheme: false) and replace all uses of root.isDarkTheme in this
component (such as in textColor, hoverColor, backgroundColor and any other
references) with tile.isDarkTheme (or the component id you use for
ThumbnailTile), then update the parent component to bind the value (e.g., in the
parent set isDarkTheme: root.isDarkTheme) so the component no longer depends on
the parent's id.
In `@faststack/tests/thumbnail_view/test_folder_stats.py`:
- Around line 141-145: The test uses time.sleep(0.01) to try to change file
mtime which is flaky on coarse filesystems; instead, after updating data and
calling json_path.write_text(json.dumps(data)) explicitly set the file mtime
with os.utime(json_path, (new_atime, new_mtime)) (e.g., use time.time() + 1 or a
deterministic timestamp) to ensure the mtime changes deterministically; add an
import for os if missing and remove the time.sleep call, referencing json_path
and the json_path.write_text(...) call in the test.
In `@faststack/tests/thumbnail_view/test_model.py`:
- Line 162: The test function test_no_parent_folder_at_base (and the other test
at the second occurrence around line 259) has an unused fixture parameter
temp_folder which triggers Ruff ARG002; fix by either removing temp_folder from
the function signature if it isn't needed, or mark it intentionally unused by
renaming it to _temp_folder (or prefixing with an underscore) so the linter
ignores it; update both occurrences (test_no_parent_folder_at_base and the other
test signature) accordingly.
In `@faststack/tests/thumbnail_view/test_prefetcher.py`:
- Line 170: The test function test_submit_deduplicates_inflight currently
declares an unused fixture parameter cache; remove the unused parameter (or
rename it to _cache if the fixture's setup side-effects are required) from the
function signature to satisfy the linter and eliminate Ruff ARG002, locating the
change in the test_submit_deduplicates_inflight definition.
In `@faststack/tests/thumbnail_view/test_selection.py`:
- Around line 151-163: The test test_shift_click_without_anchor uses a weak
assertion; update it to assert the precise expected behavior by checking that
only one item is selected when no anchor exists: after clearing selection and
setting model_with_images._last_selected_index = None, call
model_with_images.select_index(2, shift=True, ctrl=False) then replace assert
len(selected) >= 1 with an exact assertion assert len(selected) == 1 (using
get_selected_paths to obtain selected) to reflect the intended single-item
selection behavior.
In `@faststack/thumbnail_view/__init__.py`:
- Around line 8-17: The __all__ export list in
faststack/thumbnail_view/__init__.py is not alphabetically sorted (Ruff RUF022);
reorder the entries in the __all__ list alphabetically (e.g., FolderStats,
PathResolver, read_folder_stats, ThumbnailCache, ThumbnailEntry, ThumbnailModel,
ThumbnailPrefetcher, ThumbnailProvider) so the list is isort-style sorted and
Ruff will be satisfied.
In `@faststack/thumbnail_view/folder_stats.py`:
- Around line 78-83: The code assumes `data` is a dict before calling
`data.get("entries", {})`, which can raise if the JSON root is not a dict; add a
defensive check like `if not isinstance(data, dict): log.debug("Invalid JSON
root in %s", json_path); return None` before accessing `data.get`, then proceed
with the existing `entries = data.get("entries", {})` and the subsequent `if not
isinstance(entries, dict): ...` check to ensure tolerant parsing; reference the
`data`, `entries`, and `json_path` variables when locating the change.
In `@faststack/thumbnail_view/provider.py`:
- Around line 209-219: The update_from_model method uses a forward reference
"ThumbnailModel" that isn't imported at runtime; add a TYPE_CHECKING import
block at module top (from typing import TYPE_CHECKING; if TYPE_CHECKING: from
.models import ThumbnailModel) so the type hint resolves for static checking
without runtime import, and keep the existing MD5 usage in
update_from_model/_hash_to_path but add a brief comment next to the hashlib.md5
usage indicating it's non-cryptographic (cache key) to silence static-analysis
false positives (or add the appropriate linter noqa flag).
🧹 Nitpick comments (11)
faststack/thumbnail_view/folder_stats.py (1)
90-92: Rename unused loop variable for clarity/lint.The loop variable
stemisn’t used (Line 90). Renaming it to_stemmakes intent explicit and satisfies linting.♻️ Proposed tweak
- for stem, meta in entries.items(): + for _stem, meta in entries.items():faststack/thumbnail_view/prefetcher.py (2)
44-49: Align cache type annotation with the actual cache class.
cache: "ByteLRUCache"(Line 46) doesn’t exist in this module. This confuses typing/static analysis; consider annotating with"ThumbnailCache"(or a small protocol if multiple cache types are intended).♻️ Proposed fix
- cache: "ByteLRUCache", + cache: "ThumbnailCache",
127-136:marginparameter is unused; align API and behavior.
marginis documented but never applied (Line 127). Either implement the “prefetch beyond visible range” logic or remove the parameter/docstring to avoid misleading callers.♻️ Possible simplification (if margin isn’t needed)
- def prefetch_batch(self, entries: list, margin: int = 2): + def prefetch_batch(self, entries: list): """Prefetch thumbnails for a batch of entries. Args: entries: List of ThumbnailEntry objects - margin: Extra entries to prefetch beyond visible range """faststack/tests/thumbnail_view/test_prefetcher.py (1)
145-156: Replace fixed sleep with a deterministic wait.Line 150-152 uses a fixed
sleep(0.5), which is prone to flakiness on slower CI. Prefer waiting until the cache entry appears (with a timeout). Apply the same pattern to the othersleepcalls below to keep tests stable.♻️ Proposed fix (example for this test)
result = prefetcher.submit(test_image, mtime_ns) assert result is True - # Wait for job to complete - time.sleep(0.5) - - # Check cache was populated - path_hash = _compute_path_hash(test_image) - cache_key = f"200/{path_hash}/{mtime_ns}" + # Wait for job to complete (avoid fixed sleeps) + path_hash = _compute_path_hash(test_image) + cache_key = f"200/{path_hash}/{mtime_ns}" + deadline = time.monotonic() + 2 + while time.monotonic() < deadline and cache.get(cache_key) is None: + time.sleep(0.01) assert cache.get(cache_key) is not Nonefaststack/qml/ThumbnailGridView.qml (1)
141-147: Avoid magic role numbers inmodel.datacalls.Line 144-147 hard-code roles (257/259/269). This is brittle if role values change. Prefer exposing role constants (e.g.,
thumbnailModel.FilePathRole) or helper methods/slots on the model and use those from QML.faststack/tests/thumbnail_view/test_selection.py (1)
287-303: Consider a more robust test for file-only filtering.The test
test_returns_only_filescreates only a file entry without any folder entries. Consider adding a folder entry alongside the file to actually verify the filtering behavior:model._entries = [ + ThumbnailEntry(path=temp_folder / "subfolder", name="subfolder", is_folder=True), ThumbnailEntry(path=temp_folder / "image.jpg", name="image.jpg", is_folder=False), ] - model.select_index(0, shift=False, ctrl=False) + model.select_index(1, shift=False, ctrl=False) # Select the file, not folder selected = model.get_selected_paths() assert len(selected) == 1faststack/ui/provider.py (1)
1213-1241: Consider batching prefetch submissions.The
gridPrefetchRangemethod iterates and submits individual prefetch jobs in a loop. TheThumbnailPrefetcheralready has aprefetch_batchmethod (per the relevant code snippets) that could be used instead:`@Slot`(int, int) def gridPrefetchRange(self, startIndex: int, endIndex: int): """Prefetch thumbnails for the given index range.""" if not hasattr(self.app_controller, '_thumbnail_model') or not self.app_controller._thumbnail_model: return if not hasattr(self.app_controller, '_thumbnail_prefetcher') or not self.app_controller._thumbnail_prefetcher: return model = self.app_controller._thumbnail_model prefetcher = self.app_controller._thumbnail_prefetcher # Clamp indices startIndex = max(0, startIndex) endIndex = min(model.rowCount() - 1, endIndex) - # Submit prefetch jobs for visible range - for i in range(startIndex, endIndex + 1): - entry = model.get_entry(i) - if entry and not entry.is_folder: - prefetcher.submit(entry.path, entry.mtime_ns) + # Collect entries for batch prefetch + entries = [model.get_entry(i) for i in range(startIndex, endIndex + 1)] + entries = [e for e in entries if e is not None] + prefetcher.prefetch_batch(entries)This would leverage the existing batch functionality and potentially reduce overhead.
faststack/thumbnail_view/provider.py (1)
36-55: Update type hint to use explicit Optional.The
path_resolverparameter uses implicitOptionalwhich is discouraged per PEP 484. Also, useCallablefrom typing for better specificity.Suggested fix
+from typing import TYPE_CHECKING, Optional, Callable + +if TYPE_CHECKING: + from faststack.thumbnail_view.prefetcher import ThumbnailPrefetcher, ThumbnailCache + from faststack.thumbnail_view.model import ThumbnailModel class ThumbnailProvider(QQuickImageProvider): ... def __init__( self, cache: "ThumbnailCache", prefetcher: "ThumbnailPrefetcher", - path_resolver: callable = None, + path_resolver: Optional[Callable[[str], Optional[Path]]] = None, default_size: int = 200, ):faststack/thumbnail_view/model.py (3)
297-305: Log exceptions instead of silently passing.The try-except block silently swallows all exceptions when fetching metadata. This can hide issues that are difficult to debug later.
Suggested fix
if self._get_metadata: try: meta = self._get_metadata(img.path.stem) is_stacked = meta.get("stacked", False) is_uploaded = meta.get("uploaded", False) is_edited = meta.get("edited", False) is_restacked = meta.get("restacked", False) - except Exception: - pass + except Exception as e: + log.debug("Failed to get metadata for %s: %s", img.path.stem, e)
179-186: Tight coupling with parent controller.
_get_loupe_index_for_entryaccesses the parent's_path_to_indexattribute directly, creating tight coupling between the model andAppController. Consider passing this as a callback during construction (similar toget_metadata_callback) if you want to make the model more testable and reusable.However, given the current architecture where the model is always owned by the controller, this is acceptable for now.
486-495: Consider caching path resolution for find_image_index.The
find_image_indexmethod performs a linear search withpath.resolve()called on each entry. For large directories, this could be slow. If this method is called frequently (e.g., for highlighting the current image), consider maintaining a reverse lookup dict similar to_id_to_row.However, if this is only used occasionally, the current implementation is fine.
| def grid_open_index(self, index: int): | ||
| """Open an image from grid view in loupe view.""" | ||
| entry = self._thumbnail_model.get_entry(index) | ||
| if not entry: | ||
| log.warning("grid_open_index: no entry at index %d", index) | ||
| return | ||
|
|
||
| if entry.is_folder: | ||
| # Navigate into folder instead of opening | ||
| self.grid_navigate_to(str(entry.path)) | ||
| return | ||
|
|
||
| # Find this image in the main image list using O(1) lookup | ||
| resolved_path = entry.path.resolve() | ||
| loupe_index = self._path_to_index.get(resolved_path) | ||
|
|
||
| if loupe_index is None: | ||
| # Index might be stale - rebuild and retry once | ||
| self._rebuild_path_to_index() | ||
| loupe_index = self._path_to_index.get(resolved_path) | ||
|
|
||
| if loupe_index is None: | ||
| log.warning("grid_open_index: image not found in current list: %s", entry.path) | ||
| # Image might be in a different directory - don't switch view | ||
| return | ||
|
|
||
| self.current_index = loupe_index | ||
|
|
||
| # Switch to loupe view | ||
| self._set_grid_view_active(False) | ||
|
|
||
| # Sync UI and trigger image load | ||
| self.sync_ui_state() | ||
| self.prefetcher.update_prefetch(self.current_index) | ||
| log.info("Opened image from grid: %s", entry.path) |
There was a problem hiding this comment.
Use _set_current_index when opening from grid to keep state consistent.
Directly assigning current_index skips edit-mode reset, crop reset, histogram refresh, and navigation-specific prefetch behavior. Centralizing through _set_current_index prevents subtle state bugs when opening from the grid.
🔧 Proposed fix
- self.current_index = loupe_index
-
- # Switch to loupe view
- self._set_grid_view_active(False)
-
- # Sync UI and trigger image load
- self.sync_ui_state()
- self.prefetcher.update_prefetch(self.current_index)
+ # Switch to loupe view
+ self._set_grid_view_active(False)
+ # Use centralized index change logic to reset state consistently
+ self._set_current_index(loupe_index, direction=0, is_navigation=False)🤖 Prompt for AI Agents
In `@faststack/app.py` around lines 1032 - 1066, Replace the direct assignment
self.current_index = loupe_index in grid_open_index with a call to
self._set_current_index(loupe_index) so edit-mode reset, crop reset, histogram
refresh and navigation-specific prefetch behavior occur; keep the subsequent
view switch call self._set_grid_view_active(False) and UI sync, but remove or
avoid duplicating prefetcher.update_prefetch if _set_current_index already
triggers prefetching to prevent double updates.
| def _get_metadata_dict(self, stem: str) -> dict: | ||
| """Get metadata for a file stem as a dict for thumbnail model.""" | ||
| try: | ||
| meta = self.sidecar.get_metadata(stem) | ||
| return { | ||
| "stacked": getattr(meta, "stacked", False), | ||
| "uploaded": getattr(meta, "uploaded", False), | ||
| "edited": getattr(meta, "edited", False), | ||
| "restacked": getattr(meta, "restacked", False), | ||
| } | ||
| except Exception: | ||
| return {"stacked": False, "uploaded": False, "edited": False, "restacked": False} |
There was a problem hiding this comment.
Avoid blanket exception swallowing in metadata lookup.
Catching Exception hides real metadata errors and makes debugging harder. Narrow the exception set and log once.
🛠️ Suggested adjustment
- except Exception:
- return {"stacked": False, "uploaded": False, "edited": False, "restacked": False}
+ except (KeyError, OSError, ValueError) as exc:
+ log.warning("Failed to read metadata for %s: %s", stem, exc)
+ return {"stacked": False, "uploaded": False, "edited": False, "restacked": False}🧰 Tools
🪛 Ruff (0.14.14)
1094-1094: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@faststack/app.py` around lines 1084 - 1095, The _get_metadata_dict method
currently swallows all Exceptions from sidecar.get_metadata; change it to catch
only expected exceptions (e.g., AttributeError, KeyError, IOError or a specific
SidecarError if one exists) and log the error once before returning the default
dict. Locate _get_metadata_dict and replace the broad except Exception with
targeted except clauses (or except (AttributeError, KeyError, IOError) as e) and
call the module/class logger (or self.logger) to record a concise message
including the stem and exception details, then return the default {"stacked":
False, "uploaded": False, "edited": False, "restacked": False}.
| // Clear selection button | ||
| Button { | ||
| text: "Clear" | ||
| visible: uiState ? uiState.gridSelectedCount > 0 : false | ||
| onClicked: uiState.gridClearSelection() | ||
| implicitWidth: 60 | ||
| implicitHeight: 28 | ||
| } | ||
|
|
||
| // Refresh button | ||
| Button { | ||
| text: "Refresh" | ||
| onClicked: uiState.gridRefresh() | ||
| implicitWidth: 70 | ||
| implicitHeight: 28 | ||
| } | ||
|
|
||
| // Single Image View button | ||
| Button { | ||
| text: "Single Image" | ||
| onClicked: uiState.toggleGridView() | ||
| implicitWidth: 90 | ||
| implicitHeight: 28 | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing null checks for button click handlers.
The Clear, Refresh, and Single Image buttons directly call uiState methods without null checks, unlike other places in the file that use if (uiState) guards.
Suggested fix
// Clear selection button
Button {
text: "Clear"
visible: uiState ? uiState.gridSelectedCount > 0 : false
- onClicked: uiState.gridClearSelection()
+ onClicked: if (uiState) uiState.gridClearSelection()
implicitWidth: 60
implicitHeight: 28
}
// Refresh button
Button {
text: "Refresh"
- onClicked: uiState.gridRefresh()
+ onClicked: if (uiState) uiState.gridRefresh()
implicitWidth: 70
implicitHeight: 28
}
// Single Image View button
Button {
text: "Single Image"
- onClicked: uiState.toggleGridView()
+ onClicked: if (uiState) uiState.toggleGridView()
implicitWidth: 90
implicitHeight: 28
}📝 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.
| // Clear selection button | |
| Button { | |
| text: "Clear" | |
| visible: uiState ? uiState.gridSelectedCount > 0 : false | |
| onClicked: uiState.gridClearSelection() | |
| implicitWidth: 60 | |
| implicitHeight: 28 | |
| } | |
| // Refresh button | |
| Button { | |
| text: "Refresh" | |
| onClicked: uiState.gridRefresh() | |
| implicitWidth: 70 | |
| implicitHeight: 28 | |
| } | |
| // Single Image View button | |
| Button { | |
| text: "Single Image" | |
| onClicked: uiState.toggleGridView() | |
| implicitWidth: 90 | |
| implicitHeight: 28 | |
| } | |
| } | |
| // Clear selection button | |
| Button { | |
| text: "Clear" | |
| visible: uiState ? uiState.gridSelectedCount > 0 : false | |
| onClicked: if (uiState) uiState.gridClearSelection() | |
| implicitWidth: 60 | |
| implicitHeight: 28 | |
| } | |
| // Refresh button | |
| Button { | |
| text: "Refresh" | |
| onClicked: if (uiState) uiState.gridRefresh() | |
| implicitWidth: 70 | |
| implicitHeight: 28 | |
| } | |
| // Single Image View button | |
| Button { | |
| text: "Single Image" | |
| onClicked: if (uiState) uiState.toggleGridView() | |
| implicitWidth: 90 | |
| implicitHeight: 28 | |
| } |
🤖 Prompt for AI Agents
In `@faststack/qml/Main.qml` around lines 920 - 944, The Button click handlers
call uiState methods directly and lack null checks; update each handler to guard
against a null/undefined uiState before invoking methods: for the Clear button
check uiState then call uiState.gridClearSelection(), for Refresh check uiState
then call uiState.gridRefresh(), and for Single Image check uiState then call
uiState.toggleGridView(); locate these handlers by the Button blocks referencing
uiState, gridClearSelection, gridRefresh, and toggleGridView and wrap the calls
in simple if (uiState) { ... } guards.
| property int cellWidth: 190 | ||
| property int cellHeight: 210 | ||
|
|
||
| // Selection info (for keyboard handler and external access) | ||
| property var selectedPaths: uiState ? uiState.gridGetSelectedPaths() : [] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n faststack/qml/ThumbnailGridView.qmlRepository: AlanRockefeller/faststack
Length of output: 7210
root is undefined—should be gridViewRoot.
Line 124 references root.isDarkTheme, but this component is identified as gridViewRoot (line 7). Change the reference to gridViewRoot.isDarkTheme. Also add an isDarkTheme property to the component and bind it from the parent to avoid QML ReferenceErrors at runtime.
Proposed fix
// Configuration
property int cellWidth: 190
property int cellHeight: 210
+ property bool isDarkTheme: false
// Empty state
Text {
anchors.centerIn: parent
visible: thumbnailGrid.count === 0
text: "No images in this folder"
- color: root.isDarkTheme ? "#888888" : "#666666"
+ color: gridViewRoot.isDarkTheme ? "#888888" : "#666666"
font.pixelSize: 16
}🤖 Prompt for AI Agents
In `@faststack/qml/ThumbnailGridView.qml` around lines 11 - 16, The code
references an undefined root object (used as root.isDarkTheme) but the component
id is gridViewRoot; update any root.isDarkTheme usages to
gridViewRoot.isDarkTheme, and add a new property bool isDarkTheme (default
false) on the component (e.g., property bool isDarkTheme: false) so parents can
bind into it; ensure the parent binds to gridViewRoot.isDarkTheme when
instantiating this ThumbnailGridView.qml to avoid QML ReferenceErrors at
runtime.
| property int tileSize: 180 | ||
| property int thumbnailSize: 160 | ||
| property int textHeight: 24 | ||
| property color textColor: root.isDarkTheme ? "white" : "black" | ||
| property color selectedColor: "#4CAF50" | ||
| property color currentColor: "#FFD700" // Gold for current image | ||
| property color hoverColor: root.isDarkTheme ? "#404040" : "#e0e0e0" | ||
| property color backgroundColor: root.isDarkTheme ? "#2d2d2d" : "#fafafa" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the ThumbnailTile.qml file and check its full content
find . -name "ThumbnailTile.qml" -type fRepository: AlanRockefeller/faststack
Length of output: 102
🏁 Script executed:
# Search for root.isDarkTheme and context property definitions
rg -n "root\.isDarkTheme|setContextProperty|contextProperty" -g "*.qml" -g "*.py"Repository: AlanRockefeller/faststack
Length of output: 4584
🏁 Script executed:
# Check for isDarkTheme property definition
rg -n "property.*isDarkTheme" -g "*.qml" -g "*.py"Repository: AlanRockefeller/faststack
Length of output: 961
🏁 Script executed:
# Read ThumbnailTile.qml to check the root element and its id
head -50 faststack/qml/ThumbnailTile.qmlRepository: AlanRockefeller/faststack
Length of output: 1924
🏁 Script executed:
# Check where ThumbnailTile is instantiated in Main.qml
rg -n "ThumbnailTile" -A 3 -B 1 faststack/qml/Main.qmlRepository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Check the opening of Main.qml to verify its root id
head -30 faststack/qml/Main.qmlRepository: AlanRockefeller/faststack
Length of output: 1019
🏁 Script executed:
# Search for ThumbnailTile usage with broader pattern
rg -n "ThumbnailTile" faststack/qml/*.qmlRepository: AlanRockefeller/faststack
Length of output: 141
🏁 Script executed:
# Get lines around 100 in ThumbnailTile.qml to see context
sed -n '95,105p' faststack/qml/ThumbnailTile.qmlRepository: AlanRockefeller/faststack
Length of output: 507
🏁 Script executed:
# Check if there's a property that provides root to ThumbnailTile
rg -n "required property|property.*root" faststack/qml/ThumbnailTile.qmlRepository: AlanRockefeller/faststack
Length of output: 293
🏁 Script executed:
# Check ThumbnailGridView.qml to see how it uses ThumbnailTile
head -50 faststack/qml/ThumbnailGridView.qmlRepository: AlanRockefeller/faststack
Length of output: 1559
🏁 Script executed:
# Check if ThumbnailGridView is used in Main.qml
rg -n "ThumbnailGridView" faststack/qml/Main.qml | head -5Repository: AlanRockefeller/faststack
Length of output: 120
🏁 Script executed:
# Check the complete property definitions and context in ThumbnailTile.qml
sed -n '20,35p' faststack/qml/ThumbnailTile.qmlRepository: AlanRockefeller/faststack
Length of output: 707
🏁 Script executed:
# Check the context around line 740 in Main.qml where ThumbnailGridView is loaded
sed -n '735,745p' faststack/qml/Main.qmlRepository: AlanRockefeller/faststack
Length of output: 472
🏁 Script executed:
# Check parent hierarchy - is ThumbnailGridView directly in Main.qml or wrapped?
sed -n '730,750p' faststack/qml/Main.qmlRepository: AlanRockefeller/faststack
Length of output: 689
🏁 Script executed:
# Let's look at more lines of ThumbnailTile.qml to see if isDarkTheme could be a required property
head -100 faststack/qml/ThumbnailTile.qml | tail -60Repository: AlanRockefeller/faststack
Length of output: 2209
🏁 Script executed:
# Check if ThumbnailTile has any required properties or parent bindings
rg -n "required property" faststack/qml/ThumbnailTile.qmlRepository: AlanRockefeller/faststack
Length of output: 51
root is not defined in ThumbnailTile.qml and relies on accessing the parent component's scope.
Lines 29, 32, 33, and 100 reference root.isDarkTheme, which implicitly accesses Main.qml's id: root. This scope-dependent pattern is fragile and breaks if the component hierarchy changes. Add a local isDarkTheme property to ThumbnailTile.qml and update references to use tile.isDarkTheme instead.
Proposed fix
// Configuration
property int tileSize: 180
property int thumbnailSize: 160
property int textHeight: 24
+ property bool isDarkTheme: false
- property color textColor: root.isDarkTheme ? "white" : "black"
+ property color textColor: tile.isDarkTheme ? "white" : "black"
property color selectedColor: "#4CAF50"
property color currentColor: "#FFD700" // Gold for current image
- property color hoverColor: root.isDarkTheme ? "#404040" : "#e0e0e0"
- property color backgroundColor: root.isDarkTheme ? "#2d2d2d" : "#fafafa"
+ property color hoverColor: tile.isDarkTheme ? "#404040" : "#e0e0e0"
+ property color backgroundColor: tile.isDarkTheme ? "#2d2d2d" : "#fafafa"
@@
- color: root.isDarkTheme ? "#3c3c3c" : "#e0e0e0"
+ color: tile.isDarkTheme ? "#3c3c3c" : "#e0e0e0"Parent component binds: isDarkTheme: root.isDarkTheme
🤖 Prompt for AI Agents
In `@faststack/qml/ThumbnailTile.qml` around lines 26 - 33, Add a local property
to ThumbnailTile.qml (e.g., property bool isDarkTheme: false) and replace all
uses of root.isDarkTheme in this component (such as in textColor, hoverColor,
backgroundColor and any other references) with tile.isDarkTheme (or the
component id you use for ThumbnailTile), then update the parent component to
bind the value (e.g., in the parent set isDarkTheme: root.isDarkTheme) so the
component no longer depends on the parent's id.
| result = prefetcher.submit(test_image, mtime_ns) | ||
| assert result is False | ||
|
|
||
| def test_submit_deduplicates_inflight(self, prefetcher, test_image, cache): |
There was a problem hiding this comment.
Remove unused cache fixture parameter.
Line 170 defines cache but never uses it (Ruff ARG002). If no side effects are needed, remove it or rename to _cache.
🧹 Suggested tweak
-def test_submit_deduplicates_inflight(self, prefetcher, test_image, cache):
+def test_submit_deduplicates_inflight(self, prefetcher, test_image, _cache):📝 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.
| def test_submit_deduplicates_inflight(self, prefetcher, test_image, cache): | |
| def test_submit_deduplicates_inflight(self, prefetcher, test_image, _cache): |
🧰 Tools
🪛 Ruff (0.14.14)
170-170: Unused method argument: cache
(ARG002)
🤖 Prompt for AI Agents
In `@faststack/tests/thumbnail_view/test_prefetcher.py` at line 170, The test
function test_submit_deduplicates_inflight currently declares an unused fixture
parameter cache; remove the unused parameter (or rename it to _cache if the
fixture's setup side-effects are required) from the function signature to
satisfy the linter and eliminate Ruff ARG002, locating the change in the
test_submit_deduplicates_inflight definition.
| def test_shift_click_without_anchor(self, model_with_images): | ||
| """Test Shift+click when no previous selection.""" | ||
| # Clear any existing selection | ||
| model_with_images.clear_selection() | ||
| model_with_images._last_selected_index = None | ||
|
|
||
| # Shift+click without anchor should just select the item | ||
| model_with_images.select_index(2, shift=True, ctrl=False) | ||
|
|
||
| # Should select from 0 to 2 or just the item depending on implementation | ||
| # In our implementation, if no anchor, it just selects the single item | ||
| selected = model_with_images.get_selected_paths() | ||
| assert len(selected) >= 1 |
There was a problem hiding this comment.
Weak assertion in test_shift_click_without_anchor.
The assertion assert len(selected) >= 1 is imprecise. Based on the comment on line 161, the implementation should select only the single item when there's no anchor. Consider making this assertion more specific:
- # Should select from 0 to 2 or just the item depending on implementation
- # In our implementation, if no anchor, it just selects the single item
- selected = model_with_images.get_selected_paths()
- assert len(selected) >= 1
+ # In our implementation, if no anchor, Shift+click does nothing (returns early)
+ selected = model_with_images.get_selected_paths()
+ assert len(selected) == 0 # No selection when anchor is NoneAlternatively, if the intent is that a single item should be selected, update the assertion to == 1.
🤖 Prompt for AI Agents
In `@faststack/tests/thumbnail_view/test_selection.py` around lines 151 - 163, The
test test_shift_click_without_anchor uses a weak assertion; update it to assert
the precise expected behavior by checking that only one item is selected when no
anchor exists: after clearing selection and setting
model_with_images._last_selected_index = None, call
model_with_images.select_index(2, shift=True, ctrl=False) then replace assert
len(selected) >= 1 with an exact assertion assert len(selected) == 1 (using
get_selected_paths to obtain selected) to reflect the intended single-item
selection behavior.
| __all__ = [ | ||
| "FolderStats", | ||
| "read_folder_stats", | ||
| "ThumbnailModel", | ||
| "ThumbnailEntry", | ||
| "ThumbnailPrefetcher", | ||
| "ThumbnailCache", | ||
| "ThumbnailProvider", | ||
| "PathResolver", | ||
| ] |
There was a problem hiding this comment.
Sort __all__ to satisfy Ruff RUF022.
Current ordering is not isort-style sorted.
🧹 Suggested ordering
__all__ = [
"FolderStats",
- "read_folder_stats",
- "ThumbnailModel",
- "ThumbnailEntry",
- "ThumbnailPrefetcher",
- "ThumbnailCache",
- "ThumbnailProvider",
"PathResolver",
+ "ThumbnailCache",
+ "ThumbnailEntry",
+ "ThumbnailModel",
+ "ThumbnailPrefetcher",
+ "ThumbnailProvider",
+ "read_folder_stats",
]📝 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.
| __all__ = [ | |
| "FolderStats", | |
| "read_folder_stats", | |
| "ThumbnailModel", | |
| "ThumbnailEntry", | |
| "ThumbnailPrefetcher", | |
| "ThumbnailCache", | |
| "ThumbnailProvider", | |
| "PathResolver", | |
| ] | |
| __all__ = [ | |
| "FolderStats", | |
| "PathResolver", | |
| "read_folder_stats", | |
| "ThumbnailCache", | |
| "ThumbnailEntry", | |
| "ThumbnailModel", | |
| "ThumbnailPrefetcher", | |
| "ThumbnailProvider", | |
| ] |
🧰 Tools
🪛 Ruff (0.14.14)
8-17: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
🤖 Prompt for AI Agents
In `@faststack/thumbnail_view/__init__.py` around lines 8 - 17, The __all__ export
list in faststack/thumbnail_view/__init__.py is not alphabetically sorted (Ruff
RUF022); reorder the entries in the __all__ list alphabetically (e.g.,
FolderStats, PathResolver, read_folder_stats, ThumbnailCache, ThumbnailEntry,
ThumbnailModel, ThumbnailPrefetcher, ThumbnailProvider) so the list is
isort-style sorted and Ruff will be satisfied.
| # Handle different sidecar formats | ||
| entries = data.get("entries", {}) | ||
| if not isinstance(entries, dict): | ||
| log.debug("Invalid entries format in %s", json_path) | ||
| return None | ||
|
|
There was a problem hiding this comment.
Guard against non-dict JSON roots to keep parsing tolerant.
data.get(...) will raise if the JSON root isn’t a dict, which contradicts the “tolerant to errors” contract and can bubble up to the UI. Consider a defensive type check (Line 79) to return None cleanly.
🛠️ Proposed fix
- # Handle different sidecar formats
- entries = data.get("entries", {})
+ # Handle different sidecar formats
+ if not isinstance(data, dict):
+ log.debug("Invalid JSON root in %s", json_path)
+ return None
+ entries = data.get("entries", {})📝 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.
| # Handle different sidecar formats | |
| entries = data.get("entries", {}) | |
| if not isinstance(entries, dict): | |
| log.debug("Invalid entries format in %s", json_path) | |
| return None | |
| # Handle different sidecar formats | |
| if not isinstance(data, dict): | |
| log.debug("Invalid JSON root in %s", json_path) | |
| return None | |
| entries = data.get("entries", {}) | |
| if not isinstance(entries, dict): | |
| log.debug("Invalid entries format in %s", json_path) | |
| return None | |
🤖 Prompt for AI Agents
In `@faststack/thumbnail_view/folder_stats.py` around lines 78 - 83, The code
assumes `data` is a dict before calling `data.get("entries", {})`, which can
raise if the JSON root is not a dict; add a defensive check like `if not
isinstance(data, dict): log.debug("Invalid JSON root in %s", json_path); return
None` before accessing `data.get`, then proceed with the existing `entries =
data.get("entries", {})` and the subsequent `if not isinstance(entries, dict):
...` check to ensure tolerant parsing; reference the `data`, `entries`, and
`json_path` variables when locating the change.
| def update_from_model(self, model: "ThumbnailModel"): | ||
| """Update registrations from a ThumbnailModel.""" | ||
| import hashlib | ||
| self.clear() | ||
| for i in range(model.rowCount()): | ||
| entry = model.get_entry(i) | ||
| if entry and not entry.is_folder: | ||
| path_hash = hashlib.md5( | ||
| str(entry.path.resolve()).encode("utf-8") | ||
| ).hexdigest()[:16] | ||
| self._hash_to_path[path_hash] = entry.path |
There was a problem hiding this comment.
Add TYPE_CHECKING import for ThumbnailModel.
The ThumbnailModel type hint is undefined at runtime because it's not imported. Add it to the TYPE_CHECKING block:
if TYPE_CHECKING:
from faststack.thumbnail_view.prefetcher import ThumbnailPrefetcher, ThumbnailCache
+ from faststack.thumbnail_view.model import ThumbnailModelRegarding the MD5 usage flagged by static analysis: this is a false positive. MD5 is being used as a fast hash for cache key generation, not for cryptographic purposes. The 16-character prefix provides sufficient uniqueness for path identification within a thumbnail cache.
🧰 Tools
🪛 Ruff (0.14.14)
209-209: Undefined name ThumbnailModel
(F821)
216-216: Probable use of insecure hash functions in hashlib: md5
(S324)
🤖 Prompt for AI Agents
In `@faststack/thumbnail_view/provider.py` around lines 209 - 219, The
update_from_model method uses a forward reference "ThumbnailModel" that isn't
imported at runtime; add a TYPE_CHECKING import block at module top (from typing
import TYPE_CHECKING; if TYPE_CHECKING: from .models import ThumbnailModel) so
the type hint resolves for static checking without runtime import, and keep the
existing MD5 usage in update_from_model/_hash_to_path but add a brief comment
next to the hashlib.md5 usage indicating it's non-cryptographic (cache key) to
silence static-analysis false positives (or add the appropriate linter noqa
flag).
1 participant
Summary by CodeRabbit
Release Notes v1.5.3
New Features
Bug Fixes
Performance
✏️ Tip: You can customize this high-level summary in your review settings.