From dd9abc60b32cb3888824a54664c75afc7a80397f Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Fri, 26 May 2017 17:31:13 -0400 Subject: [PATCH 1/4] Properly restore default values when removing props driven by native animated on Android --- .../react/animated/NativeAnimatedModule.java | 83 ++++++------ .../animated/NativeAnimatedNodesManager.java | 31 +++-- .../react/animated/PropsAnimatedNode.java | 68 +++++++--- .../react/uimanager/UIImplementation.java | 4 + .../react/uimanager/UIManagerModule.java | 25 +++- .../uimanager/UIManagerModuleListener.java | 21 +++ .../react/uimanager/UIViewOperationQueue.java | 4 + .../java/com/facebook/react/animated/BUCK | 1 + .../animated/NativeAnimatedModuleTest.java | 123 ++++++++++++++++++ .../NativeAnimatedNodeTraversalTest.java | 47 +++++++ 10 files changed, 338 insertions(+), 69 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleListener.java create mode 100644 ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedModuleTest.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java index f03cc8a460fbd5..008b6998dbcda9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java @@ -17,17 +17,20 @@ import com.facebook.react.bridge.Arguments; import com.facebook.react.bridge.Callback; import com.facebook.react.bridge.LifecycleEventListener; -import com.facebook.react.bridge.OnBatchCompleteListener; import com.facebook.react.bridge.ReactApplicationContext; import com.facebook.react.bridge.ReactContextBaseJavaModule; import com.facebook.react.bridge.ReactMethod; import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.WritableMap; +import com.facebook.react.common.annotations.VisibleForTesting; import com.facebook.react.module.annotations.ReactModule; import com.facebook.react.modules.core.DeviceEventManagerModule; import com.facebook.react.modules.core.ReactChoreographer; import com.facebook.react.uimanager.GuardedFrameCallback; +import com.facebook.react.uimanager.NativeViewHierarchyManager; +import com.facebook.react.uimanager.UIBlock; import com.facebook.react.uimanager.UIManagerModule; +import com.facebook.react.uimanager.UIManagerModuleListener; /** * Module that exposes interface for creating and managing animated nodes on the "native" side. @@ -72,7 +75,7 @@ */ @ReactModule(name = NativeAnimatedModule.NAME) public class NativeAnimatedModule extends ReactContextBaseJavaModule implements - OnBatchCompleteListener, LifecycleEventListener { + LifecycleEventListener, UIManagerModuleListener { protected static final String NAME = "NativeAnimatedModule"; @@ -80,11 +83,10 @@ private interface UIThreadOperation { void execute(NativeAnimatedNodesManager animatedNodesManager); } - private final Object mOperationsCopyLock = new Object(); private final GuardedFrameCallback mAnimatedFrameCallback; private final ReactChoreographer mReactChoreographer; private ArrayList mOperations = new ArrayList<>(); - private volatile @Nullable ArrayList mReadyOperations = null; + private ArrayList mPreOperations = new ArrayList<>(); private @Nullable NativeAnimatedNodesManager mNodesManager; @@ -95,24 +97,6 @@ public NativeAnimatedModule(ReactApplicationContext reactContext) { mAnimatedFrameCallback = new GuardedFrameCallback(reactContext) { @Override protected void doFrameGuarded(final long frameTimeNanos) { - if (mNodesManager == null) { - UIManagerModule uiManager = getReactApplicationContext() - .getNativeModule(UIManagerModule.class); - mNodesManager = new NativeAnimatedNodesManager(uiManager); - } - - ArrayList operations; - synchronized (mOperationsCopyLock) { - operations = mReadyOperations; - mReadyOperations = null; - } - - if (operations != null) { - for (int i = 0, size = operations.size(); i < size; i++) { - operations.get(i).execute(mNodesManager); - } - } - if (mNodesManager.hasActiveAnimations()) { mNodesManager.runUpdates(frameTimeNanos); } @@ -130,7 +114,11 @@ protected void doFrameGuarded(final long frameTimeNanos) { @Override public void initialize() { - getReactApplicationContext().addLifecycleEventListener(this); + ReactApplicationContext reactCtx = getReactApplicationContext(); + reactCtx.addLifecycleEventListener(this); + UIManagerModule uiManager = reactCtx.getNativeModule(UIManagerModule.class); + mNodesManager = new NativeAnimatedNodesManager(uiManager); + uiManager.addUIManagerListener(this); } @Override @@ -139,24 +127,30 @@ public void onHostResume() { } @Override - public void onBatchComplete() { - // Note: The order of executing onBatchComplete handler (especially in terms of onBatchComplete - // from the UIManagerModule) doesn't matter as we only enqueue operations for the UI thread to - // be executed from here. Thanks to ReactChoreographer all the operations from here are going - // to be executed *after* all the operations enqueued by UIManager as the callback type that we - // use for ReactChoreographer (CallbackType.NATIVE_ANIMATED_MODULE) is run after callbacks that - // UIManager uses. - ArrayList operations = mOperations.isEmpty() ? null : mOperations; - if (operations != null) { - mOperations = new ArrayList<>(); - synchronized (mOperationsCopyLock) { - if (mReadyOperations == null) { - mReadyOperations = operations; - } else { - mReadyOperations.addAll(operations); + public void willDispatchViewUpdates(final UIManagerModule uiManager) { + if (mOperations.isEmpty() && mPreOperations.isEmpty()) { + return; + } + final ArrayList preOperations = mPreOperations; + final ArrayList operations = mOperations; + mPreOperations = new ArrayList<>(); + mOperations = new ArrayList<>(); + uiManager.prependUIBlock(new UIBlock() { + @Override + public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { + for (UIThreadOperation operation : preOperations) { + operation.execute(mNodesManager); } } - } + }); + uiManager.addUIBlock(new UIBlock() { + @Override + public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { + for (UIThreadOperation operation : operations) { + operation.execute(mNodesManager); + } + } + }); } @Override @@ -186,6 +180,11 @@ private void enqueueFrameCallback() { mAnimatedFrameCallback); } + @VisibleForTesting + public void setNodesManager(NativeAnimatedNodesManager nodesManager) { + mNodesManager = nodesManager; + } + @ReactMethod public void createAnimatedNode(final int tag, final ReadableMap config) { mOperations.add(new UIThreadOperation() { @@ -336,6 +335,12 @@ public void execute(NativeAnimatedNodesManager animatedNodesManager) { @ReactMethod public void disconnectAnimatedNodeFromView(final int animatedNodeTag, final int viewTag) { + mPreOperations.add(new UIThreadOperation() { + @Override + public void execute(NativeAnimatedNodesManager animatedNodesManager) { + animatedNodesManager.restoreDefaultValues(animatedNodeTag, viewTag); + } + }); mOperations.add(new UIThreadOperation() { @Override public void execute(NativeAnimatedNodesManager animatedNodesManager) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java index 6251b8f4dd5528..06f65ca02a0bdc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java @@ -93,7 +93,7 @@ public void createAnimatedNode(int tag, ReadableMap config) { } else if ("value".equals(type)) { node = new ValueAnimatedNode(config); } else if ("props".equals(type)) { - node = new PropsAnimatedNode(config, this); + node = new PropsAnimatedNode(config, this, mUIImplementation); } else if ("interpolation".equals(type)) { node = new InterpolationAnimatedNode(config); } else if ("addition".equals(type)) { @@ -271,11 +271,7 @@ public void connectAnimatedNodeToView(int animatedNodeTag, int viewTag) { "of type " + PropsAnimatedNode.class.getName()); } PropsAnimatedNode propsAnimatedNode = (PropsAnimatedNode) node; - if (propsAnimatedNode.mConnectedViewTag != -1) { - throw new JSApplicationIllegalArgumentException("Animated node " + animatedNodeTag + " is " + - "already attached to a view"); - } - propsAnimatedNode.mConnectedViewTag = viewTag; + propsAnimatedNode.connectToView(viewTag); mUpdatedNodes.put(animatedNodeTag, node); } @@ -290,11 +286,24 @@ public void disconnectAnimatedNodeFromView(int animatedNodeTag, int viewTag) { "of type " + PropsAnimatedNode.class.getName()); } PropsAnimatedNode propsAnimatedNode = (PropsAnimatedNode) node; - if (propsAnimatedNode.mConnectedViewTag != viewTag) { - throw new JSApplicationIllegalArgumentException("Attempting to disconnect view that has " + - "not been connected with the given animated node"); + propsAnimatedNode.disconnectFromView(viewTag); + } + + public void restoreDefaultValues(int animatedNodeTag, int viewTag) { + AnimatedNode node = mAnimatedNodes.get(animatedNodeTag); + // Restoring default values needs to happen before UIManager operations so it is + // possible the node hasn't been created yet if it is being connected and + // disconnected in the same batch. In that case we don't need to restore + // default values since it will never actually update the view. + if (node == null) { + return; } - propsAnimatedNode.mConnectedViewTag = -1; + if (!(node instanceof PropsAnimatedNode)) { + throw new JSApplicationIllegalArgumentException("Animated node connected to view should be" + + "of type " + PropsAnimatedNode.class.getName()); + } + PropsAnimatedNode propsAnimatedNode = (PropsAnimatedNode) node; + propsAnimatedNode.restoreDefaultValues(); } public void addAnimatedEventToView(int viewTag, String eventName, ReadableMap eventMapping) { @@ -494,7 +503,7 @@ private void updateNodes(List nodes) { if (nextNode instanceof PropsAnimatedNode) { // Send property updates to native view manager try { - ((PropsAnimatedNode) nextNode).updateView(mUIImplementation); + ((PropsAnimatedNode) nextNode).updateView(); } catch (IllegalViewOperationException e) { // An exception is thrown if the view hasn't been created yet. This can happen because views are // created in batches. If this particular view didn't make it into a batch yet, the view won't diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/PropsAnimatedNode.java b/ReactAndroid/src/main/java/com/facebook/react/animated/PropsAnimatedNode.java index de5c3e6f0278e9..036f88435cc21e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/PropsAnimatedNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/PropsAnimatedNode.java @@ -9,6 +9,7 @@ package com.facebook.react.animated; +import com.facebook.react.bridge.JSApplicationIllegalArgumentException; import com.facebook.react.bridge.JavaOnlyMap; import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.ReadableMapKeySetIterator; @@ -27,47 +28,78 @@ */ /*package*/ class PropsAnimatedNode extends AnimatedNode { - /*package*/ int mConnectedViewTag = -1; - + private int mConnectedViewTag = -1; private final NativeAnimatedNodesManager mNativeAnimatedNodesManager; - private final Map mPropMapping; + private final UIImplementation mUIImplementation; + private final Map mPropNodeMapping; + // This is the backing map for `mDiffMap` we can mutate this to update it instead of having to + // create a new one for each update. + private final JavaOnlyMap mPropMap; + private final ReactStylesDiffMap mDiffMap; - PropsAnimatedNode(ReadableMap config, NativeAnimatedNodesManager nativeAnimatedNodesManager) { + PropsAnimatedNode(ReadableMap config, NativeAnimatedNodesManager nativeAnimatedNodesManager, UIImplementation uiImplementation) { ReadableMap props = config.getMap("props"); ReadableMapKeySetIterator iter = props.keySetIterator(); - mPropMapping = new HashMap<>(); + mPropNodeMapping = new HashMap<>(); while (iter.hasNextKey()) { String propKey = iter.nextKey(); int nodeIndex = props.getInt(propKey); - mPropMapping.put(propKey, nodeIndex); + mPropNodeMapping.put(propKey, nodeIndex); } + mPropMap = new JavaOnlyMap(); + mDiffMap = new ReactStylesDiffMap(mPropMap); mNativeAnimatedNodesManager = nativeAnimatedNodesManager; + mUIImplementation = uiImplementation; + } + + public void connectToView(int viewTag) { + if (mConnectedViewTag != -1) { + throw new JSApplicationIllegalArgumentException("Animated node " + mTag + " is " + + "already attached to a view"); + } + mConnectedViewTag = viewTag; + } + + public void disconnectFromView(int viewTag) { + if (mConnectedViewTag != viewTag) { + throw new JSApplicationIllegalArgumentException("Attempting to disconnect view that has " + + "not been connected with the given animated node"); + } + + mConnectedViewTag = -1; } - public final void updateView(UIImplementation uiImplementation) { + public void restoreDefaultValues() { + ReadableMapKeySetIterator it = mPropMap.keySetIterator(); + while(it.hasNextKey()) { + mPropMap.putNull(it.nextKey()); + } + + mUIImplementation.synchronouslyUpdateViewOnUIThread( + mConnectedViewTag, + mDiffMap); + } + + public final void updateView() { if (mConnectedViewTag == -1) { - throw new IllegalStateException("Node has not been attached to a view"); + return; } - JavaOnlyMap propsMap = new JavaOnlyMap(); - for (Map.Entry entry : mPropMapping.entrySet()) { + for (Map.Entry entry : mPropNodeMapping.entrySet()) { @Nullable AnimatedNode node = mNativeAnimatedNodesManager.getNodeById(entry.getValue()); if (node == null) { throw new IllegalArgumentException("Mapped property node does not exists"); } else if (node instanceof StyleAnimatedNode) { - ((StyleAnimatedNode) node).collectViewUpdates(propsMap); + ((StyleAnimatedNode) node).collectViewUpdates(mPropMap); } else if (node instanceof ValueAnimatedNode) { - propsMap.putDouble(entry.getKey(), ((ValueAnimatedNode) node).getValue()); + mPropMap.putDouble(entry.getKey(), ((ValueAnimatedNode) node).getValue()); } else { throw new IllegalArgumentException("Unsupported type of node used in property node " + node.getClass()); } } - // TODO: Reuse propsMap and stylesDiffMap objects - note that in subsequent animation steps - // for a given node most of the time we will be creating the same set of props (just with - // different values). We can take advantage on that and optimize the way we allocate property - // maps (we also know that updating view props doesn't retain a reference to the styles object). - uiImplementation.synchronouslyUpdateViewOnUIThread( + + mUIImplementation.synchronouslyUpdateViewOnUIThread( mConnectedViewTag, - new ReactStylesDiffMap(propsMap)); + mDiffMap); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java index 95ca85c9acc19b..c5873d36074bcc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java @@ -881,6 +881,10 @@ public void addUIBlock(UIBlock block) { mOperationsQueue.enqueueUIBlock(block); } + public void prependUIBlock(UIBlock block) { + mOperationsQueue.prependUIBlock(block); + } + public int resolveRootTagFromReactTag(int reactTag) { if (mShadowNodeRegistry.isRootNode(reactTag)) { return reactTag; diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java index d6b7d554bb2836..e8bd2f101c7b56 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java @@ -11,6 +11,7 @@ import javax.annotation.Nullable; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -85,6 +86,7 @@ public class UIManagerModule extends ReactContextBaseJavaModule implements private final Map mModuleConstants; private final UIImplementation mUIImplementation; private final MemoryTrimCallback mMemoryTrimCallback = new MemoryTrimCallback(); + private final List mListeners = new ArrayList<>(); private int mNextRootViewTag = 1; private int mBatchId = 0; @@ -533,6 +535,9 @@ public void onBatchComplete() { SystraceMessage.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "onBatchCompleteUI") .arg("BatchId", batchId) .flush(); + for (UIManagerModuleListener listener : mListeners) { + listener.willDispatchViewUpdates(this); + } try { mUIImplementation.dispatchViewUpdates(batchId); } finally { @@ -570,10 +575,28 @@ public void execute (NativeViewHierarchyManager nvhm) { } }); */ - public void addUIBlock (UIBlock block) { + public void addUIBlock(UIBlock block) { mUIImplementation.addUIBlock(block); } + /** + * Schedule a block to be executed on the UI thread. Useful if you need to execute + * view logic before all currently queued view updates have completed. + * + * @param block that contains UI logic you want to execute. + */ + public void prependUIBlock(UIBlock block) { + mUIImplementation.prependUIBlock(block); + } + + public void addUIManagerListener(UIManagerModuleListener listener) { + mListeners.add(listener); + } + + public void removeUIManagerListener(UIManagerModuleListener listener) { + mListeners.remove(listener); + } + /** * Given a reactTag from a component, find its root node tag, if possible. * Otherwise, this will return 0. If the reactTag belongs to a root node, this diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleListener.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleListener.java new file mode 100644 index 00000000000000..1dda859eb3f459 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModuleListener.java @@ -0,0 +1,21 @@ +/** + * Copyright (c) 2015-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package com.facebook.react.uimanager; + +/** + * Listener used to hook into the UIManager update process. + */ +public interface UIManagerModuleListener { + /** + * Called right before view updates are dispatched at the end of a batch. This is useful if a + * module needs to add UIBlocks to the queue before it is flushed. + */ + void willDispatchViewUpdates(UIManagerModule uiManager); +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java index cf6ceac30a00cc..5633e49dc671dd 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java @@ -725,6 +725,10 @@ public void enqueueUIBlock(UIBlock block) { mOperations.add(new UIBlockOperation(block)); } + public void prependUIBlock(UIBlock block) { + mOperations.add(0, new UIBlockOperation(block)); + } + /* package */ void dispatchViewUpdates(final int batchId) { SystraceMessage.beginSection( Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, diff --git a/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK b/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK index a81a1ab62bf4ff..3a93ccb165285b 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK @@ -19,6 +19,7 @@ rn_robolectric_test( react_native_target("java/com/facebook/react/animated:animated"), react_native_target("java/com/facebook/react/bridge:bridge"), react_native_target("java/com/facebook/react/common:common"), + react_native_target("java/com/facebook/react/modules:core"), react_native_target("java/com/facebook/react/uimanager:uimanager"), react_native_tests_target("java/com/facebook/react/bridge:testhelpers"), ], diff --git a/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedModuleTest.java new file mode 100644 index 00000000000000..8cd5dfb872572d --- /dev/null +++ b/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedModuleTest.java @@ -0,0 +1,123 @@ +/** + * Copyright (c) 2015-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + + +package com.facebook.react.animated; + +import com.facebook.react.bridge.ReactApplicationContext; +import com.facebook.react.modules.core.ReactChoreographer; +import com.facebook.react.uimanager.NativeViewHierarchyManager; +import com.facebook.react.uimanager.UIBlock; +import com.facebook.react.uimanager.UIManagerModule; +import com.facebook.react.uimanager.events.EventDispatcher; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InOrder; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PowerMockIgnore; +import org.powermock.core.classloader.annotations.PrepareEverythingForTest; +import org.robolectric.RobolectricTestRunner; + +import java.util.ArrayList; +import java.util.List; + +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyInt; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +@PrepareEverythingForTest +@RunWith(RobolectricTestRunner.class) +@PowerMockIgnore({"org.mockito.*", "org.robolectric.*", "android.*"}) +public class NativeAnimatedModuleTest { + private NativeAnimatedModule mNativeAnimatedModule; + private NativeAnimatedNodesManager mNodeManager; + private UIManagerModule mUIManager; + private List mUIBlocks; + + private void flushUIBlocks() { + mNativeAnimatedModule.willDispatchViewUpdates(mUIManager); + for (UIBlock block : mUIBlocks) { + block.execute(mock(NativeViewHierarchyManager.class)); + } + mUIBlocks.clear(); + } + + @Before + public void setUp() { + ReactApplicationContext context = mock(ReactApplicationContext.class); + mUIManager = mock(UIManagerModule.class); + PowerMockito.doReturn(mock(EventDispatcher.class)).when(mUIManager).getEventDispatcher(); + PowerMockito.doAnswer(new Answer() { + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + mUIBlocks.add(invocation.getArgumentAt(0, UIBlock.class)); + return null; + } + }).when(mUIManager).addUIBlock(any(UIBlock.class)); + PowerMockito.doAnswer(new Answer() { + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + mUIBlocks.add(0, invocation.getArgumentAt(0, UIBlock.class)); + return null; + } + }).when(mUIManager).prependUIBlock(any(UIBlock.class)); + PowerMockito.doReturn(mUIManager).when(context).getNativeModule(UIManagerModule.class); + + ReactChoreographer.initialize(); + mNativeAnimatedModule = new NativeAnimatedModule(context); + mNativeAnimatedModule.initialize(); + mNodeManager = mock(NativeAnimatedNodesManager.class); + mNativeAnimatedModule.setNodesManager(mNodeManager); + mUIBlocks = new ArrayList<>(); + } + + @Test + public void testNodeManagerSchedulingWaitsForFlush() { + // Make sure operations are not executed until UIManager has flushed it's queue. + mNativeAnimatedModule.connectAnimatedNodeToView(1, 1000); + verify(mNodeManager, never()).connectAnimatedNodeToView(anyInt(), anyInt()); + flushUIBlocks(); + verify(mNodeManager, times(1)).connectAnimatedNodeToView(anyInt(), anyInt()); + } + + @Test + public void testNodeManagerSchedulingOperation() { + UIBlock otherBlock = mock(UIBlock.class); + mUIManager.addUIBlock(otherBlock); + mNativeAnimatedModule.connectAnimatedNodeToView(1, 1000); + flushUIBlocks(); + + // Connect should be called after other UI operations. + InOrder inOrder = inOrder(mNodeManager, otherBlock); + inOrder.verify(otherBlock, times(1)).execute(any(NativeViewHierarchyManager.class)); + inOrder.verify(mNodeManager, times(1)).connectAnimatedNodeToView(1, 1000); + } + + @Test + public void testNodeManagerSchedulingPreOperation() { + UIBlock otherBlock = mock(UIBlock.class); + mUIManager.addUIBlock(otherBlock); + mNativeAnimatedModule.disconnectAnimatedNodeFromView(1, 1000); + flushUIBlocks(); + + // Disconnect should be called before other UI operations. + InOrder inOrder = inOrder(mNodeManager, otherBlock); + inOrder.verify(mNodeManager, times(1)).restoreDefaultValues(1, 1000); + inOrder.verify(otherBlock, times(1)).execute(any(NativeViewHierarchyManager.class)); + inOrder.verify(mNodeManager, times(1)).disconnectAnimatedNodeFromView(1, 1000); + } +} diff --git a/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java b/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java index ced79811c36960..caa62fecfbe427 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java @@ -992,4 +992,51 @@ public Object answer(InvocationOnMock invocation) throws Throwable { verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture()); assertThat(stylesCaptor.getValue().getDouble("opacity", Double.NaN)).isEqualTo(10); } + + @Test + public void testRestoreDefaultProps() { + int viewTag = 1000; + int propsNodeTag = 3; + mNativeAnimatedNodesManager.createAnimatedNode( + 1, + JavaOnlyMap.of("type", "value", "value", 1d, "offset", 0d)); + mNativeAnimatedNodesManager.createAnimatedNode( + 2, + JavaOnlyMap.of("type", "style", "style", JavaOnlyMap.of("opacity", 1))); + mNativeAnimatedNodesManager.createAnimatedNode( + propsNodeTag, + JavaOnlyMap.of("type", "props", "props", JavaOnlyMap.of("style", 2))); + mNativeAnimatedNodesManager.connectAnimatedNodes(1, 2); + mNativeAnimatedNodesManager.connectAnimatedNodes(2, propsNodeTag); + mNativeAnimatedNodesManager.connectAnimatedNodeToView(propsNodeTag, viewTag); + + JavaOnlyArray frames = JavaOnlyArray.of(0d, 0.5d, 1d); + Callback animationCallback = mock(Callback.class); + mNativeAnimatedNodesManager.startAnimatingNode( + 1, + 1, + JavaOnlyMap.of("type", "frames", "frames", frames, "toValue", 0d), + animationCallback); + + ArgumentCaptor stylesCaptor = + ArgumentCaptor.forClass(ReactStylesDiffMap.class); + + reset(mUIImplementationMock); + mNativeAnimatedNodesManager.runUpdates(nextFrameTime()); + verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture()); + assertThat(stylesCaptor.getValue().getDouble("opacity", Double.NaN)).isEqualTo(1); + + for (int i = 0; i < frames.size(); i++) { + reset(mUIImplementationMock); + mNativeAnimatedNodesManager.runUpdates(nextFrameTime()); + } + + verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture()); + assertThat(stylesCaptor.getValue().getDouble("opacity", Double.NaN)).isEqualTo(0); + + reset(mUIImplementationMock); + mNativeAnimatedNodesManager.disconnectAnimatedNodeFromView(propsNodeTag, viewTag); + verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture()); + assertThat(stylesCaptor.getValue().isNull("opacity")); + } } From 5fbf352d0d8077d1137dd61004953a22471da4d9 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Wed, 28 Jun 2017 01:27:18 -0400 Subject: [PATCH 2/4] Fix NativeAnimatedNodesManager initialization race condition --- .../react/animated/NativeAnimatedModule.java | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java index 008b6998dbcda9..0591f5dbb7e14c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java @@ -97,8 +97,9 @@ public NativeAnimatedModule(ReactApplicationContext reactContext) { mAnimatedFrameCallback = new GuardedFrameCallback(reactContext) { @Override protected void doFrameGuarded(final long frameTimeNanos) { - if (mNodesManager.hasActiveAnimations()) { - mNodesManager.runUpdates(frameTimeNanos); + NativeAnimatedNodesManager nodesManager = getNodesManager(); + if (nodesManager.hasActiveAnimations()) { + nodesManager.runUpdates(frameTimeNanos); } // TODO: Would be great to avoid adding this callback in case there are no active animations @@ -115,9 +116,8 @@ protected void doFrameGuarded(final long frameTimeNanos) { @Override public void initialize() { ReactApplicationContext reactCtx = getReactApplicationContext(); - reactCtx.addLifecycleEventListener(this); UIManagerModule uiManager = reactCtx.getNativeModule(UIManagerModule.class); - mNodesManager = new NativeAnimatedNodesManager(uiManager); + reactCtx.addLifecycleEventListener(this); uiManager.addUIManagerListener(this); } @@ -138,16 +138,18 @@ public void willDispatchViewUpdates(final UIManagerModule uiManager) { uiManager.prependUIBlock(new UIBlock() { @Override public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { + NativeAnimatedNodesManager nodesManager = getNodesManager(); for (UIThreadOperation operation : preOperations) { - operation.execute(mNodesManager); + operation.execute(nodesManager); } } }); uiManager.addUIBlock(new UIBlock() { @Override public void execute(NativeViewHierarchyManager nativeViewHierarchyManager) { + NativeAnimatedNodesManager nodesManager = getNodesManager(); for (UIThreadOperation operation : operations) { - operation.execute(mNodesManager); + operation.execute(nodesManager); } } }); @@ -168,6 +170,15 @@ public String getName() { return NAME; } + private NativeAnimatedNodesManager getNodesManager() { + if (mNodesManager == null) { + UIManagerModule uiManager = getReactApplicationContext().getNativeModule(UIManagerModule.class); + mNodesManager = new NativeAnimatedNodesManager(uiManager); + } + + return mNodesManager; + } + private void clearFrameCallback() { Assertions.assertNotNull(mReactChoreographer).removeFrameCallback( ReactChoreographer.CallbackType.NATIVE_ANIMATED_MODULE, From a5a49cb2569e3eba0e9a18befcc3103c889f99ee Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Fri, 4 Aug 2017 13:48:52 -0400 Subject: [PATCH 3/4] Fix buck build --- ReactAndroid/src/test/java/com/facebook/react/animated/BUCK | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK b/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK index 3a93ccb165285b..4a175cc5a52c00 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK @@ -19,7 +19,7 @@ rn_robolectric_test( react_native_target("java/com/facebook/react/animated:animated"), react_native_target("java/com/facebook/react/bridge:bridge"), react_native_target("java/com/facebook/react/common:common"), - react_native_target("java/com/facebook/react/modules:core"), + react_native_target("java/com/facebook/react/modules/core:core"), react_native_target("java/com/facebook/react/uimanager:uimanager"), react_native_tests_target("java/com/facebook/react/bridge:testhelpers"), ], From deeb71388155661c55a413466c231143c7ebb314 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Fri, 4 Aug 2017 14:49:56 -0400 Subject: [PATCH 4/4] Fix tests --- .../react/animated/NativeAnimatedNodeTraversalTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java b/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java index caa62fecfbe427..91dd104bcee776 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java @@ -1035,7 +1035,7 @@ public void testRestoreDefaultProps() { assertThat(stylesCaptor.getValue().getDouble("opacity", Double.NaN)).isEqualTo(0); reset(mUIImplementationMock); - mNativeAnimatedNodesManager.disconnectAnimatedNodeFromView(propsNodeTag, viewTag); + mNativeAnimatedNodesManager.restoreDefaultValues(propsNodeTag, viewTag); verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture()); assertThat(stylesCaptor.getValue().isNull("opacity")); }