Support videos in Android CameraRoll.getPhotos by kevinresol · Pull Request #11877 · react/react-native

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

        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

    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

      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

    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

        selectionArgs.add(mGroupName);
      }
+      if (!TextUtils.isEmpty(mAssetType)) {
+        if(mAssetType.equals("Videos")) {


nit: Space after if

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

+        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

    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

      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

        int first,
        @Nullable String after,
        @Nullable String groupName,
+        @Nullable String assetType,


Remove @Nullable.

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

        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

-    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

Conversation

kevinresol commented Jan 13, 2017

Uh oh!

facebook-github-bot commented Jan 13, 2017

Uh oh!

facebook-github-bot commented Jan 13, 2017

Uh oh!

facebook-github-bot commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinresol Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hramos left a comment

Choose a reason for hiding this comment

Uh oh!

kevinresol commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gevgasparyan commented Mar 28, 2017

Uh oh!

Ehesp commented Mar 29, 2017

Uh oh!

kevinresol commented Mar 29, 2017

Uh oh!

kesha-antonov commented Apr 30, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kevinresol Jan 21, 2017 •

edited

Loading

mkonicek Feb 1, 2017 •

edited

Loading

mkonicek Feb 1, 2017 •

edited

Loading