Skip to content

Commit

Permalink
compose: Allow uploading videos from media library (iOS, and Android …
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
chrisbobbe committed May 23, 2023
1 parent 73f23b5 commit 025eccc
Showing 1 changed file with 13 additions and 2 deletions.
15 changes: 13 additions & 2 deletions src/compose/ComposeMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,18 @@ const kShouldOfferImageMultiselect =
// Android 13
|| androidSdkVersion() >= 33;

// As of r-n-image-picker v5.3.1, this is needed to ensure that any received
// videos have a `fileName` that includes a filename extension. (Servers
// will interpret the filename extension.) See library implementation.
//
// For older Android, it seems the alternative is a generic file-picker UI
// not specialized to images/videos (in a library codepath that uses
// Intent.ACTION_GET_CONTENT).
const kShouldOfferImagePickerMixedMedia =
Platform.OS === 'ios'
// Android 13
|| androidSdkVersion() >= 33;

type MenuButtonProps = $ReadOnly<{|
onPress: () => void | Promise<void>,
IconComponent: SpecificIconType,
Expand Down Expand Up @@ -215,8 +227,7 @@ export default function ComposeMenu(props: Props): Node {
const handleImagePicker = useCallback(() => {
launchImageLibrary(
{
// TODO(#3624): Try 'mixed', to allow both photos and videos
mediaType: 'photo',
mediaType: kShouldOfferImagePickerMixedMedia ? 'mixed' : 'photo',

quality: 1.0,
includeBase64: false,
Expand Down

0 comments on commit 025eccc

Please sign in to comment.