Native Animated - Restore default values when removing props on Android #12842

janicduplessis · 2017-03-10T01:12:36Z

Same as #11819 but for Android. I didn't notice the bug initially in my app because I was using different animations on Android which did not trigger this issue.

Test plan
Created a simple repro example and tested that this fixes it. https://gist.github.com/janicduplessis/0f3eb362dae63fedf99a0d3ee041796a

mkonicek · 2017-03-14T22:38:07Z

Hi Janic, do you know who can review changes to native animated on Android?

janicduplessis · 2017-03-14T23:04:37Z

Replied on slack

janicduplessis · 2017-03-17T20:42:59Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java

@@ -181,6 +182,11 @@ private void enqueueFrameCallback() {
      mAnimatedFrameCallback);
  }

+  @VisibleForTesting
+  public void setNodesManager(NativeAnimatedNodesManager nodesManager) {


Not sure if there is a better way to do this, we have to inject a mock in here but there is no way currently. I tried using PowerMockito.whenNew but could not get it to work.

janicduplessis · 2017-03-17T20:44:58Z

ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedModuleTest.java

+    InOrder inOrder = inOrder(mNodeManager, otherBlock);
+    inOrder.verify(mNodeManager, times(1)).disconnectAnimatedNodeFromView(1, 1000);
+    inOrder.verify(otherBlock, times(1)).execute(any(NativeViewHierarchyManager.class));
+  }


Ideally we'd also test multithreading here to make sure everything gets called on the proper thread and is safe but I'm not sure how to do that.

janicduplessis · 2017-03-17T20:46:33Z

Also added a regression test and a few extra tests around the scheduling of NativeAnimatedNodesManager operations.

hramos

Sorry for the delayed review. Is there someone on the React or RN team that you think should review this? I can reach out to them.

