Skip to content
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.

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

5196: Fix location stripped from EXIF metadata #5227

Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,9 @@
import static fr.free.nrw.commons.wikidata.WikidataConstants.PLACE_OBJECT;

import android.Manifest;
import android.Manifest.permission;
import android.app.Activity;
import android.content.Context;
import android.content.Intent;
import android.os.Build.VERSION;
import android.os.Build.VERSION_CODES;
import androidx.annotation.NonNull;
import fr.free.nrw.commons.R;
import fr.free.nrw.commons.filepicker.DefaultCallback;
Expand Down Expand Up @@ -70,15 +67,6 @@ public void initiateCustomGalleryPickWithPermission(final Activity activity) {
PermissionUtils.checkPermissionsAndPerformAction(activity,
Manifest.permission.WRITE_EXTERNAL_STORAGE,
() -> {
if (VERSION.SDK_INT >= VERSION_CODES.Q) {
PermissionUtils.checkPermissionsAndPerformAction(
activity,
permission.ACCESS_MEDIA_LOCATION,
() -> {},
R.string.media_location_permission_denied,
R.string.add_location_manually
);
}
FilePicker.openCustomSelector(activity, 0);
},
R.string.storage_permission_title,
Expand All @@ -91,7 +79,8 @@ public void initiateCustomGalleryPickWithPermission(final Activity activity) {
*/
private void initiateGalleryUpload(final Activity activity, final boolean allowMultipleUploads) {
setPickerConfiguration(activity, allowMultipleUploads);
FilePicker.openGallery(activity, 0);
boolean isGetContentPickerPreferred = defaultKvStore.getBoolean("getContentPhotoPickerPref");
FilePicker.openGallery(activity, 0, isGetContentPickerPreferred);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import android.content.Context;
import android.content.Intent;
import android.content.SharedPreferences;
import android.content.pm.PackageManager;
import android.os.Build.VERSION;
import android.os.Build.VERSION_CODES;
import android.os.Bundle;
Expand Down Expand Up @@ -152,6 +151,20 @@ public void onCreate(Bundle savedInstanceState) {
}
}
setUpPager();
/**
* Ask the user for media location access just after login
* so that location in the EXIF metadata of the images shared by the user
* is retained on devices running Android 10 or above
*/
if (VERSION.SDK_INT >= VERSION_CODES.Q) {
PermissionUtils.checkPermissionsAndPerformAction(
this,
permission.ACCESS_MEDIA_LOCATION,
() -> {},
R.string.media_location_permission_denied,
R.string.add_location_manually
);
}
}
}

Expand Down
45 changes: 39 additions & 6 deletions app/src/main/java/fr/free/nrw/commons/filepicker/FilePicker.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,11 @@ private static Uri createCameraPictureFile(@NonNull Context context) throws IOEx
return uri;
}

