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

Add dpi info to saved PNGs, and scale images in reports #246

Merged
merged 1 commit into from
May 5, 2023

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Apr 21, 2023

Issue

Fixes #240.

Description of changes
  • report_plot scales the image (via CSS' zoom property) if the image writer provides a dict with meta data that includes pixel ratio.
  • DPI information is now also included in saved .png files.
  • ... and in clipboard. (This seems to have no effect on macOS, but the problem is in Qt. Even if I do image = QImage("x.png"); QApplication.clipboard().setImage(image), where "x.png" is a 144 dpi png, the image in the clipboard is twice the size, with 72 dpi. It's no problem, however; the user will usually resize the pasted image to some convenient size.)
Includes
  • Code changes
  • Tests
  • Documentation

@janezd janezd changed the title Add dpi to saved PNGs, and scale images in reports Add dpi info to saved PNGs, and scale images in reports Apr 21, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 71.42% and project coverage change: -1.46 ⚠️

Comparison is base (b0f3206) 72.20% compared to head (57416ff) 70.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #246      +/-   ##
==========================================
- Coverage   72.20%   70.75%   -1.46%     
==========================================
  Files          34       34              
  Lines        9023     9036      +13     
==========================================
- Hits         6515     6393     -122     
- Misses       2508     2643     +135     
Impacted Files Coverage Δ
orangewidget/io.py 55.24% <65.00%> (-19.40%) ⬇️
orangewidget/report/report.py 56.33% <87.50%> (-0.13%) ⬇️

... and 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@markotoplak
Copy link
Member

markotoplak commented Apr 24, 2023

I've tried this on my non-highres Linux laptop. It certainly changes something. If I export a scatterplot as a PNG with the old code I see the following with identify -verbose -units PixelsPerInch file.png

Geometry: 805x663+0+0
Resolution: 96.01x96.01
Print size: 8.38454x6.90553

And with the new code:

Geometry: 805x663+0+0
Resolution: 72.01x72.01
Print size: 11.179x9.20705

So the new code behaves as specified, but is it really correct? It seems that resolutions was already set properly before. Can you share similar info from a PNG exported on a retina Mac?

Otherwise this code does not break anything for me, a non-retina user.

@janezd
Copy link
Contributor Author

janezd commented Apr 25, 2023

I get 144x144, as I should.

I don't know where your 96 came from. Now that it is changed to 72, some programs might show it larger then before. But to my experience, most either ignore it (e.g. browsers) or it doesn't matter (e.g. Word, Powerpoint, where the user resizes the image anyway).

@markotoplak
Copy link
Member

With this PR I understand that we get either 72 or 144. But I saw that dpi was saved correctly before on my computer (it is set to 96 dpi), so I wonder if it was also saved correctly on your Mac on master. If it is, I'd remove the dpi-setting code from the png exporter.

@janezd
Copy link
Contributor Author

janezd commented Apr 28, 2023

For me, it was always 72. Now, with 144, the saved image has the same size as the original in the widget.

I would think that using devicePixelRatio should result in the appropriate image size, because Qt uses this same scaling factor internally. If your correct dpi is 96, than devicePixelRatio should be returning 1.33?

Could you please investigate this? We need to fix this and I don't have a suitable machine to try it on.

@markotoplak
Copy link
Member

Sure, I'll look into this.

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.

Images in reports are too large
4 participants