-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
[Android][Fixed] - Fix the IndexOutOfBoundsException when wrong tag is used in manageChildren because of animation. #23530
Conversation
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.
Hey @zhangmincons, thanks for working on this. I added few comments
Moving the PR back to you to review my comments.
ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java
Outdated
Show resolved
Hide resolved
@zhangmincons @mdvacca Isn't this PR already trying to fix the same problem? |
Nope, I've read that PR, and it fixed a different problem. |
Thank you, I will read the comments and fix the problems. |
d4d3d9d
to
ea816ea
Compare
hi, @mdvacca, the problems are solved. Can you review the code again? Thanks! |
…s used in manageChildren because of animation.
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.
@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Is there any progress on this? |
Hi @zhangmincons this was fixed internally here: Apologies about the lack of synchronization on this! |
Based on @lunaleaps's comment, I'll close this PR here since the underlying problem should already have been fixed. |
Fix the IndexOutOfBoundsException when wrong tag is used in manageChildren because of animation.
Summary
When using animation, some views may not be removed immediately. Instead, they will be removed when the animation is finished. This can cause incorrect index or even crashes.
Solution:
A mark is used to adjust the index. Using an delete-mark to represent the view going to be deleted can help find out the correct index when the animation finish and therefore fix the problem. In mDeleteMark, Integer(0) represents a normal view while Integer(1) represents the view that should be removed but because of animation it should be removed later. The true index can be gotten by calculating the number of Integer(1).
Issue:App crash with error "index=4 count=3" using LayoutAnimation.easeInEaseOut() on Android #17118
Changelog
[Android][Fixed] - Fix the IndexOutOfBoundsException when wrong tag is used in manageChildren because of animation.
Test Plan