Skip to content

Commit

Permalink
Rearrange order of manageChildren
Browse files Browse the repository at this point in the history
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
Luna Wei authored and facebook-github-bot committed May 6, 2019
1 parent 38483d4 commit 5f027ec
Showing 1 changed file with 39 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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) {
Expand Down

2 comments on commit 5f027ec

@turnrye
Copy link
Contributor

@turnrye turnrye commented on 5f027ec Jun 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lunaleaps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turnrye that's right! My mistake

Please sign in to comment.