-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix centre square rectangle selector part 1 #21604
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 centre square rectangle selector part 1 #21604
Conversation
… existing shape, part 1, with centering on
… existing shape, part 2, with centering off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall - I've left a few suggestions and questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, all comments should have been addressed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - all looks good to me!
I think this is a bugfix, so going to milestone to 3.5.x - second reviewer feel free to correct me on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericpre Since you plan a followup PR, the comment on using tuples can be addressed therein. I'll merge as is to enable you continuing with that PR.
size_on_press = [x1 - x0, y1 - y0] | ||
center = [x0 + size_on_press[0] / 2, y0 + size_on_press[1] / 2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use tuples instead of lists for coordinates.
…604-on-v3.5.x Backport PR #21604 on branch v3.5.x (Fix centre square rectangle selector part 1)
@dstansby, as mentioned in #20839 (comment), this is the first part of #20839.
PR Summary
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.