From bd3023abeadf9b6dfbe18cbac649961e0efe73f0 Mon Sep 17 00:00:00 2001 From: Luna Wei Date: Tue, 28 May 2019 09:05:57 -0700 Subject: [PATCH] Layout Animation fix for normalized indices Summary: [Android][Fix] - Fix how we normalize indices Before, we were incorrectly normalizing indices given pending view deletion in the view hierarchy (namely, using LayoutAnimations) What we had before (that was wrong): * Maintained a pendingIndices sparse array for each tag * For each pendingIndices sparse array we'd keep track of how many views we deleted at each abstract index * Given an abstract index to delete a view at, we'd consult `pendingIndices` array to sum how many pending deletes we had for indices equal or smaller than and add to abstract index ^ Above algorithm is wrong and you can follow along with the following example to see how. ## The correct approach Given these operations in this order: 1. {tagsToDelete: [123], indicesToDelete [2]} 2. {tagsToDelete: [124], indicesToDelete [1]} 3. {tagsToDelete: [125], indicesToDelete [2]} 4. {tagsToDelete: [126], indicesToDelete [1]} The approach we want to be using to calculate normalized indices: ### Step 1: Delete tag 124 at index 2 |Views:|122|123|124|125|126|127| |Actual Indices:|0|1|2|3|4|5| |Abstract Indices:|0|1|2|3|4|5| => simple, we just mark the view at 2 ### Step 2: Delete tag 123 at index 1 View tags and indices: |Views|122|123|~~124~~|125|126|127| |Actual indices|0|1|~~2~~|3|4|5| |Abstract Indices|0|1||2|3|4| => again, simple, we can just use the normalized index 1 because no pending deletes affect this operation ### Step 3: Delete tag 126 at index 2 View tags and indices: |Views|122|~~123~~|~~124~~|125|126|127| |Actual Indices|0|~~1~~|~~2~~|3|4|5| |Abstract Indices|0|||1|2|3| => Here we want to normalize this index to 4 because we need to account the 2 views that should be skipped ### Step 4: Delete tag 125 at index 1 View tags and indices: |Views|122|~~123~~|~~124~~|125|~~126~~|127| |Actual Indices|0|~~1~~|~~2~~|3|~~4~~|5| |Abstract Indices|0|||1||2| => The normalized index should be 3. This diff updates the function `normalizeIndex` to do the above algorithm by repurposing `pendingIndicesToDelete` to instead be a sparse int array that holds [normalizedIndex]=[tag] pairs It's required that `pendingIndicesToDelete` is ordered by the normalizedIndex. Reviewed By: mdvacca Differential Revision: D15485132 fbshipit-source-id: 43e57dffa807e8ea50fa1650c5dec13a6fded624 --- .../uimanager/NativeViewHierarchyManager.java | 54 +++++++++++++++---- 1 file changed, 44 insertions(+), 10 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 48d1daefd5..4c6c626d6e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -338,17 +338,52 @@ public class NativeViewHierarchyManager { } /** - * 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) + * Given an index, normalize against pending view deletion indices in the native view hierarchy + * @param index the index in the view operation under the assumption all view operations are synchronous + * @param pendingIndicesToDelete sparse array of view tags at normalized indices */ - private int normalizeIndex(int index, SparseIntArray pendingIndices) { - int normalizedIndex = index; - for (int i = 0; i <= index; i++) { - normalizedIndex += pendingIndices.get(i); + private int normalizeIndex(int index, SparseIntArray pendingIndicesToDelete) { + int normalizedIndex = -1; + while (index >= 0) { + normalizedIndex += 1; + if (pendingIndicesToDelete.get(normalizedIndex, -1) == -1) { // assuming we never have negative tag + index--; + } } return normalizedIndex; } + /** + * Add view tag to pendingIndicesToDelete. Views in pendingIndicesToDelete are marked for deletion but have not been deleted yet + * @param index the index in the view to be deleted as provided by the view operation + * @param tag the view tag + * @param pendingIndicesToDelete sparse array of normalizedIndices to view tags marked for deletion + */ + private void addPendingIndex(int index, int tag, SparseIntArray pendingIndicesToDelete) { + int normalizedIndex = normalizeIndex(index, pendingIndicesToDelete); + if (pendingIndicesToDelete.get(normalizedIndex) > 0) { + throw new IllegalViewOperationException("Invalid!!"); + } + pendingIndicesToDelete.put(normalizedIndex, tag); + } + + /** + * When delete is completed, remove view from pendingIndicesToDelete + * @param tag tag of the view to be removed + * @param pendingIndicesToDelete sparse array of normalizedIndices to view tags marked for deletion + */ + private void removePendingIndex(int tag, SparseIntArray pendingIndicesToDelete) { + // indexAt refers to index within pendingIndicesToDelete sparse array, not the normalized index + int indexAt = pendingIndicesToDelete.indexOfValue(tag); + pendingIndicesToDelete.removeAt(indexAt); + for (indexAt = indexAt + 1; indexAt < pendingIndicesToDelete.size(); indexAt ++) { + int nextTag = pendingIndicesToDelete.valueAt(indexAt); + int nextKey = pendingIndicesToDelete.keyAt(indexAt); + pendingIndicesToDelete.removeAt(indexAt); + pendingIndicesToDelete.put(nextKey - 1, nextTag); + } + } + /** * Given React tag, return sparse array of direct child indices that are pending deletion (due to * async view deletion) @@ -471,8 +506,8 @@ public class NativeViewHierarchyManager { if (mLayoutAnimationEnabled && mLayoutAnimator.shouldAnimateLayout(viewToDestroy)) { - int updatedCount = pendingIndicesToDelete.get(indexToDelete, 0) + 1; - pendingIndicesToDelete.put(indexToDelete, updatedCount); + + addPendingIndex(indexToDelete, tagToDelete, pendingIndicesToDelete); mLayoutAnimator.deleteView( viewToDestroy, new LayoutAnimationListener() { @@ -481,8 +516,7 @@ public class NativeViewHierarchyManager { viewManager.removeView(viewToManage, viewToDestroy); dropView(viewToDestroy); - int count = pendingIndicesToDelete.get(indexToDelete, 0); - pendingIndicesToDelete.put(indexToDelete, Math.max(0, count - 1)); + removePendingIndex(viewToDestroy.getId(), pendingIndicesToDelete); } }); } else {