From 1fd48811dda5ab152c061b8a5be0fbd71a75dc03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A1s=20Kurucz?= Date: Wed, 26 Oct 2022 15:46:36 +0000 Subject: [PATCH] Fix Notification group dismissing animations When dismissing a notification group, keep the notifiication children in their parent to ensure correct animations. Fixes: 251058695 Fixes: 253266681 Fixes: 254277797 Bug: 259217907 Test: atest ShadeViewDifferTest Test: atest NotifCollectionTest Test: atest ExpandableNotificationRowTest Test: Manually dismiss Notification Groups (both app created groups and auto-grouped notifs) Change-Id: Ia5b6e5a1f058b5608896e33d0db2bd5b8fa39667 --- .../collection/NotificationEntry.java | 7 -- .../render/MediaContainerController.kt | 4 + .../collection/render/NodeController.kt | 24 ++++- .../collection/render/RootNodeController.kt | 3 + .../render/SectionHeaderController.kt | 3 + .../collection/render/ShadeViewDiffer.kt | 61 ++++++++--- .../render/ShadeViewDifferLogger.kt | 14 +++ .../row/ExpandableNotificationRow.java | 84 +++++++++++++-- .../ExpandableNotificationRowController.java | 63 +++++++++-- .../notification/row/NotificationRowLogger.kt | 53 +++++++++ .../stack/NotificationStackScrollLayout.java | 4 + ...tificationStackScrollLayoutController.java | 4 + .../collection/NotifCollectionTest.java | 17 +-- .../collection/render/ShadeViewDifferTest.kt | 102 ++++++++++++++++++ .../row/ExpandableNotificationRowTest.java | 76 +++++++++++++ .../row/NotificationTestHelper.java | 10 +- 16 files changed, 483 insertions(+), 46 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationRowLogger.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java index 585d871bad4ee..37d82caba229c 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java @@ -676,9 +676,6 @@ public final class NotificationEntry extends ListEntry { return row != null && row.areChildrenExpanded(); } - public boolean keepInParent() { - return row != null && row.keepInParent(); - } //TODO: probably less confusing to say "is group fully visible" public boolean isGroupNotFullyVisible() { @@ -698,10 +695,6 @@ public final class NotificationEntry extends ListEntry { return row != null && row.isSummaryWithChildren(); } - public void setKeepInParent(boolean keep) { - if (row != null) row.setKeepInParent(keep); - } - public void onDensityOrFontScaleChanged() { if (row != null) row.onDensityOrFontScaleChanged(); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/MediaContainerController.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/MediaContainerController.kt index f949af0688be1..8de03819ec322 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/MediaContainerController.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/MediaContainerController.kt @@ -55,4 +55,8 @@ class MediaContainerController @Inject constructor( override val view: View get() = mediaContainerView!! + + override fun offerToKeepInParentForAnimation(): Boolean = false + override fun removeFromParentIfKeptForAnimation(): Boolean = false + override fun resetKeepInParentForAnimation() {} } \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/NodeController.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/NodeController.kt index 26ba12c975756..ae72a3cd5a04a 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/NodeController.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/NodeController.kt @@ -30,6 +30,7 @@ import java.lang.StringBuilder * below. */ interface NodeController { + /** A string that uniquely(ish) represents the node in the tree. Used for debugging. */ val nodeLabel: String @@ -64,6 +65,27 @@ interface NodeController { /** Called when this view has been removed */ fun onViewRemoved() {} + + /** + * Called before removing a node from its parent + * + * If returned true, the ShadeViewDiffer won't detach this row and the view system is + * responsible for ensuring the row is in eventually removed from the parent. + * + * @return false to opt out from this feature + */ + fun offerToKeepInParentForAnimation(): Boolean + + /** + * Called before a node is reattached. Removes the view from its parent + * if it was flagged to be kept before. + * + * @return whether it did a removal + */ + fun removeFromParentIfKeptForAnimation(): Boolean + + /** Called when a node is being reattached */ + fun resetKeepInParentForAnimation() } /** @@ -90,7 +112,7 @@ fun treeSpecToStr(tree: NodeSpec): String { } private fun treeSpecToStrHelper(tree: NodeSpec, sb: StringBuilder, indent: String) { - sb.append("${indent}{${tree.controller.nodeLabel}}\n") + sb.append("$indent{${tree.controller.nodeLabel}}\n") if (tree.children.isNotEmpty()) { val childIndent = "$indent " for (child in tree.children) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/RootNodeController.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/RootNodeController.kt index 2073e92cd45b2..5ff686ae0270e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/RootNodeController.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/RootNodeController.kt @@ -32,6 +32,9 @@ class RootNodeController( override val view: View ) : NodeController, PipelineDumpable { override val nodeLabel: String = "" + override fun offerToKeepInParentForAnimation(): Boolean = false + override fun removeFromParentIfKeptForAnimation(): Boolean = false + override fun resetKeepInParentForAnimation() {} override fun getChildAt(index: Int): View? { return listContainer.getContainerChildAt(index) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/SectionHeaderController.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/SectionHeaderController.kt index 2c9508e84aef8..7b59266e207a1 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/SectionHeaderController.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/SectionHeaderController.kt @@ -100,4 +100,7 @@ internal class SectionHeaderNodeControllerImpl @Inject constructor( override val view: View get() = _view!! + override fun offerToKeepInParentForAnimation(): Boolean = false + override fun removeFromParentIfKeptForAnimation(): Boolean = false + override fun resetKeepInParentForAnimation() {} } \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDiffer.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDiffer.kt index 9a9941e7a19c7..18ee4816fac91 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDiffer.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDiffer.kt @@ -86,10 +86,10 @@ class ShadeViewDiffer( } private fun maybeDetachChild( - parentNode: ShadeNode, - parentSpec: NodeSpec?, - childNode: ShadeNode, - childSpec: NodeSpec? + parentNode: ShadeNode, + parentSpec: NodeSpec?, + childNode: ShadeNode, + childSpec: NodeSpec? ) { val newParentNode = childSpec?.parent?.let { getNode(it) } @@ -100,14 +100,27 @@ class ShadeViewDiffer( nodes.remove(childNode.controller) } - logger.logDetachingChild( - key = childNode.label, - isTransfer = !childCompletelyRemoved, - isParentRemoved = parentSpec == null, - oldParent = parentNode.label, - newParent = newParentNode?.label) - parentNode.removeChild(childNode, isTransfer = !childCompletelyRemoved) - childNode.parent = null + if (childCompletelyRemoved && parentSpec == null && + childNode.offerToKeepInParentForAnimation()) { + // If both the child and the parent are being removed at the same time, then + // keep the child attached to the parent for animation purposes + logger.logSkipDetachingChild( + key = childNode.label, + parentKey = parentNode.label, + isTransfer = !childCompletelyRemoved, + isParentRemoved = true + ) + } else { + logger.logDetachingChild( + key = childNode.label, + isTransfer = !childCompletelyRemoved, + isParentRemoved = parentSpec == null, + oldParent = parentNode.label, + newParent = newParentNode?.label + ) + parentNode.removeChild(childNode, isTransfer = !childCompletelyRemoved) + childNode.parent = null + } } } @@ -119,6 +132,16 @@ class ShadeViewDiffer( val childNode = getNode(childSpec) if (childNode.view != currView) { + val removedFromParent = childNode.removeFromParentIfKeptForAnimation() + if (removedFromParent) { + logger.logDetachingChild( + key = childNode.label, + isTransfer = false, + isParentRemoved = true, + oldParent = null, + newParent = null + ) + } when (childNode.parent) { null -> { @@ -142,6 +165,8 @@ class ShadeViewDiffer( } } + childNode.resetKeepInParentForAnimation() + if (childSpec.children.isNotEmpty()) { attachChildren(childNode, specMap) } @@ -213,4 +238,16 @@ private class ShadeNode(val controller: NodeController) { controller.removeChild(child.controller, isTransfer) child.controller.onViewRemoved() } + + fun offerToKeepInParentForAnimation(): Boolean { + return controller.offerToKeepInParentForAnimation() + } + + fun removeFromParentIfKeptForAnimation(): Boolean { + return controller.removeFromParentIfKeptForAnimation() + } + + fun resetKeepInParentForAnimation() { + controller.resetKeepInParentForAnimation() + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDifferLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDifferLogger.kt index b4b9438cd6be6..1e22c2cd7c0e8 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDifferLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDifferLogger.kt @@ -43,6 +43,20 @@ class ShadeViewDifferLogger @Inject constructor( }) } + fun logSkipDetachingChild( + key: String, + parentKey: String?, + isTransfer: Boolean, + isParentRemoved: Boolean + ) { + buffer.log(TAG, LogLevel.DEBUG, { + str1 = key + str2 = parentKey + bool1 = isTransfer + bool2 = isParentRemoved + }, { "Skip detaching $str1 from $str2 isTransfer=$bool1 isParentRemoved=$bool2" }) + } + fun logAttachingChild(key: String, parent: String, atIndex: Int) { buffer.log(TAG, LogLevel.DEBUG, { str1 = key 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 b93e1500e570a..1eccc9879a873 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 @@ -240,7 +240,7 @@ public class ExpandableNotificationRow extends ActivatableNotificationView private NotificationContentView mPrivateLayout; private NotificationContentView[] mLayouts; private int mNotificationColor; - private ExpansionLogger mLogger; + private ExpandableNotificationRowLogger mLogger; private String mLoggingKey; private NotificationGuts mGuts; private NotificationEntry mEntry; @@ -339,7 +339,7 @@ public class ExpandableNotificationRow extends ActivatableNotificationView } } }; - private boolean mKeepInParent; + private boolean mKeepInParentForDismissAnimation; private boolean mRemoved; private static final Property TRANSLATE_CONTENT = new FloatProperty("translate") { @@ -825,6 +825,12 @@ public class ExpandableNotificationRow extends ActivatableNotificationView if (mChildrenContainer == null) { mChildrenContainerStub.inflate(); } + + if (row.keepInParentForDismissAnimation()) { + logSkipAttachingKeepInParentChild(row); + return; + } + mChildrenContainer.addNotification(row, childIndex); onAttachedChildrenCountChanged(); row.setIsChildInGroup(true, this); @@ -833,12 +839,38 @@ public class ExpandableNotificationRow extends ActivatableNotificationView public void removeChildNotification(ExpandableNotificationRow row) { if (mChildrenContainer != null) { mChildrenContainer.removeNotification(row); + row.setKeepInParentForDismissAnimation(false); } onAttachedChildrenCountChanged(); row.setIsChildInGroup(false, null); row.requestBottomRoundness(0.0f, /* animate = */ false, SourceType.DefaultValue); } + /** + * Removes the children notifications which were marked to keep for the dismissal animation. + */ + public void removeChildrenWithKeepInParent() { + if (mChildrenContainer == null) return; + + List clonedList = new ArrayList<>( + mChildrenContainer.getAttachedChildren()); + boolean childCountChanged = false; + for (ExpandableNotificationRow child : clonedList) { + if (child.keepInParentForDismissAnimation()) { + mChildrenContainer.removeNotification(child); + child.setIsChildInGroup(false, null); + child.requestBottomRoundness(0.0f, /* animate = */ false, SourceType.DefaultValue); + child.setKeepInParentForDismissAnimation(false); + logKeepInParentChildDetached(child); + childCountChanged = true; + } + } + + if (childCountChanged) { + onAttachedChildrenCountChanged(); + } + } + /** * Returns the child notification at [index], or null if no such child. */ @@ -1361,12 +1393,15 @@ public class ExpandableNotificationRow extends ActivatableNotificationView } } - public boolean keepInParent() { - return mKeepInParent; + /** + * @return if this entry should be kept in its parent during removal. + */ + public boolean keepInParentForDismissAnimation() { + return mKeepInParentForDismissAnimation; } - public void setKeepInParent(boolean keepInParent) { - mKeepInParent = keepInParent; + public void setKeepInParentForDismissAnimation(boolean keepInParent) { + mKeepInParentForDismissAnimation = keepInParent; } @Override @@ -1537,8 +1572,29 @@ public class ExpandableNotificationRow extends ActivatableNotificationView mUseIncreasedHeadsUpHeight = use; } - public interface ExpansionLogger { + /** + * Interface for logging {{@link ExpandableNotificationRow} events.} + */ + public interface ExpandableNotificationRowLogger { + /** + * Called when the notification is expanded / collapsed. + */ void logNotificationExpansion(String key, boolean userAction, boolean expanded); + + /** + * Called when a notification which was previously kept in its parent for the + * dismiss animation is finally detached from its parent. + */ + void logKeepInParentChildDetached(NotificationEntry child, NotificationEntry oldParent); + + /** + * Called when we want to attach a notification to a new parent, + * but it still has the keepInParent flag set, so we skip it. + */ + void logSkipAttachingKeepInParentChild( + NotificationEntry child, + NotificationEntry newParent + ); } public ExpandableNotificationRow(Context context, AttributeSet attrs) { @@ -1556,7 +1612,7 @@ public class ExpandableNotificationRow extends ActivatableNotificationView RemoteInputViewSubcomponent.Factory rivSubcomponentFactory, String appName, String notificationKey, - ExpansionLogger logger, + ExpandableNotificationRowLogger logger, KeyguardBypassController bypassController, GroupMembershipManager groupMembershipManager, GroupExpansionManager groupExpansionManager, @@ -3567,6 +3623,18 @@ public class ExpandableNotificationRow extends ActivatableNotificationView }); } + private void logKeepInParentChildDetached(ExpandableNotificationRow child) { + if (mLogger != null) { + mLogger.logKeepInParentChildDetached(child.getEntry(), getEntry()); + } + } + + private void logSkipAttachingKeepInParentChild(ExpandableNotificationRow child) { + if (mLogger != null) { + mLogger.logSkipAttachingKeepInParentChild(child.getEntry(), getEntry()); + } + } + private void setTargetPoint(Point p) { mTargetPoint = p; } 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 842526ee03718..f79714968d3bc 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 @@ -83,13 +83,11 @@ public class ExpandableNotificationRowController implements NotifViewController private final GroupExpansionManager mGroupExpansionManager; private final RowContentBindStage mRowContentBindStage; private final NotificationLogger mNotificationLogger; + private final NotificationRowLogger mLogBufferLogger; private final HeadsUpManager mHeadsUpManager; private final ExpandableNotificationRow.OnExpandClickListener mOnExpandClickListener; private final StatusBarStateController mStatusBarStateController; private final MetricsLogger mMetricsLogger; - - private final ExpandableNotificationRow.ExpansionLogger mExpansionLogger = - this::logNotificationExpansion; private final ExpandableNotificationRow.CoordinateOnClickListener mOnFeedbackClickListener; private final NotificationGutsManager mNotificationGutsManager; private final OnUserInteractionCallback mOnUserInteractionCallback; @@ -101,8 +99,32 @@ public class ExpandableNotificationRowController implements NotifViewController private final Optional mBubblesManagerOptional; private final SmartReplyConstants mSmartReplyConstants; private final SmartReplyController mSmartReplyController; - private final ExpandableNotificationRowDragController mDragController; + private final ExpandableNotificationRow.ExpandableNotificationRowLogger mLoggerCallback = + new ExpandableNotificationRow.ExpandableNotificationRowLogger() { + @Override + public void logNotificationExpansion(String key, boolean userAction, + boolean expanded) { + mNotificationLogger.onExpansionChanged(key, userAction, expanded); + } + + @Override + public void logKeepInParentChildDetached( + NotificationEntry child, + NotificationEntry oldParent + ) { + mLogBufferLogger.logKeepInParentChildDetached(child, oldParent); + } + + @Override + public void logSkipAttachingKeepInParentChild( + NotificationEntry child, + NotificationEntry newParent + ) { + mLogBufferLogger.logSkipAttachingKeepInParentChild(child, newParent); + } + }; + @Inject public ExpandableNotificationRowController( @@ -110,6 +132,7 @@ public class ExpandableNotificationRowController implements NotifViewController ActivatableNotificationViewController activatableNotificationViewController, RemoteInputViewSubcomponent.Factory rivSubcomponentFactory, MetricsLogger metricsLogger, + NotificationRowLogger logBufferLogger, NotificationListContainer listContainer, NotificationMediaManager mediaManager, SmartReplyConstants smartReplyConstants, @@ -163,6 +186,7 @@ public class ExpandableNotificationRowController implements NotifViewController mBubblesManagerOptional = bubblesManagerOptional; mDragController = dragController; mMetricsLogger = metricsLogger; + mLogBufferLogger = logBufferLogger; mSmartReplyConstants = smartReplyConstants; mSmartReplyController = smartReplyController; } @@ -177,7 +201,7 @@ public class ExpandableNotificationRowController implements NotifViewController mRemoteInputViewSubcomponentFactory, mAppName, mNotificationKey, - mExpansionLogger, + mLoggerCallback, mKeyguardBypassController, mGroupMembershipManager, mGroupExpansionManager, @@ -243,10 +267,6 @@ public class ExpandableNotificationRowController implements NotifViewController } }; - private void logNotificationExpansion(String key, boolean userAction, boolean expanded) { - mNotificationLogger.onExpansionChanged(key, userAction, expanded); - } - @Override @NonNull public String getNodeLabel() { @@ -336,4 +356,29 @@ public class ExpandableNotificationRowController implements NotifViewController public void setFeedbackIcon(@Nullable FeedbackIcon icon) { mView.setFeedbackIcon(icon); } + + @Override + public boolean offerToKeepInParentForAnimation() { + if (mFeatureFlags.isEnabled(Flags.NOTIFICATION_GROUP_DISMISSAL_ANIMATION)) { + mView.setKeepInParentForDismissAnimation(true); + return true; + } + return false; + } + + @Override + public boolean removeFromParentIfKeptForAnimation() { + ExpandableNotificationRow parent = mView.getNotificationParent(); + if (mView.keepInParentForDismissAnimation() && parent != null) { + parent.removeChildNotification(mView); + return true; + } + + return false; + } + + @Override + public void resetKeepInParentForAnimation() { + mView.setKeepInParentForDismissAnimation(false); + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationRowLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationRowLogger.kt new file mode 100644 index 0000000000000..ce11be36acf38 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationRowLogger.kt @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2022 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.row + +import com.android.systemui.log.dagger.NotificationLog +import com.android.systemui.plugins.log.LogBuffer +import com.android.systemui.plugins.log.LogLevel +import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.logKey +import javax.inject.Inject + +class NotificationRowLogger @Inject constructor(@NotificationLog private val buffer: LogBuffer) { + fun logKeepInParentChildDetached(child: NotificationEntry, oldParent: NotificationEntry?) { + buffer.log( + TAG, + LogLevel.DEBUG, + { + str1 = child.logKey + str2 = oldParent.logKey + }, + { "Detach child $str1 kept in parent $str2" } + ) + } + + fun logSkipAttachingKeepInParentChild(child: NotificationEntry, newParent: NotificationEntry?) { + buffer.log( + TAG, + LogLevel.WARNING, + { + str1 = child.logKey + str2 = newParent.logKey + }, + { "Skipping to attach $str1 to $str2, because it still flagged to keep in parent" } + ) + } +} + +private const val TAG = "NotifRow" 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 41dbf1d6dc60a..76997b8826656 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 @@ -2739,6 +2739,10 @@ public class NotificationStackScrollLayout extends ViewGroup implements Dumpable } } else { mSwipedOutViews.remove(child); + + if (child instanceof ExpandableNotificationRow) { + ((ExpandableNotificationRow) child).removeChildrenWithKeepInParent(); + } } updateAnimationState(false, child); 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 0240bbcf1e19e..ad4501a75ebbc 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 @@ -443,7 +443,11 @@ public class NotificationStackScrollLayoutController { if (!row.isDismissed()) { handleChildViewDismissed(view); } + row.removeFromTransientContainer(); + if (row instanceof ExpandableNotificationRow) { + ((ExpandableNotificationRow) row).removeChildrenWithKeepInParent(); + } } /** diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java index 3b05321e1a6bb..94e3e6cb5f8e1 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java @@ -92,6 +92,7 @@ import com.android.systemui.statusbar.notification.collection.notifcollection.No import com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLogger; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifDismissInterceptor; import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender; +import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow; import com.android.systemui.util.concurrency.FakeExecutor; import com.android.systemui.util.time.FakeSystemClock; @@ -741,22 +742,24 @@ public class NotifCollectionTest extends SysuiTestCase { @Test public void testGroupChildrenAreDismissedLocallyWhenSummaryIsDismissed() { // GIVEN a collection with two grouped notifs in it - CollectionEvent notif0 = postNotif( + CollectionEvent groupNotif = postNotif( buildNotif(TEST_PACKAGE, 0) .setGroup(mContext, GROUP_1) .setGroupSummary(mContext, true)); - CollectionEvent notif1 = postNotif( + CollectionEvent childNotif = postNotif( buildNotif(TEST_PACKAGE, 1) .setGroup(mContext, GROUP_1)); - NotificationEntry entry0 = mCollectionListener.getEntry(notif0.key); - NotificationEntry entry1 = mCollectionListener.getEntry(notif1.key); + NotificationEntry groupEntry = mCollectionListener.getEntry(groupNotif.key); + NotificationEntry childEntry = mCollectionListener.getEntry(childNotif.key); + ExpandableNotificationRow childRow = mock(ExpandableNotificationRow.class); + childEntry.setRow(childRow); // WHEN the summary is dismissed - mCollection.dismissNotification(entry0, defaultStats(entry0)); + mCollection.dismissNotification(groupEntry, defaultStats(groupEntry)); // THEN all members of the group are marked as dismissed locally - assertEquals(DISMISSED, entry0.getDismissState()); - assertEquals(PARENT_DISMISSED, entry1.getDismissState()); + assertEquals(DISMISSED, groupEntry.getDismissState()); + assertEquals(PARENT_DISMISSED, childEntry.getDismissState()); } @Test diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDifferTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDifferTest.kt index 15cf17dbcf86d..6167b464c5fbe 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDifferTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/render/ShadeViewDifferTest.kt @@ -18,6 +18,7 @@ package com.android.systemui.statusbar.notification.collection.render import android.content.Context import android.testing.AndroidTestingRunner import android.view.View +import android.view.ViewGroup import android.widget.FrameLayout import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase @@ -26,6 +27,10 @@ import org.junit.Assert import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.mockito.ArgumentMatchers.isNull +import org.mockito.Mockito.anyBoolean +import org.mockito.Mockito.matches +import org.mockito.Mockito.verify @SmallTest @RunWith(AndroidTestingRunner::class) @@ -124,6 +129,64 @@ class ShadeViewDifferTest : SysuiTestCase() { Assert.assertNull(controller3.view.parent) Assert.assertNull(controller4.view.parent) Assert.assertNull(controller5.view.parent) + verifyDetachingChildLogged(controller3, oldParent = controller2) + verifyDetachingChildLogged(controller4, oldParent = controller2) + verifyDetachingChildLogged(controller5, oldParent = controller2) + } + + @Test + fun testRemovedGroupsWithKeepInParentAreKeptTogether() { + // GIVEN a preexisting tree with a group + // AND the group children supports keepInParent + applySpecAndCheck( + node(controller1), + node(controller2, node(controller3), node(controller4), node(controller5)) + ) + controller3.supportsKeepInParent = true + controller4.supportsKeepInParent = true + controller5.supportsKeepInParent = true + + // WHEN the new spec removes the entire group + applySpecAndCheck(node(controller1)) + + // THEN the group children are still attached to their parent + Assert.assertEquals(controller2.view, controller3.view.parent) + Assert.assertEquals(controller2.view, controller4.view.parent) + Assert.assertEquals(controller2.view, controller5.view.parent) + verifySkipDetachingChildLogged(controller3, parent = controller2) + verifySkipDetachingChildLogged(controller4, parent = controller2) + verifySkipDetachingChildLogged(controller5, parent = controller2) + } + + @Test + fun testReuseRemovedGroupsWithKeepInParent() { + // GIVEN a preexisting tree with a dismissed group + // AND the group children supports keepInParent + controller3.supportsKeepInParent = true + controller4.supportsKeepInParent = true + controller5.supportsKeepInParent = true + applySpecAndCheck( + node(controller1), + node(controller2, node(controller3), node(controller4), node(controller5)) + ) + applySpecAndCheck(node(controller1)) + + // WHEN a new spec is applied which reuses the dismissed views + applySpecAndCheck( + node(controller1), + node(controller2), + node(controller3), + node(controller4), + node(controller5) + ) + + // THEN the dismissed views can be reused + Assert.assertEquals(rootController.view, controller3.view.parent) + Assert.assertEquals(rootController.view, controller4.view.parent) + Assert.assertEquals(rootController.view, controller5.view.parent) + verifyDetachingChildLogged(controller3, oldParent = null) + verifyDetachingChildLogged(controller4, oldParent = null) + verifyDetachingChildLogged(controller5, oldParent = null) } @Test @@ -184,7 +247,30 @@ class ShadeViewDifferTest : SysuiTestCase() { } } + private fun verifySkipDetachingChildLogged(child: NodeController, parent: NodeController) { + verify(logger) + .logSkipDetachingChild( + key = matches(child.nodeLabel), + parentKey = matches(parent.nodeLabel), + anyBoolean(), + anyBoolean() + ) + } + + private fun verifyDetachingChildLogged(child: NodeController, oldParent: NodeController?) { + verify(logger) + .logDetachingChild( + key = matches(child.nodeLabel), + isTransfer = anyBoolean(), + isParentRemoved = anyBoolean(), + oldParent = oldParent?.let { matches(it.nodeLabel) } ?: isNull(), + newParent = isNull() + ) + } + private class FakeController(context: Context, label: String) : NodeController { + var supportsKeepInParent: Boolean = false + override val view: FrameLayout = FrameLayout(context) override val nodeLabel: String = label override fun getChildCount(): Int = view.childCount @@ -209,6 +295,22 @@ class ShadeViewDifferTest : SysuiTestCase() { override fun onViewAdded() {} override fun onViewMoved() {} override fun onViewRemoved() {} + override fun offerToKeepInParentForAnimation(): Boolean { + return supportsKeepInParent + } + + override fun removeFromParentIfKeptForAnimation(): Boolean { + if (supportsKeepInParent) { + (view.parent as? ViewGroup)?.removeView(view) + return true + } + + return false + } + + override fun resetKeepInParentForAnimation() { + supportsKeepInParent = false + } } private class SpecBuilder( diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java index 12cc11450edeb..ee8db18a10031 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java @@ -38,6 +38,7 @@ import static org.mockito.Mockito.reset; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import android.app.Notification; @@ -458,4 +459,79 @@ public class ExpandableNotificationRowTest extends SysuiTestCase { verify(mNotificationTestHelper.mOnUserInteractionCallback, never()) .registerFutureDismissal(any(), anyInt()); } + + @Test + public void testAddChildNotification() throws Exception { + ExpandableNotificationRow group = mNotificationTestHelper.createGroup(0); + ExpandableNotificationRow child = mNotificationTestHelper.createRow(); + + group.addChildNotification(child); + + Assert.assertEquals(child, group.getChildNotificationAt(0)); + Assert.assertEquals(group, child.getNotificationParent()); + Assert.assertTrue(child.isChildInGroup()); + } + + @Test + public void testAddChildNotification_childSkipped() throws Exception { + ExpandableNotificationRow group = mNotificationTestHelper.createGroup(0); + ExpandableNotificationRow child = mNotificationTestHelper.createRow(); + child.setKeepInParentForDismissAnimation(true); + + group.addChildNotification(child); + + Assert.assertTrue(group.getAttachedChildren().isEmpty()); + Assert.assertNotEquals(group, child.getNotificationParent()); + verify(mNotificationTestHelper.getMockLogger()).logSkipAttachingKeepInParentChild( + /*child=*/ child.getEntry(), + /*newParent=*/ group.getEntry() + ); + } + + @Test + public void testRemoveChildNotification() throws Exception { + ExpandableNotificationRow group = mNotificationTestHelper.createGroup(1); + ExpandableNotificationRow child = group.getAttachedChildren().get(0); + child.setKeepInParentForDismissAnimation(true); + + group.removeChildNotification(child); + + Assert.assertNull(child.getParent()); + Assert.assertNull(child.getNotificationParent()); + Assert.assertFalse(child.keepInParentForDismissAnimation()); + verifyNoMoreInteractions(mNotificationTestHelper.getMockLogger()); + } + + @Test + public void testRemoveChildrenWithKeepInParent_removesChildWithKeepInParent() throws Exception { + ExpandableNotificationRow group = mNotificationTestHelper.createGroup(1); + ExpandableNotificationRow child = group.getAttachedChildren().get(0); + child.setKeepInParentForDismissAnimation(true); + + group.removeChildrenWithKeepInParent(); + + Assert.assertNull(child.getParent()); + Assert.assertNull(child.getNotificationParent()); + Assert.assertFalse(child.keepInParentForDismissAnimation()); + verify(mNotificationTestHelper.getMockLogger()).logKeepInParentChildDetached( + /*child=*/ child.getEntry(), + /*oldParent=*/ group.getEntry() + ); + } + + @Test + public void testRemoveChildrenWithKeepInParent_skipsChildrenWithoutKeepInParent() + throws Exception { + ExpandableNotificationRow group = mNotificationTestHelper.createGroup(1); + ExpandableNotificationRow child = group.getAttachedChildren().get(0); + + group.removeChildrenWithKeepInParent(); + + Assert.assertEquals(group, child.getNotificationParent()); + Assert.assertFalse(child.keepInParentForDismissAnimation()); + verify(mNotificationTestHelper.getMockLogger(), never()).logKeepInParentChildDetached( + /*child=*/ any(), + /*oldParent=*/ any() + ); + } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java index ab4ae6a9216b4..5fd9448b129d0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java @@ -73,7 +73,7 @@ import com.android.systemui.statusbar.notification.collection.render.GroupMember import com.android.systemui.statusbar.notification.icon.IconBuilder; import com.android.systemui.statusbar.notification.icon.IconManager; import com.android.systemui.statusbar.notification.people.PeopleNotificationIdentifier; -import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow.ExpansionLogger; +import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow.ExpandableNotificationRowLogger; import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow.OnExpandClickListener; import com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.InflationFlag; import com.android.systemui.statusbar.phone.ConfigurationControllerImpl; @@ -116,6 +116,7 @@ public class NotificationTestHelper { private final Context mContext; private final TestableLooper mTestLooper; private int mId; + private final ExpandableNotificationRowLogger mMockLogger; private final GroupMembershipManager mGroupMembershipManager; private final GroupExpansionManager mGroupExpansionManager; private ExpandableNotificationRow mRow; @@ -139,6 +140,7 @@ public class NotificationTestHelper { dependency.injectMockDependency(NotificationMediaManager.class); dependency.injectMockDependency(NotificationShadeWindowController.class); dependency.injectMockDependency(MediaOutputDialogFactory.class); + mMockLogger = mock(ExpandableNotificationRowLogger.class); mStatusBarStateController = mock(StatusBarStateController.class); mGroupMembershipManager = mock(GroupMembershipManager.class); mGroupExpansionManager = mock(GroupExpansionManager.class); @@ -197,6 +199,10 @@ public class NotificationTestHelper { mDefaultInflationFlags = defaultInflationFlags; } + public ExpandableNotificationRowLogger getMockLogger() { + return mMockLogger; + } + /** * Creates a generic row with rounded border. * @@ -527,7 +533,7 @@ public class NotificationTestHelper { mock(RemoteInputViewSubcomponent.Factory.class), APP_NAME, entry.getKey(), - mock(ExpansionLogger.class), + mMockLogger, mock(KeyguardBypassController.class), mGroupMembershipManager, mGroupExpansionManager,