Fix some bugs#34
Conversation
WalkthroughThis PR introduces comprehensive imaging pipeline improvements including TurboJPEG-accelerated JPEG decoding, ICC color management with saturation compensation in the prefetcher, EXIF orientation sanitization in the editor, restored mathematical utilities for highlight recovery, and significantly expands the UI state surface with new properties, signals, and snake_case naming conventions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as JPEG Request
participant Decoder as Decode Module
participant TurboJPEG as TurboJPEG Library
participant Pillow as Pillow (PIL)
participant ICC as ICC Transform Cache
participant Output as RGB Array + Metadata
Client->>Decoder: decode_jpeg_rgb(jpeg_bytes, fast_dct)
Decoder->>Decoder: Check TURBO_AVAILABLE flag
alt TurboJPEG Available
Decoder->>TurboJPEG: Extract JPEG header
Decoder->>TurboJPEG: decode_from_file() with optional TJFLAG_FASTDCT
TurboJPEG-->>Decoder: RGB numpy array
else TurboJPEG Unavailable
Decoder->>Pillow: Image.open(BytesIO(jpeg_bytes))
Pillow-->>Decoder: RGB PIL Image
Decoder->>Decoder: Convert PIL Image to numpy array
end
Decoder-->>Output: Return Optional[np.ndarray]
Client->>Decoder: decode_jpeg_resized(jpeg_bytes, width, height)
Decoder->>Decoder: Compute max_dim and scaling factor
alt TurboJPEG + Downsampling
Decoder->>TurboJPEG: decode with scaling factor
TurboJPEG-->>Decoder: Pre-downsampled RGB array
else Fallback to Pillow + Resize
Decoder->>Pillow: decode and thumbnail
Pillow-->>Decoder: Resized RGB array
end
Decoder-->>Output: Return Optional[np.ndarray]
sequenceDiagram
participant Prefetcher as Prefetcher Class
participant Queue as Task Priority Queue
participant Pool as Thread Pool (Executor)
participant Decoder as _decode_and_cache
participant ICC as ICC Transform Cache
participant Memory as Buffer Cache
Prefetcher->>Prefetcher: set_image_files(paths, current_index)
Prefetcher->>Prefetcher: Initialize generation=0
Prefetcher->>Prefetcher: update_prefetch(current_index, direction)
Prefetcher->>Prefetcher: Compute prefetch_radius around index
Prefetcher->>Prefetcher: Expand radius adaptively after navigation
loop Priority Order (current → nearest)
Prefetcher->>Prefetcher: Determine task priority
Prefetcher->>Queue: Cancel lower-priority stale tasks (old generation)
Prefetcher->>Pool: submit_task(index, generation)
end
Pool->>Decoder: _decode_and_cache(path, generation)
alt ICC Color Management Enabled
Decoder->>ICC: get_icc_transform(src, monitor, key, path)
ICC-->>Decoder: Cached transform or new transform
Decoder->>Decoder: Apply ICC transform in-place
else Standard sRGB Path
Decoder->>Decoder: Decode and apply orientation only
end
Decoder->>Decoder: apply_saturation_compensation(buffer, factor)
Decoder->>Memory: Cache decoded image with generation key
Memory-->>Decoder: Return cache_key
Decoder-->>Prefetcher: DecodedImage object / (Path, generation)
Prefetcher->>Memory: Invalidate cache entries from prior generations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In @.gitattributes:
- Around line 1-2: Remove the undefined "filter=crlf" attribute from the
.gitattributes entry "* text=auto eol=lf filter=crlf" (or alternatively add a
matching filter configuration in git config if you intend to use a custom
filter) so only "text=auto eol=lf" remains; also delete the duplicated "*.tiff
binary" line so there is only one "*.tiff binary" entry in the file.
In `@faststack/imaging/prefetch.py`:
- Around line 449-464: The ICC-fallback path currently memory-maps
image_file.path and always calls JPEG-specific decoders (decode_jpeg_resized /
decode_jpeg_rgb), which will fail for non-JPEG images; change the fallback to
detect the file type (e.g., via Pillow by attempting
PILImage.open(image_file.path) or checking image format) and only use mmap +
decode_jpeg_resized / decode_jpeg_rgb when the format is JPEG, otherwise fall
back to a Pillow-based decode/resize path (open with PILImage.open, convert to
RGB, apply thumbnail/resizing when should_resize/use_resized) and set buffer
from the resulting array, preserving the existing use_resized/should_resize
logic and the buffer variable behavior.
In `@faststack/qml/ExifDialog.qml`:
- Around line 71-72: The font.family assignment in ExifDialog.qml uses an
invalid comma-separated list ("Consolas, monospace"); update the font.family
property in ExifDialog.qml to a single font family name (e.g., "Consolas" to
match Main.qml usage) so it conforms to QML's expected single-family value for
the font.family property.
In `@faststack/test_pil_blur.py`:
- Around line 6-25: The test_blur function currently swallows failures inside a
broad try/except and only prints errors; remove the blanket try/except (or if
you must keep it, re-raise the caught exception) so test failures propagate, and
add an explicit assertion that verifies the blur actually changed the image (for
example assert np.std(blurred) < np.std(data) or use numpy.testing to compare),
referencing variables test_blur, data, img, blurred and result so the test fails
when GaussianBlur breaks.
In `@faststack/tests/manual_test_error_handling.py`:
- Around line 45-72: The test currently expects a PermissionError but
ImageEditor.save_image wraps save exceptions in a RuntimeError; update the
test_save_image_raises() logic to catch RuntimeError instead of PermissionError
and assert the RuntimeError message includes the "Save failed" prefix and the
original error text (e.g., "Mocked save error"); keep the existing patches/mocks
(create_backup_file, PIL.Image.fromarray -> mock_img with
mock_img.save.side_effect) and only change the exception assertion and message
checks to validate RuntimeError coming from ImageEditor.save_image.
- Around line 13-37: The test test_load_image_raises assumes
ImageEditor.load_image will raise, but load_image catches exceptions and returns
False and also short‑circuits if the path doesn't exist; update the test to
patch Path.exists to True (or create a temp file) so the code reaches the
PIL.Image.open call, patch PIL.Image.open to raise OSError, call
ImageEditor.load_image and assert it returns False rather than expecting an
exception, and keep the cv2 patching as in the current diff to avoid external
deps.
In `@faststack/tests/reproduce_exif_bug.py`:
- Around line 22-41: The tests expect _get_sanitized_exif_bytes to return the
original EXIF bytes on failure or when Exif.tobytes is missing; update the
_get_sanitized_exif_bytes implementation to (1) check for the presence of
tobytes (e.g., hasattr or try/except AttributeError) and call it only if
present, and (2) wrap the tobytes call in a broad try/except that catches
Exception and returns the original source EXIF bytes (the variable holding the
original bytes used by the editor) instead of None when serialization fails;
reference _get_sanitized_exif_bytes and the code paths that call
PIL.Image.Exif.tobytes to locate the change.
In `@faststack/tests/test_cache_invalidation.py`:
- Around line 49-66: Replace the manual print-based PASS/FAIL checks with pytest
assertions so failures surface as test failures: assert that hash1 == hash2
(stable across reloads) after calling editor.load_image and
editor._get_upstream_edits_hash on current_edits, and assert that after touching
the file (img_path.touch) and reloading (editor.load_image) the new hash (hash3
from editor._get_upstream_edits_hash) is not equal to hash2; keep the time.sleep
to ensure mtime changes and preserve the existing calls to editor.load_image,
editor._get_upstream_edits_hash, current_edits, img_path.touch and the hash
variables.
In `@faststack/tests/test_exif_orientation.py`:
- Around line 155-160: The test expectation is wrong: because
editor.sanitize_exif_orientation is now always applied to _source_exif_bytes (so
orientation is reset to 1), update the assertion in
test_orientation_preserved_no_rotation to expect Orientation == 1 instead of 6
and remove the unused local variable res from the save step; locate the call
using self.editor.save_image(...) and the EXIF assertion using
exif.get(ExifTags.Base.Orientation) to change the expected value and delete the
unused variable.
In `@faststack/tests/test_highlight_state_normalization.py`:
- Around line 14-42: There are two test methods named
test_highlight_state_normalization_standard which causes the first (the one that
only sets self.mock_editor._last_highlight_state and contains exploratory
comments) to be overridden and never run; remove that incomplete duplicate
method so only the canonical-key test using
self.mock_editor._last_highlight_state = {'headroom_pct': 0.1,
'source_clipped_pct': 0.4, 'current_nearwhite_pct': 0.5} and assertions against
ui_state.highlightState remain, ensuring the single test method name
test_highlight_state_normalization_standard is unique.
In `@faststack/tests/test_highlights_responsiveness.py`:
- Around line 16-43: The test
TestHighlightsResponsiveness::test_highlights_at_various_levels currently only
prints values and cannot fail; replace the prints with real assertions: compute
diffs = vals - out[0,:,0] after calling editor._apply_edits(edits={'highlights':
-1.0} using editor._initial_edits and editor._apply_edits) and assert expected
behavior—e.g., that diffs are zero for indices below the pivot (confirming no
change for low brightness) by checking diffs[:pivot_index].sum() == 0 (or
allclose to 0), and that diffs above some threshold (e.g., for indices >=
pivot_index or a chosen start index) contain at least one value > 0.01 and are
monotonic/non-decreasing if that is the requirement; use
test_highlights_at_various_levels, editor._initial_edits, and
editor._apply_edits to locate the code to change.
In `@faststack/verify_wb.py`:
- Around line 40-41: Replace the hardcoded "test_grey.jpg" file creation with a
tempfile-based approach: use tempfile.NamedTemporaryFile(delete=False) (or
tempfile.TemporaryDirectory and create a file inside it) to obtain a safe
temporary filename, assign that to grey_path, then call
grey_img.save(grey_path); ensure you close the NamedTemporaryFile before saving
and remove the temp file after the test to avoid leaving artifacts. Target the
grey_path variable and the grey_img.save(...) call in verify_wb.py when making
this change.
- Around line 45-46: Update the incorrect explanatory comment that uses 0.25
instead of the actual white_balance_by value (0.5): when white_balance_by = 0.5
compute r_gain = 1.0 + 0.5 = 1.5 -> 128 * 1.5 = 192 and b_gain = 1.0 - 0.5 = 0.5
-> 128 * 0.5 = 64; change the comment near the white_balance_by/r_gain/b_gain
logic in verify_wb.py (referencing the variables white_balance_by, r_gain,
b_gain) to reflect these correct values.
- Around line 13-14: The code uses a hardcoded filename "test_black.jpg" and
calls black_img.save(black_path), which may fail in read-only dirs or under
concurrent runs; replace this with a tempfile-based approach: use
tempfile.NamedTemporaryFile(delete=False, suffix=".jpg") or
tempfile.TemporaryDirectory to create a safe temp path, write the image via
black_img.save(temp_file.name), and ensure the temp file is cleaned up after the
test; update references to black_path accordingly (look for the symbol
black_path and the call black_img.save) and add proper cleanup in the
surrounding test or teardown.
🟡 Minor comments (23)
faststack/tests/test_highlights_responsiveness.py-7-12 (1)
7-12: Scope module mocks to avoid cross‑test leakage.Mocking
sys.modulesat import time affects the entire process and can mask issues in other tests. Preferunittest.mock.patch.dictwithin the test or a fixture to localize the effect.faststack/tests/debug_metadata.py-54-57 (1)
54-57: Avoid broadExceptionand unusede.Lint will flag this. Either catch a narrower exception or use
_and keep traceback.🔧 Proposed fix
- except Exception as e: + except Exception: f.write("Test FAILED\n") import traceback traceback.print_exc(file=f)faststack/tests/test_drag_logic.py-10-10 (1)
10-10: Fix unusedcurrent_indexto avoid Ruff ARG001.Ruff flags this argument as unused. If the signature must match the production logic, rename it to
_current_index; otherwise remove and update callers.🔧 Proposed fix
-def get_drag_paths(image_files, current_index, existing_indices, current_edit_source_mode): +def get_drag_paths(image_files, _current_index, existing_indices, current_edit_source_mode):faststack/tests/mini_test.py-12-40 (1)
12-40: Align test expectations withImageEditor.load_imagereturningFalse(not raising).
load_imageis documented to returnFalseon missing files or load errors, so these tests will print FAIL even when behavior is correct. Adjust to assertFalseinstead of expecting exceptions.✅ Proposed fix
- try: - editor.load_image("non_existent_file.jpg") - print("FAIL 1: No exception raised for missing file") - except FileNotFoundError: - print("PASS 1: Caught FileNotFoundError") - except Exception as e: - print(f"FAIL 1: Unexpected exception: {type(e)} {e}") + try: + result = editor.load_image("non_existent_file.jpg") + if result is False: + print("PASS 1: Returned False for missing file") + else: + print("FAIL 1: Expected False for missing file") + except Exception as e: + print(f"FAIL 1: Unexpected exception: {type(e)} {e}") @@ - try: - editor.load_image(tmp_name) - print("FAIL 2: No exception raised for bad load") - except OSError as e: - if "FAIL_PIL" in str(e): - print("PASS 2: Caught expected OSError") - else: - print(f"FAIL 2: Wrong error: {e}") - except Exception as e: - print(f"FAIL 2: Unexpected exception: {type(e)} {e}") + try: + result = editor.load_image(tmp_name) + if result is False: + print("PASS 2: Returned False for bad load") + else: + print("FAIL 2: Expected False for bad load") + except Exception as e: + print(f"FAIL 2: Unexpected exception: {type(e)} {e}")reproduce_bug.py-37-37 (1)
37-37: Remove unused variablet1.The variable
t1is assigned but never used. Line 38 usesimg_path.touch()which automatically sets the modification time to the current time, making thet1assignment unnecessary.♻️ Proposed fix
- # 2. Save Main (update mtime to T1) - t1 = time.time() - img_path.touch() # Updates mtime + # 2. Save Main (update mtime to current time) + img_path.touch() # Updates mtimeBased on static analysis hints (Ruff F841).
faststack/verify_wb.py-58-64 (1)
58-64: Improve cleanup robustness and error handling.Two issues with the current cleanup approach:
- If an exception occurs before line 58, temporary files won't be cleaned up.
- The bare
except OSError: passsilently swallows all OS errors, which might hide legitimate issues.Consider using a
try-finallyblock or context managers to ensure cleanup always occurs, and be more explicit about which exceptions to ignore (e.g.,FileNotFoundError).🔧 Suggested improvement
def test_white_balance(): + temp_files = [] + try: editor = ImageEditor() # ... test code ... + temp_files = [black_path, grey_path] + finally: + # Cleanup + for path in temp_files: + try: + os.remove(path) + except FileNotFoundError: + pass # Already deletedfaststack/tests/test_executable_validator.py-28-49 (1)
28-49: Resolve Ruff ARG005/RUF059 warnings in mocks and unpacking.Use
__str__.return_value(or_parameter) and rename unusederrorto_errorto satisfy lint.🧹 Suggested fix
- mock_path_instance.__str__ = lambda self: photoshop_path + mock_path_instance.__str__.return_value = photoshop_path @@ - is_valid, error = validate_executable_path( + is_valid, _error = validate_executable_path( @@ - mock_path_instance.__str__ = lambda self: r"C:\Windows\System32\malware.exe" + mock_path_instance.__str__.return_value = r"C:\Windows\System32\malware.exe" @@ - is_valid, error = validate_executable_path(suspicious_path) + is_valid, _error = validate_executable_path(suspicious_path) @@ - mock_path_instance.__str__ = lambda self: wrong_exe + mock_path_instance.__str__.return_value = wrong_exe @@ - is_valid, error = validate_executable_path( + is_valid, _error = validate_executable_path(Also applies to: 51-69, 111-130
faststack/tests/manual_test_error_handling.py-77-86 (1)
77-86: Split one-line conditionals to satisfy Ruff E701.This keeps lint clean and improves readability in the manual runner.
🧹 Suggested tweak
- if not test_load_image_raises(): success = False + if not test_load_image_raises(): + success = False @@ - if not test_save_image_raises(): success = False + if not test_save_image_raises(): + success = Falsefaststack/logging_setup.py-38-42 (1)
38-42: Close existing handlers before clearing to avoid FD leaks.If
setup_loggingis called more than once,root_logger.handlers.clear()drops handlers without closing them, leavingRotatingFileHandleropen. Close/remove old handlers first.🛠️ Proposed fix
- root_logger.handlers.clear() + for handler in list(root_logger.handlers): + root_logger.removeHandler(handler) + handler.close()README.md-17-19 (1)
17-19: Fix merged bullet in the Features list.The "Quick Auto White Balance" and "Photoshop Integration" bullets are concatenated, so Markdown renders as one line.
📝 Proposed fix
-- **Quick Auto White Balance:** Press A to apply auto white balance and save automatically with undo support (Ctrl+Z). For better white balance, load the raw into Photoshop with the P key.- **Photoshop Integration:** Edit current image in Photoshop (P key) - always uses RAW files when available. +- **Quick Auto White Balance:** Press A to apply auto white balance and save automatically with undo support (Ctrl+Z). For better white balance, load the raw into Photoshop with the P key. +- **Photoshop Integration:** Edit current image in Photoshop (P key) - always uses RAW files when available.faststack/io/helicon.py-48-56 (1)
48-56: Guard against launching with an empty temp file.If all RAW files are missing, the temp file is empty but the process still launches. Consider checking that at least one valid file was written and clean up otherwise.
🛠️ Proposed fix
- with tempfile.NamedTemporaryFile("w", delete=False, suffix=".txt", encoding='utf-8') as tmp: + valid_files = [] + with tempfile.NamedTemporaryFile("w", delete=False, suffix=".txt", encoding='utf-8') as tmp: for f in raw_files: # Ensure file path is resolved and exists if not f.exists(): log.warning(f"RAW file does not exist, skipping: {f}") continue - tmp.write(f"{f.resolve()}\n") + resolved = f.resolve() + valid_files.append(resolved) + tmp.write(f"{resolved}\n") tmp_path = Path(tmp.name) + + if not valid_files: + log.warning("No valid RAW files selected for Helicon Focus.") + try: + tmp_path.unlink() + except FileNotFoundError: + pass + return False, Nonefaststack/imaging/metadata.py-85-86 (1)
85-86: Split single-lineifstatements to satisfy Ruff E701.The Ruff linter is configured to enforce the "E7" rule set, which includes E701 (multiple statements on one line). Lines 85-86 violate this rule with the pattern
if condition: assignment.Proposed fix
- if make: make = clean_exif_value(make) - if model: model = clean_exif_value(model) + if make: + make = clean_exif_value(make) + if model: + model = clean_exif_value(model)faststack/tests/test_raw_pipeline.py-31-37 (1)
31-37: Suppress Ruff unused-variable warnings by renaming unused parameters and variables.Rename unused function parameters and unpacked variables with underscore prefix to silence Ruff lint checks.
🛠️ Proposed fix
- def side_effect_start(*args, **kwargs): + def side_effect_start(*_args, **_kwargs): _, thread_kwargs = mock_thread.call_args target = thread_kwargs.get('target') if target: target()Apply the same pattern to lines 72-77 and 116-121. Additionally, rename unused function parameters and unpacked variables on lines 190 and 238-241:
- Line 190:
mock_run→_mock_run- Line 240:
backup_path→_backup_pathdebug_al.py-17-18 (1)
17-18: Prefix unused auto_levels outputs with underscore to avoid lint warnings.The variables
blacks,whites, andp_loware unpacked but never used. Mark them as intentionally unused with underscore prefixes.🔧 Proposed fix
- 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_cache_invalidation.py-69-72 (1)
69-72: Useshutil.rmtree(test_dir, ignore_errors=True)for cleanup instead of bare except.This bare
excepttriggers Ruff E722 and S110. Theignore_errors=Trueparameter is the idiomatic way to handle cleanup where suppressing errors is acceptable.🔧 Proposed fix
- try: - shutil.rmtree(test_dir) - except: - pass + shutil.rmtree(test_dir, ignore_errors=True).gitattributes-25-26 (1)
25-26: Duplicate entry for*.tiff.
*.tiff binaryappears on both line 26 and line 40. Remove the duplicate.🔧 Suggested fix
*.so binary *.dll binary *.dylib binary -*.tiff binary *.nef binaryAlso applies to: 40-40
faststack/imaging/math_utils.py-83-145 (1)
83-145: Return dictionary is missing keys documented in docstring.The docstring (lines 86-88) documents that the return dict includes
clipped_pctandnear_white_pctas aliases, but the actual return dict (lines 141-145) only containsheadroom_pct,source_clipped_pct, andcurrent_nearwhite_pct. This mismatch could causeKeyErrorfor callers expecting the documented aliases.🐛 Proposed fix: Add the documented alias keys
return { 'headroom_pct': headroom_pct, 'source_clipped_pct': source_clipped_pct, 'current_nearwhite_pct': current_nearwhite_pct, + 'clipped_pct': source_clipped_pct, # Alias for backward compatibility + 'near_white_pct': current_nearwhite_pct, # Alias for backward compatibility }faststack/tests/test_auto_levels.py-24-24 (1)
24-24: Fix unused unpacked variables to satisfy Ruff.Ruff flags unused
blacks(Line 24) andblacks/p_low(Line 91). Use underscore placeholders.💡 Suggested change
- 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)Also applies to: 91-91
faststack/tests/test_sensitivity.py-5-6 (1)
5-6: Prepend repo root to avoid importing an installedfaststack.Using
sys.path.append(...)risks resolving to a globally installed package instead of the local repo. Preferinsert(0, ...)to ensure tests exercise this codebase.💡 Suggested change
-sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "..", ".."))) +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..")))faststack/tests/test_highlights_v2.py-44-45 (1)
44-45: Remove unusedsrgbto keep lint clean.
srgbis assigned but never used, which triggers Ruff F841.💡 Suggested change
- # sRGB mock indicating some clipping (e.g. 255) - srgb = np.ones((100, 100, 3), dtype=np.uint8) * 255 + # (sRGB mock not needed here)faststack/tests/test_generation_aware_preview.py-51-52 (1)
51-52: Remove unused variable assignment.The variable
imgis assigned but never used. Per static analysis hint F841, this should be removed.🔧 Proposed fix
# Request with matching generation - img = self.provider.requestImage("0/5", None, None) + self.provider.requestImage("0/5", None, None)faststack/imaging/jpeg.py-165-167 (1)
165-167: Remove redundant check and fix indentation.The check
if width <= 0 or height <= 0at line 166 is unreachable in the Pillow fallback path because the same condition at line 118-119 would have already returned early. Additionally, line 167 has an extra leading space.🔧 Proposed fix
from io import BytesIO img = Image.open(BytesIO(jpeg_bytes)) - - if width <= 0 or height <= 0: - return np.array(img.convert("RGB")) - scale_factor_ratio = min(img.width / width, img.height / height)faststack/imaging/prefetch.py-467-469 (1)
467-469: Remove dead code: orphanedpassstatement.The
passstatement at line 469 appears to be leftover from removed EXIF orientation code. The comment at line 467 references code that was moved elsewhere but thepassremains.🔧 Proposed fix
- # EXIF orientation correction - - pass - # Memory Optimization: Avoid explicit copy
🧹 Nitpick comments (35)
faststack/tests/test_version_sort.py (1)
5-11: Avoid duplicating production logic in tests.
Consider extractingversion_sort_keyinto a small importable utility (e.g.,faststack/versioning.py) and importing it in bothconfig.pyand this test to prevent drift if the logic changes.faststack/tests/mini_test.py (1)
19-19: Avoid blindexcept Exceptionin test harness.These handlers mask unexpected failures; consider re-raising or exiting non‑zero after logging to ensure CI fails when it should.
Also applies to: 39-39, 45-45
faststack/verify_wb.py (3)
25-25: Consider avoiding direct access to private methods in tests.Calling
_apply_edits()directly couples the test to internal implementation details. If ImageEditor provides a public method to retrieve the processed image (e.g.,get_processed_image()orsave()followed by reload), prefer that approach for more robust tests.
30-35: Replace print statements with proper assertions for automated testing.Using print statements and manual inspection prevents integration with automated test frameworks (pytest, unittest). Consider converting to proper assertions that will fail the test on unexpected behavior.
♻️ Example refactor with assertions
- # Check max value - should still be 0 or very close to it - max_val = arr.max() - print(f"Black Image Max Value after WB: {max_val}") - - if max_val > 0: - print("FAIL: Black level not preserved!") - else: - print("PASS: Black level preserved.") + # Check max value - should still be 0 or very close to it + max_val = arr.max() + assert max_val == 0, f"Black level not preserved: max_val={max_val}"
51-56: Replace print statements with proper assertions.Same issue as the black preservation test: use assertions instead of print statements for automated test execution.
♻️ Example refactor with assertions
- r, g, b = arr[0,0] - print(f"Grey Image RGB after Warm shift: R={r}, G={g}, B={b}") - - if r > 128 and b < 128: - print("PASS: Grey shifted warm correctly.") - else: - print("FAIL: Grey did not shift as expected.") + r, g, b = arr[0, 0] + assert r > 128, f"Red channel should increase with warm shift, got R={r}" + assert b < 128, f"Blue channel should decrease with warm shift, got B={b}"faststack/io/watcher.py (1)
72-74: Return a strictboolfromis_alive.
andcan yieldNone; returningbool(...)keeps the signature honest.♻️ Suggested tweak
- return self.observer and self.observer.is_alive() + return bool(self.observer and self.observer.is_alive())faststack/tests/test_new_features.py (2)
166-166: Prefix unused unpacked variables with underscores.The variables
blacksandwhitesare unpacked but never used in this test case. Prefix them with underscores to indicate they are intentionally unused.♻️ 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)
230-230: Prefix unused unpacked variables with underscores.The variables
p_lowandp_highare unpacked but never used in this test case. Prefix them with underscores to indicate they are intentionally unused.♻️ 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/tests/test_pairing.py (1)
55-71: Split statements separated by semicolons into separate lines.Multiple statements on one line using semicolons reduce readability and violate PEP 8 style guidelines. Consider splitting these into separate lines.
♻️ Proposed fix
- jpg_stat = MagicMock(); jpg_stat.st_mtime = 1000.0 + jpg_stat = MagicMock() + jpg_stat.st_mtime = 1000.0 # Case 1: Perfect match - raw1_path = Path("IMG_01.CR3"); raw1_stat = MagicMock(); raw1_stat.st_mtime = 1000.1 + raw1_path = Path("IMG_01.CR3") + raw1_stat = MagicMock() + raw1_stat.st_mtime = 1000.1 potentials = [(raw1_path, raw1_stat)] assert _find_raw_pair(jpg_path, jpg_stat, potentials) == raw1_path # Case 2: No match (time delta too large) - raw2_path = Path("IMG_01.CR3"); raw2_stat = MagicMock(); raw2_stat.st_mtime = 1003.0 + raw2_path = Path("IMG_01.CR3") + raw2_stat = MagicMock() + raw2_stat.st_mtime = 1003.0 potentials = [(raw2_path, raw2_stat)] assert _find_raw_pair(jpg_path, jpg_stat, potentials) is None # Case 3: Closest match is chosen - raw3_path = Path("IMG_01_A.CR3"); raw3_stat = MagicMock(); raw3_stat.st_mtime = 1000.5 - raw4_path = Path("IMG_01_B.CR3"); raw4_stat = MagicMock(); raw4_stat.st_mtime = 1001.8 + raw3_path = Path("IMG_01_A.CR3") + raw3_stat = MagicMock() + raw3_stat.st_mtime = 1000.5 + raw4_path = Path("IMG_01_B.CR3") + raw4_stat = MagicMock() + raw4_stat.st_mtime = 1001.8 potentials = [(raw3_path, raw3_stat), (raw4_path, raw4_stat)] assert _find_raw_pair(jpg_path, jpg_stat, potentials) == raw3_pathfaststack/tests/test_metadata.py (2)
9-11: Remove or prefix unused mock parameter.The
mock_existsparameter is patched but never used in the test. Either remove the patch decorator or prefix the parameter with an underscore to indicate it's intentionally unused.♻️ Proposed fix - Option 1: Remove the unused patch
- `@patch`('pathlib.Path.exists', return_value=True) `@patch`('faststack.imaging.metadata.Image.open') - def test_get_exif_data_success(self, mock_open, mock_exists): + def test_get_exif_data_success(self, mock_open):♻️ Proposed fix - Option 2: Prefix with underscore
`@patch`('pathlib.Path.exists', return_value=True) `@patch`('faststack.imaging.metadata.Image.open') - def test_get_exif_data_success(self, mock_open, mock_exists): + def test_get_exif_data_success(self, mock_open, _mock_exists):
66-69: Use bareraiseto re-raise the exception.When re-raising an exception in an exception handler, use bare
raiseinstead ofraise eto preserve the original traceback.♻️ Proposed fix
except Exception as e: import traceback traceback.print_exc() - raise e + raisefaststack/config.py (1)
173-174: Consider usinglog.exception()for better debugging.Using
log.exception()instead oflog.error()would automatically include the full traceback, which would be helpful for diagnosing config save failures.♻️ Proposed improvement
except IOError as e: - log.error(f"Failed to save config to {self.config_path}: {e}") + log.exception(f"Failed to save config to {self.config_path}: {e}")faststack/io/indexer.py (3)
41-43: Drop the unused exception binding.
Keeps the handler clean and resolves the lint hint.♻️ Proposed fix
- except OSError as e: + except OSError: log.exception("Error scanning directory %s", directory) return []
87-90: Avoid the unused loop variable.
Iterating values directly is clearer.♻️ Proposed fix
- for stem, raw_list in raws.items(): + for raw_list in raws.values(): for raw_path, raw_stat in raw_list: if raw_path not in used_raws:
62-63: Remove the unusedjpg_pathparameter (or use it).
If it’s not needed, drop it to reduce noise.♻️ Proposed fix
- raw_pair = _find_raw_pair(p, stat, raws.get(p.stem, [])) + raw_pair = _find_raw_pair(stat, raws.get(p.stem, [])) -def _find_raw_pair( - jpg_path: Path, - jpg_stat: os.stat_result, - potential_raws: List[Tuple[Path, os.stat_result]] -) -> Path | None: +def _find_raw_pair( + jpg_stat: os.stat_result, + potential_raws: List[Tuple[Path, os.stat_result]] +) -> Path | None:Also applies to: 120-124
faststack/io/sidecar.py (2)
78-81: Consider usinglogging.exceptionfor automatic traceback inclusion.When logging in exception handlers,
log.exception()automatically includes the traceback, which aids debugging.♻️ Suggested improvement
except (json.JSONDecodeError, TypeError) as e: - log.error(f"Failed to load or parse sidecar file {self.path}: {e}") + log.exception(f"Failed to load or parse sidecar file {self.path}: {e}") # Consider backing up the corrupted file here return Sidecar()
108-112: Same suggestion: uselogging.exceptionfor traceback context.♻️ Suggested improvement
except (IOError, TypeError) as e: - log.error(f"Failed to save sidecar file {self.path}: {e}") + log.exception(f"Failed to save sidecar file {self.path}: {e}") finally: if was_watcher_running: self.start_watcher()faststack/tests/test_config_setters.py (2)
95-102: Remove unused variable assignment.The
mock_path_clsvariable is assigned but never used, and the accompanying comments indicate this was exploratory code that wasn't completed.♻️ Suggested cleanup
# Initialize controller - # Mock Path for init argument - mock_path_cls = self.patches[-1].target # access the mock object ? NO, p.start returns mock - # Ideally capture the return of start() - - # Simpler: just instantiate. The mocks are active. - # But we need to pass a mock path to __init__ + # Mocks are active, just instantiate with mock arguments self.controller = AppController(MagicMock(), MagicMock())
69-71: Consider removing or gating debug prints.These debug statements are useful during development but may clutter test output in CI. Consider removing them or gating behind a flag like
if os.getenv('DEBUG_TESTS').faststack/tests/test_prefetch_logic.py (2)
50-51: Consider usingself.fail()instead of raising a generic Exception.Using
self.fail()is more idiomatic for unittest and provides clearer test failure semantics.♻️ Suggested improvement
# Check if task 4 was added if 4 not in prefetcher.futures: - raise Exception("Task 4 was not added to futures!") + self.fail("Task 4 was not added to futures!")
64-67: Debug-style exception handling may obscure test failures.The broad
except Exceptionwithtraceback.print_exc()can obscure the actual failure. Consider removing this wrapper and letting unittest handle exceptions naturally, which provides better integration with test runners.faststack/imaging/math_utils.py (1)
81-81: Use ASCIIxinstead of Unicode multiplication sign.The docstring uses
×(Unicode multiplication sign U+00D7) which can cause issues with some tools and editors. Use lowercasexfor consistency.♻️ Suggested fix
- MUST have same H×W dimensions as rgb_linear (or be stride-compatible). + MUST have same HxW dimensions as rgb_linear (or be stride-compatible).faststack/tests/test_sidecar.py (1)
14-14: Fix implicitOptionaltype hint.Per PEP 484, use explicit
Noneunion syntax instead of implicitOptionalvia default value.Proposed fix
- def _create(content: dict = None): + def _create(content: dict | None = None):faststack/tests/test_editor_lifecycle_and_safety.py (1)
43-45: Remove unused variable from context manager.The
mock_editor_clsvariable is assigned but never used. Use_to indicate intentional discard.Proposed fix
- with patch('faststack.app.ImageEditor') as mock_editor_cls: + with patch('faststack.app.ImageEditor'): self.controller = AppController(Path("."), self.mock_engine) self.mock_editor_instance = self.controller.image_editorrepro_crash.py (2)
35-37: Use bareraiseto preserve traceback.When re-raising the same exception, use
raisewithout the exception name to preserve the original traceback.Proposed fix
except Exception as e: print(f"CRASHED: {e}") - raise e + raise
6-13: Consider test isolation for sys.modules mocking.Global
sys.modulesmocking persists beyond this test and could affect other tests run in the same process. For a standalone reproduction script this is acceptable, but if integrated into the test suite, consider usingpatch.dict(sys.modules, ...)in setUp/tearDown liketest_exif_orientation.pydoes.faststack/tests/test_exif_orientation.py (1)
25-30: Remove unnecessary try/except wrapper.The exception handler immediately re-raises without adding value. If the import fails, the test should fail with the original exception anyway.
Proposed fix
# Import internally to respect the patch - try: - from faststack.imaging.editor import ImageEditor - self.ImageEditorClass = ImageEditor - except ImportError: - # Fallback if path issues persist (shouldn't with sys.path.append) - raise + from faststack.imaging.editor import ImageEditor + self.ImageEditorClass = ImageEditorfaststack/imaging/jpeg.py (2)
42-43: Remove redundant exception object fromlogging.exceptioncalls.
logging.exception()automatically includes the exception traceback, so passingein the message is redundant. This applies to multiple locations in this file.♻️ Suggested fix (example for line 43)
- log.exception(f"PyTurboJPEG failed to decode image: {e}. Trying Pillow.") + log.exception("PyTurboJPEG failed to decode image. Trying Pillow.")Apply similar changes to lines 52, 81, 90, 158, and 180.
Also applies to: 51-52, 80-81, 89-90, 157-158, 179-180
75-79: Consider moving the successful return to anelseblock.Per static analysis hint TRY300, the
return decodedat line 79 could be moved to anelseblock for clarity, though this is a minor style consideration.♻️ Optional refactor
if decoded.shape[0] > max_dim or decoded.shape[1] > max_dim: img = Image.fromarray(decoded) img.thumbnail((max_dim, max_dim), Image.Resampling.LANCZOS) return np.array(img) - return decoded + else: + return decodedfaststack/imaging/prefetch.py (1)
312-646: Consider extracting the_decode_and_cachemethod into smaller functions.The
_decode_and_cachemethod spans ~335 lines with deeply nested conditionals for different decode paths (ICC, saturation, standard). This complexity makes the code harder to maintain and test. Consider extracting the ICC decode path, standard decode path, and post-processing steps into separate helper methods.faststack/ui/provider.py (1)
110-111: Uselog.exceptioninstead oflog.errorfor better diagnostics.When catching exceptions,
log.exceptionautomatically includes the traceback, which is more useful for debugging thanlog.errorwith just the exception message.♻️ Proposed fix
except (ValueError, IndexError) as e: - log.error(f"Invalid image ID requested from QML: {id}. Error: {e}") + log.exception("Invalid image ID requested from QML: %s", id)faststack/tests/test_generation_aware_preview.py (2)
26-31: Mock buffer type may not match production behavior.The mock
bufferattributes are set tobytesobjects (e.g.,b'\x00' * 100), but the realDecodedImageclass usesmemoryview. While QImage can accept both, this inconsistency could mask issues. Consider usingmemoryview(bytearray(100))for more realistic mocking.♻️ Suggested improvement
# Setup mock images self.mock_preview = MagicMock() - self.mock_preview.buffer = b'\x00' * 100 + self.mock_preview.buffer = memoryview(bytearray(100)) self.mock_preview.width = 10 self.mock_preview.height = 10 self.mock_preview.bytes_per_line = 30 self.mock_preview.format = QImage.Format.Format_RGB888 self.mock_decoded = MagicMock() - self.mock_decoded.buffer = b'\xFF' * 100 + self.mock_decoded.buffer = memoryview(bytearray([0xFF] * 100))Also applies to: 33-38
12-13: Consider using standard test discovery instead ofsys.pathmanipulation.Modifying
sys.pathat runtime is fragile. Consider using pytest with proper package installation (pip install -e .) or aconftest.pyto handle imports cleanly.faststack/qml/Main.qml (2)
254-255: Inconsistent hover color usage in menu items.The "Settings..." menu item at line 255 uses an inline hover color expression
(root.isDarkTheme ? "#555555" : "#e0e0e0")instead of thehoverColorproperty defined at line 27. This pattern is repeated in other menu items but not in the "Open Folder..." item (line 237). Consider usinghoverColorconsistently for maintainability.♻️ Example fix for consistency
background: Rectangle { - color: parent.hovered ? (root.isDarkTheme ? "#555555" : "#e0e0e0") : "transparent" + color: parent.hovered ? hoverColor : "transparent" }
17-19: EmptyComponent.onCompletedhandler.The
Component.onCompletedhandler contains only a comment and no actual initialization code. Consider removing it if not needed, or add a TODO if initialization is planned.♻️ Proposed fix
- Component.onCompleted: { - // Initialization complete - }
| t_before_fallback_read = time.perf_counter() | ||
| with open(image_file.path, "rb") as f: | ||
| with mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ) as mmapped: | ||
| # Pass mmap directly - no copy! | ||
| if use_resized and should_resize: | ||
| buffer = decode_jpeg_resized(mmapped, display_width, display_height, fast_dct=fast_dct) | ||
| else: | ||
| # Quality mode or Full Res: decode full image then resize with high quality | ||
| buffer = decode_jpeg_rgb(mmapped, fast_dct=fast_dct) | ||
| if buffer is not None and should_resize: | ||
| img = PILImage.fromarray(buffer) | ||
| img.thumbnail((display_width, display_height), PILImage.Resampling.LANCZOS) | ||
| buffer = np.array(img) | ||
| t_after_fallback_read = time.perf_counter() | ||
| if buffer is None: | ||
| return None |
There was a problem hiding this comment.
ICC fallback path assumes JPEG but uses mmap unconditionally.
When ICC conversion fails, the fallback code at lines 450-461 opens the file with mmap and passes it to decode_jpeg_resized or decode_jpeg_rgb. However, this fallback can be reached for non-JPEG files (since the ICC path handles all image types via Pillow at lines 371-382). Passing a non-JPEG file to these JPEG-specific decoders will fail.
🔧 Proposed fix
except (OSError, ImageCms.PyCMSError, ValueError) as e:
# ICC conversion failed, fall back to standard decode
log.warning("ICC profile conversion failed for %s: %s, falling back to standard decode", image_file.path, e)
+ # Only use TurboJPEG fallback for JPEGs
+ if is_jpeg:
t_before_fallback_read = time.perf_counter()
with open(image_file.path, "rb") as f:
with mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ) as mmapped:
# Pass mmap directly - no copy!
if use_resized and should_resize:
buffer = decode_jpeg_resized(mmapped, display_width, display_height, fast_dct=fast_dct)
else:
# Quality mode or Full Res: decode full image then resize with high quality
buffer = decode_jpeg_rgb(mmapped, fast_dct=fast_dct)
if buffer is not None and should_resize:
img = PILImage.fromarray(buffer)
img.thumbnail((display_width, display_height), PILImage.Resampling.LANCZOS)
buffer = np.array(img)
t_after_fallback_read = time.perf_counter()
+ else:
+ # Non-JPEG: use Pillow
+ t_before_fallback_read = time.perf_counter()
+ with PILImage.open(image_file.path) as img:
+ img = img.convert("RGB")
+ if should_resize:
+ img.thumbnail((display_width, display_height), PILImage.Resampling.LANCZOS)
+ buffer = np.array(img)
+ t_after_fallback_read = time.perf_counter()📝 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.
| t_before_fallback_read = time.perf_counter() | |
| with open(image_file.path, "rb") as f: | |
| with mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ) as mmapped: | |
| # Pass mmap directly - no copy! | |
| if use_resized and should_resize: | |
| buffer = decode_jpeg_resized(mmapped, display_width, display_height, fast_dct=fast_dct) | |
| else: | |
| # Quality mode or Full Res: decode full image then resize with high quality | |
| buffer = decode_jpeg_rgb(mmapped, fast_dct=fast_dct) | |
| if buffer is not None and should_resize: | |
| img = PILImage.fromarray(buffer) | |
| img.thumbnail((display_width, display_height), PILImage.Resampling.LANCZOS) | |
| buffer = np.array(img) | |
| t_after_fallback_read = time.perf_counter() | |
| if buffer is None: | |
| return None | |
| # Only use TurboJPEG fallback for JPEGs | |
| if is_jpeg: | |
| t_before_fallback_read = time.perf_counter() | |
| with open(image_file.path, "rb") as f: | |
| with mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ) as mmapped: | |
| # Pass mmap directly - no copy! | |
| if use_resized and should_resize: | |
| buffer = decode_jpeg_resized(mmapped, display_width, display_height, fast_dct=fast_dct) | |
| else: | |
| # Quality mode or Full Res: decode full image then resize with high quality | |
| buffer = decode_jpeg_rgb(mmapped, fast_dct=fast_dct) | |
| if buffer is not None and should_resize: | |
| img = PILImage.fromarray(buffer) | |
| img.thumbnail((display_width, display_height), PILImage.Resampling.LANCZOS) | |
| buffer = np.array(img) | |
| t_after_fallback_read = time.perf_counter() | |
| else: | |
| # Non-JPEG: use Pillow | |
| t_before_fallback_read = time.perf_counter() | |
| with PILImage.open(image_file.path) as img: | |
| img = img.convert("RGB") | |
| if should_resize: | |
| img.thumbnail((display_width, display_height), PILImage.Resampling.LANCZOS) | |
| buffer = np.array(img) | |
| t_after_fallback_read = time.perf_counter() |
🤖 Prompt for AI Agents
In `@faststack/imaging/prefetch.py` around lines 449 - 464, The ICC-fallback path
currently memory-maps image_file.path and always calls JPEG-specific decoders
(decode_jpeg_resized / decode_jpeg_rgb), which will fail for non-JPEG images;
change the fallback to detect the file type (e.g., via Pillow by attempting
PILImage.open(image_file.path) or checking image format) and only use mmap +
decode_jpeg_resized / decode_jpeg_rgb when the format is JPEG, otherwise fall
back to a Pillow-based decode/resize path (open with PILImage.open, convert to
RGB, apply thumbnail/resizing when should_resize/use_resized) and set buffer
from the resulting array, preserving the existing use_resized/should_resize
logic and the buffer variable behavior.
| font.family: "Consolas, monospace" | ||
| font.pixelSize: 14 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for font.family usage patterns in QML files
rg -n "font\.family\s*:" --type qml -C 2Repository: AlanRockefeller/faststack
Length of output: 1289
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about fonts in the codebase
rg -i "font.*fallback\|fallback.*font" --type qml -C 2
rg -i "qt quick\|qml.*font" --type md -C 2 2>/dev/null | head -20Repository: AlanRockefeller/faststack
Length of output: 51
Use a single font family name instead of a comma-separated list.
The font.family property in QML expects a single font family name. Qt does not support CSS-style comma-separated fallback lists. Change "Consolas, monospace" to either "Consolas" or "monospace" to match the pattern used elsewhere in the codebase (e.g., Main.qml line 795).
🤖 Prompt for AI Agents
In `@faststack/qml/ExifDialog.qml` around lines 71 - 72, The font.family
assignment in ExifDialog.qml uses an invalid comma-separated list ("Consolas,
monospace"); update the font.family property in ExifDialog.qml to a single font
family name (e.g., "Consolas" to match Main.qml usage) so it conforms to QML's
expected single-family value for the font.family property.
| def test_blur(): | ||
| try: | ||
| # Create a dummy float image | ||
| data = np.random.rand(100, 100).astype(np.float32) | ||
| img = Image.fromarray(data, mode='F') | ||
|
|
||
| print("Attempting blur on mode 'F'...") | ||
| start = time.time() | ||
| blurred = img.filter(ImageFilter.GaussianBlur(radius=5)) | ||
| print(f"Blur took {time.time() - start:.4f}s") | ||
|
|
||
| result = np.array(blurred) | ||
| print(f"Result shape: {result.shape}, dtype: {result.dtype}") | ||
|
|
||
| # Check if it actually blurred (simple check: std dev should decrease) | ||
| print(f"Original std: {np.std(data):.4f}") | ||
| print(f"Blurred std: {np.std(result):.4f}") | ||
|
|
||
| except Exception as e: | ||
| print(f"Failed: {e}") |
There was a problem hiding this comment.
Don’t swallow failures in a test.
The broad try/except turns errors into prints, so this test won’t fail even if blur breaks. Remove the blanket catch or re-raise after logging, and add an assertion (e.g., blurred std < original std) so the test is effective.
✅ Proposed fix (fail fast + assert)
def test_blur():
- try:
- # Create a dummy float image
- data = np.random.rand(100, 100).astype(np.float32)
- img = Image.fromarray(data, mode='F')
-
- print("Attempting blur on mode 'F'...")
- start = time.time()
- blurred = img.filter(ImageFilter.GaussianBlur(radius=5))
- print(f"Blur took {time.time() - start:.4f}s")
-
- result = np.array(blurred)
- print(f"Result shape: {result.shape}, dtype: {result.dtype}")
-
- # Check if it actually blurred (simple check: std dev should decrease)
- print(f"Original std: {np.std(data):.4f}")
- print(f"Blurred std: {np.std(result):.4f}")
-
- except Exception as e:
- print(f"Failed: {e}")
+ # Create a dummy float image
+ data = np.random.rand(100, 100).astype(np.float32)
+ img = Image.fromarray(data, mode='F')
+
+ print("Attempting blur on mode 'F'...")
+ start = time.time()
+ blurred = img.filter(ImageFilter.GaussianBlur(radius=5))
+ print(f"Blur took {time.time() - start:.4f}s")
+
+ result = np.array(blurred)
+ print(f"Result shape: {result.shape}, dtype: {result.dtype}")
+
+ # Check if it actually blurred (simple check: std dev should decrease)
+ original_std = np.std(data)
+ blurred_std = np.std(result)
+ print(f"Original std: {original_std:.4f}")
+ print(f"Blurred std: {blurred_std:.4f}")
+ assert blurred_std < original_std🧰 Tools
🪛 Ruff (0.14.14)
24-24: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@faststack/test_pil_blur.py` around lines 6 - 25, The test_blur function
currently swallows failures inside a broad try/except and only prints errors;
remove the blanket try/except (or if you must keep it, re-raise the caught
exception) so test failures propagate, and add an explicit assertion that
verifies the blur actually changed the image (for example assert np.std(blurred)
< np.std(data) or use numpy.testing to compare), referencing variables
test_blur, data, img, blurred and result so the test fails when GaussianBlur
breaks.
| def test_load_image_raises(): | ||
| print("Running test_load_image_raises...") | ||
| try: | ||
| from faststack.imaging.editor import ImageEditor | ||
| editor = ImageEditor() | ||
|
|
||
| # Patch Image.open to raise an exception | ||
| with patch('PIL.Image.open', side_effect=OSError("Mocked file error")): | ||
| with patch.dict(sys.modules, {'cv2': MagicMock()}): | ||
| sys.modules['cv2'].imread.return_value = None | ||
|
|
||
| try: | ||
| editor.load_image("non_existent_file.jpg") | ||
| print("FAILURE: load_image did NOT raise exception") | ||
| return False | ||
| except OSError as e: | ||
| if "Mocked file error" in str(e): | ||
| print("SUCCESS: load_image raised expected exception") | ||
| return True | ||
| else: | ||
| print(f"FAILURE: load_image raised wrong exception: {e}") | ||
| return False | ||
| except Exception as e: | ||
| print(f"FAILURE: load_image raised unexpected exception type: {type(e)} {e}") | ||
| return False |
There was a problem hiding this comment.
Align test_load_image_raises with load_image’s return‑False contract.
ImageEditor.load_image catches exceptions and returns False, and it short‑circuits when the file doesn’t exist, so this test won’t see the mocked Image.open error. Patch Path.exists (or use a temp file) and assert the False return instead of expecting OSError.
🐛 Suggested fix
- with patch('PIL.Image.open', side_effect=OSError("Mocked file error")):
- with patch.dict(sys.modules, {'cv2': MagicMock()}):
- sys.modules['cv2'].imread.return_value = None
-
- try:
- editor.load_image("non_existent_file.jpg")
- print("FAILURE: load_image did NOT raise exception")
- return False
- except OSError as e:
- if "Mocked file error" in str(e):
- print("SUCCESS: load_image raised expected exception")
- return True
- else:
- print(f"FAILURE: load_image raised wrong exception: {e}")
- return False
- except Exception as e:
- print(f"FAILURE: load_image raised unexpected exception type: {type(e)} {e}")
- return False
+ with patch('PIL.Image.open', side_effect=OSError("Mocked file error")):
+ with patch('faststack.imaging.editor.cv2', MagicMock()) as mock_cv2:
+ mock_cv2.imread.return_value = None
+ # Bypass the early "file not found" return to hit the error path
+ with patch('faststack.imaging.editor.Path.exists', return_value=True):
+ try:
+ result = editor.load_image("non_existent_file.jpg")
+ if result is False:
+ print("SUCCESS: load_image returned False on error")
+ return True
+ print("FAILURE: load_image unexpectedly succeeded")
+ return False
+ except Exception as e:
+ print(f"FAILURE: load_image raised unexpected exception: {type(e)} {e}")
+ return False🧰 Tools
🪛 Ruff (0.14.14)
27-27: Consider moving this statement to an else block
(TRY300)
35-35: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@faststack/tests/manual_test_error_handling.py` around lines 13 - 37, The test
test_load_image_raises assumes ImageEditor.load_image will raise, but load_image
catches exceptions and returns False and also short‑circuits if the path doesn't
exist; update the test to patch Path.exists to True (or create a temp file) so
the code reaches the PIL.Image.open call, patch PIL.Image.open to raise OSError,
call ImageEditor.load_image and assert it returns False rather than expecting an
exception, and keep the cv2 patching as in the current diff to avoid external
deps.
| def test_highlight_state_normalization_standard(self): | ||
| """Test with standard keys.""" | ||
| self.mock_editor._last_highlight_state = { | ||
| 'headroom_pct': 0.1, | ||
| 'clipped_pct': 0.2, | ||
| 'near_white_pct': 0.3 | ||
| } | ||
| # Controller returns canonical keys using the passed dict (even if they were wrong in backend, provider normalizes? | ||
| # NO, provider simply gets what is in the dict. | ||
| # Wait, provider logic: | ||
| # return { | ||
| # 'headroom_pct': state.get('headroom_pct', 0.0), | ||
| # 'source_clipped_pct': state.get('source_clipped_pct', 0.0), | ||
| # 'current_nearwhite_pct': state.get('current_nearwhite_pct', 0.0) | ||
| # } | ||
| # So if backend has OLD keys, provider will return 0.0 for new keys! | ||
| # This confirms that backend MUST populate new keys. | ||
|
|
||
| def test_highlight_state_normalization_standard(self): | ||
| """Test with canonical keys present.""" | ||
| self.mock_editor._last_highlight_state = { | ||
| 'headroom_pct': 0.1, | ||
| 'source_clipped_pct': 0.4, | ||
| 'current_nearwhite_pct': 0.5 | ||
| } | ||
| state = self.ui_state.highlightState | ||
| self.assertEqual(state['headroom_pct'], 0.1) | ||
| self.assertEqual(state['source_clipped_pct'], 0.4) | ||
| self.assertEqual(state['current_nearwhite_pct'], 0.5) |
There was a problem hiding this comment.
Duplicate method name causes first test to be silently overridden.
There are two methods named test_highlight_state_normalization_standard (lines 14 and 32). Python will silently override the first with the second, meaning the first test case (lines 14-30) will never execute. The first method also lacks assertions and appears to contain exploratory comments.
🐛 Proposed fix: Remove the incomplete duplicate method
class TestUIStateNormalization(unittest.TestCase):
def setUp(self):
# Mock app_controller and image_editor
self.mock_controller = MagicMock()
self.mock_editor = MagicMock()
self.mock_controller.image_editor = self.mock_editor
self.ui_state = UIState(self.mock_controller)
- def test_highlight_state_normalization_standard(self):
- """Test with standard keys."""
- self.mock_editor._last_highlight_state = {
- 'headroom_pct': 0.1,
- 'clipped_pct': 0.2,
- 'near_white_pct': 0.3
- }
- # Controller returns canonical keys using the passed dict (even if they were wrong in backend, provider normalizes?
- # NO, provider simply gets what is in the dict.
- # Wait, provider logic:
- # return {
- # 'headroom_pct': state.get('headroom_pct', 0.0),
- # 'source_clipped_pct': state.get('source_clipped_pct', 0.0),
- # 'current_nearwhite_pct': state.get('current_nearwhite_pct', 0.0)
- # }
- # So if backend has OLD keys, provider will return 0.0 for new keys!
- # This confirms that backend MUST populate new keys.
-
def test_highlight_state_normalization_standard(self):
"""Test with canonical keys present."""📝 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_highlight_state_normalization_standard(self): | |
| """Test with standard keys.""" | |
| self.mock_editor._last_highlight_state = { | |
| 'headroom_pct': 0.1, | |
| 'clipped_pct': 0.2, | |
| 'near_white_pct': 0.3 | |
| } | |
| # Controller returns canonical keys using the passed dict (even if they were wrong in backend, provider normalizes? | |
| # NO, provider simply gets what is in the dict. | |
| # Wait, provider logic: | |
| # return { | |
| # 'headroom_pct': state.get('headroom_pct', 0.0), | |
| # 'source_clipped_pct': state.get('source_clipped_pct', 0.0), | |
| # 'current_nearwhite_pct': state.get('current_nearwhite_pct', 0.0) | |
| # } | |
| # So if backend has OLD keys, provider will return 0.0 for new keys! | |
| # This confirms that backend MUST populate new keys. | |
| def test_highlight_state_normalization_standard(self): | |
| """Test with canonical keys present.""" | |
| self.mock_editor._last_highlight_state = { | |
| 'headroom_pct': 0.1, | |
| 'source_clipped_pct': 0.4, | |
| 'current_nearwhite_pct': 0.5 | |
| } | |
| state = self.ui_state.highlightState | |
| self.assertEqual(state['headroom_pct'], 0.1) | |
| self.assertEqual(state['source_clipped_pct'], 0.4) | |
| self.assertEqual(state['current_nearwhite_pct'], 0.5) | |
| def setUp(self): | |
| # Mock app_controller and image_editor | |
| self.mock_controller = MagicMock() | |
| self.mock_editor = MagicMock() | |
| self.mock_controller.image_editor = self.mock_editor | |
| self.ui_state = UIState(self.mock_controller) | |
| def test_highlight_state_normalization_standard(self): | |
| """Test with canonical keys present.""" | |
| self.mock_editor._last_highlight_state = { | |
| 'headroom_pct': 0.1, | |
| 'source_clipped_pct': 0.4, | |
| 'current_nearwhite_pct': 0.5 | |
| } | |
| state = self.ui_state.highlightState | |
| self.assertEqual(state['headroom_pct'], 0.1) | |
| self.assertEqual(state['source_clipped_pct'], 0.4) | |
| self.assertEqual(state['current_nearwhite_pct'], 0.5) |
🧰 Tools
🪛 Ruff (0.14.14)
32-32: Redefinition of unused test_highlight_state_normalization_standard from line 14: test_highlight_state_normalization_standard redefined here
(F811)
🤖 Prompt for AI Agents
In `@faststack/tests/test_highlight_state_normalization.py` around lines 14 - 42,
There are two test methods named test_highlight_state_normalization_standard
which causes the first (the one that only sets
self.mock_editor._last_highlight_state and contains exploratory comments) to be
overridden and never run; remove that incomplete duplicate method so only the
canonical-key test using self.mock_editor._last_highlight_state =
{'headroom_pct': 0.1, 'source_clipped_pct': 0.4, 'current_nearwhite_pct': 0.5}
and assertions against ui_state.highlightState remain, ensuring the single test
method name test_highlight_state_normalization_standard is unique.
| class TestHighlightsResponsiveness(unittest.TestCase): | ||
| def test_highlights_at_various_levels(self): | ||
| """Test how much highlights recovery affects various brightness levels.""" | ||
| editor = ImageEditor() | ||
|
|
||
| # Create a gradient from 0.0 to 1.0 (linear) | ||
| # 0.5 linear is about 186/255 in sRGB | ||
| # 0.25 linear is about 137/255 in sRGB | ||
| steps = 11 | ||
| vals = np.linspace(0.0, 1.0, steps, dtype=np.float32) | ||
| linear = np.stack([vals]*3, axis=-1).reshape(1, steps, 3) | ||
|
|
||
| # Apply edits with highlights at -1.0 (max recovery) | ||
| edits = editor._initial_edits() | ||
| edits['highlights'] = -1.0 | ||
|
|
||
| out = editor._apply_edits(linear.copy(), edits=edits, for_export=True) | ||
|
|
||
| print("\nBrightness Levels (Linear 0.0 -> 1.0):") | ||
| print("Input -> Output (Diff)") | ||
| for i in range(steps): | ||
| inp = vals[i] | ||
| outp = out[0, i, 0] | ||
| diff = inp - outp | ||
| print(f"{inp:0.2f} -> {outp:0.4f} ({diff:0.4f})") | ||
|
|
||
| # The goal is to see significant changes (diff > 0.01) starting from lower levels | ||
| # Currently, with pivot 0.75, values below 0.75 should be unchanged (diff=0) |
There was a problem hiding this comment.
Add real assertions—test currently can’t fail.
The test only prints values; without assertions it provides no signal and will always pass. Add explicit expectations (e.g., monotonicity or minimum diff thresholds for certain ranges).
🤖 Prompt for AI Agents
In `@faststack/tests/test_highlights_responsiveness.py` around lines 16 - 43, The
test TestHighlightsResponsiveness::test_highlights_at_various_levels currently
only prints values and cannot fail; replace the prints with real assertions:
compute diffs = vals - out[0,:,0] after calling
editor._apply_edits(edits={'highlights': -1.0} using editor._initial_edits and
editor._apply_edits) and assert expected behavior—e.g., that diffs are zero for
indices below the pivot (confirming no change for low brightness) by checking
diffs[:pivot_index].sum() == 0 (or allclose to 0), and that diffs above some
threshold (e.g., for indices >= pivot_index or a chosen start index) contain at
least one value > 0.01 and are monotonic/non-decreasing if that is the
requirement; use test_highlights_at_various_levels, editor._initial_edits, and
editor._apply_edits to locate the code to change.
| black_path = "test_black.jpg" | ||
| black_img.save(black_path) |
There was a problem hiding this comment.
Use the tempfile module for creating temporary test files.
Creating temporary files in the current working directory can cause issues if the script runs in a read-only location or if multiple instances execute concurrently. Consider using tempfile.NamedTemporaryFile() or tempfile.mkdtemp() for safer temporary file handling.
🔧 Suggested refactor using tempfile
+import tempfile
+
def test_white_balance():
editor = ImageEditor()
# 1. Test Black Preservation
# Create a purely black image
black_img = Image.new('RGB', (100, 100), (0, 0, 0))
- black_path = "test_black.jpg"
+ with tempfile.NamedTemporaryFile(suffix='.jpg', delete=False) as f:
+ black_path = f.name
black_img.save(black_path)📝 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.
| black_path = "test_black.jpg" | |
| black_img.save(black_path) | |
| with tempfile.NamedTemporaryFile(suffix='.jpg', delete=False) as f: | |
| black_path = f.name | |
| black_img.save(black_path) |
🤖 Prompt for AI Agents
In `@faststack/verify_wb.py` around lines 13 - 14, The code uses a hardcoded
filename "test_black.jpg" and calls black_img.save(black_path), which may fail
in read-only dirs or under concurrent runs; replace this with a tempfile-based
approach: use tempfile.NamedTemporaryFile(delete=False, suffix=".jpg") or
tempfile.TemporaryDirectory to create a safe temp path, write the image via
black_img.save(temp_file.name), and ensure the temp file is cleaned up after the
test; update references to black_path accordingly (look for the symbol
black_path and the call black_img.save) and add proper cleanup in the
surrounding test or teardown.
| grey_path = "test_grey.jpg" | ||
| grey_img.save(grey_path) |
There was a problem hiding this comment.
Use the tempfile module for creating temporary test files.
Same issue as with the black test image: creating temporary files in the current working directory can cause problems. Apply the same tempfile-based solution here.
🤖 Prompt for AI Agents
In `@faststack/verify_wb.py` around lines 40 - 41, Replace the hardcoded
"test_grey.jpg" file creation with a tempfile-based approach: use
tempfile.NamedTemporaryFile(delete=False) (or tempfile.TemporaryDirectory and
create a file inside it) to obtain a safe temporary filename, assign that to
grey_path, then call grey_img.save(grey_path); ensure you close the
NamedTemporaryFile before saving and remove the temp file after the test to
avoid leaving artifacts. Target the grey_path variable and the
grey_img.save(...) call in verify_wb.py when making this change.
| # r_gain = 1 + 0.25 = 1.25 -> 128 * 1.25 = 160 | ||
| # b_gain = 1 - 0.25 = 0.75 -> 128 * 0.75 = 96 |
There was a problem hiding this comment.
Fix incorrect calculation in comment.
The comment incorrectly calculates the gains. With white_balance_by = 0.5, the actual gains should be:
r_gain = 1.0 + 0.5 = 1.5→128 * 1.5 = 192b_gain = 1.0 - 0.5 = 0.5→128 * 0.5 = 64
The comment states 0.25 instead of 0.5, leading to incorrect expected values.
📝 Proposed fix for the comment
editor.load_image(grey_path)
editor.set_edit_param('white_balance_by', 0.5) # Warm
- # r_gain = 1 + 0.25 = 1.25 -> 128 * 1.25 = 160
- # b_gain = 1 - 0.25 = 0.75 -> 128 * 0.75 = 96
+ # r_gain = 1.0 + 0.5 = 1.5 -> 128 * 1.5 = 192
+ # b_gain = 1.0 - 0.5 = 0.5 -> 128 * 0.5 = 64Based on learnings from the relevant code snippets showing the white balance gain formulas.
📝 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.
| # r_gain = 1 + 0.25 = 1.25 -> 128 * 1.25 = 160 | |
| # b_gain = 1 - 0.25 = 0.75 -> 128 * 0.75 = 96 | |
| # r_gain = 1.0 + 0.5 = 1.5 -> 128 * 1.5 = 192 | |
| # b_gain = 1.0 - 0.5 = 0.5 -> 128 * 0.5 = 64 |
🤖 Prompt for AI Agents
In `@faststack/verify_wb.py` around lines 45 - 46, Update the incorrect
explanatory comment that uses 0.25 instead of the actual white_balance_by value
(0.5): when white_balance_by = 0.5 compute r_gain = 1.0 + 0.5 = 1.5 -> 128 * 1.5
= 192 and b_gain = 1.0 - 0.5 = 0.5 -> 128 * 0.5 = 64; change the comment near
the white_balance_by/r_gain/b_gain logic in verify_wb.py (referencing the
variables white_balance_by, r_gain, b_gain) to reflect these correct values.
1 participant
Fix bugs found by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.