Skip to content

Allow PolygonSelector points to be removed #19660

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 3 commits into from
May 3, 2021

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Mar 7, 2021

PR Summary

This allows one to remove individual points of a PolygonSelector by right-clicking on them (mouse button 3).

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).

if self._nverts <= 2:
# If only one point left, return to un-complete state to let user
# start drawing again
self._polygon_completed = False
Copy link
Member

Choose a reason for hiding this comment

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

Should add a test for this.

@dstansby dstansby force-pushed the remove-poly-points branch from 4c4f33a to ed2ab4b Compare April 6, 2021 09:26
@dstansby
Copy link
Member Author

dstansby commented Apr 6, 2021

Thanks for the review. I've:

  • Fixed typos
  • Added a test for re-drawing the selector when only one point is left
  • Added a note in the docstring that about the re-drawing behaviour

@QuLogic QuLogic added this to the v3.5.0 milestone Apr 6, 2021
@jklymak jklymak marked this pull request as draft April 23, 2021 17:01
@dstansby dstansby force-pushed the remove-poly-points branch from d09cffa to 0554f6e Compare May 3, 2021 09:57
@dstansby
Copy link
Member Author

dstansby commented May 3, 2021

@jklymak was this marked as a draft because it needed a rebase (now done), or is there something else?

@jklymak
Copy link
Member

jklymak commented May 3, 2021

Just the rebase.

When looking at old PRs it is easy to hit "draft:false" in the search terms. It is not easy to put "not:label" for the 10 labels that indicate author action needed.

@dstansby
Copy link
Member Author

dstansby commented May 3, 2021

👍, just checking

@jklymak
Copy link
Member

jklymak commented May 3, 2021

... when you clear the "needs rebase" totally feel free to move back to "Ready for Review". Its just a way to keep things off the radar until the author has cleared the issue. Our PR backlog is still too big for this to be super effective, but I go through once a week or so and try and sort another dozen or so.

@jklymak jklymak marked this pull request as ready for review May 3, 2021 19:35
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

I didn't test this, but looks good, and @QuLogic seemed to do a thorough review...

@jklymak jklymak merged commit 93d1434 into matplotlib:master May 3, 2021
@dstansby dstansby deleted the remove-poly-points branch May 3, 2021 19:47
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.

3 participants