LocationComponentPluginImpl allows duplicate subscriptions

[kmadsen]

https://github.com/mapbox/mapbox-maps-android/blob/3c4e0e55a1fb49a5ce2932afa00ba723f67dce7d/plugin-locationcomponent/src/main/java/com/mapbox/maps/plugin/locationcomponent/LocationComponentPluginImpl.kt#L41-L45

  override fun addOnIndicatorPositionChangedListener(listener: OnIndicatorPositionChangedListener) {
    onIndicatorPositionChangedListener.add(listener)
  }

From a caller perspective, sometimes there's no way of knowing if you've already added a listener without a big architecture change or adding complexity.

Will you make these CopyOnWriteArraySet, so we don't add the same listener multiple times?

[kmadsen]

The specific case that brought me here: With android auto there are a few unique hooks into lifecycle events. A developer may also want to hook up buttons to allow users to enable/disable the locations

I noticed that the callback was continuing to fire, even though the surface has been deleted.

        lifecycle.addObserver(object : DefaultLifecycleObserver {
            override fun onCreate(owner: LifecycleOwner) {
                carContext.getCarService(AppManager::class.java)
                    .setSurfaceCallback(surfaceCallback)
            }

            override fun onStart(owner: LifecycleOwner) {
                // this would cause duplicate subscriptions
                // mapSurface?.location?.addOnIndicatorPositionChangedListener(positionChangedListener)
            }

            override fun onStop(owner: LifecycleOwner) {
                // this will only remove one of the duplicates
                // mapSurface?.location?.removeOnIndicatorPositionChangedListener(positionChangedListener)
            }

            override fun onDestroy(owner: LifecycleOwner) {
                log("onDestroy")
                mapSurface?.location?.removeOnIndicatorPositionChangedListener(positionChangedListener)
                mapSurface?.onDestroy() // << == unfair to assume this would clean up subscriptions?
                mapSurface = null
            }
        })
    }

    private val surfaceCallback: SurfaceCallback = object : SurfaceCallback {
        override fun onSurfaceAvailable(surfaceContainer: SurfaceContainer) {
            ....
            // Here's where we are first able to add the change listener
            surface.location.addOnIndicatorPositionChangedListener(positionChangedListener)

Work around

For Android Auto, hooking up the lifecycle to the surface fixes an issue. But not ideal :)

override fun onSurfaceDestroyed(surfaceContainer: SurfaceContainer) {
            log("Surface destroyed")
            mapSurface?.location?.removeOnIndicatorPositionChangedListener(positionChangedListener)
            mapSurface?.surfaceDestroyed()
        }

[kmadsen]

cc: @pengdev

[pengdev]

This is resolved as a part of https://github.com/mapbox/mapbox-maps-android/pull/1016 and will be included in the v10.4.0-beta.1 release.