Improve editor reuse safety and harden crop/save session handling (thanks to DJShortCut for these suggestions!)

faststack/app.py

@@ -1630,6 +1630,9 @@ def save_edited_image(self):
             save_image_key,
             getattr(self, "view_override_kind", None),
             self.image_editor.session_id if self.image_editor else None,
+            # Include edit revision so _on_save_finished does not clear state
+            # if the user continued editing after save was submitted.
+            getattr(self.image_editor, "_edits_rev", None),
         )
 
         if save_image_key and save_image_key in self._saving_keys:
@@ -1778,28 +1781,40 @@ def _on_save_finished(self, save_result: dict):
                 current_image_key,
                 getattr(self, "view_override_kind", None),
                 self.image_editor.session_id if self.image_editor else None,
+                getattr(self.image_editor, "_edits_rev", None),
             )
 
-            still_on_same_image = (
+            # Check whether the user is still viewing the same image/session
+            # (image key, variant kind, and session_id matching)
+            still_on_same_session = (
                 save_session_token is not None
                 and current_session_token is not None
-                and current_session_token == save_session_token
+                and len(save_session_token) >= 3
+                and current_session_token[:3] == save_session_token[:3]
             )
 
-            if still_on_same_image:
-                # Clear Editor State (release memory) — only when the
-                # editor dialog was actually open for this save.
-                if editor_was_open:
-                    if self.ui_state.isEditorOpen:
-                        self.ui_state.isEditorOpen = False
-                    # Closing triggers _on_editor_open_changed -> image_editor.clear()
-                    # but we call it explicitly here just in case they closed it manually.
-                    self.image_editor.clear()
-
-                # Call this regardless of editor_was_open IF it was a restore-override
-                if save_result.get("started_from_restore_override"):
-                    self._clear_variant_override()
+            # Check if it is the EXACT identical revision (no user edits since save started)
+            still_on_identical_revision = (
+                still_on_same_session
+                and len(save_session_token) >= 4
+                and current_session_token[3] == save_session_token[3]
+            )
 
+            if still_on_same_session:
+                # 1. Editor Cleanup (only if revision is unchanged)
+                if still_on_identical_revision:
+                    if editor_was_open:
+                        if self.ui_state.isEditorOpen:
+                            self.ui_state.isEditorOpen = False
+                        # Closing triggers _on_editor_open_changed -> image_editor.clear()
+                        # but we call it explicitly here just in case they closed it manually.
+                        self.image_editor.clear()
+
+                    # Also clear variant override if we started from one
+                    if save_result.get("started_from_restore_override"):
+                        self._clear_variant_override()
+
+                # 2. Update variants and re-select index
                 # Refresh list to pick up new backup files and update variant map
                 self.refresh_image_list()
 
@@ -5956,6 +5971,13 @@ def load_image_for_editing(self):
                 except (OSError, ValueError):
                     pass
 
+            if match:
+                # Also require an intact float buffer — a preview_only load leaves
+                # current_filepath/mtime set but float_image=None, which breaks
+                # crop, darken, and full-editor flows that need the master buffer.
+                if getattr(self.image_editor, "float_image", None) is None:
+                    match = False
+
             if match:
                 log.debug(
                     "load_image_for_editing: Reusing existing session for %s", filepath
@@ -6823,34 +6845,48 @@ def cancel_crop_mode(self):
         """Cancel crop mode without applying changes."""
         if self.ui_state.isCropping:
             self.ui_state.isCropping = False
-            self.ui_state.currentCropBox = [0, 0, 1000, 1000]
-            # Ensure preview rotation is cleared
+            self.ui_state.currentCropBox = (0, 0, 1000, 1000)
+            # Ensure backend crop state and preview rotation are cleared
+            self.image_editor.set_crop_box(None)
             self.image_editor.set_edit_param("straighten_angle", 0.0)
-            # Force QML to refresh if it's showing provider preview frames
+            # Notify UI and kick fresh render
             self.ui_refresh_generation += 1
-            self.ui_state.currentImageSourceChanged.emit()
+            self._kick_preview_worker()
             self.update_status_message("Crop cancelled")
 
     @Slot()
     def toggle_crop_mode(self):
         """Toggle crop mode on/off."""
-        self.ui_state.isCropping = not self.ui_state.isCropping
-
         if self.ui_state.isCropping:
-            # Entering crop mode: reset to full image defaults
+            # Exiting crop mode: reuse the specialized cleanup
+            self.cancel_crop_mode()
+        else:
+            # Entering crop mode requires a loaded image with a valid float buffer.
+            if not self.image_files or not (
+                0 <= self.current_index < len(self.image_files)
+            ):
+                self.update_status_message("No image to crop")
+                return
+
+            # Block if a save is already in progress for this image.
+            current_path = self.image_files[self.current_index].path
+            if self._block_if_saving(current_path):
+                return
+
+            if not self.load_image_for_editing():
+                return
+
+            self.ui_state.isCropping = True
+            # Reset to full image defaults (UI and Backend)
             self.ui_state.currentCropBox = (0, 0, 1000, 1000)
+            self.image_editor.set_crop_box(None)
             self.ui_state.aspectRatioNames = [r["name"] for r in ASPECT_RATIOS]
             self.ui_state.currentAspectRatioIndex = 0
 
             # Reset rotation to 0 when starting fresh crop mode
             self.image_editor.set_edit_param("straighten_angle", 0.0)
+            self._kick_preview_worker()
             self.update_status_message("Crop mode: Drag to select area, Enter to crop")
-        else:
-            # Exiting crop mode: cleanup
-            self.ui_state.currentCropBox = (0, 0, 1000, 1000)
-            # Ensure preview rotation is cleared when exiting
-            self.image_editor.set_edit_param("straighten_angle", 0.0)
-            self.update_status_message("Crop cancelled")
 
     @Slot()
     def stack_source_raws(self):

faststack/tests/test_editor_reopening.py

@@ -219,6 +219,127 @@ def test_editor_close_clears_memory_if_no_save_active(self):
         # VERIFY: Clear IS called because no save active for this file
         self.controller.image_editor.clear.assert_called_once()
 
+    def test_reuse_blocked_when_float_image_is_none(self):
+        """Matching path/mtime with float_image=None must force a real reload,
+        not silently reuse a preview-only (float-less) session."""
+        target = Path("test.jpg")
+        self.controller.image_editor.current_filepath = target
+        self.controller.image_editor.current_mtime = 123.4
+        # Simulate a preview_only load: filepath/mtime set, but no float buffer.
+        self.controller.image_editor.float_image = None
+        self.controller.image_editor.load_image.return_value = True
+
+        with patch("pathlib.Path.resolve", return_value=target.absolute()):
+            with patch("pathlib.Path.stat") as mock_stat:
+                mock_stat.return_value.st_mtime = 123.4
+                res = self.controller.load_image_for_editing()
+
+        # Must perform a real reload, not _REUSED, and return truthy.
+        self.controller.image_editor.load_image.assert_called_once()
+        self.assertIsNot(res, AppController._REUSED)
+        self.assertTrue(res)
+
+    def test_crop_mode_blocked_while_saving(self):
+        """toggle_crop_mode must not enter crop mode when a save is in flight."""
+        mock_file = MagicMock()
+        target = Path("test.jpg")
+        mock_file.path = target
+        self.controller.image_files = [mock_file]
+        self.controller.current_index = 0
+
+        # Put the image key in saving_keys
+        save_key = self.controller._key(target)
+        self.controller._saving_keys = {save_key}
+        self.controller.ui_state.isCropping = False
+
+        with patch.object(self.controller, "load_image_for_editing") as mock_load:
+            self.controller.toggle_crop_mode()
+
+        # isCropping must remain False
+        self.assertFalse(self.controller.ui_state.isCropping)
+        mock_load.assert_not_called()
+
+    def test_crop_mode_blocked_when_load_fails(self):
+        """toggle_crop_mode must not set isCropping when load_image_for_editing fails."""
+        self.controller._saving_keys = set()
+        self.controller.ui_state.isCropping = False
+
+        with patch.object(
+            self.controller, "load_image_for_editing", return_value=False
+        ):
+            self.controller.toggle_crop_mode()
+
+        self.assertFalse(self.controller.ui_state.isCropping)
+
+    def test_crop_mode_blocked_no_image(self):
+        """toggle_crop_mode must not enter crop mode if no image is available."""
+        self.controller.image_files = []
+        self.controller.current_index = -1
+        self.controller.ui_state.isCropping = False
+
+        with patch.object(self.controller, "update_status_message") as mock_msg:
+            self.controller.toggle_crop_mode()
+
+        self.assertFalse(self.controller.ui_state.isCropping)
+        mock_msg.assert_called_with("No image to crop")
+
+    def test_save_finished_does_not_clear_editor_when_edits_rev_advanced(self):
+        """If _edits_rev changed after save started, _on_save_finished must not
+        call image_editor.clear() — the user has unsaved changes."""
+        target = Path("test.jpg")
+        target_abs = self.controller._key(target)
+        self.controller.image_editor.current_filepath = target
+        self.controller.image_editor.session_id = "sess-1"
+        # _edits_rev at save-start was 5; user bumped it to 6 during the save
+        save_rev = 5
+        self.controller.image_editor._edits_rev = 6  # newer than save token
+
+        save_result = {
+            "success": True,
+            "result": (target, None),
+            "target": target_abs,
+            "save_image_key": target_abs,
+            "session_token": (target_abs, None, "sess-1", save_rev),
+            "editor_was_open": True,
+            "started_from_restore_override": False,
+        }
+
+        # Patch list/refresh helpers the handler calls
+        with patch.object(self.controller, "refresh_image_list"):
+            with patch.object(self.controller, "sync_ui_state"):
+                self.controller._on_save_finished(save_result)
+
+        # Token mismatch on _edits_rev → still_on_same_image is False → no clear
+        self.controller.image_editor.clear.assert_not_called()
+
+    def test_save_finished_clears_editor_when_edits_rev_unchanged(self):
+        """Normal save completion (no edits made during save) must still clear
+        editor memory — the 4-part tokens are equal so still_on_same_image is True."""
+        target = Path("test.jpg")
+        target_abs = self.controller._key(target)
+        self.controller.image_editor.current_filepath = target
+        self.controller.image_editor.session_id = "sess-1"
+        # _edits_rev same at save-start and now — user did not edit during save
+        rev = 5
+        self.controller.image_editor._edits_rev = rev
+
+        save_result = {
+            "success": True,
+            "result": (target, None),
+            "target": target_abs,
+            "save_image_key": target_abs,
+            "session_token": (target_abs, None, "sess-1", rev),
+            "editor_was_open": True,
+            "started_from_restore_override": False,
+        }
+
+        with patch.object(self.controller, "refresh_image_list"):
+            with patch.object(self.controller, "sync_ui_state"):
+                self.controller._on_save_finished(save_result)
+
+        # Tokens equal → still_on_same_image is True → editor_was_open → clear called
+        self.controller.image_editor.clear.assert_called_once()
+
 
 if __name__ == "__main__":
     unittest.main()