Skip to content

Support videos in Android CameraRoll.getPhotos #11877

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kevinresol wants to merge 2 commits into from
Closed

Support videos in Android CameraRoll.getPhotos #11877

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14a8e57
Support videos in Android CameraRoll.getPhotos
kevinresol Jan 13, 2017
16c6d1f
Remove unused import
kevinresol Jan 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions ReactAndroid/src/main/java/com/facebook/react/modules/camera/CameraRollManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ public class CameraRollManager extends ReactContextBaseJavaModule {
Images.Media.WIDTH,
Images.Media.HEIGHT,
Images.Media.LONGITUDE,
Images.Media.LATITUDE
Images.Media.LATITUDE,
MediaStore.MediaColumns.DATA
};
} else {
PROJECTION = new String[] {
Expand All @@ -86,7 +87,8 @@ public class CameraRollManager extends ReactContextBaseJavaModule {
Images.Media.BUCKET_DISPLAY_NAME,
Images.Media.DATE_TAKEN,
Images.Media.LONGITUDE,
Images.Media.LATITUDE
Images.Media.LATITUDE,
MediaStore.MediaColumns.DATA
};
}
}
Expand Down Expand Up @@ -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";

@hramos hramos Jan 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kevinresol kevinresol Jan 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hramos hramos Jan 23, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ReadableArray mimeTypes = params.hasKey("mimeTypes")
? params.getArray("mimeTypes")
: null;
Expand All @@ -238,6 +241,7 @@ public void getPhotos(final ReadableMap params, final Promise promise) {
first,
after,
groupName,
assetType,
mimeTypes,
promise)
.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
Expand All @@ -248,6 +252,7 @@ private static class GetPhotosTask extends GuardedAsyncTask<Void, Void> {
private final int mFirst;
private final @Nullable String mAfter;
private final @Nullable String mGroupName;
private final @Nullable String mAssetType;

@mkonicek mkonicek Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private final @Nullable ReadableArray mMimeTypes;
private final Promise mPromise;

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

@mkonicek mkonicek Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove @Nullable.

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

@mkonicek mkonicek Feb 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@mkonicek mkonicek Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if(mAssetType.equals("Videos")) {

@hramos hramos Jan 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ManikSmartdata ManikSmartdata Aug 29, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hramos hramos Aug 29, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR was not merged.

@ManikSmartdata ManikSmartdata Aug 30, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mkonicek mkonicek Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Space after if

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

@mkonicek mkonicek Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after if

selection.append(" AND " + MediaStore.Files.FileColumns.MEDIA_TYPE + " = "
+ MediaStore.Files.FileColumns.MEDIA_TYPE_IMAGE);
} else {
selection.append(" AND " + MediaStore.Files.FileColumns.MEDIA_TYPE + " IN ("
+ MediaStore.Files.FileColumns.MEDIA_TYPE_VIDEO + ","
+ MediaStore.Files.FileColumns.MEDIA_TYPE_IMAGE + ")");
}
}
if (mMimeTypes != null && mMimeTypes.size() > 0) {
selection.append(" AND " + Images.Media.MIME_TYPE + " IN (");
for (int i = 0; i < mMimeTypes.size(); i++) {
Expand All @@ -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"),

@hramos hramos Jan 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this change is needed?

@kevinresol kevinresol Jan 21, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PROJECTION,
selection.toString(),
selectionArgs.toArray(new String[selectionArgs.size()]),
Expand Down Expand Up @@ -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);

@mkonicek mkonicek Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mkonicek mkonicek Feb 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kevinresol kevinresol Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for (int i = 0; i < limit && !photos.isAfterLast(); i++) {
WritableMap edge = new WritableNativeMap();
WritableMap node = new WritableNativeMap();
boolean imageInfoSuccess =
putImageInfo(resolver, photos, node, idIndex, widthIndex, heightIndex);
putImageInfo(resolver, photos, node, idIndex, widthIndex, heightIndex, dataIndex);
if (imageInfoSuccess) {
putBasicNodeInfo(photos, node, mimeTypeIndex, groupNameIndex, dateTakenIndex);
putLocationInfo(photos, node, longitudeIndex, latitudeIndex);
Expand Down Expand Up @@ -386,11 +407,10 @@ private static boolean putImageInfo(
WritableMap node,
int idIndex,
int widthIndex,
int heightIndex) {
int heightIndex,
int dataIndex) {
WritableMap image = new WritableNativeMap();
Uri photoUri = Uri.withAppendedPath(
Images.Media.EXTERNAL_CONTENT_URI,
photos.getString(idIndex));
Uri photoUri = Uri.parse("file://" + photos.getString(dataIndex));

@mkonicek mkonicek Feb 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kevinresol kevinresol Feb 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image.putString("uri", photoUri.toString());
float width = -1;
float height = -1;
Expand Down