Fixes Issue #5713: "Edit location" is hard to use (and confusing) (#5767)

* LocationPickerActivity.java: fix "Show in Map App" bug

Once the "Show in Map App" button is pressed, the Map app will center on the current photo's EXIF location,
if that data is available. If not, the map app will center on where the location picker's map is centered.
Javadoc updated to reflect this small change.

* LocationPickerActivity.java: add methods to easily move the map center

Before this change, any time the map center had to move, several lines of code had to be written.

This change creates several methods which move the map center to a specified location. By using these new methods,
moving the map center only takes one simple method call.

* LocationPickerActivity.java: add null check to method

The original method did not include a null check for the input GeoPoint.

This change includes a null check. If the input Geopoint is null, the method will simply return.

* LocationPickerActivity.java: remove redundant method, renaming method

Before this change, there were two methods with the same behavior.

This change removes the newer method and renames the old method to the descriptive name the newer one had.
Additionally, code which calls this method has been changed to reflect the new name.

* LocationPickerActivity.java: rearrange method calls.

Before this change, a method call to move the map to the device's GPS location was called at the very end of the map setup method.
This would move the map away from the media's EXIF location data (if it was available).

This change places the method call to move the map to the device's GPS location before the method call to move the map to the media's EXIF location (if the data is available).
In short, if no exif data is available, the map attempts to move to the device's GPS location. If the exif location data does exist, the map will move to that location.

* LocationPickerActivity.java: rewrite method scope, name, and documentation

Before this commit, the method name was unclear and the documentation did not fully explain the method's behavior. Additionally, it was a public method.

This commit renames the method (also changing calls to it), adds more documentation, and changes the method scope from public to private.

* LocationPickerActivity.java: create method to move location picker map to device GPS location

Before this method was created, several lines of code were required to move the location picker map to the device's GPS location.

After this method was created, the location picker map can be moved to the GPS location in a single method call.

* LocationPickerActivity.java: add code to move map to either EXIF or device GPS location

Before this change, there was no explicit if check on whether there was EXIF data available before moving the location picker map.

After this change, an explicit if check (which checks the activity name string) will move the location picker map to either
the EXIF location (if that data is available) or to the device's GPS location (if EXIF data is not available).

* LocationPickerActivity.java: refactor showInMapApp method

Before this change, the showInMapApp method had correct behavior, but could be rewritten to become more clear.

After this change, the showInMapApp code has been rewritten.
Specifically, common code in an if statement has been factored out.

* LocationPickerActivity.java: add null checks to showInMapApp

Before this change, there was no null checks when accessing data from an object,
which could create null pointer exceptions at runtime.

After this change, null checks are introduced to make sure null pointer exceptions will not occur.

* LocationPickerActivity.java: rewrite comments to clarify if statement

Before this change, a comment in showInMapApp did not properly describe an if statement's intended behavior.

After this change, the old comment was removed and 2 new comments were added in each part of the if statement
to properly describe the intended behavior.

* LocationPickerActivity.java: replace code with a method call

Before this change, there was several lines of code which moved the map center to the EXIF location.

After this change, the several lines of code have been replaced with a simpler method call which produces
the same result.

* LocationPickerActivity.java: remove redundant code

Before this change, there was two sections of code which moved the map center to the same location. Both of these code sections
occur very close to each other (one in onCreate(), the other in setupMapView())

After this change, the code section in onCreate() has been removed. With the map centering code only in the setupMapView() method.
This change eliminates redundancy, reduces the amount of code in onCreate(), and makes this java file easier to understand.

* content_location_picker.xml: adjust picker pin and shadow location

Before this commit, the location picker pin and shadow graphics were incorrectly located on the screen.
Specifically, the bottom of the pin was not located at the center of the view. Since pressing the
checkmark button saves the location at the center of the view, users would most likely save the wrong location.

After this commit, the location picker pin and shadow graphics have been moved upward. The shadow graphic is
now located at the exact center of the view, and the bottom of the location picker pin is directly over the
center as well. Users may now use the bottom of the pin as an accurate location picker.

* LocationPickerActivity.java: fix bug with permissions menu moving map

Prior to this change, if the menu asking for permissions to access the device's GPS was accepted, the map
would automatically move to the most recent or current GPS location, rather than staying at the uploaded
image's available EXIF location.

After this change, the map will stay centered at the image's available EXIF location, as intended.
The relevant method name and javadoc have been changed to more accurately describe the method's behavior.

* LocationPickerActivity.java: fix map centering bug when editing location on already uploaded media

Before this commit, when the user pressed the edit location icon on media which was already uploaded,
the map would center on the device's current GPS location.

After this commit, pressing the same edit location icon will now center the map on the location metadata.
This is done by checking the activity string to see if the media is already uploaded. If so, a method
is called to center the map on the media's location metadata.

* LocationPickerActivity.java: replace references to EXIF in documentation

Before this commit, there were several mentions of EXIF location data in the javadocs. It is not clear if
this is correct, since many images have location data that is chosen by the user rather than derived from EXIF.

After this commit, the more generic term "location metadata" is used in place of EXIF location data.
Hopefully this change will help avoid confusion in the future.

---------

Co-authored-by: Nicolas Raoul <nicolas.raoul@gmail.com>

Author: Jason-Whitmore

app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerActivity.java

@@ -40,7 +40,6 @@
 import fr.free.nrw.commons.Utils;
 import fr.free.nrw.commons.auth.SessionManager;
 import fr.free.nrw.commons.auth.csrf.CsrfTokenClient;
-import fr.free.nrw.commons.auth.csrf.InvalidLoginTokenException;
 import fr.free.nrw.commons.coordinates.CoordinateEditHelper;
 import fr.free.nrw.commons.filepicker.Constants;
 import fr.free.nrw.commons.kvstore.BasicKvStore;
@@ -237,15 +236,28 @@ protected void onCreate(@Nullable final Bundle savedInstanceState) {
                 cameraPosition.getLongitude()));
         }
         setupMapView();
-        
-        if("UploadActivity".equals(activity)){
-            if(mapView != null && mapView.getController() != null && cameraPosition != null){
-                GeoPoint cameraGeoPoint = new GeoPoint(cameraPosition.getLatitude(),
-                    cameraPosition.getLongitude());
-
-                mapView.getController().setCenter(cameraGeoPoint);
-                mapView.getController().animateTo(cameraGeoPoint);
-            }
+    }
+
+    /**
+     * Moves the center of the map to the specified coordinates
+     *
+     */
+    private void moveMapTo(double latitude, double longitude){
+        if(mapView != null && mapView.getController() != null){
+            GeoPoint point = new GeoPoint(latitude, longitude);
+
+            mapView.getController().setCenter(point);
+            mapView.getController().animateTo(point);
+        }
+    }
+
+    /**
+     * Moves the center of the map to the specified coordinates
+     * @param point The GeoPoint object which contains the coordinates to move to
+     */
+    private void moveMapTo(GeoPoint point){
+        if(point != null){
+            moveMapTo(point.getLatitude(), point.getLongitude());
         }
     }
 
@@ -304,12 +316,20 @@ private void getToolbarUI() {
     }
 
     private void setupMapView() {
-        adjustCameraBasedOnOptions();
+        requestLocationPermissions();
+
+        //If location metadata is available, move map to that location.
+        if(activity.equals("UploadActivity") || activity.equals("MediaActivity")){
+            moveMapToMediaLocation();
+        } else {
+            //If location metadata is not available, move map to device GPS location.
+            moveMapToGPSLocation();
+        }
+
         modifyLocationButton.setOnClickListener(v -> onClickModifyLocation());
         removeLocationButton.setOnClickListener(v -> onClickRemoveLocation());
-        showInMapButton.setOnClickListener(v -> showInMap());
+        showInMapButton.setOnClickListener(v -> showInMapApp());
         darkThemeSetup();
-        requestLocationPermissions();
     }
 
     /**
@@ -326,12 +346,7 @@ private void onClickModifyLocation() {
         smallToolbarText.setText(getResources().getString(R.string.pan_and_zoom_to_adjust));
         fabCenterOnLocation.setVisibility(View.VISIBLE);
         removeSelectedLocationMarker();
-        if (cameraPosition != null && mapView != null) {
-            if (mapView.getController() != null) {
-                mapView.getController().animateTo(new GeoPoint(cameraPosition.getLatitude(),
-                    cameraPosition.getLongitude()));
-            }
-        }
+        moveMapToMediaLocation();
     }
 
     /**
@@ -364,21 +379,53 @@ private void removeLocationFromImage() {
     }
 
     /**
-     * Show the location in map app
+     * Show the location in map app. Map will center on the location metadata, if available.
+     * If there is no location metadata, the map will center on the commons app map center.
      */
-    public void showInMap() {
-        Utils.handleGeoCoordinates(this,
-            new fr.free.nrw.commons.location.LatLng(mapView.getMapCenter().getLatitude(),
-                mapView.getMapCenter().getLongitude(), 0.0f));
+    private void showInMapApp() {
+        fr.free.nrw.commons.location.LatLng position = null;
+
+        if(activity.equals("UploadActivity") && cameraPosition != null){
+            //location metadata is available
+            position = new fr.free.nrw.commons.location.LatLng(cameraPosition.getLatitude(),
+                cameraPosition.getLongitude(), 0.0f);
+        } else if(mapView != null){
+            //location metadata is not available
+            position = new fr.free.nrw.commons.location.LatLng(mapView.getMapCenter().getLatitude(),
+                mapView.getMapCenter().getLongitude(), 0.0f);
+        }
+
+        if(position != null){
+            Utils.handleGeoCoordinates(this, position);
+        }
     }
 
     /**
-     * move the location to the current media coordinates
+     * Moves the center of the map to the media's location, if that data
+     * is available.
      */
-    private void adjustCameraBasedOnOptions() {
+    private void moveMapToMediaLocation() {
         if (cameraPosition != null) {
-            mapView.getController().setCenter(new GeoPoint(cameraPosition.getLatitude(),
-                cameraPosition.getLongitude()));
+
+            GeoPoint point = new GeoPoint(cameraPosition.getLatitude(),
+                cameraPosition.getLongitude());
+
+            moveMapTo(point);
+        }
+    }
+
+    /**
+     * Moves the center of the map to the device's GPS location, if that data is available.
+     */
+    private void moveMapToGPSLocation(){
+        if(locationManager != null){
+            fr.free.nrw.commons.location.LatLng location = locationManager.getLastLocation();
+
+            if(location != null){
+                GeoPoint point = new GeoPoint(location.getLatitude(), location.getLongitude());
+
+                moveMapTo(point);
+            }
         }
     }
 
@@ -557,26 +604,25 @@ public void onLocationPermissionGranted() {
                 locationManager.requestLocationUpdatesFromProvider(
                     LocationManager.NETWORK_PROVIDER);
                 locationManager.requestLocationUpdatesFromProvider(LocationManager.GPS_PROVIDER);
-                getLocation();
+                addMarkerAtGPSLocation();
             } else {
-                getLocation();
+                addMarkerAtGPSLocation();
                 locationPermissionsHelper.showLocationOffDialog(this,
                     R.string.ask_to_turn_location_on_text);
             }
         }
     }
 
     /**
-     * Gets new location if locations services are on, else gets last location
+     * Adds a marker to the map at the most recent GPS location
+     * (which may be the current GPS location).
      */
-    private void getLocation() {
+    private void addMarkerAtGPSLocation() {
         fr.free.nrw.commons.location.LatLng currLocation = locationManager.getLastLocation();
         if (currLocation != null) {
             GeoPoint currLocationGeopoint = new GeoPoint(currLocation.getLatitude(),
                 currLocation.getLongitude());
             addLocationMarker(currLocationGeopoint);
-            mapView.getController().setCenter(currLocationGeopoint);
-            mapView.getController().animateTo(currLocationGeopoint);
             markerImage.setTranslationY(0);
         }
     }

app/src/main/res/layout/content_location_picker.xml

@@ -16,6 +16,7 @@
     app:layout_constraintStart_toStartOf="parent"
     app:layout_constraintTop_toTopOf="parent"
     app:srcCompat="@drawable/map_default_map_marker"
+    android:layout_marginBottom="45dp"
     android:contentDescription="@string/location_picker_image_view" />
 
   <ImageView
@@ -24,10 +25,10 @@
     android:layout_height="wrap_content"
     android:contentDescription="@string/location_picker_image_view_shadow"
     android:elevation="1dp"
+    app:layout_constraintBottom_toBottomOf="parent"
     app:layout_constraintEnd_toEndOf="parent"
-    app:layout_constraintRight_toRightOf="parent"
     app:layout_constraintStart_toStartOf="parent"
-    app:layout_constraintTop_toTopOf="@+id/location_picker_image_view_marker"
+    app:layout_constraintTop_toTopOf="parent"
     app:srcCompat="@drawable/map_default_map_marker_shadow" />
 
   <org.osmdroid.views.MapView