Performance improvements, modernize Settings dialog#30
Conversation
WalkthroughRefactors imaging, caching, prefetch concurrency, and UI: adds ByteLRUCache.max_bytes, expands ImageEditor.auto_levels to return percentile anchors with stretch capping, makes Prefetcher thread-safe and zero-copy via memoryview-backed buffers, and rewrites SettingsDialog to a Window with tabbed settings and live cache metrics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI
participant Prefetcher
participant ThreadPool as WorkerPool
participant Decoder
participant Cache
UI->>Prefetcher: navigate(index)
Prefetcher->>Prefetcher: acquire _futures_lock
Prefetcher->>Prefetcher: compute generation, scheduled set
Prefetcher->>WorkerPool: submit decode task (with generation, priority)
WorkerPool->>Decoder: decode image -> contiguous buffer + memoryview
Decoder-->>WorkerPool: DecodedImage(mv buffer)
WorkerPool->>Cache: insert DecodedImage (key, bytes)
WorkerPool-->>Prefetcher: notify completion (generation)
Prefetcher->>Prefetcher: release _futures_lock
Prefetcher->>UI: signal decoded available / update histogram
Note: lock-held regions surround scheduling/cancellation; high-priority submissions may cancel lower-priority futures before enqueuing. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/imaging/prefetch.py (1)
514-528: Critical bug:arris undefined; saturation compensation will crash.After the memory optimization refactor,
bufferis converted to amemoryview(mv), but line 518 referencesarrwhich no longer exists in this scope. This will raise aNameErrorat runtime whencolor_mode == "saturation".Proposed fix
# Apply saturation compensation if enabled if color_mode == "saturation": try: factor = float(config.get('color', 'saturation_factor', fallback="1.0")) - apply_saturation_compensation(arr, w, h, bytes_per_line, factor) + # Need to work with the underlying numpy array, not memoryview + apply_saturation_compensation(buffer, w, h, bytes_per_line, factor) t_after_saturation = time.perf_counter()Note: Verify that
apply_saturation_compensationcan work with the contiguous buffer. Since it expects a 1D uint8 array and usesarr.reshape(), passingbuffer(fromnp.ascontiguousarray) should work. However,bufferis a 3D array at this point - you may need to flatten it first:apply_saturation_compensation(buffer.ravel(), w, h, bytes_per_line, factor)
🧹 Nitpick comments (12)
tests/test_prefetch_concurrency.py (1)
118-129: Unused loop variable and minor assertion gap.The
scheduled_setvariable is unused (per static analysis hint B007). Also, there's no assertion verifying that_scheduledkeys don't exceed the current generation.Proposed fix
- for gen, scheduled_set in prefetcher._scheduled.items(): + for gen in prefetcher._scheduled: if gen > prefetcher.generation: pytest.fail(f"Found scheduled set for future generation {gen} > {prefetcher.generation}") + + # Also verify futures keys don't contain stale generations + for idx in prefetcher.futures: + assert isinstance(idx, int), f"Expected int key, got {type(idx)}"test_cachetools_api.py (1)
1-16: This appears to be a debugging script, not a proper test.This file uses
print()statements for output and has no assertions, making it unsuitable as an automated test. It's also located at the repository root instead of in a tests directory.Consider either:
- Removing this file if it was only for exploratory purposes
- Converting it to a proper pytest test with assertions if it needs to be kept
If you want to keep this as a test
-"""Quick test to check cachetools.LRUCache API.""" +"""Test to verify cachetools.LRUCache API compatibility.""" +import pytest from cachetools import LRUCache -# Create a basic LRUCache -cache = LRUCache(maxsize=100) -# Check if maxsize is a property or method -print(f"Type of maxsize: {type(cache.maxsize)}") -print(f"maxsize value: {cache.maxsize}") +def test_lrucache_maxsize_property(): + """Verify LRUCache exposes maxsize as expected.""" + cache = LRUCache(maxsize=100) + + assert cache.maxsize == 100 + assert isinstance(cache.maxsize, int) -# Check if we can access the internal attribute -if hasattr(cache, '_Cache__maxsize'): - print(f"Internal _Cache__maxsize: {cache._Cache__maxsize}") -# List all attributes -print(f"\nAll cache attributes: {[attr for attr in dir(cache) if not attr.startswith('_')]}") +def test_lrucache_has_expected_public_interface(): + """Verify LRUCache has expected public methods.""" + cache = LRUCache(maxsize=100) + public_attrs = [attr for attr in dir(cache) if not attr.startswith('_')] + + assert 'maxsize' in public_attrs + assert 'currsize' in public_attrstest_max_bytes.py (1)
1-36: Test script at wrong location with code duplication.This test file has several issues:
- Located at repository root instead of
faststack/tests/- Duplicates
MockItemclass that already exists infaststack/tests/test_cache.py- Mixes
print()statements with assertions (should use pytest's reporting)Consider moving this to
faststack/tests/test_max_bytes.pyand importing the sharedMockItem.Proposed refactor
-"""Quick test to verify ByteLRUCache.max_bytes works correctly.""" +"""Tests for ByteLRUCache.max_bytes dynamic adjustment.""" +import pytest from faststack.imaging.cache import ByteLRUCache +from faststack.tests.test_cache import MockItem # Reuse existing MockItem -class MockItem: - def __init__(self, size: int): - self._size = size - - def __sizeof__(self) -> int: - return self._size -# Test 1: Initialize cache -cache = ByteLRUCache(max_bytes=1000, size_of=lambda x: x.__sizeof__()) -print(f"Initial max_bytes: {cache.max_bytes}") -assert cache.max_bytes == 1000, "Initial max_bytes should be 1000" +def test_max_bytes_dynamic_update(): + """Test that max_bytes can be dynamically updated and triggers eviction.""" + cache = ByteLRUCache(max_bytes=1000, size_of=lambda x: x.__sizeof__()) + assert cache.max_bytes == 1000 + + cache["a"] = MockItem(50) + cache["b"] = MockItem(40) + assert cache.currsize == 90 + + # Reduce capacity + cache.max_bytes = 80 + assert cache.max_bytes == 80 + + # Add item that triggers eviction + cache["c"] = MockItem(50) + + assert "a" not in cache # LRU evicted + assert cache.currsize <= cache.max_bytesfaststack/imaging/cache.py (1)
78-90: Memoryview buffers should be handled correctly, but verify.With the prefetch changes using
memoryview, this size calculation should still work sincememoryviewhas annbytesattribute (line 80-81 check). However, the comment and type hints don't reflect this new buffer type.Optional: Document memoryview support
if isinstance(item, DecodedImage): - # Handle both numpy arrays and memoryview buffers + # Handle numpy arrays, memoryview, bytes, and bytearray buffers if hasattr(item.buffer, "nbytes"): + # numpy arrays and memoryview both have nbytes return item.buffer.nbytesfaststack/tests/test_new_features.py (3)
166-166: Prefix unused variables with underscore.The linter correctly identifies that
blacksandwhitesare unpacked but never used in this test. Since you only needp_lowandp_highfor the stretch calculation, prefix the unused variables with underscore to indicate they're intentionally ignored.🔎 Proposed fix
- blacks, whites, p_low, p_high = self.editor.auto_levels(threshold_percent) + _blacks, _whites, p_low, p_high = self.editor.auto_levels(threshold_percent)
156-174: Strengthen assertions for the degenerate case.Test case 3 validates that very low dynamic range is handled, but it only checks that
dynamic_range < 3.0. According to the app.py logic, whendynamic_range < 1.0, strength should be set to exactly 0.0. Consider adding an assertion to verify this behavior explicitly:# For very low range, strength should be 0 if dynamic_range < 1.0: self.assertEqual(expected_strength, 0.0, "Degenerate range should result in zero strength")This would provide stronger validation of the edge case handling.
230-230: Prefix unused variables with underscore.Similar to line 166, the linter identifies that
p_lowandp_highare unpacked but never used in the gradient monotonicity test. Prefix them with underscore to indicate they're intentionally ignored.🔎 Proposed fix
- blacks, whites, p_low, p_high = self.editor.auto_levels(threshold_percent) + blacks, whites, _p_low, _p_high = self.editor.auto_levels(threshold_percent)faststack/app.py (2)
3253-3290: Consider making STRETCH_CAP configurable.The stretch-capping logic is well-implemented with clear handling of edge cases (degenerate range, reasonable stretch, excessive stretch). However,
STRETCH_CAP = 4.0is hardcoded at line 3277. Different image types or user preferences might benefit from different cap values.Consider making this configurable:
STRETCH_CAP = config.getfloat('core', 'auto_level_stretch_cap', 4.0)This would allow users to fine-tune the behavior without code changes, similar to
auto_level_thresholdandauto_level_strength.
3253-3263: Consider condensing the philosophy comment.The multi-line comment explaining the stretch-capping philosophy is helpful but could be more concise. The key points are:
- Threshold clipping is intentional, not prevented
- Stretch-capping prevents extreme levels on low-dynamic-range images
Consider a more succinct version:
# Auto-strength caps stretch factor to prevent extreme adjustments on low-dynamic-range images. # Intentional clipping (defined by threshold_percent) is preserved. # Cap approach: limit stretch to STRETCH_CAP (e.g., 4x) by blending strength.This maintains clarity while reducing verbosity.
faststack/qml/SettingsDialog.qml (3)
7-20: Consider adding window centering logic.The Window doesn't specify initial positioning, which may cause it to appear at an arbitrary screen location on first open. Consider adding centering logic in the
open()function.🔎 Suggested addition to the open() function
function open() { // Reload all properties from uiState to ensure Cancel discards edits if (uiState) { heliconPath = uiState.get_helicon_path() // ... rest of property loading } + // Center the window on screen + x = (Screen.width - width) / 2 + y = (Screen.height - height) / 2 visible = true raise() requestActivate() }Note: This requires
import QtQuick.Window 2.15which is already present.
135-327: Consider extracting custom components to separate files for reusability.The custom styled components (TextField, Slider, SpinBox, etc.) are well-designed but tightly coupled to
settingsDialogproperties. For better reusability across the application, consider extracting them to separate QML files with exposed theming properties.Example refactor pattern
Create a separate file
StyledTextField.qml:import QtQuick 2.15 import QtQuick.Controls 2.15 TextField { id: control // Expose theme properties property color textColor: "white" property color accentColor: "#6366f1" property color controlBorder: "#30ffffff" color: textColor placeholderTextColor: "#80ffffff" selectionColor: accentColor selectedTextColor: "#ffffff" font.pixelSize: 13 background: Rectangle { color: control.enabled ? "transparent" : "#05ffffff" border.color: control.activeFocus ? accentColor : controlBorder border.width: 1 radius: 4 } }Then use it directly without Loader overhead:
StyledTextField { textColor: settingsDialog.textColor accentColor: settingsDialog.accentColor // ... }This would improve performance by eliminating Loader overhead and make components reusable across dialogs.
337-371: Minor optimization opportunity: Direct instantiation for static tab buttons.The tab buttons use Loaders, which adds overhead for what appear to be static elements. Since the tab count is fixed, consider direct instantiation instead.
Alternative approach without Loaders
RowLayout { anchors.fill: parent anchors.margins: 20 anchors.bottomMargin: 0 spacing: 20 - Loader { + Rectangle { + property string text: "General" + property int index: 0 Layout.fillWidth: true Layout.fillHeight: true - sourceComponent: tabButton - onLoaded: { item.text = "General"; item.index = 0 } + // ... tab button implementation inline } - Loader { + Rectangle { + property string text: "Auto Adjustments" + property int index: 1 Layout.fillWidth: true Layout.fillHeight: true - sourceComponent: tabButton - onLoaded: { item.text = "Auto Adjustments"; item.index = 1 } + // ... tab button implementation inline } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
faststack/app.pyfaststack/imaging/cache.pyfaststack/imaging/editor.pyfaststack/imaging/prefetch.pyfaststack/qml/Main.qmlfaststack/qml/SettingsDialog.qmlfaststack/tests/test_cache.pyfaststack/tests/test_new_features.pytest_cachetools_api.pytest_max_bytes.pytests/test_prefetch_concurrency.py
🧰 Additional context used
🧬 Code graph analysis (5)
faststack/tests/test_new_features.py (2)
faststack/app.py (1)
auto_levels(3233-3315)faststack/imaging/editor.py (3)
auto_levels(488-537)set_edit_param(594-637)_apply_edits(278-486)
faststack/imaging/editor.py (2)
faststack/app.py (1)
auto_levels(3233-3315)faststack/ui/provider.py (4)
blacks(903-904)blacks(907-910)whites(913-914)whites(917-920)
faststack/tests/test_cache.py (1)
faststack/imaging/cache.py (2)
max_bytes(28-30)max_bytes(33-37)
faststack/app.py (3)
faststack/imaging/cache.py (3)
build_cache_key(93-99)max_bytes(28-30)max_bytes(33-37)faststack/ui/provider.py (4)
blacks(903-904)blacks(907-910)whites(913-914)whites(917-920)faststack/imaging/editor.py (1)
auto_levels(488-537)
test_max_bytes.py (2)
faststack/imaging/cache.py (3)
ByteLRUCache(12-69)max_bytes(28-30)max_bytes(33-37)faststack/tests/test_cache.py (1)
MockItem(7-13)
🪛 Ruff (0.14.10)
faststack/tests/test_new_features.py
166-166: Unpacked variable blacks is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
166-166: Unpacked variable whites is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
230-230: Unpacked variable p_low is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
230-230: Unpacked variable p_high is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/test_prefetch_concurrency.py
31-31: Unused function argument: args
(ARG001)
31-31: Unused function argument: kwargs
(ARG001)
63-63: Multiple statements on one line (colon)
(E701)
67-67: Do not catch blind exception: Exception
(BLE001)
75-75: Multiple statements on one line (colon)
(E701)
78-78: Do not catch blind exception: Exception
(BLE001)
90-90: Multiple statements on one line (colon)
(E701)
94-94: Do not catch blind exception: Exception
(BLE001)
121-121: Loop control variable scheduled_set not used within loop body
Rename unused scheduled_set to _scheduled_set
(B007)
🔇 Additional comments (25)
tests/test_prefetch_concurrency.py (1)
59-96: LGTM - Solid concurrency stress test pattern.The barrier synchronization, stop_event for early termination, and error aggregation pattern are well-designed for stress testing race conditions. The broad
Exceptioncatches are acceptable here since the goal is to capture any failure from concurrent operations.faststack/imaging/editor.py (1)
488-537: LGTM - Clean signature expansion with good defensive practices.The expanded return type
(blacks, whites, p_low, p_high)aligns with the app-level consumption shown infaststack/app.py:3249-3250. Defensive clamping ofthreshold_percentto[0.0, 10.0]and proper lock usage for state updates are good practices.faststack/imaging/prefetch.py (1)
163-166: Good addition of_futures_lockfor thread safety.Using
RLockis appropriate here since the methods may potentially call each other (e.g.,shutdowncallscancel_all). The lock guardsfutures,_scheduled, andgenerationconsistently acrossupdate_prefetch,submit_task, andcancel_all.faststack/tests/test_cache.py (1)
15-19: LGTM - Assertion updated to use newmax_bytesproperty.The test correctly uses the new
max_bytesproperty instead of accessingmaxsizedirectly, aligning with the API changes inByteLRUCache.faststack/imaging/cache.py (1)
32-37: Consider whether immediate eviction is needed when reducingmax_bytes.The setter updates
maxsizebut doesn't proactively evict items ifcurrsize > max_bytesafter the update. Items will only be evicted on the next insert. This lazy eviction approach may be intentional, but if immediate enforcement is needed:Is the current lazy eviction behavior acceptable, or should reducing
max_bytesbelowcurrsizetrigger immediate eviction?Optional: Immediate eviction on capacity reduction
@max_bytes.setter def max_bytes(self, value: int) -> None: """Set the maximum cache size in bytes.""" v = max(0, int(value)) self.maxsize = v log.debug(f"Cache max_bytes updated to {v / 1024**2:.2f} MB") + + # Proactively evict if over capacity + while self.currsize > self.maxsize: + try: + self.popitem() + except KeyError: + break # Cache is emptyfaststack/tests/test_new_features.py (1)
24-40: LGTM! Test properly validates the new API.The test correctly adapts to the new 4-tuple return signature from
auto_levelsand adds meaningful assertions for the percentile anchor points (p_low,p_high). The expected ranges (45-55 forp_low, 195-205 forp_high) are appropriate given the input distribution (50-200) and 0.1% threshold.faststack/app.py (4)
462-462: LGTM! Proper use of parameterized logging.The change from f-string to parameterized logging (
%splaceholders) follows Python logging best practices. This defers string formatting until the log is actually emitted, improving performance when the log level filters out debug messages.
517-557: LGTM! Robust exception handling with proper fallbacks.The refactored exception handling is well-structured:
- Handles
TimeoutErrorandCancelledErrorexplicitly- Cascading validation (future → result → cache_key) prevents edge cases
- Consistent fallback to
last_displayed_imageprevents grey squares on failures- 5-second timeout is a sensible safety net
The improved defensive programming will make the decoding pipeline more resilient.
603-620: LGTM! Consistent exception handling for background workers.The exception handling mirrors the pattern in
get_decoded_image, which is appropriate for thread-safe decoding. The timeout handling (lines 603-610) and general exception catch (lines 618-619) provide defensive coverage for background operations.
1325-1332: LGTM! Consistent use of the new max_bytes property.The changes from
maxsizetomax_bytesalign with the new public property added toByteLRUCache. The property provides a clearer, more explicit API for cache capacity management. All usages (lines 1325, 1332, and 2269) are consistent.faststack/qml/SettingsDialog.qml (13)
1-6: LGTM!The imports appropriately support the new Window-based design with Material theming.
22-59: LGTM!Properties are well-organized with appropriate types. The color palette provides a clean, modern theme, and the live cache monitoring property (
cacheUsage) is properly structured for timer-based updates.
61-87: LGTM!The
open()function correctly implements "Cancel discards edits" semantics by reloading all properties fromuiStatebefore showing the window. The activation sequence ensures proper window focus.
89-93: LGTM!The Escape shortcut provides standard keyboard navigation with appropriate window scope.
95-108: LGTM!The visibility handler correctly manages the cache usage timer and dialog lifecycle notifications. The defensive checks for loader items before accessing their properties are good practice.
110-133: LGTM!Centralized settings persistence is a clean design pattern. Individual controls handle validation, so bulk write-back is appropriate here.
517-543: LGTM!Cache size validation is robust with appropriate range checks and user-friendly fallback behavior. The live usage display helps users make informed decisions about cache sizing.
504-516: LGTM!The tooltip pattern provides contextual help effectively. The descriptions are clear and informative without overwhelming the interface.
630-710: LGTM!Auto Levels section has well-designed controls. The Auto checkbox correctly disables the manual strength slider to prevent conflicting inputs, and the tooltips provide excellent guidance on parameter usage.
720-824: LGTM!The AWB controls provide granular control with clear visual feedback. The dynamic percentage display in the strength label enhances usability.
833-910: LGTM!Advanced threshold controls provide fine-grained tuning capability. The tooltips clearly explain the impact of each threshold on the AWB algorithm's pixel selection.
919-975: LGTM!The bottom action bar provides clear, well-styled controls with appropriate visual hierarchy. The Save button's accent color styling effectively indicates the primary action.
977-985: LGTM!The cache usage timer is appropriately scoped to run only when visible, with defensive checks for uiState existence.
faststack/qml/Main.qml (2)
249-253: LGTM!The simplified settings dialog opening delegates state management to the dialog's
open()method, improving separation of concerns and eliminating code duplication.
858-862: LGTM!The text formatting adjustments improve readability and clarity in the help dialog. These are minor cosmetic improvements with no functional impact.
Also applies to: 882-882, 897-897
|
|
||
| # Memory Optimization: Avoid explicit copy | ||
| buffer = np.ascontiguousarray(rgb) | ||
| bytes_per_line = buffer.strides[0] | ||
| mv = memoryview(buffer).cast("B") | ||
| t_after_copy = time.perf_counter() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file size and read the relevant sections
wc -l faststack/imaging/prefetch.pyRepository: AlanRockefeller/faststack
Length of output: 102
🏁 Script executed:
# Read the code around lines 395-400 and 542-548
sed -n '385,410p' faststack/imaging/prefetch.py
echo "---"
sed -n '535,555p' faststack/imaging/prefetch.pyRepository: AlanRockefeller/faststack
Length of output: 3129
🏁 Script executed:
# Find the DecodedImage class definition and see how it stores the memoryview
ast-grep --pattern $'class DecodedImage {
$$$
}'Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Search for where mv is assigned to DecodedImage
rg -n "DecodedImage" faststack/imaging/prefetch.py -B 2 -A 10Repository: AlanRockefeller/faststack
Length of output: 1184
🏁 Script executed:
# Find DecodedImage definition in models
fd "models.py" faststack/Repository: AlanRockefeller/faststack
Length of output: 88
🏁 Script executed:
# Read the DecodedImage class definition
rg -n "class DecodedImage" faststack/models.py -A 30Repository: AlanRockefeller/faststack
Length of output: 360
🏁 Script executed:
# Also check if there are any usages of DecodedImage.buffer to understand the lifecycle
rg -n "\.buffer" faststack/ --type py -B 2 -A 2 | head -100Repository: AlanRockefeller/faststack
Length of output: 2328
🏁 Script executed:
# Check if there's any mechanism to retain the buffer beyond just the memoryview
# Let's look at the full _decode_and_cache function to see the complete context
sed -n '300,560p' faststack/imaging/prefetch.py | tail -100Repository: AlanRockefeller/faststack
Length of output: 5903
🏁 Script executed:
# Check if there's a @dataclass decorator on DecodedImage that might add __post_init__
rg -B 10 "class DecodedImage" faststack/models.pyRepository: AlanRockefeller/faststack
Length of output: 407
🏁 Script executed:
# Look for any additional cache mechanisms or buffer retention
rg -n "buffer.*=" faststack/models.pyRepository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Verify one more time - check if there's any possibility that the buffer array is retained elsewhere
# Let's look at the complete function signature and see all local variables
sed -n '200,250p' faststack/imaging/prefetch.py | head -50Repository: AlanRockefeller/faststack
Length of output: 2688
🏁 Script executed:
# Check if cache.put or similar might retain the buffer
rg -n "cache_put\|put\(" faststack/imaging/cache.py -B 3 -A 5 | head -50Repository: AlanRockefeller/faststack
Length of output: 51
Critical: Memoryview references a local numpy array that will be garbage collected after function returns, causing dangling memory references.
The buffer variable is created locally in _decode_and_cache via np.ascontiguousarray() and only the memoryview(buffer) is stored in DecodedImage. Once the function returns, the numpy array goes out of scope and will be garbage collected, leaving the memoryview as a dangling reference. Keeping the memoryview in _keepalive (as attempted in ui/provider.py) does not prevent garbage collection of the underlying object.
Store a reference to the numpy array alongside the memoryview in DecodedImage, or refactor to ensure the underlying buffer remains alive for the lifetime of the cached DecodedImage.
Also applies to lines 542-548 (all code paths that create memoryview from local buffer).
🤖 Prompt for AI Agents
In faststack/imaging/prefetch.py around lines 395-400 (and similarly lines
542-548), the code creates a local numpy array via np.ascontiguousarray(...) and
stores only a memoryview(buffer) in DecodedImage, which leads to a dangling
memory reference when the local numpy array is garbage collected; fix by storing
a strong reference to the underlying numpy array alongside the memoryview within
DecodedImage (e.g., add an attribute that keeps the numpy array alive for the
lifetime of the cached object) or refactor to avoid creating a memoryview of a
transient local buffer so the underlying buffer remains owned by the
DecodedImage for all code paths that create memoryviews.
| // Helicon Path | ||
| Label { text: "Helicon Focus Path"; color: "#aaaaaa"; font.pixelSize: 12 } | ||
| RowLayout { | ||
| Layout.fillWidth: true | ||
| Loader { | ||
| id: heliconField | ||
| sourceComponent: styledTextField | ||
| Layout.fillWidth: true | ||
| onLoaded: { | ||
| // Text is set once in onVisibleChanged | ||
| item.text = settingsDialog.heliconPath | ||
| item.textEdited.connect(function() { settingsDialog.heliconPath = item.text }) | ||
| } | ||
| } | ||
| Button { | ||
| text: "Browse" | ||
| flat: true | ||
| onClicked: { | ||
| var path = uiState.open_file_dialog() | ||
| if (path) { | ||
| settingsDialog.heliconPath = path | ||
| if (heliconField.item) heliconField.item.text = path | ||
| } | ||
| } | ||
| background: Rectangle { color: parent.pressed ? "#20ffffff" : "#10ffffff"; radius: 4 } | ||
| contentItem: Text { text: parent.text; color: settingsDialog.textColor; horizontalAlignment: Text.AlignHCenter; verticalAlignment: Text.AlignVCenter } | ||
| } | ||
| Label { | ||
| text: "✔" | ||
| color: "#4ade80" | ||
| visible: uiState && uiState.check_path_exists(settingsDialog.heliconPath) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find files that might define uiState
fd -type f -name "*.py" -o -name "*.qml" | head -20Repository: AlanRockefeller/faststack
Length of output: 239
🏁 Script executed:
# Search for uiState definition and open_file_dialog implementation
rg "open_file_dialog|class.*[Uu]i.*[Ss]tate|def open_file_dialog" --type py -A 5Repository: AlanRockefeller/faststack
Length of output: 822
🏁 Script executed:
# Search for check_path_exists implementation
rg "check_path_exists" --type py -A 5Repository: AlanRockefeller/faststack
Length of output: 737
🏁 Script executed:
# Check if there's a separate open_directory_dialog method
rg "open_directory_dialog|FileMode" -A 3Repository: AlanRockefeller/faststack
Length of output: 1630
🏁 Script executed:
# Search for other usages of open_file_dialog to see if pattern is consistent
rg "open_file_dialog\(\)"Repository: AlanRockefeller/faststack
Length of output: 363
Fix edge case handling in uiState dialog methods.
While the QML code correctly handles cancelled dialogs with if (path) checks, the underlying open_file_dialog() and check_path_exists() methods have unhandled edge cases:
open_file_dialog()has a hardcoded "Executables (*.exe)" filter, which will prevent selecting valid Helicon Focus paths on Linux/Mac systemscheck_path_exists()lacks error handling for permission errors that could cause crashesopen_file_dialog()implicitly returnsNoneon cancel, whileopen_directory_dialog()explicitly returns"", creating an inconsistencyselectedFiles()[0]access is not protected against unexpected empty results
Ensure dialog methods handle permission errors gracefully and use consistent return values.
🤖 Prompt for AI Agents
faststack/qml/SettingsDialog.qml lines 396-428: the review notes underlying
uiState dialog methods need robust edge-case handling — update
open_file_dialog() to use platform-appropriate filters (or no hardcoded
"Executables (*.exe)") so Linux/Mac paths can be selected, make its cancel
return consistent with open_directory_dialog() (use "" rather than null/None),
protect access to selectedFiles()[0] by checking selectedFiles().length>0 before
indexing and return "" on empty selection, and add try/catch around file-dialog
and check_path_exists() logic to handle permission errors (log/return false or
"" as appropriate) so permission-denied conditions don’t throw and behavior is
consistent across dialogs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
faststack/app.py (1)
1301-1311: Harden open_file_dialog path selection and avoid over-strict filter.
open_file_dialog()is still a bit fragile and tightly Windows‑specific:dialog = QFileDialog() dialog.setFileMode(QFileDialog.FileMode.ExistingFile) dialog.setNameFilter("Executables (*.exe)") if dialog.exec(): return dialog.selectedFiles()[0] return ""Two points:
selectedFiles()[0]is not guarded against an unexpected empty list; if the dialog reports success butselectedFiles()is empty, this will raiseIndexError.- The
"Executables (*.exe)"filter blocks non‑.exepaths, which is fine for a strictly Windows‑only Helicon/Photoshop setup but will prevent selecting valid binaries/scripts if you ever run this code on other platforms.You can keep behavior the same while making it safer:
Suggested hardening
def open_file_dialog(self): dialog = QFileDialog() dialog.setFileMode(QFileDialog.FileMode.ExistingFile) - dialog.setNameFilter("Executables (*.exe)") - if dialog.exec(): - return dialog.selectedFiles()[0] - return "" + dialog.setNameFilter("Executables (*.exe)") + if dialog.exec(): + files = dialog.selectedFiles() + if files: + return files[0] + return ""(If you later broaden platform support, swapping the hardcoded filter for something platform‑aware would be the next step.)
🧹 Nitpick comments (3)
debug_al.py (1)
6-18: Tidy unused unpacked values in debug harness.You only use
p_highfromauto_levelshere; prefix the others with_to silence Ruff and make intent clear:Proposed tweak
- blacks, whites, p_low, p_high = editor.auto_levels(threshold_percent=0.1) + _blacks, _whites, _p_low, p_high = editor.auto_levels(threshold_percent=0.1)faststack/tests/test_auto_levels.py (1)
8-137: Tests look solid; consider hiding unused tuple elements.The scenarios you cover (highlight/shadow pinning, tiny hot pixel, degenerate image, normal range) match the new
auto_levelsbehavior well. In a couple of places you unpack values you don’t assert on (blacksintest_auto_levels_pins_highlights_if_clipped, andblacks/p_lowintest_auto_levels_tiny_hot_pixel_ignored). For cleanliness and to satisfy Ruff, you could rename the unused ones:Example adjustments
- blacks, whites, p_low, p_high = editor.auto_levels(threshold_percent=0.0) + _blacks, whites, p_low, p_high = editor.auto_levels(threshold_percent=0.0) - blacks, whites, p_low, p_high = editor.auto_levels(threshold_percent=0.1) + _blacks, whites, _p_low, p_high = editor.auto_levels(threshold_percent=0.1)faststack/app.py (1)
3233-3335: Consider signaling “no-op” auto-levels to quick_auto_levels to avoid pointless saves.The integration with the new
ImageEditor.auto_levels()output looks correct:
blacks/whitesare scaled by a strength derived from a capped stretch factor whenauto_level_strength_autois enabled.- Status messages are driven by
p_low/p_high, independent of the strength scaling, which is the right metric for pinning/degenerate detection.However,
auto_levels()always returnsTrueon success, even in cases where:
dynamic_range < 1.0→strength = 0.0, and the editor already setblacks = whites = 0.0(degenerate range), or- the image is already full-range (
p_low <= 0andp_high >= 255).
quick_auto_levels()currently uses the boolean return only to skip saving when auto-levels were “skipped” entirely:applied = self.auto_levels() if self.auto_level_strength_auto and not applied: returnIf you want to avoid creating backups and re-saving files when nothing effectively changes, you could have
auto_levels()returnFalsein the “no changes (degenerate/full range)” cases and letquick_auto_levels()’snot appliedcheck short‑circuit there as well.Sketch of possible refinement
- if abs(p_high - p_low) < 1.0: - msg = "Auto levels: no changes (degenerate range)" - elif p_low <= 0 and p_high >= 255: - msg = "Auto levels: no changes (image already covers full range)" + no_effect = False + if abs(p_high - p_low) < 1.0: + msg = "Auto levels: no changes (degenerate range)" + no_effect = True + elif p_low <= 0 and p_high >= 255: + msg = "Auto levels: no changes (image already covers full range)" + no_effect = True # ... - self.update_status_message(f"{msg} (preview only)") + self.update_status_message(f"{msg} (preview only)") + if no_effect: + return False log.info(...) - return True + return True(Not suggesting you must do it now, but this would align the docstring “Returns False if skipped” with the new behavior and keep
quick_auto_levels()from doing unnecessary disk I/O.)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
debug_al.pyfaststack/app.pyfaststack/imaging/editor.pyfaststack/qml/SettingsDialog.qmlfaststack/tests/test_auto_levels.pypyproject.tomlreproduce_config_issue.pytest_output.txt
✅ Files skipped from review due to trivial changes (1)
- reproduce_config_issue.py
🧰 Additional context used
🧬 Code graph analysis (3)
faststack/tests/test_auto_levels.py (1)
faststack/imaging/editor.py (1)
auto_levels(488-563)
faststack/imaging/editor.py (2)
faststack/app.py (1)
auto_levels(3233-3334)faststack/ui/provider.py (4)
blacks(903-904)blacks(907-910)whites(913-914)whites(917-920)
debug_al.py (1)
faststack/imaging/editor.py (2)
ImageEditor(169-798)auto_levels(488-563)
🪛 Ruff (0.14.10)
faststack/tests/test_auto_levels.py
24-24: Unpacked variable blacks is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
91-91: Unpacked variable blacks is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
91-91: Unpacked variable p_low is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
debug_al.py
17-17: Unpacked variable blacks is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
17-17: Unpacked variable whites is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
17-17: Unpacked variable p_low is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (5)
pyproject.toml (1)
20-28: OpenCV dependency aligns with new Lab AWB path.Adding
opencv-python>=4.0,<5.0is consistent withauto_white_balance_lab()usingcv2infaststack/app.py. Just make sure your deployment environment has matching wheels for your target platforms and that you’re okay with the extra binary size/runtime dependency.faststack/imaging/editor.py (1)
488-563: auto_levels math and state updates look consistent and well-bounded.The percentile-based
p_low/p_highcomputation, clipping detection (eps_pctwith near-extreme counts), and mapping back toblacks/whites(black_point = p_low,white_point = p_highvia the 40x scale factors) line up correctly with_apply_edits. Degenerate-range handling (p_high - p_low < 1.0) avoids extreme stretching, and edits are committed under the lock with_edits_revbumped, which matches the preview caching protocol. Tests infaststack/tests/test_auto_levels.pyappear to cover the key edge cases; I don’t see any required changes here.faststack/qml/SettingsDialog.qml (1)
7-133: Window-based settings dialog + open/save flow look coherent.The switch to a
Windowwith anopen()helper that reloads all properties fromuiState, paired with a singlesaveSettings()that writes everything back and closes, gives you clear Cancel semantics and a much simpler mental model than per-field writes. TheonVisibleChangedwiring tocontroller.dialog_opened()/dialog_closed()andcacheUsageTimeralso fits neatly with the_dialog_openflag and cacheUsage UI. No functional issues spotted in this part.faststack/app.py (2)
436-565: Robust decode path with futures + last_displayed_image fallback looks good.The new
get_decoded_imageflow:
- Bails out early for invalid indices.
- Uses a priority prefetch task and cleanly handles
TimeoutError,CancelledError, and generic exceptions fromfuture.result(timeout=5.0).- Falls back consistently to
last_displayed_imagewhen the task can’t be obtained, returnsNone, or the cache key is missing, with the decoding indicator reliably cleared infinally.Similarly,
_get_decoded_image_safenow wraps its ownfuture.result(timeout=5.0)with the same timeout/cancellation handling, which is appropriate for the histogram worker. Overall this is a solid improvement in resilience; I don’t see a correctness regression here.
1319-1343: Byte-based cache resizing and thrash metrics match ByteLRUCache semantics.Switching
set_cache_size()to operate onimage_cache.max_bytesandcurrsize(with a while-loop eviction usingpopitem()untilcurrsize <= new_max_bytes) is consistent with a size-of-based LRU likeByteLRUCache. The updated_on_cache_evictmessage usingself.image_cache.max_bytesto computemax_gbalso keeps the UI/debug output aligned with the actual capacity after runtime resizes. No further changes needed here.Also applies to: 2268-2272
| Layout.fillWidth: true | ||
|
|
||
| // AWB Mode | ||
| Label { | ||
| text: "Algorithm" | ||
| color: settingsDialog.textColor | ||
|
|
||
| MouseArea { | ||
| id: awbAlgorithmHover | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| } | ||
|
|
||
| ToolTip.visible: awbAlgorithmHover.containsMouse | ||
| ToolTip.text: "Algorithm for auto white balance. 'lab' analyzes in LAB color space for perceptually uniform results. 'rgb' works directly in RGB space. Most users should use 'lab'." | ||
| } | ||
| ComboBox { | ||
| model: ["lab", "rgb"] | ||
| currentIndex: Math.max(0, model.indexOf(settingsDialog.awbMode)) | ||
| onActivated: settingsDialog.awbMode = model[currentIndex] | ||
| Layout.preferredWidth: 150 | ||
| delegate: ItemDelegate { | ||
| width: parent.width | ||
| contentItem: Text { text: modelData; color: settingsDialog.textColor; verticalAlignment: Text.AlignVCenter } | ||
| background: Rectangle { color: parent.highlighted ? "#20ffffff" : "transparent" } | ||
| } | ||
| contentItem: Text { text: parent.displayText; color: settingsDialog.textColor; verticalAlignment: Text.AlignVCenter; leftPadding: 10 } | ||
| background: Rectangle { color: "#10ffffff"; border.color: settingsDialog.controlBorder; radius: 4 } | ||
| } | ||
|
|
||
| // Strength | ||
| Label { | ||
| text: "Strength (" + (awbStrSlider.item ? Math.round(awbStrSlider.item.value * 100) : 0) + "%)" | ||
| color: settingsDialog.textColor | ||
|
|
||
| MouseArea { | ||
| id: awbStrengthHover | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| } | ||
|
|
||
| ToolTip.visible: awbStrengthHover.containsMouse | ||
| ToolTip.text: "How aggressively to apply the auto white balance correction. 100% applies full correction, lower values blend with original. Range: 30-100%. Recommended: 70%" | ||
| } | ||
| Loader { | ||
| id: awbStrSlider | ||
| sourceComponent: styledSlider | ||
| Layout.fillWidth: true | ||
| onLoaded: { | ||
| item.from = 0.3; item.to = 1.0 | ||
| item.value = settingsDialog.awbStrength | ||
| item.valueChanged.connect(function() { settingsDialog.awbStrength = item.value }) | ||
| } | ||
| Binding { | ||
| target: parent.item | ||
| property: "value" | ||
| value: settingsDialog.awbStrength | ||
| when: parent.item && !parent.item.pressed | ||
| } | ||
| } | ||
|
|
||
| // Warm Bias | ||
| Label { | ||
| text: "Warm Bias (Yel/Blu)" | ||
| color: settingsDialog.textColor | ||
|
|
||
| MouseArea { | ||
| id: warmBiasHover | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| } | ||
|
|
||
| ToolTip.visible: warmBiasHover.containsMouse | ||
| ToolTip.text: "Shifts the white balance warmer (yellow, positive values) or cooler (blue, negative values) after auto correction. Useful to compensate for systematic color casts. Range: -50 to +50. Default: +6" | ||
| } | ||
| Loader { | ||
| sourceComponent: styledSpinBox | ||
| onLoaded: { | ||
| item.from = -50; item.to = 50 | ||
| item.value = settingsDialog.awbWarmBias | ||
| item.valueChanged.connect(function() { settingsDialog.awbWarmBias = item.value }) | ||
| } | ||
| Binding { | ||
| target: parent.item | ||
| property: "value" | ||
| value: settingsDialog.awbWarmBias | ||
| when: parent.item && !parent.item.down // SpinBox uses down, not pressed? Or implicit pressed? SpinBox interaction is complex. | ||
| // Actually SpinBox 'value' property should be bound. SpinBox breaks binding on user input. | ||
| // 'down' property exists for internal buttons but maybe not the whole control. | ||
| // Let's assume standard Binding restoration behavior works or checking activeFocus might correspond to editing. | ||
| // Standard QtQuick Controls 2 SpinBox has 'down'. | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix Binding condition for AWB “Warm Bias” SpinBox to avoid fragile down usage.
The Binding
Binding {
target: parent.item
property: "value"
value: settingsDialog.awbWarmBias
when: parent.item && !parent.item.down
// ...
}is risky: parent.item is a SpinBox, which doesn’t expose a boolean down property in the same way a button does, so !parent.item.down can misbehave or generate binding warnings.
You can safely mirror the pattern you use elsewhere (e.g., text fields) and gate on focus instead:
Suggested change
- Binding {
- target: parent.item
- property: "value"
- value: settingsDialog.awbWarmBias
- when: parent.item && !parent.item.down
- // ...
- }
+ Binding {
+ target: parent.item
+ property: "value"
+ value: settingsDialog.awbWarmBias
+ // Only push config value when the user isn't actively editing
+ when: parent.item && !parent.item.activeFocus
+ }This keeps external updates in sync without relying on an implementation-detail property.
🤖 Prompt for AI Agents
In faststack/qml/SettingsDialog.qml around lines 736 to 828 the Binding for the
AWB "Warm Bias" SpinBox uses parent.item.down which is fragile because SpinBox
doesn't reliably expose a down property; replace the when clause with a
focus-based guard (e.g. when: parent.item && !parent.item.activeFocus) so
external updates are applied only when the control is not focused, mirroring the
safer pattern used elsewhere; ensure the Binding target remains
parent.item.value and that the control's onLoaded still wires item.valueChanged
to update settingsDialog.awbWarmBias.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.