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

Fix ROI resizing in Insight (see #11384) #1530

Merged
merged 6 commits into from
Sep 26, 2013

Conversation

bpindelski
Copy link

This PR fixes https://trac.openmicroscopy.org.uk/ome/ticket/11384 and also add a bonus feature.

Now a checkbox with the "Scale Proportionally" is present in the Inspector tab of the Measurement Tool (ROI) in Insight. As expected, when the checkbox is checked, the ROI will be set to a new size with both width and height updated with the same value.

To test this PR:

This PR will most probably be have to rebased onto develop.

Blazej Pindelski added 4 commits September 18, 2013 16:28
This enables the user to scale the ROI proportionally.
It is a checkbox element in the Measurement Tool that, when
activated, will change width and height in unison.
/** The row indicating to show the text or not. */
private static final int WIDTH_ROW = 1;
private static final int WIDTH_ROW = 2;

/** The row indicating to show the text or not. */
Copy link
Member

Choose a reason for hiding this comment

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

is it really?

@bpindelski
Copy link
Author

@mtbc At least I kept the changes consistent with the overall insanity that is happening in those classes ;) I shall clean up the comments etc., but we have to always be aware that that will make the diff longer to read, hence increasing the PR review time.

@jburel
Copy link
Member

jburel commented Sep 20, 2013

@bpindelski: Thanks. I think we should draw a line and do not make more changes. The Measurement tool needs to be rewritten

@mtbc
Copy link
Member

mtbc commented Sep 20, 2013

This PR certainly fixes the bug.

The "Scale Proportionally" checkbox isn't available for new ROIs; if I save and reopen then it works, though.

I think I'd probably word it differently, too. "Equal Dimensions" or something? I'm not sure. "Scale Proportionally" makes me expect that, if I change one dimension, the other will be changed so as to maintain the previous aspect ratio.

@mtbc
Copy link
Member

mtbc commented Sep 20, 2013

Also, it works fine for rectangles, but ellipses don't seem to get the same instant update for some reason. Still, if that's not an actual regression, then it's not worth much effort to fix right now if we're going to overhaul the measurement tool anyway.

@joshmoore
Copy link
Member

@jbreul / @mtbc: shall we merge @bpindelski's PR as it stands and open another one with @mtbc's suggestions?

@mtbc
Copy link
Member

mtbc commented Sep 26, 2013

Blazej is back on Monday anyway, but yes that would be okay with me. I don't mind opening the different-wording PR if we agree on the need and the words. The new-ROIs issue could just be ticketed.

@jburel
Copy link
Member

jburel commented Sep 26, 2013

@mtbc, @joshmoore: i would rather add information to the https://docs.google.com/document/d/1n3S-gVu1lUt37SnGQKab-cWGXhiW1AwIHBFKgDYwI4I/edit than creating more ticket.
That piece of code needs to be totally rewritten. so the ticket will quickly become invalid.
Happy to merge

@mtbc
Copy link
Member

mtbc commented Sep 26, 2013

Sure, that works for me too.

@joshmoore
Copy link
Member

Leaving the two of you to move the info then. Merging.

joshmoore added a commit that referenced this pull request Sep 26, 2013
Fix ROI resizing in Insight (see #11384)
@joshmoore joshmoore merged commit 5efe2af into ome:dev_4_4 Sep 26, 2013
@jburel
Copy link
Member

jburel commented Sep 26, 2013

--rebased-to #1550

@bpindelski bpindelski deleted the 11384-roisize branch September 30, 2013 11:05
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.

None yet

4 participants