Removed set_roi_alpha method and replaced it with set_roi_visibility #2345
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…flags to consolidate visibility and alpha management.
samtygier-stfc
requested changes
Oct 16, 2024
Comment on lines
93
to
109
| @parameterized.expand([("Visible", "visible_roi", 255), ("Invisible", "invisible_roi", 0)]) | ||
| def test_WHEN_set_roi_alpha_called_THEN_roi_alpha_updated(self, _, name, alpha): | ||
| def test_WHEN_set_roi_visibility_flags_called_THEN_roi_alpha_updated(self, _, name, alpha): | ||
| spectrum_roi = SpectrumROI(name, self.sensible_roi, rotatable=False, scaleSnap=True, translateSnap=True) | ||
| self.spectrum_widget.roi_dict[name] = spectrum_roi | ||
| self.spectrum_widget.spectrum_data_dict[name] = np.array([0, 0, 0, 0]) | ||
| self.spectrum_widget.set_roi_alpha(name, alpha) | ||
| self.spectrum_widget.set_roi_visibility_flags(name, visible=bool(alpha), alpha=alpha) | ||
| self.assertEqual(self.spectrum_widget.roi_dict[name].pen.color().getRgb()[-1], alpha) | ||
| self.assertEqual(self.spectrum_widget.roi_dict[name].hoverPen.color().getRgb()[-1], alpha) | ||
|
|
||
| @parameterized.expand([("Visible", "visible_roi", 1), ("Invisible", "invisible_roi", 0)]) | ||
| def test_WHEN_set_roi_alpha_called_THEN_set_roi_visibility_flags_called(self, _, name, alpha): | ||
| def test_WHEN_set_roi_visibility_flags_called_THEN_visibility_and_alpha_handled(self, _, name, alpha): | ||
| spectrum_roi = SpectrumROI(name, self.sensible_roi, rotatable=False, scaleSnap=True, translateSnap=True) | ||
| self.spectrum_widget.roi_dict[name] = spectrum_roi | ||
| self.spectrum_widget.spectrum_data_dict[name] = np.array([0, 0, 0, 0]) | ||
| with mock.patch.object(SpectrumWidget, "set_roi_visibility_flags") as mock_set_roi_visibility_flags: | ||
| self.spectrum_widget.set_roi_alpha(name, alpha) | ||
| mock_set_roi_visibility_flags.assert_called_once_with(name, alpha) | ||
| with mock.patch.object(spectrum_roi, "set_visibility") as mock_set_visibility: | ||
| self.spectrum_widget.set_roi_visibility_flags(name, visible=bool(alpha), alpha=alpha) | ||
| mock_set_visibility.assert_called_once_with(bool(alpha)) |
There was a problem hiding this comment.
These can be removed, as there is no need to change alpha
Comment on lines
181
to
193
| def set_roi_visibility_flags(self, name: str, visible: bool, alpha: float = 1.0) -> None: | ||
| """ | ||
| Change the visibility of an existing ROI including handles and update | ||
| the ROI dictionary, sending a signal to the main window. | ||
| the ROI dictionary, setting the alpha (transparency) value at the same time. | ||
|
|
||
| @param name: The name of the ROI. | ||
| @param visible: The new visibility of the ROI. | ||
| @param alpha: The new alpha (transparency) value of the ROI. | ||
| """ | ||
| self.roi_dict[name].set_visibility(visible) | ||
|
|
||
| def set_roi_alpha(self, name: str, alpha: float) -> None: | ||
| """ | ||
| Change the alpha value of an existing ROI | ||
| @param name: The name of the ROI. | ||
| @param alpha: The new alpha value of the ROI. | ||
| """ | ||
| self.roi_dict[name].colour = self.roi_dict[name].colour[:3] + (alpha, ) | ||
| self.roi_dict[name].setPen(self.roi_dict[name].colour) | ||
| self.roi_dict[name].hoverPen = mkPen(self.roi_dict[name].colour, width=3) | ||
| self.set_roi_visibility_flags(name, bool(alpha)) | ||
| if alpha is not None: | ||
| self.roi_dict[name].colour = self.roi_dict[name].colour[:3] + (alpha, ) | ||
| self.roi_dict[name].set_visibility(visible) |
There was a problem hiding this comment.
I don't think there is a need to have the alpha value. If the visibility is set to false then the element will be hidden.
Comment on lines
251
to
254
| if roi_visible is False: | ||
| self.set_roi_alpha(0, roi_name) | ||
| self.set_roi_visibility_flags(roi_name, visible=False, alpha=0) | ||
| else: | ||
| self.set_roi_alpha(255, roi_name) | ||
| self.set_roi_visibility_flags(roi_name, visible=True, alpha=255) |
There was a problem hiding this comment.
If statement can be removed, and this can be:
self.set_roi_visibility_flags(roi_name, roi_visible)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…flags to consolidate visibility and alpha management.
Issue
Closes #2342
Description
Removed set_roi_alpha and replaced it with set_roi_visibility_flags to handle both visibility and alpha transparency. Updated references in presenter.py, view.py, and spectrum_widget.py.
Testing
Refactored existing tests to validate the new set_roi_visibility_flags method.
Verified that ROIs are correctly shown/hidden and their transparency is adjusted as expected.
Acceptance Criteria
The reviewer should confirm that ROI visibility and alpha transparency are handled correctly.
Verify that set_roi_alpha references are fully replaced by set_roi_visibility_flags.
Run all tests to ensure they pass without errors.
Documentation
No changes needed in the documentation since this refactor does not affect the external API or user-facing functionality. Internal functionality has been improved for better maintainability.