Unset the canvas manager when saving the figure. #10292
Conversation
|
This addresses enough bugs and seems manageable to review for 2.2 |
| @@ -2149,6 +2150,15 @@ def print_figure(self, filename, dpi=None, facecolor=None, edgecolor=None, | |||
|
|
|||
| """ | |||
| self._is_saving = True | |||
| # Remove the figure manager, if any, to avoid resizing the GUI widget. | |||
There was a problem hiding this comment.
Maybe # Remove the figure manager, currently set in some backeneds (i.e. nbagg).... I was looking at this and not understanding when self.manager ever got set. Similarly above when you instantiate it, maybe a comment on what a manager is and where one is set?
There was a problem hiding this comment.
There is a FigureManagerBase class defined in the same file, and if you look at its init you'll see it creates the manager attribute on the canvas. I think it's better to have a single doc that describes the interaction between all classes in backend_bases rather than document this in a disjoint, one-comment-at-a-time way... (but it is also true that that single doc doesn't exist yet)
There was a problem hiding this comment.
I'm still confused what backend I need to use to test this works, and how. I assume I need to trigger a manual save. This works fine on Qt5Agg, but did not check if manager is set in Qt5Agg.
There was a problem hiding this comment.
Set the savefig.dpi rcparam to some large number (say, three times your figure.dpi, just to be safe with hidpi stuff...) and then interactively trigger a save, while the window has a relatively small size on your desktop. Depending on yourdesktop environment (specifically, how it handles fast window resize events), you may see the window flicker in size, first to, well, three times its current size, and then back.
This resize is handled by the canvas manager. This PR disconnects the manager for the duration of the save, thus suppressing the flicker.
There was a problem hiding this comment.
Hmmm, I guess my machine is too fast - I don't get a flicker w/ Qt5Agg on Master or any apparent resizing. The PR doesn't make things any better or worse on Qt5Agg for me, so if it helps other folks its fine by me.
There was a problem hiding this comment.
Can you try on linux? It may well be that OSX just detects and suppresses fast resizes.
May also help if you make the plot relatively slow to draw (e.g., N=100000; scatter(rand(N), rand(N), s=rand(N))), which may make the effect more apparent.
There was a problem hiding this comment.
No, I don't have a linux machine (set up for this). I guess OSX is just clever - making the plot more ornate doesn't change anything.
There was a problem hiding this comment.
I assume @tacaswell checked that this works for him too?
lib/matplotlib/backend_bases.py
Outdated
| @@ -1772,6 +1772,7 @@ def __init__(self, figure): | |||
| self._is_saving = False | |||
| figure.set_canvas(self) | |||
| self.figure = figure | |||
| self.manager = None | |||
There was a problem hiding this comment.
Why set None if you're checking using hasattr in print_figure?
There was a problem hiding this comment.
oops. wanted to figure out the difference between None and not having it at all, that's leftover from some debugging. removing.
Saving a figure may involve temporarily resizing the canvas (e.g. due to different dpi), but we don't want that resize to be propagated to the actual GUI widget. Ensure this by temporarily unsetting the manager as well.
c92e48b to
a6b34d4
Compare
|
I like how direct of a solution this is. |
|
You know how I value simplicity and elegance :-) </irony> |
|
@anntzer I just had a thought: what about making a context manager? It seems like the natural way to handle something like this. It might cause less of the details to bleed into the (already lengthy) function body. Of course, you'd have to indent a ton of lines... Just a thought, feel free to disregard. |
|
I have a plan to make a setattr_cm() contextmanager so that one can do which is a pattern used again and again throughout the codebase. I don't think it's worth implementing a one-shot version for this PR. |
|
Fair--I like your general solution. |
Saving a figure may involve temporarily resizing the canvas (e.g. due to
different dpi), but we don't want that resize to be propagated to the
actual GUI widget. Ensure this by temporarily unsetting the manager as
well.
Closes #8736, #10287, and (I believe) #8852. #9131 still remains valid though I believe.
Milestoning as 2.2 only to match the above mentioned issues.
PR Summary
PR Checklist