compose: Make it possible to upload videos (on iOS, and on Android 13+) 🎉 #5734
Conversation
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
chrisbobbe
added
the
a-compose/send
This has changes that look interesting for zulip#3624: react-native-image-picker/react-native-image-picker@fcb8aa641 (More details in an upcoming commit with our fix for that issue.) Also, a small change that should let us offer video capture on iOS: react-native-image-picker/react-native-image-picker@4de5e5a39 If we end up having to revert this upgrade because of something unrelated, that change should be easy to patch in with `patch-package`; see b546b07 for an example of using patch-package.
…13+) This issue was stalled while waiting for react-native-image-picker to include a filename extension in the `fileName` property of the asset payload, specifically in the case of videos on Android. We sent two PRs with reasonable approaches to do that, but they went stale: react-native-image-picker/react-native-image-picker#1881 react-native-image-picker/react-native-image-picker#1895 (We need appropriate filename extensions because Zulip servers want to interpret them.) But in January, a commit landed that actually added the filename extensions (!): react-native-image-picker/react-native-image-picker@fcb8aa641 Or at least, it added the extensions in the case where the Intent gives us a `content://` URL for the asset. (Note that the `.getExtensionFromMimeType` function call is added to `getFileTypeFromMime`, which is a helper for `getAppSpecificStorageUri`, which -- starting in that same commit -- is called for videos when the URL has `content://`.) We can guarantee getting a `content://` URL in at least the following way: restrict this feature to Android SDK 33+. As we know (and can confirm from reading code), the library uses Android's preferred new media-picker UI starting at that version: new Intent(MediaStore.ACTION_PICK_IMAGES) and the doc confirms that that indeed gives "content URI(s)": https://developer.android.com/reference/android/provider/MediaStore#ACTION_PICK_IMAGES It'd be nicer to allow video selection on older Android too. But that seems nontrivial, unless we can guarantee getting `content://` URLs from the ACTION_PICK intents created by this library, and I don't think we can. The alternative is a result we've rejected before: a bad, generic file-picker UI that's not specialized to images/videos, via a library codepath that uses Intent.ACTION_GET_CONTENT. Or I suppose I could basically rebase one of my library PRs I mentioned at the top of this commit message, so we'd get filename extensions irrespective of the `content://` detail. We wouldn't even have to wait for it to land; we found `patch-package` quite usable; see, e.g., b546b07. But at this point I think I've spent plenty of time on this, and the benefit of the current approach will be felt by a large and growing portion of our users. Fixes: zulip#3624
Thanks to the recent upgrade of react-native-image-picker, which had a small change to support 'mixed' in `launchCamera`: react-native-image-picker/react-native-image-picker@4de5e5a39 If we end up having to revert that upgrade because of something unrelated, that change should be easy to patch in with `patch-package`; see b546b07 for an example of using patch-package. Fixes: zulip#5733
gnprice
approved these changes
Aug 18, 2023
There was a problem hiding this comment.
Neat! Glad to get these features in; thanks for the investigation. These changes look good.
I tested on Android 13, and all worked as desired:
- the "camera" icon got me a camera interaction where I could successfully take and upload a photo;
- the "picture" icon got me the Android "photo picker" UI, just as described at https://developer.android.com/training/data-storage/shared/photopicker ;
- that "photo picker" UI included videos as well as photos;
- I selected both a video and a photo there, and they both uploaded successfully.
I'll go ahead and merge.
gnprice
force-pushed
the
pr-upload-videos
branch
from
b4e4b5c
to
8f2864a
Compare
|
Thanks much! 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Before this, it actually wasn't possible to upload videos at all, except perhaps through the share-to-Zulip UI we offer on Android. As I discovered today, it actually seems pretty straightforward to fix.
If this r-n-image-picker upgrade seems too risky, a different approach also seems doable: use
patch-package(which we've done before; see b546b07) for the small code change that lets us fix #5733, and then also, for #3624, to apply something like react-native-image-picker/react-native-image-picker#1881 or react-native-image-picker/react-native-image-picker#1895. (Hmm, though I notice that latter PR adds dependencies in the package'spackage.json; perhaps that case is handled bypatch-packagebut I'm not sure.)That said, I tested our existing features that use the library. Specifically, on my iPhone 13 Pro (iOS 16) and the office Android device (Samsung Galaxy S9 running Android 9).
I tested the new features on my iPhone 13 Pro running iOS 16. The only new feature on Android here is restricted to Android 13+.
@gnprice, I'd appreciate your testing on a later Android device running Android 13 or later; I tried with an emulator a few times and it didn't boot/respond in a reasonable time. (And a real device is better for testing these changes anyway. 🙂) In particular, I'm interested to know if the existing image-upload flows still work (camera and library), and whether you can now select and upload videos (#3624).
Fixes: #5733
Fixes: #3624