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

[expo-media-library] Add isNetworkAsset flag to ios #8800

Closed

Conversation

jarvisluong
Copy link
Contributor

Why & How

Mentioned in #8541

Test Plan

With a real device, make huge asset so that iCloud delete the local version, then open the test app and select that asset

@jarvisluong jarvisluong marked this pull request as draft June 14, 2020 09:18
@jarvisluong
Copy link
Contributor Author

jarvisluong commented Jun 14, 2020

@byCedric @tsapeta

One thing I just noticed that the key PHImageResultIsInCloudKey and PHContentEditingInputResultIsInCloudKey is only present if we make networkAccessAllowed to NO like in here:

options.networkAccessAllowed = YES;

However, setting that flag to NO won't download the iCloud asset to the device, so we would need to fetch first with NO value, then again with YES for the asset to be downloaded. Do you think we should go with this solution?

Otherwise, we would need to make the method getAssetInfoAsync to be more configurable in JS side (in order to dynamically specify PHContentEditingInputRequestOptions and PHVideoRequestOptions

Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

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

Otherwise, we would need to make the method getAssetInfoAsync to be more configurable in JS side (in order to dynamically specify PHContentEditingInputRequestOptions and PHVideoRequestOptions

I believe that is the best option - I don't like making two calls for the asset.
Nevertheless, please update the documentation ;)

@tsapeta
Copy link
Member

tsapeta commented Jun 15, 2020

Yeah, I agree that making it more configurable is a good idea 👍

Does it also mean that PHContentEditingInputResultIsInCloudKey is not available when networkAccessAllowed is set to YES? What's its value then? We need to make sure it's not nil because it would throw an exception when converting it to JS object. We would need to use [NSNull null] instead of nil.

@jarvisluong
Copy link
Contributor Author

Yeah, I agree that making it more configurable is a good idea 👍

Does it also mean that PHContentEditingInputResultIsInCloudKey is not available when networkAccessAllowed is set to YES? What's its value then? We need to make sure it's not nil because it would throw an exception when converting it to JS object. We would need to use [NSNull null] instead of nil.

You are right, the value will be nil

@jarvisluong
Copy link
Contributor Author

I started working on an updated PR, hence closing this one

@jarvisluong jarvisluong deleted the feature/network-asset-flag branch July 28, 2020 19:01
lukmccall pushed a commit that referenced this pull request Jul 29, 2020
# Why

This is an enhanced PR from #8800

# How

I added `options` as the optional second parameter to `MediaLibrary.getAssetInfoAsync`.

# Test Plan

Currently I modified `MediaDetailsScreen` in bare-expo app with payload of `shouldDownloadFromNetwork` to `false` so that the response payload also contains the field `isNetworkAsset`.

# Open question

Since this changes the public API of `MediaLibrary`, I wonder how should I update the bare-expo app to reflect this option?
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.

None yet

3 participants