Fix camera animators ordering on Android 6 and lower (#597)

#596: Remove callback order dependency from CameraAnimationsPluginImpl

app/src/main/AndroidManifest.xml

@@ -522,6 +522,19 @@
                 android:name="android.support.PARENT_ACTIVITY"
                 android:value=".ExampleOverviewActivity" />
         </activity>
+
+        <activity
+            android:name=".examples.OngoingAnimationActivity"
+            android:description="@string/description_animate_camera_during_gesture"
+            android:exported="true"
+            android:label="@string/activity_camera_animate_during_gesture">
+            <meta-data
+                android:name="@string/category"
+                android:value="@string/category_camera" />
+            <meta-data
+                android:name="android.support.PARENT_ACTIVITY"
+                android:value=".ExampleOverviewActivity" />
+        </activity>
         <activity
             android:name=".examples.markersandcallouts.MultipleGeometriesActivity"
             android:description="@string/description_dds_multiple_geometries"

app/src/main/java/com/mapbox/maps/testapp/examples/OngoingAnimationActivity.kt

@@ -0,0 +1,54 @@
+package com.mapbox.maps.testapp.examples
+
+import android.animation.ValueAnimator
+import android.os.Bundle
+import android.view.animation.LinearInterpolator
+import androidx.appcompat.app.AppCompatActivity
+import com.mapbox.geojson.Point
+import com.mapbox.maps.CameraOptions
+import com.mapbox.maps.MapView
+import com.mapbox.maps.plugin.animation.CameraAnimatorOptions
+import com.mapbox.maps.plugin.animation.camera
+import com.mapbox.maps.plugin.gestures.gestures
+import java.util.concurrent.TimeUnit
+
+class OngoingAnimationActivity : AppCompatActivity() {
+
+  override fun onCreate(savedInstanceState: Bundle?) {
+    super.onCreate(savedInstanceState)
+    val mapView = MapView(this)
+    setContentView(mapView)
+    mapView.getMapboxMap()
+      .apply {
+        setCamera(
+          CameraOptions.Builder()
+            .center(Point.fromLngLat(LONGITUDE, LATITUDE))
+            .zoom(9.0)
+            .build()
+        )
+      }
+
+    mapView.gestures.addProtectedAnimationOwner(OWNER)
+
+    val anim = mapView.camera.createPitchAnimator(
+      CameraAnimatorOptions.cameraAnimatorOptions(0.0, 30.0) {
+        owner(OWNER)
+      }
+    ) {
+      repeatCount = ValueAnimator.INFINITE
+      repeatMode = ValueAnimator.REVERSE
+      duration = TimeUnit.SECONDS.toMillis(2)
+      interpolator = LinearInterpolator()
+    }
+
+    mapView.camera.registerAnimators(anim)
+
+    anim.start()
+  }
+
+  companion object {
+    private const val OWNER = "Example"
+    private const val LATITUDE = 40.0
+    private const val LONGITUDE = -74.5
+  }
+}

app/src/main/res/values/example_descriptions.xml

@@ -35,6 +35,7 @@
     <string name="activity_camera_animate_description">Animate the camera changes with the fly-to animation. This causes the camera to ease from a starting point to an end destination.</string>
     <string name="activity_camera_predefined_animators_description">Use setCamera to animate the camera position.</string>
     <string name="description_camera_low_level">Animate the map camera to a new position using camera animators. Individual camera properties such as zoom, bearing, and center coordinate can be animated independently.</string>
+    <string name="description_animate_camera_during_gesture">Animate the map camera properties during user gestures.</string>
     <string name="description_fill_extrusion">Use extrusions to display buildings height in 3D map.</string>
     <string name="description_icon_property">Use different images to represent features within a symbol layer based on properties.</string>
     <string name="description_animated_image_source">Use an ImageSource and RasterLayer to animate weather data</string>

app/src/main/res/values/example_titles.xml

@@ -38,6 +38,7 @@
     <string name="activity_animated_image_source">Animated ImageSource</string>
     <string name="activity_camera_predefined_animators_title">Using camera animations</string>
     <string name="activity_camera_low_level">Using custom camera animations</string>
+    <string name="activity_camera_animate_during_gesture">Continue camera animation during gestures</string>
     <string name="activity_dds_multiple_geometries_title">Draw multiple geometries</string>
     <string name="activity_wms_source">WMS Source</string>
     <string name="activity_within">Within expression</string>

plugin-animation/src/main/java/com/mapbox/maps/plugin/animation/CameraAnimationsPluginImpl.kt

@@ -1,8 +1,11 @@
 package com.mapbox.maps.plugin.animation
 
 import android.animation.Animator
+import android.animation.AnimatorListenerAdapter
 import android.animation.AnimatorSet
 import android.animation.ValueAnimator
+import android.os.Handler
+import android.os.Looper
 import androidx.annotation.VisibleForTesting
 import androidx.annotation.VisibleForTesting.PRIVATE
 import com.mapbox.common.Logger
@@ -42,8 +45,6 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
    * Consists of set owner + animator set itself.
    */
   private var highLevelAnimatorSet: HighLevelAnimatorSet? = null
-  @VisibleForTesting(otherwise = PRIVATE)
-  internal var highLevelListener: Animator.AnimatorListener? = null
 
   private val centerListeners = CopyOnWriteArraySet<CameraAnimatorChangeListener<Point>>()
   private val zoomListeners = CopyOnWriteArraySet<CameraAnimatorChangeListener<Double>>()
@@ -55,6 +56,16 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
 
   private val lifecycleListeners = CopyOnWriteArraySet<CameraAnimationsLifecycleListener>()
 
+  private val handler = Handler(Looper.getMainLooper())
+  private var commitScheduled = false
+  private val commitChangesRunnable = Runnable {
+    performMapJump(cameraOptionsBuilder.anchor(anchor).build())
+
+    // reset values
+    cameraOptionsBuilder = CameraOptions.Builder()
+    commitScheduled = false
+  }
+
   /**
    * If debug mode is enabled extra logs will be written about animation lifecycle and
    * some other events that may be useful for debugging.
@@ -163,6 +174,7 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
     paddingListeners.clear()
     lifecycleListeners.clear()
     animators.clear()
+    handler.removeCallbacks(commitChangesRunnable)
   }
 
   private fun performMapJump(cameraOptions: CameraOptions) {
@@ -238,21 +250,6 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
     }
   }
 
-  private fun correctInitialCamera(
-    currentCameraBuilder: CameraOptions.Builder,
-    cameraAnimator: CameraAnimator<*>
-  ): CameraOptions {
-    return when (cameraAnimator) {
-      is CameraCenterAnimator -> currentCameraBuilder.center(cameraAnimator.animatedValue as? Point).build()
-      is CameraZoomAnimator -> currentCameraBuilder.zoom(cameraAnimator.animatedValue as? Double).build()
-      is CameraAnchorAnimator -> currentCameraBuilder.anchor(cameraAnimator.animatedValue as? ScreenCoordinate).build()
-      is CameraPaddingAnimator -> currentCameraBuilder.padding(cameraAnimator.animatedValue as? EdgeInsets).build()
-      is CameraBearingAnimator -> currentCameraBuilder.bearing(cameraAnimator.animatedValue as? Double).build()
-      is CameraPitchAnimator -> currentCameraBuilder.pitch(cameraAnimator.animatedValue as? Double).build()
-      else -> throw RuntimeException("Unsupported animator type!")
-    }
-  }
-
   private fun registerInternalListener(animator: CameraAnimator<*>) {
     animator.addInternalListener(object : Animator.AnimatorListener {
 
@@ -266,10 +263,6 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
           lifecycleListeners.forEach {
             it.onAnimatorStarting(type, this, owner)
           }
-          if (runningAnimatorsQueue.isEmpty()) {
-            highLevelAnimatorSet?.started = true
-            highLevelListener?.onAnimationStart(animation)
-          }
           mapTransformDelegate.setUserAnimationInProgress(true)
           // check if such animation is not running already
           // if it is - then cancel it
@@ -326,14 +319,6 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
             unregisterAnimators(this, cancelAnimators = false)
           }
           if (runningAnimatorsQueue.isEmpty()) {
-            cameraOptionsBuilder.build().let { options ->
-              if (options.hasChanges) {
-                // cameraOptionsBuilder might have some accumulated, but not applied changes.
-                performMapJump(options)
-                cameraOptionsBuilder = CameraOptions.Builder()
-              }
-            }
-
             mapTransformDelegate.setUserAnimationInProgress(false)
           }
           lifecycleListeners.forEach {
@@ -342,17 +327,8 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
               AnimationFinishStatus.ENDED -> it.onAnimatorEnding(type, this, owner)
             }
           }
-          // it could happen that some animators are cancelled inside onAnimatorStarting callback
-          // while high-level animation is starting and highLevelListener != null already,
-          // so we check on `started` flag to validate that high-level animation truly started
-          if (runningAnimatorsQueue.isEmpty() && highLevelAnimatorSet?.started == true) {
-            when (finishStatus) {
-              AnimationFinishStatus.CANCELED -> {
-                highLevelListener?.onAnimationCancel(animation)
-                highLevelListener?.onAnimationEnd(animation)
-              }
-              AnimationFinishStatus.ENDED -> highLevelListener?.onAnimationEnd(animation)
-            }
+          if (runningAnimatorsQueue.isEmpty()) {
+            commitChanges()
           }
         } ?: throw RuntimeException(
           "Could not finish animation in CameraManager! " +
@@ -364,46 +340,38 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
 
   private fun registerInternalUpdateListener(animator: CameraAnimator<*>) {
     animator.addInternalUpdateListener {
+      // add current animator to queue-set if was not present
+      runningAnimatorsQueue.add(animator)
+
       // set current animator value in any case
       updateCameraValue(animator)
-      // main idea here is not to update map on each option change
-      // we perform jump based on update tick of first (oldest) animation
-      val firstAnimator = if (runningAnimatorsQueue.iterator().hasNext()) {
-        runningAnimatorsQueue.iterator().next()
-      } else {
-        null
-      }
+
       if (animator.type == CameraAnimatorType.ANCHOR) {
         anchor = it.animatedValue as ScreenCoordinate
       }
-      val cameraOptions = when {
-        // if no running animators in queue - get current map camera but apply first updated value
-        firstAnimator == null -> {
-          correctInitialCamera(
-            mapCameraManagerDelegate.cameraState.toCameraOptions(anchor).toBuilder(),
-            animator
-          )
-        }
-        // if update is triggered for first (oldest) animator - build options and jump
-        it == firstAnimator -> {
-          cameraOptionsBuilder.anchor(anchor).build()
-        }
-        // do not perform jump if update is triggered for not first (oldest) animator
-        else -> {
-          null
+
+      if (animator.hasUserListeners) {
+        // If the animator have third-party listeners camera changes must be applied immediately
+        // to be seen from the listeners.
+        commitChanges()
+      } else {
+        // main idea here is not to update map on each option change.
+        // the runnable posted here will be executed right after all the animators are applied.
+        if (!commitScheduled) {
+          handler.postAtFrontOfQueue(commitChangesRunnable)
+          commitScheduled = true
         }
       }
-      cameraOptions?.let { camera ->
-        // move map camera
-        performMapJump(camera)
-        // reset values
-        cameraOptionsBuilder = CameraOptions.Builder()
-      }
-      // add current animator to queue-set if was not present
-      runningAnimatorsQueue.add(it)
     }
   }
 
+  private fun commitChanges() {
+    if (commitScheduled) {
+      handler.removeCallbacks(commitChangesRunnable)
+    }
+    commitChangesRunnable.run()
+  }
+
   private fun cancelAnimatorSet() {
     highLevelAnimatorSet?.let {
       it.animatorSet.cancel()
@@ -857,16 +825,16 @@ class CameraAnimationsPluginImpl : CameraAnimationsPlugin {
         interpolator = it
       }
       animationOptions?.animatorListener?.let {
-        // duration == 0L does not behave the same as > 0 and actually
-        // our custom `highLevelListener` designed for proper notifying of lifecycle
-        // based on `runningAnimatorsQueue` is not working because animators end immediately.
-        // We use standard listener in that corner case.
-        highLevelListener = if (animationOptions.duration == 0L) {
-          addListener(it)
-          null
-        } else {
-          it
-        }
+        addListener(object : AnimatorListenerAdapter() {
+          override fun onAnimationEnd(animation: Animator?) {
+            commitChanges()
+
+            if (highLevelAnimatorSet?.animatorSet === animation) {
+              highLevelAnimatorSet = null
+            }
+          }
+        })
+        addListener(it)
       }
       playTogether(*animators)
     }

plugin-animation/src/main/java/com/mapbox/maps/plugin/animation/animator/CameraAnimator.kt

@@ -174,6 +174,12 @@ abstract class CameraAnimator<out T> (
     super.cancel()
   }
 
+  /**
+   * true if CameraAnimator have any external listeners registered.
+   */
+  internal val hasUserListeners: Boolean
+    get() = userUpdateListeners.isNotEmpty()
+
   internal fun addInternalUpdateListener(listener: AnimatorUpdateListener) {
     super.removeAllUpdateListeners()
     internalUpdateListener = listener

plugin-animation/src/test/java/com/mapbox/maps/plugin/animation/CameraAnimationsChangeListenersTest.kt

@@ -6,8 +6,11 @@ import com.mapbox.maps.ScreenCoordinate
 import com.mapbox.maps.plugin.animation.CameraAnimationsPluginImplTest.Companion.cameraState
 import org.junit.Assert
 import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
 import kotlin.math.pow
 
+@RunWith(RobolectricTestRunner::class)
 class CameraAnimationsChangeListenersTest {
 
   @Test

plugin-animation/src/test/java/com/mapbox/maps/plugin/animation/CameraAnimationsPluginImplTest.kt

@@ -140,6 +140,9 @@ class CameraAnimationsPluginImplTest {
       playTogether(*animators)
       start()
     }
+
+    shadowOf(getMainLooper()).idle()
+
     verify { mapCameraManagerDelegate.setCamera(any<CameraOptions>()) }
   }
 
@@ -827,26 +830,6 @@ class CameraAnimationsPluginImplTest {
     assertEquals(true, listenerTwo.interrupting)
   }
 
-  @Test
-  fun easeToTestNonZeroDurationWithListener() {
-    val options = mapAnimationOptions {
-      duration(1000)
-      animatorListener(object : AnimatorListenerAdapter() {})
-    }
-    cameraAnimationsPluginImpl.easeTo(cameraState.toCameraOptions(), options)
-    assert(cameraAnimationsPluginImpl.highLevelListener != null)
-  }
-
-  @Test
-  fun easeToTestZeroDurationWithListener() {
-    val options = mapAnimationOptions {
-      duration(0)
-      animatorListener(object : AnimatorListenerAdapter() {})
-    }
-    cameraAnimationsPluginImpl.easeTo(cameraState.toCameraOptions(), options)
-    assert(cameraAnimationsPluginImpl.highLevelListener == null)
-  }
-
   @Test
   fun registerOnlyCameraAnimatorsTest() {
     val pitch = createPitchAnimator(15.0, 0, 5)
@@ -962,13 +945,9 @@ class CameraAnimationsPluginImplTest {
     bearingAnimator.start()
     pitchAnimator.start()
     shadowOf(getMainLooper()).idle()
-    // first bearing tick as first animation -
-    // bearing updated to start value and pitch is not updated
-    assertEquals(20.0, updateList[0].bearing!!, EPS)
-    assertEquals(0.0, updateList[0].pitch!!, EPS)
-    // second bearing tick as first animation -
-    // pitch is updated to start value
-    assertEquals(10.0, updateList[1].pitch!!, EPS)
+    // Both, tick and bearing are updated to the start values at first tick.
+    assertEquals(20.0, updateList.first().bearing!!, EPS)
+    assertEquals(10.0, updateList.first().pitch!!, EPS)
   }
 
   @Test