-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
CallbackRegistry should not silently drop duplicate callbacks #20210
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
Comments
I agree that silently dropping the second connection request is bad style. What I don't understand is your choice of preference (preferred/less preferred). I don't where it could be useful to have duplicated connects. OTOH I can imagine scenarios in which certain actions that connect an event can happen multiple times, but you don't want multiple registrations. So requireing the user to track duplicates seems like an unnecessary burden. I'd Intuitively go for |
I guess defaulting to raising-on-duplicate is fine, but my point of view is that you shouldn't really rely on CallbackRegistry doing the dedupe for you anyways. For example, imagine (grossly simplified): def handler():
# do whatever
fig.canvas.mpl_connect("foo_event", lambda event: handler()) # indirectly through other functions
fig.canvas.mpl_connect("foo_event", lambda event: handler()) # indirectly somewhere else (Assume that for whatever reason you indeed wanted handler to be called twice.) Some time later, you realize that your handler needs to take event as argument, so you write def handler(event):
# do whatever
fig.canvas.mpl_connect("foo_event", lambda event: handler(event))
fig.canvas.mpl_connect("foo_event", lambda event: handler(event)) and then you realize that you can rewrite this as def handler(event):
# do whatever
fig.canvas.mpl_connect("foo_event", handler)
fig.canvas.mpl_connect("foo_event", handler) All of a sudden, the last version has different semantics, which I think is a bit unexpected... (IOW, I think one should normally be able to replace OTOH I don't have a super strong opinion there either. |
In GTK and Qt, connecting a second time to an event simply connects again. In Tk, you can only bind once, it seems like. |
I'm of two minds on this. I can see why it would be useful to do this de-duplication ("please connect this function" "it already is! here is its cid") and as Elliott points out the GUI toolkits are split on this. On the other hand, I can see why "do exactly what I said" is nice. I would not block a PR to start this deprecation cycle. |
If we can’t guess what the desired behavior is, likely, there is no universal answer and making the behavior configurable is reasonable. Apart form the (limited) implementation effort, I don‘t see any downside with this. |
Bug report
Bug summary
Currently, if a same function is connected twice to the same signal and same callbackregisry, the second connection is silently dropped, per
matplotlib/lib/matplotlib/cbook/__init__.py
Lines 208 to 210 in b6a6414
This seems not so nice and a bit surprising (as in #15785 (comment)); we should deprecate that behavior and either
keep_duplicates={None,True,False}
where None (default) raises an exception for duplicates, True keeps them, False drops them.Code for reproduction
and click on the canvas.
Actual outcome
The event is printed once.
Expected outcome
The event is printed twice.
Matplotlib version
import matplotlib; print(matplotlib.__version__)
): HEAD (3.4.x+)print(matplotlib.get_backend())
): anyThe text was updated successfully, but these errors were encountered: