Fix ExpandableView's transient container tracking; prevent crash when mismatched.
Fixes: 213038631
Test: manual dismissing; adding to groups
Signed-off-by: Jeff DeCew <jeffdq@google.com>
Change-Id: Iedad4a54fc44e9ff900357b3486cc62516701c45
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableView.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableView.java
index ab36da5..6218c77 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableView.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableView.java
@@ -520,26 +520,66 @@
}
/**
+ * Called when removing a view from its transient container, such as at the end of an animation.
+ * Generally, when operating on ExpandableView instances, this should be used rather than
+ * {@link ExpandableView#removeTransientView(View)} to ensure that the
+ * {@link #getTransientContainer() transient container} is correctly reset.
+ */
+ public void removeFromTransientContainer() {
+ final ViewGroup transientContainer = getTransientContainer();
+ if (transientContainer == null) {
+ return;
+ }
+ final ViewParent parent = getParent();
+ if (parent != transientContainer) {
+ Log.w(TAG, "Expandable view " + this
+ + " has transient container " + transientContainer
+ + " but different parent " + parent);
+ setTransientContainer(null);
+ return;
+ }
+ transientContainer.removeTransientView(this);
+ setTransientContainer(null);
+ }
+
+ /**
* Called before adding this view to a group, which would always throw an exception if this view
- * has a parent, so clean up the transient container and throw an exception if the parent isn't
- * a transient container. Provide as much detail in the event of a crash as possible.
+ * has a different parent, so clean up the transient container and throw an exception if the
+ * parent isn't a transient container. Provide as much detail as possible in the crash.
*/
public void removeFromTransientContainerForAdditionTo(ViewGroup newParent) {
final ViewParent parent = getParent();
+ final ViewGroup transientContainer = getTransientContainer();
if (parent == null) {
- // If this view has no parent, the add will succeed, so do nothing.
+ // If this view has no parent, the add will succeed, so just make sure the tracked
+ // transient container is in sync with the lack of a parent.
+ if (transientContainer != null) {
+ Log.w(TAG, "Expandable view " + this
+ + " has transient container " + transientContainer
+ + " but no parent");
+ setTransientContainer(null);
+ }
return;
}
- ViewGroup transientContainer = getTransientContainer();
if (transientContainer == null) {
throw new IllegalStateException(
"Can't add view " + this + " to container " + newParent + "; current parent "
+ parent + " is not a transient container");
}
if (transientContainer != parent) {
- throw new IllegalStateException(
- "Expandable view " + this + " has transient container " + transientContainer
- + " which is not the same as its parent " + parent);
+ String transientContainerOutOfSyncError = "Expandable view " + this
+ + " has transient container " + transientContainer
+ + " but different parent " + parent;
+ if (parent != newParent) {
+ // Crash with details before addView() crashes without any; the view is being added
+ // to a different parent, and the transient container isn't the parent, so we can't
+ // even (safely) clean that up.
+ throw new IllegalStateException(transientContainerOutOfSyncError);
+ } else {
+ Log.w(TAG, transientContainerOutOfSyncError);
+ setTransientContainer(null);
+ return;
+ }
}
if (parent != newParent) {
Log.w(TAG, "Moving view " + this + " from transient container "
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManager.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManager.kt
index 88198f3..1d90780 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManager.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManager.kt
@@ -215,8 +215,7 @@
// TODO: We should really cancel the active animations here. This will
// happen automatically when the view's intro animation starts, but
// it's a fragile link.
- header.transientContainer?.removeTransientView(header)
- header.transientContainer = null
+ header.removeFromTransientContainer()
parent.addView(header, target)
} else {
parent.changeViewPosition(header, target)
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java
index 3e9ce25..943f05f 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java
@@ -3213,10 +3213,7 @@
// Clean up any potential transient views if the child has already been swiped
// out, as we won't be animating it further (due to its height already being
// clipped to 0.
- ViewGroup transientContainer = child.getTransientContainer();
- if (transientContainer != null) {
- transientContainer.removeTransientView(child);
- }
+ child.removeFromTransientContainer();
}
}
int animationType = childWasSwipedOut
@@ -3933,7 +3930,11 @@
@ShadeViewRefactor(RefactorComponent.STATE_RESOLVER)
private void clearTemporaryViewsInGroup(ViewGroup viewGroup) {
while (viewGroup != null && viewGroup.getTransientViewCount() != 0) {
- viewGroup.removeTransientView(viewGroup.getTransientView(0));
+ final View transientView = viewGroup.getTransientView(0);
+ viewGroup.removeTransientView(transientView);
+ if (transientView instanceof ExpandableView) {
+ ((ExpandableView) transientView).setTransientContainer(null);
+ }
}
}
@@ -4102,7 +4103,7 @@
@ShadeViewRefactor(RefactorComponent.STATE_RESOLVER)
private void clearTransient() {
for (ExpandableView view : mClearTransientViewsWhenFinished) {
- StackStateAnimator.removeTransientView(view);
+ view.removeFromTransientContainer();
}
mClearTransientViewsWhenFinished.clear();
}
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutController.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutController.java
index 41a80c7..ff36c9f 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutController.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutController.java
@@ -454,10 +454,7 @@
if (!row.isDismissed()) {
handleChildViewDismissed(view);
}
- ViewGroup transientContainer = row.getTransientContainer();
- if (transientContainer != null) {
- transientContainer.removeTransientView(view);
- }
+ row.removeFromTransientContainer();
}
/**
diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/StackStateAnimator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/StackStateAnimator.java
index 2dc9276..1d0d374 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/StackStateAnimator.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/StackStateAnimator.java
@@ -322,9 +322,8 @@
private void onAnimationFinished() {
mHostLayout.onChildAnimationFinished();
- for (ExpandableView transientViewsToRemove : mTransientViewsToRemove) {
- transientViewsToRemove.getTransientContainer()
- .removeTransientView(transientViewsToRemove);
+ for (ExpandableView transientViewToRemove : mTransientViewsToRemove) {
+ transientViewToRemove.removeFromTransientContainer();
}
mTransientViewsToRemove.clear();
}
@@ -353,7 +352,7 @@
} else if (event.animationType ==
NotificationStackScrollLayout.AnimationEvent.ANIMATION_TYPE_REMOVE) {
if (changingView.getVisibility() != View.VISIBLE) {
- removeTransientView(changingView);
+ changingView.removeFromTransientContainer();
continue;
}
@@ -390,12 +389,11 @@
}
changingView.performRemoveAnimation(ANIMATION_DURATION_APPEAR_DISAPPEAR,
0 /* delay */, translationDirection, false /* isHeadsUpAppear */,
- 0, () -> removeTransientView(changingView), null);
+ 0, changingView::removeFromTransientContainer, null);
} else if (event.animationType ==
NotificationStackScrollLayout.AnimationEvent.ANIMATION_TYPE_REMOVE_SWIPED_OUT) {
- if (mHostLayout.isFullySwipedOut(changingView)
- && changingView.getTransientContainer() != null) {
- changingView.getTransientContainer().removeTransientView(changingView);
+ if (mHostLayout.isFullySwipedOut(changingView)) {
+ changingView.removeFromTransientContainer();
}
} else if (event.animationType == NotificationStackScrollLayout
.AnimationEvent.ANIMATION_TYPE_GROUP_EXPANSION_CHANGED) {
@@ -425,7 +423,7 @@
mHostLayout.addTransientView(changingView, 0);
changingView.setTransientContainer(mHostLayout);
mTmpState.initFrom(changingView);
- endRunnable = () -> removeTransientView(changingView);
+ endRunnable = changingView::removeFromTransientContainer;
}
float targetLocation = 0;
boolean needsAnimation = true;
@@ -468,12 +466,6 @@
}
}
- public static void removeTransientView(ExpandableView viewToRemove) {
- if (viewToRemove.getTransientContainer() != null) {
- viewToRemove.getTransientContainer().removeTransientView(viewToRemove);
- }
- }
-
public void animateOverScrollToAmount(float targetAmount, final boolean onTop,
final boolean isRubberbanded) {
final float startOverScrollAmount = mHostLayout.getCurrentOverScrollAmount(onTop);
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManagerTest.java
index 276f246..4d2c0c3 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManagerTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationSectionsManagerTest.java
@@ -32,6 +32,7 @@
import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
@@ -46,9 +47,7 @@
import androidx.test.filters.SmallTest;
-import com.android.systemui.ActivityStarterDelegate;
import com.android.systemui.SysuiTestCase;
-import com.android.systemui.flags.FeatureFlags;
import com.android.systemui.media.KeyguardMediaController;
import com.android.systemui.plugins.statusbar.StatusBarStateController;
import com.android.systemui.statusbar.StatusBarState;
@@ -56,7 +55,6 @@
import com.android.systemui.statusbar.notification.NotificationSectionsFeatureManager;
import com.android.systemui.statusbar.notification.collection.NotificationEntry;
import com.android.systemui.statusbar.notification.collection.render.SectionHeaderController;
-import com.android.systemui.statusbar.notification.people.PeopleHubViewAdapter;
import com.android.systemui.statusbar.notification.row.ActivatableNotificationViewController;
import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow;
import com.android.systemui.statusbar.notification.row.dagger.NotificationRowComponent;
@@ -66,6 +64,7 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mockito.InOrder;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;
@@ -266,8 +265,9 @@
// THEN the header is first removed from the transient parent before being added to the
// NSSL.
- verify(transientParent).removeTransientView(silentHeaderView);
- verify(mNssl).addView(silentHeaderView, 1);
+ final InOrder inOrder = inOrder(silentHeaderView, mNssl);
+ inOrder.verify(silentHeaderView).removeFromTransientContainer();
+ inOrder.verify(mNssl).addView(eq(silentHeaderView), eq(1));
}
@Test