hramos · 2017-04-26T13:35:45Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java


  private @Nullable NativeAnimatedNodesManager mNodesManager;

  public NativeAnimatedModule(ReactApplicationContext reactContext) {
    super(reactContext);

    mReactChoreographer = ReactChoreographer.getInstance();
-    mAnimatedFrameCallback = new GuardedFrameCallback(reactContext) {
+    mAnimatedFrameCallback = new GuardedFrameCallback(reactCtx) {


Is reactCtx defined at this point?

You are right, I messed that up when fixing conflicts.

hramos · 2017-04-26T13:36:57Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java

@@ -139,24 +128,30 @@ public void onHostResume() {
  }

  @Override
-  public void onBatchComplete() {


Was this method used anywhere?

This is a method native modules can override that gets called when batches are completed so it is fine to remove it since it will just call the one from the super class.

janicduplessis · 2017-04-26T22:25:25Z

@AaaChiuuu or Andrew Chen could review this

janicduplessis · 2017-05-26T19:47:19Z

@hramos Could you try to have this reviewed?

hramos · 2017-05-26T21:44:57Z

Just pinged @AaaChiuuu

animated on Android

hramos

Approving this without any further review as it has been a while since the review was requested, and Janic has already been using this in production for a while. I don't want to slow down contributions from the core team unnecessarily.

facebook-github-bot · 2017-08-03T19:36:57Z

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-08-04T08:49:10Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

pull-bot · 2017-08-04T18:17:23Z

This PR has been submitted by a core contributor.

Attention: @janicduplessis

Generated by 🚫 dangerJS

janicduplessis · 2017-08-04T19:36:49Z

@hramos Oups, there was a buck build issue and a test I forgot to update, circle is green now it should land smoothly.

facebook-github-bot · 2017-08-04T19:46:35Z

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hramos · 2017-08-04T22:59:15Z

ac43548 is in the process of being reverted. The NativeAnimatedModuleTest started failing when this landed:

javac_jar
stderr: ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedModuleTest.java:67: error: cannot find symbol
        mUIBlocks.add(invocation.getArgumentAt(0, UIBlock.class));
                                ^
  symbol:   method getArgumentAt(int,java.lang.Class<com.facebook.react.uimanager.UIBlock>)
  location: variable invocation of type org.mockito.invocation.InvocationOnMock

ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedModuleTest.java:74: error: cannot find symbol
        mUIBlocks.add(0, invocation.getArgumentAt(0, UIBlock.class));
                                   ^
  symbol:   method getArgumentAt(int,java.lang.Class<com.facebook.react.uimanager.UIBlock>)
  location: variable invocation of type org.mockito.invocation.InvocationOnMock
Errors: 2. Warnings: 0.

At this point I am not yet sure why this is breaking internally, when the PR passed tests. Tests also passed on master after this landed.

janicduplessis · 2017-08-04T23:14:25Z

@hramos Strange that it doesn't fail on OSS CI, is it possible that you are using a different version of mockito internally? According to OSS gradle.properties (https://github.com/facebook/react-native/blob/master/ReactAndroid/gradle.properties#L10) we are using version 1.10.19, same with buck https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/third-party/java/mockito/BUCK#L19.

Summary: Rebased version of #12842 that was reverted because of failing fb internal tests. Closes #15919 Differential Revision: D5823956 Pulled By: hramos fbshipit-source-id: 4ece19a403f5ebbe4829c4c26696ea0575ab1d0e

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Mar 10, 2017

janicduplessis force-pushed the native-animated-default-values-android branch from 6203dcb to ffc8b92 Compare March 10, 2017 02:04

janicduplessis commented Mar 17, 2017

View reviewed changes

janicduplessis added Android labels Mar 17, 2017

hramos reviewed Apr 26, 2017

View reviewed changes

ptc-jhughes mentioned this pull request Apr 26, 2017

Header of first screen rendered behind second screen header react-navigation/react-navigation#229

Closed

hramos added the Core Team label Apr 27, 2017

janicduplessis force-pushed the native-animated-default-values-android branch from 0ede136 to f6b3f7f Compare May 26, 2017 21:33

janicduplessis added 2 commits June 28, 2017 00:50

Properly restore default values when removing props driven by native

dd9abc6

animated on Android

Fix NativeAnimatedNodesManager initialization race condition

5fbf352

janicduplessis force-pushed the native-animated-default-values-android branch from f6b3f7f to 5fbf352 Compare June 28, 2017 05:29

janicduplessis added 2 commits July 13, 2017 17:59

Merge branch 'master' into native-animated-default-values-android

3657ccd

Merge branch 'master' into native-animated-default-values-android

6ffbc97

hramos approved these changes Aug 3, 2017

View reviewed changes

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Aug 3, 2017

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 4, 2017

Fix buck build

a5a49cb

Fix tests

deeb713

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 4, 2017

facebook-github-bot closed this in ac43548 Aug 4, 2017

janicduplessis mentioned this pull request Sep 13, 2017

Native Animated - Restore default values when removing props on Android #15919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native Animated - Restore default values when removing props on Android #12842

Native Animated - Restore default values when removing props on Android #12842

janicduplessis commented Mar 10, 2017

mkonicek commented Mar 14, 2017

janicduplessis commented Mar 14, 2017

janicduplessis Mar 17, 2017

janicduplessis Mar 17, 2017

janicduplessis commented Mar 17, 2017

hramos left a comment

hramos Apr 26, 2017

janicduplessis Apr 26, 2017

hramos Apr 26, 2017

janicduplessis Apr 26, 2017

janicduplessis commented Apr 26, 2017

janicduplessis commented May 26, 2017

hramos commented May 26, 2017

hramos left a comment

facebook-github-bot commented Aug 3, 2017

facebook-github-bot commented Aug 4, 2017

pull-bot commented Aug 4, 2017 •

edited

Loading

janicduplessis commented Aug 4, 2017

facebook-github-bot commented Aug 4, 2017

hramos commented Aug 4, 2017 •

edited

Loading

janicduplessis commented Aug 4, 2017 •

edited

Loading

Native Animated - Restore default values when removing props on Android #12842

Native Animated - Restore default values when removing props on Android #12842

Conversation

janicduplessis commented Mar 10, 2017

mkonicek commented Mar 14, 2017

janicduplessis commented Mar 14, 2017

janicduplessis Mar 17, 2017

Choose a reason for hiding this comment

janicduplessis Mar 17, 2017

Choose a reason for hiding this comment

janicduplessis commented Mar 17, 2017

hramos left a comment

Choose a reason for hiding this comment

hramos Apr 26, 2017

Choose a reason for hiding this comment

janicduplessis Apr 26, 2017

Choose a reason for hiding this comment

hramos Apr 26, 2017

Choose a reason for hiding this comment

janicduplessis Apr 26, 2017

Choose a reason for hiding this comment

janicduplessis commented Apr 26, 2017

janicduplessis commented May 26, 2017

hramos commented May 26, 2017

hramos left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 3, 2017

facebook-github-bot commented Aug 4, 2017

pull-bot commented Aug 4, 2017 • edited Loading

janicduplessis commented Aug 4, 2017

facebook-github-bot commented Aug 4, 2017

hramos commented Aug 4, 2017 • edited Loading

janicduplessis commented Aug 4, 2017 • edited Loading

pull-bot commented Aug 4, 2017 •

edited

Loading

hramos commented Aug 4, 2017 •

edited

Loading

janicduplessis commented Aug 4, 2017 •

edited

Loading