New features and bug fixes#21
Conversation
… many images as you delete
WalkthroughVersion 1.4 release adds TurboJPEG-based image caching acceleration, RGB histogram visualization (H key), batch image deletion with confirmation, auto-levels functionality with configurable threshold and strength, texture enhancement, improved white balance using multiplicative gains, straightening angle support, and EXIF metadata extraction and display (I key). New configuration options control speed-versus-quality optimization and auto-levels behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as QML UI
participant Controller as Python Controller
participant ImageEditor as ImageEditor
participant Cache as Image Cache
participant Decoder as JPEG Decoder
rect rgb(200, 220, 255)
Note over User,Decoder: Auto-Levels Flow
User->>UI: Press L (auto_levels) or click button
UI->>Controller: quick_auto_levels()
Controller->>ImageEditor: auto_levels(threshold=0.1)
ImageEditor->>ImageEditor: compute blacks/whites from histogram
ImageEditor->>Controller: return (blacks, whites)
Controller->>UI: emit autoLevelClippingThresholdChanged
UI->>UI: update histogram display with new values
end
rect rgb(220, 200, 255)
Note over User,Decoder: EXIF Display Flow (I Key)
User->>UI: Press I (show EXIF)
UI->>Controller: show_exif_dialog() or toggle
Controller->>ImageEditor: get_exif_data(image_path)
ImageEditor->>Controller: return {summary, full}
Controller->>UI: emit populateExifDialog(data)
UI->>UI: openExifDialog with summary/full data
User->>UI: Click "Show All" button
UI->>UI: toggle showFull, refresh display text
end
rect rgb(255, 220, 200)
Note over User,Decoder: Batch Delete Confirmation Flow
User->>UI: select multiple images, press delete
UI->>Controller: on delete action triggered
Controller->>UI: show_delete_batch_dialog(batch_count)
UI->>UI: open DeleteBatchDialog(count=N)
User->>UI: click "Delete All" button
UI->>Controller: delete_batch_images()
Controller->>ImageEditor: apply to each selected image
Controller->>UI: update status/refresh UI
end
rect rgb(200, 255, 220)
Note over User,Decoder: JPEG Decode with TurboJPEG Path
Controller->>Cache: request decoded image for path
Cache->>Decoder: decode_jpeg_resized(jpeg_bytes, width, height, fast_dct=True)
alt TurboJPEG Available
Decoder->>Decoder: compute scaling_factor from dimensions
Decoder->>Decoder: decode via TurboJPEG with fast_dct flag
Decoder->>Cache: return decoded RGB array (fast path)
else TurboJPEG Unavailable or Fails
Decoder->>Decoder: fallback to Pillow resize (BILINEAR)
Decoder->>Cache: return decoded RGB array (fallback path)
end
Cache->>Cache: cache result with build_cache_key(path, generation)
Cache->>Controller: return image
Controller->>UI: update displayed image
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (34)
faststack/faststack/models.py (1)
41-41: Optional: Consider a more specific type hint for theformatfield (if applicable).The
formatfield is typed asobjectwith a comment noting it should beQImage.Format. If PyQt imports are available, consider a more specific type hint (e.g.,Optional[QImage.Format]) to improve IDE support and type checking. If this is intentionally loose to avoid hard dependencies, the current approach is fine.faststack/faststack/io/helicon.py (2)
63-72: Safe argument parsing with shlex; minor logging redundancy.The use of
shlex.splitwith OS-awareposixparameter is a solid security practice for handling user-provided arguments. However, on Line 71,logging.exception()automatically includes the exception traceback, making the{e}parameter redundant.- log.exception(f"Invalid helicon args format: {e}") + log.exception("Invalid helicon args format")Alternatively, if you need the exception message for clarity: use
log.errorwith explicit exception info instead.
87-92: Exception handling covers appropriate cases; clean up redundant logging parameters.The exception handling properly catches and logs errors, returning
(False, None)in all failure paths. However, Lines 88 and 91 include redundant exception parameters inlogging.exception()calls—the framework automatically includes exception info.- log.exception(f"Failed to launch Helicon Focus: {e}") + log.exception("Failed to launch Helicon Focus")- log.exception(f"Failed to create temporary file for Helicon Focus: {e}") + log.exception("Failed to create temporary file for Helicon Focus")faststack/faststack/tests/test_sidecar.py (1)
14-14: Use explicitOptionalor| Nonesyntax for type hints.Line 14 has an implicit
Optional(parameter with defaultNonebut no type annotation for theNonecase). Per PEP 484, convert the type hint to explicitly indicate the optional nature:- def _create(content: dict = None): + def _create(content: dict | None = None):Verify that the project targets Python 3.10+ for the
|union syntax. If targeting earlier versions, useOptional[dict]instead.faststack/faststack/tests/test_executable_validator.py (1)
1-131: Formatting changes only—no functional review needed.This file contains only whitespace/formatting adjustments with no logic changes.
Static analysis identified some minor cleanup opportunities (pre-existing, not introduced by this PR):
- Lines 40, 62, 123: Lambda functions don't use
selfparameter. Consider removing or prefixing with_.- Lines 66, 127: Unpacked
errorvariable is unused. Consider using_instead:is_valid, _ = validate_executable_path(...)These are low-priority style improvements.
faststack/faststack/io/indexer.py (1)
1-84: Formatting changes only—no functional review needed.This file contains only whitespace/formatting adjustments with no logic changes.
Static analysis identified minor cleanup opportunities (pre-existing, not introduced by this PR):
- Line 39: Exception variable
eis captured but never used. Sincelog.exception()automatically includes exception details, you can use bareexcept OSError:instead.- Line 65: Function parameter
jpg_pathis declared but never used in_find_raw_pair(). Consider removing it if truly unnecessary.These are low-priority style improvements.
faststack/faststack/AGENTS.md (1)
3-19: Repository guidelines doc looks solidThe structure, workflow, and security sections are coherent and match the layout implied by the code. As a small optional tweak, you could later add a note that once installed via the package metadata, users can also launch via the configured
faststackconsole script, not justpython -m faststack.app.faststack/faststack/tests/check_turbo.py (1)
2-11: Guard diagnostics to avoid side effects on importRight now, importing this module will immediately run the diagnostic and print to stdout. If this is meant as a standalone check script, consider wrapping the logic in a
main()function and guarding it:-try: - import turbojpeg - print("turbojpeg module found") - print(f"Dir: {dir(turbojpeg)}") - if hasattr(turbojpeg, 'TJFLAG_FASTDCT'): - print(f"TJFLAG_FASTDCT: {turbojpeg.TJFLAG_FASTDCT}") - else: - print("TJFLAG_FASTDCT not found in module") -except ImportError: - print("turbojpeg module not found") +def main() -> None: + try: + import turbojpeg + print("turbojpeg module found") + print(f"Dir: {dir(turbojpeg)}") + if hasattr(turbojpeg, "TJFLAG_FASTDCT"): + print(f"TJFLAG_FASTDCT: {turbojpeg.TJFLAG_FASTDCT}") + else: + print("TJFLAG_FASTDCT not found in module") + except ImportError: + print("turbojpeg module not found") + + +if __name__ == "__main__": + main()faststack/faststack/io/sidecar.py (2)
28-60: Log full traceback when sidecar load failsSince you already treat parse failures as “start fresh”, capturing the traceback will help diagnose corrupted/old sidecar issues without changing behavior. You can switch to
logging.exception:- except (json.JSONDecodeError, TypeError) as e: - log.error(f"Failed to load or parse sidecar file {self.path}: {e}") + except (json.JSONDecodeError, TypeError): + log.exception("Failed to load or parse sidecar file %s", self.path)
61-90: Likewise, uselogging.exceptionon save errorsFor symmetry with load and better observability, consider logging the full traceback when persisting fails:
- except (IOError, TypeError) as e: - log.error(f"Failed to save sidecar file {self.path}: {e}") + except (IOError, TypeError): + log.exception("Failed to save sidecar file %s", self.path)faststack/faststack/tests/test_pairing.py (1)
55-71: Split multiple statements per line for clarityLines 55, 58, 63, 68, and 69 chain assignments with
;, which is harder to read and is typically flagged by linters. Consider one statement per line:- jpg_stat = MagicMock(); jpg_stat.st_mtime = 1000.0 + jpg_stat = MagicMock() + jpg_stat.st_mtime = 1000.0 @@ - 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 @@ - 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 @@ - raw3_path = Path("IMG_01_A.CR3"); raw3_stat = MagicMock(); raw3_stat.st_mtime = 1000.5 + 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 + raw4_path = Path("IMG_01_B.CR3") + raw4_stat = MagicMock() + raw4_stat.st_mtime = 1001.8faststack/README.md (2)
3-3: Optional: Consider adding comma after year for clarity.Some style guides recommend a comma after the year in month-day-year format: "December 1, 2025," for improved readability.
16-16: Optional: Consider hyphenating compound adjective."high-performance decoding" (with hyphen) follows conventional style for compound adjectives modifying a noun.
faststack/faststack/debug_output.txt (1)
1-13: Consider excluding debug output from version control.This file appears to be a generated test artifact. Debug output files like this should typically be excluded from version control via
.gitignoreto prevent test artifacts from polluting the repository.faststack/faststack/config.py (1)
82-82: Uselogging.exceptionfor better error diagnostics.Replace
log.errorwithlog.exceptionto automatically include the traceback, which aids debugging when configuration saves fail.- 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/faststack/tests/debug_metadata.py (1)
54-54: Optional: Remove unused exception variable.The exception variable
eis captured but never used. You can simplify toexcept Exception:since the traceback is printed viatraceback.print_exc.- except Exception as e: + except Exception: f.write("Test FAILED\n") import traceback traceback.print_exc(file=f)faststack/faststack/verify_wb.py (1)
7-65: Consider converting to a proper unittest and improving test assertions.This test file uses
print()statements for pass/fail reporting instead of proper test assertions, which means failures won't be caught by test runners. Additionally, the file is located infaststack/faststack/rather thanfaststack/faststack/tests/where other tests reside.Consider refactoring to use
unittestlike other test files in this PR:+import unittest import numpy as np from PIL import Image from faststack.imaging.editor import ImageEditor import os -def test_white_balance(): +class TestWhiteBalance(unittest.TestCase): + def setUp(self): + self.editor = ImageEditor() + self.black_path = "test_black.jpg" + self.grey_path = "test_grey.jpg" + + def tearDown(self): + for path in [self.black_path, self.grey_path]: + if os.path.exists(path): + os.remove(path) + + def test_black_preservation(self): + black_img = Image.new('RGB', (100, 100), (0, 0, 0)) + black_img.save(self.black_path) + self.editor.load_image(self.black_path) + self.editor.set_edit_param('white_balance_by', 1.0) + self.editor.set_edit_param('white_balance_mg', 1.0) + processed_img = self.editor._apply_edits(self.editor.original_image.copy()) + arr = np.array(processed_img) + self.assertEqual(arr.max(), 0, "Black level not preserved")faststack/faststack/tests/test_prefetch_logic.py (2)
9-64: Remove redundant try/except wrapper; unittest handles exceptions.The
try/exceptblock that prints traceback and re-raises is unnecessary sinceunittestalready captures exceptions and displays full tracebacks on test failures. The print statements also clutter test output.def test_submit_task_priority_cancellation(self): - try: - # Mock dependencies - mock_cache_put = MagicMock() - ... - print("Test passed!") - except Exception: - import traceback - traceback.print_exc() - raise + # Mock dependencies + mock_cache_put = MagicMock() + mock_get_display_info = MagicMock(return_value=(100, 100, 1)) + + # ... rest of test without try/except wrapper ... + + # Assertions (no print statements needed) + self.assertIn(4, prefetcher.futures, "Task 4 was not added to futures") + f0.cancel.assert_called() + f5.cancel.assert_not_called()
46-48: Useself.assertIninstead of raising a generic Exception.Replace the manual check with a proper unittest assertion for better error messages and consistency.
- # Check if task 4 was added - if 4 not in prefetcher.futures: - raise Exception("Task 4 was not added to futures!") + # Check if task 4 was added + self.assertIn(4, prefetcher.futures, "Task 4 was not added to futures")faststack/faststack/tests/test_metadata.py (1)
66-69: Use bareraiseto preserve the original traceback.Using
raise einstead ofraiseresets the traceback to this line, losing the original context. As flagged by Ruff TRY201.except Exception as e: import traceback traceback.print_exc() - raise e + raiseOr better yet, remove the try/except entirely since unittest captures exceptions and displays tracebacks automatically (similar to the pattern in
test_prefetch_logic.py).faststack/faststack/tests/benchmark_decode_bilinear.py (2)
1-6: Remove leading blank line.Line 1 has an unnecessary blank line at the start of the file. This is a minor style issue.
- import time
8-51: Consider extracting resampling mode as a parameter to avoid code duplication.This function largely duplicates
decode_jpeg_resizedfromjpeg.py, differing only in the resampling method (BILINEAR vs LANCZOS). Consider adding aresamplingparameter to the existing function injpeg.pyrather than maintaining two nearly identical implementations.Additionally, the exception handling uses
print()while the mainjpeg.pyuseslog.exception(). For consistency, consider using logging here as well, or since this is a benchmark script, the print statements may be acceptable for quick debugging.faststack/faststack/tests/test_new_features.py (2)
1-1: Remove unused import.The
sysmodule is imported but never used in this file.-import sys import unittest
10-11: Redundant type conversion.The
astype(np.uint8)call at the end is redundant sincenp.arange(256, dtype=np.uint8)already produces a uint8 array, andnp.tilepreserves the dtype.- self.img = Image.fromarray(np.tile(np.arange(256, dtype=np.uint8), (10, 1)).astype(np.uint8)) + self.img = Image.fromarray(np.tile(np.arange(256, dtype=np.uint8), (10, 1)))faststack/faststack/qml/SettingsDialog.qml (2)
1-3: Inconsistent import versioning.Line 1 uses an unversioned import (
import QtQuick) while lines 2-3 use versioned imports. For consistency, consider using the same style throughout.-import QtQuick +import QtQuick 2.15 import QtQuick.Controls 2.15 import QtQuick.Layouts 1.15
27-35: Inconsistent initialization pattern for auto level settings.Line 33 initializes
autoLevelThresholdFieldfromsettingsDialog.autoLevelClippingThreshold, but line 34 assignsautoLevelStrengthdirectly fromuiState.autoLevelStrength. For consistency, consider initializing both from the same source (either the dialog property or uiState).autoLevelThresholdField.text = settingsDialog.autoLevelClippingThreshold.toFixed(4) - settingsDialog.autoLevelStrength = uiState.autoLevelStrength + autoLevelStrengthSlider.value = settingsDialog.autoLevelStrengthfaststack/faststack/imaging/editor.py (1)
143-157: Update step numbering comments.The cropping step is labeled "3. Cropping" on line 148, but it should now be step 3 after rotation (step 1) and straighten (step 2). Also, line 158 labels exposure as "3. Exposure" but it should be step 4. Consider updating all step comments for accuracy.
- # 3. Cropping + # 3. Cropping (after rotation and straighten) crop_box = self.current_edits.get('crop_box')faststack/faststack/imaging/jpeg.py (2)
42-43: Redundant exception inlog.exceptioncalls.
log.exception()automatically includes exception info from the current context. The explicit{e}in the f-string is redundant and adds noise to the log message.Apply this pattern throughout the file:
- log.exception(f"PyTurboJPEG failed to decode image: {e}. Trying Pillow.") + log.exception("PyTurboJPEG failed to decode image. Trying Pillow.")
37-40: Use the exportedTJFLAG_FASTDCTconstant from turbojpeg instead of the hardcoded magic number.The turbojpeg library exports
TJFLAG_FASTDCTfor exactly this purpose. Import it alongsideTJPF_RGBand replace the hardcoded2048value to improve maintainability.- from turbojpeg import TurboJPEG, TJPF_RGB + from turbojpeg import TurboJPEG, TJPF_RGB, TJFLAG_FASTDCTThen use
flags |= TJFLAG_FASTDCTinstead offlags |= 2048.faststack/faststack/imaging/prefetch.py (2)
22-22: Move import to top of file.The
import threadingon line 22 should be at the top of the file with other imports for consistency and PEP 8 compliance.import logging import os import io import hashlib +import threading from pathlib import Path from concurrent.futures import ThreadPoolExecutor, Future from typing import List, Dict, Optional, Callable import mmapThen remove line 22.
295-518: Consider extracting duplicated decode logic.The decode-and-resize logic is repeated in multiple places:
- Lines 320-332 (ICC path)
- Lines 391-402 (ICC fallback)
- Lines 421-433 (ICC no profile fallback)
- Lines 451-463 (standard path)
A helper function would reduce duplication and maintenance burden.
Example extraction:
def _decode_image_from_file(self, image_path: Path, display_width: int, display_height: int, use_resized: bool, fast_dct: bool) -> Optional[np.ndarray]: """Decode image using mmap with optional resize.""" with open(image_path, "rb") as f: with mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ) as mmapped: if use_resized: return decode_jpeg_resized(mmapped, display_width, display_height, fast_dct=fast_dct) else: buffer = decode_jpeg_rgb(mmapped, fast_dct=fast_dct) if buffer is not None: img = PILImage.fromarray(buffer) img.thumbnail((display_width, display_height), PILImage.Resampling.LANCZOS) return np.array(img) return Nonefaststack/faststack/qml/Components.qml (2)
198-204: Redundant variable re-declaration.
cropCenterXandcropCenterYare already declared at lines 182-183 and can be reused here, avoiding redundant declarations.else if (mainMouseArea.isRotating) { cropDragMode = "rotate" - var cropCenterX = cropRect.x + cropRect.width / 2 - var cropCenterY = cropRect.y + cropRect.height / 2 cropStartAngle = Math.atan2(mouse.y - cropCenterY, mouse.x - cropCenterX) * 180 / Math.PI cropStartRotation = cropRotation }
228-228: Variableboxis already declared at line 174.Re-declaring
boxshadows the earlier declaration. Since they reference the same data, this works but is confusing.Remove the
varkeyword to reuse the existing variable:- var box = uiState.currentCropBox + box = uiState.currentCropBoxOr rename for clarity if distinct semantics are intended.
faststack/faststack/ui/provider.py (1)
63-65: Uselog.exceptionfor error with traceback.When catching an exception,
log.exceptionprovides the full traceback which is more useful for debugging thanlog.error.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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
faststack/ChangeLog.md(1 hunks)faststack/README.md(1 hunks)faststack/faststack.egg-info/PKG-INFO(1 hunks)faststack/faststack/AGENTS.md(1 hunks)faststack/faststack/config.py(1 hunks)faststack/faststack/debug_output.txt(1 hunks)faststack/faststack/faststack.json(1 hunks)faststack/faststack/imaging/cache.py(1 hunks)faststack/faststack/imaging/editor.py(7 hunks)faststack/faststack/imaging/jpeg.py(1 hunks)faststack/faststack/imaging/metadata.py(1 hunks)faststack/faststack/imaging/prefetch.py(1 hunks)faststack/faststack/io/executable_validator.py(1 hunks)faststack/faststack/io/helicon.py(1 hunks)faststack/faststack/io/indexer.py(1 hunks)faststack/faststack/io/sidecar.py(1 hunks)faststack/faststack/io/watcher.py(1 hunks)faststack/faststack/logging_setup.py(1 hunks)faststack/faststack/models.py(1 hunks)faststack/faststack/next.prompt(1 hunks)faststack/faststack/qml/Components.qml(1 hunks)faststack/faststack/qml/DeleteBatchDialog.qml(1 hunks)faststack/faststack/qml/ExifDialog.qml(1 hunks)faststack/faststack/qml/FilterDialog.qml(1 hunks)faststack/faststack/qml/HistogramWindow.qml(6 hunks)faststack/faststack/qml/ImageEditorDialog.qml(8 hunks)faststack/faststack/qml/JumpToImageDialog.qml(1 hunks)faststack/faststack/qml/Main.qml(1 hunks)faststack/faststack/qml/SettingsDialog.qml(1 hunks)faststack/faststack/tests/benchmark_decode.py(1 hunks)faststack/faststack/tests/benchmark_decode_bilinear.py(1 hunks)faststack/faststack/tests/check_turbo.py(1 hunks)faststack/faststack/tests/debug_metadata.py(1 hunks)faststack/faststack/tests/test_cache.py(1 hunks)faststack/faststack/tests/test_editor.py(1 hunks)faststack/faststack/tests/test_executable_validator.py(1 hunks)faststack/faststack/tests/test_metadata.py(1 hunks)faststack/faststack/tests/test_new_features.py(1 hunks)faststack/faststack/tests/test_pairing.py(1 hunks)faststack/faststack/tests/test_prefetch_logic.py(1 hunks)faststack/faststack/tests/test_sidecar.py(1 hunks)faststack/faststack/ui/keystrokes.py(1 hunks)faststack/faststack/ui/provider.py(1 hunks)faststack/faststack/verify_wb.py(1 hunks)faststack/pyproject.toml(1 hunks)faststack/requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
faststack/faststack/tests/debug_metadata.py (1)
faststack/faststack/imaging/metadata.py (1)
get_exif_data(39-195)
faststack/faststack/imaging/editor.py (2)
faststack/faststack/ui/provider.py (6)
highlights(755-756)highlights(759-762)blacks(795-796)blacks(799-802)whites(805-806)whites(809-812)faststack/faststack/app.py (3)
auto_levels(2673-2713)rotate_image_cw(2233-2237)rotate_image_ccw(2240-2246)
faststack/faststack/io/indexer.py (2)
faststack/faststack/models.py (1)
ImageFile(8-12)faststack/faststack/config.py (1)
get(84-85)
faststack/faststack/tests/benchmark_decode.py (1)
faststack/faststack/imaging/jpeg.py (1)
decode_jpeg_resized(114-177)
faststack/faststack/io/sidecar.py (2)
faststack/faststack/models.py (2)
Sidecar(27-32)EntryMetadata(15-23)faststack/faststack/app.py (1)
load(308-321)
faststack/faststack/tests/test_editor.py (1)
faststack/faststack/imaging/editor.py (1)
save_image(376-458)
faststack/faststack/tests/test_pairing.py (1)
faststack/faststack/io/indexer.py (2)
find_images(20-62)_find_raw_pair(64-84)
faststack/faststack/tests/test_cache.py (1)
faststack/faststack/imaging/cache.py (1)
ByteLRUCache(12-49)
faststack/faststack/tests/test_new_features.py (1)
faststack/faststack/imaging/editor.py (2)
ImageEditor(56-477)_apply_edits(132-300)
faststack/faststack/tests/test_sidecar.py (2)
faststack/faststack/io/sidecar.py (4)
SidecarManager(13-97)set_last_index(96-97)get_metadata(92-94)save(61-90)faststack/faststack/models.py (1)
EntryMetadata(15-23)
faststack/faststack/tests/test_metadata.py (1)
faststack/faststack/imaging/metadata.py (2)
get_exif_data(39-195)clean_exif_value(9-37)
faststack/faststack/io/helicon.py (2)
faststack/faststack/io/executable_validator.py (1)
validate_executable_path(23-95)faststack/faststack/config.py (1)
get(84-85)
faststack/faststack/config.py (1)
faststack/faststack/logging_setup.py (1)
get_app_data_dir(8-13)
faststack/faststack/imaging/prefetch.py (5)
faststack/faststack/models.py (2)
ImageFile(8-12)DecodedImage(35-44)faststack/faststack/imaging/jpeg.py (2)
decode_jpeg_rgb(31-53)decode_jpeg_resized(114-177)faststack/faststack/imaging/cache.py (1)
build_cache_key(72-78)faststack/faststack/config.py (1)
get(84-85)faststack/faststack/app.py (1)
get_display_info(210-213)
faststack/faststack/tests/benchmark_decode_bilinear.py (2)
faststack/faststack/imaging/jpeg.py (2)
decode_jpeg_rgb(31-53)_get_turbojpeg_scaling_factor(94-111)faststack/faststack/tests/benchmark_decode.py (2)
create_test_jpeg(8-16)benchmark(18-37)
faststack/faststack/ui/provider.py (3)
faststack/faststack/models.py (1)
DecodedImage(35-44)faststack/faststack/app.py (1)
get_decoded_image(352-450)faststack/faststack/config.py (1)
get(84-85)
🪛 LanguageTool
faststack/ChangeLog.md
[grammar] ~18-~18: Ensure spelling is correct
Context: ...n the cache is full so you can consider increassing the cache size in settings.
[1....
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~24-~24: The wording of this phrase can be improved.
Context: ... cool.
- Updated auto white balance to make it better, and put some controls for it in the se...
(MAKE_STYLE_BETTER)
[style] ~149-~149: Consider using a different verb for a more formal wording.
Context: ...e JPEG scaling factor calculation.
- Fixed an issue where panning the image was no...
(FIX_RESOLVE)
[grammar] ~164-~164: Use a hyphen to join words.
Context: ... 0.4
Todo
Make it use the full res image when zooming in
When multiple...
(QB_NEW_EN_HYPHEN)
faststack/README.md
[style] ~3-~3: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...astStack
Version 1.4 - December 1, 2025
By Alan Rockefeller
Ultra-fast, c...
(MISSING_COMMA_AFTER_YEAR)
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...10ms next/previous image switching, high performance decoding via PyTurboJPEG.
...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
faststack/ChangeLog.md
17-17: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.7)
faststack/faststack/imaging/metadata.py
56-56: Do not catch blind exception: Exception
(BLE001)
81-81: Multiple statements on one line (colon)
(E701)
82-82: Multiple statements on one line (colon)
(E701)
114-114: Do not catch blind exception: Exception
(BLE001)
130-130: Do not catch blind exception: Exception
(BLE001)
142-142: Do not catch blind exception: Exception
(BLE001)
180-180: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/tests/debug_metadata.py
54-54: Do not catch blind exception: Exception
(BLE001)
54-54: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
faststack/faststack/imaging/editor.py
455-455: Consider moving this statement to an else block
(TRY300)
456-456: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/io/executable_validator.py
110-110: Consider moving this statement to an else block
(TRY300)
faststack/faststack/io/indexer.py
39-39: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
65-65: Unused function argument: jpg_path
(ARG001)
faststack/faststack/imaging/jpeg.py
43-43: Redundant exception object included in logging.exception call
(TRY401)
52-52: Redundant exception object included in logging.exception call
(TRY401)
79-79: Consider moving this statement to an else block
(TRY300)
81-81: Redundant exception object included in logging.exception call
(TRY401)
90-90: Redundant exception object included in logging.exception call
(TRY401)
158-158: Redundant exception object included in logging.exception call
(TRY401)
176-176: Redundant exception object included in logging.exception call
(TRY401)
faststack/faststack/io/sidecar.py
57-57: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
87-87: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/tests/test_prefetch_logic.py
48-48: Abstract raise to an inner function
(TRY301)
48-48: Create your own exception
(TRY002)
48-48: Avoid specifying long messages outside the exception class
(TRY003)
faststack/faststack/verify_wb.py
62-62: Do not use bare except
(E722)
62-63: try-except-pass detected, consider logging the exception
(S110)
faststack/faststack/tests/test_executable_validator.py
40-40: Unused lambda argument: self
(ARG005)
62-62: Unused lambda argument: self
(ARG005)
66-66: Unpacked variable error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
123-123: Unused lambda argument: self
(ARG005)
127-127: Unpacked variable error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
faststack/faststack/tests/test_pairing.py
55-55: Multiple statements on one line (semicolon)
(E702)
58-58: Multiple statements on one line (semicolon)
(E702)
58-58: Multiple statements on one line (semicolon)
(E702)
63-63: Multiple statements on one line (semicolon)
(E702)
63-63: Multiple statements on one line (semicolon)
(E702)
68-68: Multiple statements on one line (semicolon)
(E702)
68-68: Multiple statements on one line (semicolon)
(E702)
69-69: Multiple statements on one line (semicolon)
(E702)
69-69: Multiple statements on one line (semicolon)
(E702)
faststack/faststack/imaging/cache.py
19-19: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
faststack/faststack/tests/test_sidecar.py
14-14: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
faststack/faststack/tests/test_metadata.py
11-11: Unused method argument: mock_exists
(ARG002)
69-69: Use raise without specifying exception name
Remove exception name
(TRY201)
faststack/faststack/io/helicon.py
71-71: Redundant exception object included in logging.exception call
(TRY401)
78-78: subprocess call: check for execution of untrusted input
(S603)
86-86: Consider moving this statement to an else block
(TRY300)
88-88: Redundant exception object included in logging.exception call
(TRY401)
91-91: Redundant exception object included in logging.exception call
(TRY401)
faststack/faststack/config.py
82-82: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/imaging/prefetch.py
513-513: Consider moving this statement to an else block
(TRY300)
faststack/faststack/tests/benchmark_decode_bilinear.py
41-41: Do not catch blind exception: Exception
(BLE001)
49-49: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/ui/provider.py
28-28: Unused method argument: size
(ARG002)
28-28: Unused method argument: requestedSize
(ARG002)
64-64: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
705-705: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🔇 Additional comments (63)
faststack/faststack/models.py (1)
1-44: LGTM! Well-structured dataclass definitions with appropriate type hints.The module provides clean, correct data structures for image metadata and image buffers. Type hints are sensible, and the use of
dataclasses.field(default_factory=...)for mutable defaults is the right pattern. The__sizeof__implementation inDecodedImageproperly reports buffer memory usage.faststack/faststack/io/helicon.py (4)
1-15: Import structure looks good.The addition of the executable validator import properly supports the security hardening in the function.
26-40: Strong security validation of executable path.The use of
validate_executable_pathbefore execution is a solid security improvement. The early-return pattern on validation failure is appropriate.
77-86: Subprocess hardening is sound; clarify temp file lifecycle.The subprocess call correctly uses
shell=False, validates the executable beforehand, and closes file descriptors—mitigating the S603 linter concern. However, the function returns thetmp_pathon success but creates the file withdelete=False(line 47). Verify that the caller is responsible for cleanup, or document the expected lifecycle. If not handled by the caller, this becomes a resource leak.Please confirm: who is responsible for deleting the temporary file after Helicon Focus consumes it? Is cleanup handled by the caller or should this function provide cleanup logic?
1-92: AI summary inconsistency: claims formatting-only changes but code includes functional security improvements.The AI-generated summary states this is a "formatting/reflow only" change with "no functional changes," but the code demonstrates substantive security improvements: executable path validation via
validate_executable_path, safe argument parsing usingshlex.split, and hardened subprocess execution withshell=Falseandclose_fds=True. These are functional enhancements, not mere formatting adjustments.faststack/faststack/logging_setup.py (1)
1-46: Formatting changes only—no functional review needed.This file contains only whitespace/formatting adjustments with no logic changes.
faststack/faststack/faststack.json (1)
1-6: Formatting changes only—no functional review needed.JSON formatting adjustment with no semantic changes.
faststack/requirements.txt (1)
1-8: Formatting changes only—no functional review needed.Cosmetic formatting adjustment with no changes to package versions or dependencies.
faststack/faststack/io/watcher.py (1)
1-74: Formatting changes only—no functional review needed.This file contains only whitespace/formatting adjustments with no logic changes.
faststack/faststack/qml/JumpToImageDialog.qml (1)
1-90: Formatting changes only—no functional review needed.This file contains only whitespace/formatting adjustments with no logic changes.
faststack/faststack/io/executable_validator.py (1)
1-112: Formatting changes only—no functional review needed.This file contains only whitespace/formatting adjustments with no logic changes. The static analysis suggestion about moving Line 110 to an
elseblock is a stylistic preference; the current try/return/except/return pattern is clear and idiomatic.faststack/pyproject.toml (1)
2-34: Metadata and script declaration look consistentThe version bump and script entry wire cleanly into this configuration; nothing here looks problematic from a packaging perspective.
faststack/faststack/imaging/metadata.py (1)
9-37: EXIF value cleaning looks robust for display/JSON useHandling bytes, strings, and nested lists/tuples via
clean_exif_valueshould keep both the summary and full maps printable and JSON-serializable; this is a good balance between robustness and not over-normalizing the raw values.faststack/faststack/tests/test_cache.py (1)
7-61: Cache tests adequately cover core behaviorThe tests validate initialization, size accounting, eviction of least-recently-used entries, and size updates on overwrite using a simple
MockItem; this is a nice focused unit test suite forByteLRUCache.faststack/faststack/qml/FilterDialog.qml (1)
1-87: Filter dialog wiring and imports look goodThe dialog’s imports, bindings to
filterDialog.filterString, and coordination with thecontrolleron open/close are consistent and self-contained; nothing concerning here.faststack/README.md (1)
29-29: LGTM! Clear feature documentation.The RGB histogram feature is well-documented with clear usage instructions and functional details about clipping detection and zoom responsiveness.
faststack/faststack/tests/test_editor.py (1)
9-27: LGTM! Well-structured test for mtime preservation.The test correctly validates that
save_imagepreserves the original file's modification time after applying edits. The use ofpytest.approxwith tight tolerance ensures precision while accommodating filesystem timestamp limitations.faststack/ChangeLog.md (1)
5-11: LGTM! Clear release notes for version 1.4.The changelog entry accurately documents the new features and improvements in this release.
faststack/faststack/config.py (1)
17-19: LGTM! Well-defined configuration defaults.The new configuration options for optimization mode and auto-levels functionality are clearly defined with sensible defaults.
faststack/faststack/qml/HistogramWindow.qml (2)
15-26: LGTM! Good keyboard focus management.Adding a FocusScope with H-key handling allows users to close the histogram window using the same key that opened it, improving UX consistency.
197-197: No issues found. The histogram data property names are correct and consistent. TheuiState.histogramDatastructure is created with keys'r','g','b','r_clip','g_clip','b_clip','r_preclip','g_preclip','b_preclip'(faststack/faststack/app.py, lines 2361-2371), which match exactly the property accesses in HistogramWindow.qml at lines 197, 220, and 242.faststack/faststack/tests/debug_metadata.py (1)
13-57: LGTM! Effective debug harness for EXIF extraction.The test script appropriately mocks PIL components and validates EXIF data extraction. The broad exception handling is suitable for a debug script that logs all errors to a file.
faststack/faststack/ui/keystrokes.py (1)
42-42: Both methods are properly implemented.The methods
show_exif_dialog(line 524) andquick_auto_levels(line 2716) are implemented in faststack/faststack/app.py with complete implementations and docstrings.faststack/faststack/tests/benchmark_decode.py (1)
1-40: LGTM!The benchmark implementation follows good practices: uses
time.perf_counter()for accurate timing, includes a warmup iteration, and generates appropriately sized test data. The iteration count and timing methodology are suitable for measuring decode performance.faststack/faststack/tests/test_metadata.py (2)
71-89: LGTM!Thorough test coverage for
clean_exif_valuecovering strings, bytes, binary fallback, numbers, and lists. The assertions correctly verify the expected behavior for each input type.
91-105: LGTM!Good edge case coverage testing both the "no EXIF data" scenario and non-existent file handling, ensuring the function gracefully returns empty dictionaries.
faststack/faststack/qml/ExifDialog.qml (1)
81-113: LGTM!The
getDisplayText()function correctly handles both summary (ordered keys) and full (alphabetically sorted) views with appropriate fallback for empty data. The implementation cleanly formats EXIF data for display.Minor note: Lines 88 and 94 both declare
var i, and lines 95 and 102 usevar key. While JavaScript's function-level scoping makes this work, using distinct variable names (e.g.,jfor the second loop) would improve readability.faststack/faststack/next.prompt (1)
1-1: This appears to be an internal requirements/prompt file.This file contains natural language descriptions of features and bugs rather than code. It may have been accidentally included in the PR. Consider whether this should be:
- Removed from the PR
- Converted to a proper issue/task tracker
- Moved to a documentation directory if intentional
faststack/faststack/qml/Main.qml (5)
38-42: LGTM!The
openExifDialoghelper cleanly separates summary and full data population from dialog opening, providing a clear API for displaying EXIF information.
650-678: LGTM!The keyboard handling for the editor toggle (E key) and save (Ctrl+S) is well implemented with proper guards for
uiStateandcontrolleravailability, and correctly setsevent.acceptedto prevent event bubbling.
682-692: Good fix for layout stability.Using a fixed footer height prevents the main image area from resizing when status labels appear/disappear, which avoids unnecessary cache invalidations. This is a thoughtful optimization.
966-969: LGTM!The
show_delete_batch_dialogfunction properly sets the batch count before opening the dialog, ensuring the confirmation dialog displays the correct number of images to be deleted.
971-975: LGTM!The ExifDialog is properly instantiated with theme-aware colors bound from the root window properties, ensuring consistent styling with the rest of the application.
faststack/faststack/tests/benchmark_decode_bilinear.py (2)
53-60: LGTM!The
create_test_jpegfunction is consistent with the implementation inbenchmark_decode.pyand correctly generates a random test image for benchmarking.
62-84: LGTM!The benchmark function follows the same pattern as
benchmark_decode.pywith appropriate warmup and iteration-based timing. The main entry point is correctly structured.faststack/faststack/tests/test_new_features.py (2)
42-67: LGTM!The
test_highlights_recoverytest correctly validates the negative highlights (recovery) behavior by checking that bright pixels (255) are significantly darkened while midtone pixels (128) remain largely unchanged. The expected calculations match the implementation in_apply_edits.
69-81: LGTM!The
test_straighten_angletest verifies that applying a 45-degree rotation withexpand=Truechanges the image dimensions, which is the expected behavior for free rotation.faststack/faststack/qml/DeleteBatchDialog.qml (3)
5-23: LGTM!The dialog setup is well-structured with appropriate modal behavior, configurable colors, and clean background styling.
49-88: Good UX: Destructive action is visually distinguished.The "Delete All" button appropriately uses red coloring to indicate a destructive action, and the "Delete Current Image" button uses neutral styling. The controller method calls are properly guarded.
109-121: LGTM!The
onOpenedandonClosedhandlers correctly notify the Python controller about dialog state, which is consistent with patterns used by other dialogs (e.g., ExifDialog).faststack/faststack/qml/SettingsDialog.qml (2)
56-75: LGTM!The
onAcceptedhandler correctly persists all settings including the newoptimizeFor, auto-level, and AWB settings touiState.
224-253: LGTM!The Auto Levels UI section is well-implemented with appropriate validation on the threshold field (0.0-10.0 range) and a slider for strength (0.0-1.0). The percentage display for strength is a nice touch for user clarity.
faststack/faststack/imaging/editor.py (7)
93-95: LGTM!New edit parameters
textureandstraighten_angleare properly initialized with sensible defaults.
187-196: LGTM!The new negative highlights (recovery) branch correctly implements the recovery algorithm by applying a multiplicative factor that darkens bright pixels while leaving midtones unchanged. The mask calculation and factor application align with the test expectations in
test_new_features.py.
242-261: LGTM!The reworked white balance using multiplicative gains is a significant improvement over additive shifts. This approach correctly preserves black levels (since 0 × gain = 0) while adjusting color balance proportionally across the tonal range. The comments clearly explain the temperature and tint adjustments.
283-299: LGTM!The texture enhancement implementation follows the same pattern as clarity but with a smaller blur radius (2.0 vs 20) to target fine details. The midtone masking approach is appropriate for this use case.
302-344: LGTM!The
auto_levelsimplementation correctly computes percentile-based black and white points from the luminance histogram. The conversion formulas forblacksandwhitesare consistent with the level mapping in_apply_edits.
389-394: Good defensive coding for timestamp capture.Properly handling the case where reading file timestamps fails, with a clear warning message. This prevents crashes on edge cases like permission issues.
460-466: LGTM!The
_restore_file_timeshelper is well-designed with best-effort semantics and appropriate error handling.faststack/faststack/imaging/prefetch.py (3)
37-52: LGTM on ICC transform caching.The caching strategy using stable keys (SHA-256 digest for embedded profiles, file path for monitor profiles) is sound and thread-safe with proper lock usage.
96-136: LGTM on saturation compensation.The in-place RGB saturation adjustment is correctly implemented with proper bounds checking, early return optimization for
factor=1.0, and vectorized numpy operations.
541-545: LGTM on shutdown handling.The shutdown method correctly cancels all pending tasks before shutting down the executor with
wait=Falsefor responsive shutdown.faststack/faststack/qml/ImageEditorDialog.qml (5)
43-53: LGTM on keyboard shortcuts.The E and Escape keys for closing the editor provide good UX for quick dismissal, and
event.accepted = trueproperly prevents event propagation.
122-131: LGTM on Auto Levels button.The button correctly calls
controller.auto_levels()and triggers a UI refresh viaupdatePulse++, following the same pattern as the Auto White Balance button.
200-222: LGTM on direct value input.The TextInput with validation, clamping, and proper normalization for backend values provides a clean UX for precise parameter adjustment.
255-263: LGTM on double-tap reset.The TapHandler for double-tap to reset provides convenient UX for returning sliders to their neutral value.
160-174: LGTM on button renaming and behavior.The renamed buttons clearly communicate their actions, and the "Save and Close" button correctly chains
save_edited_image()with closing the editor.faststack/faststack/qml/Components.qml (3)
54-70: LGTM on zoom state management.The
updateZoomState()function correctly manages zoom threshold detection (1.1 for zoomed in, 1.0 for zoomed out) and triggers histogram updates when visible.
545-630: LGTM on aspect ratio constraint logic.The function correctly handles all drag modes, maintains aspect ratio after boundary clamping, and properly anchors to the correct edge/corner based on drag direction.
715-751: Overlay rectangles don't account for crop rotation.The four semi-transparent overlay rectangles use
cropRect.x,cropRect.y, etc. directly without accounting for the rotation applied tocropRectat line 759. When the crop box is rotated, these overlays won't align correctly with the rotated crop area.Is the rotation feature expected to show aligned overlays, or is this a known limitation? If alignment is expected, the overlay rendering would need to use a more complex approach (e.g., canvas-based masking or rotated clip paths).
faststack/faststack/ui/provider.py (3)
46-61: LGTM on color space handling.The conditional color space assignment correctly handles ICC mode (where pixels are already in monitor space), non-ICC mode (sets sRGB), and gracefully degrades when
QColorSpaceis unavailable. The buffer attachment at line 60 correctly prevents premature garbage collection.
637-652: LGTM on snake_case aliases for QML.The snake_case property aliases for
white_balance_byandwhite_balance_mgprovide convenient QML bracket notation access while maintaining the camelCase primary properties.
28-28: Unused parameters are required by interface.The
sizeandrequestedSizeparameters flagged by static analysis are required by theQQuickImageProvider.requestImageinterface signature, even if not used. No change needed.
1 participant
Fixed crop, added crop rotation, added auto levels
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.