Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2378 Part One: A New ROI Properties Table Class #2493

Merged
merged 5 commits into from
Feb 14, 2025

Conversation

JackEAllen
Copy link
Collaborator

@JackEAllen JackEAllen commented Feb 10, 2025

Issue Contributes to: #2378

Description

The proposed changes moves the ROI Table Properties out of SpectrumViewerWindowView and into the class: ROIPropertiesTableWidget in preparation for extracting the related xml GUI code out of spectrum_viewer.ui and into mantidimaging/gui/widgets/spectrum_widgets/ROIFORMWidget.ui and ROIPropertiesTableWidget into mantidimaging/gui/widgets/spectrum_widgets/ROIFORMWidget.py

Developer Testing

  • I have verified unit tests pass locally: python -m pytest -vs
  • I have manually tested the Spectrum Viewer ROI properties remain up to date when:
    • Creating new ROIs
    • Renaming ROIs
    • Removing ROIs
    • Selecting an ROI with the intention to rename, then selecting another ROI
    • Swapping tabs between ROI and Image tabs
    • Toggling ROI visibility for various ROIs
    • Toggling ROI visibility and swapping between Image and ROI tab views.

Acceptance Criteria and Reviewer Testing

  • Unit tests pass locally: python -m pytest -vs
  • ROI Properties table values always display the selected ROI
  • The Selected ROI can only be a visible ROI
  • No change in functional behaviour is introduced when compared to main branch.

Documentation and Additional Notes

Release notes are not needed as they will be added as part of the final PR completing the work described by #2378

GUI change due to chance bug fix. Correct width and height now shown in screenshot

  • Screenshot tests have been updated
    • Before merge for developer: Resolve the change on applitools by, creating a new baseline for test and verify that the tests pass.
    • After merge for reviewer: Go to Applitools compare and merge page, select all changes and merge dev branch to default branch
    • After merge for reviewer: Verify that other PR's screenshot tests do not start to fail (See Applitools baselines for more info)

@JackEAllen JackEAllen self-assigned this Feb 10, 2025
@samtygier-stfc
Copy link
Collaborator

I got this when adjusting the ROI from the spin boxes, I can fix this

  File "/home/sam/git/mantidimaging/mantidimaging/gui/windows/spectrum_viewer/presenter.py", line 433, in do_adjust_roi
    new_roi = self.convert_spinbox_roi_to_SensibleROI(self.view.roiPropertiesSpinBoxes)
                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'SpectrumViewerWindowView' object has no attribute 'roiPropertiesSpinBoxes'. Did you mean: 'roiPropertiesGroupBox'?

@samtygier-stfc samtygier-stfc force-pushed the 2378_PART_ONE_Extract_ROI_Properties_Table branch from d03c5be to 57fc561 Compare February 12, 2025 15:29
@samtygier-stfc samtygier-stfc force-pushed the 2378_PART_ONE_Extract_ROI_Properties_Table branch from 57fc561 to d72767a Compare February 13, 2025 17:00
@samtygier-stfc samtygier-stfc marked this pull request as ready for review February 14, 2025 09:18
@samtygier-stfc samtygier-stfc added this pull request to the merge queue Feb 14, 2025
Merged via the queue into main with commit 34d5abd Feb 14, 2025
8 checks passed
@samtygier-stfc samtygier-stfc deleted the 2378_PART_ONE_Extract_ROI_Properties_Table branch February 14, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants