-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
[ios][file-system] Fix copy async #27208
Conversation
7104035
to
6caa112
Compare
import Photos | ||
|
||
private let assetIdentifier = "ph://" | ||
private let resourceManager = PHAssetResourceManager() |
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.
nit: could we lazy initiate ths instance or even just using local variable in the usage?
PHAssetResourceManager().writeData(...)
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.
Good catch, we should make it lazy. We still need to keep it outside of the function because it has to live long enough for the callback to get executed.
Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines. I've found some issues in your pull request that should be addressed (click on them for more details) 👇
|
|
||
let firstResource = PHAssetResource.assetResources(for: asset).first | ||
if let firstResource { | ||
resourceManager.writeData(for: firstResource, toFile: toUrl, options: nil) { error in |
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.
🟠 SwiftLint: Unused Closure Parameter Violation
Unused parameter in a closure should be replaced with _
Thanks for fixing this @alanjhughes ! |
@alanjhughes I gave it a try, but I'm facing another problem now: the files gets copied, but the promise never resolves. Tried a simple test case of selecting a photo from camera roll and copying it to the documents path. The promise never resolves, but when a got to the directory and check if the file is there, it is.
|
Hi @protetore - This will be fixed by #27381. Apologies for the inconvenience. I shouldn't have missed that |
Why
Closes #27021
When an image is selected from the photo library it will have a url like
ph://***
. OurURL
conversion will accept this but when we accessurl.path
it will be blank, causing the copy to fail.How
Check if we are dealing with a photo library asset and handle the copy using the
PHAssetResourceManager
instead of theFileManager
.Test Plan
All tests still passing in bare-expo
Tested using the provided repro.