Improve performance of setting puck style properties (#1716) (#1720)

* Improve performance of setting puck style properties * fix test * fix more tests
mapbox · May 17, 2023 · f7db35d · f7db35d
1 parent c88aee5
commit f7db35d
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 62 deletions.
diff --git a/...ncomponent/src/main/java/com/mapbox/maps/plugin/locationcomponent/LocationLayerWrapper.kt b/...ncomponent/src/main/java/com/mapbox/maps/plugin/locationcomponent/LocationLayerWrapper.kt
@@ -22,14 +22,10 @@ internal open class LocationLayerWrapper(val layerId: String) {
 
   protected fun updateProperty(propertyName: String, value: Value) {
     layerProperties[propertyName] = value
-    style?.let { styleDelegate ->
-      if (styleDelegate.styleLayerExists(layerId)) {
-        val expected = styleDelegate.setStyleLayerProperty(layerId, propertyName, value)
-        expected.error?.let {
-          throw MapboxLocationComponentException("Set layer property \"${propertyName}\" failed:\n$it\n$value")
-        }
-      } else {
-        logW(TAG, "Skip updating layer property $propertyName, layer $layerId not ready yet.")
+    style?.let { style ->
+      val expected = style.setStyleLayerProperty(layerId, propertyName, value)
+      expected.error?.let {
+        logE(TAG, "Set layer property \"${propertyName}\" failed:\nError: $it\nValue set: $value")
       }
     }
   }

diff --git a/...ioncomponent/src/main/java/com/mapbox/maps/plugin/locationcomponent/ModelSourceWrapper.kt b/...ioncomponent/src/main/java/com/mapbox/maps/plugin/locationcomponent/ModelSourceWrapper.kt
@@ -4,7 +4,7 @@ import android.util.Log
 import com.mapbox.bindgen.Value
 import com.mapbox.maps.MapboxLocationComponentException
 import com.mapbox.maps.Style
-import com.mapbox.maps.logW
+import com.mapbox.maps.logE
 
 internal class ModelSourceWrapper(
   val sourceId: String,
@@ -55,21 +55,14 @@ internal class ModelSourceWrapper(
 
   private fun updateProperty(propertyName: String, value: Value) {
     sourceProperties[propertyName] = value
-    style?.let { styleDelegate ->
-      if (styleDelegate.styleSourceExists(sourceId)) {
-        val expected = styleDelegate.setStyleSourceProperty(
-          sourceId,
-          propertyName,
-          value
-        )
-        expected.error?.let {
-          throw MapboxLocationComponentException("Set source property \"${propertyName}\" failed:\nError: $it\nValue set: $value")
-        }
-      } else {
-        logW(
-          TAG,
-          "Skip updating source property $propertyName, source $sourceId not ready yet."
-        )
+    style?.let { style ->
+      val expected = style.setStyleSourceProperty(
+        sourceId,
+        propertyName,
+        value
+      )
+      expected.error?.let {
+        logE(TAG, "Set source property \"${propertyName}\" failed:\nError: $it\nValue set: $value")
       }
     }
   }

diff --git a/...src/test/java/com/mapbox/maps/plugin/locationcomponent/LocationComponentPluginImplTest.kt b/...src/test/java/com/mapbox/maps/plugin/locationcomponent/LocationComponentPluginImplTest.kt
@@ -373,6 +373,7 @@ class LocationComponentPluginImplTest {
   fun testLocationProviderRegisterDisableEnable() {
     every { logE(any(), any()) } just Runs
     every { style.addPersistentStyleLayer(any(), any()) } returns ExpectedFactory.createNone()
+    every { style.setStyleLayerProperty(any(), any(), any()) } returns ExpectedFactory.createNone()
 
     preparePluginInitialisationWithEnabled()
 

diff --git a/...c/test/java/com/mapbox/maps/plugin/locationcomponent/LocationIndicatorLayerWrapperTest.kt b/...c/test/java/com/mapbox/maps/plugin/locationcomponent/LocationIndicatorLayerWrapperTest.kt
@@ -1,10 +1,11 @@
 package com.mapbox.maps.plugin.locationcomponent
 
 import com.mapbox.bindgen.Expected
+import com.mapbox.bindgen.ExpectedFactory
 import com.mapbox.bindgen.None
 import com.mapbox.bindgen.Value
 import com.mapbox.maps.Style
-import com.mapbox.maps.logW
+import com.mapbox.maps.logE
 import io.mockk.*
 import org.junit.After
 import org.junit.Assert.assertEquals
@@ -23,8 +24,7 @@ class LocationIndicatorLayerWrapperTest {
   @Before
   fun setup() {
     mockkStatic("com.mapbox.maps.MapboxLogger")
-    every { logW(any(), any()) } just Runs
-    every { style.styleLayerExists(any()) } returns true
+    every { logE(any(), any()) } just Runs
     every { style.addPersistentStyleLayer(any(), any()) } returns expected
     every { style.setStyleLayerProperty(any(), any(), any()) } returns expected
     every { expected.error } returns null
@@ -147,16 +147,16 @@ class LocationIndicatorLayerWrapperTest {
 
   @Test
   fun testLayerNotReady() {
-    every { style.styleLayerExists(any()) } returns false
+    every { style.setStyleLayerProperty(any(), any(), any()) } returns ExpectedFactory.createError("error")
     val bearing = 1.0
     layer.bearing(bearing)
-    verify(exactly = 0) { style.setStyleLayerProperty(INDICATOR_LAYER_ID, "bearing", Value(bearing)) }
+    verify(exactly = 1) { style.setStyleLayerProperty(INDICATOR_LAYER_ID, "bearing", Value(bearing)) }
+    verify(exactly = 1) { logE(any(), any()) }
   }
 
   @Test
   fun testUpdateStyle() {
     val newStyle = mockk<Style>(relaxed = true)
-    every { newStyle.styleLayerExists(any()) } returns true
     every { newStyle.setStyleLayerProperty(any(), any(), any()) } returns expected
     layer.updateStyle(newStyle)
     val radius = 1.0

diff --git a/...ponent/src/test/java/com/mapbox/maps/plugin/locationcomponent/LocationLayerWrapperTest.kt b/...ponent/src/test/java/com/mapbox/maps/plugin/locationcomponent/LocationLayerWrapperTest.kt
@@ -4,14 +4,13 @@ import com.mapbox.bindgen.ExpectedFactory
 import com.mapbox.bindgen.Value
 import com.mapbox.maps.LayerPosition
 import com.mapbox.maps.Style
-import com.mapbox.maps.logW
+import com.mapbox.maps.logE
 import io.mockk.*
 import org.junit.After
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.robolectric.RobolectricTestRunner
-import java.lang.RuntimeException
 
 @RunWith(RobolectricTestRunner::class)
 class LocationLayerWrapperTest {
@@ -22,7 +21,7 @@ class LocationLayerWrapperTest {
   @Before
   fun setup() {
     mockkStatic("com.mapbox.maps.MapboxLogger")
-    every { logW(any(), any()) } just Runs
+    every { logE(any(), any()) } just Runs
     locationLayerWrapper = LocationLayerWrapper(layerId)
   }
 
@@ -51,45 +50,30 @@ class LocationLayerWrapperTest {
   fun testVisibilityWhenLayerExists() {
     every { mapStyleDelegate.addPersistentStyleLayer(any(), any()) } returns ExpectedFactory.createNone()
     every { mapStyleDelegate.setStyleLayerProperty(layerId, any(), any()) } returns ExpectedFactory.createNone()
-    every { mapStyleDelegate.styleLayerExists(any()) } returns true
     val position = LayerPosition("above", "below", 0)
     locationLayerWrapper.bindTo(mapStyleDelegate, position)
     locationLayerWrapper.visibility(true)
     verify { mapStyleDelegate.setStyleLayerProperty(layerId, "visibility", Value("visible")) }
   }
 
-  @Test
-  fun testVisibilityWhenLayerNotExist() {
-    every { mapStyleDelegate.addPersistentStyleLayer(any(), any()) } returns ExpectedFactory.createNone()
-    every { mapStyleDelegate.setStyleLayerProperty(layerId, any(), any()) } returns ExpectedFactory.createNone()
-    every { mapStyleDelegate.styleLayerExists(any()) } returns false
-    val position = LayerPosition("above", "below", 0)
-    locationLayerWrapper.bindTo(mapStyleDelegate, position)
-    locationLayerWrapper.visibility(true)
-    verify(exactly = 0) { mapStyleDelegate.setStyleLayerProperty(layerId, "visibility", Value("visible")) }
-  }
-
-  @Test(expected = RuntimeException::class)
   fun testVisibilityWhenLayerExistSetPropertyFailed() {
     every { mapStyleDelegate.addPersistentStyleLayer(any(), any()) } returns ExpectedFactory.createNone()
     every { mapStyleDelegate.setStyleLayerProperty(layerId, any(), any()) } returns ExpectedFactory.createError("error")
-    every { mapStyleDelegate.styleLayerExists(any()) } returns true
     val position = LayerPosition("above", "below", 0)
     locationLayerWrapper.bindTo(mapStyleDelegate, position)
     locationLayerWrapper.visibility(true)
     verify(exactly = 1) { mapStyleDelegate.setStyleLayerProperty(layerId, "visibility", Value("visible")) }
+    verify(exactly = 1) { logE(any(), any()) }
   }
 
   @Test
   fun updateStyle() {
     every { mapStyleDelegate.addPersistentStyleLayer(any(), any()) } returns ExpectedFactory.createNone()
     every { mapStyleDelegate.setStyleLayerProperty(layerId, any(), any()) } returns ExpectedFactory.createNone()
-    every { mapStyleDelegate.styleLayerExists(any()) } returns true
     val position = LayerPosition("above", "below", 0)
     locationLayerWrapper.bindTo(mapStyleDelegate, position)
     val newStyle = mockk<Style>()
     every { newStyle.setStyleLayerProperty(layerId, any(), any()) } returns ExpectedFactory.createNone()
-    every { newStyle.styleLayerExists(any()) } returns true
     locationLayerWrapper.updateStyle(newStyle)
     locationLayerWrapper.visibility(true)
     verify(exactly = 0) { mapStyleDelegate.setStyleLayerProperty(layerId, "visibility", Value("visible")) }

diff --git a/...omponent/src/test/java/com/mapbox/maps/plugin/locationcomponent/ModelSourceWrapperTest.kt b/...omponent/src/test/java/com/mapbox/maps/plugin/locationcomponent/ModelSourceWrapperTest.kt
@@ -1,10 +1,11 @@
 package com.mapbox.maps.plugin.locationcomponent
 
 import com.mapbox.bindgen.Expected
+import com.mapbox.bindgen.ExpectedFactory
 import com.mapbox.bindgen.None
 import com.mapbox.bindgen.Value
 import com.mapbox.maps.Style
-import com.mapbox.maps.logW
+import com.mapbox.maps.logE
 import io.mockk.*
 import org.junit.After
 import org.junit.Assert.assertEquals
@@ -22,8 +23,7 @@ class ModelSourceWrapperTest {
   @Before
   fun setup() {
     mockkStatic("com.mapbox.maps.MapboxLogger")
-    every { logW(any(), any()) } just Runs
-    every { style.styleSourceExists(any()) } returns true
+    every { logE(any(), any()) } just Runs
     every { style.addStyleSource(any(), any()) } returns expected
     every { style.setStyleSourceProperty(any(), any(), any()) } returns expected
     every { expected.error } returns null
@@ -59,9 +59,9 @@ class ModelSourceWrapperTest {
   }
 
   @Test
-  fun testSourceNotReady() {
-    every { style.styleSourceExists(any()) } returns false
+  fun testSetPropertyFailed() {
     val modelSource = ModelSourceWrapper(SOURCE_ID, "uri", listOf(1.0, 2.0))
+    every { style.setStyleSourceProperty(any(), any(), any()) } returns ExpectedFactory.createError("error")
     modelSource.bindTo(style)
 
     val position = arrayListOf(5.0, 5.0)
@@ -73,15 +73,15 @@ class ModelSourceWrapperTest {
       Pair(ModelSourceWrapper.URL, Value("uri"))
     )
     val updateModel = hashMapOf(Pair(ModelSourceWrapper.DEFAULT_MODEL_NAME, Value(property)))
-    verify(exactly = 0) { style.setStyleSourceProperty(SOURCE_ID, ModelSourceWrapper.MODELS, Value(updateModel)) }
+    verify(exactly = 1) { style.setStyleSourceProperty(SOURCE_ID, ModelSourceWrapper.MODELS, Value(updateModel)) }
+    verify(exactly = 1) { logE(any(), any()) }
   }
 
   @Test
   fun testUpdateStyle() {
     val modelSource = ModelSourceWrapper(SOURCE_ID, "uri", listOf(1.0, 2.0))
     modelSource.bindTo(style)
     val newStyle = mockk<Style>()
-    every { newStyle.styleSourceExists(any()) } returns true
     every { newStyle.addStyleSource(any(), any()) } returns expected
     every { newStyle.setStyleSourceProperty(any(), any(), any()) } returns expected
     modelSource.updateStyle(newStyle)

diff --git a/...ent/src/testPublic/java/com/mapbox/maps/plugin/locationcomponent/ModelLayerWrapperTest.kt b/...ent/src/testPublic/java/com/mapbox/maps/plugin/locationcomponent/ModelLayerWrapperTest.kt
@@ -1,10 +1,11 @@
 package com.mapbox.maps.plugin.locationcomponent
 
 import com.mapbox.bindgen.Expected
+import com.mapbox.bindgen.ExpectedFactory
 import com.mapbox.bindgen.None
 import com.mapbox.bindgen.Value
 import com.mapbox.maps.Style
-import com.mapbox.maps.logW
+import com.mapbox.maps.logE
 import io.mockk.*
 import org.junit.After
 import org.junit.Assert.assertEquals
@@ -30,8 +31,7 @@ class ModelLayerWrapperTest {
   @Before
   fun setup() {
     mockkStatic("com.mapbox.maps.MapboxLogger")
-    every { logW(any(), any()) } just Runs
-    every { style.styleLayerExists(any()) } returns true
+    every { logE(any(), any()) } just Runs
     every { style.addPersistentStyleLayer(any(), any()) } returns expected
     every { style.setStyleLayerProperty(any(), any(), any()) } returns expected
     every { expected.error } returns null
@@ -97,22 +97,22 @@ class ModelLayerWrapperTest {
 
   @Test
   fun testLayerNotReady() {
-    every { style.styleLayerExists(any()) } returns false
+    every { style.setStyleLayerProperty(any(), any(), any()) } returns ExpectedFactory.createError("error")
     val scale = arrayListOf(1.0, 2.0)
     layer.modelScale(scale)
-    verify(exactly = 0) {
+    verify(exactly = 1) {
       style.setStyleLayerProperty(
         MODEL_LAYER_ID,
         "model-scale",
         Value(scale.map(::Value))
       )
     }
+    verify(exactly = 1) { logE(any(), any()) }
   }
 
   @Test
   fun testUpdateStyle() {
     val newStyle = mockk<Style>(relaxed = true)
-    every { newStyle.styleLayerExists(any()) } returns true
     every { newStyle.setStyleLayerProperty(any(), any(), any()) } returns expected
     layer.updateStyle(newStyle)
     val scale = listOf(1.0, 2.0, 3.0)