-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
fix detecting which artist(s) the mouse is over #10356
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 detecting which artist(s) the mouse is over #10356
Conversation
This fix seems right. Can you rebase against a recent master - your test failure should be fixed in recent changes to master (I think). I think you are correct that the return from |
Yeah. Changing it would be simple, but it will definitely hurt people that use
I thought bug fixes used to be against |
|
39f759c
to
57e0f0a
Compare
Alright, rebased on |
@megies. Sorry this fell off the radar. It’s still not passing tests though I’m not sure if that is the prs fault. Can you ping if it’s done? |
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.
With a rebase...
since Artist.contains() returns a tuple it is not usable like "if my_artist.contains(): do something"
57e0f0a
to
d0762c3
Compare
Rebase done. |
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.
anyone can merge post-CI
Thanks @megies! |
There is a bug in how moving the mouse over the plot detects what artist should be used to collect additional info about the cursor position from (in my case several
Images
in the axes). For a plot with multiple images this means that only one image is properly reporting z coordinates and all other images won't be able to show z coordinate info.Example:
Only moving through one of the images shows correct z coordinates in the interactive window info bar at the bottom.
The reason for this bug is an oversight in #3989:
Since
Artist.contains()
returns a tuple it is not usable like this:So, what is happening is that all artists are included in the list of artists that contain the event and then the wrong artist is queried for data on the location of the event.
Since the name of the method
"contains"
might easily in the future be prone to such bugs again, I think a better solution would be to change the return from a 2-tuple (which will always evaluate toTrue
) to onlyFalse
(not contained) or a dictionary (contained) that in minimal fashion is set up like{'artist': self}
so that it evaluates toTrue
. But obviously that would drastically change a public API, so I'm assuming this won't be an option, likely..Sorry, I'm not sure how to best test this, I think to add a test would mean to first refactor
NavigationToolbar2.mouse_move()
so that getting the cursor info from all artists is in a separate helper method..PR Checklist
New features are documented, with examples if plot relatedDocumentation is sphinx and numpydoc compliantAdded an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way