diff --git a/mantidimaging/gui/windows/spectrum_viewer/model.py b/mantidimaging/gui/windows/spectrum_viewer/model.py index 1eab85c63e2..beb79e78cf1 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/model.py +++ b/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 diff --git a/mantidimaging/gui/windows/spectrum_viewer/presenter.py b/mantidimaging/gui/windows/spectrum_viewer/presenter.py index 380f06387f5..ee682637ff9 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/presenter.py +++ b/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) diff --git a/mantidimaging/gui/windows/spectrum_viewer/spectrum_widget.py b/mantidimaging/gui/windows/spectrum_viewer/spectrum_widget.py index 72a30206038..d9439981a77 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/spectrum_widget.py +++ b/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): diff --git a/mantidimaging/gui/windows/spectrum_viewer/test/model_test.py b/mantidimaging/gui/windows/spectrum_viewer/test/model_test.py index 63e30e23f63..359c75edbf3 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/test/model_test.py +++ b/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) diff --git a/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py b/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py index 6c682e70a12..44513fdc48c 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py +++ b/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"] diff --git a/mantidimaging/gui/windows/spectrum_viewer/test/spectrum_test.py b/mantidimaging/gui/windows/spectrum_viewer/test/spectrum_test.py index fee63dbf2a7..62845a14a1f 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/test/spectrum_test.py +++ b/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")