Skip to content
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

Handle new FigureCanvasBase.device_pixel_ratio. #317

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Apr 6, 2021

This is a corollary to
matplotlib/matplotlib#19126.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2021

Binder 👈 Launch a binder notebook on branch QuLogic/ipympl/dpi-update

@QuLogic
Copy link
Member Author

QuLogic commented Apr 6, 2021

There's a _dpi_ratio thing in Canvas, but I'm not sure if I need to do the same for _device_pixel_ratio (as set in FigureCanvasBase instead of FigureCanvasWebAgg).

@ianhi
Copy link
Collaborator

ianhi commented Apr 7, 2021

Thanks!

This looks safe to merge prior to the the matplotlib PR correct? I think it makes sense to merge this prior to #314

@QuLogic
Copy link
Member Author

QuLogic commented Apr 7, 2021

It should be safe yes, as the new message will just be logged as unknown in older Matplotlib.

@ianhi
Copy link
Collaborator

ianhi commented Apr 8, 2021

There's a _dpi_ratio thing in Canvas, but I'm not sure if I need to do the same for _device_pixel_ratio (as set in FigureCanvasBase instead of FigureCanvasWebAgg).

That's all the way back from the very first commit 047ed95 with a comment by saying: # Must declare the superclass private members. @SylvainCorlay you left that comment do you know why they need to be declared? It's not clear to me why we need to transform those into traitlet variables if we aren't syncing them to the frontend.

@ianhi
Copy link
Collaborator

ianhi commented Apr 15, 2021

Actually looking back at #264 I'm not sure that that comment is still relevant. At the very least nothing bad happened when we removed all those traitlets.

So in conclusion I don't think you need to change anything more. If anything we could even remove the _dpi_ratio being a traitlet.

@ianhi ianhi merged commit beec42b into matplotlib:master Apr 16, 2021
@ianhi
Copy link
Collaborator

ianhi commented Apr 16, 2021

Thanks!

@QuLogic QuLogic deleted the dpi-update branch April 16, 2021 21:36
@SylvainCorlay
Copy link
Member

Sorry I should have looked into this earlier.

One issue with fetching the device pixel ratio from the front-end (which is what we used to do in the past) is that it may depend on the browser window, so that different views of the same figure would have different values for that property.

@ianhi
Copy link
Collaborator

ianhi commented Apr 16, 2021

One issue with fetching the device pixel ratio from the front-end (which is what we used to do in the past)

As far as I can tell that was how ipympl behaved prior to this PR as well. This PR just sends the same information to where it needs to be on the python side for mpl 3.5.

Is there any way to deal with this? It seems to me that the frontend GUI framework is necessarily the source of truth for this information.

@SylvainCorlay
Copy link
Member

As far as I can tell that was how ipympl behaved prior to this PR as well. This PR just sends the same information to where it needs to be on the python side for mpl 3.5.

I thought I removed that logic at some point because of that problem. If I recall correctly, we decided for an arbitrary fixed number (which could be overwritten in the backend).

Is there any way to deal with this? It seems to me that the frontend GUI framework is necessarily the source of truth for this information.

Setting an arbitrary number may be good enough, since the ipympl front-end isn't really meant for print, pixel-units may be good enough.

@ianhi
Copy link
Collaborator

ianhi commented Apr 16, 2021

I thought I removed that logic at some point because of that problem. If I recall correctly, we decided for an arbitrary fixed number (which could be overwritten in the backend).

I think this is quantity we are talking about?

this.ratio = (window.devicePixelRatio || 1) / backingStore;

On my monitor at least (not hidpi) this is mostly affected by the zoom level of the jupyterlab tab.

@SylvainCorlay
Copy link
Member

What I meant is that it is impacted if you open the same notebook with two different computers at the same time. As we are working on live collaboration, this may happen in the near future.

@SylvainCorlay
Copy link
Member

Note that I am totally fine with keeping this for now, but we may want to keep this issue in mind. A solution could be to completely get rid of that variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants