Refactor ROI Handling: removed rename_roi and remove_roi from Model (#…

…2435)

mantidimaging/gui/windows/spectrum_viewer/model.py

@@ -500,36 +500,6 @@ def export_spectrum_to_rits(self, path: Path, tof: np.ndarray, transmission: np.
         rits_data = saver.create_rits_format(tof, transmission, absorption)
         saver.export_to_dat_rits_format(rits_data, path)
 
-    def remove_roi(self, roi_name: str) -> None:
-        """
-        Remove the selected ROI from the model
-
-        @param roi_name: The name of the ROI to remove
-        """
-        if roi_name in self._roi_ranges.keys():
-            if roi_name in self.special_roi_list:
-                raise RuntimeError(f"Cannot remove ROI: {roi_name}")
-            del self._roi_ranges[roi_name]
-        else:
-            raise KeyError(
-                f"Cannot remove ROI {roi_name} as it does not exist. \n Available ROIs: {self._roi_ranges.keys()}")
-
-    def rename_roi(self, old_name: str, new_name: str) -> None:
-        """
-        Rename the selected ROI from the model
-
-        @param old_name: The current name of the ROI
-        @param new_name: The new name of the ROI
-        @raises KeyError: If the ROI does not exist
-        @raises RuntimeError: If the ROI is 'all'
-        """
-        if old_name in self._roi_ranges.keys() and new_name not in self._roi_ranges.keys():
-            if old_name in self.special_roi_list:
-                raise RuntimeError(f"Cannot remove ROI: {old_name}")
-            self._roi_ranges[new_name] = self._roi_ranges.pop(old_name)
-        else:
-            raise KeyError(f"Cannot rename {old_name} to {new_name} Available:{self._roi_ranges.keys()}")
-
     def remove_all_roi(self) -> None:
         """
         Remove all ROIs from the model

mantidimaging/gui/windows/spectrum_viewer/presenter.py

@@ -376,7 +376,6 @@ def rename_roi(self, old_name: str, new_name: str) -> None:
         @param new_name: New name of the ROI
         """
         self.view.spectrum_widget.rename_roi(old_name, new_name)
-        self.model.rename_roi(old_name, new_name)
 
     def do_remove_roi(self, roi_name: str | None = None) -> None:
         """
@@ -391,7 +390,6 @@ def do_remove_roi(self, roi_name: str | None = None) -> None:
             self.model.remove_all_roi()
         else:
             self.view.spectrum_widget.remove_roi(roi_name)
-            self.model.remove_roi(roi_name)
 
     def handle_export_tab_change(self, index: int) -> None:
         self.export_mode = ExportMode(index)

mantidimaging/gui/windows/spectrum_viewer/spectrum_widget.py

@@ -250,9 +250,13 @@ def remove_roi(self, roi_name: str) -> None:
 
         @param roi_name: The name of the ROI to remove.
         """
-        if roi_name in self.roi_dict.keys() and roi_name != "all":
-            self.image.vb.removeItem(self.roi_dict[roi_name])
-            del self.roi_dict[roi_name]
+        if roi_name == "all":
+            raise RuntimeError("Cannot remove the default 'all' ROI.")
+        if roi_name not in self.roi_dict:
+            raise KeyError(f"ROI '{roi_name}' does not exist.")
+
+        self.image.vb.removeItem(self.roi_dict[roi_name])
+        del self.roi_dict[roi_name]
 
     def rename_roi(self, old_name: str, new_name: str) -> None:
         """
@@ -263,10 +267,15 @@ def rename_roi(self, old_name: str, new_name: str) -> None:
         @param new_name: The new name of the ROI.
         @raise KeyError: If the new name is already in use or equal to 'roi' or 'all'.
         """
-        if old_name in self.roi_dict.keys() and new_name not in self.roi_dict.keys():
-            self.roi_dict[new_name] = self.roi_dict.pop(old_name)
-            self.spectrum_data_dict[new_name] = self.spectrum_data_dict.pop(old_name)
-            self.roi_dict[new_name].rename_roi(new_name)
+
+        if old_name not in self.roi_dict.keys():
+            raise RuntimeError(f"Cannot rename non-existent ROI: {old_name}")
+        if new_name in self.roi_dict.keys():
+            raise KeyError(f"New ROI name '{new_name}' is invalid or already in use.")
+
+        self.roi_dict[new_name] = self.roi_dict.pop(old_name)
+        self.spectrum_data_dict[new_name] = self.spectrum_data_dict.pop(old_name)
+        self.roi_dict[new_name].rename_roi(new_name)
 
 
 class CustomViewBox(ViewBox):

mantidimaging/gui/windows/spectrum_viewer/test/model_test.py

@@ -392,26 +392,6 @@ def test_WHEN_stack_value_set_THEN_can_export_returns_(self, _, image_stack, exp
         self.model.set_stack(image_stack)
         self.assertEqual(self.model.has_stack(), expected)
 
-    def test_WHEN_roi_removed_THEN_roi_name_removed_from_list_of_roi_names(self):
-        self.model.set_stack(generate_images())
-        rois = ["roi", "new_roi"]
-        for roi in rois:
-            self.model.set_new_roi(roi)
-        self.assertListEqual(list(self.model._roi_ranges.keys()), ["all"] + rois)
-        self.model.remove_roi("new_roi")
-        self.assertListEqual(list(self.model._roi_ranges.keys()), ["all", "roi"])
-
-    def test_WHEN_remove_roi_called_with_default_roi_THEN_raise_runtime_error(self):
-        self.model.set_stack(generate_images())
-        with self.assertRaises(RuntimeError):
-            self.model.remove_roi("all")
-        self.assertListEqual(list(self.model._roi_ranges.keys()), ["all"])
-
-    def test_WHEN_invalid_roi_removed_THEN_keyerror_raised(self):
-        self.model.set_stack(generate_images())
-        with self.assertRaises(KeyError):
-            self.model.remove_roi("non_existent_roi")
-
     def test_WHEN_remove_all_rois_called_THEN_all_but_default_rois_removed(self):
         self.model.set_stack(generate_images())
         rois = ["new_roi", "new_roi_2"]
@@ -421,24 +401,6 @@ def test_WHEN_remove_all_rois_called_THEN_all_but_default_rois_removed(self):
         self.model.remove_all_roi()
         self.assertListEqual(list(self.model._roi_ranges.keys()), [])
 
-    def test_WHEN_roi_renamed_THEN_roi_name_changed_in_list_of_roi_names(self):
-        self.model.set_stack(generate_images())
-        self.model.set_new_roi("new_roi")
-        self.assertListEqual(list(self.model._roi_ranges.keys()), ["all", "new_roi"])
-        self.model.rename_roi("new_roi", "imaging_is_the_coolest")
-        self.assertListEqual(list(self.model._roi_ranges.keys()), ["all", "imaging_is_the_coolest"])
-
-    def test_WHEN_invalid_roi_renamed_THEN_keyerror_raised(self):
-        self.model.set_stack(generate_images())
-        with self.assertRaises(KeyError):
-            self.model.rename_roi("non_existent_roi", "imaging_is_the_coolest")
-
-    def test_WHEN_default_roi_renamed_THEN_runtime_error_raised(self):
-        self.model.set_stack(generate_images())
-        self.assertListEqual(list(self.model._roi_ranges.keys()), ["all"])
-        with self.assertRaises(RuntimeError):
-            self.model.rename_roi("all", "imaging_is_the_coolest")
-
     def test_WHEN_no_stack_tof_THEN_time_of_flight_none(self):
         # No Stack
         self.model.set_stack(None)

mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py

@@ -259,13 +259,11 @@ def test_WHEN_do_add_roi_to_table_called_THEN_roi_added_to_table(self):
         self.view.add_roi_table_row.assert_called_once_with("roi_1", (255, 0, 0))
 
     def test_WHEN_do_remove_roi_called_THEN_roi_removed(self):
-        self.presenter.model.set_new_roi("all")
-        self.presenter.view.spectrum_widget.add_roi(self.presenter.model._roi_ranges["all"], "all")
-        for _ in range(2):
-            self.presenter.do_add_roi()
-        self.assertEqual(["all", "roi", "roi_1"], list(self.presenter.model._roi_ranges.keys()))
+        self.presenter.view.spectrum_widget.roi_dict = {"all": mock.Mock(), "roi": mock.Mock(), "roi_1": mock.Mock()}
+        self.presenter.view.spectrum_widget.remove_roi = mock.Mock()
         self.presenter.do_remove_roi("roi_1")
-        self.assertEqual(["all", "roi"], list(self.presenter.model._roi_ranges.keys()))
+
+        self.presenter.view.spectrum_widget.remove_roi.assert_called_once_with("roi_1")
 
     def test_WHEN_roi_clicked_THEN_roi_updated(self):
         roi = SpectrumROI("themightyroi", SensibleROI())
@@ -284,21 +282,20 @@ def test_WHEN_rits_roi_clicked_THEN_rois_not_updated(self):
 
     def test_WHEN_ROI_renamed_THEN_roi_renamed(self):
         rois = ["all", "roi", "roi_1"]
-        self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois}
-        self.presenter.model._roi_ranges = {roi: mock.Mock() for roi in rois}
+        self.view.spectrum_widget.rois = {roi: mock.Mock() for roi in rois}
         self.view.spectrum_widget.rename_roi = mock.Mock()
         self.presenter.rename_roi("roi_1", "new_name")
+
         self.view.spectrum_widget.rename_roi.assert_called_once_with("roi_1", "new_name")
-        self.assertIn("new_name", self.presenter.model._roi_ranges)
-        self.assertNotIn("roi_1", self.presenter.model._roi_ranges)
 
-    @parameterized.expand([("all", ), ("roi", )])
-    def test_WHEN_ROI_renamed_to_existing_name_THEN_runtimeerror(self, name):
+    def test_WHEN_invalid_ROI_renamed_THEN_error_raised(self):
         rois = ["all", "roi", "roi_1"]
         self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois}
         self.presenter.model._roi_ranges = {roi: mock.Mock() for roi in rois}
