fix geotagging location selection, providers, lifecycle and staleness

app/src/main/java/app/grapheneos/camera/App.kt

@@ -8,17 +8,23 @@ import android.location.LocationListener
 import android.location.LocationManager
 import android.os.Build
 import android.os.CountDownTimer
+import android.os.SystemClock
 import android.view.WindowManager
 import androidx.annotation.RequiresPermission
 import androidx.appcompat.app.AppCompatActivity
-import app.grapheneos.camera.ktx.isSystemApp
 import app.grapheneos.camera.ui.activities.MainActivity
 import com.google.android.material.color.DynamicColors
+import java.util.concurrent.TimeUnit
 
 class App : Application() {
 
     companion object {
         private const val STALE_LOCATION_THRESHOLD = 11 * 1000L
+
+        // Locations older than this aren't attached to captures. Apps holding
+        // only the approximate location permission receive at most one location
+        // every 10 minutes from the OS, so this must stay above that interval
+        private const val MAX_LOCATION_AGE = 15 * 60 * 1000L
     }
 
     private var activity: MainActivity? = null
@@ -43,10 +49,7 @@ class App : Application() {
             }
 
             override fun onLocationChanged(locations: MutableList<Location>) {
-                val location = locations.getOptimalLocation()
-                if (location != null) {
-                    this@App.location = location
-                }
+                location = (locations + location).getOptimalLocation()
             }
 
             override fun onProviderEnabled(provider: String) {}
@@ -82,32 +85,25 @@ class App : Application() {
         return false
     }
 
-    fun List<Location?>.getOptimalLocation(): Location? {
-        if (isNullOrEmpty()) return null
-
-        var optimalLocation: Location? = null
-        forEach { location ->
-            if (location != null) {
-                if (optimalLocation == null) {
-                    optimalLocation = location
-                    return@forEach
-                }
-
-                val timeDifference = location.time - optimalLocation.time
-
-                // If the location is older than STALE_LOCATION_THRESHOLD ms
-                if (timeDifference > STALE_LOCATION_THRESHOLD) {
-                    optimalLocation = location
-                } else {
-                    // Compare their accuracy instead of time if the difference is below
-                    // threshold
-                    if (location.accuracy > optimalLocation.accuracy) {
-                        optimalLocation = location
-                    }
-                }
-            }
-        }
-        return optimalLocation
+    private fun List<Location?>.getOptimalLocation(): Location? {
+        val locations = filterNotNull()
+
+        // Compare ages with the monotonic clock; Location.getTime() is wall-clock
+        // time, which can jump and can disagree between providers
+        val newest = locations.maxByOrNull { it.elapsedRealtimeNanos } ?: return null
+
+        // Among the locations that aren't significantly older than the newest
+        // one, pick the most accurate. getAccuracy() is a radius in meters, so
+        // a smaller value is more accurate; a location without accuracy
+        // information is considered least accurate, and ties go to the newer one
+        return locations.filter {
+            val ageDifferenceMs =
+                TimeUnit.NANOSECONDS.toMillis(newest.elapsedRealtimeNanos - it.elapsedRealtimeNanos)
+            ageDifferenceMs <= STALE_LOCATION_THRESHOLD
+        }.minWithOrNull(compareBy(
+            { if (it.hasAccuracy()) it.accuracy else Float.MAX_VALUE },
+            { -it.elapsedRealtimeNanos }
+        ))
     }
 
     override fun onCreate() {
@@ -126,37 +122,60 @@ class App : Application() {
             dropLocationUpdates()
         }
         isLocationFetchInProgress = true
-        if (location == null) {
-            val providers = if (applicationInfo.isSystemApp() && Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
-                listOf<String>(LocationManager.FUSED_PROVIDER)
-            } else {
-                locationManager.allProviders
-            }
-            val locations = providers.map {
-                locationManager.getLastKnownLocation(it)
-            }
-            val fetchedLocation = locations.getOptimalLocation()
-            if (fetchedLocation != null) {
-                location = fetchedLocation
-            }
+
+        val providers = locationProviders
+
+        // A location stored from an earlier session shouldn't shadow a fresher
+        // last known location, so merge them instead of skipping the query
+        val lastKnownLocations = providers.map {
+            locationManager.getLastKnownLocation(it)
         }
+        location = (lastKnownLocations + location).getOptimalLocation()
 
-        locationManager.allProviders.forEach { provider ->
+        providers.forEach { provider ->
             locationManager.requestLocationUpdates(
                 provider,
                 2000,
-                10f,
+                0f,
                 locationListener
             )
         }
     }
 
+    // The fused provider combines the underlying providers itself, so use it
+    // alone when it's available (GrapheneOS implements it in the OS; elsewhere
+    // it may be backed by Play services)
+    private val locationProviders: List<String>
+        get() {
+            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S &&
+                locationManager.hasProvider(LocationManager.FUSED_PROVIDER)
+            ) {
+                return listOf(LocationManager.FUSED_PROVIDER)
+            }
+            return locationManager.allProviders.filter {
+                it == LocationManager.GPS_PROVIDER || it == LocationManager.NETWORK_PROVIDER
+            }
+        }
+
     fun dropLocationUpdates() {
         isLocationFetchInProgress = false
         locationManager.removeUpdates(locationListener)
     }
 
-    fun getLocation(): Location? = location
+    // For when the user turns geotagging off, unlike a temporary drop of the
+    // updates while the activity is paused
+    fun disableLocationFetching() {
+        dropLocationUpdates()
+        location = null
+    }
+
+    fun getLocation(): Location? {
+        val location = this.location ?: return null
+        val ageMs = TimeUnit.NANOSECONDS.toMillis(
+            SystemClock.elapsedRealtimeNanos() - location.elapsedRealtimeNanos
+        )
+        return if (ageMs > MAX_LOCATION_AGE) null else location
+    }
 
     private fun isLocationEnabled(): Boolean = locationManager.isLocationEnabled
 

app/src/main/java/app/grapheneos/camera/ui/activities/MainActivity.kt

@@ -585,6 +585,9 @@ open class MainActivity : AppCompatActivity(),
         } else {
             imageCapturer.cancelPendingCaptureRequest()
         }
+        if (camConfig.requireLocation) {
+            application.dropLocationUpdates()
+        }
         lastFrame = null
     }
 
@@ -1690,7 +1693,7 @@ open class MainActivity : AppCompatActivity(),
         if (required) {
             requestLocation()
         } else {
-            application.dropLocationUpdates()
+            application.disableLocationFetching()
         }
     }