From 506a24fa7169ce83e29c23e1682bb9ecc44a6521 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Wed, 22 Feb 2023 03:31:42 +0000 Subject: [PATCH] Add transient notification views of group children to the child container, not directly to the row. Ever since the introduction of the new pipeline, we have been removing adding the transient child to the ExpandableNotificationRow instead of the NotificationChildrenContainer due to a trivial oversight. This works nearly all of the time because ExpandableViews track their transient container from which they can be removed at the end of their animation. However, there are some situations where animations end abruptly, and transient views are cleaned up through traversal. In this case we were only checking the NotificationChildrenContainer which would allow the transient children in the ExpandableNotificationRow to stay around forever. We had a single "Transient Views" output at the end of the NSSL's dump() but that didn't include views which were child groups, so this change also adds a dump of transient views added to NotificationChildrenContainer. There is no dump for ENR transient views, because there should not be any. Fixes: 260191323 Test: atest ExpandableNotificationRowControllerTest Change-Id: Ib5722ee5e6f5ae46902f276bd2bca7d6544e32a2 --- .../row/ExpandableNotificationRow.java | 16 +++++++-- .../ExpandableNotificationRowController.java | 2 +- .../stack/NotificationStackScrollLayout.java | 2 +- .../collection/render/FakeNodeController.kt | 29 ++++++++++++++++ ...ExpandableNotificationRowControllerTest.kt | 34 +++++++++++++++++++ 5 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/render/FakeNodeController.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java index 7c6efe47eafcd..d49c968e18cd0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java @@ -3734,7 +3734,9 @@ public class ExpandableNotificationRow extends ActivatableNotificationView } pw.println("Roundness: " + getRoundableState().debugString()); - if (mIsSummaryWithChildren) { + int transientViewCount = mChildrenContainer == null + ? 0 : mChildrenContainer.getTransientViewCount(); + if (mIsSummaryWithChildren || transientViewCount > 0) { pw.println(); pw.print("ChildrenContainer"); pw.print(" visibility: " + mChildrenContainer.getVisibility()); @@ -3742,8 +3744,7 @@ public class ExpandableNotificationRow extends ActivatableNotificationView pw.print(", translationY: " + mChildrenContainer.getTranslationY()); pw.println(); List notificationChildren = getAttachedChildren(); - pw.println("Children: " + notificationChildren.size()); - pw.print("{"); + pw.print("Children: " + notificationChildren.size() + " {"); pw.increaseIndent(); for (ExpandableNotificationRow child : notificationChildren) { pw.println(); @@ -3751,6 +3752,15 @@ public class ExpandableNotificationRow extends ActivatableNotificationView } pw.decreaseIndent(); pw.println("}"); + pw.print("Transient Views: " + transientViewCount + " {"); + pw.increaseIndent(); + for (int i = 0; i < transientViewCount; i++) { + pw.println(); + ExpandableView child = (ExpandableView) mChildrenContainer.getTransientView(i); + child.dump(pw, args); + } + pw.decreaseIndent(); + pw.println("}"); } else if (mPrivateLayout != null) { mPrivateLayout.dumpSmartReplies(pw); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowController.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowController.java index f1694aceac6c9..2dda6fd802e89 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowController.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowController.java @@ -317,7 +317,7 @@ public class ExpandableNotificationRowController implements NotifViewController } mView.removeChildNotification(childView); if (!isTransfer) { - mListContainer.notifyGroupChildRemoved(childView, mView); + mListContainer.notifyGroupChildRemoved(childView, mView.getChildrenContainer()); } } 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 d2087ba6ca1c3..e09b94bb661d0 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 @@ -2792,7 +2792,7 @@ public class NotificationStackScrollLayout extends ViewGroup implements Dumpable } child.setOnHeightChangedListener(null); updateScrollStateForRemovedChild(child); - boolean animationGenerated = generateRemoveAnimation(child); + boolean animationGenerated = container != null && generateRemoveAnimation(child); if (animationGenerated) { if (!mSwipedOutViews.contains(child) || !isFullySwipedOut(child)) { container.addTransientView(child, 0); diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/render/FakeNodeController.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/render/FakeNodeController.kt new file mode 100644 index 0000000000000..2de21ae8f0a14 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/render/FakeNodeController.kt @@ -0,0 +1,29 @@ +/* + * Copyright (c) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package com.android.systemui.statusbar.notification.collection.render + +import android.view.View + +class FakeNodeController( + override val view: View, + override val nodeLabel: String = "fakeNodeController" +) : NodeController { + override fun offerToKeepInParentForAnimation(): Boolean = false + override fun removeFromParentIfKeptForAnimation(): Boolean = false + override fun resetKeepInParentForAnimation() = Unit +} diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowControllerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowControllerTest.kt index 5b6c8c6a3f925..fb3aba1a77a0c 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowControllerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowControllerTest.kt @@ -31,15 +31,19 @@ import com.android.systemui.plugins.statusbar.StatusBarStateController import com.android.systemui.statusbar.NotificationMediaManager import com.android.systemui.statusbar.SmartReplyController import com.android.systemui.statusbar.notification.collection.provider.NotificationDismissibilityProvider +import com.android.systemui.statusbar.notification.collection.render.FakeNodeController import com.android.systemui.statusbar.notification.collection.render.GroupExpansionManager import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager import com.android.systemui.statusbar.notification.logging.NotificationLogger import com.android.systemui.statusbar.notification.people.PeopleNotificationIdentifier +import com.android.systemui.statusbar.notification.stack.NotificationChildrenContainer import com.android.systemui.statusbar.notification.stack.NotificationListContainer import com.android.systemui.statusbar.phone.KeyguardBypassController import com.android.systemui.statusbar.policy.HeadsUpManager import com.android.systemui.statusbar.policy.SmartReplyConstants import com.android.systemui.statusbar.policy.dagger.RemoteInputViewSubcomponent +import com.android.systemui.util.mockito.any +import com.android.systemui.util.mockito.eq import com.android.systemui.util.mockito.mock import com.android.systemui.util.time.SystemClock import com.android.systemui.wmshell.BubblesManager @@ -68,6 +72,7 @@ class ExpandableNotificationRowControllerTest : SysuiTestCase() { private val metricsLogger: MetricsLogger = mock() private val logBufferLogger: NotificationRowLogger = mock() private val listContainer: NotificationListContainer = mock() + private val childrenContainer: NotificationChildrenContainer = mock() private val mediaManager: NotificationMediaManager = mock() private val smartReplyConstants: SmartReplyConstants = mock() private val smartReplyController: SmartReplyController = mock() @@ -129,6 +134,7 @@ class ExpandableNotificationRowControllerTest : SysuiTestCase() { dragController, dismissibilityProvider ) + whenever(view.childrenContainer).thenReturn(childrenContainer) } @After @@ -173,4 +179,32 @@ class ExpandableNotificationRowControllerTest : SysuiTestCase() { Assert.assertFalse(controller.removeFromParentIfKeptForAnimation()) Mockito.verifyNoMoreInteractions(parentView) } + + @Test + fun removeChild_whenTransfer() { + val childView: ExpandableNotificationRow = mock() + val childNodeController = FakeNodeController(childView) + + // GIVEN a child is removed for transfer + controller.removeChild(childNodeController, /* isTransfer= */ true) + + // VERIFY the listContainer is not notified + Mockito.verify(childView).isChangingPosition = eq(true) + Mockito.verify(view).removeChildNotification(eq(childView)) + Mockito.verify(listContainer, never()).notifyGroupChildRemoved(any(), any()) + } + + @Test + fun removeChild_whenNotTransfer() { + val childView: ExpandableNotificationRow = mock() + val childNodeController = FakeNodeController(childView) + + // GIVEN a child is removed for real + controller.removeChild(childNodeController, /* isTransfer= */ false) + + // VERIFY the listContainer is passed the childrenContainer for transient animations + Mockito.verify(childView, never()).isChangingPosition = any() + Mockito.verify(view).removeChildNotification(eq(childView)) + Mockito.verify(listContainer).notifyGroupChildRemoved(eq(childView), eq(childrenContainer)) + } }