-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keep non-react views in AnimationManager #4439
Conversation
android/src/main/java/com/swmansion/reanimated/layoutReanimation/AnimationsManager.java
Outdated
Show resolved
Hide resolved
Could you provide to issue description the source code of reproduction? |
android/src/main/java/com/swmansion/reanimated/layoutReanimation/AnimationsManager.java
Outdated
Show resolved
Hide resolved
18b7b9b
to
3a63917
Compare
android/src/main/java/com/swmansion/reanimated/layoutReanimation/AnimationsManager.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/reanimated/layoutReanimation/AnimationsManager.java
Outdated
Show resolved
Hide resolved
Any update regarding this pr? Running into the crashes described above and hoping this will fix it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should add check to removeOrAnimateExitRecursive
method:
if (hasAnimatedChildren) {
+ if (tag == -1) {
+ // View tags are used to identify views, but it is hard for no-react views with -1 tag.
+ // We shouldn't to manage the lifetime of no-react components.
+ cancelAnimationsRecursive(view);
+ return false;
+ }
mAncestorsToRemove.add(tag);
}
end remove changes from removeView
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we need to test it well
@Piaskowy Suggested changes fix #4437, but app still crashes on #4306 The error log is the same (except for PID etc.)E/AndroidRuntime: FATAL EXCEPTION: main Process: com.swmansion.reanimated.example, PID: 7138 java.lang.IndexOutOfBoundsException: getChildDrawingOrder() returned invalid index 1 (child count is 1) at android.view.ViewGroup.getAndVerifyPreorderedIndex(ViewGroup.java:2099) at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4287) at android.view.View.draw(View.java:23197) at android.view.View.updateDisplayListIfDirty(View.java:22061) at android.view.View.draw(View.java:22925) at android.view.ViewGroup.drawChild(ViewGroup.java:4529) at com.facebook.react.views.view.ReactViewGroup.drawChild(ReactViewGroup.java:836) at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4290) at com.facebook.react.views.view.ReactViewGroup.dispatchDraw(ReactViewGroup.java:809) at android.view.View.draw(View.java:23197) at android.view.View.updateDisplayListIfDirty(View.java:22061) at android.view.View.draw(View.java:22925) at android.view.ViewGroup.drawChild(ViewGroup.java:4529) at com.facebook.react.views.view.ReactViewGroup.drawChild(ReactViewGroup.java:836) at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4290) at com.facebook.react.views.view.ReactViewGroup.dispatchDraw(ReactViewGroup.java:809) at android.view.View.updateDisplayListIfDirty(View.java:22052) at android.view.View.draw(View.java:22925) at android.view.ViewGroup.drawChild(ViewGroup.java:4529) at com.facebook.react.views.view.ReactViewGroup.drawChild(ReactViewGroup.java:836) at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4290) at com.facebook.react.views.view.ReactViewGroup.dispatchDraw(ReactViewGroup.java:809) at android.view.View.updateDisplayListIfDirty(View.java:22052) at android.view.View.draw(View.java:22925) at android.view.ViewGroup.drawChild(ViewGroup.java:4529) at com.facebook.react.views.view.ReactViewGroup.drawChild(ReactViewGroup.java:836) at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4290) at com.facebook.react.views.view.ReactViewGroup.dispatchDraw(ReactViewGroup.java:809) at android.view.View.updateDisplayListIfDirty(View.java:22052) at android.view.View.draw(View.java:22925) at android.view.ViewGroup.drawChild(ViewGroup.java:4529) at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4290) at android.view.View.updateDisplayListIfDirty(View.java:22052) at android.view.View.draw(View.java:22925) at android.view.ViewGroup.drawChild(ViewGroup.java:4529) at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4290) at android.view.View.updateDisplayListIfDirty(View.java:22052) at android.view.View.draw(View.java:22925) at android.view.ViewGroup.drawChild(ViewGroup.java:4529) at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4290) at android.view.View.updateDisplayListIfDirty(View.java:22052) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) E/AndroidRuntime: at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:4513) at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:4486) at android.view.View.updateDisplayListIfDirty(View.java:22017) at android.view.ThreadedRenderer.updateViewTreeDisplayList(ThreadedRenderer.java:689) at android.view.ThreadedRenderer.updateRootDisplayList(ThreadedRenderer.java:695) at android.view.ThreadedRenderer.draw(ThreadedRenderer.java:793) at android.view.ViewRootImpl.draw(ViewRootImpl.java:4670) at android.view.ViewRootImpl.performDraw(ViewRootImpl.java:4381) at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3600) at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:2328) at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:9087) at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1231) at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1239) at android.view.Choreographer.doCallbacks(Choreographer.java:899) at android.view.Choreographer.doFrame(Choreographer.java:832) at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1214) at android.os.Handler.handleCallback(Handler.java:942) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loopOnce(Looper.java:201) at android.os.Looper.loop(Looper.java:288) at android.app.ActivityThread.main(ActivityThread.java:7872) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548) |
I tested leaking views, but everything seems to be ok 👍 |
@@ -529,6 +529,12 @@ private boolean removeOrAnimateExitRecursive(View view, boolean shouldRemove) { | |||
} | |||
|
|||
if (hasAnimatedChildren) { | |||
if (tag == -1) { | |||
// View tags are used to identify views, but it is hard for no-react views with -1 tag. | |||
// We shouldn't to manage the lifetime of no-react components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// We shouldn't to manage the lifetime of no-react components. | |
// We shouldn't manage lifetime of non-react components. |
@@ -529,6 +529,12 @@ private boolean removeOrAnimateExitRecursive(View view, boolean shouldRemove) { | |||
} | |||
|
|||
if (hasAnimatedChildren) { | |||
if (tag == -1) { | |||
// View tags are used to identify views, but it is hard for no-react views with -1 tag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand "it is hard for no-react views with -1 tag" correctly, what exactly do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've accidentally merged this PR without fixing this comment 😱
I opened this: #4634 to fix it
@@ -503,7 +503,7 @@ private boolean removeOrAnimateExitRecursive(View view, boolean shouldRemove) { | |||
View child = viewGroup.getChildAt(i); | |||
if (removeOrAnimateExitRecursive(child, shouldRemove)) { | |||
hasAnimatedChildren = true; | |||
} else if (shouldRemove) { | |||
} else if (shouldRemove && child.getId() != -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for this basic question but could you please explain in which cases getId()
returns -1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that the view is native only, and not a react one.
Summary
Removing views with id -1 (views that are not a react-native ones) causes at least two bugs on Android.
Fixes #4306 and Fixes #4437
Bug #4306 - [Android] <Animated.ScrollView /> is crashing on navigate.goBack() when using refreshControl prop
Error log
Screen.Recording.2023-05-09.at.11.00.57.mov
Screen.Recording.2023-05-09.at.10.59.53.mov
Bug #4437 - ReanimatedError: Exception in HostFunction: java.lang.ClassNotFoundException: Could not retrieve ViewPager2 instance, js engine: reanimated
No error logs 😢
Screen.Recording.2023-05-09.at.11.05.24.mov
Screen.Recording.2023-05-09.at.11.03.21.mov
Test plan
Tested on following repos: