Skip to content
Permalink
Browse files

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
  • Loading branch information...
lunaleaps authored and facebook-github-bot committed May 6, 2019
1 parent 38483d4 commit 5f027ec64d6764fbbb9813fabb373194dec79db7
@@ -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) {

2 comments on commit 5f027ec

@turnrye

This comment has been minimized.

Copy link
Contributor

replied Jun 2, 2019

Hey @lunaleaps, it looks like this change is actually just [Android] rather than [General]; do I understand that right?

@lunaleaps

This comment has been minimized.

Copy link
Contributor Author

replied Jun 3, 2019

@turnrye that's right! My mistake

Please sign in to comment.
You can’t perform that action at this time.