[media-library][android] retrieve location data from photos on androi… #14413

ajsmth · 2021-09-14T20:45:21Z

Why

Android 10+ requires an additional permission and getter implementation to get exif location data for photos.

From what I can tell, the old implementation still seems to work via the requestLegacyExternalStorage key in Android manifest, but as of 11 this no longer seems to work

I was able to reproduce the location data being missing via a emulator running Android 11

How

The new implementation checks whether or not the device is running Android 10+ and if so, runs a different call to retrieve location info. This method throws an exception if the ACCESS_MEDIA_LOCATION permission is missing from AndroidManifest - so I also added this value as well as handled the exception

Test Plan

From what I can tell, this implementation still doesn't seem to work on the emulator, which is unfortunate, but I was able to confirm that it works on a real device running Android 10

Also - the permissions prompt doesn't seem to change at all between this implementation and the previous, the only concern might be if adding this additional permission would cause issues with app submissions

Checklist

Documentation is up to date to reflect these changes (eg: https://docs.expo.io and README.md).
This diff will work correctly for expo build (eg: updated @expo/xdl).
This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

EvanBacon · 2021-09-14T23:54:35Z

packages/expo-media-library/android/src/main/AndroidManifest.xml

@@ -2,4 +2,5 @@
 <manifest package="expo.modules.medialibrary" xmlns:android="http://schemas.android.com/apk/res/android">
  <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />
  <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
+  <uses-permission android:name="android.permission.ACCESS_MEDIA_LOCATION" />


Maybe it's better to use a config plugin for this so it can be more visible, and easier to remove. Otherwise users will be getting a new permission for a library they've already implemented.

okay will do - I would like to understand better though -- doesn't a config plugin accomplish the same thing? e.g it will add this new permission regardless?

EvanBacon · 2021-09-14T23:54:50Z

...s/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryModule.java

      };
    }
    return new String[]{
      READ_EXTERNAL_STORAGE,
-      WRITE_EXTERNAL_STORAGE
+      WRITE_EXTERNAL_STORAGE,
+            ACCESS_MEDIA_LOCATION,


barthap · 2021-09-15T06:03:21Z

#13123 probably related

barthap · 2021-09-15T06:10:55Z

...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java

+          if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
+            Uri photoUri = Uri.withAppendedPath(MediaStore.Images.Media.EXTERNAL_CONTENT_URI, cursor.getString(idIndex));
+            getExifLocationForUri(contentResolver, photoUri, asset);
+          } else {


Is this block intentionally inside if (exifInterface != null)? As getExifLocationForUri does not depend on that original exifInterface, maybe we can extract it outside?

it does require this check as it also requires the getExifFullInfo - I could extract but then would have to duplicate the logic around adding extra info - I think its a matter of taste so either way let me know what you'd prefer!

Hmm, if most likely nullified "global" ExifInterface implies null exifInterface created by getExifLocationForUri (so most likely there are cases when either none or both of them are null), then it can stay as it is.
And I suspect in most cases it's true (unless permission is not granted, but this is caught in a separate exception).

barthap · 2021-09-15T06:14:14Z

...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java

@@ -358,8 +369,40 @@ static void getExifFullInfo(ExifInterface exifInterface, Bundle response) {
    response.putParcelable("exif", exifMap);
  }

+  //  reference: https://developer.android.com/training/data-storage/shared/media#location-info-photos
+  @RequiresApi(api = Build.VERSION_CODES.Q)
+  static void getExifLocationForUri(ContentResolver contentResolver, Uri photoUri, Bundle asset) throws Exception {


does this throw general Exception or maybe some specific one? It would be good to narrow down the exception range because error handling in MediaLib is already a bit confusing (regarding the real source of error).

Hint: when you hover and click on throws Exception (the Exception word) - Android Studio highlights all calls which throw that exception (or its subclasses)

barthap · 2021-09-15T06:17:19Z

...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java

+    photoUri = MediaStore.setRequireOriginal(photoUri);
+    InputStream stream = contentResolver.openInputStream(photoUri);
+    if (stream != null) {
+      ExifInterface exifInterface = new ExifInterface(stream);


I think we should sync this somehow with #14408

barthap

Maybe a good idea would be to display a meaningful message on rejection, when ACCESS_MEDIA_LOCATION permission is needed

packages/expo-media-library/CHANGELOG.md

barthap · 2021-09-16T06:23:05Z

packages/expo-media-library/android/src/main/java/expo/modules/medialibrary/GetAssets.java

@@ -60,7 +60,7 @@ protected Void doInBackground(Void... params) {
      mPromise.reject(ERROR_UNABLE_TO_LOAD_PERMISSION,
          "Could not get asset: need READ_EXTERNAL_STORAGE permission.", e);
    } catch (IOException e) {
-      mPromise.reject(ERROR_UNABLE_TO_LOAD, "Could not read file", e);
+      mPromise.reject(ERROR_UNABLE_TO_LOAD, "Could not read file or parse EXIF tags", e);


Just rebase mistake or intentional? (EXIF-specific exceptions are now handled elsewhere)

barthap · 2021-09-16T06:23:40Z

...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java

@@ -155,7 +158,7 @@ static void queryAssetInfo(Context context, final String selection, final String
      promise.reject(ERROR_UNABLE_TO_LOAD_PERMISSION,
        "Could not get asset: need READ_EXTERNAL_STORAGE permission.", e);
    } catch (IOException e) {
-      promise.reject(ERROR_IO_EXCEPTION, "Could not read file", e);
+      promise.reject(ERROR_IO_EXCEPTION, "Could not read file or parse EXIF tags", e);


same here 😉

barthap · 2021-09-16T06:31:06Z

...es/expo-media-library/android/src/main/java/expo/modules/medialibrary/MediaLibraryUtils.java

+        } else {
+          asset.putParcelable("location", null);
+        }
+        stream.close();


When ExifInterface creation fails, the stream is never closed. It should be moved to the finally block.

Unfortunately, this means the whole getExifLocationForUri will have to throws IOException instead.

barthap

I have no opinion on the config plugin, besides that looks good to me 😁 thanks for taking my suggestions and ideas into account 😉

…d 10+

# Why [versioned qa][android] test-suite "MediaLibrary" show `Tests were skipped - not enough permissions to run them.` on android 8 device # How requesting `ACCESS_MEDIA_LOCATION` permission only on android 10+ # Test Plan bare-expo test-suite "MediaLibrary" on android 8 device for android 10 emulator, which is expected failed as #14413 mentioned.

# Why [versioned qa][android] test-suite "MediaLibrary" show `Tests were skipped - not enough permissions to run them.` on android 8 device # How requesting `ACCESS_MEDIA_LOCATION` permission only on android 10+ # Test Plan bare-expo test-suite "MediaLibrary" on android 8 device for android 10 emulator, which is expected failed as #14413 mentioned. (cherry picked from commit 35d639f)

expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 14, 2021

ajsmth requested review from lukmccall, barthap and byCedric and removed request for lukmccall and barthap September 14, 2021 23:04

ajsmth marked this pull request as ready for review September 14, 2021 23:04

ajsmth requested a review from tsapeta as a code owner September 14, 2021 23:04

expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 14, 2021

EvanBacon reviewed Sep 14, 2021

View reviewed changes

barthap reviewed Sep 15, 2021

View reviewed changes

barthap linked an issue Sep 15, 2021 that may be closed by this pull request

MediaLibrary.getAssetInfoAsync does not provide exif data on Android API 30 #13123

Closed

ajsmth mentioned this pull request Sep 15, 2021

[media-library] Do not reject on EXIF parsing failure #14408

Merged

6 tasks

ajsmth force-pushed the andrew/android-media-lib-location branch from 7ee70f7 to a64060c Compare September 15, 2021 21:57

barthap reviewed Sep 16, 2021

View reviewed changes

ajsmth requested a review from barthap September 16, 2021 18:54

barthap approved these changes Sep 16, 2021

View reviewed changes

ajsmth mentioned this pull request Sep 22, 2021

[android] add ACCESS_MEDIA_LOCATION to blacklist for android permissions expo/xdl#38

Merged

[media-library][android] retrieve location data from photos on androi…

6d0f621

…d 10+

ajsmth force-pushed the andrew/android-media-lib-location branch from 642587d to 6d0f621 Compare September 23, 2021 22:50

byCedric approved these changes Sep 27, 2021

View reviewed changes

ajsmth merged commit 7fc4786 into master Sep 27, 2021

ajsmth deleted the andrew/android-media-lib-location branch September 27, 2021 18:16

Kudo mentioned this pull request Sep 29, 2021

[media-library] fix permission not granted on android api < 29 #14570

Merged

3 tasks

barthap mentioned this pull request Dec 20, 2021

[android] Fix missing scoped permission #15658

Merged

3 tasks

barthap mentioned this pull request Jan 24, 2022

[media-library][android] Fix requesting optional location permission #16034

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[media-library][android] retrieve location data from photos on androi… #14413

[media-library][android] retrieve location data from photos on androi… #14413

ajsmth commented Sep 14, 2021 •

edited

EvanBacon Sep 14, 2021

ajsmth Sep 15, 2021

EvanBacon Sep 14, 2021

barthap commented Sep 15, 2021

barthap Sep 15, 2021

ajsmth Sep 15, 2021

barthap Sep 15, 2021 •

edited

barthap Sep 15, 2021

barthap Sep 15, 2021

barthap left a comment

barthap Sep 16, 2021

barthap Sep 16, 2021

barthap Sep 16, 2021

barthap left a comment

[media-library][android] retrieve location data from photos on androi… #14413

[media-library][android] retrieve location data from photos on androi… #14413

Conversation

ajsmth commented Sep 14, 2021 • edited

Why

How

Test Plan

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barthap commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barthap Sep 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barthap left a 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

barthap left a comment

Choose a reason for hiding this comment

ajsmth commented Sep 14, 2021 •

edited

barthap Sep 15, 2021 •

edited