-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix several SVG closing issues #7330
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
Conversation
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.
Hope this works on Python 2...
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.
Guess I'll be needing a Python 2 version.
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.
I think you are doing it not in a right way. Just RendererSVG.writer should be deleted in RendererSVG.finalize.
Closes #6870
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.
The call to finalize should go it in the finally block then.
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.
I do not fully understand the difference between detach and fh_to_close.
There may need to be a bigger change in that renderers are allowed to report that they can not be cached (as I suspect that calling draw_artist with the svg renderer would not work).
I previously refactored print_svg and print_svgz with context manager, but have not opened a PR with the changes. Would you like me to open a PR?
Sure, open directly to this branch and we'll include it in this PR.
It does not clearly cherry-picks here due to added detach magic.
Post a branch to your fork and I'll see; maybe I can drop the detach stuff...
It doesn't seem like it needs to cache the renderer itself, since other artists get it from the figure in the same way.
It's not allowed to be pickled on a Figure, so presumably it shouldn't be allowed on the Axes either. Fixes matplotlib#6870. Fixes matplotlib#6181. Fixes matplotlib#3899.
1b1406a to
84eec23
Compare
This leaves the owned-by-caller file object alone instead of forcing it to close as well. Fixes matplotlib#6926, though I'm not entirely sure why only the cleanup decorator + usetex triggers it.
84eec23 to
1810f1f
Compare
Thanks; I've incorporated it into this PR and we can drop the confusing fh_to_close stuff. However, the detach stuff is still necessary to fix #6926.
Restarted the failed build, so it's good from my side.
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.
These look good to me now.
So I'm a little rusty--do I need to backport this to master?
@QuLogic will you make sure all the issues fixed by this get closed?
@dopplershift no, we every-so-often merge v2.x into master.
I think @QuLogic and I are currently playing chicken about who will do it next 😉 .
The issues will close automatically when the commits get into master.
Fix a leak where the wrong file handle was closed (maybe related to warnings in #6870? @Kojoley). Fix premature closing of underlying files, and some pickling issues.