diff --git a/changelog.d/5913.misc b/changelog.d/5913.misc new file mode 100644 index 0000000000..24be7194d5 --- /dev/null +++ b/changelog.d/5913.misc @@ -0,0 +1,3 @@ +- Notify of the latest known location in LocationTracker to avoid multiple locations at start +- Debounce location updates +- Improve location providers access diff --git a/vector/src/main/java/im/vector/app/features/location/LocationSharingService.kt b/vector/src/main/java/im/vector/app/features/location/LocationSharingService.kt index 8b9a1c75ae..62aba9318c 100644 --- a/vector/src/main/java/im/vector/app/features/location/LocationSharingService.kt +++ b/vector/src/main/java/im/vector/app/features/location/LocationSharingService.kt @@ -198,7 +198,7 @@ class LocationSharingService : VectorService(), LocationTracker.Callback { ) } - override fun onLocationProviderIsNotAvailable() { + override fun onNoLocationProviderAvailable() { stopForeground(true) stopSelf() } diff --git a/vector/src/main/java/im/vector/app/features/location/LocationSharingViewModel.kt b/vector/src/main/java/im/vector/app/features/location/LocationSharingViewModel.kt index 9ca340f098..71f59c6fdf 100644 --- a/vector/src/main/java/im/vector/app/features/location/LocationSharingViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/location/LocationSharingViewModel.kt @@ -182,7 +182,7 @@ class LocationSharingViewModel @AssistedInject constructor( } } - override fun onLocationProviderIsNotAvailable() { + override fun onNoLocationProviderAvailable() { _viewEvents.post(LocationSharingViewEvents.LocationNotAvailableError) } } diff --git a/vector/src/main/java/im/vector/app/features/location/LocationTracker.kt b/vector/src/main/java/im/vector/app/features/location/LocationTracker.kt index 1913f4202d..cdf13a7004 100644 --- a/vector/src/main/java/im/vector/app/features/location/LocationTracker.kt +++ b/vector/src/main/java/im/vector/app/features/location/LocationTracker.kt @@ -21,13 +21,19 @@ import android.content.Context import android.location.Location import android.location.LocationManager import androidx.annotation.RequiresPermission +import androidx.annotation.VisibleForTesting import androidx.core.content.getSystemService import androidx.core.location.LocationListenerCompat import im.vector.app.BuildConfig +import im.vector.app.core.utils.Debouncer +import im.vector.app.core.utils.createBackgroundHandler import timber.log.Timber import javax.inject.Inject import javax.inject.Singleton +private const val BKG_HANDLER_NAME = "LocationTracker.BKG_HANDLER_NAME" +private const val LOCATION_DEBOUNCE_ID = "LocationTracker.LOCATION_DEBOUNCE_ID" + @Singleton class LocationTracker @Inject constructor( context: Context @@ -36,62 +42,92 @@ class LocationTracker @Inject constructor( private val locationManager = context.getSystemService() interface Callback { + /** + * Called on every location update. + */ fun onLocationUpdate(locationData: LocationData) - fun onLocationProviderIsNotAvailable() + + /** + * Called when no location provider is available to request location updates. + */ + fun onNoLocationProviderAvailable() } - private val callbacks = mutableListOf() + @VisibleForTesting + val callbacks = mutableListOf() - private var hasGpsProviderLiveLocation = false + @VisibleForTesting + var hasLocationFromFusedProvider = false + + @VisibleForTesting + var hasLocationFromGPSProvider = false private var lastLocation: LocationData? = null + private val debouncer = Debouncer(createBackgroundHandler(BKG_HANDLER_NAME)) + @RequiresPermission(anyOf = [Manifest.permission.ACCESS_COARSE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION]) fun start() { - Timber.d("## LocationTracker. start()") - hasGpsProviderLiveLocation = false + Timber.d("start()") if (locationManager == null) { - callbacks.forEach { it.onLocationProviderIsNotAvailable() } - Timber.v("## LocationTracker. LocationManager is not available") + Timber.v("LocationManager is not available") + onNoLocationProviderAvailable() return } - locationManager.allProviders - .takeIf { it.isNotEmpty() } - // Take GPS first - ?.sortedByDescending { if (it == LocationManager.GPS_PROVIDER) 1 else 0 } - ?.forEach { provider -> - Timber.d("## LocationTracker. track location using $provider") + val providers = locationManager.allProviders - // Send last known location without waiting location updates - locationManager.getLastKnownLocation(provider)?.let { lastKnownLocation -> - if (BuildConfig.LOW_PRIVACY_LOG_ENABLE) { - Timber.d("## LocationTracker. lastKnownLocation: $lastKnownLocation") - } else { - Timber.d("## LocationTracker. lastKnownLocation: ${lastKnownLocation.provider}") - } - notifyLocation(lastKnownLocation, isLive = false) + if (providers.isEmpty()) { + Timber.v("There is no location provider available") + onNoLocationProviderAvailable() + } else { + // Take GPS first + providers.sortedByDescending(::getProviderPriority) + .mapNotNull { provider -> + Timber.d("track location using $provider") + + locationManager.requestLocationUpdates( + provider, + MIN_TIME_TO_UPDATE_LOCATION_MILLIS, + MIN_DISTANCE_TO_UPDATE_LOCATION_METERS, + this + ) + + locationManager.getLastKnownLocation(provider) } + .maxByOrNull { location -> location.time } + ?.let { latestKnownLocation -> + if (BuildConfig.LOW_PRIVACY_LOG_ENABLE) { + Timber.d("lastKnownLocation: $latestKnownLocation") + } else { + Timber.d("lastKnownLocation: ${latestKnownLocation.provider}") + } + notifyLocation(latestKnownLocation) + } + } + } - locationManager.requestLocationUpdates( - provider, - MIN_TIME_TO_UPDATE_LOCATION_MILLIS, - MIN_DISTANCE_TO_UPDATE_LOCATION_METERS, - this - ) - } - ?: run { - callbacks.forEach { it.onLocationProviderIsNotAvailable() } - Timber.v("## LocationTracker. There is no location provider available") - } + /** + * Compute the priority of the given provider name. + * @return an integer representing the priority: the higher the value, the higher the priority is. + */ + private fun getProviderPriority(provider: String): Int = when (provider) { + LocationManager.FUSED_PROVIDER -> 2 + LocationManager.GPS_PROVIDER -> 1 + else -> 0 } @RequiresPermission(anyOf = [Manifest.permission.ACCESS_COARSE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION]) fun stop() { - Timber.d("## LocationTracker. stop()") + Timber.d("stop()") locationManager?.removeUpdates(this) - callbacks.clear() + synchronized(this) { + callbacks.clear() + } + debouncer.cancelAll() + hasLocationFromGPSProvider = false + hasLocationFromFusedProvider = false } /** @@ -99,15 +135,17 @@ class LocationTracker @Inject constructor( * Please ensure adding a callback to receive the value. */ fun requestLastKnownLocation() { - lastLocation?.let { location -> callbacks.forEach { it.onLocationUpdate(location) } } + lastLocation?.let { locationData -> onLocationUpdate(locationData) } } + @Synchronized fun addCallback(callback: Callback) { if (!callbacks.contains(callback)) { callbacks.add(callback) } } + @Synchronized fun removeCallback(callback: Callback) { callbacks.remove(callback) if (callbacks.size == 0) { @@ -117,34 +155,86 @@ class LocationTracker @Inject constructor( override fun onLocationChanged(location: Location) { if (BuildConfig.LOW_PRIVACY_LOG_ENABLE) { - Timber.d("## LocationTracker. onLocationChanged: $location") + Timber.d("onLocationChanged: $location") } else { - Timber.d("## LocationTracker. onLocationChanged: ${location.provider}") + Timber.d("onLocationChanged: ${location.provider}") } - notifyLocation(location, isLive = true) - } - private fun notifyLocation(location: Location, isLive: Boolean) { when (location.provider) { + LocationManager.FUSED_PROVIDER -> { + hasLocationFromFusedProvider = true + } LocationManager.GPS_PROVIDER -> { - hasGpsProviderLiveLocation = isLive + if (hasLocationFromFusedProvider) { + hasLocationFromGPSProvider = false + // Ignore this update + Timber.d("ignoring location from ${location.provider}, we have location from fused provider") + return + } else { + hasLocationFromGPSProvider = true + } } else -> { - if (hasGpsProviderLiveLocation) { + if (hasLocationFromFusedProvider || hasLocationFromGPSProvider) { // Ignore this update - Timber.d("## LocationTracker. ignoring location from ${location.provider}, we have gps live location") + Timber.d("ignoring location from ${location.provider}, we have location from GPS provider") return } } } + + debouncer.debounce(LOCATION_DEBOUNCE_ID, MIN_TIME_TO_UPDATE_LOCATION_MILLIS) { + notifyLocation(location) + } + } + + private fun notifyLocation(location: Location) { + if (BuildConfig.LOW_PRIVACY_LOG_ENABLE) { + Timber.d("notify location: $location") + } else { + Timber.d("notify location: ${location.provider}") + } + val locationData = location.toLocationData() lastLocation = locationData - callbacks.forEach { it.onLocationUpdate(locationData) } + onLocationUpdate(locationData) } override fun onProviderDisabled(provider: String) { - Timber.d("## LocationTracker. onProviderDisabled: $provider") - callbacks.forEach { it.onLocationProviderIsNotAvailable() } + Timber.d("onProviderDisabled: $provider") + when (provider) { + LocationManager.FUSED_PROVIDER -> hasLocationFromFusedProvider = false + LocationManager.GPS_PROVIDER -> hasLocationFromGPSProvider = false + } + + locationManager?.allProviders + ?.takeIf { it.isEmpty() } + ?.let { + Timber.e("all providers have been disabled") + onNoLocationProviderAvailable() + } + } + + @Synchronized + private fun onNoLocationProviderAvailable() { + callbacks.forEach { + try { + it.onNoLocationProviderAvailable() + } catch (error: Exception) { + Timber.e(error, "error in onNoLocationProviderAvailable callback $it") + } + } + } + + @Synchronized + private fun onLocationUpdate(locationData: LocationData) { + callbacks.forEach { + try { + it.onLocationUpdate(locationData) + } catch (error: Exception) { + Timber.e(error, "error in onLocationUpdate callback $it") + } + } } private fun Location.toLocationData(): LocationData { diff --git a/vector/src/test/java/im/vector/app/features/location/LocationTrackerTest.kt b/vector/src/test/java/im/vector/app/features/location/LocationTrackerTest.kt new file mode 100644 index 0000000000..b0f3974226 --- /dev/null +++ b/vector/src/test/java/im/vector/app/features/location/LocationTrackerTest.kt @@ -0,0 +1,297 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.features.location + +import android.content.Context +import android.location.Location +import android.location.LocationManager +import im.vector.app.core.utils.Debouncer +import im.vector.app.core.utils.createBackgroundHandler +import im.vector.app.test.fakes.FakeContext +import im.vector.app.test.fakes.FakeHandler +import im.vector.app.test.fakes.FakeLocationManager +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import io.mockk.mockkConstructor +import io.mockk.mockkStatic +import io.mockk.runs +import io.mockk.slot +import io.mockk.unmockkAll +import io.mockk.verify +import io.mockk.verifyOrder +import org.amshove.kluent.shouldBeEqualTo +import org.junit.After +import org.junit.Before +import org.junit.Test + +private const val A_LATITUDE = 1.2 +private const val A_LONGITUDE = 44.0 +private const val AN_ACCURACY = 5.0f + +class LocationTrackerTest { + + private val fakeHandler = FakeHandler() + private val fakeLocationManager = FakeLocationManager() + private val fakeContext = FakeContext().also { + it.givenService(Context.LOCATION_SERVICE, android.location.LocationManager::class.java, fakeLocationManager.instance) + } + + private lateinit var locationTracker: LocationTracker + + @Before + fun setUp() { + mockkConstructor(Debouncer::class) + every { anyConstructed().cancelAll() } just runs + val runnable = slot() + every { anyConstructed().debounce(any(), MIN_TIME_TO_UPDATE_LOCATION_MILLIS, capture(runnable)) } answers { + runnable.captured.run() + true + } + mockkStatic("im.vector.app.core.utils.HandlerKt") + every { createBackgroundHandler(any()) } returns fakeHandler.instance + locationTracker = LocationTracker(fakeContext.instance) + fakeLocationManager.givenRemoveUpdates(locationTracker) + } + + @After + fun tearDown() { + unmockkAll() + } + + @Test + fun `given available list of providers when starting then location updates are requested in priority order`() { + val providers = listOf(LocationManager.GPS_PROVIDER, LocationManager.FUSED_PROVIDER, LocationManager.NETWORK_PROVIDER) + mockAvailableProviders(providers) + + locationTracker.start() + + verifyOrder { + fakeLocationManager.instance.requestLocationUpdates( + LocationManager.FUSED_PROVIDER, + MIN_TIME_TO_UPDATE_LOCATION_MILLIS, + MIN_DISTANCE_TO_UPDATE_LOCATION_METERS, + locationTracker + ) + fakeLocationManager.instance.requestLocationUpdates( + LocationManager.GPS_PROVIDER, + MIN_TIME_TO_UPDATE_LOCATION_MILLIS, + MIN_DISTANCE_TO_UPDATE_LOCATION_METERS, + locationTracker + ) + fakeLocationManager.instance.requestLocationUpdates( + LocationManager.NETWORK_PROVIDER, + MIN_TIME_TO_UPDATE_LOCATION_MILLIS, + MIN_DISTANCE_TO_UPDATE_LOCATION_METERS, + locationTracker + ) + } + } + + @Test + fun `given available list of providers when list is empty then callbacks are notified`() { + val providers = emptyList() + val callback = mockCallback() + + locationTracker.addCallback(callback) + fakeLocationManager.givenActiveProviders(providers) + + locationTracker.start() + + verify { callback.onNoLocationProviderAvailable() } + locationTracker.removeCallback(callback) + } + + @Test + fun `when adding or removing a callback then it is added into or removed from the list of callbacks`() { + val callback1 = mockCallback() + val callback2 = mockCallback() + + locationTracker.addCallback(callback1) + locationTracker.addCallback(callback2) + + locationTracker.callbacks.size shouldBeEqualTo 2 + locationTracker.callbacks.first() shouldBeEqualTo callback1 + locationTracker.callbacks[1] shouldBeEqualTo callback2 + + locationTracker.removeCallback(callback1) + + locationTracker.callbacks.size shouldBeEqualTo 1 + + locationTracker.removeCallback(callback2) + + locationTracker.callbacks.size shouldBeEqualTo 0 + verify { locationTracker.stop() } + } + + @Test + fun `when location updates are received from fused provider then fused locations are taken in priority`() { + val providers = listOf(LocationManager.GPS_PROVIDER, LocationManager.FUSED_PROVIDER, LocationManager.NETWORK_PROVIDER) + mockAvailableProviders(providers) + val callback = mockCallback() + locationTracker.addCallback(callback) + locationTracker.start() + + val fusedLocation = mockLocation( + provider = LocationManager.FUSED_PROVIDER, + latitude = 1.0, + longitude = 3.0, + accuracy = 4f + ) + val gpsLocation = mockLocation( + provider = LocationManager.GPS_PROVIDER + ) + + val networkLocation = mockLocation( + provider = LocationManager.NETWORK_PROVIDER + ) + locationTracker.onLocationChanged(fusedLocation) + locationTracker.onLocationChanged(gpsLocation) + locationTracker.onLocationChanged(networkLocation) + + val expectedLocationData = LocationData( + latitude = 1.0, + longitude = 3.0, + uncertainty = 4.0 + ) + verify { callback.onLocationUpdate(expectedLocationData) } + verify { anyConstructed().debounce(any(), MIN_TIME_TO_UPDATE_LOCATION_MILLIS, any()) } + locationTracker.hasLocationFromFusedProvider shouldBeEqualTo true + locationTracker.hasLocationFromGPSProvider shouldBeEqualTo false + } + + @Test + fun `when location updates are received from gps provider then gps locations are taken if none are received from fused provider`() { + val providers = listOf(LocationManager.GPS_PROVIDER, LocationManager.FUSED_PROVIDER, LocationManager.NETWORK_PROVIDER) + mockAvailableProviders(providers) + val callback = mockCallback() + locationTracker.addCallback(callback) + locationTracker.start() + + val gpsLocation = mockLocation( + provider = LocationManager.GPS_PROVIDER, + latitude = 1.0, + longitude = 3.0, + accuracy = 4f + ) + + val networkLocation = mockLocation( + provider = LocationManager.NETWORK_PROVIDER + ) + locationTracker.onLocationChanged(gpsLocation) + locationTracker.onLocationChanged(networkLocation) + + val expectedLocationData = LocationData( + latitude = 1.0, + longitude = 3.0, + uncertainty = 4.0 + ) + verify { callback.onLocationUpdate(expectedLocationData) } + verify { anyConstructed().debounce(any(), MIN_TIME_TO_UPDATE_LOCATION_MILLIS, any()) } + locationTracker.hasLocationFromFusedProvider shouldBeEqualTo false + locationTracker.hasLocationFromGPSProvider shouldBeEqualTo true + } + + @Test + fun `when location updates are received from network provider then network locations are taken if none are received from fused or gps provider`() { + val providers = listOf(LocationManager.GPS_PROVIDER, LocationManager.FUSED_PROVIDER, LocationManager.NETWORK_PROVIDER) + mockAvailableProviders(providers) + val callback = mockCallback() + locationTracker.addCallback(callback) + locationTracker.start() + + val networkLocation = mockLocation( + provider = LocationManager.NETWORK_PROVIDER, + latitude = 1.0, + longitude = 3.0, + accuracy = 4f + ) + locationTracker.onLocationChanged(networkLocation) + + val expectedLocationData = LocationData( + latitude = 1.0, + longitude = 3.0, + uncertainty = 4.0 + ) + verify { callback.onLocationUpdate(expectedLocationData) } + verify { anyConstructed().debounce(any(), MIN_TIME_TO_UPDATE_LOCATION_MILLIS, any()) } + locationTracker.hasLocationFromFusedProvider shouldBeEqualTo false + locationTracker.hasLocationFromGPSProvider shouldBeEqualTo false + } + + @Test + fun `when requesting the last location then last location is notified via callback`() { + val providers = listOf(LocationManager.GPS_PROVIDER) + fakeLocationManager.givenActiveProviders(providers) + val lastLocation = mockLocation(provider = LocationManager.GPS_PROVIDER) + fakeLocationManager.givenLastLocationForProvider(provider = LocationManager.GPS_PROVIDER, location = lastLocation) + fakeLocationManager.givenRequestUpdatesForProvider(provider = LocationManager.GPS_PROVIDER, listener = locationTracker) + val callback = mockCallback() + locationTracker.addCallback(callback) + locationTracker.start() + + locationTracker.requestLastKnownLocation() + + val expectedLocationData = LocationData( + latitude = A_LATITUDE, + longitude = A_LONGITUDE, + uncertainty = AN_ACCURACY.toDouble() + ) + verify { callback.onLocationUpdate(expectedLocationData) } + } + + @Test + fun `when stopping then location updates are stopped and callbacks are cleared`() { + locationTracker.stop() + + verify { fakeLocationManager.instance.removeUpdates(locationTracker) } + verify { anyConstructed().cancelAll() } + locationTracker.callbacks.isEmpty() shouldBeEqualTo true + locationTracker.hasLocationFromGPSProvider shouldBeEqualTo false + locationTracker.hasLocationFromFusedProvider shouldBeEqualTo false + } + + private fun mockAvailableProviders(providers: List) { + fakeLocationManager.givenActiveProviders(providers) + providers.forEach { provider -> + fakeLocationManager.givenLastLocationForProvider(provider = provider, location = null) + fakeLocationManager.givenRequestUpdatesForProvider(provider = provider, listener = locationTracker) + } + } + + private fun mockCallback(): LocationTracker.Callback { + return mockk().also { + every { it.onNoLocationProviderAvailable() } just runs + every { it.onLocationUpdate(any()) } just runs + } + } + + private fun mockLocation( + provider: String, + latitude: Double = A_LATITUDE, + longitude: Double = A_LONGITUDE, + accuracy: Float = AN_ACCURACY + ): Location { + return mockk().also { + every { it.time } returns 123 + every { it.latitude } returns latitude + every { it.longitude } returns longitude + every { it.accuracy } returns accuracy + every { it.provider } returns provider + } + } +} diff --git a/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt b/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt index eb491c9e0c..226f6458de 100644 --- a/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt +++ b/vector/src/test/java/im/vector/app/test/fakes/FakeContext.kt @@ -56,7 +56,7 @@ class FakeContext( givenService(Context.CONNECTIVITY_SERVICE, ConnectivityManager::class.java, connectivityManager.instance) } - private fun givenService(name: String, klass: Class, service: T) { + fun givenService(name: String, klass: Class, service: T) { every { instance.getSystemService(name) } returns service every { instance.getSystemService(klass) } returns service } diff --git a/vector/src/test/java/im/vector/app/test/fakes/FakeHandler.kt b/vector/src/test/java/im/vector/app/test/fakes/FakeHandler.kt new file mode 100644 index 0000000000..11340946ec --- /dev/null +++ b/vector/src/test/java/im/vector/app/test/fakes/FakeHandler.kt @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.test.fakes + +import android.os.Handler +import io.mockk.mockk + +class FakeHandler { + + val instance = mockk() +} diff --git a/vector/src/test/java/im/vector/app/test/fakes/FakeLocationManager.kt b/vector/src/test/java/im/vector/app/test/fakes/FakeLocationManager.kt new file mode 100644 index 0000000000..30c30e6b4a --- /dev/null +++ b/vector/src/test/java/im/vector/app/test/fakes/FakeLocationManager.kt @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.test.fakes + +import android.location.Location +import android.location.LocationListener +import android.location.LocationManager +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import io.mockk.runs + +class FakeLocationManager { + val instance = mockk() + + fun givenActiveProviders(providers: List) { + every { instance.allProviders } returns providers + } + + fun givenRequestUpdatesForProvider( + provider: String, + listener: LocationListener + ) { + every { instance.requestLocationUpdates(provider, any(), any(), listener) } just runs + } + + fun givenRemoveUpdates(listener: LocationListener) { + every { instance.removeUpdates(listener) } just runs + } + + fun givenLastLocationForProvider(provider: String, location: Location?) { + every { instance.getLastKnownLocation(provider) } returns location + } +}