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

"Remove location" button for new uploads #5262

Closed
wants to merge 18 commits into from

Conversation

paco-arana
Copy link
Contributor

Fixes #5247

What changes did you make and why?

The Media Details step has a location editor, but no way to remove location for privacy reasons or just because it is not that relevant to the picture. A few changes were made to give the user the option to remove this information.

  • Added "Remove Location" button and set it to be displayed in situations where the "Edit Location" button is present in the screen.
  • When the "Remove Location" button is clicked it displays a popup warning that pictures without location aren't as useful.
  • Currently if the user decides to proceed the location will be overwritten to 0,0 using similar code to that of placeSelected().

I will revise the last point once I understand the model better so that it actually displays as "No coordinates provided", hence why I've marked this PR as a draft.

Tests performed

Tested ProdDebug on virtual device and Xiaomi Redmi 9 and with API level 30.

Screenshots
Screenshot 2023-07-24 at 12 40 08
Screenshot 2023-07-24 at 12 40 16

private void removeLocationFromPicture() {
// Set the camera position to (0, 0)
cameraPosition = new CameraPosition.Builder()
.target(new LatLng(0, 0))
Copy link
Member

Choose a reason for hiding this comment

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

Great work so far! As you wrote, removing location altogether is needed here. 🙂

@paco-arana
Copy link
Contributor Author

I got it to display location as none provided in uploads but I still have to add a flag so that this popup is not shown when location is removed by the user
Screenshot 2023-07-26 at 17 34 11

@nicolas-raoul
Copy link
Member

Great!
Ah yes, not showing this popup when the user removed the location on purpose would be a nice extra touch.
If difficult or if it makes the code complex feel free to not do it though. :-)
Thanks a lot!

@paco-arana
Copy link
Contributor Author

Thanks, I think I will skip it then. It's proving a bit more complicated than I thought.

What should be done about the checks failed? Maybe I'm reading it wrong, but it seems an issue with gradle, is there something I can do about that?

@nicolas-raoul
Copy link
Member

About unit tests, this appears to be the problematic part:

 fr.free.nrw.commons.locationpicker.LocationPickerActivityUnitTests > testOnClickModifyLocation FAILED
    java.lang.reflect.InvocationTargetException at LocationPickerActivityUnitTests.kt:220
        Caused by: java.lang.NullPointerException at LocationPickerActivityUnitTests.kt:220

fr.free.nrw.commons.locationpicker.LocationPickerActivityUnitTests > testOnStyleLoaded FAILED
    java.lang.reflect.InvocationTargetException at LocationPickerActivityUnitTests.kt:204
        Caused by: java.lang.NullPointerException at LocationPickerActivityUnitTests.kt:204

https://github.com/commons-app/apps-android-commons/actions/runs/5675123008/job/15379874043?pr=5262#step:10:280

To me it seems related to the present pull request... Would you mind checking? :-)

@paco-arana
Copy link
Contributor Author

I'm having an issue running the tests on my machine, pretty much all the tests fail with the same text. I've been trying some solutions from the web but I think I might as well ask if it's a common problem with an easy fix:

Screenshot 2023-07-27 at 18 04 51

@nicolas-raoul
Copy link
Member

Strange... maybe try in a clean environment such as a new VM?

@nicolas-raoul
Copy link
Member

Or how about blindly trying to fix the code of testOnClickModifyLocation and testOnStyleLoaded, and pushing to this branch every time you want to try? CI will happily run the tests for you.

@paco-arana
Copy link
Contributor Author

I think I'll do that. I did try cloning the project to a different pc but ran into the same issue.

@nicolas-raoul
Copy link
Member

Do not hesitate to push commits as many time as needed. Thanks a lot! :-)

@paco-arana
Copy link
Contributor Author

I'm sorry but is there someone I can ask for help? I think I've gotten stuck for good. I don't know much about testing and I'm having trouble understanding why and how my changes affect those two tests in particular.

@nicolas-raoul
Copy link
Member

Hi Paco,

Unfortunately it is difficult for potential contributors to see your call for help here, so I created an issue about that.

By the way, ideally the location should also be removed from the EXIF of the JPG file itself. Does that happen here? The app already has code to remove various EXIF field from the JPG file for privacy purposes, that can probably be either reused or taken as a code example.

Thanks! :-)

@nicolas-raoul
Copy link
Member

nicolas-raoul commented Oct 14, 2023

Hi Paco,

Tomer has kindly fixed the unit tests. :-)

Now the "only" thing left is to remove the latitude/longitude from the EXIF itself.
I just uploaded a picture with your code, ad as you can see the EXIF location is still present:

https://commons.wikimedia.org/wiki/File:Car_carrier_loading_Maybach_in_Tokyo.jpg
Screenshot 2023-10-14 at 18 40 55

Steps to reproduce:

  1. Select for upload a picture that has EXIF location.
  2. At the caption/description step, tap the map icon then tap Remove location.
  3. Continue and finish the upload.
  4. In the home activity, tap the uploaded image, then tap the three dots, then tap View file page, then scroll down to the Metadata section, and find the latitude and longitude values.

As I mentioned above, the app already has code to remove various EXIF field from the JPG file for privacy purposes, that can probably be either reused or taken as a code example.

If you have no time to do it, please let us know and someone else may be able to finish the work. :-)
Thanks a lot!

@shankarpriyank
Copy link
Contributor

In case Paco is not available, I am willing to fix(removing the lat/long) it.

@nicolas-raoul
Copy link
Member

Tomer (who fixed the unit tests) has already expressed interest in fixing this issue, but Priyank's help would be very welcome if both Paco and Tomer are unavailable.

@paco-arana If you are interested in fixing the EXIF issue. please let us know within 24 hours from now, otherwise Tomer can work on it. If no news from Tomer either within 48 hours from now, Priyank can work on it.

Thanks all!

@TomerPacific
Copy link

@nicolas-raoul - I can work on it, but I don't mind giving it to @shankarpriyank.

@rohit9625
Copy link
Contributor

I guess issue has been resolved. So, this PR should be closed.
What do you think @nicolas-raoul

@nicolas-raoul
Copy link
Member

@rohit9625 I don't see a "Remove location" button in main, do you?

@rohit9625
Copy link
Contributor

You are right Mr. @nicolas-raoul
I also don't see any remove location button.
Sorry I thought issue is solved, by reading comments from this and referenced PR.

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

Successfully merging this pull request may close these issues.

Upload Wizard: Let user remove picture location
5 participants