5196: Fix location stripped from EXIF metadata #5227

RitikaPahwa4444 · 2023-05-30T16:18:47Z

Description (required)

What changes did you make and why?

Moved the ACCESS_MEDIA_LOCATION permission check to the MainActivity so that it is asked right after the login and media location is retained even when the user does not make use of the custom selector and uses other ways to upload an image.

Tests performed (required)

Tested prodDebug on Moto g62 with API level 32.

…cation info in EXIF metadata

kartikaykaushik14 · 2023-05-30T17:50:14Z

Hi Rikita,

I tried to run your branch to find the root cause of the test failures. It requires upgrading to a newer SDK version of 29 (Currently at 21). But I'm not sure how feasible this is since we try to keep the SDK version as low as possible to not disadvantage people who do not have access to recent technology. Version 29 covers only about 68% of the android users.

224227843-e15417cc-0379-4bb2-83ac-f306c728605e

nicolas-raoul · 2023-05-31T02:46:11Z

Thanks Ritika for the pull request and Kartikay for the code review and investigation!

I believe that this similar pull request did not break unit tests... How about adding more if statements where needed so that each version of Android gets tested successfully, without upgrading to SDK version of 29 if possible?

RitikaPahwa4444 · 2023-05-31T05:05:51Z

I thought the CI is failing on master too and did not add any extra if statements to the code in that pull request. We are already using 29 as the minSDK version and the app does not run on any devices running an API level below 29. Some of the tests did have SDK version as 21 before the minSDK version was changed. I wonder if the app would run with an SDK version of 21 in the tests as it is not running on physical devices too🤔I'll just check out. Thank you for the hint!

sivaraam · 2023-05-31T06:18:07Z

@RitikaPahwa4444 You are right. The test failures are not a consequence of your changes. They're due to bumping the minSDk to 29 (which we should reduce back to 21 soonish [ref]).

As a workaround, you could feel free to specify the minSdk as 21 locally and run the unit tests. That should help for the meanwhile.

nicolas-raoul · 2023-05-31T06:35:23Z

Thanks for letting us know about the unit tests failing in the current master, I created a new issue about that: #5228

It seems like unit tests were running fine for the PR I mentioned above: https://github.com/commons-app/apps-android-commons/pull/5190/checks so hopefully the changes in the present PR should not trigger new fails either. :-)

nicolas-raoul · 2023-06-02T13:45:52Z

When trying the app with this pull request, unfortunately the location is lost when uploading via Nearby.
Location is still not lost when uploading via the custom picker, though.
In other words, both behave the same as on the master branch.

RitikaPahwa4444 · 2023-06-02T14:03:54Z

Hi Nicolas, I'm somehow not able to reproduce this on my Android 13 emulator and would need some additional details too, to understand the underlying cause in case of Nearby. Could you please share a screencast too? May be I'm missing out on something while testing :')

sivaraam · 2023-06-02T14:53:08Z

@RitikaPahwa4444 As we've observed, the MEDIA_LOCATION permission seems to be handled differently in Android 13. Could you kindly try checking what Nicolas mentioned on an Android 12 device?

Also, could you kidnly explore a bit on why the behavior is a bit different om Android 13? Any hints on it would be helpful with solving the core issue in a bettwe way 🙂

RitikaPahwa4444 · 2023-06-02T15:35:00Z

@sivaraam, Nearby is working fine for me on both Android 12 and 13. I think if the ACCESS_MEDIA_LOCATION is working fine for the custom selector, it should work for Nearby too. I'll check why the behaviour is still different on some devices.

sivaraam · 2023-06-06T05:51:06Z

@sivaraam, Nearby is working fine for me on both Android 12 and 13. I think if the ACCESS_MEDIA_LOCATION is working fine for the custom selector, it should work for Nearby too.

I could confirm this. This MR results in the location being recognized while using the normal file picker / while uploading via Nearby. I'm check this using a Nord running Android 12.

With respect to checking this on Android 13, our app still only targets API level 31 (Android 11). Android 13 has quite a few changes w.r.t accessing media files [ref]. So, would it be ideal to first make our app target API level 33 while making the changes necessary for the same before verifying if the choosing media files works properly on it ?

This article has some good information: Android 13 changelog: A deep dive by Mishaal Rahman. Specifically, I found the sections on "More granular media file permission and "system photo picker" to be informative.

RitikaPahwa4444 · 2023-06-06T06:24:52Z

I don't think the new Photo Picker allows location access through EXIF data. This photo picker ignores the ACCESS_MEDIA_LOCATION permission completely, as discussed in this comment on the Google issue tracker. So, I am not sure if targeting API level 33 will have any effect as far as this issue is concerned.

…iles

nicolas-raoul · 2023-06-09T01:52:48Z

app/src/main/java/fr/free/nrw/commons/filepicker/FilePicker.java

@@ -201,7 +201,7 @@ private static boolean isPhoto(Intent data) {
    }

    private static Intent plainGalleryPickerIntent() {
-        Intent intent = new Intent(Intent.ACTION_GET_CONTENT);
+        Intent intent = new Intent(Intent.ACTION_OPEN_DOCUMENT);


Could you please add a comment explaining in details how we came to this? Do not hesitate to link to existing GitHub comments, the Android bug tracker, etc. Don't hesitate to write a long comment with several paragraphs if you need. Thanks!

I have explained the change in a comment.

nicolas-raoul · 2023-06-14T13:51:29Z

app/src/main/java/fr/free/nrw/commons/settings/SettingsFragment.java

@@ -162,6 +175,26 @@ public boolean onPreferenceClick(Preference preference) {
        }
    }

+    /**
+     * The new Photo Picker with GET_CONTENT takeover redacts location tags


I would write On some devices, the new Photo Picker with GET_CONTENT takeover redacts location tags from EXIF metadata.

I've modified it now.

nicolas-raoul · 2023-06-14T13:55:14Z

@kartikaykaushik14 Any other comment maybe? :-)

…ore clarity

sivaraam · 2023-06-15T02:34:40Z

app/src/main/java/fr/free/nrw/commons/filepicker/FilePicker.java

+         * Reported on the Google Issue Tracker: https://issuetracker.google.com/issues/243294058
+         * Status: Won't fix (Intended behaviour)
+         *
+         * Switched intent from ACTION_GET_CONTENT to ACTION_OPEN_DOCUMENT


This comment could be updated a bit to reflect the current state of the code where it chooses the intent conditionally based on user preference.

The reason for the same could also be conveyed briefly.

sivaraam · 2023-06-15T04:11:49Z

app/src/main/res/values/strings.xml

@@ -440,6 +440,9 @@ Upload your first media by tapping on the add button.</string>
  <string name="ends_on">Ends on:</string>
  <string name="display_campaigns">Display campaigns</string>
  <string name="display_campaigns_explanation">See the ongoing campaigns</string>


This title might not be so helpful. Could we think of a better title for the description?

Something like "Use document style photo picker" amd invert the values accordingly.

sivaraam · 2023-06-15T04:21:20Z

app/src/main/res/values/strings.xml

@@ -440,6 +440,9 @@ Upload your first media by tapping on the add button.</string>
  <string name="ends_on">Ends on:</string>
  <string name="display_campaigns">Display campaigns</string>
  <string name="display_campaigns_explanation">See the ongoing campaigns</string>
+  <string name="get_content_photo_picker_title">Use GET_CONTENT photo picker</string>
+  <string name="get_content_photo_picker_explanation">Disable if your pictures get uploaded without location</string>


Clearly, it's incorrect to claim that this switch would magically fix all location loss issues. So, may be tweak this into something like (assuming we're doing the above suggested inversion):

Enable if you're using the new photo picker to upload images. It has the potential to strip location information from images.

We would need some way for users to help recognize the new photo picker.

sivaraam · 2023-06-15T10:06:10Z

app/src/main/res/values/strings.xml

@@ -440,6 +440,9 @@ Upload your first media by tapping on the add button.</string>
  <string name="ends_on">Ends on:</string>
  <string name="display_campaigns">Display campaigns</string>
  <string name="display_campaigns_explanation">See the ongoing campaigns</string>
+  <string name="get_content_photo_picker_title">Use GET_CONTENT photo picker</string>
+  <string name="get_content_photo_picker_explanation">Disable if your pictures get uploaded without location</string>
+  <string name="location_loss_warning">Please make sure that this new Android picker does not strip location from your pictures.</string>


This message isn't going to help much as we can't expect the users to know what the "new Android picker" means and whether / not it strips location.

It would be great to tweak this in a way that helps them in some way. Some set of actionable steps to help them verify this would be better.

The only way to not strip locations while using the new picker is to use the "Browse.." option in the new picker's overflow menu. That does not have any location stripping issues since it goes to the Documents UI based picker. We could convey this may be in the dialog.

Personally I believe this is good enough for now.
Switching this UI element makes a popup with a help link appear. So people who want to know the details will be able to get the full details on that page. The setting's title and explanation should focus on the most essential.
I believe users will naturally understand that this new Android picker refers to the GET_CONTENT photo picker, as this message only appears when enabling it.

It's fine if that we do not need to explain in much detail in the dialog given we have an accompanying help page. My concern is that , description and dialog text expect a bit more Android knowledge that seems ideal. This has been the base of my suggestions and I don't understand how they're not an improvement of the status quo.

I believe users will naturally understand that this new Android picker refers to the GET_CONTENT photo picker, ...

My whole point is we can't expect users to understand what GET_CONTENT even means unless someone explains it to them. To be clear, I'm not suggesting to explain here what GET_CONTENT picker means. I'm only suggesting to use phrasing that would reduce the need for us to even do that :-)

Is it possible to rephrase the warning as - "Please make sure that the current setting option does not strip location from your pictures" or something along these lines?

This way the users who do not have Android knowledge can also understand the intent behind the warning.

Please make sure that the current setting option does not strip location from your pictures

Additionally giving some hint as to how they could identify that locations are stripped (by checking the "Read More" link) would be helpful.

sivaraam · 2023-06-15T10:28:06Z

app/src/main/res/xml/preferences.xml

@@ -70,6 +70,12 @@
          app:singleLineTitle="false"
          android:summary="@string/display_campaigns_explanation"
          android:title="@string/display_campaigns" />
+
+        <SwitchPreference
+            android:defaultValue="false"


I still prefer keeping this turned on by default (ignoring the inversion mentioned above) with a way to suggest turning it off in a particular case. Given this has merged, I'll open a new issue about it with some details.

I would disagree on this one, priority number one is to avoid any data loss, this is far more important than any potential convenience.

My suggestion tries to balance both the data loss and convenience 🙂

More details in this issue: #5242

sivaraam · 2023-06-18T18:12:21Z

@RitikaPahwa4444 Hope you're looking into addressing the changes suggested above (other than the #5242 related on which we could act once we reach a consensus).

RitikaPahwa4444 · 2023-06-19T15:08:31Z

I'm sorry, I'd been delaying this for a while because apparently, it was only this comment that we'd consensus on. Is it only the comment that I need to address as of now?

RitikaPahwa4444 added 2 commits May 30, 2023 21:05

MainActivity: add ACCESS_MEDIA_LOCATION permission check to retain lo…

62b0346

…cation info in EXIF metadata

remove redundant permission check and optimise imports

064d814

FilePicker: switch to ACTION_OPEN_DOCUMENT intent for opening image f…

ad13fe5

…iles

nicolas-raoul requested changes Jun 9, 2023

View reviewed changes

RitikaPahwa4444 added 3 commits June 9, 2023 10:45

add a comment explaining the change

1b9a668

implement GET_CONTENT photo picker toggle switch

e5945ef

add location loss warning pop up

e25783c

nicolas-raoul requested changes Jun 14, 2023

View reviewed changes

SettingsFragment: modify the comment about GET_CONTENT takeover for m…

27dcc77

…ore clarity

nicolas-raoul approved these changes Jun 15, 2023

View reviewed changes

nicolas-raoul merged commit 9a0f35c into commons-app:master Jun 15, 2023

sivaraam reviewed Jun 15, 2023

View reviewed changes

sivaraam mentioned this pull request Jun 25, 2023

Toggle photo picker switch behaviour and tweak phrases #5250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5196: Fix location stripped from EXIF metadata #5227

5196: Fix location stripped from EXIF metadata #5227

RitikaPahwa4444 commented May 30, 2023

kartikaykaushik14 commented May 30, 2023

nicolas-raoul commented May 31, 2023

RitikaPahwa4444 commented May 31, 2023

sivaraam commented May 31, 2023

nicolas-raoul commented May 31, 2023

nicolas-raoul commented Jun 2, 2023

RitikaPahwa4444 commented Jun 2, 2023 •

edited

Loading

sivaraam commented Jun 2, 2023

RitikaPahwa4444 commented Jun 2, 2023

sivaraam commented Jun 6, 2023

RitikaPahwa4444 commented Jun 6, 2023

nicolas-raoul Jun 9, 2023

RitikaPahwa4444 Jun 9, 2023

nicolas-raoul Jun 14, 2023

RitikaPahwa4444 Jun 14, 2023 •

edited

Loading

nicolas-raoul commented Jun 14, 2023 •

edited

Loading

sivaraam Jun 15, 2023 •

edited

Loading

sivaraam Jun 15, 2023

sivaraam Jun 15, 2023

sivaraam Jun 15, 2023

nicolas-raoul Jun 19, 2023

sivaraam Jun 20, 2023

kartikaykaushik14 Jun 21, 2023

sivaraam Jun 21, 2023

sivaraam Jun 15, 2023

nicolas-raoul Jun 15, 2023

sivaraam Jun 18, 2023

sivaraam commented Jun 18, 2023

RitikaPahwa4444 commented Jun 19, 2023

5196: Fix location stripped from EXIF metadata #5227

5196: Fix location stripped from EXIF metadata #5227

Conversation

RitikaPahwa4444 commented May 30, 2023

kartikaykaushik14 commented May 30, 2023

nicolas-raoul commented May 31, 2023

RitikaPahwa4444 commented May 31, 2023

sivaraam commented May 31, 2023

nicolas-raoul commented May 31, 2023

nicolas-raoul commented Jun 2, 2023

RitikaPahwa4444 commented Jun 2, 2023 • edited Loading

sivaraam commented Jun 2, 2023

RitikaPahwa4444 commented Jun 2, 2023

sivaraam commented Jun 6, 2023

RitikaPahwa4444 commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RitikaPahwa4444 Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

nicolas-raoul commented Jun 14, 2023 • edited Loading

sivaraam Jun 15, 2023 • 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

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

Choose a reason for hiding this comment

sivaraam commented Jun 18, 2023

RitikaPahwa4444 commented Jun 19, 2023

RitikaPahwa4444 commented Jun 2, 2023 •

edited

Loading

RitikaPahwa4444 Jun 14, 2023 •

edited

Loading

nicolas-raoul commented Jun 14, 2023 •

edited

Loading

sivaraam Jun 15, 2023 •

edited

Loading