From 5f027ec64d6764fbbb9813fabb373194dec79db7 Mon Sep 17 00:00:00 2001 From: Luna Wei Date: Mon, 6 May 2019 08:31:07 -0700 Subject: [PATCH] Rearrange order of manageChildren Summary: [General] [Fix] - Reorder operations of native view hierarchy When we update the native view hierarchy in `manageChildren` we: 1. iterate through all views we plan to remove and remove them from the native hierarchy 2. iterate through all views we plan to add and add them to the native hierarchy 3. iterate through all views we plan to delete and delete them (remove them from memory) This covers these cases: a. A view is moved from one place to another -- handled by steps 1 & 2 b. A view is added -- handled by step 2 c. A view is deleted -- handled by step 1 & 3 > Note the difference between remove and delete Everything above sounds fine! But... The important bit: **A view that is going to be deleted asynchronously (by a layout animation) is NOT removed in step 1. It is removed and deleted all in step 3.** See: https://fburl.com/ryxp626i If the reader may recall we solved a similar problem in D14529038 where we introduced the `pendingIndicesToDelete` data structure to keep track of views that were marked for deletion but had not yet been deleted. An example of an order of operations that we would've solved with D14529038 is: * we "delete" the view asynchronously (view A) in one operation * we add a view B that shares a parent with view A in subsequent operation * view A finally calls its callback after the animation is complete and removes itself from the native view hierarchy A case that D14529038 would not fix: 1. we add a view B in one operation 2. we delete a view A in the same operation asynchronously because it's in a layout animation 3. ... etc. What we must remember is that the index we use to add view B in step 1 is based on the indices assuming that all deletions are synchronous as this [comment notes](https://fburl.com/j9uillje): removals (both deletions and moveFroms) use the indices of the current order of the views and are assumed independent of each other. Whereas additions are indexed on the updated order (after deletions!) This diff re-arranges the order in how we act upon the operations to update the native view hierarchy -- similar to how UIImplementation does its operations on the shadow tree here: https://fburl.com/j9uillje By doing the removals and deletions first, we know that the addAt indices will be correct because either the view is removed from the native view hierarchy or `pendingIndicesToDelete` should be tracking an async delete already so the addAt index will be normalized Reviewed By: mdvacca Differential Revision: D15112664 fbshipit-source-id: 85d4b21211ac802183ca2f0fd28fc4437d312100 --- .../uimanager/NativeViewHierarchyManager.java | 78 +++++++++---------- 1 file changed, 39 insertions(+), 39 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 3498dec6a79303..cec15412ed3aa7 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -446,6 +446,45 @@ public synchronized void manageChildren( } } + 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( + "Trying to destroy unknown view tag: " + + tagToDelete + "\n detail: " + + constructManageChildrenErrorMessage( + viewToManage, + viewManager, + indicesToRemove, + viewsToAdd, + tagsToDelete)); + } + + if (mLayoutAnimationEnabled && + mLayoutAnimator.shouldAnimateLayout(viewToDestroy)) { + int updatedCount = pendingIndicesToDelete.get(indexToDelete, 0) + 1; + pendingIndicesToDelete.put(indexToDelete, updatedCount); + mLayoutAnimator.deleteView( + viewToDestroy, + new LayoutAnimationListener() { + @Override + public void onAnimationEnd() { + viewManager.removeView(viewToManage, viewToDestroy); + dropView(viewToDestroy); + + int count = pendingIndicesToDelete.get(indexToDelete, 0); + pendingIndicesToDelete.put(indexToDelete, Math.max(0, count - 1)); + } + }); + } else { + dropView(viewToDestroy); + } + } + } + if (viewsToAdd != null) { for (int i = 0; i < viewsToAdd.length; i++) { ViewAtIndex viewAtIndex = viewsToAdd[i]; @@ -465,45 +504,6 @@ public synchronized void manageChildren( viewManager.addView(viewToManage, viewToAdd, normalizedIndexToAdd); } } - - 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( - "Trying to destroy unknown view tag: " - + tagToDelete + "\n detail: " + - constructManageChildrenErrorMessage( - viewToManage, - viewManager, - indicesToRemove, - viewsToAdd, - tagsToDelete)); - } - - if (mLayoutAnimationEnabled && - mLayoutAnimator.shouldAnimateLayout(viewToDestroy)) { - int updatedCount = pendingIndicesToDelete.get(indexToDelete, 0) + 1; - pendingIndicesToDelete.put(indexToDelete, updatedCount); - mLayoutAnimator.deleteView( - viewToDestroy, - new LayoutAnimationListener() { - @Override - public void onAnimationEnd() { - viewManager.removeView(viewToManage, viewToDestroy); - dropView(viewToDestroy); - - int count = pendingIndicesToDelete.get(indexToDelete, 0); - pendingIndicesToDelete.put(indexToDelete, Math.max(0, count - 1)); - } - }); - } else { - dropView(viewToDestroy); - } - } - } } private boolean arrayContains(@Nullable int[] array, int ele) {