Skip to content

Commit d3b93f7

Browse files
Luna Weifacebook-github-bot
authored andcommitted
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
1 parent 42c1957 commit d3b93f7

File tree

4 files changed

+12
-67
lines changed

4 files changed

+12
-67
lines changed

ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java

Lines changed: 4 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import android.graphics.RectF;
1313
import android.util.SparseArray;
1414
import android.util.SparseBooleanArray;
15-
import android.util.SparseIntArray;
1615
import android.view.Menu;
1716
import android.view.MenuItem;
1817
import android.view.View;
@@ -75,7 +74,6 @@ public class NativeViewHierarchyManager {
7574
private final JSResponderHandler mJSResponderHandler = new JSResponderHandler();
7675
private final RootViewManager mRootViewManager;
7776
private final LayoutAnimationController mLayoutAnimator = new LayoutAnimationController();
78-
private final SparseArray<SparseIntArray> mTagsToPendingIndicesToDelete = new SparseArray<>();
7977
private final int[] mDroppedViewArray = new int[100];
8078
private final RectF mBoundingBox = new RectF();
8179

@@ -351,49 +349,20 @@ private static String constructManageChildrenErrorMessage(
351349
return stringBuilder.toString();
352350
}
353351

354-
/**
355-
* Given an index to action on under synchronous deletes, return an updated index factoring in
356-
* asynchronous deletes (where the async delete operations have not yet been performed)
357-
*/
358-
private int normalizeIndex(int index, SparseIntArray pendingIndices) {
359-
int normalizedIndex = index;
360-
for (int i = 0; i <= index; i++) {
361-
normalizedIndex += pendingIndices.get(i);
362-
}
363-
return normalizedIndex;
364-
}
365-
366-
/**
367-
* Given React tag, return sparse array of direct child indices that are pending deletion (due to
368-
* async view deletion)
369-
*/
370-
private SparseIntArray getOrCreatePendingIndicesToDelete(int tag) {
371-
SparseIntArray pendingIndicesToDelete = mTagsToPendingIndicesToDelete.get(tag);
372-
if (pendingIndicesToDelete == null) {
373-
pendingIndicesToDelete = new SparseIntArray();
374-
mTagsToPendingIndicesToDelete.put(tag, pendingIndicesToDelete);
375-
}
376-
return pendingIndicesToDelete;
377-
}
378-
379352
/**
380353
* @param tag react tag of the node we want to manage
381354
* @param indicesToRemove ordered (asc) list of indicies at which view should be removed
382355
* @param viewsToAdd ordered (asc based on mIndex property) list of tag-index pairs that represent
383356
* a view which should be added at the specified index
384357
* @param tagsToDelete list of tags corresponding to views that should be removed
385-
* @param indicesToDelete parallel list to tagsToDelete, list of indices of those tags
386358
*/
387359
public synchronized void manageChildren(
388360
int tag,
389361
@Nullable int[] indicesToRemove,
390362
@Nullable ViewAtIndex[] viewsToAdd,
391-
@Nullable int[] tagsToDelete,
392-
@Nullable int[] indicesToDelete) {
363+
@Nullable int[] tagsToDelete) {
393364
UiThreadUtil.assertOnUiThread();
394365

395-
final SparseIntArray pendingIndicesToDelete = getOrCreatePendingIndicesToDelete(tag);
396-
397366
final ViewGroup viewToManage = (ViewGroup) mTagsToViews.get(tag);
398367
final ViewGroupManager viewManager = (ViewGroupManager) resolveViewManager(tag);
399368
if (viewToManage == null) {
@@ -446,16 +415,15 @@ public synchronized void manageChildren(
446415
viewToManage, viewManager, indicesToRemove, viewsToAdd, tagsToDelete));
447416
}
448417

449-
int normalizedIndexToRemove = normalizeIndex(indexToRemove, pendingIndicesToDelete);
450-
View viewToRemove = viewManager.getChildAt(viewToManage, normalizedIndexToRemove);
418+
View viewToRemove = viewManager.getChildAt(viewToManage, indexToRemove);
451419

452420
if (mLayoutAnimationEnabled
453421
&& mLayoutAnimator.shouldAnimateLayout(viewToRemove)
454422
&& arrayContains(tagsToDelete, viewToRemove.getId())) {
455423
// The view will be removed and dropped by the 'delete' layout animation
456424
// instead, so do nothing
457425
} else {
458-
viewManager.removeViewAt(viewToManage, normalizedIndexToRemove);
426+
viewManager.removeViewAt(viewToManage, indexToRemove);
459427
}
460428

461429
lastIndexToRemove = indexToRemove;
@@ -465,7 +433,6 @@ && arrayContains(tagsToDelete, viewToRemove.getId())) {
465433
if (tagsToDelete != null) {
466434
for (int i = 0; i < tagsToDelete.length; i++) {
467435
int tagToDelete = tagsToDelete[i];
468-
final int indexToDelete = indicesToDelete[i];
469436
final View viewToDestroy = mTagsToViews.get(tagToDelete);
470437
if (viewToDestroy == null) {
471438
throw new IllegalViewOperationException(
@@ -477,8 +444,6 @@ && arrayContains(tagsToDelete, viewToRemove.getId())) {
477444
}
478445

479446
if (mLayoutAnimationEnabled && mLayoutAnimator.shouldAnimateLayout(viewToDestroy)) {
480-
int updatedCount = pendingIndicesToDelete.get(indexToDelete, 0) + 1;
481-
pendingIndicesToDelete.put(indexToDelete, updatedCount);
482447
mLayoutAnimator.deleteView(
483448
viewToDestroy,
484449
new LayoutAnimationListener() {
@@ -490,9 +455,6 @@ public void onAnimationEnd() {
490455

491456
viewManager.removeView(viewToManage, viewToDestroy);
492457
dropView(viewToDestroy);
493-
494-
int count = pendingIndicesToDelete.get(indexToDelete, 0);
495-
pendingIndicesToDelete.put(indexToDelete, Math.max(0, count - 1));
496458
}
497459
});
498460
} else {
@@ -513,8 +475,7 @@ public void onAnimationEnd() {
513475
+ constructManageChildrenErrorMessage(
514476
viewToManage, viewManager, indicesToRemove, viewsToAdd, tagsToDelete));
515477
}
516-
int normalizedIndexToAdd = normalizeIndex(viewAtIndex.mIndex, pendingIndicesToDelete);
517-
viewManager.addView(viewToManage, viewToAdd, normalizedIndexToAdd);
478+
viewManager.addView(viewToManage, viewToAdd, viewAtIndex.mIndex);
518479
}
519480
}
520481
}
@@ -624,7 +585,6 @@ protected synchronized void dropView(View view) {
624585
}
625586
viewGroupManager.removeAllViews(viewGroup);
626587
}
627-
mTagsToPendingIndicesToDelete.remove(view.getId());
628588
mTagsToViews.remove(view.getId());
629589
mTagsToViewManagers.remove(view.getId());
630590
}

ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyOptimizer.java

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -141,12 +141,11 @@ public void handleManageChildren(
141141
int[] indicesToRemove,
142142
int[] tagsToRemove,
143143
ViewAtIndex[] viewsToAdd,
144-
int[] tagsToDelete,
145-
int[] indicesToDelete) {
144+
int[] tagsToDelete) {
146145
if (!ENABLED) {
147146
assertNodeSupportedWithoutOptimizer(nodeToManage);
148147
mUIViewOperationQueue.enqueueManageChildren(
149-
nodeToManage.getReactTag(), indicesToRemove, viewsToAdd, tagsToDelete, indicesToDelete);
148+
nodeToManage.getReactTag(), indicesToRemove, viewsToAdd, tagsToDelete);
150149
return;
151150
}
152151

