[android] do not invoke #onCancel when animation is scheduled from #onFinish blockupstream/lp-13735

3 files changed, 115 insertions, 24 deletions

diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java
index 9356b112ba..c40994d7ca 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java
@@ -5,6 +5,7 @@ import android.os.Handler;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.support.annotation.UiThread;
+
 import com.mapbox.mapboxsdk.camera.CameraPosition;
 import com.mapbox.mapboxsdk.camera.CameraUpdate;
 import com.mapbox.mapboxsdk.camera.CameraUpdateFactory;
@@ -77,13 +78,15 @@ final class Transform implements MapView.OnCameraDidChangeListener {
     if (animated) {
       invalidateCameraPosition();
       if (cameraCancelableCallback != null) {
+        final MapboxMap.CancelableCallback callback = cameraCancelableCallback;
+
+        // nullification has to happen before Handler#post, see https://github.com/robolectric/robolectric/issues/1306
+        cameraCancelableCallback = null;
+
         handler.post(new Runnable() {
           @Override
           public void run() {
-            if (cameraCancelableCallback != null) {
-              cameraCancelableCallback.onFinish();
-              cameraCancelableCallback = null;
-            }
+            callback.onFinish();
           }
         });
       }
@@ -173,13 +176,16 @@ final class Transform implements MapView.OnCameraDidChangeListener {
     if (cameraCancelableCallback != null) {
       final MapboxMap.CancelableCallback callback = cameraCancelableCallback;
       cameraChangeDispatcher.onCameraIdle();
+
+      // nullification has to happen before Handler#post, see https://github.com/robolectric/robolectric/issues/1306
+      cameraCancelableCallback = null;
+
       handler.post(new Runnable() {
         @Override
         public void run() {
           callback.onCancel();
         }
       });
-      cameraCancelableCallback = null;
     }
 
     // cancel ongoing transitions
diff --git a/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.kt b/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.kt
index 4c67049b39..1c0d1f76b7 100644
--- a/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.kt
+++ b/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/maps/MapboxMapTest.kt
@@ -22,23 +22,23 @@ class MapboxMapTest {
 
     private lateinit var nativeMapView: NativeMapView
 
+    private lateinit var transform: Transform
+
     @Before
     fun setup() {
         val cameraChangeDispatcher = spyk<CameraChangeDispatcher>()
-        val mapView = mockk<MapView>()
         nativeMapView = mockk()
-        mapboxMap = MapboxMap(nativeMapView, Transform(mapView, nativeMapView, cameraChangeDispatcher), null, null, null, cameraChangeDispatcher)
+        transform = mockk()
+        mapboxMap = MapboxMap(nativeMapView, transform, null, null, null, cameraChangeDispatcher)
         every { nativeMapView.styleUrl = any() } answers {}
         every { nativeMapView.transitionOptions = any() } answers {}
         every { nativeMapView.isDestroyed } returns false
-        every { nativeMapView.cameraPosition } returns CameraPosition.DEFAULT
-        every { nativeMapView.cancelTransitions() } answers {}
-        every { nativeMapView.jumpTo(any(), any(), any(), any()) } answers {}
-        every { nativeMapView.minZoom = any() } answers {}
-        every { nativeMapView.maxZoom = any() } answers {}
         every { nativeMapView.setOnFpsChangedListener(any()) } answers {}
         every { nativeMapView.prefetchTiles = any() } answers {}
         every { nativeMapView.setLatLngBounds(any()) } answers {}
+        every { transform.minZoom = any() } answers {}
+        every { transform.maxZoom = any() } answers {}
+        every { transform.moveCamera(any(), any(), any()) } answers {}
         mapboxMap.injectLocationComponent(spyk())
         mapboxMap.setStyle(Style.MAPBOX_STREETS)
         mapboxMap.onFinishLoadingStyle()
@@ -51,27 +51,26 @@ class MapboxMapTest {
         verify { nativeMapView.transitionOptions = expected }
     }
 
-    @Test
-    fun testMoveCamera() {
-        val callback = mockk<MapboxMap.CancelableCallback>()
-        every { callback.onFinish() } answers {}
-        val target = LatLng(1.0, 2.0)
-        val expected = CameraPosition.Builder().target(target).build()
-        mapboxMap.moveCamera(CameraUpdateFactory.newCameraPosition(expected), callback)
-        verify { nativeMapView.jumpTo(target, -1.0, -1.0, -1.0) }
-        verify { callback.onFinish() }
-    }
+  @Test
+  fun testMoveCamera() {
+    val callback = mockk<MapboxMap.CancelableCallback>()
+    val target = LatLng(1.0, 2.0)
+    val expected = CameraPosition.Builder().target(target).build()
+    val update = CameraUpdateFactory.newCameraPosition(expected)
+    mapboxMap.moveCamera(update, callback)
+    verify { transform.moveCamera(mapboxMap, update, callback) }
+  }
 
     @Test
     fun testMinZoom() {
         mapboxMap.setMinZoomPreference(10.0)
-        verify { nativeMapView.minZoom = 10.0 }
+        verify { transform.minZoom = 10.0 }
     }
 
     @Test
     fun testMaxZoom() {
         mapboxMap.setMaxZoomPreference(10.0)
-        verify { nativeMapView.maxZoom = 10.0 }
+        verify { transform.maxZoom = 10.0 }
     }
 
     @Test
diff --git a/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/maps/TransformTest.kt b/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/maps/TransformTest.kt
new file mode 100644
index 0000000000..8c985ca3a8
--- /dev/null
+++ b/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/maps/TransformTest.kt
@@ -0,0 +1,86 @@
+package com.mapbox.mapboxsdk.maps
+
+import com.mapbox.mapboxsdk.camera.CameraPosition
+import com.mapbox.mapboxsdk.camera.CameraUpdateFactory
+import com.mapbox.mapboxsdk.geometry.LatLng
+import io.mockk.*
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+
+@RunWith(RobolectricTestRunner::class)
+class TransformTest {
+
+  private lateinit var mapView: MapView
+  private lateinit var nativeMapView: NativeMapView
+  private lateinit var transform: Transform
+
+  @Before
+  fun setup() {
+    val cameraChangeDispatcher = spyk<CameraChangeDispatcher>()
+    mapView = mockk()
+    nativeMapView = mockk()
+    transform = Transform(mapView, nativeMapView, cameraChangeDispatcher)
+    every { nativeMapView.isDestroyed } returns false
+    every { nativeMapView.cameraPosition } returns CameraPosition.DEFAULT
+    every { nativeMapView.cancelTransitions() } answers {}
+    every { nativeMapView.jumpTo(any(), any(), any(), any()) } answers {}
+    every { nativeMapView.flyTo(any(), any(), any(), any(), any()) } answers {}
+    every { nativeMapView.minZoom = any() } answers {}
+    every { nativeMapView.maxZoom = any() } answers {}
+  }
+
+  @Test
+  fun testMoveCamera() {
+    val mapboxMap = mockk<MapboxMap>()
+    every { mapboxMap.cameraPosition } answers { CameraPosition.DEFAULT }
+
+    val callback = mockk<MapboxMap.CancelableCallback>()
+    every { callback.onFinish() } answers {}
+
+    val target = LatLng(1.0, 2.0)
+    val expected = CameraPosition.Builder().target(target).build()
+    val update = CameraUpdateFactory.newCameraPosition(expected)
+    transform.moveCamera(mapboxMap, update, callback)
+
+    verify { nativeMapView.jumpTo(target, -1.0, -1.0, -1.0) }
+    verify { callback.onFinish() }
+  }
+
+  @Test
+  fun testMinZoom() {
+    transform.minZoom = 10.0
+    verify { nativeMapView.minZoom = 10.0 }
+  }
+
+  @Test
+  fun testMaxZoom() {
+    transform.maxZoom = 10.0
+    verify { nativeMapView.maxZoom = 10.0 }
+  }
+
+  @Test
+  fun testCancelNotInvokedFromOnFinish() {
+    val slot = slot<MapView.OnCameraDidChangeListener>()
+    every { mapView.addOnCameraDidChangeListener(capture(slot)) } answers { slot.captured.onCameraDidChange(true) }
+    every { mapView.removeOnCameraDidChangeListener(any()) } answers {}
+    // regression test for https://github.com/mapbox/mapbox-gl-native/issues/13735
+    val mapboxMap = mockk<MapboxMap>()
+    every { mapboxMap.cameraPosition } answers { CameraPosition.DEFAULT }
+
+    val target = LatLng(1.0, 2.0)
+    val expected = CameraPosition.Builder().target(target).build()
+
+    val callback = object : MapboxMap.CancelableCallback {
+      override fun onCancel() {
+        throw IllegalStateException("onCancel shouldn't be called from onFinish")
+      }
+
+      override fun onFinish() {
+        transform.animateCamera(mapboxMap, CameraUpdateFactory.newCameraPosition(expected), 500, null)
+      }
+    }
+    transform.animateCamera(mapboxMap, CameraUpdateFactory.newCameraPosition(expected), 500, callback)
+  }
+}
\ No newline at end of file