From c9dcd64ed557d477828549e54afa71a12e5294ec Mon Sep 17 00:00:00 2001 From: Zeya Peng Date: Tue, 16 Sep 2025 00:44:23 -0700 Subject: [PATCH] Clean up batchingControlledByJS in NativeAnimated kotlin (#53793) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/53793 ## Changelog: [Android] [Deprecated] - Clean up batchingControlledByJS in NativeAnimated kotlin `start/finishOperationBatch` will no longer be called on kotlin NativeAnimated since D78005971 (https://github.com/facebook/react-native/pull/52521), so `batchingControlledByJS` will remain false. Cleaning up some logic and TODO comments there this feature was added in D23010844 Reviewed By: christophpurrer Differential Revision: D82461457 fbshipit-source-id: a1208720b83e614c2a5f994ec1a5005189c5f197 --- .../react/animated/NativeAnimatedModule.kt | 43 ++++++++----------- .../animated/NativeAnimatedNodesManager.kt | 4 -- 2 files changed, 19 insertions(+), 28 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.kt index 6c5a505c921..9d9ce251430 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.kt @@ -196,11 +196,9 @@ public class NativeAnimatedModule(reactContext: ReactApplicationContext) : private val nodesManagerRef = AtomicReference() - private var batchingControlledByJS = false // TODO T71377544: delete + @Volatile private var currentFrameNumber: Long = 0 - @Volatile private var currentFrameNumber: Long = 0 // TODO T71377544: delete - - @Volatile private var currentBatchNumber: Long = 0 + @Volatile private var currentBatchNumber: Long = 0 // frame number at last operations dispatch private var initializedForFabric = false private var initializedForNonFabric = false @@ -282,22 +280,19 @@ public class NativeAnimatedModule(reactContext: ReactApplicationContext) : var batchNumber = currentBatchNumber - 1 - // TODO T71377544: delete this when the JS method is confirmed safe - if (!batchingControlledByJS) { - // The problem we're trying to solve here: we could be in the middle of queueing - // a batch of related animation operations when Fabric flushes a batch of MountItems. - // It's visually bad if we execute half of the animation ops and then wait another frame - // (or more) to execute the rest. - // See mFrameNumber. If the dispatchedFrameNumber drifts too far - that - // is, if no MountItems are scheduled for a while, which can happen if a tree - // is committed but there are no changes - bring these counts back in sync and - // execute any queued operations. This number is arbitrary, but we want it low - // enough that the user shouldn't be able to see this delay in most cases. - currentFrameNumber++ - if ((currentFrameNumber - currentBatchNumber) > 2) { - currentBatchNumber = currentFrameNumber - batchNumber = currentBatchNumber - } + // The problem we're trying to solve here: we could be in the middle of queueing + // a batch of related animation operations when Fabric flushes a batch of MountItems. + // It's visually bad if we execute half of the animation ops and then wait another frame + // (or more) to execute the rest. + // See mFrameNumber. If the dispatchedFrameNumber drifts too far - that + // is, if no MountItems are scheduled for a while, which can happen if a tree + // is committed but there are no changes - bring these counts back in sync and + // execute any queued operations. This number is arbitrary, but we want it low + // enough that the user shouldn't be able to see this delay in most cases. + currentFrameNumber++ + if ((currentFrameNumber - currentBatchNumber) > 2) { + currentBatchNumber = currentFrameNumber + batchNumber = currentBatchNumber } preOperations.executeBatch(batchNumber, nodesManager) @@ -484,14 +479,14 @@ public class NativeAnimatedModule(reactContext: ReactApplicationContext) : } } + @Suppress("DEPRECATION") override fun startOperationBatch() { - batchingControlledByJS = true - currentBatchNumber++ + // no-op } + @Suppress("DEPRECATION") override fun finishOperationBatch() { - batchingControlledByJS = false - currentBatchNumber++ + // no-op } override fun createAnimatedNode(tagDouble: Double, config: ReadableMap) { diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.kt index f7237025ab3..4641503ac4f 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.kt @@ -774,12 +774,8 @@ public class NativeAnimatedNodesManager( ("Looks like animated nodes graph has ${reason}, there are $activeNodesCount but toposort visited only $updatedNodesCount") ) if (eventListenerInitializedForFabric && cyclesDetected == 0) { - // TODO T71377544: investigate these SoftExceptions and see if we can remove entirely - // or fix the root cause ReactSoftExceptionLogger.logSoftException(TAG, ReactNoCrashSoftException(ex)) } else if (eventListenerInitializedForFabric) { - // TODO T71377544: investigate these SoftExceptions and see if we can remove entirely - // or fix the root cause ReactSoftExceptionLogger.logSoftException(TAG, ReactNoCrashSoftException(ex)) } else { throw ex