made suggested changes

mantidimaging/gui/windows/spectrum_viewer/presenter.py

@@ -375,14 +375,7 @@ def rename_roi(self, old_name: str, new_name: str) -> None:
         @param old_name: Name of the ROI to rename
         @param new_name: New name of the ROI
         """
-        try:
-            self.view.spectrum_widget.rename_roi(old_name, new_name)
-        except RuntimeError as err:
-            raise RuntimeError(f"Cannot remove ROI: {old_name}") from err
-        except KeyError as err:
-            raise KeyError(
-                f"Cannot rename {old_name} to {new_name}. Available: {list(self.view.spectrum_widget.rois.keys())}"
-            ) from err
+        self.view.spectrum_widget.rename_roi(old_name, new_name)
 
     def do_remove_roi(self, roi_name: str | None = None) -> None:
         """

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/presenter_test.py

@@ -264,12 +264,6 @@ def test_WHEN_do_remove_roi_called_THEN_roi_removed(self):
         self.presenter.do_remove_roi("roi_1")
 
         self.presenter.view.spectrum_widget.remove_roi.assert_called_once_with("roi_1")
-        expected_roi_dict = {"all": mock.Mock(), "roi": mock.Mock()}
-        actual_roi_dict = {
-            key: value
-            for key, value in self.presenter.view.spectrum_widget.roi_dict.items() if key != "roi_1"
-        }
-        self.assertEqual(expected_roi_dict.keys(), actual_roi_dict.keys())
 
     def test_WHEN_roi_clicked_THEN_roi_updated(self):
         roi = SpectrumROI("themightyroi", SensibleROI())
@@ -288,16 +282,11 @@ 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.view.spectrum_widget.rois = {roi: mock.Mock() for roi in rois}
-        self.view.spectrum_widget.rename_roi = mock.Mock(
-            side_effect=lambda old_name, new_name: self.view.spectrum_widget.rois.update(
-                {new_name: self.view.spectrum_widget.rois.pop(old_name)}))
-
+        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.view.spectrum_widget.rois)
-        self.assertNotIn("roi_1", self.view.spectrum_widget.rois)
 
     def test_WHEN_invalid_ROI_renamed_THEN_error_raised(self):
         rois = ["all", "roi", "roi_1"]
@@ -308,18 +297,6 @@ def test_WHEN_invalid_ROI_renamed_THEN_error_raised(self):
         with self.assertRaises(KeyError):
             self.presenter.rename_roi("invalid_roi", "new_name")
 
-    @parameterized.expand([("all", ), ("roi", )])
-    def test_WHEN_ROI_renamed_to_existing_name_THEN_keyerror(self, name):
-        rois = ["all", "roi", "roi_1"]
-        self.view.spectrum_widget.roi_dict = {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(side_effect=lambda old_name, new_name: (
-            (_ for _ in ()).throw(KeyError(f"Cannot rename to existing ROI: {new_name}"))
-            if new_name in self.view.spectrum_widget.rois else None))
-        with self.assertRaises(KeyError):
-            self.presenter.rename_roi("roi", name)
-
     def test_WHEN_do_remove_roi_called_with_no_arguments_THEN_all_rois_removed(self):
         rois = ["all", "roi", "roi_1", "roi_2"]
         self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois}

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")