+        self.view.spectrum_widget.rename_roi = mock.Mock(side_effect=KeyError("Invalid ROI"))
+        self.view.spectrum_widget.rois = {roi: mock.Mock() for roi in rois}
         with self.assertRaises(KeyError):
-            self.presenter.rename_roi("roi", name)
+            self.presenter.rename_roi("invalid_roi", "new_name")
 
     def test_WHEN_do_remove_roi_called_with_no_arguments_THEN_all_rois_removed(self):
         rois = ["all", "roi", "roi_1", "roi_2"]

mantidimaging/gui/windows/spectrum_viewer/test/spectrum_test.py

@@ -132,15 +132,46 @@ def test_WHEN_rename_roi_called_THEN_roi_renamed(self):
         self.assertNotIn("roi_1", self.spectrum_widget.roi_dict)
         self.assertIn("roi_2", self.spectrum_widget.roi_dict)
 
-    def test_WHEN_rename_roi_called_with_default_roi_THEN_roi_name_not_changed(self):
+    def test_WHEN_rename_roi_called_with_default_roi_THEN_keyerror_is_raised(self):
         spectrum_roi = SpectrumROI("roi_1", self.sensible_roi, rotatable=False, scaleSnap=True, translateSnap=True)
         self.spectrum_widget.roi_dict["roi"] = spectrum_roi
         self.spectrum_widget.roi_dict["roi_1"] = spectrum_roi
         self.spectrum_widget.spectrum_data_dict["roi_1"] = np.array([0, 0, 0, 0])
