Version 1.2#19
Conversation
WalkthroughThe PR adds comprehensive Auto White Balance (AWB) functionality with Lab and RGB modes, introduces AWB configuration parameters, adds color-aware UI styling to multiple dialogs, modifies image saving to return path pairs for undo/redo tracking, and improves JPEG scaling with aspect-ratio awareness. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant QML as QML UI<br/>(SettingsDialog)
participant UIState as UIState<br/>(provider.py)
participant AppCtrl as AppController<br/>(app.py)
participant Config as Config<br/>(config.py)
User->>QML: Adjust AWB Mode/Strength
QML->>UIState: Set awbMode/awbStrength
UIState->>AppCtrl: get/set_awb_mode<br/>get/set_awb_strength
AppCtrl->>AppCtrl: Update internal state
AppCtrl->>Config: Read/persist AWB params
AppCtrl-->>UIState: Return updated values
UIState-->>QML: Signal change<br/>(awbModeChanged, etc.)
QML->>QML: Update bindings
Note over User,QML: Settings persisted in config
sequenceDiagram
participant Editor as Image Editor<br/>(editor.py)
participant ImageFile as Image File
participant UndoMgr as Undo Manager<br/>(app.py)
Editor->>Editor: Apply edits
Editor->>ImageFile: save_image()
ImageFile->>ImageFile: Backup original
ImageFile->>ImageFile: Save modified image<br/>with EXIF (JPEG)
ImageFile-->>Editor: (saved_path, backup_path)
Editor->>UndoMgr: Track undo with<br/>(saved_path, backup_path)
Note over Editor,UndoMgr: Previous: tracked only<br/>filepath tuple
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/faststack/app.py (1)
1753-1788: save_edited_image still assumes save_image returns a Path, not (saved_path, backup_path)
ImageEditor.save_image()now returns(saved_path, backup_path), butsave_edited_image()still treats the result as a singlePath. This will makeoriginal_patha tuple, so the later comparisonif img_file.path == original_pathwill never match and the UI won’t navigate to the edited image correctly.@Slot() def save_edited_image(self): """Saves the edited image.""" - saved_path = self.image_editor.save_image() - if saved_path: + save_result = self.image_editor.save_image() + if save_result: + saved_path, backup_path = save_result # Clear the image editor state so it will reload fresh next time self.image_editor.original_image = None self.image_editor.current_filepath = None self.image_editor._preview_image = None @@ - # Refresh the view - need to refresh image list since backup file was created - original_path = saved_path + # Refresh the view - need to refresh image list since backup file was created + original_path = saved_pathIf you also want manual editor saves to be undoable like quick AWB, you could additionally push
(saved_path, backup_path)intoundo_history, but that’s an optional enhancement.
🧹 Nitpick comments (6)
faststack/ChangeLog.md (1)
8-8: Consider more specific wording.The phrase "make it better" could be more descriptive. Consider something like "improved auto white balance algorithm" or "enhanced auto white balance with Lab and RGB modes".
faststack/README.md (1)
12-12: Consider hyphenating the compound adjective.While the typo fix is good, the phrase "high performance decoding" would be more grammatically correct as "high-performance decoding" since it's a compound adjective modifying a noun.
faststack/faststack/imaging/editor.py (1)
245-300: save_image contract and error handling look reasonable; consider tightening exceptions/loggingThe new
(saved_path, backup_path)return and EXIF/format handling are consistent and should work for standard JPEG/bitmap formats. The nestedtry/except Exceptionwithprint()is a bit loose; if you care about diagnostics, consider catching narrower exceptions and routing through the logger instead offaststack/faststack/qml/SettingsDialog.qml (1)
27-32: AWB settings surface and UI wiring are coherent end‑to‑endThe new AWB properties on the dialog, their propagation to
uiStateinonAccepted, and the General/AWB tab layout (including advanced bounds) line up cleanly with the backend AWB getters/setters; I don’t see functional issues here. Only optional follow‑up would be persisting the “Advanced Settings” checkbox state if you want that UX.Also applies to: 41-48, 50-66, 68-295
faststack/faststack/ui/provider.py (1)
87-93: UIState AWB properties and open_folder slot correctly mirror AppControllerThe new AWB properties cleanly delegate to
app_controllergetters/setters with matching notify signals, andopen_folderis exposed as a simple slot wrapper. If slider-driven updates ever become noisy, you could add “value unchanged” checks in setters, but it’s not required.Also applies to: 294-356, 446-449
faststack/faststack/app.py (1)
618-621: clear_all_batches now also clears stacks; align naming/docstrings with behavior
clear_all_batches()now delegates toclear_all_stacks(), andclear_all_stacks()was expanded to clear stacks, batches, and markers. If this unified “clear everything” behavior is intentional, consider renaming or updating the clear_all_batches docstring/UI copy so it doesn’t imply batches‑only.Also applies to: 955-970
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
faststack/ChangeLog.md(1 hunks)faststack/README.md(1 hunks)faststack/faststack/app.py(9 hunks)faststack/faststack/config.py(1 hunks)faststack/faststack/faststack.json(1 hunks)faststack/faststack/imaging/editor.py(2 hunks)faststack/faststack/imaging/jpeg.py(1 hunks)faststack/faststack/qml/FilterDialog.qml(4 hunks)faststack/faststack/qml/ImageEditorDialog.qml(9 hunks)faststack/faststack/qml/ImageEditorDialog.qml.old(0 hunks)faststack/faststack/qml/JumpToImageDialog.qml(3 hunks)faststack/faststack/qml/Main.qml(13 hunks)faststack/faststack/qml/SettingsDialog.qml(2 hunks)faststack/faststack/ui/provider.py(3 hunks)
💤 Files with no reviewable changes (1)
- faststack/faststack/qml/ImageEditorDialog.qml.old
🧰 Additional context used
🧬 Code graph analysis (2)
faststack/faststack/ui/provider.py (1)
faststack/faststack/app.py (15)
get_awb_mode(1097-1098)set_awb_mode(1101-1103)get_awb_strength(1106-1107)set_awb_strength(1110-1112)get_awb_warm_bias(1115-1116)set_awb_warm_bias(1119-1121)get_awb_luma_lower_bound(1124-1125)set_awb_luma_lower_bound(1128-1130)get_awb_luma_upper_bound(1133-1134)set_awb_luma_upper_bound(1137-1139)get_awb_rgb_lower_bound(1142-1143)set_awb_rgb_lower_bound(1146-1148)get_awb_rgb_upper_bound(1151-1152)set_awb_rgb_upper_bound(1155-1157)open_folder(1174-1179)
faststack/faststack/app.py (4)
faststack/faststack/ui/provider.py (9)
clear_all_stacks(377-378)get_default_directory(435-436)set_default_directory(439-440)open_directory_dialog(443-444)open_folder(447-448)white_balance_by(552-553)white_balance_by(556-557)white_balance_mg(560-561)white_balance_mg(564-565)faststack/faststack/config.py (5)
save(67-75)get(77-78)getfloat(83-84)getint(80-81)load(48-64)faststack/faststack/io/sidecar.py (2)
save(62-91)load(28-60)faststack/faststack/imaging/editor.py (2)
save_image(245-302)set_edit_param(234-239)
🪛 LanguageTool
faststack/ChangeLog.md
[style] ~8-~8: The wording of this phrase can be improved.
Context: ...k cool. - Updated auto white balance to make it better, and put some controls for it in the se...
(MAKE_STYLE_BETTER)
faststack/README.md
[grammar] ~12-~12: Use a hyphen to join words.
Context: ...10ms next/previous image switching, high performance decoding via PyTurboJPEG. ...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.5)
faststack/faststack/imaging/editor.py
294-294: Do not catch blind exception: Exception
(BLE001)
299-299: Consider moving this statement to an else block
(TRY300)
faststack/faststack/app.py
1159-1159: Redefinition of unused get_default_directory from line 1093
(F811)
1889-1889: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1940-1940: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (13)
faststack/faststack/qml/JumpToImageDialog.qml (1)
15-24: LGTM! Color theming implemented correctly.The placeholder colors provide good visual feedback if the properties aren't properly initialized from Main.qml, and the implementation correctly replaces Material theme-based styling with explicit color properties.
faststack/faststack/faststack.json (1)
1-6: LGTM! Valid initialization structure.The JSON structure is valid and appropriate for persistent storage of application state with empty initial collections.
faststack/faststack/config.py (1)
31-39: LGTM! Well-structured AWB configuration.The new AWB configuration section follows the established pattern and includes reasonable defaults. The inline comment documents the mode options clearly.
faststack/faststack/imaging/jpeg.py (1)
122-130: LGTM! Correct aspect-ratio aware scaling.The cross-product comparison correctly determines which dimension constrains the resize operation by comparing the image aspect ratio to the target aspect ratio. This is an improvement over the previous
max(width, height)approach and will produce more optimal TurboJPEG scaling factors.faststack/faststack/qml/FilterDialog.qml (1)
15-24: LGTM! Consistent color theming implementation.The color properties are applied consistently across all UI elements, following the same pattern as the other dialogs in this PR.
faststack/faststack/qml/ImageEditorDialog.qml (2)
15-16: LGTM! Color theming implemented consistently.The backgroundColor and textColor properties are correctly applied throughout the dialog, maintaining consistency with the other dialogs in this PR.
Also applies to: 41-41
218-218: LGTM! Dynamic slider track color provides proper contrast.The conditional use of
Qt.lightervsQt.darkerbased on the background color ensures the slider track remains visible regardless of the theme.faststack/faststack/qml/Main.qml (1)
25-28: Centralized theming, hover color, and open_folder wiring look solidUsing
currentBackgroundColor/hoverColoracross menus, footer, and dialogs keeps light/dark theming consistent, and theuiState.open_folder()hookup for “Open Folder…” is correctly guarded onuiState. Just make sureFilterDialog,JumpToImageDialog, andImageEditorDialogall declarebackgroundColor/textColorproperties as expected by these bindings.Also applies to: 100-105, 127-132, 154-159, 181-186, 215-238, 302-311, 424-428, 563-567, 628-635, 850-870
faststack/faststack/app.py (5)
32-34: Disabling Pillow’s MAX_IMAGE_PIXELS is a conscious DoS trade‑offSetting
Image.MAX_IMAGE_PIXELS = Noneremoves Pillow’s decompression‑bomb protection, allowing arbitrarily large images. That’s fine for trusted local image sets, but if there’s any chance of untrusted input, you may want to cap this or at least document the risk.
955-970: clear_all_stacks implementation is coherent for stacks + batchesWithin its new scope,
clear_all_stacks()correctly resets stack/batch lists and start markers, updates the sidecar, invalidates cached metadata, and syncs UI state with a status message. No functional issues seen here.
1806-1861: quick_auto_white_balance: tuple handling and undo wiring look correct
quick_auto_white_balance()correctly unpacks(saved_path, backup_path)fromsave_image()and records both paths inundo_historyfor theauto_white_balanceaction. The subsequent cache invalidation and UI refresh are consistent with the general image‑change pattern.
1855-1885: auto_white_balance dispatcher and legacy (RGB) implementation are logically soundThe dispatcher selects between
auto_white_balance_lab()andauto_white_balance_legacy()based onconfig['awb']['mode'], with a clear error path for unknown modes. The legacy RGB “grey world” implementation clamps slider values and updates both the editor and UI state consistently.
1927-2015: Lab‑based AWB implementation is consistent with slider semantics and config parametersThe Lab AWB path:
- Filters pixels by RGB and luma bounds from
config['awb'],- Computes mean a*/b* on the masked set,
- Targets (128, 128+warm_bias) and scales shifts into
white_balance_by/white_balance_mgusingstrengthand a fixed factor,- Updates both editor and UI state, then refreshes the image.
This matches the white-balance math in
ImageEditor._apply_editsand respects the new AWB config fields.
1 participant
Improve auto white balance
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.