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 7 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 @@ -3,18 +3,24 @@
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.content.pm.PackageManager;
import android.provider.Settings;
import androidx.annotation.NonNull;
import fr.free.nrw.commons.R;
import fr.free.nrw.commons.filepicker.DefaultCallback;
import fr.free.nrw.commons.filepicker.FilePicker;
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.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 +34,10 @@ public class ContributionController {

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

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

PermissionUtils.checkPermissionsAndPerformAction(activity,
Manifest.permission.WRITE_EXTERNAL_STORAGE,
() -> initiateCameraUpload(activity),
() -> {
if (!(PermissionUtils.hasPermission(activity, Manifest.permission.ACCESS_FINE_LOCATION)
&& isLocationAccessToAppsTurnedOn())) {
Copy link
Member

Choose a reason for hiding this comment

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

Having a check like this to trigger the consent dialog could be a bit incorrect. We attach the location to the upload when the EXIF location is unavailable. This could also occur in cases where the app has location access and location is turned on the device but saving location is turned off in the device camera. The user could've done this intentionally. The app obtained location would be uploaded in this case without the user having any idea about it. I believe it is ideal to get the consent from the user before doing so.

There's also the case of lower Android version devices (such as API 21) for which there is no notion of location permission. So, this dialog is shown to them only when location is turned off.

I also observed this dialog appears each time when trying to use the in-app camera and location is either off / location permissions is unavailable. This might become a bit annoying if the user has intentionally not granted those.

To better handle this, I would suggested showcasing the consent dialog first time (unconditinally) when user users in-app camera and store their preference. Given this would be a one-time dialog, we should also have a corresponding preference in settings in case the user wants to toggle their choice.

When we have user's consent to attach the location, we could then show dialog about lack of location permission / location enabled when in-app camera is triggered.

This seems like a better and reasonable flow to me. Thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sivaraam Just a clarification -

Given this would be a one-time dialog, we should also have a corresponding preference in settings in case the user wants to toggle their choice.

Is this referring to displaying the consent dialog?
If yes, I'm a little skeptical about having a setting to toggle the choice to display the dialog. In most apps, we usually have an option like "Do not show this message again" and once the user selects the option, we do not see it again. A user who's curious about enabling location could probably go through the information we have already associated while enabling/disabling permissions for location and would not want a pop up enabled again.

I may be wrong but just wanted to understand the perspective behind it.

Copy link
Collaborator Author

@RitikaPahwa4444 RitikaPahwa4444 Jun 30, 2023

Choose a reason for hiding this comment

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

Having a check like this to trigger the consent dialog could be a bit incorrect.

Appeared incorrect to me as well, I felt I was missing out some case but did not know which one. Thank you for elaborating about it, I'll make this dialog compulsory on the first run.

Also, I'd suggested this "Do not show this message again" in my comment above. If this sounds okay, I'll add it for the Dismiss case (it does not appear again if the user allows).

askUserToAllowLocationAccess(activity);
} else {
initiateCameraUpload(activity);
}
},
R.string.storage_permission_title,
R.string.write_storage_permission_rationale);
}

/**
* 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.location_permission_title),
Copy link
Member

Choose a reason for hiding this comment

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

The title for this dialog could be something more meaningful than "Requesting location permission"

May be try "Record location for in-app shots" ?

activity.getString(R.string.in_app_camera_location_access_explanation),
activity.getString(R.string.option_allow),
activity.getString(R.string.option_dismiss),
()-> requestForLocationAccess(activity),
() -> initiateCameraUpload(activity),
null,
true);
}

/**
* Ask for location permission if the user agrees on attaching location with pictures
* and the app does not have the access to location
*
* @param activity
*/

private void requestForLocationAccess(Activity activity) {
PermissionUtils.checkPermissionsAndPerformAction(activity,
permission.ACCESS_FINE_LOCATION,
() -> onLocationPermissionGranted(activity),
() -> {},
R.string.ask_to_turn_location_on,
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, this should be R.string.location_permission_title since this is title for the location permission dialog.

R.string.in_app_camera_location_permission_rationale);
}

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

/**
* Initiate in-app camera if apps have access to location.
* Otherwise, show location-off dialog.
*
* @param activity
*/
private void onLocationPermissionGranted(Activity activity) {
if (!isLocationAccessToAppsTurnedOn()) {
showLocationOffDialog(activity);
} else {
initiateCameraUpload(activity);
}
}

/**
* Ask user to grant location access to apps
*
* @param activity
*/

private void showLocationOffDialog(Activity activity) {
DialogUtil
.showAlertDialog(activity,
activity.getString(R.string.location_permission_title),
Copy link
Member

Choose a reason for hiding this comment

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

This should be R.string.ask_to_turn_location_on since this is a dialog for requesting to turn on location.

activity.getString(R.string.in_app_camera_needs_location),
activity.getString(R.string.title_app_shortcut_setting),
() -> openLocationSettings(activity),
true);
}

/**
* Open location source settings so that apps with location access can access it
*
* @param activity
*/

private void openLocationSettings(Activity activity) {
final Intent intent = new Intent(Settings.ACTION_LOCATION_SOURCE_SETTINGS);
Copy link
Member

Choose a reason for hiding this comment

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

Strangely, this is opening the app's setting screen instead of the location settings screen on my device (Nord running Android 12). Any thoughts on possible reasons for this ? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I observed a similar behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is used at many places in the app already and works pretty well on my device, so I assumed it to be correct. Will check this out. Thank you for mentioning this!

final PackageManager packageManager = activity.getPackageManager();

if (intent.resolveActivity(packageManager)!= null) {
activity.startActivity(intent);
}
}

/**
* Initiate gallery picker
*/
Expand Down Expand Up @@ -99,6 +209,7 @@ private void setPickerConfiguration(Activity activity,
*/
private void initiateCameraUpload(Activity activity) {
setPickerConfiguration(activity, false);
locationBeforeImageCapture = locationManager.getLastLocation();
FilePicker.openCameraForImage(activity, 0);
}

Expand Down Expand Up @@ -134,7 +245,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 +260,12 @@ private Intent handleImagesPicked(Context context,
shareIntent.putExtra(PLACE_OBJECT, place);
}

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

return shareIntent;
}

Expand Down
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 location) {
return uploadModel.preProcessImage(uploadableFile, place,
similarImageInterface);
similarImageInterface, location);
}

/**
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
19 changes: 15 additions & 4 deletions app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.kt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import android.net.Uri
import androidx.exifinterface.media.ExifInterface
import fr.free.nrw.commons.R
import fr.free.nrw.commons.kvstore.JsonKvStore
import fr.free.nrw.commons.location.LatLng
import fr.free.nrw.commons.mwapi.CategoryApi
import fr.free.nrw.commons.mwapi.OkHttpJsonApiClient
import fr.free.nrw.commons.settings.Prefs
Expand Down Expand Up @@ -48,7 +49,8 @@ class FileProcessor @Inject constructor(
/**
* Processes filePath coordinates, either from EXIF data or user location
*/
fun processFileCoordinates(similarImageInterface: SimilarImageInterface, filePath: String?)
fun processFileCoordinates(similarImageInterface: SimilarImageInterface,
filePath: String?, location: LatLng?)
: ImageCoordinates {
val exifInterface: ExifInterface? = try {
ExifInterface(filePath!!)
Expand All @@ -59,7 +61,7 @@ class FileProcessor @Inject constructor(
// Redact EXIF data as indicated in preferences.
redactExifTags(exifInterface, getExifTagsToRedact())
Timber.d("Calling GPSExtractor")
val originalImageCoordinates = ImageCoordinates(exifInterface)
val originalImageCoordinates = ImageCoordinates(exifInterface, location)
if (originalImageCoordinates.decimalCoords == null) {
//Find other photos taken around the same time which has gps coordinates
findOtherImages(
Expand All @@ -82,6 +84,13 @@ class FileProcessor @Inject constructor(
defaultKvStore.getStringSet(Prefs.MANAGED_EXIF_TAGS) ?: emptySet()
val redactTags: Set<String> =
context.resources.getStringArray(R.array.pref_exifTag_values).toSet()
// This will be useful when implementing https://github.com/commons-app/apps-android-commons/issues/5247
/* Remove EXIF location if user has chosen
nicolas-raoul marked this conversation as resolved.
Show resolved Hide resolved
"No, do not attach location" while using in-app camera */
/*if (!defaultKvStore.getBoolean("locationInfoPref")) {
val locationTag: String = context.getString(R.string.exif_tag_location)
return redactTags - prefManageEXIFTags.minus(locationTag)
}*/
return redactTags - prefManageEXIFTags
}

Expand Down Expand Up @@ -156,11 +165,13 @@ class FileProcessor @Inject constructor(

private fun readImageCoordinates(file: File) =
try {
ImageCoordinates(contentResolver.openInputStream(Uri.fromFile(file))!!)
/* Used null location as location for similar images captured before is not available
in case it is not present in the EXIF. */
ImageCoordinates(contentResolver.openInputStream(Uri.fromFile(file))!!, null)
} catch (e: IOException) {
Timber.e(e)
try {
ImageCoordinates(file.absolutePath)
ImageCoordinates(file.absolutePath, null)
} catch (ex: IOException) {
Timber.e(ex)
null
Expand Down
5 changes: 3 additions & 2 deletions app/src/main/java/fr/free/nrw/commons/upload/FileUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import androidx.exifinterface.media.ExifInterface;

import fr.free.nrw.commons.location.LatLng;
import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
Expand Down Expand Up @@ -64,11 +65,11 @@ static String getSHA1(InputStream is) {
/**
* Get Geolocation of filePath from input filePath path
*/
static String getGeolocationOfFile(String filePath) {
static String getGeolocationOfFile(String filePath, LatLng location) {

try {
ExifInterface exifInterface = new ExifInterface(filePath);
ImageCoordinates imageObj = new ImageCoordinates(exifInterface);
ImageCoordinates imageObj = new ImageCoordinates(exifInterface, location);
if (imageObj.getDecimalCoords() != null) { // If image has geolocation information in its EXIF
return imageObj.getDecimalCoords();
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package fr.free.nrw.commons.upload;

import android.content.Context;
import fr.free.nrw.commons.location.LatLng;
import io.reactivex.Observable;
import java.io.BufferedInputStream;
import java.io.File;
Expand Down Expand Up @@ -37,8 +38,8 @@ public FileInputStream getFileInputStream(String filePath) throws FileNotFoundEx
return FileUtils.getFileInputStream(filePath);
}

public String getGeolocationOfFile(String filePath) {
return FileUtils.getGeolocationOfFile(filePath);
public String getGeolocationOfFile(String filePath, LatLng location) {
return FileUtils.getGeolocationOfFile(filePath, location);
}


Expand Down
16 changes: 13 additions & 3 deletions app/src/main/java/fr/free/nrw/commons/upload/ImageCoordinates.kt
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ import java.io.InputStream
/**
* Extracts geolocation to be passed to API for category suggestions. If a picture with geolocation
* is uploaded, extract latitude and longitude from EXIF data of image.
* Otherwise, if current user location is available while using the in-app camera,
* use it to set image coordinates
*/
class ImageCoordinates internal constructor(exif: ExifInterface?) {
class ImageCoordinates internal constructor(exif: ExifInterface?, location: LatLng?) {
var decLatitude = 0.0
var decLongitude = 0.0
var imageCoordsExists = false
Expand All @@ -26,13 +28,13 @@ class ImageCoordinates internal constructor(exif: ExifInterface?) {
/**
* Construct from a stream.
*/
internal constructor(stream: InputStream) : this(ExifInterface(stream))
internal constructor(stream: InputStream, location: LatLng?) : this(ExifInterface(stream), location)
/**
* Construct from the file path of the image.
* @param path file path of the image
*/
@Throws(IOException::class)
internal constructor(path: String) : this(ExifInterface(path))
internal constructor(path: String, location: LatLng?) : this(ExifInterface(path), location)

init {
//If image has no EXIF data and user has enabled GPS setting, get user's location
Expand Down Expand Up @@ -61,6 +63,14 @@ class ImageCoordinates internal constructor(exif: ExifInterface?) {
//If image has EXIF data, extract image coords
imageCoordsExists = true
Timber.d("EXIF data has location info")
} else if (location != null) {
decLatitude = location.latitude
decLongitude = location.longitude
if (!(decLatitude == 0.0 && decLongitude == 0.0)) {
decimalCoords = "$decLatitude|$decLongitude"
imageCoordsExists = true
Timber.d("Image coordinates recorded while using in-app camera")
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_OK;

import android.content.Context;
import fr.free.nrw.commons.location.LatLng;
import fr.free.nrw.commons.media.MediaClient;
import fr.free.nrw.commons.nearby.Place;
import fr.free.nrw.commons.utils.ImageUtils;
Expand Down Expand Up @@ -46,7 +47,7 @@ public ImageProcessingService(FileUtilsWrapper fileUtilsWrapper,
* Check image quality before upload - checks duplicate image - checks dark image - checks
* geolocation for image - check for valid title
*/
Single<Integer> validateImage(UploadItem uploadItem) {
Single<Integer> validateImage(UploadItem uploadItem, LatLng location) {
int currentImageQuality = uploadItem.getImageQuality();
Timber.d("Current image quality is %d", currentImageQuality);
if (currentImageQuality == ImageUtils.IMAGE_KEEP) {
Expand All @@ -57,7 +58,7 @@ Single<Integer> validateImage(UploadItem uploadItem) {

return Single.zip(
checkDuplicateImage(filePath),
checkImageGeoLocation(uploadItem.getPlace(), filePath),
checkImageGeoLocation(uploadItem.getPlace(), filePath, location),
checkDarkImage(filePath),
validateItemTitle(uploadItem),
checkFBMD(filePath),
Expand Down Expand Up @@ -148,13 +149,13 @@ private Single<Integer> checkDarkImage(String filePath) {
* @param filePath file to be checked
* @return IMAGE_GEOLOCATION_DIFFERENT or IMAGE_OK
*/
private Single<Integer> checkImageGeoLocation(Place place, String filePath) {
private Single<Integer> checkImageGeoLocation(Place place, String filePath, LatLng location) {
Timber.d("Checking for image geolocation %s", filePath);
if (place == null || StringUtils.isBlank(place.getWikiDataEntityId())) {
return Single.just(ImageUtils.IMAGE_OK);
}
return Single.fromCallable(() -> filePath)
.map(fileUtilsWrapper::getGeolocationOfFile)
.flatMap(path -> Single.just(fileUtilsWrapper.getGeolocationOfFile(path, location)))
.flatMap(geoLocation -> {
if (StringUtils.isBlank(geoLocation)) {
return Single.just(ImageUtils.IMAGE_OK);
Expand Down
Loading