Fix histogram#28
Conversation
WalkthroughReclassifies ignore patterns and project deps; adjusts imaging behavior (preview/export edit paths, white-point math); adds ByteLRUCache.clear(); narrows PySide6 import exception handling; triggers histogram recalculation after preview renders; minor UI and test updates; removes a tooling script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI
participant AppController as App
participant Editor
participant HistogramWorker as Worker
UI->>App: user requests preview/render
App->>Editor: render preview (async)
Editor-->>App: preview result (image/preview_data)
Note right of App fill:`#E8F5E9`: new step — always call update_histogram()
App->>Worker: kick histogram worker with preview_data
alt preview_data missing
Worker->>App: request fallback image
App-->>Worker: provide decoded main image
end
Worker->>Worker: compute histogram
Worker-->>App: histogram ready
App->>UI: update histogram display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
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: 3
🧹 Nitpick comments (3)
tools/reproduce_issue.py (1)
5-5: Consider using or removing the unused logger.The module-level logger is declared but never used in this script. All output currently uses
print(). Consider either:
- Using the logger instead of print statements for consistency
- Removing the unused logger declaration
faststack/qml/Main.qml (1)
25-25: Verify the dark theme background color change.The background color changed from
#2b2b2b(dark gray) to#000000(pure black). While this increases contrast, pure black backgrounds can cause eye strain and OLED smearing on some displays. Modern UI guidelines (including Material Design) typically recommend dark grays like#121212or#1e1e1eover pure black for dark themes.Is this change intentional and related to histogram visibility? If the goal is better contrast for histogram display, consider whether a less extreme dark background might achieve the same result with better ergonomics.
faststack/qml/ImageEditorDialog.qml (1)
440-455: Keeping_pendingValue/_lastSentValuein sync on double‑click reset is correctResetting both cached values to
0.0along withvalueand the backend call avoids the timer re-emitting stale nonzero values after a double-tap reset. If you want to micro-optimize, you could also skip startingsendTimerin the double-click path, but that’s optional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignorefaststack/app.pyfaststack/imaging/cache.pyfaststack/imaging/editor.pyfaststack/imaging/prefetch.pyfaststack/logging_setup.pyfaststack/qml/ImageEditorDialog.qmlfaststack/qml/Main.qmlfaststack/tests/test_new_features.pyfaststack/tests/test_sidecar.pypyproject.tomlrequirements.txttools/reproduce_issue.py
💤 Files with no reviewable changes (2)
- requirements.txt
- faststack/tests/test_sidecar.py
🧰 Additional context used
🧬 Code graph analysis (3)
faststack/tests/test_new_features.py (1)
faststack/imaging/editor.py (1)
_apply_edits(278-486)
faststack/app.py (2)
faststack/faststack/app.py (4)
_apply_histogram_result(2734-2748)_apply_preview_result(2793-2812)update_histogram(2610-2626)prev_image(590-598)faststack/faststack/ui/provider.py (2)
histogramData(679-682)isHistogramVisible(641-650)
faststack/imaging/editor.py (1)
faststack/ui/provider.py (2)
whites(913-914)whites(917-920)
🪛 Ruff (0.14.10)
tools/reproduce_issue.py
9-9: Undefined name os
(F821)
9-9: Undefined name os
(F821)
9-9: Undefined name os
(F821)
🔇 Additional comments (10)
faststack/logging_setup.py (1)
19-19: LGTM! Documentation improvement.The docstring now explicitly states the WARNING log level for non-debug mode, which accurately reflects the implementation on line 40. The addition of "to reduce noise" provides helpful context for this design choice.
pyproject.toml (1)
47-47: LGTM: Pytest temp directory configuration.The
--basetemp=./var/pytest-tempconfiguration properly organizes pytest temporary files into a dedicated directory that's ignored by git (as seen in the updated .gitignore).faststack/imaging/cache.py (1)
49-58: LGTM: Well-implemented clear method.The implementation correctly disables the eviction callback during mass clear operations to prevent spurious "thrashing" warnings. The use of try/finally ensures the callback is always restored, even if an exception occurs during clearing.
.gitignore (1)
13-13: LGTM: Standard Python build artifact.Adding
faststack.egg-info/to the ignore list is correct. This directory is generated during package installation and should not be committed.faststack/imaging/prefetch.py (1)
14-19: Narrowing Qt import fallback toImportErroris a good safety improvementCatching only
ImportErroravoids silently swallowing other runtime errors during PySide6 import while preserving the intended “GUI-optional” behavior (QTimer/QImage set toNonewhen Qt isn’t installed).faststack/app.py (1)
2819-2835: Hooking histogram updates to preview completion matches intended behaviorTriggering
update_histogram()right after a preview frame is accepted ensures the histogram is computed from_last_rendered_previewinstead of stale data, while the existing visibility guard and throttling keep it cheap when the histogram/editor isn’t shown.faststack/tests/test_new_features.py (1)
15-46: Tests correctly updated for new auto-levels and export‑only straighten behaviorAsserting
whites > 0.0matches the newwhite_point = 255 - whites * 40convention, and explicitly calling_apply_edits(..., for_export=True)intest_straighten_angleensures the straighten path is exercised now that the default is preview-mode (for_export=False).Also applies to: 75-87
faststack/imaging/editor.py (3)
13-18: Qt import fallback narrowed appropriately toImportErrorCatching only
ImportErrorforPySide6.QtGui.QImagemeans real runtime errors during import won’t be masked by silently settingQImage = None, while still supporting non-GUI environments where PySide6 isn’t installed.
351-361: Blacks/whites mapping now self‑consistent withauto_levelsand test expectationsUsing
black_point = -blacks * 40andwhite_point = 255 - whites * 40, and derivingblacks = -p_low / 40.0,whites = (255.0 - p_high) / 40.0inauto_levels, makes the level mapping algebraically coherent and explains whywhitesis now positive for compressed highlights (as reflected intest_auto_levels_strength).Also applies to: 488-525
278-341: All call sites are correctly configured; no issues found with straighten_angle or export logic.The implementation is correct. The method defaults to
for_export=False(preview mode), which means:
verify_wb.pycalls (white balance tests) correctly use preview behavior by defaulttest_highlights_recoverycorrectly uses preview behavior by defaultsave_image()correctly passesfor_export=Trueto apply straighten_angle before exportrotate_autocrop_rgb()exists and is properly used for auto-straighten scenariosNo call sites rely on an incorrect default—the default is already set to preview behavior, and all export paths explicitly pass
for_export=True.
| # Mock the parts of the app needed for setup_logging | ||
| # We need to make sure we can import faststack modules | ||
| sys.path.append(os.getcwd()) | ||
| sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) |
There was a problem hiding this comment.
Critical: Missing os import.
Line 9 uses os.path.dirname, os.path.abspath, and os.path.dirname but the os module is not imported. This will cause a NameError at runtime.
🔎 Proposed fix
Add the missing import at the top of the file:
import logging
import sys
+import os
log = logging.getLogger(__name__)📝 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.
| sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) | |
| import logging | |
| import sys | |
| import os | |
| log = logging.getLogger(__name__) |
🧰 Tools
🪛 Ruff (0.14.10)
9-9: Undefined name os
(F821)
9-9: Undefined name os
(F821)
9-9: Undefined name os
(F821)
🤖 Prompt for AI Agents
In tools/reproduce_issue.py around line 9, the code calls os.path functions but
the os module is not imported; add "import os" to the top of the file (alongside
the other imports) so os is defined before sys.path.insert is executed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/app.py (1)
651-656: Minor: Potential UI blocking on histogram fallback.The fallback to
get_decoded_image(self.current_index)enables histogram computation when the editor isn't open, which fixes the reported issue. However,get_decoded_image()can block the UI thread on cache misses (line 439-441). Since_kick_histogram_worker()runs on the main thread (via timer), this introduces potential UI stuttering.Impact: Low - cache misses should be rare due to prefetching, and the blocking behavior is acceptable for an image viewer. However, it's inconsistent with the goal of offloading histogram computation to a background thread.
Suggested improvement: Consider passing
self.current_indexto_compute_histogram_worker()and retrieving the decoded image in the worker thread to avoid any main-thread blocking.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignorefaststack/app.pypyproject.tomltools/reproduce_issue.py
💤 Files with no reviewable changes (1)
- tools/reproduce_issue.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🔇 Additional comments (2)
.gitignore (1)
13-37: Good catch on the previous issue—the leading+has been removed.The prior review flagged
+var/as incorrect; the current version correctly showsvar/without the leading character. All new sections are properly organized and formatted:
- Line 13:
faststack.egg-info/addition under Build outputs ✓- Lines 26–30: Documentation/Generated section with ARCHITECTURE.md properly reclassified ✓
- Lines 32–34: Caches section correctly added ✓
- Lines 36–37: Runtime/Data section with
var/properly formatted ✓faststack/app.py (1)
834-836: LGTM: Histogram update after preview render.Correctly triggers histogram recalculation after successful preview renders, ensuring the histogram reflects the latest edited image. The visibility guards in
update_histogram()(lines 2620-2622) and throttling logic prevent unnecessary computation when the histogram window is closed.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.