private static Intent createGalleryIntent(@NonNull Context context, int type) {
private static Intent createGalleryIntent(@NonNull Context context, int type,
boolean isGetContentPickerPreferred) {
// storing picked image type to shared preferences
storeType(context, type);
return plainGalleryPickerIntent()
return plainGalleryPickerIntent(isGetContentPickerPreferred)
.putExtra(Intent.EXTRA_ALLOW_MULTIPLE, configuration(context).allowsMultiplePickingInGallery());
}

Expand Down Expand Up @@ -105,8 +106,8 @@ private static int restoreType(@NonNull Context context) {
*
* @param type Custom type of your choice, which will be returned with the images
*/
public static void openGallery(Activity activity, int type) {
Intent intent = createGalleryIntent(activity, type);
public static void openGallery(Activity activity, int type, boolean isGetContentPickerPreferred) {
Intent intent = createGalleryIntent(activity, type, isGetContentPickerPreferred);
activity.startActivityForResult(intent, RequestCodes.PICK_PICTURE_FROM_GALLERY);
}

Expand Down Expand Up @@ -200,8 +201,40 @@ private static boolean isPhoto(Intent data) {
return data == null || (data.getData() == null && data.getClipData() == null);
}

private static Intent plainGalleryPickerIntent() {
Intent intent = new Intent(Intent.ACTION_GET_CONTENT);
private static Intent plainGalleryPickerIntent(boolean isGetContentPickerPreferred) {
/**
* Asking for ACCESS_MEDIA_LOCATION at runtime solved the location-loss issue
* in the custom selector in Contributions fragment.
* Detailed discussion: https://github.com/commons-app/apps-android-commons/issues/5015
*
* This permission check, however, was insufficient to fix location-loss in
* the regular selector in Contributions fragment and Nearby fragment,
* especially on some devices running Android 13 that use the new Photo Picker by default.
*
* New Photo Picker: https://developer.android.com/training/data-storage/shared/photopicker
*
* The new Photo Picker introduced by Android redacts location tags from EXIF metadata.
* 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
Copy link
Member

@sivaraam sivaraam Jun 15, 2023

Choose a reason for hiding this comment

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

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.

* (based on user's preference) as:
*
* ACTION_GET_CONTENT opens the 'best application' for choosing that kind of data
* The best application is the new Photo Picker that redacts the location tags
*
* ACTION_OPEN_DOCUMENT, however, displays the various DocumentsProvider instances
* installed on the device, letting the user interactively navigate through them.
*
* So, this allows us to use the traditional file picker that does not redact location tags from EXIF.
*
*/
Intent intent;
if (isGetContentPickerPreferred) {
intent = new Intent(Intent.ACTION_GET_CONTENT);
} else {
intent = new Intent(Intent.ACTION_OPEN_DOCUMENT);
}
intent.setType("image/*");
return intent;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import fr.free.nrw.commons.recentlanguages.RecentLanguagesAdapter;
import fr.free.nrw.commons.recentlanguages.RecentLanguagesDao;
import fr.free.nrw.commons.upload.LanguagesAdapter;
import fr.free.nrw.commons.utils.DialogUtil;
import fr.free.nrw.commons.utils.PermissionUtils;
import fr.free.nrw.commons.utils.ViewUtil;
import java.util.HashMap;
Expand Down Expand Up @@ -71,6 +72,7 @@ public class SettingsFragment extends PreferenceFragmentCompat {
private TextView recentLanguagesTextView;
private View separator;
private ListView languageHistoryListView;
private static final String GET_CONTENT_PICKER_HELP_URL = "https://commons-app.github.io/docs.html#get-content";

@Override
public void onCreatePreferences(Bundle savedInstanceState, String rootKey) {
Expand Down Expand Up @@ -150,6 +152,17 @@ public boolean onPreferenceClick(Preference preference) {
checkPermissionsAndSendLogs();
return true;
});

Preference getContentPickerPreference = findPreference("getContentPhotoPickerPref");
getContentPickerPreference.setOnPreferenceChangeListener(
(preference, newValue) -> {
boolean isGetContentPickerTurnedOn = (boolean) newValue;
if (isGetContentPickerTurnedOn) {
showLocationLossWarning();
}
return true;
}
);
// Disable some settings when not logged in.
if (defaultKvStore.getBoolean("login_skipped", false)) {
findPreference("useExternalStorage").setEnabled(false);
Expand All @@ -162,6 +175,26 @@ public boolean onPreferenceClick(Preference preference) {
}
}

/**
* On some devices, the new Photo Picker with GET_CONTENT takeover
* redacts location tags from EXIF metadata
*
* Show warning to the user when ACTION_GET_CONTENT intent is enabled
*/
private void showLocationLossWarning() {
DialogUtil.showAlertDialog(
getActivity(),
null,
getString(R.string.location_loss_warning),
getString(R.string.ok),
getString(R.string.read_help_link),
() -> {},
() -> Utils.handleWebUrl(requireContext(), Uri.parse(GET_CONTENT_PICKER_HELP_URL)),
null,
true
);
}

@Override
protected Adapter onCreateAdapter(final PreferenceScreen preferenceScreen) {
return new PreferenceGroupAdapter(preferenceScreen) {
Expand Down
3 changes: 3 additions & 0 deletions app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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.

<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>
Copy link
Member

Choose a reason for hiding this comment

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

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.

<string name="location_loss_warning">Please make sure that this new Android picker does not strip location from your pictures.</string>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


<string name="nearby_campaign_dismiss_message">You won\'t see the campaigns anymore. However, you can re-enable this notification in Settings if you wish.</string>
<string name="this_function_needs_network_connection">This function requires network connection, please check your connection settings.</string>
Expand Down
6 changes: 6 additions & 0 deletions app/src/main/res/xml/preferences.xml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@
app:singleLineTitle="false"
android:summary="@string/display_campaigns_explanation"
android:title="@string/display_campaigns" />

<SwitchPreference
android:defaultValue="false"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

More details in this issue: #5242

android:key="getContentPhotoPickerPref"
android:summary="@string/get_content_photo_picker_explanation"
android:title="@string/get_content_photo_picker_title"/>
</PreferenceCategory>

<PreferenceCategory
Expand Down