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

Set InteractCheckableTool._roi to None on deactivate #420

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

dhomeier
Copy link
Contributor

Description

Reset any _roi in an InteractCheckableTool when calling deactivate.
This is to avoid inheriting changed attributes like rotation angle from previously created selections. It will not reset the attributes when creating new ROIs with the same (left open) tool instance.

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (307a11d) 86.63% compared to head (b26dd56) 86.64%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #420   +/-   ##
=======================================
  Coverage   86.63%   86.64%           
=======================================
  Files          89       89           
  Lines        5163     5165    +2     
=======================================
+ Hits         4473     4475    +2     
  Misses        690      690           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pllim
Copy link
Contributor

pllim commented Feb 6, 2024

Is this trying to solve the same problem that I approached differently in #416 ? Or is this for something else?

@rosteen
Copy link
Contributor

rosteen commented Feb 19, 2024

Is this trying to solve the same problem that I approached differently in #416 ? Or is this for something else?

This is trying to solve the problem I was hacking around in spacetelescope/jdaviz#2639.

@dhomeier dhomeier marked this pull request as ready for review February 19, 2024 16:20
@rosteen
Copy link
Contributor

rosteen commented Feb 19, 2024

@dhomeier This looks good to go in to me, I confirmed on the Jdaviz side that it fixes the unwanted behavior.

@pllim
Copy link
Contributor

pllim commented Feb 19, 2024

tl;dr -- This does not reintroduce the problem where the angle resets when you click and drag a rotated shape, does it?

@rosteen
Copy link
Contributor

rosteen commented Feb 19, 2024

tl;dr -- This does not reintroduce the problem where the angle resets when you click and drag a rotated shape, does it?

The existing problem of the gray preview not matching the rotation of the actual subset persists, but it's not worse than the current state. The dragged subset retains its angle in my testing.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@astrofrog astrofrog merged commit 090f214 into glue-viz:main Feb 20, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants