Usage of outdated location

[Nakaner]

This is a follow-up of sozialhelden/wheelmap/#648 and following threads in the German OSM Forum:

• Immer wieder falsch positionierte Wheelmap - Eintragungen
• Warum wheelmap.org in München einfach so sauviel "Spaß" macht!
• Wieder mal falsch plazierte Wheelmap-POIs; diesmal: Halle (Saale)

The OpenStreetMap community still observes new POIs being added at wrong locations and is going to lose patience.

I assume that the bug is located in org.wheelmap.android.manager.MyLocationManager.calcBestLastKnownLocation(). This method is called by the constructor of the MyLocationManager class.

    private Location calcBestLastKnownLocation() {
        Location networkLocation = mLocationManager
                .getLastKnownLocation(LocationManager.NETWORK_PROVIDER);
        Location gpsLocation = mLocationManager
                .getLastKnownLocation(LocationManager.GPS_PROVIDER);

        long now = System.currentTimeMillis();
        if (gpsLocation == null && networkLocation == null) {
            return null;
        } else if (gpsLocation == null) {
            return networkLocation;
        } else if (networkLocation == null) {
            return gpsLocation;
        } else if (now - gpsLocation.getTime() < TIME_DISTANCE_LIMIT) {
            return gpsLocation;
        } else if (gpsLocation.getTime() < networkLocation.getTime()) {
            return gpsLocation;
        } else {
            return networkLocation;
        }
    }

As a first step, the code retrieves the last known location from the network and GPS provider using the Android system library. If both fails, the method returns null. The constructor is already able to handle null properly.

If one of the two retrieved locations is null, the other one is returned even if it is old. That's one reason of the bug. If both locations are not null, the age of the GPS location is checked. If it is less than 5 minutes old, it will be returned.

The next line contains the second part of the bug. It returns true if the GPS location is older than the network location. The comparison operator is < but it should be >= or > because Location.getTime() returns the time in seconds since 1970-01-01.

How to fix?

There are two possible fixes: A simple hack and a better solution.

The simple hack would only use the GPS as source of location and harm user experience where GPS is difficult to use (cities with high buildings, indoor etc.). The method would look like this:

    private Location calcBestLastKnownLocation() {
        Location gpsLocation = mLocationManager
                .getLastKnownLocation(LocationManager.GPS_PROVIDER);

        long now = System.currentTimeMillis();
        if (gpsLocation == null) {
            return null;
        } else if (now - gpsLocation.getTime() < TIME_DISTANCE_LIMIT) {
            return gpsLocation;
        }
        return null;
    }

The second solution would require to check the status of the location everywhere in the app where the location is used for anything else than just centering the map view. Following changes are necessary:

1. Apply following patch:

diff --git a/app/src/main/java/org/wheelmap/android/manager/MyLocationManager.java b/app/src/main/java/org/wheelmap/android/manager/MyLocationManager.java
index f8e4947..bfbcad7 100644
--- a/app/src/main/java/org/wheelmap/android/manager/MyLocationManager.java
+++ b/app/src/main/java/org/wheelmap/android/manager/MyLocationManager.java
@@ -75,6 +75,8 @@ public class MyLocationManager {
 
     private Handler mHandler = new Handler();
 
+    private boolean isLocationValid = false;
+
     public static class LocationEvent {
 
         public final Location location;
@@ -124,16 +126,15 @@ public class MyLocationManager {
         mNetworkLocationListener = new MyNetworkLocationListener();
 
         mCurrentBestLocation = calcBestLastKnownLocation();
-        boolean isValid = true;
         if (mCurrentBestLocation == null) {
-            isValid = false;
+            isLocationValid = false;
             mCurrentBestLocation = new Location(
                     LocationManager.NETWORK_PROVIDER);
             mCurrentBestLocation.setLongitude(DEFAULT_LONGITUDE);
             mCurrentBestLocation.setLatitude(DEFAULT_LATITUDE);
             mCurrentBestLocation.setAccuracy(1000 * 1000);
         }
-        mBus.postSticky(new LocationEvent(mCurrentBestLocation, isValid));
+        mBus.postSticky(new LocationEvent(mCurrentBestLocation, isLocationValid));
         wasBestLastKnownLocation = true;
 
     }
@@ -226,14 +227,16 @@ public class MyLocationManager {
         if (gpsLocation == null && networkLocation == null) {
             return null;
         } else if (gpsLocation == null) {
+            isLocationValid = false;
             return networkLocation;
-        } else if (networkLocation == null) {
-            return gpsLocation;
         } else if (now - gpsLocation.getTime() < TIME_DISTANCE_LIMIT) {
+            isLocationValid = true;
             return gpsLocation;
-        } else if (gpsLocation.getTime() < networkLocation.getTime()) {
+        } else if (networkLocation == null || gpsLocation.getTime() < networkLocation.getTime()) {
+            isLocationValid = false;
             return gpsLocation;
         } else {
+            isLocationValid = false;
             return networkLocation;
         }
     }

2. Check the validity of the location everywhere where it is necessary.

[holgerd]

Thanks, we'll review it and update here.

[dooley]

How long will it take to review some 10 lines of code?