#596: Remove callback order dependency from CameraAnimationsPluginImpl

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