Deprecate setting pickradius via set_picker #16154
Conversation
dd78991 to
addd0ca
Compare
addd0ca to
429ae03
Compare
429ae03 to
0105b67
Compare
| 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.
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.
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.
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.
But at least functions are not misinterpretable as numbers...
There was a problem hiding this comment.
Is the other solution to only set the pick radius if picker is not a bool?
There was a problem hiding this comment.
Sure, you can do that (but what about np.bool_(True)?). But e.g. LineCollections also have a pickradius which is not settable with set_picker, and noone ever requested to change that, so...
There was a problem hiding this comment.
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 in set_pickradius, but don't feel strongly about it
There was a problem hiding this comment.
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.
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 that set_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 pickradius to 1 (because True == 1...). Also don't set self._contains in Line2D.set_picker -- this is consistent with all other artist classes.
0105b67 to
a2de5bb
Compare
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