Support videos in Android CameraRoll.getPhotos #11877

kevinresol · 2017-01-13T09:46:53Z

Support getting videos for CameraRoll.getPhotos() on Android. Before this PR, assetType is ignored in Android.

facebook-github-bot · 2017-01-13T09:46:59Z

By analyzing the blame information on this pull request, we identified @AaaChiuuu and @corbt to be potential reviewers.

facebook-github-bot · 2017-01-13T09:47:00Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

facebook-github-bot · 2017-01-13T09:50:37Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

hramos · 2017-01-20T20:44:45Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

@@ -279,6 +286,19 @@ protected void doInBackgroundGuarded(Void... params) {
        selection.append(" AND " + SELECTION_BUCKET);
        selectionArgs.add(mGroupName);
      }
+      if (!TextUtils.isEmpty(mAssetType)) {
+        if(mAssetType.equals("Videos")) {


Can you compare against a constant here instead of the string, "Videos"?

Do i need to to upgrade my RN to 0.47.2. because below version doesn't have any video support

This PR was not merged.

I am using RN version 0.45.0. Video support is not there in 0.45.0. What should I do. Please let me know

hramos · 2017-01-20T20:45:00Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

@@ -226,6 +228,7 @@ public void getPhotos(final ReadableMap params, final Promise promise) {
    int first = params.getInt("first");
    String after = params.hasKey("after") ? params.getString("after") : null;
    String groupName = params.hasKey("groupName") ? params.getString("groupName") : null;
+    String assetType = params.hasKey("assetType") ? params.getString("assetType") : "Photos";


Can you use a constant here instead of "Photos"?

Do you mean using a constant in Java or exporting the constant to JS? Because I found that the iOS companion didn't export constants to JS.

I meant using a constant in Java, but after taking another look at this file, it seems like that would be against convention. Disregard.

hramos · 2017-01-20T20:45:25Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

@@ -294,7 +314,7 @@ protected void doInBackgroundGuarded(Void... params) {
      // an SQLite DB and forwards parameters to it without doing any parsing / validation.
      try {
        Cursor photos = resolver.query(
-            Images.Media.EXTERNAL_CONTENT_URI,
+            MediaStore.Files.getContentUri("external"),


Can you explain why this change is needed?

This is because querying against Images.Media.EXTERNAL_CONTENT_URI will only gives you images. So we need to query against the "whole external content uri" and use MediaStore.Files.FileColumns.MEDIA_TYPE to filter out what we need, either photos or videos or both.

hramos

Approved. It looks like tests are not passing at this time, however.

kevinresol · 2017-01-24T06:52:21Z

Approved. It looks like tests are not passing at this time, however.

I am not very familiar with RN's test suite. Is there something I should do here?

mkonicek · 2017-02-01T10:33:31Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

@@ -347,12 +367,13 @@ private static void putEdges(
    int heightIndex = IS_JELLY_BEAN_OR_LATER ? photos.getColumnIndex(Images.Media.HEIGHT) : -1;
    int longitudeIndex = photos.getColumnIndex(Images.Media.LONGITUDE);
    int latitudeIndex = photos.getColumnIndex(Images.Media.LATITUDE);
+    int dataIndex = photos.getColumnIndex(MediaStore.MediaColumns.DATA);


This variable should not be called photos anymore since it's either photos or vides, correct? How about media?

Why is dataIndex needed when it wasn't needed previously. Please add a comment above this line explaining what dataIndex is and why it's needed.

I tried to limit the amount of changes to functional parts only. There are quite a few things that can be improved but I am just afraid to expand the scope of this PR too much.

dataIndex is needed for extracting the uri of the file. I found this the easiest way to reliably obtain the file uri.

mkonicek · 2017-02-01T10:35:04Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

@@ -279,6 +286,19 @@ protected void doInBackgroundGuarded(Void... params) {
        selection.append(" AND " + SELECTION_BUCKET);
        selectionArgs.add(mGroupName);
      }
+      if (!TextUtils.isEmpty(mAssetType)) {
+        if(mAssetType.equals("Videos")) {


nit: Space after if

mkonicek · 2017-02-01T10:35:22Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

+        if(mAssetType.equals("Videos")) {
+          selection.append(" AND " + MediaStore.Files.FileColumns.MEDIA_TYPE + " = "
+            + MediaStore.Files.FileColumns.MEDIA_TYPE_VIDEO);
+        } else if(mAssetType.equals("Photos")) {


space after if

mkonicek · 2017-02-01T10:36:10Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

@@ -248,6 +252,7 @@ public void getPhotos(final ReadableMap params, final Promise promise) {
    private final int mFirst;
    private final @Nullable String mAfter;
    private final @Nullable String mGroupName;
+    private final @Nullable String mAssetType;


Remove @Nullable from here because your code below expects it's always "Photos" or "Videos".

mkonicek · 2017-02-01T10:37:16Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

        @Nullable ReadableArray mimeTypes,
        Promise promise) {
      super(context);
      mContext = context;
      mFirst = first;
      mAfter = after;
      mGroupName = groupName;
+      mAssetType = assetType;


Validation: Throw a NPE if nullor an empty string is passed.

mkonicek · 2017-02-01T10:37:30Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

@@ -256,13 +261,15 @@ private GetPhotosTask(
        int first,
        @Nullable String after,
        @Nullable String groupName,
+        @Nullable String assetType,


Remove @Nullable.

mkonicek · 2017-02-01T10:38:33Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

@@ -279,6 +286,19 @@ protected void doInBackgroundGuarded(Void... params) {
        selection.append(" AND " + SELECTION_BUCKET);
        selectionArgs.add(mGroupName);
      }
+      if (!TextUtils.isEmpty(mAssetType)) {


This can't be empty - correct? Otherwise the code below won't filter photos or videos. What would the method return then?

mkonicek · 2017-02-01T10:41:27Z

ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java

-    Uri photoUri = Uri.withAppendedPath(
-        Images.Media.EXTERNAL_CONTENT_URI,
-        photos.getString(idIndex));
+    Uri photoUri = Uri.parse("file://" + photos.getString(dataIndex));


Why get rid of the EXTERNAL_CONTENT_URI here and not use MediaStore.Files.getContentUri("external") like above?

Because it simply won't work to append the id directly to the external content uri.

Needs some changes.

gevgasparyan · 2017-03-28T06:39:10Z

is this issue already resolved?

Ehesp · 2017-03-29T13:30:01Z

@kevinresol @mkonicek Would it be possible to make headway on this? As far as I can see videos still don't show from the camera roll.

kevinresol · 2017-03-29T13:41:49Z

Sorry but I can't maintain this anymore. If anyone is interested feel free to fork from my branch and create another PR.

kesha-antonov · 2017-04-30T07:54:20Z

Hey all.
I've created new PR: #13715

Please vote up to get it merged ASAP!

kevinresol added 2 commits January 13, 2017 17:43

Support videos in Android CameraRoll.getPhotos

14a8e57

Remove unused import

16c6d1f

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 13, 2017

hramos reviewed Jan 20, 2017

View reviewed changes

hramos previously approved these changes Jan 23, 2017

View reviewed changes

mkonicek reviewed Feb 1, 2017

View reviewed changes

mkonicek added GH Review: needs-revision and removed GH Review: review-needed labels Feb 1, 2017

JBerendes mentioned this pull request Mar 25, 2017

Android: CameraRoll.getPhotos({assetType: 'Videos'}) doesn't return videos #13147

Closed

kevinresol closed this Mar 29, 2017

kesha-antonov mentioned this pull request Apr 30, 2017

[Android] CameraRoll support for Videos #13715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support videos in Android CameraRoll.getPhotos #11877

Support videos in Android CameraRoll.getPhotos #11877

kevinresol commented Jan 13, 2017

facebook-github-bot commented Jan 13, 2017

facebook-github-bot commented Jan 13, 2017

facebook-github-bot commented Jan 13, 2017

hramos Jan 20, 2017

ManikSmartdata Aug 29, 2017

hramos Aug 29, 2017

ManikSmartdata Aug 30, 2017

hramos Jan 20, 2017

kevinresol Jan 21, 2017 •

edited

Loading

hramos Jan 23, 2017

hramos Jan 20, 2017

kevinresol Jan 21, 2017

hramos left a comment

kevinresol commented Jan 24, 2017

mkonicek Feb 1, 2017

mkonicek Feb 1, 2017 •

edited

Loading

kevinresol Feb 13, 2017

mkonicek Feb 1, 2017

mkonicek Feb 1, 2017

mkonicek Feb 1, 2017

mkonicek Feb 1, 2017 •

edited

Loading

mkonicek Feb 1, 2017

mkonicek Feb 1, 2017

mkonicek Feb 1, 2017

kevinresol Feb 13, 2017

gevgasparyan commented Mar 28, 2017

Ehesp commented Mar 29, 2017

kevinresol commented Mar 29, 2017

kesha-antonov commented Apr 30, 2017

Support videos in Android CameraRoll.getPhotos #11877

Support videos in Android CameraRoll.getPhotos #11877

Conversation

kevinresol commented Jan 13, 2017

facebook-github-bot commented Jan 13, 2017

facebook-github-bot commented Jan 13, 2017

facebook-github-bot commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinresol Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hramos left a comment

Choose a reason for hiding this comment

kevinresol commented Jan 24, 2017

Choose a reason for hiding this comment

mkonicek Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gevgasparyan commented Mar 28, 2017

Ehesp commented Mar 29, 2017

kevinresol commented Mar 29, 2017

kesha-antonov commented Apr 30, 2017

kevinresol Jan 21, 2017 •

edited

Loading

mkonicek Feb 1, 2017 •

edited

Loading

mkonicek Feb 1, 2017 •

edited

Loading