-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Go back to checking figures for their manager in destroy. #18184
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
Go back to checking figures for their manager in destroy. #18184
Conversation
num = next((manager.num for manager in cls.figs.values() | ||
if manager.canvas.figure == fig), None) | ||
if num is not None: | ||
cls.destroy(num) |
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.
Can we add a comment, why we have to destroy by number?
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.
It actually works either way (manager
or manager.num
); it's just that the code in destroy
is simpler for num
, i.e., we know that cls.figs.get(manager.num) is manager
because we just pulled manager
from cls.figs
.
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.
Travis OSX fails. I'm unclear if the error is related to the PR.
Yea, that's weird, a restart was not effective; I'll try it out on the mac. |
It worked on the Mac Mini, so I restarted Travis again. |
4d8e0d1
to
f65eb17
Compare
I tried increasing the timeout, which didn't work, but it's appearing on other tests now, so I've reverted that as the problem doesn't seem strictly related to this PR. |
As discussed on the call to get 3.3.1 out the door we are going to disable this test on travis (it should be noted that we do run this test on azure) so the current working theory is that this is a travis-specific issues with the GUI and is not worth the time right now to track down the root problem (but we should open a new issue and label it "good first issue / medium" localized, but involves defeating CI ;) ). |
This is a partial revert of a small part of matplotlib#13581. Fixes matplotlib#18163.
We will follow up on this later, matplotlib#18213.
f65eb17
to
db6fb4d
Compare
…anager in destroy.
…184-on-v3.3.x Backport PR #18184 on branch v3.3.x (Go back to checking figures for their manager in destroy.)
PR Summary
This is a partial revert of a small part of #13581.
Fixes #18163.
PR Checklist