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

2062 Spectrum Viewer ROI Desync Fix #2064

Merged
merged 2 commits into from
Feb 26, 2024
Merged

Conversation

MikeSullivan7
Copy link
Collaborator

Issue

Closes #2062

Description

Upon removing an ROI, the current roi and the old_table_names are refreshed to the newer state of the ROI Table and then the Properties Table is refreshed. This prevents the view checking for ROIs that no longer exist in the table when on_data_in_table_change is triggered by changing the visibility on a different ROI.

Testing

make check

Acceptance Criteria

  1. Load data and open Spectrum View
  2. Create a few ROIs
  3. Delete the first ROI in the ROI Table
  4. Change the visibility of the other ROIs in the table, they should no behave as expected
  5. Check that the active ROI in the Properties Table is correct when deleting the current ROI.
  6. Perform Usual checks of switching tabs, samples, etc.

@coveralls
Copy link

coveralls commented Feb 23, 2024

Coverage Status

coverage: 74.427% (-0.06%) from 74.482%
when pulling 81197be on 2062_ROI_tracking
into 8bb4100 on release-2.7.0.

@samtygier-stfc
Copy link
Collaborator

From testing with the checker from the 2065-state-check branch, I think current_roi should be updated after removing an roi. Otherwise it references a name that does not exist.

@MikeSullivan7
Copy link
Collaborator Author

From testing with the checker from the 2065-state-check branch, I think current_roi should be updated after removing an roi. Otherwise it references a name that does not exist.

This is already done in this PR on line 426 self.current_roi = self.roi_table_model.row_data(self.selected_row)[0]

After the roi is removed, the current roi is changed to the newly selected row in the table model

@samtygier-stfc
Copy link
Collaborator

I can get an inconsistent state, with the following.

git switch 2062_ROI_tracking  # 524f03f8d715e5591ba2a3e509899e23cba4b339
git cherry-pick 6db64556bc887dfb63da6037fb4e02eb2d2b886b

Open dataset, open spectrum viewer
Add an ROI, "roi_1"
Select "roi"
Remove "roi"
Untick visibility of roi_1

Ctrl+D shows
current_roi = "roi"
which no longer exists.

I can then trigger an error by editing one of the roi position spin boxes.

@samtygier-stfc
Copy link
Collaborator

Thanks, I think that solves it. I see a few false error messages from my checker (it does not handle all ROIs deleted, or being on the image tab properly). But I can't see anything that would trigger a user bug.

Happy for a post release refactoring session.

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the issue, and I can't see any new ones

@samtygier-stfc samtygier-stfc merged commit 19458d0 into release-2.7.0 Feb 26, 2024
7 checks passed
@samtygier-stfc samtygier-stfc deleted the 2062_ROI_tracking branch February 26, 2024 13:54
@MikeSullivan7 MikeSullivan7 restored the 2062_ROI_tracking branch February 26, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants