-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Deprecate unused LassoSelector event handlers. #20603
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
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. |
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.
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? |
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.
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
pytest
passes).flake8
on changed files to check).flake8-docstrings
and 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).