FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

CallbackRegistry should not silently drop duplicate callbacks · Issue #20210 · matplotlib/matplotlib · GitHub

Repository navigation

CallbackRegistry should not silently drop duplicate callbacks #20210

Description

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

proxy = _weak_or_strong_ref(func, self._remove_proxy)
if proxy in self._func_cid_map[signal]:
return self._func_cid_map[signal][proxy]

This seems not so nice and a bit surprising (as in #15785 (comment)); we should deprecate that behavior and either

  • (preferred) just always perform the connection, even if duplicated; it's the user's job to keep track of duplicates if they want; or
  • (less preferred) add something like keep_duplicates={None,True,False} where None (default) raises an exception for duplicates, True keeps them, False drops them.

Code for reproduction

for _ in range(2): gcf().canvas.mpl_connect("button_press_event", print)
show()

and click on the canvas.

Actual outcome

The event is printed once.

Expected outcome

The event is printed twice.

Matplotlib version

  • Operating system: linux
  • Matplotlib version (import matplotlib; print(matplotlib.__version__)): HEAD (3.4.x+)
  • Matplotlib backend (print(matplotlib.get_backend())): any
  • Python version: 39
  • Jupyter version (if applicable):
  • Other libraries:

Activity

  1. added
    API: changesChanges to the public API, typically requiring deprecation.
    on May 12, 2021
  2. timhoffm commented on May 13, 2021

    Member

    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 keep_duplicates (naming t.b.d.). But I'm not the event expert and maybe I'm missing something here.

  3. anntzer commented on May 16, 2021

    ContributorAuthor

    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 func by lambda *args, **kwargs: func(*args, **kwargs) without changing the semantics of a program.)

    OTOH I don't have a super strong opinion there either.

  4. QuLogic commented on May 18, 2021

    Member

    In GTK and Qt, connecting a second time to an event simply connects again. In Tk, you can only bind once, it seems like.

  5. tacaswell commented on Dec 20, 2023

    Member

    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.

  6. timhoffm commented on Dec 21, 2023

    Member

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    API: changesChanges to the public API, typically requiring deprecation.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions


      Back | FazBrowse Home | New Git URL