Use callback registry for all selector widgets#31042
Use callback registry for all selector widgets#31042ianhi wants to merge 1 commit intomatplotlib:mainfrom
Conversation
github-actions
bot
added
topic: widgets/UI
Documentation: examples
| New code should use `on_select` to register callbacks. | ||
| This property may be deprecated in a future release. | ||
| """ | ||
| return self._onselect |
There was a problem hiding this comment.
Rather than keeping the state 2x, we should pull this out of the _observers and raise if there is more than one.
Similarly for the setter, we should fail if there is more than one. That will keep us from having to add any new state.
There was a problem hiding this comment.
I'm worried that disallowing setting onselect when there are multiple registered leaves no way for the user to de-register that initial callback, becasue it's callbackid is hidden from inside init.
👍 on not saving it separately though
There was a problem hiding this comment.
currently no one has more than one registered callback so no existing code should break, but if someone writes new code that mixes the two then we drive our self to a maybe inconsistent state. By erroring if the behavior is ambiguous in a mixed usage we can avoid that without leaving latent bugs for new usage (because it should error while they are writing it) and without breaking existing users / forcing a migration.
There was a problem hiding this comment.
currently you can deregister by setting None (I think), that would be straight forward to keep the behavior of.
2 participants
PR summary
Addresses the core issue underlying: #24039
Prior to this you could only have one callback associated with the
onselectof any of the selectors which is limiting. This PR expands that to use a CallbackRegistry internally (like the other widgets) whcih allows handling multiple onselect callbacks.I used the name
on_selectinstead ofon_changedto differentiate from the value selection widgets vs these selectors. I also tried to keep back compat with the possible use case of directly assigning toonselect.For consistency with other widgets the selectors now also check for
self.eventsonbefore firing the callbacks🤖 Idea and direction by me. I used claude to do the typing. Iterated through several implement review cycles locally before posting
PR checklist