-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix set_size_inches on HiDPI and also GTK4 #21212
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
This is causing Qt figures to double in size as the |
7267d23
to
a0a1294
Compare
So Qt was also buggy because the I've now told Qt to do the conversion as well, so they should all work correctly. |
@@ -2657,10 +2657,9 @@ def set_size_inches(self, w, h=None, forward=True): | |||
if forward: | |||
canvas = getattr(self, 'canvas') | |||
if canvas is not None: | |||
dpi_ratio = getattr(canvas, '_dpi_ratio', 1) |
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.
Note, this always returned 1 since the attribute was renamed, so the resize was always in physical pixels.
9ea14b1
to
38691d7
Compare
I propagated the cast to all the backends with the changes, instead of just Qt, as they all technically use |
38691d7
to
9dc00b4
Compare
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.
This is broken for macosx right now. It keeps doubling in size when jumping between monitors. All other backends look good to me though.
I think the macosx backend is the only one that hasn't been updated to use the proper superclass pixel ratio in #19126, and so it actually does have |
This works for me locally to keep the figure size in check. diff --git a/lib/matplotlib/backends/backend_macosx.py b/lib/matplotlib/backends/backend_macosx.py
index a692504afb..c9bf34f5e4 100644
--- a/lib/matplotlib/backends/backend_macosx.py
+++ b/lib/matplotlib/backends/backend_macosx.py
@@ -30,14 +30,6 @@ class FigureCanvasMac(_macosx.FigureCanvas, FigureCanvasAgg):
FigureCanvasBase.__init__(self, figure)
width, height = self.get_width_height()
_macosx.FigureCanvas.__init__(self, width, height)
- self._dpi_ratio = 1.0
-
- def _set_device_scale(self, value):
- if self._dpi_ratio != value:
- # Need the new value in place before setting figure.dpi, which
- # will trigger a resize
- self._dpi_ratio, old_value = value, self._dpi_ratio
- self.figure.dpi = self.figure.dpi / old_value * self._dpi_ratio
def set_cursor(self, cursor):
# docstring inherited
@@ -63,8 +55,8 @@ class FigureCanvasMac(_macosx.FigureCanvas, FigureCanvasAgg):
dpi = self.figure.dpi
width /= dpi
height /= dpi
- self.figure.set_size_inches(width * self._dpi_ratio,
- height * self._dpi_ratio,
+ self.figure.set_size_inches(width,
+ height,
forward=False)
FigureCanvasBase.resize_event(self)
self.draw_idle()
diff --git a/src/_macosx.m b/src/_macosx.m
index ef999ca9ea..2d666788a3 100755
--- a/src/_macosx.m
+++ b/src/_macosx.m
@@ -1658,7 +1658,7 @@ -(void)drawRect:(NSRect)rect
if (device_scale != new_device_scale) {
device_scale = new_device_scale;
- if (!PyObject_CallMethod(canvas, "_set_device_scale", "d", device_scale, NULL)) {
+ if (!PyObject_CallMethod(canvas, "_set_device_pixel_ratio", "d", device_scale, NULL)) {
PyErr_Print();
goto exit;
} |
That makes sense; I have a bit more cleanup to do and will put that into a separate PR. |
Dismissing this, expecting a follow-up PR to address the doubling size in the macosx backend.
@QuLogic, so you're saying this is ready to go in as-is, or did you have additional clean-up for this PR? |
Not for this PR directly, unless you want to wait for the macosx backend fixes first? I think your patch is effectively correct in that it stops the resizing, but not technically correct as the pixel ratio is not handled in the right place, and not in all locations. Possibly, macosx had some attribute wrong the entire time, but I'm not sure yet. |
@@ -1760,7 +1760,7 @@ def blit(self, bbox=None): | |||
"""Blit the canvas in bbox (default entire canvas).""" | |||
|
|||
def resize(self, w, h): | |||
"""Set the canvas size in pixels.""" |
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.
So this is on FigureCanvasBase
, but it's actually unused. I will modify the docstring for FigureManagerBase.resize
, and mark this one unused.
4635702
to
56fe546
Compare
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.
This currently still has an issue with the MacOSX backend, but #21365 fixes that, so it should go in first probably.
This passes physical pixels to the backend, as it will be more accurate due to the int-cast. Fixes matplotlib#21090
56fe546
to
9869826
Compare
…212-on-v3.5.x Backport PR #21212 on branch v3.5.x (Fix set_size_inches on HiDPI and also GTK4)
PR Summary
I tested GTK3 and GTK4, but could not test Tk because I don't have Windows now.
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).