Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Deprecate setting pickradius via set_picker#16154
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dd78991 toaddd0caCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
addd0ca to429ae03Compare429ae03 to0105b67Compare| Setting `.Line2D`\'s pickradius via `.Line2D.set_picker` | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| Setting a `.Line2D`\'s pickradius (i.e. the tolerance for pick events and | ||
| containment checks) is deprecated. Use `.Line2D.set_pickradius` instead. |
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.
This sentence reads funny. Suggest "Setting a.Line2D's pickradius via.Line2D.set_picker." Yes, I know it says that the previous line , but if you don't want to repeat, don't repeat any of it?
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.
nah, that was just a plain typo, edited to yours.
| @@ -543,6 +535,10 @@ def set_picker(self, picker): | |||
| artist, return *hit=True* and props is a dictionary of | |||
| properties you want added to the PickEvent attributes. | |||
| - *deprecated*: For `.Line2D` only, *picker* can also be a float | |||
| that sets the tolerance for checking whether an event occurred | |||
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.
Hmmm, you can see how this ended up here, given that we can also pass a function at this point.
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.
But at least functions are not misinterpretable as numbers...
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.
Is the other solution to only set the pick radius if picker is not abool?
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.
Sure, you can do that (but what aboutnp.bool_(True)?). But e.g. LineCollections also have a pickradius which isnot settable with set_picker, and noone ever requested to change that, so...
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.
Well, I guess you can deprecate it and see if anyone notices... I do think there is some advantage to having the pick logic all in one place, rather than here and inset_pickradius, but don't feel strongly about it
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 have some further cleanups too, but as always and as you say, if people show up with pitchforks, we can always revert the change...
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 do think there is some advantage to having the pick logic all in one place
I was thinking about t his as well, but it occurred to me thatset_picker(5) is not quite readable.
We can later always try and find something even better, cleaning this up now and aligning withwith other parts of the API is a reasonable step.
There's set_pickradius just for that purpose; moreover`set_picker(True)` would previously accidentally also set the pickradiusto 1 (because True == 1...).Also don't set self._contains in Line2D.set_picker -- this is consistentwith all other artist classes.
0105b67 toa2de5bbCompare
There's set_pickradius just for that purpose; moreover
set_picker(True)would previously accidentally also set the pickradiusto 1 (because True == 1...).
Also don't set self._contains in Line2D.set_picker -- this is consistent
with all other artist classes.
PR Summary
PR Checklist