Repository navigation
Deprecate unused LassoSelector event handlers. - #20603
Conversation
These are unused since 61ab6df (and have trivial replacements if someone *really* wants to call them manually, which seems unlikely).
|
Are we sure this is the direction we want to deprecate? Most other event callbacks start with |
|
I think we should move towards making all widget callbacks private, with the idea that the "correct" way to programatically interact with widgets is to send events to the canvas and just let the canvas' CallbackRegistry do the dispatching. |
ericpre
left a comment
ericpre
left a comment
There was a problem hiding this comment.
Are we sure this is the direction we want to deprecate?
Most other event callbacks start with on though naming is not consistent.
It would be good to tidy up redundant callbacks, but maybe an alternative is to rename press and release to onpress and onrelease in _SelectorWidget?
That would be a bigger API break: anyone who was using press or release now has to change their code and version-gate, whereas with the change proposed here 1) likely they were using press or release to start with, as that's the only thing that worked for everything except LassoSelector, and 2) one can keep using press and release and have something that works for all versions. ... or, if we believe that the API break doesn't matter because no one is using the callbacks directly, we should (repeating myself) consider just making them private. |
|
@ericpre is the above OK w/ you? |
ericpre
left a comment
ericpre
left a comment
There was a problem hiding this comment.
Yes, renaming the press/release is a different discussion.
These are unused since 61ab6df (and have trivial replacements if
someone really wants to call them manually, which seems unlikely).
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).