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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ios][image-picker] Copy video file instead of moving #5434

Merged
merged 2 commits into from
Aug 27, 2019

Conversation

tsapeta
Copy link
Member

@tsapeta tsapeta commented Aug 24, 2019

Why

Fixes #5321

How

Simply replaced moveItemAtURL:toURL:error with copyItemAtURL:toURL:error, as the first one throws an error due to missing permissions 馃し鈥嶁檪 Probably the temporary directory to which the compressed videos are saved is now protected in iOS 13.

Test Plan

Snack https://snack.expo.io/@tsapeta/imagepicker-from-camera-roll works as expected on iOS 13.

@tsapeta tsapeta requested a review from bbarthec as a code owner August 24, 2019 00:29
@tsapeta tsapeta requested review from ide and sjchmiela August 24, 2019 00:30
Copy link
Contributor

@esamelson esamelson left a 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 delete the file, or will the OS take care of this for us? We've had issues in the past with ImagePicker creating multiple copies of files in temp locations and taking up a bunch of space. Just want to make sure we aren't creating that problem again, inadvertently.

Also would advocate for not backporting this to older SDK versions unless we are planning to cherry-pick to old release branches and create new shell app bases. Otherwise this creates more confusing behavior differences between the client and shell apps (i.e. ImagePicker works on my SDK 33 project in the Expo Client, why is it suddenly failing in my standalone app?). Better to leave it alone in the Expo Client so it's clear you need to upgrade to 35 in order to fix this.

@tsapeta
Copy link
Member Author

tsapeta commented Aug 27, 2019

@esamelson So in terms of deleting the file, looks like in iOS 13 we're no longer able to move the file from that temporary location where these videos are saved. It throws an error - see #5321 issue.

And yeah, you're right about backporting - will revert these changes, but I'm also ok with releasing them for standalone apps.

@tsapeta tsapeta force-pushed the @tsapeta/fix-image-picker-on-ios13 branch from 2d0d939 to fb2282b Compare August 27, 2019 18:15
@tsapeta tsapeta requested a review from esamelson August 27, 2019 18:19
@esamelson
Copy link
Contributor

Cool, sounds good. I would only be hesitant about fixing the shell apps because it takes a fair amount of time to do properly (with testing and everything) and would prefer to invest that time into making SDK 35 better sooner instead 馃檪 but we can revisit if a bunch of people ask for this fix to be backported.

Regarding the file -- I was more wondering if we should try removing it after copying it (and just ignore any errors that happen when removing)? Or do you think that's overkill and the OS will take care of clearing the temp files?

@tsapeta tsapeta merged commit 00f9f9e into master Aug 27, 2019
@tsapeta tsapeta deleted the @tsapeta/fix-image-picker-on-ios13 branch August 27, 2019 23:39
@mvjansen
Copy link

Hi, there still appears to be an issue in ImagePicker.launchImageLibraryAsync. I get the following error when I select a video: "The callback callMethod() exists in module NativeUnimoduleProxy, but only one callback may be registered to a function in a native module"

@sjchmiela
Copy link
Contributor

Hey @mvjansen! Could you please create a new issue with all the details provided? This sounds like a completely different problem than the one fixed by this PR. Thanks!

@Nnoerregaard
Copy link

Nnoerregaard commented Sep 23, 2019

@mvjansen I experienced the exact same issue on iOS 13 before upgrading to expo SDK 35 (I was on SDK 33 previously).
After upgrading, the issue has disappeared

@tomelsner
Copy link

@mvjansen I had the same issue, it disappeared when updating to SDK 35. Make sure you also update "expo-image-picker": "^7.0.0" and you also have "sdkVersion": "35.0.0" set in your app.json.

@mvjansen
Copy link

Thanks for your help everyone - I'd missed updating the sdkVersion in app.json! @Nnoerregaard @tomelsner

@mikeRChambers610
Copy link

I am using Expo SDK 40 and "expo-image-picker": "~9.2.0", but still getting the issue.

When I try to pick a video, my error message is shown as below:

Possible Unhandled Promise Rejection (id: 0): Error: Could not get the image promiseMethodWrapper@http.......

@mikeRChambers610
Copy link

Upgraded to Expo SDK 41 and now using "expo-image-picker": "~10.1.4", still getting the same issue.

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.

ios 13 launchImageLibraryAsync video not work.
7 participants