This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 830
Fix for retina thumbnails being massive #2439
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
577c411
experimental fix for https://github.com/vector-im/riot-web/issues/2985
ara4n 8511bc2
only request thumbs on retina if the original image is too big to be …
ara4n f1b00df
Merge branch 'develop' into matthew/retina
bwindels e002a59
mistake during merge
bwindels 45f3282
cleanup and linting
bwindels 20aedce
more lint
bwindels d400ebd
add new dependencies to lock file
bwindels 15ba24f
Merge branch 'develop' into matthew/retina
bwindels ea71970
name fn to camel case
bwindels 774314b
prevent png chunk parsing and image loading racing with each other
bwindels File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we need to define the macOS header here? The spec defines pretty clear behaviour for how to handle the chunk
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.
there isn’t anything macOS specific here?
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 not macOS specific, but the resolution has to be an exact match due to line 134
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.
@turt2live what were you thinking of here? we're already parsing the png correctly into chunks, and then checking whether the
pHYs
chunk contains hidpi metrics of any flavour.The only slight cheekiness is that it only applies this fix if the DPI is 5660 pixels-per-metre (which macOS uses for its screenshots) rather than trying to support arbitrary resolutions, but the reason for this is that absolute DPI values for screen use (as opposed to print use) are illdefined at the best of times. After all, who's to say that 1024x768 should be viewed on a 15" or 17" monitor, etc?
We could refine this perhaps by assuming that anything higher than 96dpi (3780ppm) should be randomly halved in size when being viewed in Riot/Web, but i can easily see there may be some source of higher-DPI images out there where the human intention is to be viewed at 1-image-pixel = 1-screen-pixel rather than being halved, as is the intent for macOS screenshots. So let's start like this, and then broaden it when we find other images getting mishandled, rather than overshoot.
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.
We do not check that the pHYs header has hipdi metrics of any flavour, only that they have the flavour we want (5669 pixels per metre). I don't think it's impossible for us to handle other sizes. and I do think we should even if other sources of hidpi images are uncommon. We definitely do not need to handle a unit of zero (unknown) though - that's just a nightmare.