Skip to content

Update "automatically get current location" setting text to inform that files will be geotagged with it #1599

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

Closed
misaochan opened this issue Jun 7, 2018 · 27 comments

Comments

@misaochan
Copy link
Member

Based on the discussion at https://commons.wikimedia.org/wiki/Commons:Village_pump#Warning!_Mobile_uploads_are_getting_the_wrong_location! , we should make this more explicit to users.

@VojtechDostal
Copy link
Collaborator

It's a surprise to me that the coordinates can be assigned based on uploader's current position rather than position at the time of taking the picture. Isn't that mostly useless? :-) That position is usually completely different.

@misaochan
Copy link
Member Author

misaochan commented Jun 8, 2018

I think the rationale was that people sometimes upload photos on the spot (e.g. via camera), forget to enable geotagging on their camera, and if they choose to enable this setting, they will still get the other benefits of geotagging (e.g. category suggestions) based on their current position. @nicolas-raoul can perhaps elaborate on this? Indeed, this option might have less relevance now than when it was implemented (for my own Outreachy project), so we may want to reconsider having it at all?

The user has escalated this to the Commons administrative noticeboard - https://commons.wikimedia.org/wiki/Commons:Administrators%27_noticeboard#Ongoing_privacy_incident_involving_the_Commons_Mobile_App despite my response to him. I'm honestly not sure if a response of the scale that he is suggesting is warranted, since the number of users affected would be relatively small - they would need to have manually enabled the setting, and then manually enabled location permissions with each new install, and uploaded files with no exif location. Granted we should have made the flavour text more explicit, but personally if I were to manually enable all of that in an image uploading app, I would not be very surprised that images would be geotagged. There are far fewer warnings/ manual enabling required to have an image geotagged with its exif data, after all, including on the upload wizard. The solution he is proposing might remove geotags from the uploads of users who DID intend to enable the setting for this purpose.

I'm not sure if my opinions are off base though. Would anyone mind looking through the conversation and providing input?

@whym
Copy link
Collaborator

whym commented Jun 8, 2018

(I might comment at the VP later, but I think technical questions should be sorted out here first.)

Can we quantify 'relatively small'? There are 28,590 files contributed via the app with the location template on their file pages. I assume among them, the scope would be files without latitude/longitude embedded into EXIF?

Manually I struggled to find such a file among the 28,590 files (which probably means the number is indeed small) - I would have to write a script.

@misaochan
Copy link
Member Author

misaochan commented Jun 8, 2018

@whym Yeah, unfortunately there is no way to tell what the exact number is unless we write a script (and even then it doesn't distinguish between files where the uploader intentionally used our feature for this purpose, as opposed to those who don't actually desire it despite having enabled that setting). I'm making a guess based on what I see manually, as well as the fact that 3 factors (that I mentioned above) need to all be true to allow for it to happen, and then among files with all 3 factors being true, the geotagging needs to be unintentional.

Can we quantify 'relatively small'? There are 28,590 files contributed via the app with the location template on their file pages. I assume among them, the scope would be files without latitude/longitude embedded into EXIF?

Right, except for the unintentional vs intentional bit. I'm not sure if it's possible to know which ones are intentional and which ones unintentional, without reaching out to them.

@whym
Copy link
Collaborator

whym commented Jun 8, 2018

I'm working on a script to do that. From preliminary results, it appears like roughly 10% of the 28,590 images fall into the definition (except for the the unintentional vs intentional bit, as you pointed out). I think I can get the full number in a day or two.

Another technical point: is the settings screen the only entry point to enable the feature? Do/did we ask it in Share or Categorization somehow?

EDIT: the initial results were wrong - the applicable files are actually far less than that. I'll give an update later.

@nicolas-raoul
Copy link
Member

nicolas-raoul commented Jun 8, 2018

This option is off by default, right?
I just wiped the app's data using Android's Settings, and the setting is now off.
If it is the case, I would say we don't have to do more than make sure the option clearly states what it is for.

@nicolas-raoul
Copy link
Member

