-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Move pixel ratio handling into FigureCanvasBase #19126
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
I wonder if this will make #18274 work better (with adjustments there as well) |
afd6e05
to
1023939
Compare
e5a840b
to
3385d1a
Compare
OK, I renamed it, and also made the setter return whether the ratio changed; it seems common in backends to do something in such cases, so that removes some code from all of them. |
3385d1a
to
dfc9872
Compare
self.figure._original_dpi = self.figure.dpi | ||
self.figure.dpi = dpi_ratio * self.figure._original_dpi | ||
self._dpi_ratio = dpi_ratio | ||
def handle_set_device_pixel_ratio(self, event): |
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 guess these handlers are effectively private and we reserve the right to change them as we see fit with no deprecation?
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.
That's a good point, but I would say so. They're tied directly with the JavaScript side of the implementation, and I don't think we've ever guaranteed anything there.
In this case, it's only a rename for consistency's sake, so I could revert it.
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 will likely require a change in ipympl, but I think we should make the changes to keep the function names consistent (in this case).
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 restored the handler for set_dpi_ratio
, as then one can continue using an older version of ipympl
with the new Matplotlib that comes out with this change.
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.
Just one minor point about public/private API.
This is a corollary to matplotlib/matplotlib#19126.
This is a corollary to matplotlib/matplotlib#19126.
This is already implemented in two backends (Qt5 and nbAgg), and I plan to implement it in TkAgg, so it's better to remove the repetition.
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.
@QuLogic can merge on green.
This was not originally implemented in matplotlib#19126, but causes some inconsistencies with other backends. This also sets the initial scale as implemented in matplotlib#18274.
This was not originally implemented in matplotlib#19126, but causes some inconsistencies with other backends. This also sets the initial scale as implemented in matplotlib#18274.
This was not originally implemented in matplotlib#19126, but causes some inconsistencies with other backends. This also sets the initial scale as implemented in matplotlib#18274.
This was not originally implemented in matplotlib#19126, but causes some inconsistencies with other backends. This also sets the initial scale as implemented in matplotlib#18274.
PR Summary
This is already implemented in two backends (Qt5 and nbAgg), and I plan to implement it in TkAgg, so it's better to remove the repetition.
It may also be useful for external backends.
I haven't fully tested this yet (though the test suite passes), as it's waiting for #19083 and #19123.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).