From d3b93f75787267629c4033a5aa61b62d623f3c08 Mon Sep 17 00:00:00 2001 From: Luna Wei Date: Mon, 9 Mar 2020 15:53:19 -0700 Subject: [PATCH] Remove all layout index adjustments Summary: Changelog: [Internal] # Context Timeline * ~March 2019 landed D14529038 (I'll be referring to this as "index adjustment fix") which attempted to fix a reproducible issue with layout animations: P127130177, see Spencer's diff for more context: D14245985 * May 2019 I realized that "index adjustment fix" has a bug in it and attempted to fix with D15485132, but was eventually reverted because of other crashes * Just recently have been getting tasks related to crashes that are attempting to either remove or add a view that is out of bounds which is caused by invalid index because of the "index adjustment fix". # What is this diff doing? I'm removing the "index adjustment fix" because I found that the original layout animation repro, P127130177, no longer repros on latest master with the "index adjustment fix" reverted. Additionally, I've found a consistent crash in (RN bookmark > Sample Integration App > Relay Sample Friends) of a bad view deletion because of the "index adjustment fix" Removing the index adjustment fix may have effects elsewhere but it seems better to remove this and go back to what layout animations was doing a year ago than to continue on in this half-baked state. Reviewed By: JoshuaGross Differential Revision: D20323928 fbshipit-source-id: ba4a222915add00e98a9936ba2a8efc4006fb8e3 --- .../uimanager/NativeViewHierarchyManager.java | 48 ++----------------- .../NativeViewHierarchyOptimizer.java | 9 ++-- .../react/uimanager/UIImplementation.java | 9 +--- .../react/uimanager/UIViewOperationQueue.java | 13 ++--- 4 files changed, 12 insertions(+), 67 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java index 412fe269e0d..3e70a3553b6 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -12,7 +12,6 @@ import android.graphics.Matrix; import android.graphics.RectF; import android.util.SparseArray; import android.util.SparseBooleanArray; -import android.util.SparseIntArray; import android.view.Menu; import android.view.MenuItem; import android.view.View; @@ -75,7 +74,6 @@ public class NativeViewHierarchyManager { private final JSResponderHandler mJSResponderHandler = new JSResponderHandler(); private final RootViewManager mRootViewManager; private final LayoutAnimationController mLayoutAnimator = new LayoutAnimationController(); - private final SparseArray mTagsToPendingIndicesToDelete = new SparseArray<>(); private final int[] mDroppedViewArray = new int[100]; private final RectF mBoundingBox = new RectF(); @@ -351,49 +349,20 @@ public class NativeViewHierarchyManager { return stringBuilder.toString(); } - /** - * Given an index to action on under synchronous deletes, return an updated index factoring in - * asynchronous deletes (where the async delete operations have not yet been performed) - */ - private int normalizeIndex(int index, SparseIntArray pendingIndices) { - int normalizedIndex = index; - for (int i = 0; i <= index; i++) { - normalizedIndex += pendingIndices.get(i); - } - return normalizedIndex; - } - - /** - * Given React tag, return sparse array of direct child indices that are pending deletion (due to - * async view deletion) - */ - private SparseIntArray getOrCreatePendingIndicesToDelete(int tag) { - SparseIntArray pendingIndicesToDelete = mTagsToPendingIndicesToDelete.get(tag); - if (pendingIndicesToDelete == null) { - pendingIndicesToDelete = new SparseIntArray(); - mTagsToPendingIndicesToDelete.put(tag, pendingIndicesToDelete); - } - return pendingIndicesToDelete; - } - /** * @param tag react tag of the node we want to manage * @param indicesToRemove ordered (asc) list of indicies at which view should be removed * @param viewsToAdd ordered (asc based on mIndex property) list of tag-index pairs that represent * a view which should be added at the specified index * @param tagsToDelete list of tags corresponding to views that should be removed - * @param indicesToDelete parallel list to tagsToDelete, list of indices of those tags */ public synchronized void manageChildren( int tag, @Nullable int[] indicesToRemove, @Nullable ViewAtIndex[] viewsToAdd, - @Nullable int[] tagsToDelete, - @Nullable int[] indicesToDelete) { + @Nullable int[] tagsToDelete) { UiThreadUtil.assertOnUiThread(); - final SparseIntArray pendingIndicesToDelete = getOrCreatePendingIndicesToDelete(tag); - final ViewGroup viewToManage = (ViewGroup) mTagsToViews.get(tag); final ViewGroupManager viewManager = (ViewGroupManager) resolveViewManager(tag); if (viewToManage == null) { @@ -446,8 +415,7 @@ public class NativeViewHierarchyManager { viewToManage, viewManager, indicesToRemove, viewsToAdd, tagsToDelete)); } - int normalizedIndexToRemove = normalizeIndex(indexToRemove, pendingIndicesToDelete); - View viewToRemove = viewManager.getChildAt(viewToManage, normalizedIndexToRemove); + View viewToRemove = viewManager.getChildAt(viewToManage, indexToRemove); if (mLayoutAnimationEnabled && mLayoutAnimator.shouldAnimateLayout(viewToRemove) @@ -455,7 +423,7 @@ public class NativeViewHierarchyManager { // The view will be removed and dropped by the 'delete' layout animation // instead, so do nothing } else { - viewManager.removeViewAt(viewToManage, normalizedIndexToRemove); + viewManager.removeViewAt(viewToManage, indexToRemove); } lastIndexToRemove = indexToRemove; @@ -465,7 +433,6 @@ public class NativeViewHierarchyManager { if (tagsToDelete != null) { for (int i = 0; i < tagsToDelete.length; i++) { int tagToDelete = tagsToDelete[i]; - final int indexToDelete = indicesToDelete[i]; final View viewToDestroy = mTagsToViews.get(tagToDelete); if (viewToDestroy == null) { throw new IllegalViewOperationException( @@ -477,8 +444,6 @@ public class NativeViewHierarchyManager { } if (mLayoutAnimationEnabled && mLayoutAnimator.shouldAnimateLayout(viewToDestroy)) { - int updatedCount = pendingIndicesToDelete.get(indexToDelete, 0) + 1; - pendingIndicesToDelete.put(indexToDelete, updatedCount); mLayoutAnimator.deleteView( viewToDestroy, new LayoutAnimationListener() { @@ -490,9 +455,6 @@ public class NativeViewHierarchyManager { viewManager.removeView(viewToManage, viewToDestroy); dropView(viewToDestroy); - - int count = pendingIndicesToDelete.get(indexToDelete, 0); - pendingIndicesToDelete.put(indexToDelete, Math.max(0, count - 1)); } }); } else { @@ -513,8 +475,7 @@ public class NativeViewHierarchyManager { + constructManageChildrenErrorMessage( viewToManage, viewManager, indicesToRemove, viewsToAdd, tagsToDelete)); } - int normalizedIndexToAdd = normalizeIndex(viewAtIndex.mIndex, pendingIndicesToDelete); - viewManager.addView(viewToManage, viewToAdd, normalizedIndexToAdd); + viewManager.addView(viewToManage, viewToAdd, viewAtIndex.mIndex); } } } @@ -624,7 +585,6 @@ public class NativeViewHierarchyManager { } viewGroupManager.removeAllViews(viewGroup); } - mTagsToPendingIndicesToDelete.remove(view.getId()); mTagsToViews.remove(view.getId()); mTagsToViewManagers.remove(view.getId()); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java index 2dc0276d539..a42f3501ad5 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java @@ -141,12 +141,11 @@ public class NativeViewHierarchyOptimizer { int[] indicesToRemove, int[] tagsToRemove, ViewAtIndex[] viewsToAdd, - int[] tagsToDelete, - int[] indicesToDelete) { + int[] tagsToDelete) { if (!ENABLED) { assertNodeSupportedWithoutOptimizer(nodeToManage); mUIViewOperationQueue.enqueueManageChildren( - nodeToManage.getReactTag(), indicesToRemove, viewsToAdd, tagsToDelete, indicesToDelete); + nodeToManage.getReactTag(), indicesToRemove, viewsToAdd, tagsToDelete); return; } @@ -284,8 +283,7 @@ public class NativeViewHierarchyOptimizer { nativeNodeToRemoveFrom.getReactTag(), new int[] {index}, null, - shouldDelete ? new int[] {nodeToRemove.getReactTag()} : null, - shouldDelete ? new int[] {index} : null); + shouldDelete ? new int[] {nodeToRemove.getReactTag()} : null); } } @@ -300,7 +298,6 @@ public class NativeViewHierarchyOptimizer { parent.getReactTag(), null, new ViewAtIndex[] {new ViewAtIndex(child.getReactTag(), index)}, - null, null); if (child.getNativeKind() != NativeKind.PARENT) { 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 3bdce77ef36..ea6d4894821 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java @@ -333,7 +333,6 @@ public class UIImplementation { int[] indicesToRemove = new int[numToMove + numToRemove]; int[] tagsToRemove = new int[indicesToRemove.length]; int[] tagsToDelete = new int[numToRemove]; - int[] indicesToDelete = new int[numToRemove]; if (numToMove > 0) { Assertions.assertNotNull(moveFrom); @@ -365,7 +364,6 @@ public class UIImplementation { indicesToRemove[numToMove + i] = indexToRemove; tagsToRemove[numToMove + i] = tagToRemove; tagsToDelete[i] = tagToRemove; - indicesToDelete[i] = indexToRemove; } } @@ -405,12 +403,7 @@ public class UIImplementation { } mNativeViewHierarchyOptimizer.handleManageChildren( - cssNodeToManage, - indicesToRemove, - tagsToRemove, - viewsToAdd, - tagsToDelete, - indicesToDelete); + cssNodeToManage, indicesToRemove, tagsToRemove, viewsToAdd, tagsToDelete); for (int i = 0; i < tagsToDelete.length; i++) { removeShadowNode(mShadowNodeRegistry.getNode(tagsToDelete[i])); 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 f70266faae2..da2576c048d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java @@ -186,25 +186,22 @@ public class UIViewOperationQueue { private final @Nullable int[] mIndicesToRemove; private final @Nullable ViewAtIndex[] mViewsToAdd; private final @Nullable int[] mTagsToDelete; - private final @Nullable int[] mIndicesToDelete; public ManageChildrenOperation( int tag, @Nullable int[] indicesToRemove, @Nullable ViewAtIndex[] viewsToAdd, - @Nullable int[] tagsToDelete, - @Nullable int[] indicesToDelete) { + @Nullable int[] tagsToDelete) { super(tag); mIndicesToRemove = indicesToRemove; mViewsToAdd = viewsToAdd; mTagsToDelete = tagsToDelete; - mIndicesToDelete = indicesToDelete; } @Override public void execute() { mNativeViewHierarchyManager.manageChildren( - mTag, mIndicesToRemove, mViewsToAdd, mTagsToDelete, mIndicesToDelete); + mTag, mIndicesToRemove, mViewsToAdd, mTagsToDelete); } } @@ -685,11 +682,9 @@ public class UIViewOperationQueue { int reactTag, @Nullable int[] indicesToRemove, @Nullable ViewAtIndex[] viewsToAdd, - @Nullable int[] tagsToDelete, - @Nullable int[] indicesToDelete) { + @Nullable int[] tagsToDelete) { mOperations.add( - new ManageChildrenOperation( - reactTag, indicesToRemove, viewsToAdd, tagsToDelete, indicesToDelete)); + new ManageChildrenOperation(reactTag, indicesToRemove, viewsToAdd, tagsToDelete)); } public void enqueueSetChildren(int reactTag, ReadableArray childrenTags) {