I might be confusing things. My comment above was about "Automatically get current location - Retrieve current location to offer category suggestions if image is not geotagged", I am not sure whether it is related or not.

  • Are coordinates at time of upload really added to the image as its coordinates?
  • If yes, is it enabled by default after installing the app?

@whym
Copy link
Collaborator

whym commented Jun 8, 2018

As far as I can see:

  • Coordinates were added using the {{Location}} template into the file page, not to the embedded EXIF.
  • It is disabled by default. (and if the camera adds coordinates to photos, it has no effect.

Relevant code:

if (exif == null || exif.getAttribute(ExifInterface.TAG_GPS_LATITUDE) == null) {
if (useGPS) {
registerLocationManager();
imageCoordsExists = false;
Timber.d("EXIF data has no location info");
//Check what user's preference is for automatic location detection
boolean gpsPrefEnabled = gpsPreferenceEnabled();
//Check that currentLatitude and currentLongitude have been
// explicitly set by MyLocationListener
// and do not default to (0.0,0.0)
if (gpsPrefEnabled && currentLatitude != null && currentLongitude != null) {
Timber.d("Current location values: Lat = %f Long = %f",
currentLatitude, currentLongitude);
return String.valueOf(currentLatitude) + "|" + String.valueOf(currentLongitude);
} else {
// No coords found
return null;
}
} else {
return null;
}

@misaochan
Copy link
Member Author

@whym Thanks for looking into this!

Another technical point: is the settings screen the only entry point to enable the feature? Do/did we ask it in Share or Categorization somehow?

Yes, AFAIK the settings screen is the only entry point to enable the feature.

As far as I can see: Coordinates were added using the {{Location}} template into the file page, not to the embedded EXIF. It is disabled by default. (and if the camera adds coordinates to photos, it has no effect.

Yup, that is my understanding too.

Are coordinates at time of upload really added to the image as its coordinates?
If yes, is it enabled by default after installing the app?

@nicolas-raoul Yes they are added if the user has manually enabled that setting. It is disabled by default.

We should definitely change the flavour text to make it more explicit, and I will do that ASAP next week. However, the user in the Commons: administrator noticeboard is proposing that a bot be run to remove all location tags from such scenarios. I personally feel that this would be detrimental to the people who actually enabled that setting hoping that it would do that. But I guess it wouldn't be too bad either way.

@nicolas-raoul
Copy link
Member

If the setting has always been "Automatically get current location - Retrieve current location to offer category suggestions if image is not geotagged", then I have to agree that adding these coordinates to the image itself is more than described. So removing these locations sounds to me like the best thing to do for now.

I am sure we can distinguish whether the coordinates template was added by the app or manually via the web interface, thanks to edit tags.

@whym
Copy link
Collaborator

whym commented Jun 10, 2018

According to my script's final results, there were 939 applicable files (less than 4% of the geotagged files) as of last Friday. I offered to redact location information from them at the Administrators noticeboard.

What message, if any, should we send to the affected users (probably to their talk pages)? The right timing would be after the release of the fixed app anyway, though.

@misaochan
Copy link
Member Author

misaochan commented Jun 10, 2018

Thanks so much @whym ! I guess we could tell them that we apologize for an unclear description of the "Automatically get current location" setting, as we just found out that we neglected to mention that enabling the setting would also add those coordinates to the location template of the uploaded image? And let them know that due to this, in order to protect users' privacy we have to redact location information from files that match the criteria you mentioned.

Re: the fixed app, I'm not sure whether we should just bundle the change into 2.8 (beta version coming out next week), or do we need to push 2.7.2 directly into production with just that one change? The first major release always takes a while in beta, so it would be 2-3 weeks away from production most likely. So 2.7.2 will be quicker, however that does mean that everyone will have to download an updated APK with just that one change.

@nicolas-raoul
Copy link
Member

I would say that pushing an APK to all users with no real changes is OK if it is a security correction.

@misaochan
Copy link
Member Author

Pushed to beta. Would greatly appreciate if anyone could help test (just to make sure the setting looks appropriate and there are no major issues otherwise), will push to production in 1-2 days' time.

@whym
Copy link
Collaborator

whym commented Jun 11, 2018

Is there any way to solicit users to review preferences? Without that, I don't think many will notice the change - (presumably many) those who were unaware of the upload-time geotagging would continue to be so more often than not.

EDIT: One thing we could do is to abandon the current preference key (allowGps) and replace it with a new one, effectively resetting it to 'disabled' forcibly. This way, no user will continue using the feature without looking at the new label. What do you think? @misaochan

I'll be drafting an incident report page on the repo's wiki. It would be nice if we can include a link to it when notifying users.

@nicolas-raoul
Copy link
Member

Ah, indeed. Switching the setting off might have been the safest thing to do.

@whym
Copy link
Collaborator

whym commented Jun 11, 2018

Or, would it be too much to remove the feature of upload-time geotagging at least temporarily? (This may be preferable, because an abandoned key will mean that we will have to make sure not to use it again.)

@nicolas-raoul
Copy link
Member

Testing: Setting is OK, and picture uploaded correctly :-)

@misaochan
Copy link
Member Author

misaochan commented Jun 11, 2018

I actually don't mind removing the feature of upload-time geotagging entirely, personally. As we've seen from the stats, not many people even enable the setting, and now that we are considering implementing features like a to-do checklist for users, leaving the coordinates blank might actually be preferable. That will allow us to remove the setting entirely and prevent any further issues.

However, IMO this should probably be done in 2.8 and I should go ahead with pushing 2.7.2 to production, since removing the setting without breaking anything will take a bit of time to implement, and our first priority should be the quick and easy fix (making the setting more explicit).

@misaochan
Copy link
Member Author

Thanks @nicolas-raoul ! Will push to production tomorrow. :)

@whym
Copy link
Collaborator

whym commented Jun 11, 2018

However, IMO this should probably be done in 2.8 and I should go ahead with pushing 2.7.2 to production, since removing the setting without breaking anything will take a bit of time to implement, and our first priority should be the quick and easy fix (making the setting more explicit).

Okay, it makes sense.

I'll be drafting an incident report page on the repo's wiki. It would be nice if we can include a link to it when notifying users.

https://github.com/commons-app/apps-android-commons/wiki/June-2018-incident

@misaochan
Copy link
Member Author

misaochan commented Jun 12, 2018

Is this (completely removing the retrieval of current location via settings) something that we have all agreed we want to do? If yes, I can add a note to the 2.8 list #1478

@whym
Copy link
Collaborator

whym commented Jun 12, 2018

Do you mean removing the use of the current location in category suggestion as well? I am in favor of stopping the publication of upload-time location. However, isn't it potentially useful to retrieve location-related categories even if it's technically the place of uploading not photographing, assuming the uploader has not moved too far away in between? I think it is easy enough to ignore suggestions when it's not the case.

@misaochan
Copy link
Member Author

misaochan commented Jun 12, 2018

However, isn't it potentially useful to retrieve location-related categories even if it's technically the place of uploading not photographing, assuming the uploader has not moved too far away in between?

It could be, but this would make the implementation (and testing) a bit more time-consuming - we would need to distinguish between two types of coordinates, where one is added to the location template and the other is not. Definitely still doable, but I am trying to pare down the requirements for 2.8 since our team is already doing a lot of additional work with bugfixes while the new features required for our grant are put on hold.

@whym
Copy link
Collaborator

whym commented Jun 12, 2018

I understand. Then it might be better to leave it as a future enhancement issue to 'revive' that part in some form after removing it.

@misaochan
Copy link
Member Author

I submitted #1644 , which removes current location retrieval entirely from the upload process, and removes the Setting as well.

Where possible, I have left methods intact so that in the future if we decide to reimplement this (retrieving current location solely for category suggestions but not adding it to the template), we will not need to rewrite code. However, I have removed most of the method calls, and methods that would affect the user (e.g. the snackbar requesting location permissions).

@nicolas-raoul
Copy link
Member

I believe this is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants