Release version 1.5.1#32
Conversation
WalkthroughThis PR introduces comprehensive RAW image editing support via RawTherapee CLI, including 16-bit aware image processing with float-based linear-space editing, JPEG vs RAW mode switching with UI synchronization, robust EXIF/orientation handling, RAW/JPEG pairing and developed-image discovery, and extensive test coverage validating the complete pipeline. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UIState/QML
participant AppController
participant ImageEditor
participant RawTherapee as RawTherapee CLI
participant Filesystem
User->>UI: Click "Develop RAW"
UI->>AppController: enable_raw_editing()
AppController->>AppController: get_active_edit_path(index)
alt RAW mode + no working TIFF
AppController->>AppController: _develop_raw_backend()
AppController->>Filesystem: Check intermediate TIFF
AppController->>RawTherapee: rawtherapee-cli input.orf -o output.tif
RawTherapee->>Filesystem: Create output.tif
AppController->>Filesystem: Verify & persist output.tif
end
AppController->>ImageEditor: load_image(working_tif_path, source_exif)
ImageEditor->>Filesystem: Read TIFF (16-bit)
ImageEditor->>ImageEditor: Convert to float32 [0,1]
ImageEditor->>UI: Emit editSourceModeChanged("raw")
UI->>UI: Update hasRaw, isRawActive
User->>UI: Adjust exposure slider
UI->>ImageEditor: set_edit_param("exposure", value)
ImageEditor->>ImageEditor: Apply edit in linear space
ImageEditor->>ImageEditor: _apply_edits(for_preview=True)
ImageEditor->>UI: Emit previewChanged
UI->>UI: Update preview image
User->>UI: Click Save
UI->>ImageEditor: save_image(write_developed_jpg=True)
ImageEditor->>ImageEditor: _write_tiff_16bit(working_tif_path)
ImageEditor->>ImageEditor: Convert linear → sRGB for JPEG
ImageEditor->>Filesystem: Write developed-*.jpg
ImageEditor->>Filesystem: Preserve EXIF orientation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span multiple domains (image processing, threading, file I/O, UI state) with dense logic in core files (app.py: +648 lines, editor.py: +679 lines), introducing RAW workflow control flow, 16-bit color space math, EXIF serialization edge cases, and complex image discovery with developed-image pairing. While individual test files are repetitive, the heterogeneous nature of core changes (subprocess management, float-space editing, TIFF I/O, Qt signal synchronization) and integration points across AppController, ImageEditor, and UI require careful architectural review. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
faststack/config.py (1)
37-45: Version sorting can select the wrong RawTherapee install due to digits in directory names.The
natural_sort_keyfunction sorts the full path, which causes digits in parent directories (e.g., "86" from "Program Files (x86)") to interfere with version comparison. When both x64 (5.9) and x86 (5.10) installations exist, the digit "86" can dominate the sort, incorrectly selecting the x64 version even if x86 has a higher version number.The proposed fix requires a correction: Use
PureWindowsPathinstead ofPathto properly parse Windows paths on all platforms, and add the necessary import.🛠️ Corrected fix
+ from pathlib import PureWindowsPath + # Helper to extract version numbers for natural sorting # e.g., "5.10" -> [5, 10] - def natural_sort_key(path): - return [int(c) if c.isdigit() else c.lower() for c in re.split(r'(\d+)', path)] + def version_sort_key(path): + for part in reversed(PureWindowsPath(path).parts): + if re.fullmatch(r'\d+(?:\.\d+)*', part): + return [int(n) for n in part.split(".")] + return [0] - matches.sort(key=natural_sort_key, reverse=True) + matches.sort(key=version_sort_key, reverse=True)faststack/tests/test_pairing.py (1)
40-50: Missing assertion for the 4th image (orphaned RAW).The test now expects 4 images due to the new orphaned RAW handling, but there's no assertion verifying that
IMG_0004.CR3is correctly included as the 4th image with appropriate properties.Proposed addition after line 50
assert images[2].path.name == "IMG_0003.jpeg" assert images[2].raw_pair is None + + # Orphaned RAW (no matching JPG) + assert images[3].path.name == "IMG_0004.CR3" + assert images[3].raw_pair == images[3].path # Points to itselffaststack/app.py (1)
3116-3175: Duplicate AppController method definitions override RAW-aware behaviorThis class defines
reset_edit_parameters,save_edited_image,rotate_image_cw/ccw, andauto_levelstwice; the later definitions override the earlier RAW-aware versions (around Lines 737–828 and 795–828). The RAW sidecar save path and updated auto-levels logic will never run, which is a functional regression. Please consolidate into single definitions and remove the duplicates.Also applies to: 3757-3858
🤖 Fix all issues with AI agents
In `@ChangeLog.md`:
- Around line 5-20: The changelog shows release 1.5.1 but the packaging metadata
still lists 1.4 — update the project's version metadata to 1.5.1 by editing the
version field in pyproject.toml (the "version" key under your packaging tool
section), and ensure any other canonical version symbols (e.g., a package
__version__ or tool-specific version entry) match 1.5.1 so packaging and
changelog are aligned.
In `@faststack/qml/ImageEditorDialog.qml`:
- Around line 481-494: The isResetting flag is being cleared immediately after
starting resetTimer, which allows synthetic release events when the slider is
re-enabled to bypass the guard; keep isResetting true until the timer callback
completes the reset. Modify the reset sequence around slider.enabled,
resetTimer.start(), and isResetting so that isResetting is only set to false
inside the resetTimer's onTriggered/timeout handler (after re-enabling slider
and any _lastSentValue/_pendingValue updates), and ensure the timer callback
clears isResetting and stops the timer; update references to isResetting,
resetTimer.start(), and the timer's handler to implement this ordering.
In `@faststack/tests/test_editor_integration.py`:
- Around line 87-88: Remove the extra leading space on the line that calls
self.fail so the indentation matches the surrounding block; specifically fix the
conditional in the test where it checks hasattr(self.controller,
'update_histogram') so the self.fail("AppController is missing method
'update_histogram'") call is aligned with the if statement block and uses the
same indentation level as the if-body instead of having an extra leading space.
In `@faststack/tests/test_editor_lifecycle_and_safety.py`:
- Around line 74-75: The assertion inside the with-block has an extra leading
space causing inconsistent indentation; in the test method adjust the line that
calls self.assertIsNone(self.controller._last_rendered_preview) so it is
indented one level inside the with self.controller._preview_lock: block (align
with the with-block body) ensuring the call to self.assertIsNone references
self.controller._last_rendered_preview correctly.
- Line 10: Update the sys.path modification in test_editor_integration.py so the
repository root is on sys.path: change the Path(...).parents index from
parents[1] to parents[2] where sys.path.append(str(Path(__file__).parents[1]))
is used, matching the pattern in other tests (e.g., test_exif_orientation.py) to
ensure the import from faststack.app (AppController) resolves correctly.
In `@faststack/tests/test_exif_compat.py`:
- Around line 4-13: Remove the duplicated import of Path from pathlib in the
top-of-file imports: keep a single "from pathlib import Path" and delete the
second occurrence to avoid shadowing/redefinition flagged by Ruff; update the
import block near the top where Path is currently imported twice (the two "from
pathlib import Path" lines) so only one remains and ensure subsequent code still
references Path as before.
In `@faststack/tests/test_raw_pipeline.py`:
- Around line 240-264: The test_develop_raw_slot is incomplete (ends with pass)
and should actually exercise AppController.develop_raw_for_current_image and/or
AppController._develop_raw_backend: use the existing mocks (mock_config_get,
mock_exists, mock_run) and the MagicMock app to call the unbound method(s) with
the prepared self (app and image_file/current_index), then assert expected
interactions — e.g. that subprocess.run (mock_run) was called with the expected
rawtherapee-cli command and arguments, that app.update_status_message or
app.load_image_for_editing were invoked appropriately, and that the code path
for mock_run.return_value.returncode == 0 behaves as expected; update the test
to perform these calls and assertions rather than leaving pass so the test
validates behavior.
- Around line 100-111: The test test_develop_raw_timeout is incomplete — patch
QTimer.singleShot so the timeout handler runs immediately, then run
app._develop_raw_backend(), assert mock_run was called with a "timeout" kwarg
equal to 60, and assert that app._on_develop_finished (or the mocked wrapper)
was called with False; specifically, use unittest.mock.patch (or monkeypatch) to
replace QTimer.singleShot to invoke the provided callable immediately, ensure
mock_run is inspected via mock_run.call_args and that
mock_on_develop_finished.assert_called_once_with(False), and restore/unpatch
QTimer after the test (or use a context manager) so other tests are unaffected.
In `@faststack/ui/provider.py`:
- Around line 694-700: The editorFilename property should guard against
editor.current_filepath being a str by normalizing it to a pathlib.Path before
accessing .name: in editorFilename (accessing self.app_controller.image_editor
and editor.current_filepath) check editor and editor.current_filepath, convert
with Path(editor.current_filepath) and then return its .name; ensure
pathlib.Path is imported and still return "" when there is no editor or no
filepath.
In `@inspect_app.py`:
- Around line 5-11: The loop over inspect.getmembers(AppController,
predicate=inspect.isfunction) uses an unused variable named method which
triggers Ruff B007; update the for-loop signature from "for name, method in
methods:" to use a throwaway identifier (e.g., "for name, _ in methods:" or "for
name, _method in methods:") so only the used variable name remains referenced;
keep the rest of the logic (the 'auto_level' check, printing and found flag)
unchanged.
In `@repro_crash.py`:
- Around line 7-13: The global mutation of sys.modules for 'cv2', 'PIL', and
'PySide6.QtGui' must be scoped so mocks don't leak; before assigning MagicMock
to sys.modules['cv2'], sys.modules['PIL'], and sys.modules['PySide6.QtGui'] save
the originals (via sys.modules.get) into local variables, set the mocks, import
ImageEditor from faststack.imaging.editor, and then restore each sys.modules
entry to its original value (or delete the key if original was None) so the
temporary mocks are removed immediately after the import.
In `@tests/verify_manual.py`:
- Around line 50-60: The test assigns to the read-only Path.suffix (in the
img_jpg and img_raw setup), which raises AttributeError; fix by either setting
the Path objects to full filenames with the desired suffixes (e.g., img_jpg.path
= Path("test.jpg") and img_raw.path = Path("orphan.CR2") without assigning
.suffix) or make path a MagicMock when you need to mutate suffix (e.g.,
img_jpg.path = MagicMock() and set .suffix on that mock); update the setup for
img_jpg and img_raw (and keep img_jpg.raw_pair, img_jpg.working_tif_path,
img_jpg.has_working_tif as appropriate) so no assignment to Path.suffix occurs.
In `@tests/verify_raw_mode.py`:
- Around line 23-37: The DummyController class currently has a duplicated
__init__ and lacks the editSourceModeChanged attribute used by
enable_raw_editing and _set_current_index; remove the duplicate __init__ so only
one initializer remains (preserving current_edit_source_mode, image_files,
current_index, and ui_state setup) and add a simple stub attribute/method named
editSourceModeChanged (e.g., a callable or MagicMock) on DummyController so
calls to emit/notify editSourceModeChanged do not raise AttributeError during
tests.
- Around line 75-90: In setUp, remove the invalid assignments to the read-only
Path.suffix property for the test image mocks (the lines setting
self.img_jpg.path.suffix and self.img_raw_only.path.suffix); instead rely on the
suffix derived from Path("test.jpg") and Path("orphan.CR2"). Update the setup
for DummyController test fixtures (function setUp) so img_jpg and img_raw_only
only set .path, .raw_pair, .working_tif_path, and .has_working_tif without
attempting to assign to Path.suffix.
- Around line 118-133: The test creates the MagicMock for
controller._develop_raw_backend after calling controller.enable_raw_editing(),
so the assertion verifies a different (non-monitored) object; move the mock
setup for _develop_raw_backend to before calling
controller.enable_raw_editing(), keep the patch of is_valid_working_tif and the
mock for load_image_for_editing as-is, then call enable_raw_editing() and assert
that current_edit_source_mode == "raw",
_develop_raw_backend.assert_not_called(), and
load_image_for_editing.assert_called_once() to ensure the actual backend method
is monitored during the call.
🧹 Nitpick comments (20)
faststack/tests/test_raw_pipeline.py (2)
205-223: Consider movingsetUpandtearDownto the top of the test class.While Python allows method definitions in any order, placing
setUpandtearDownbefore test methods is the conventional pattern and improves readability.
293-302: Unused variable and potential test fragility.The
backup_pathvariable is unpacked but never used (as flagged by static analysis). Additionally, the test manipulateseditor.current_filepathdirectly to "trick" the editor, which couples the test to implementation details.Proposed fix for unused variable
- saved_path, backup_path = res + saved_path, _backup_path = resfaststack/io/indexer.py (1)
86-91: Unused loop variablestem.The loop iterates over
raws.items()but only usesraw_list. This is flagged by static analysis (B007).Proposed fix
- for stem, raw_list in raws.items(): + for _stem, raw_list in raws.items(): for raw_path, raw_stat in raw_list:reproduce_bug.py (3)
1-67: Debug script should be converted to a proper test or removed.This script appears to be a manual reproduction/debug script for a refresh logic bug. Consider:
- Moving the test logic to
faststack/tests/as a proper pytest test with assertions instead of print statements- Using
tmp_pathfixture for automatic cleanup- Removing this file if the bug is fixed and covered by
test_developed_sorting.pyHaving debug scripts at the repository root adds maintenance burden.
36-38: Unused variablet1.The variable is assigned but never used (as flagged by static analysis). The
touch()call alone updates the mtime.Proposed fix
# 2. Save Main (update mtime to T1) - t1 = time.time() img_path.touch() # Updates mtime
63-64: Cleanup may not run on failure.If an exception occurs before line 64, the test directory won't be cleaned up. Use
try/finallyor a context manager.Proposed fix using try/finally
def test_refresh_logic(): # Setup test dir test_dir = Path("./test_images_refresh") if test_dir.exists(): shutil.rmtree(test_dir) test_dir.mkdir() + + try: + # ... rest of test logic indented ... + finally: + # Cleanup + if test_dir.exists(): + shutil.rmtree(test_dir) - - # Cleanup - shutil.rmtree(test_dir)faststack/tests/test_drag_logic.py (1)
10-23: Drop or underscore the unusedcurrent_indexparameter.It’s not referenced, and Ruff flags it. If you want to keep parity with app signature, prefix it with
_.♻️ Suggested tweak
-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/imaging/editor.py (1)
448-471: Remove unused tuple unpacking in_rotate_float_image.Ruff flags unused
h,w,nw,nh. You can drop them or use_to avoid noise.♻️ Suggested tweak
- h, w, c = img_arr.shape + _, _, c = img_arr.shape @@ - nw, nh = channels[0].size new_arr = np.stack([np.array(ch) for ch in channels], axis=-1)repro_crash.py (1)
31-37: Preserve traceback when re‑raising.Use bare
raiseto keep the original stack trace intact.♻️ Suggested tweak
- except Exception as e: - print(f"CRASHED: {e}") - raise e + except Exception as e: + print(f"CRASHED: {e}") + raiseverify_fix_auto_levels.py (1)
46-63: Remove unused variables (t1,target_path).♻️ Suggested tweak
- t1 = time.time() img_path.touch() # Updates mtime @@ - target_path = Path(saved_path).resolve() target_name = Path(saved_path).namefaststack/tests/test_editor.py (1)
4-17: Make fallbackapproxhonorreltolerance.Right now
relis unused; if fallback is used, it won’t match pytest semantics.♻️ Suggested tweak
- def approx(val, rel=None, abs=None): + def approx(val, rel=None, abs=None): class Approx: def __init__(self, expected): self.expected = expected def __eq__(self, other): - return abs_val(self.expected - other) <= (abs or 1e-6) + abs_tol = 1e-6 if abs is None else abs + rel_tol = 0.0 if rel is None else rel + tol = max(abs_tol, rel_tol * abs_val(self.expected)) + return abs_val(self.expected - other) <= tol return Approx(val)faststack/tests/test_exif_orientation.py (1)
126-128: Use the returned path fromsave_imageand drop the unused variable.This avoids assuming a filename and removes the unused assignment.
♻️ Suggested refactor
- res = self.editor.save_image(write_developed_jpg=True) - developed_path = Path(self.test_dir) / "working_source-developed.jpg" + developed_path, _ = self.editor.save_image(write_developed_jpg=True)tests/repro_exif_fix.py (1)
32-37: Narrow the exception catch for EXIF serialization.Catching
Exceptionhides unexpected issues; use the specific exceptions raised by your supported Pillow versions.♻️ Suggested refactor
- except Exception as e: + except (OSError, ValueError, AttributeError) as e:faststack/tests/test_editor_loading.py (2)
50-58: Module reimport may leave state inconsistent across tests.The approach of deleting
sys.modules['faststack.imaging.editor']and reimporting works but doesn't restore the original module after the test. This could affect subsequent tests that import the editor module.Consider storing and restoring the original module in
tearDown:♻️ Suggested improvement
def setUp(self): """Set up a fresh ImageEditor for each test.""" self.temp_files = [] + # Store original editor module if present + self._original_editor_module = sys.modules.get('faststack.imaging.editor') def tearDown(self): """Clean up temp files.""" for f in self.temp_files: try: os.unlink(f) except (OSError, PermissionError): pass + # Restore original editor module + if self._original_editor_module is not None: + sys.modules['faststack.imaging.editor'] = self._original_editor_module + elif 'faststack.imaging.editor' in sys.modules: + del sys.modules['faststack.imaging.editor']
69-83: Inconsistent assertions across test cases.
test_imread_returns_noneassertsfloat_imageis not None (line 67), buttest_imread_returns_empty_arrayandtest_imread_returns_non_arrayomit this check. Iffloat_imagebeing set is important for the fallback behavior, all tests should verify it consistently.♻️ Suggested fix
def test_imread_returns_empty_array(self): """cv2.imread returning an empty array should fall back to PIL.""" temp_path = self._create_temp_image('blue') editor, result = self._run_with_mocked_cv2(np.array([]), temp_path) self.assertTrue(result, "load_image should succeed with PIL fallback") self.assertEqual(editor.bit_depth, 8, "Should fall back to 8-bit") + self.assertIsNotNone(editor.float_image, "float_image should be set") def test_imread_returns_non_array(self): """cv2.imread returning a non-array object should fall back to PIL.""" temp_path = self._create_temp_image('green') editor, result = self._run_with_mocked_cv2("not an array", temp_path) self.assertTrue(result, "load_image should succeed with PIL fallback") self.assertEqual(editor.bit_depth, 8, "Should fall back to 8-bit") + self.assertIsNotNone(editor.float_image, "float_image should be set")faststack/tests/run_loading_tests.py (1)
13-26: Script doesn't propagate exit code for CI integration.The script prints results but doesn't exit with a non-zero code when tests fail. This could cause CI pipelines to report success when tests actually fail.
♻️ Suggested fix
for test, traceback in result.errors: print(f"\nERROR: {test}") print(traceback) + +# Exit with non-zero code if there were failures or errors +sys.exit(0 if result.wasSuccessful() else 1)faststack/tests/test_editor_integration.py (2)
1-1: Remove empty line at start of file.The file starts with an empty line before the imports, which is against PEP 8 style conventions.
39-88: Consider usinghasattrconsistently instead of try/except for method existence.The test uses try/except
AttributeErrorfor most methods but switches tohasattrforupdate_histogram. Usinghasattrconsistently would be cleaner and more explicit about the test intent.♻️ Example refactor for one method
- # 1. set_edit_parameter - # Try calling the method. If it doesn't exist, this will raise AttributeError - try: - self.controller.set_edit_parameter("exposure", 0.5) - self.controller.image_editor.set_edit_param.assert_called_with("exposure", 0.5) - except AttributeError: - self.fail("AppController is missing method 'set_edit_parameter'") + # 1. set_edit_parameter + self.assertTrue(hasattr(self.controller, 'set_edit_parameter'), + "AppController is missing method 'set_edit_parameter'") + self.controller.set_edit_parameter("exposure", 0.5) + self.controller.image_editor.set_edit_param.assert_called_with("exposure", 0.5)faststack/tests/test_editor_lifecycle_and_safety.py (2)
1-1: Remove empty line at start of file.The file starts with an empty line before the imports, which is against PEP 8 style conventions.
43-45: Remove unused variablemock_editor_cls.The
mock_editor_clsvariable is assigned but never used. You can use_as a throwaway variable or simply omit theasclause if you only need the patch active.♻️ Suggested fix
# Instantiate AppController - 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_editorBased on static analysis hint from Ruff.
| isResetting = true | ||
| controller.set_edit_parameter(model.key, 0.0) | ||
| imageEditorDialog.updatePulse++ | ||
| value = 0.0 | ||
|
|
||
| // CRITICAL FIX: Force the slider to release 'pressed' state | ||
| // to stop tracking the mouse position. | ||
| // We must wait a tick to ensure the internal state clears. | ||
| slider.enabled = false | ||
| resetTimer.start() | ||
|
|
||
| _pendingValue = 0.0 | ||
| slider._lastSentValue = 0.0 | ||
| imageEditorDialog.updatePulse++ | ||
| isResetting = false | ||
| return |
There was a problem hiding this comment.
Potential race: isResetting is cleared before resetTimer fires.
The isResetting flag is set to false at line 493, but resetTimer doesn't fire until 50ms later (lines 520-528). When the timer re-enables the slider at line 524, Qt may trigger a synthetic release event, and the guard at line 505 won't catch it since isResetting is already false.
Consider keeping isResetting = true until the timer callback:
Proposed fix
_pendingValue = 0.0
slider._lastSentValue = 0.0
imageEditorDialog.updatePulse++
- isResetting = false
return
}And in the timer:
onTriggered: {
slider.enabled = true
// Ensure value is synced
slider.value = slider.backendValue
+ slider.isResetting = false
}🤖 Prompt for AI Agents
In `@faststack/qml/ImageEditorDialog.qml` around lines 481 - 494, The isResetting
flag is being cleared immediately after starting resetTimer, which allows
synthetic release events when the slider is re-enabled to bypass the guard; keep
isResetting true until the timer callback completes the reset. Modify the reset
sequence around slider.enabled, resetTimer.start(), and isResetting so that
isResetting is only set to false inside the resetTimer's onTriggered/timeout
handler (after re-enabling slider and any _lastSentValue/_pendingValue updates),
and ensure the timer callback clears isResetting and stops the timer; update
references to isResetting, resetTimer.start(), and the timer's handler to
implement this ordering.
| if not hasattr(self.controller, 'update_histogram'): | ||
| self.fail("AppController is missing method 'update_histogram'") |
There was a problem hiding this comment.
Extra indentation on line 88.
Line 88 has extra leading space before self.fail(), causing inconsistent indentation.
🔧 Suggested fix
# This one might be complex to mock fully due to threading, but we check existence
if not hasattr(self.controller, 'update_histogram'):
- self.fail("AppController is missing method 'update_histogram'")
+ self.fail("AppController is missing method 'update_histogram'")📝 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.
| if not hasattr(self.controller, 'update_histogram'): | |
| self.fail("AppController is missing method 'update_histogram'") | |
| if not hasattr(self.controller, 'update_histogram'): | |
| self.fail("AppController is missing method 'update_histogram'") |
🤖 Prompt for AI Agents
In `@faststack/tests/test_editor_integration.py` around lines 87 - 88, Remove the
extra leading space on the line that calls self.fail so the indentation matches
the surrounding block; specifically fix the conditional in the test where it
checks hasattr(self.controller, 'update_histogram') so the
self.fail("AppController is missing method 'update_histogram'") call is aligned
with the if statement block and uses the same indentation level as the if-body
instead of having an extra leading space.
| import time | ||
|
|
||
| # Ensure we can import faststack | ||
| sys.path.append(str(Path(__file__).parents[2])) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check test files existence and their path manipulation
echo "=== Locating test files ==="
fd -t f 'test_editor' faststack/tests
echo -e "\n=== Checking path manipulation in test_editor_lifecycle_and_safety.py ==="
if [ -f "faststack/tests/test_editor_lifecycle_and_safety.py" ]; then
head -15 faststack/tests/test_editor_lifecycle_and_safety.py | cat -n
fi
echo -e "\n=== Checking path manipulation in test_editor_integration.py ==="
if [ -f "faststack/tests/test_editor_integration.py" ]; then
head -15 faststack/tests/test_editor_integration.py | cat -n
fi
echo -e "\n=== Directory structure ==="
tree -L 3 faststack/ 2>/dev/null || find faststack -type d | head -20Repository: AlanRockefeller/faststack
Length of output: 3043
🏁 Script executed:
#!/bin/bash
# Verify what parents[1] and parents[2] resolve to
python3 << 'PYTHON_EOF'
from pathlib import Path
test_file1 = Path("faststack/tests/test_editor_lifecycle_and_safety.py")
test_file2 = Path("faststack/tests/test_editor_integration.py")
print("test_editor_lifecycle_and_safety.py:")
print(f" File: {test_file1}")
print(f" parents[1] = {test_file1.parents[1]}")
print(f" parents[2] = {test_file1.parents[2]}")
print(f" (parents[1] is 'tests', parents[2] is 'faststack')")
print("\ntest_editor_integration.py:")
print(f" File: {test_file2}")
print(f" parents[1] = {test_file2.parents[1]}")
print(f" parents[2] = {test_file2.parents[2]}")
print(f" (parents[1] is 'tests', parents[2] is 'faststack')")
print("\nWhat should sys.path include?")
print(" - For importing 'faststack' module: need parent of 'faststack' dir")
print(" - If tests are in 'faststack/tests/', then parents[2] reaches the root containing 'faststack'")
print(" - If that's what's needed, parents[2] is correct, parents[1] would only add 'tests' dir")
PYTHON_EOFRepository: AlanRockefeller/faststack
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Try to verify if the imports would work with each parent index
cd faststack/tests
echo "=== Checking what each parent index resolves to ==="
python3 << 'EOF'
from pathlib import Path
import os
# Simulate the test file paths
test_file = Path("test_editor_integration.py").absolute()
test_file_lifecycle = Path("test_editor_lifecycle_and_safety.py").absolute()
print(f"Repository root (where faststack/ package exists):")
# Find the actual repo root by checking for faststack module
import sys
sys.path.insert(0, str(Path(__file__).parents[2]))
# From test files perspective:
print("\ntest_editor_integration.py (uses parents[1]):")
p1 = test_file.parents[1]
print(f" parents[1] = {p1}")
print(f" This is the 'faststack' directory")
print(f" To import 'from faststack.app': would need faststack/ in sys.path, but would fail")
print(f" To work, would need to import as 'from app' only")
print("\ntest_editor_lifecycle_and_safety.py (uses parents[2]):")
p2 = test_file.parents[2]
print(f" parents[2] = {p2}")
print(f" This is the repository root")
print(f" To import 'from faststack.app': correct - faststack is a package at repo root")
EOFRepository: AlanRockefeller/faststack
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Check all test files for their sys.path manipulation
echo "=== Checking sys.path patterns in all test files ==="
rg "sys\.path\.(append|insert)" faststack/tests --context 2 -A 1 -B 1Repository: AlanRockefeller/faststack
Length of output: 1333
Correct the parent index in test_editor_integration.py to match other test files.
The import from faststack.app import AppController requires the repository root in sys.path. Most test files correctly use parents[2], but test_editor_integration.py (line 8) incorrectly uses parents[1] (the faststack directory). This should be changed to parents[2] for consistency and correct import behavior.
Other test files using parents[2]: test_exif_orientation.py, test_exif_compat.py, and the file under review. Only test_editor_integration.py and debug_metadata.py deviate from this pattern.
🤖 Prompt for AI Agents
In `@faststack/tests/test_editor_lifecycle_and_safety.py` at line 10, Update the
sys.path modification in test_editor_integration.py so the repository root is on
sys.path: change the Path(...).parents index from parents[1] to parents[2] where
sys.path.append(str(Path(__file__).parents[1])) is used, matching the pattern in
other tests (e.g., test_exif_orientation.py) to ensure the import from
faststack.app (AppController) resolves correctly.
| with self.controller._preview_lock: | ||
| self.assertIsNone(self.controller._last_rendered_preview) |
There was a problem hiding this comment.
Extra indentation on line 75.
Line 75 has extra leading space before self.assertIsNone(), causing inconsistent indentation.
🔧 Suggested fix
# 4. Verify preview cache was cleared
with self.controller._preview_lock:
- self.assertIsNone(self.controller._last_rendered_preview)
+ self.assertIsNone(self.controller._last_rendered_preview)📝 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.
| with self.controller._preview_lock: | |
| self.assertIsNone(self.controller._last_rendered_preview) | |
| with self.controller._preview_lock: | |
| self.assertIsNone(self.controller._last_rendered_preview) |
🤖 Prompt for AI Agents
In `@faststack/tests/test_editor_lifecycle_and_safety.py` around lines 74 - 75,
The assertion inside the with-block has an extra leading space causing
inconsistent indentation; in the test method adjust the line that calls
self.assertIsNone(self.controller._last_rendered_preview) so it is indented one
level inside the with self.controller._preview_lock: block (align with the
with-block body) ensuring the call to self.assertIsNone references
self.controller._last_rendered_preview correctly.
| # Mock modules before importing editor | ||
| sys.modules['cv2'] = MagicMock() | ||
| sys.modules['PIL'] = MagicMock() | ||
| sys.modules['PySide6.QtGui'] = MagicMock() | ||
|
|
||
| # Now import the class | ||
| from faststack.imaging.editor import ImageEditor |
There was a problem hiding this comment.
Avoid global sys.modules mutations at import time.
If this module is imported (e.g., by a runner), the mocked modules persist and can contaminate other tests. Prefer scoping the mocks (or restoring originals after import).
♻️ One option: restore originals after import
-# Mock modules before importing editor
-sys.modules['cv2'] = MagicMock()
-sys.modules['PIL'] = MagicMock()
-sys.modules['PySide6.QtGui'] = MagicMock()
-
-# Now import the class
-from faststack.imaging.editor import ImageEditor
+_orig = {k: sys.modules.get(k) for k in ("cv2", "PIL", "PySide6.QtGui")}
+sys.modules["cv2"] = MagicMock()
+sys.modules["PIL"] = MagicMock()
+sys.modules["PySide6.QtGui"] = MagicMock()
+from faststack.imaging.editor import ImageEditor
+for k, v in _orig.items():
+ if v is None:
+ del sys.modules[k]
+ else:
+ sys.modules[k] = v📝 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.
| # Mock modules before importing editor | |
| sys.modules['cv2'] = MagicMock() | |
| sys.modules['PIL'] = MagicMock() | |
| sys.modules['PySide6.QtGui'] = MagicMock() | |
| # Now import the class | |
| from faststack.imaging.editor import ImageEditor | |
| _orig = {k: sys.modules.get(k) for k in ("cv2", "PIL", "PySide6.QtGui")} | |
| sys.modules["cv2"] = MagicMock() | |
| sys.modules["PIL"] = MagicMock() | |
| sys.modules["PySide6.QtGui"] = MagicMock() | |
| from faststack.imaging.editor import ImageEditor | |
| for k, v in _orig.items(): | |
| if v is None: | |
| del sys.modules[k] | |
| else: | |
| sys.modules[k] = v |
🤖 Prompt for AI Agents
In `@repro_crash.py` around lines 7 - 13, The global mutation of sys.modules for
'cv2', 'PIL', and 'PySide6.QtGui' must be scoped so mocks don't leak; before
assigning MagicMock to sys.modules['cv2'], sys.modules['PIL'], and
sys.modules['PySide6.QtGui'] save the originals (via sys.modules.get) into local
variables, set the mocks, import ImageEditor from faststack.imaging.editor, and
then restore each sys.modules entry to its original value (or delete the key if
original was None) so the temporary mocks are removed immediately after the
import.
| img_jpg = MagicMock() | ||
| img_jpg.path = Path("test.jpg") | ||
| img_jpg.path.suffix = ".jpg" | ||
| img_jpg.raw_pair = Path("test.CR2") | ||
| img_jpg.working_tif_path = Path("test.tif") | ||
| img_jpg.has_working_tif = False | ||
|
|
||
| img_raw = MagicMock() | ||
| img_raw.path = Path("orphan.CR2") | ||
| img_raw.path.suffix = ".CR2" | ||
| img_raw.raw_pair = None |
There was a problem hiding this comment.
Do not assign to Path.suffix; it’s read‑only
Path("test.jpg").suffix already returns ".jpg". Assigning to suffix will raise AttributeError and break this script. Remove those assignments (or replace Path with a MagicMock if you need a mutable suffix).
🛠️ Proposed fix
- img_jpg.path.suffix = ".jpg"
+ # Path("test.jpg").suffix is already ".jpg"; no assignment needed
...
- img_raw.path.suffix = ".CR2"
+ # Path("orphan.CR2").suffix is already ".CR2"; no assignment needed🤖 Prompt for AI Agents
In `@tests/verify_manual.py` around lines 50 - 60, The test assigns to the
read-only Path.suffix (in the img_jpg and img_raw setup), which raises
AttributeError; fix by either setting the Path objects to full filenames with
the desired suffixes (e.g., img_jpg.path = Path("test.jpg") and img_raw.path =
Path("orphan.CR2") without assigning .suffix) or make path a MagicMock when you
need to mutate suffix (e.g., img_jpg.path = MagicMock() and set .suffix on that
mock); update the setup for img_jpg and img_raw (and keep img_jpg.raw_pair,
img_jpg.working_tif_path, img_jpg.has_working_tif as appropriate) so no
assignment to Path.suffix occurs.
| class DummyController: | ||
| def __init__(self): | ||
| self.current_edit_source_mode = "jpeg" | ||
| self.image_files = [] | ||
| self.current_index = 0 | ||
| self.ui_state = MagicMock() | ||
| self.ui_state.isHistogramVisible = False | ||
|
|
||
| def __init__(self): | ||
| self.current_edit_source_mode = "jpeg" | ||
| self.image_files = [] | ||
| self.current_index = 0 | ||
| self.ui_state = MagicMock() | ||
| self.ui_state.isHistogramVisible = False | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/verify_raw_mode.py | head -50Repository: AlanRockefeller/faststack
Length of output: 2451
🏁 Script executed:
fd "app.py" -t fRepository: AlanRockefeller/faststack
Length of output: 111
🏁 Script executed:
rg "def enable_raw_editing" faststack/app.py -A 10Repository: AlanRockefeller/faststack
Length of output: 487
🏁 Script executed:
rg "def _set_current_index" faststack/app.py -A 10Repository: AlanRockefeller/faststack
Length of output: 684
🏁 Script executed:
rg "def _set_current_index" faststack/app.py -A 30Repository: AlanRockefeller/faststack
Length of output: 1522
Add editSourceModeChanged stub and remove duplicate __init__.
enable_raw_editing and _set_current_index emit editSourceModeChanged; without a stub this will raise AttributeError in tests. The duplicate __init__ also shadows the first definition.
🛠️ Proposed fix
class DummyController:
- def __init__(self):
- self.current_edit_source_mode = "jpeg"
- self.image_files = []
- self.current_index = 0
- self.ui_state = MagicMock()
- self.ui_state.isHistogramVisible = False
-
- def __init__(self):
- self.current_edit_source_mode = "jpeg"
- self.image_files = []
- self.current_index = 0
- self.ui_state = MagicMock()
- self.ui_state.isHistogramVisible = False
+ def __init__(self):
+ self.current_edit_source_mode = "jpeg"
+ self.image_files = []
+ self.current_index = 0
+ self.ui_state = MagicMock()
+ self.ui_state.isHistogramVisible = False
+ self.editSourceModeChanged = MagicMock()
+ self.editSourceModeChanged.emit = MagicMock()🧰 Tools
🪛 Ruff (0.14.13)
31-31: Redefinition of unused __init__ from line 24: __init__ redefined here
(F811)
🤖 Prompt for AI Agents
In `@tests/verify_raw_mode.py` around lines 23 - 37, The DummyController class
currently has a duplicated __init__ and lacks the editSourceModeChanged
attribute used by enable_raw_editing and _set_current_index; remove the
duplicate __init__ so only one initializer remains (preserving
current_edit_source_mode, image_files, current_index, and ui_state setup) and
add a simple stub attribute/method named editSourceModeChanged (e.g., a callable
or MagicMock) on DummyController so calls to emit/notify editSourceModeChanged
do not raise AttributeError during tests.
| def setUp(self): | ||
| self.controller = DummyController() | ||
|
|
||
| # Create mock image files | ||
| self.img_jpg = MagicMock() | ||
| self.img_jpg.path = Path("test.jpg") | ||
| self.img_jpg.path.suffix = ".jpg" | ||
| self.img_jpg.raw_pair = Path("test.CR2") | ||
| self.img_jpg.working_tif_path = Path("test.tif") | ||
| self.img_jpg.has_working_tif = False # Initially false | ||
|
|
||
| self.img_raw_only = MagicMock() | ||
| self.img_raw_only.path = Path("orphan.CR2") | ||
| self.img_raw_only.path.suffix = ".CR2" | ||
| self.img_raw_only.raw_pair = None | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the test file
find . -name "verify_raw_mode.py" -type fRepository: AlanRockefeller/faststack
Length of output: 95
🏁 Script executed:
# Let's verify Path.suffix behavior in Python
python3 << 'EOF'
from pathlib import Path
# Create a Path object
p = Path("test.jpg")
print(f"Path suffix: {p.suffix}")
print(f"Is suffix read-only? Attempting assignment...")
try:
p.suffix = ".png"
print("Assignment succeeded - suffix is NOT read-only")
except AttributeError as e:
print(f"Assignment failed with AttributeError: {e}")
EOFRepository: AlanRockefeller/faststack
Length of output: 225
🏁 Script executed:
# Now let's examine the actual test file around lines 75-90
cat -n ./tests/verify_raw_mode.py | sed -n '70,95p'Repository: AlanRockefeller/faststack
Length of output: 1169
🏁 Script executed:
# Let's verify this will actually fail by checking if the test file runs
python3 -m pytest ./tests/verify_raw_mode.py::TestRawMode::test_default_mode -v 2>&1 | head -50Repository: AlanRockefeller/faststack
Length of output: 1330
Remove the redundant suffix assignments in setUp.
Path.suffix is a read-only property; assignments at lines 81 and 88 raise AttributeError and prevent tests from running. The suffix is already derived from the filename (Path("test.jpg").suffix is automatically .jpg).
Fix
self.img_jpg = MagicMock()
self.img_jpg.path = Path("test.jpg")
- self.img_jpg.path.suffix = ".jpg"
self.img_jpg.raw_pair = Path("test.CR2")
self.img_jpg.working_tif_path = Path("test.tif")
self.img_jpg.has_working_tif = False # Initially false
self.img_raw_only = MagicMock()
self.img_raw_only.path = Path("orphan.CR2")
- self.img_raw_only.path.suffix = ".CR2"
self.img_raw_only.raw_pair = None📝 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 setUp(self): | |
| self.controller = DummyController() | |
| # Create mock image files | |
| self.img_jpg = MagicMock() | |
| self.img_jpg.path = Path("test.jpg") | |
| self.img_jpg.path.suffix = ".jpg" | |
| self.img_jpg.raw_pair = Path("test.CR2") | |
| self.img_jpg.working_tif_path = Path("test.tif") | |
| self.img_jpg.has_working_tif = False # Initially false | |
| self.img_raw_only = MagicMock() | |
| self.img_raw_only.path = Path("orphan.CR2") | |
| self.img_raw_only.path.suffix = ".CR2" | |
| self.img_raw_only.raw_pair = None | |
| def setUp(self): | |
| self.controller = DummyController() | |
| # Create mock image files | |
| self.img_jpg = MagicMock() | |
| self.img_jpg.path = Path("test.jpg") | |
| self.img_jpg.raw_pair = Path("test.CR2") | |
| self.img_jpg.working_tif_path = Path("test.tif") | |
| self.img_jpg.has_working_tif = False # Initially false | |
| self.img_raw_only = MagicMock() | |
| self.img_raw_only.path = Path("orphan.CR2") | |
| self.img_raw_only.raw_pair = None |
🤖 Prompt for AI Agents
In `@tests/verify_raw_mode.py` around lines 75 - 90, In setUp, remove the invalid
assignments to the read-only Path.suffix property for the test image mocks (the
lines setting self.img_jpg.path.suffix and self.img_raw_only.path.suffix);
instead rely on the suffix derived from Path("test.jpg") and Path("orphan.CR2").
Update the setup for DummyController test fixtures (function setUp) so img_jpg
and img_raw_only only set .path, .raw_pair, .working_tif_path, and
.has_working_tif without attempting to assign to Path.suffix.
| def test_switch_to_raw_with_existing_tiff(self): | ||
| """Test 3: Enabling RAW should load TIFF if valid.""" | ||
| self.controller.current_index = 0 | ||
| self.img_jpg.has_working_tif = True | ||
|
|
||
| # Mock is_valid_working_tif to return True | ||
| with patch.object(self.controller, 'is_valid_working_tif', return_value=True): | ||
| self.controller.load_image_for_editing = MagicMock() | ||
| self.controller.enable_raw_editing() | ||
|
|
||
| self.assertEqual(self.controller.current_edit_source_mode, "raw") | ||
| # Should NOT develop | ||
| self.controller._develop_raw_backend = MagicMock() | ||
| self.controller._develop_raw_backend.assert_not_called() | ||
| # Should load immediately | ||
| self.controller.load_image_for_editing.assert_called_once() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file to see the current state
find . -name "verify_raw_mode.py" -type fRepository: AlanRockefeller/faststack
Length of output: 95
🏁 Script executed:
# Get the file and examine the specific lines
cat -n tests/verify_raw_mode.py | sed -n '110,140p'Repository: AlanRockefeller/faststack
Length of output: 1837
🏁 Script executed:
# Also search for the enable_raw_editing method to understand what it does
rg "def enable_raw_editing" -A 30Repository: AlanRockefeller/faststack
Length of output: 1915
🏁 Script executed:
# Check if _develop_raw_backend is used in the codebase
rg "_develop_raw_backend" -B 2 -A 2Repository: AlanRockefeller/faststack
Length of output: 7488
Move _develop_raw_backend mock creation before enable_raw_editing() call.
The mock is created after the method executes, so assert_not_called() verifies the wrong instance. If the implementation changes such that the early return no longer applies, the test will still pass even if the method is called.
🛠️ Proposed fix
with patch.object(self.controller, 'is_valid_working_tif', return_value=True):
self.controller.load_image_for_editing = MagicMock()
+ self.controller._develop_raw_backend = MagicMock()
self.controller.enable_raw_editing()
self.assertEqual(self.controller.current_edit_source_mode, "raw")
# Should NOT develop
- self.controller._develop_raw_backend = MagicMock()
self.controller._develop_raw_backend.assert_not_called()📝 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_switch_to_raw_with_existing_tiff(self): | |
| """Test 3: Enabling RAW should load TIFF if valid.""" | |
| self.controller.current_index = 0 | |
| self.img_jpg.has_working_tif = True | |
| # Mock is_valid_working_tif to return True | |
| with patch.object(self.controller, 'is_valid_working_tif', return_value=True): | |
| self.controller.load_image_for_editing = MagicMock() | |
| self.controller.enable_raw_editing() | |
| self.assertEqual(self.controller.current_edit_source_mode, "raw") | |
| # Should NOT develop | |
| self.controller._develop_raw_backend = MagicMock() | |
| self.controller._develop_raw_backend.assert_not_called() | |
| # Should load immediately | |
| self.controller.load_image_for_editing.assert_called_once() | |
| def test_switch_to_raw_with_existing_tiff(self): | |
| """Test 3: Enabling RAW should load TIFF if valid.""" | |
| self.controller.current_index = 0 | |
| self.img_jpg.has_working_tif = True | |
| # Mock is_valid_working_tif to return True | |
| with patch.object(self.controller, 'is_valid_working_tif', return_value=True): | |
| self.controller.load_image_for_editing = MagicMock() | |
| self.controller._develop_raw_backend = MagicMock() | |
| self.controller.enable_raw_editing() | |
| self.assertEqual(self.controller.current_edit_source_mode, "raw") | |
| # Should NOT develop | |
| self.controller._develop_raw_backend.assert_not_called() | |
| # Should load immediately | |
| self.controller.load_image_for_editing.assert_called_once() |
🤖 Prompt for AI Agents
In `@tests/verify_raw_mode.py` around lines 118 - 133, The test creates the
MagicMock for controller._develop_raw_backend after calling
controller.enable_raw_editing(), so the assertion verifies a different
(non-monitored) object; move the mock setup for _develop_raw_backend to before
calling controller.enable_raw_editing(), keep the patch of is_valid_working_tif
and the mock for load_image_for_editing as-is, then call enable_raw_editing()
and assert that current_edit_source_mode == "raw",
_develop_raw_backend.assert_not_called(), and
load_image_for_editing.assert_called_once() to ensure the actual backend method
is monitored during the call.
1 participant
Added beta RAW support, updated image editor
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.