Skip to content

Allow Selectors to be dragged from anywhere within their patch #19657

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

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Mar 6, 2021

PR Summary

As it says in the what's new entry:

The ~matplotlib.widgets.RectangleSelector and ~matplotlib.widgets.EllipseSelector have a new keyword argument, select_whole_region, which when set to True allows you to click and drag from anywhere inside the selector to move it. Previously it was only possible to move it by either activating the move modifier button, or clicking on the central handle.

Questions:

  • Any opinions on turning this on by default?
  • Does anyone have any better ideas for the keyword argument name? select_whole_region is the best I came up with, but it doesn't feel great.

Here's an example:

Screen.Recording.2021-03-06.at.16.53.58.mov

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@ianhi
Copy link
Contributor

ianhi commented Mar 7, 2021

Does anyone have any better ideas for the keyword argument name? select_whole_region is the best I came up with, but it doesn't feel great.

drag_from_anywhere?

@dstansby dstansby force-pushed the selector-dragging branch from e8905ad to bbf32b8 Compare April 4, 2021 10:48
@dstansby dstansby force-pushed the selector-dragging branch from 832d55e to 0c35d09 Compare April 8, 2021 14:23
@dstansby dstansby requested review from QuLogic and timhoffm April 15, 2021 14:23
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

I think you may have rebased and reverted the changes I suggested.

@@ -44,6 +44,36 @@ def test_rectangle_selector():
check_rectangle(rectprops=dict(fill=True))


@pytest.mark.parametrize('drag_from_anywhere, new_center',
[[True, (60, 75)],
[False, (30, 20)]])
Copy link
Member

Choose a reason for hiding this comment

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

If you're not dragging from the center, and you didn't enable the new option, why is the center now (30, 20) instead of remaining at (50, 65)?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because dragging with drag_from_anywhere=False creates a new rectangle, which has a center (30, 20). I'll add some more comments to the test to make it clearer what's going on.

@dstansby dstansby force-pushed the selector-dragging branch from 0c35d09 to 17c6f8c Compare April 16, 2021 12:44
@dstansby
Copy link
Member Author

Sorry about the review comments, they should all be implemented now. I've added some more comments to the test too - let me know if anything remains unclear.

@QuLogic QuLogic added this to the v3.5.0 milestone Apr 16, 2021
@QuLogic QuLogic merged commit c89cf88 into matplotlib:master Apr 16, 2021
@dstansby dstansby deleted the selector-dragging branch April 17, 2021 10:01
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