FIX: use cached renderer on Legend.get_window_extent#11971
Conversation
tacaswell
added
the
Release critical
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
Ooops, sorry about that. Hard to tell what artists require a renderer argument if we use generic *args, **kwargs.
Here, the only suggestion would be that renderer=None be explained.
| fig.canvas.draw() | ||
|
|
||
| leg.get_window_extent() | ||
| leg2.get_window_extent() |
There was a problem hiding this comment.
There is no assert in here. What exactly should this test? Just that get_window_extent() can be called without renderer?
The comment does tell differently. It says something about the "expected extent" and "various dpi". Both of which I don't see in the test code.
There was a problem hiding this comment.
I was lazy and did not remove the comment.
Yes, the test is just that this does not raise.
tacaswell
force-pushed
the
fix_legend_get_window_extent
branch
from
e1e4307 to
873ae23
Compare
| 'Return extent of the legend.' | ||
| return self._legend_box.get_window_extent(*args, **kwargs) | ||
| if renderer is None: | ||
| renderer = self.figure._cachedRenderer |
There was a problem hiding this comment.
So, we have lots of get_window_extent(renderer=...) in the code base. Is it all necessary or could a lot of it be safely replaced by this? I guess I can understand for objects that don't have a reference to their figure, but in general, I think most objects know how to get back to their containing figure...
There was a problem hiding this comment.
I don't think we want to always fall back to the cached renderer. There are some use-cases (such as the mixed-mode vector/raster cases where we need to be able to specify which renderer we want to be using with out having to propgate that state globally.
There was a problem hiding this comment.
I think the mixed mode renderer is the only case where this could be a problem, right?
But in fact even for the mixed mode renderer we're not doing this correctly right now, because artist.draw(renderer) will store the instance of MixedModeRenderer as the _renderer attribute, so e.g. even if the artist was drawn rasterized, by the end of the draw the MixedModeRenderer will have reset itself to vectorized and later calls to get_window_extent() will use the cached MixedModeRenderer in the vectorized state.
| return self._legend_title_box._text | ||
|
|
||
| def get_window_extent(self, *args, **kwargs): | ||
| def get_window_extent(self, renderer=None): |
There was a problem hiding this comment.
That's technically a backward incompatible change (although, as I've mentioned in a previous PR doing something similar, I think this is a change that is worth making.)
|
Thanks @tacaswell |
|
I still get a kick when my PRs get merged 😄 |
…971-on-v3.0.x Backport PR #11971 on branch v3.0.x (FIX: use cached renderer on Legend.get_window_extent)
|
I have the same issue using seaborn. |
|
@Dannyad84 with what version of matplotlib? |
|
n/m had the wrong version. Updated and it worked. Thanks for the quick response!! ;) |
6 participants
Closes #11970
PR Summary
PR Checklist