-        self.assertIn("roi_1", self.spectrum_widget.roi_dict.keys())
-        self.spectrum_widget.rename_roi("roi_1", "roi")
-        self.assertIn("roi_1", self.spectrum_widget.roi_dict)
+        with self.assertRaises(KeyError):
+            self.spectrum_widget.rename_roi("roi_1", "roi")
 
     def test_WHEN_tof_axis_label_changed_THEN_axis_label_set(self):
         self.spectrum_plot_widget.set_tof_axis_label("test")
         self.assertEqual(self.spectrum_plot_widget.spectrum.getAxis('bottom').labelText, "test")
+
+    def test_WHEN_roi_removed_THEN_roi_name_removed_from_list_of_roi_names(self):
+        spectrum_roi = SpectrumROI("new_roi", self.sensible_roi, rotatable=False, scaleSnap=True, translateSnap=True)
+        self.spectrum_widget.roi_dict = {"all": mock.Mock(), "roi": mock.Mock(), "new_roi": spectrum_roi}
+        self.spectrum_widget.image.vb = mock.Mock()
+
+        self.spectrum_widget.remove_roi("new_roi")
+        self.assertListEqual(list(self.spectrum_widget.roi_dict.keys()), ["all", "roi"])
+
+    def test_WHEN_remove_roi_called_with_default_roi_THEN_raise_runtime_error(self):
+        self.spectrum_widget.roi_dict = {"all": mock.Mock(), "roi": mock.Mock()}
+        with self.assertRaises(RuntimeError):
+            self.spectrum_widget.remove_roi("all")
+
+    def test_WHEN_invalid_roi_removed_THEN_keyerror_raised(self):
+        self.spectrum_widget.roi_dict = {"all": mock.Mock(), "roi": mock.Mock()}
+        with self.assertRaises(KeyError):
+            self.spectrum_widget.remove_roi("non_existent_roi")
+
+    def test_WHEN_rename_non_existent_roi_THEN_runtime_error(self):
+        self.spectrum_widget.roi_dict = {"roi_1": mock.Mock(), "roi_2": mock.Mock()}
+        self.spectrum_widget.spectrum_data_dict = {"roi_1": mock.Mock(), "roi_2": mock.Mock()}
+
+        with self.assertRaises(RuntimeError):
+            self.spectrum_widget.rename_roi("roi_invalid", "roi_new")
+
+    def test_WHEN_rename_to_existing_roi_name_THEN_key_error(self):
+        self.spectrum_widget.roi_dict = {"roi_1": mock.Mock(), "roi_2": mock.Mock()}
+        self.spectrum_widget.spectrum_data_dict = {"roi_1": mock.Mock(), "roi_2": mock.Mock()}
+
+        with self.assertRaises(KeyError):
+            self.spectrum_widget.rename_roi("roi_1", "roi_2")