Skip to content
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

let KeyErrors pass silently on callback removal #2985

Closed
wants to merge 1 commit into from
Closed

let KeyErrors pass silently on callback removal #2985

wants to merge 1 commit into from

Conversation

jbmohler
Copy link
Contributor

I've run into a spurious KeyError when I remove an event connection during another event. This patch fixes that by letting it pass silently. I don't have a failing test written as the contribution guidelines request; I'm hoping this commit can be accepted despite that as it is pretty innocent & writing such a test seems to be breaking very new ground -- there is only one test containing "mpl_connect" and simulating events with-out picking a backend is unknown to me.

@tacaswell
Copy link
Member

In general silently eating exceptions is a Bad Thing (R).

This sounds like a threading issue as the dictionary is getting changed under this block of code while it is iterating over the dictionary.

@jbmohler
Copy link
Contributor Author

I agree that eating exceptions is bad generally; I believe that this is a valid use (well-targeted and goal of code has evidently been already achieved).

The motivation for this change is shown at https://gist.github.com/jbmohler/11052384 . The heart of the matter is in GraphWindow.on_press where the connected events are changed and object reference dropped during an event. To reproduce issue click & drag twice in the axis.

@tacaswell
Copy link
Member

Wouldn't it be simpler to just turn off the tools you don't want? Just setting my_widget.active = False will short-circuit the event logic.

@jbmohler
Copy link
Contributor Author

Are you saying that changing attached events through-out the lifetime of an object is not supported?

There are a number of different ways I've considered to undertake the task at hand. Some methods are not impacted by this bug. However, it seemed clear enough and easy to fix.

@tacaswell
Copy link
Member

No, connecting/disconnecting events while processing events is (apparently) not supported. I suspect the construction list(six.iterkey()) is an artifact of automated tools rather than a design decision (I think that the list call should be removed there).

In the case you show in the gist you can solve the problem by just toggling the span selectors on and off via signals from the actions. Alternately, use the Qt signal/slot infrastructure to add/delete the widgets.

@jbmohler
Copy link
Contributor Author

Sure, I can fix this other ways although I still think my change is reasonable and worthwhile. The list call seemed (to me) likely to be intentional since it is a familiar idiom for compensating for the possibility that the list changes underneath the iteration.

In fact, I think I can also fix it in my gist by simply not explicitly calling the disconnect_events. The very code in question will clean up as the weak reference goes away.

@jbmohler jbmohler closed this Apr 18, 2014
@tacaswell
Copy link
Member

@jbmohler I hope you don't feel aggrieved, if you do I apologize. Please do not be discouraged from future contributions to matplotlib.

If you feel that this PR should go in, please re-open it. I do not have final veto power and my views are my own, not representative of all of the devs. My opinion alone should not be enough to shut down the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants