Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class LocationComponentManager(mapView: RNMBXMapView, context: Context) {

private fun applyStateChanges(map: RNMBXMapView, oldState: State, newState: State, fullUpdate: Boolean) {
val mapView = map.mapPlugins
if (map.getLifecycleState() != Lifecycle.State.STARTED && map.getLifecycleState() != Lifecycle.State.INITIALIZED) {
if (!isRunning(map)) {
// In case lifecycle was already stopped, so we're part of shutdown, do not call updateSettings as it'll just restart
// the loationComponent that will not be stopped. See https://github.com/mapbox/mapbox-maps-android/issues/2017
if (!newState.enabled) {
Expand Down Expand Up @@ -223,6 +223,18 @@ class LocationComponentManager(mapView: RNMBXMapView, context: Context) {
update { it }
}

private fun isRunning(map: RNMBXMapView) =
map.getLifecycleState() == Lifecycle.State.STARTED || map.getLifecycleState() == Lifecycle.State.INITIALIZED

// Lets RNMBXNativeUserLocation rebuild a puck the Maps SDK can no longer add.
internal val puckGuardComponent = LocationComponentReset(
isRequested = { mState.enabled },
isRunning = { isRunning(mMapView) },
isEnabled = { mMapView.mapPlugins.location.enabled },
setEnabled = { mMapView.mapPlugins.location.enabled = it },
holdProvider = ::holdLocationProviderBriefly,
)

// All maps share one Mapbox device location provider, and restarting it before its stop completes leaks a
// fusedLocationClientHandlerThread (#4225), so stay subscribed briefly in case another map takes over in the same frame.
private fun holdLocationProviderBriefly() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package com.rnmapbox.rnmbx.components.location

/**
* Applies location puck settings, recovering when the Maps SDK fails to re-add the puck layer.
*
* The location component keeps the last puck bearing it received, including NaN or Infinity.
* The next time the puck layer is added, such as when the puck settings change,
* `addPersistentStyleLayer` throws `IllegalArgumentException: Parse error at offset 165: Missing
* a colon after a name of object member.` See https://github.com/mapbox/mapbox-maps-android/issues/2340
*
* Resetting the location component drops the puck state that holds the bad bearing, so the guard
* resets it and applies once more. If the map is stopped, or the retry fails because the location
* provider replays the same bearing, the component stays disabled and the next [apply] resets it.
*/
internal class LocationPuckGuard {
interface Component {
/** Whether the location component should be shown and the map is running. */
val canReset: Boolean

/** Disables and re-enables the location component. */
fun reset()

fun disable()
}

/** True while this guard keeps the location component disabled after a failure. */
var disabledAfterError = false
private set

/** @return the failure that was recovered from, or null when [apply] succeeded the first time. */
fun apply(component: Component, apply: () -> Unit): IllegalArgumentException? {
val resetting = disabledAfterError && component.canReset
try {
if (resetting) {
disabledAfterError = false
component.reset()
}
apply()
return null
} catch (error: IllegalArgumentException) {
if (!error.isAddLayerFailure()) throw error
component.disable()
disabledAfterError = true
if (!resetting && component.canReset) {
try {
component.reset()
apply()
disabledAfterError = false
} catch (retryError: IllegalArgumentException) {
if (!retryError.isAddLayerFailure()) throw retryError
component.disable()
error.addSuppressed(retryError)
}
}
return error
}
}
}

/**
* Resets the location component for [LocationPuckGuard] without restarting the location provider.
*
* Giving up location that is on holds the provider briefly, so re-enabling does not restart it
* before its stop completes (#4225). Location that is already off, or on a stopped map, is not held:
* holding subscribes to the provider, which would start location collection.
*/
internal class LocationComponentReset(
private val isRequested: () -> Boolean,
private val isRunning: () -> Boolean,
private val isEnabled: () -> Boolean,
private val setEnabled: (Boolean) -> Unit,
private val holdProvider: () -> Unit,
) : LocationPuckGuard.Component {
override val canReset: Boolean
get() = isRequested() && isRunning()

override fun reset() {
disable()
setEnabled(true)
}

override fun disable() {
if (isEnabled() && isRunning()) {
holdProvider()
}
setEnabled(false)
}
}

private fun IllegalArgumentException.isAddLayerFailure() =
stackTrace.any { it.methodName == "addPersistentStyleLayer" }
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class RNMBXNativeUserLocation(context: Context) : AbstractMapFeature(context), O
private var mContext : Context = context

private var imageManager: ImageManager? = null
private val puckGuard = LocationPuckGuard()

// region bearing
var androidRenderMode: RenderMode? = null
Expand Down Expand Up @@ -134,6 +135,18 @@ class RNMBXNativeUserLocation(context: Context) : AbstractMapFeature(context), O
}

private fun _apply(mapView: MapView) {
val locationComponent = mMapView?.locationComponentManager?.puckGuardComponent
if (locationComponent == null) {
_applySettings(mapView)
return
}
puckGuard.apply(locationComponent) { _applySettings(mapView) }?.let { error ->
val outcome = if (puckGuard.disabledAfterError) "disabled the location component" else "recreated the puck"
Logger.e(LOG_TAG, "Failed to add the location puck, $outcome: ${error.message}")
}
}

private fun _applySettings(mapView: MapView) {
val location2 = mapView.location2;

val locationPuck3D = model?.let { makeLocationPuck3D(it) }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
package com.rnmapbox.rnmbx.components.location

import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Assertions.assertFalse
import org.junit.jupiter.api.Assertions.assertNull
import org.junit.jupiter.api.Assertions.assertSame
import org.junit.jupiter.api.Assertions.assertTrue
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertThrows

class LocationPuckGuardTest {
private class FakeComponent(override var canReset: Boolean = true) : LocationPuckGuard.Component {
val calls = mutableListOf<String>()
override fun reset() {
calls += "reset"
}

override fun disable() {
calls += "disable"
}
}

// The Maps SDK's failure when a non-finite puck bearing reaches addPersistentStyleLayer.
private fun addLayerError() = IllegalArgumentException(
"Parse error at offset 165: Missing a colon after a name of object member."
).apply {
stackTrace = arrayOf(
StackTraceElement("com.mapbox.maps.StyleManager", "addPersistentStyleLayer", "StyleManager.java", -2),
)
}

@Test
fun `leaves the location component alone when the puck applies`() {
val component = FakeComponent()

val error = LocationPuckGuard().apply(component) {}

assertNull(error)
assertEquals(emptyList<String>(), component.calls)
}

@Test
fun `resets the location component and applies again after the layer cannot be added`() {
val component = FakeComponent()
val guard = LocationPuckGuard()
val failure = addLayerError()
var applies = 0

val error = guard.apply(component) {
applies++
if (applies == 1) throw failure
}

assertSame(failure, error)
assertEquals(2, applies)
assertEquals(listOf("disable", "reset"), component.calls)
assertFalse(guard.disabledAfterError)
}

@Test
fun `leaves the location component disabled when the retry also fails`() {
val component = FakeComponent()
val guard = LocationPuckGuard()
var applies = 0

val error = guard.apply(component) {
applies++
throw addLayerError()
}

assertEquals(2, applies)
assertEquals(1, error?.suppressed?.size)
assertEquals(listOf("disable", "reset", "disable"), component.calls)
assertTrue(guard.disabledAfterError)
}

@Test
fun `resets the location component on the next apply`() {
val component = FakeComponent()
val guard = LocationPuckGuard()
guard.apply(component) { throw addLayerError() }
component.calls.clear()

val error = guard.apply(component) {}

assertNull(error)
assertEquals(listOf("reset"), component.calls)
assertFalse(guard.disabledAfterError)
}

@Test
fun `disables without a second retry when the next reset fails again`() {
val component = FakeComponent()
val guard = LocationPuckGuard()
guard.apply(component) { throw addLayerError() }
component.calls.clear()
var applies = 0

guard.apply(component) {
applies++
throw addLayerError()
}

assertEquals(1, applies)
assertEquals(listOf("reset", "disable"), component.calls)
assertTrue(guard.disabledAfterError)
}

@Test
fun `does not reset the location component while the map is stopped`() {
val component = FakeComponent(canReset = false)
val guard = LocationPuckGuard()
var applies = 0

guard.apply(component) {
applies++
throw addLayerError()
}
guard.apply(component) { applies++ }

assertEquals(2, applies)
assertEquals(listOf("disable"), component.calls)
assertTrue(guard.disabledAfterError)

component.canReset = true
guard.apply(component) {}

assertEquals(listOf("disable", "reset"), component.calls)
assertFalse(guard.disabledAfterError)
}

@Test
fun `does not catch other IllegalArgumentExceptions`() {
val component = FakeComponent()

assertThrows<IllegalArgumentException> {
LocationPuckGuard().apply(component) { throw IllegalArgumentException("unrelated") }
}
assertEquals(emptyList<String>(), component.calls)
}

@Test
fun `does not catch other exceptions`() {
val component = FakeComponent()

assertThrows<IllegalStateException> {
LocationPuckGuard().apply(component) { throw IllegalStateException("unrelated") }
}
assertEquals(emptyList<String>(), component.calls)
}
}

class LocationComponentResetTest {
private class FakeLocation(var enabled: Boolean = true, var running: Boolean = true) {
val calls = mutableListOf<String>()
val reset = LocationComponentReset(
isRequested = { true },
isRunning = { running },
isEnabled = { enabled },
setEnabled = {
calls += "enabled=$it"
enabled = it
},
holdProvider = { calls += "hold" },
)
}

@Test
fun `holds the provider before giving up location on a running map`() {
val location = FakeLocation()

location.reset.disable()

assertEquals(listOf("hold", "enabled=false"), location.calls)
}

@Test
fun `does not hold the provider when location is already off`() {
val location = FakeLocation(enabled = false)

location.reset.disable()

assertEquals(listOf("enabled=false"), location.calls)
}

@Test
fun `does not hold the provider while the map is stopped`() {
val location = FakeLocation(running = false)

location.reset.disable()

assertFalse(location.reset.canReset)
assertEquals(listOf("enabled=false"), location.calls)
}

@Test
fun `reset hands location over through the provider hold`() {
val location = FakeLocation()

location.reset.reset()

assertEquals(listOf("hold", "enabled=false", "enabled=true"), location.calls)
assertTrue(location.enabled)
}

@Test
fun `reset of location that is already off enables it without a hold`() {
val location = FakeLocation(enabled = false)

location.reset.reset()

assertEquals(listOf("enabled=false", "enabled=true"), location.calls)
assertTrue(location.enabled)
}
}