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 in-app camera location loss #5249

Merged
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
02acabe
fix in-app camera location loss
RitikaPahwa4444 Jun 24, 2023
d6ac060
fix failing unit tests
RitikaPahwa4444 Jun 24, 2023
655ff4c
UploadMediaDetailFragmentUnitTest: modify testOnActivityResultAddLoca…
RitikaPahwa4444 Jun 24, 2023
a8e0c54
reintroduce removed variable
RitikaPahwa4444 Jun 24, 2023
7208018
enable prePopulateCategoriesAndDepictionsBy for current user location
RitikaPahwa4444 Jun 25, 2023
3c0339b
add relevant comment and fix failing test
RitikaPahwa4444 Jun 25, 2023
5f9ff36
modify dialog and disable location tag redaction from EXIF
RitikaPahwa4444 Jun 28, 2023
e72be00
modify in-app camera dialog flow and change location to inAppPictureL…
RitikaPahwa4444 Jul 4, 2023
74f8c13
change location to inAppPictureLocation
RitikaPahwa4444 Jul 4, 2023
fa8674c
fix location flow
RitikaPahwa4444 Jul 26, 2023
def8079
Merge branch 'master' into in_app_camera_location_loss
RitikaPahwa4444 Jul 26, 2023
5f873d1
preferences.xml: remove redundant default value
RitikaPahwa4444 Jul 26, 2023
f0dd7d2
inform users about location loss happening for first upload
RitikaPahwa4444 Jul 27, 2023
5837b8b
Merge branch 'commons-app:master' into in_app_camera_location_loss
RitikaPahwa4444 Jul 28, 2023
a8783f6
FileProcessor.kt: remove commented-out code
RitikaPahwa4444 Jul 28, 2023
96c7b4d
prevent user location from getting attached to images with no EXIF lo…
RitikaPahwa4444 Jul 30, 2023
4417140
handle onPermissionDenied for location permission
RitikaPahwa4444 Aug 2, 2023
28618ce
remove last location when the user turns the GPS off
RitikaPahwa4444 Aug 16, 2023
efbd199
disable photo picker and in app camera preferences in settings for lo…
RitikaPahwa4444 Aug 16, 2023
0a2a10b
remove debug statements and add toast inside runnables
RitikaPahwa4444 Aug 25, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,13 @@
import fr.free.nrw.commons.filepicker.FilePicker.ImageSource;
import fr.free.nrw.commons.filepicker.UploadableFile;
import fr.free.nrw.commons.kvstore.JsonKvStore;
import fr.free.nrw.commons.location.LatLng;
import fr.free.nrw.commons.location.LocationPermissionsHelper;
import fr.free.nrw.commons.location.LocationPermissionsHelper.Dialog;
import fr.free.nrw.commons.location.LocationServiceManager;
import fr.free.nrw.commons.nearby.Place;
import fr.free.nrw.commons.upload.UploadActivity;
import fr.free.nrw.commons.utils.DialogUtil;
import fr.free.nrw.commons.utils.PermissionUtils;
import fr.free.nrw.commons.utils.ViewUtil;
import java.util.ArrayList;
Expand All @@ -28,7 +33,11 @@ public class ContributionController {

public static final String ACTION_INTERNAL_UPLOADS = "internalImageUploads";
private final JsonKvStore defaultKvStore;
private LatLng locationBeforeImageCapture;
private Boolean isInAppCameraUpload;

@Inject
LocationServiceManager locationManager;
@Inject
public ContributionController(@Named("default_preferences") JsonKvStore defaultKvStore) {
this.defaultKvStore = defaultKvStore;
Expand All @@ -46,15 +55,90 @@ public void initiateCameraPick(Activity activity) {

PermissionUtils.checkPermissionsAndPerformAction(activity,
Manifest.permission.WRITE_EXTERNAL_STORAGE,
() -> initiateCameraUpload(activity),
() -> {
if (defaultKvStore.getBoolean("inAppCameraFirstRun")) {
defaultKvStore.putBoolean("inAppCameraFirstRun", false);
askUserToAllowLocationAccess(activity);
} else if(!(PermissionUtils.hasPermission(activity,
Manifest.permission.ACCESS_FINE_LOCATION)
&& isLocationAccessToAppsTurnedOn())
&& defaultKvStore.getBoolean("inAppCameraLocationPref")) {
createDialogsAndHandleLocationPermissions(activity);
} else {
initiateCameraUpload(activity);
}
},
R.string.storage_permission_title,
R.string.write_storage_permission_rationale);
}

/**
* Asks users to provide location access
*
* @param activity
*/
private void createDialogsAndHandleLocationPermissions(Activity activity) {
LocationPermissionsHelper.Dialog locationAccessDialog = new Dialog(
R.string.location_permission_title,
R.string.in_app_camera_location_permission_rationale
);

LocationPermissionsHelper.Dialog locationOffDialog = new Dialog(
R.string.ask_to_turn_location_on,
R.string.in_app_camera_needs_location
);

LocationPermissionsHelper locationPermissionsHelper = new LocationPermissionsHelper(activity, locationManager);
locationPermissionsHelper.handleLocationPermissions(
locationAccessDialog,
locationOffDialog
);
}

/**
* Suggest user to attach location information with pictures.
* If the user selects "Yes", then:
*
* Location is taken from the EXIF if the default camera application
* does not redact location tags.
*
* Otherwise, if the EXIF metadata does not have location information,
* then location captured by the app is used
*
* @param activity
*/
private void askUserToAllowLocationAccess(Activity activity) {
DialogUtil.showAlertDialog(activity,
activity.getString(R.string.in_app_camera_location_permission_title),
activity.getString(R.string.in_app_camera_location_access_explanation),
activity.getString(R.string.option_allow),
activity.getString(R.string.option_dismiss),
()-> {
defaultKvStore.putBoolean("inAppCameraLocationPref", true);
createDialogsAndHandleLocationPermissions(activity);
},
() -> {
defaultKvStore.putBoolean("inAppCameraLocationPref", false);
initiateCameraUpload(activity);
},
null,
true);
}

/**
* Check if apps have access to location even after having individual access
*
* @return
*/
private boolean isLocationAccessToAppsTurnedOn() {
return (locationManager.isNetworkProviderEnabled() || locationManager.isGPSProviderEnabled());
}

/**
* Initiate gallery picker
*/
public void initiateGalleryPick(final Activity activity, final boolean allowMultipleUploads) {
isInAppCameraUpload = false;
initiateGalleryUpload(activity, allowMultipleUploads);
}

Expand All @@ -67,6 +151,7 @@ public void initiateCustomGalleryPickWithPermission(final Activity activity) {
PermissionUtils.checkPermissionsAndPerformAction(activity,
Manifest.permission.WRITE_EXTERNAL_STORAGE,
() -> {
isInAppCameraUpload = false;
FilePicker.openCustomSelector(activity, 0);
},
R.string.storage_permission_title,
Expand Down Expand Up @@ -99,6 +184,10 @@ private void setPickerConfiguration(Activity activity,
*/
private void initiateCameraUpload(Activity activity) {
setPickerConfiguration(activity, false);
if (defaultKvStore.getBoolean("inAppCameraLocationPref", false)) {
locationBeforeImageCapture = locationManager.getLastLocation();
Copy link
Member

Choose a reason for hiding this comment

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

We're presuming the device location would be turned on whenever the setting is turned on. This is incorrect since the user has the freedom to turn on location of their device even after the setting is turned on.

Ditto on the location permission.

It is prudent to verify that location permission is available and location is turned on whenever the setting is true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're presuming the device location would be turned on whenever the setting is turned on.

We've already prompted the user to turn it on, redirected to the Settings too.

Copy link
Member

Choose a reason for hiding this comment

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

The world is never ideal when it comes to Android. The user can have the setting enabled and then later on turn off location on their device. This is a normal case which we should be wary of. That's precisely why the Nearby feature asks user to turn on location when it is turned off.

The same applies for the location permission (especially on Android 11 and above where permissions are auto-removed for unused apps). So, it is the ideal case to ensure we have location permission and if its unavailable request for the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A user who wants pictures to be geo-tagged will be turning location on for the camera too but I see it was a bad assumption. Thank you for explaining it! :)

}
isInAppCameraUpload = true;
FilePicker.openCameraForImage(activity, 0);
}

Expand Down Expand Up @@ -134,7 +223,8 @@ public List<UploadableFile> handleExternalImagesPicked(Activity activity,

/**
* Returns intent to be passed to upload activity
* Attaches place object for nearby uploads
* Attaches place object for nearby uploads and
* location before image capture if in-app camera is used
*/
private Intent handleImagesPicked(Context context,
List<UploadableFile> imagesFiles) {
Expand All @@ -148,6 +238,17 @@ private Intent handleImagesPicked(Context context,
shareIntent.putExtra(PLACE_OBJECT, place);
}

if (locationBeforeImageCapture != null) {
shareIntent.putExtra(
UploadActivity.LOCATION_BEFORE_IMAGE_CAPTURE,
locationBeforeImageCapture);
}

shareIntent.putExtra(
UploadActivity.IN_APP_CAMERA_UPLOAD,
isInAppCameraUpload
);

return shareIntent;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ protected void onResume() {

if ((applicationKvStore.getBoolean("firstrun", true)) &&
(!applicationKvStore.getBoolean("login_skipped"))) {
defaultKvStore.putBoolean("inAppCameraFirstRun", true);
WelcomeActivity.startYourself(this);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package fr.free.nrw.commons.location;

import android.Manifest.permission;
import android.app.Activity;
import android.content.Intent;
import android.content.pm.PackageManager;
import android.provider.Settings;
import fr.free.nrw.commons.R;
import fr.free.nrw.commons.utils.DialogUtil;
import fr.free.nrw.commons.utils.PermissionUtils;

/**
* Helper class to handle location permissions
*/
public class LocationPermissionsHelper {
Activity activity;
LocationServiceManager locationManager;
public LocationPermissionsHelper(Activity activity, LocationServiceManager locationManager) {
this.activity = activity;
this.locationManager = locationManager;
}
public static class Dialog {
int dialogTitleResource;
int dialogTextResource;

public Dialog(int dialogTitle, int dialogText) {
dialogTitleResource = dialogTitle;
dialogTextResource = dialogText;
}
}

public void handleLocationPermissions(Dialog locationAccessDialog,
Dialog locationOffDialog) {
requestForLocationAccess(locationAccessDialog, locationOffDialog);
}

/**
* Ask for location permission if the user agrees on attaching location with pictures
* and the app does not have the access to location
*
* @param locationAccessDialog
* @param locationOffDialog
*/
private void requestForLocationAccess(
Dialog locationAccessDialog,
Dialog locationOffDialog
) {
PermissionUtils.checkPermissionsAndPerformAction(activity,
permission.ACCESS_FINE_LOCATION,
() -> {
if(!isLocationAccessToAppsTurnedOn()) {
showLocationOffDialog(locationOffDialog);
}
},
() -> {},
locationAccessDialog.dialogTitleResource,
locationAccessDialog.dialogTextResource);
}

/**
* Check if apps have access to location even after having individual access
*
* @return
*/
private boolean isLocationAccessToAppsTurnedOn() {
return (locationManager.isNetworkProviderEnabled() || locationManager.isGPSProviderEnabled());
}

/**
* Ask user to grant location access to apps
*
*/

private void showLocationOffDialog(Dialog locationOffDialog) {
DialogUtil
.showAlertDialog(activity,
activity.getString(locationOffDialog.dialogTitleResource),
activity.getString(locationOffDialog.dialogTextResource),
activity.getString(R.string.title_app_shortcut_setting),
() -> openLocationSettings(),
true);
}

/**
* Open location source settings so that apps with location access can access it
*
* TODO: modify it to fix https://github.com/commons-app/apps-android-commons/issues/5255
*/

private void openLocationSettings() {
final Intent intent = new Intent(Settings.ACTION_LOCATION_SOURCE_SETTINGS);
final PackageManager packageManager = activity.getPackageManager();

if (intent.resolveActivity(packageManager)!= null) {
activity.startActivity(intent);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,9 @@ public int getCount() {
* @return
*/
public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Place place,
SimilarImageInterface similarImageInterface) {
SimilarImageInterface similarImageInterface, LatLng inAppPictureLocation) {
return uploadModel.preProcessImage(uploadableFile, place,
similarImageInterface);
similarImageInterface, inAppPictureLocation);
}

/**
Expand All @@ -199,8 +199,8 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Pla
* @param uploadItem
* @return
*/
public Single<Integer> getImageQuality(UploadItem uploadItem) {
return uploadModel.getImageQuality(uploadItem);
public Single<Integer> getImageQuality(UploadItem uploadItem, LatLng location) {
return uploadModel.getImageQuality(uploadItem, location);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,14 @@
import com.karumi.dexter.Dexter;
import com.karumi.dexter.listener.PermissionGrantedResponse;
import com.karumi.dexter.listener.single.BasePermissionListener;
import com.mapbox.mapboxsdk.Mapbox;
import fr.free.nrw.commons.R;
import fr.free.nrw.commons.Utils;
import fr.free.nrw.commons.campaigns.CampaignView;
import fr.free.nrw.commons.contributions.MainActivity;
import fr.free.nrw.commons.di.ApplicationlessInjection;
import fr.free.nrw.commons.kvstore.JsonKvStore;
import fr.free.nrw.commons.location.LocationPermissionsHelper;
import fr.free.nrw.commons.location.LocationServiceManager;
import fr.free.nrw.commons.logging.CommonsLogSender;
import fr.free.nrw.commons.recentlanguages.Language;
import fr.free.nrw.commons.recentlanguages.RecentLanguagesAdapter;
Expand Down Expand Up @@ -65,6 +66,9 @@ public class SettingsFragment extends PreferenceFragmentCompat {
@Inject
RecentLanguagesDao recentLanguagesDao;

@Inject
LocationServiceManager locationManager;

private ListPreference themeListPreference;
private Preference descriptionLanguageListPreference;
private Preference appUiLanguageListPreference;
Expand Down Expand Up @@ -97,6 +101,18 @@ public void onCreatePreferences(Bundle savedInstanceState, String rootKey) {
});
}

Preference inAppCameraLocationPref = findPreference("inAppCameraLocationPref");

inAppCameraLocationPref.setOnPreferenceChangeListener(
(preference, newValue) -> {
boolean isInAppCameraLocationTurnedOn = (boolean) newValue;
if (isInAppCameraLocationTurnedOn) {
createDialogsAndHandleLocationPermissions(getActivity());
}
return true;
}
);

// Gets current language code from shared preferences
String languageCode;

Expand Down Expand Up @@ -175,6 +191,29 @@ public boolean onPreferenceClick(Preference preference) {
}
}

/**
* Asks users to provide location access
*
* @param activity
*/
private void createDialogsAndHandleLocationPermissions(Activity activity) {
LocationPermissionsHelper.Dialog locationAccessDialog = new LocationPermissionsHelper.Dialog(
R.string.location_permission_title,
R.string.in_app_camera_location_permission_rationale
);

LocationPermissionsHelper.Dialog locationOffDialog = new LocationPermissionsHelper.Dialog(
R.string.ask_to_turn_location_on,
R.string.in_app_camera_needs_location
);

LocationPermissionsHelper locationPermissionsHelper = new LocationPermissionsHelper(activity, locationManager);
locationPermissionsHelper.handleLocationPermissions(
locationAccessDialog,
locationOffDialog
);
}

/**
* On some devices, the new Photo Picker with GET_CONTENT takeover
* redacts location tags from EXIF metadata
Expand Down
Loading