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.

Sign up for GitHub

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

Jump to bottom

[iOS] Fix image wrong scale factor when load image from file system #23446

Closed
wants to merge 3 commits into from
Closed

[iOS] Fix image wrong scale factor when load image from file system #23446

wants to merge 3 commits into from

Conversation

zhongwuzw
Copy link
Contributor

Summary

Regression, fix image load from ~/Library not respect scale factor.
Fixes #22383 , the bug comes from Clean up some URL path handling.

Changelog

[iOS] [Fixed] - Fix image wrong scale factor when load image from file system

Test Plan

If we have picture panda, and has @1x/@2x/@3x photo, we can load the correct image based on screen scale factor.
<Image source={{uri: '~/Library/panda.jpg', width: 300, height: 300}} />

@zhongwuzw zhongwuzw requested a review from shergin as a code owner February 14, 2019 03:14
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 14, 2019
@cpojer
Copy link
Contributor

cpojer commented Feb 14, 2019

This change was originally made because accessing imageURL.path for filesystem operations is not safe. Is there a different way to fix this that doesn't fall back to NSURL?

@zhongwuzw
Copy link
Contributor Author

@cpojer Hi, can you please give more details about unsafe? 🤔 you mean the imageURL may not be a valid file URL?

@cpojer
Copy link
Contributor

cpojer commented Feb 14, 2019

@zhongwuzw I actually don't know myself, but this is from the original diff that introduced this change:

It's not safe to use -[NSURL path] for filesystem paths.
This is why NSURL has methods for dealing
with and producing filesystem paths (fileSystemRepresentation, etc).
This change switches to some filesystem-safe NSURL methods.

@zhongwuzw
Copy link
Contributor Author

@cpojer 😂 TBO, I don't get the whole meaning about safe things, I add a file url check that ensure path is a valid file system path, can we ping anyone to check this?

sahrens
sahrens suggested changes Feb 14, 2019
View reviewed changes
React/Base/RCTUtils.m Outdated
fileData = [NSData dataWithContentsOfURL:[imageURL URLByAppendingPathExtension:@"png"]];
} else {
fileData = [NSData dataWithContentsOfURL:imageURL];
NSString *filePath = imageURL.path;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use this instead:

NSString *filePath = [NSString stringWithUTF8String:[imageURL fileSystemRepresentation]];

@zhongwuzw
Copy link
Contributor Author

@sahrens @cpojer Done! 🍻

cpojer
cpojer approved these changes Feb 15, 2019
View reviewed changes
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 15, 2019
facebook-github-bot
facebook-github-bot approved these changes Feb 15, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@zhongwuzw merged commit e5fbd39 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 15, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 15, 2019
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 23, 2019
grabbou pushed a commit that referenced this pull request Mar 11, 2019
@zhongwuzw @grabbou
Fix image wrong scale factor when load image from file system (#23446)
456a984 
Summary:
Regression, fix image load from `~/Library` not respect scale factor.
Fixes #22383 , the bug comes from [Clean up some URL path handling](998197f).

[iOS] [Fixed] - Fix image wrong scale factor when load image from file system
Pull Request resolved: #23446

Differential Revision: D14099614

Pulled By: cpojer

fbshipit-source-id: eb2267b195a05eb70cdc4671536a4c1d47fb03e2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sahrens sahrens sahrens requested changes

@facebook-github-bot facebook-github-bot facebook-github-bot approved these changes

@cpojer cpojer cpojer approved these changes

@shergin shergin Awaiting requested review from shergin

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: Image Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression: iOS images loaded from filesystem don't respect scaling factor in filename
6 participants
@zhongwuzw @cpojer @react-native-bot @sahrens @facebook-github-bot @hramos