@@ -284,8 +283,7 @@ private void removeNodeFromParent(ReactShadowNode nodeToRemove, boolean shouldDe
284283
nativeNodeToRemoveFrom.getReactTag(),
285284
new int[] {index},
286285
null,
287-
shouldDelete ? new int[] {nodeToRemove.getReactTag()} : null,
288-
shouldDelete ? new int[] {index} : null);
286+
shouldDelete ? new int[] {nodeToRemove.getReactTag()} : null);
289287
}
290288
}
291289

@@ -300,7 +298,6 @@ private void addNativeChild(ReactShadowNode parent, ReactShadowNode child, int i
300298
parent.getReactTag(),
301299
null,
302300
new ViewAtIndex[] {new ViewAtIndex(child.getReactTag(), index)},
303-
null,
304301
null);
305302

306303
if (child.getNativeKind() != NativeKind.PARENT) {

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,6 @@ public void manageChildren(
333333
int[] indicesToRemove = new int[numToMove + numToRemove];
334334
int[] tagsToRemove = new int[indicesToRemove.length];
335335
int[] tagsToDelete = new int[numToRemove];
336-
int[] indicesToDelete = new int[numToRemove];
337336

338337
if (numToMove > 0) {
339338
Assertions.assertNotNull(moveFrom);
@@ -365,7 +364,6 @@ public void manageChildren(
365364
indicesToRemove[numToMove + i] = indexToRemove;
366365
tagsToRemove[numToMove + i] = tagToRemove;
367366
tagsToDelete[i] = tagToRemove;
368-
indicesToDelete[i] = indexToRemove;
369367
}
370368
}
371369

@@ -405,12 +403,7 @@ public void manageChildren(
405403
}
406404

407405
mNativeViewHierarchyOptimizer.handleManageChildren(
408-
cssNodeToManage,
409-
indicesToRemove,
410-
tagsToRemove,
411-
viewsToAdd,
412-
tagsToDelete,
413-
indicesToDelete);
406+
cssNodeToManage, indicesToRemove, tagsToRemove, viewsToAdd, tagsToDelete);
414407

415408
for (int i = 0; i < tagsToDelete.length; i++) {
416409
removeShadowNode(mShadowNodeRegistry.getNode(tagsToDelete[i]));

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -186,25 +186,22 @@ private final class ManageChildrenOperation extends ViewOperation {
186186
private final @Nullable int[] mIndicesToRemove;
187187
private final @Nullable ViewAtIndex[] mViewsToAdd;
188188
private final @Nullable int[] mTagsToDelete;
189-
private final @Nullable int[] mIndicesToDelete;
190189

191190
public ManageChildrenOperation(
192191
int tag,
193192
@Nullable int[] indicesToRemove,
194193
@Nullable ViewAtIndex[] viewsToAdd,
195-
@Nullable int[] tagsToDelete,
196-
@Nullable int[] indicesToDelete) {
194+
@Nullable int[] tagsToDelete) {
197195
super(tag);
198196
mIndicesToRemove = indicesToRemove;
199197
mViewsToAdd = viewsToAdd;
200198
mTagsToDelete = tagsToDelete;
201-
mIndicesToDelete = indicesToDelete;
202199
}
203200

204201
@Override
205202
public void execute() {
206203
mNativeViewHierarchyManager.manageChildren(
207-
mTag, mIndicesToRemove, mViewsToAdd, mTagsToDelete, mIndicesToDelete);
204+
mTag, mIndicesToRemove, mViewsToAdd, mTagsToDelete);
208205
}
209206
}
210207

@@ -685,11 +682,9 @@ public void enqueueManageChildren(
685682
int reactTag,
686683
@Nullable int[] indicesToRemove,
687684
@Nullable ViewAtIndex[] viewsToAdd,
688-
@Nullable int[] tagsToDelete,
689-
@Nullable int[] indicesToDelete) {
685+
@Nullable int[] tagsToDelete) {
690686
mOperations.add(
691-
new ManageChildrenOperation(
692-
reactTag, indicesToRemove, viewsToAdd, tagsToDelete, indicesToDelete));
687+
new ManageChildrenOperation(reactTag, indicesToRemove, viewsToAdd, tagsToDelete));
693688
}
694689

695690
public void enqueueSetChildren(int reactTag, ReadableArray childrenTags) {

0 commit comments

Comments
 (0)