From 5d3cdb4e4c4e7fffe810d450434bc890991f423e Mon Sep 17 00:00:00 2001 From: Beverly Date: Fri, 28 Aug 2020 17:28:07 -0400 Subject: [PATCH] Update removal logic for notifications New pipeline: Cancel the lifetime extension for notifications that are manually dismissed by the user whether this is via manually dismissal or if the user is clicking on an autoCancellable notification Old pipeline: Move the logic for dismissing the summary of a notification if the user is dismissing the last child of a notification group to the OnUserInteraction callback. Test: atest NotifCollectionTest Test: manual Fixes: 166784583 Change-Id: I55398075ad9733b73d8ff900cd465fee9a4f01d1 --- .../collection/NotifCollection.java | 16 ++++--- .../OnUserInteractionCallbackImplLegacy.java | 14 +++++- .../dagger/NotificationsModule.java | 12 ++++-- .../row/ExpandableNotificationRow.java | 15 ------- .../NotificationBlockingHelperManager.java | 14 ++++-- .../StatusBarNotificationActivityStarter.java | 31 ++++++------- .../collection/NotifCollectionTest.java | 43 ++++++++++++++----- ...NotificationBlockingHelperManagerTest.java | 6 ++- 8 files changed, 91 insertions(+), 60 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java index 90492b5d606dc..fdfd72489e93d 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java @@ -18,6 +18,7 @@ package com.android.systemui.statusbar.notification.collection; import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL; import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL_ALL; +import static android.service.notification.NotificationListenerService.REASON_CANCEL; import static android.service.notification.NotificationListenerService.REASON_CANCEL_ALL; import static android.service.notification.NotificationListenerService.REASON_CHANNEL_BANNED; import static android.service.notification.NotificationListenerService.REASON_CLICK; @@ -459,8 +460,7 @@ public class NotifCollection implements Dumpable { + ": has not been marked for removal")); } - if (isDismissedByUser(entry)) { - // User-dismissed notifications cannot be lifetime-extended + if (cannotBeLifetimeExtended(entry)) { cancelLifetimeExtension(entry); } else { updateLifetimeExtension(entry); @@ -583,7 +583,7 @@ public class NotifCollection implements Dumpable { } private void cancelLocalDismissal(NotificationEntry entry) { - if (isDismissedByUser(entry)) { + if (entry.getDismissState() != NOT_DISMISSED) { entry.setDismissState(NOT_DISMISSED); if (entry.getSbn().getNotification().isGroupSummary()) { for (NotificationEntry otherEntry : mNotificationSet.values()) { @@ -669,12 +669,16 @@ public class NotifCollection implements Dumpable { * immediately removed from the collection, but can sometimes stick around due to lifetime * extenders. */ - private static boolean isCanceled(NotificationEntry entry) { + private boolean isCanceled(NotificationEntry entry) { return entry.mCancellationReason != REASON_NOT_CANCELED; } - private static boolean isDismissedByUser(NotificationEntry entry) { - return entry.getDismissState() != NOT_DISMISSED; + private boolean cannotBeLifetimeExtended(NotificationEntry entry) { + final boolean locallyDismissedByUser = entry.getDismissState() != NOT_DISMISSED; + final boolean systemServerReportedUserCancel = + entry.mCancellationReason == REASON_CLICK + || entry.mCancellationReason == REASON_CANCEL; + return locallyDismissedByUser || systemServerReportedUserCancel; } /** diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java index cce8cdc64d306..610cd33383e91 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java @@ -26,6 +26,7 @@ import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.statusbar.notification.NotificationEntryManager; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.notifcollection.DismissedByUserStats; +import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager; import com.android.systemui.statusbar.notification.logging.NotificationLogger; import com.android.systemui.statusbar.notification.row.OnUserInteractionCallback; import com.android.systemui.statusbar.policy.HeadsUpManager; @@ -38,17 +39,20 @@ public class OnUserInteractionCallbackImplLegacy implements OnUserInteractionCal private final HeadsUpManager mHeadsUpManager; private final StatusBarStateController mStatusBarStateController; private final VisualStabilityManager mVisualStabilityManager; + private final GroupMembershipManager mGroupMembershipManager; public OnUserInteractionCallbackImplLegacy( NotificationEntryManager notificationEntryManager, HeadsUpManager headsUpManager, StatusBarStateController statusBarStateController, - VisualStabilityManager visualStabilityManager + VisualStabilityManager visualStabilityManager, + GroupMembershipManager groupMembershipManager ) { mNotificationEntryManager = notificationEntryManager; mHeadsUpManager = headsUpManager; mStatusBarStateController = statusBarStateController; mVisualStabilityManager = visualStabilityManager; + mGroupMembershipManager = groupMembershipManager; } /** @@ -69,6 +73,13 @@ public class OnUserInteractionCallbackImplLegacy implements OnUserInteractionCal dismissalSurface = NotificationStats.DISMISSAL_AOD; } + if (mGroupMembershipManager.isOnlyChildInGroup(entry)) { + NotificationEntry groupSummary = mGroupMembershipManager.getLogicalGroupSummary(entry); + if (groupSummary.isClearable()) { + onDismiss(groupSummary, cancellationReason); + } + } + mNotificationEntryManager.performRemoveNotification( entry.getSbn(), new DismissedByUserStats( @@ -82,6 +93,7 @@ public class OnUserInteractionCallbackImplLegacy implements OnUserInteractionCal NotificationLogger.getNotificationLocation(entry))), cancellationReason ); + } @Override diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java index e2aae64ce220e..ea86d25389fae 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java @@ -205,9 +205,11 @@ public interface NotificationsModule { Context context, NotificationGutsManager notificationGutsManager, NotificationEntryManager notificationEntryManager, - MetricsLogger metricsLogger) { + MetricsLogger metricsLogger, + GroupMembershipManager groupMembershipManager) { return new NotificationBlockingHelperManager( - context, notificationGutsManager, notificationEntryManager, metricsLogger); + context, notificationGutsManager, notificationEntryManager, metricsLogger, + groupMembershipManager); } /** Provides an instance of {@link GroupMembershipManager} */ @@ -273,7 +275,8 @@ public interface NotificationsModule { Lazy notifCollection, Lazy visualStabilityCoordinator, NotificationEntryManager entryManager, - VisualStabilityManager visualStabilityManager) { + VisualStabilityManager visualStabilityManager, + Lazy groupMembershipManagerLazy) { return featureFlags.isNewNotifPipelineRenderingEnabled() ? new OnUserInteractionCallbackImpl( pipeline.get(), @@ -285,7 +288,8 @@ public interface NotificationsModule { entryManager, headsUpManager, statusBarStateController, - visualStabilityManager); + visualStabilityManager, + groupMembershipManagerLazy.get()); } /** */ 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 89f720535402b..adda049951ac0 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 @@ -817,13 +817,6 @@ public class ExpandableNotificationRow extends ActivatableNotificationView return mNotificationParent != null; } - /** - * @return whether this notification is the only child in the group summary - */ - public boolean isOnlyChildInGroup() { - return mGroupMembershipManager.isOnlyChildInGroup(mEntry); - } - public ExpandableNotificationRow getNotificationParent() { return mNotificationParent; } @@ -1425,14 +1418,6 @@ public class ExpandableNotificationRow extends ActivatableNotificationView } public void performDismiss(boolean fromAccessibility) { - if (isOnlyChildInGroup()) { - NotificationEntry groupSummary = mGroupMembershipManager.getLogicalGroupSummary(mEntry); - if (groupSummary.isClearable()) { - // If this is the only child in the group, dismiss the group, but don't try to show - // the blocking helper affordance! - groupSummary.getRow().performDismiss(fromAccessibility); - } - } dismiss(fromAccessibility); if (mEntry.isClearable()) { if (mOnUserInteractionCallback != null) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationBlockingHelperManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationBlockingHelperManager.java index 921232568755c..ab78d197da0bb 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationBlockingHelperManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationBlockingHelperManager.java @@ -28,6 +28,8 @@ import com.android.internal.logging.MetricsLogger; import com.android.internal.logging.nano.MetricsProto.MetricsEvent; import com.android.systemui.plugins.statusbar.NotificationMenuRowPlugin; import com.android.systemui.statusbar.notification.NotificationEntryManager; +import com.android.systemui.statusbar.notification.collection.NotificationEntry; +import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager; import com.android.systemui.statusbar.notification.dagger.NotificationsModule; import com.android.systemui.statusbar.notification.logging.NotificationCounters; @@ -48,6 +50,7 @@ public class NotificationBlockingHelperManager { private final NotificationGutsManager mNotificationGutsManager; private final NotificationEntryManager mNotificationEntryManager; private final MetricsLogger mMetricsLogger; + private final GroupMembershipManager mGroupMembershipManager; /** Row that the blocking helper will be shown in (via {@link NotificationGuts}. */ private ExpandableNotificationRow mBlockingHelperRow; private Set mNonBlockablePkgs; @@ -65,7 +68,8 @@ public class NotificationBlockingHelperManager { Context context, NotificationGutsManager notificationGutsManager, NotificationEntryManager notificationEntryManager, - MetricsLogger metricsLogger) { + MetricsLogger metricsLogger, + GroupMembershipManager groupMembershipManager) { mContext = context; mNotificationGutsManager = notificationGutsManager; mNotificationEntryManager = notificationEntryManager; @@ -73,6 +77,7 @@ public class NotificationBlockingHelperManager { mNonBlockablePkgs = new HashSet<>(); Collections.addAll(mNonBlockablePkgs, mContext.getResources().getStringArray( com.android.internal.R.array.config_nonBlockableNotificationPackages)); + mGroupMembershipManager = groupMembershipManager; } /** @@ -92,11 +97,12 @@ public class NotificationBlockingHelperManager { // - The row is blockable (i.e. not non-blockable) // - The dismissed row is a valid group (>1 or 0 children from the same channel) // or the only child in the group - if ((row.getEntry().getUserSentiment() == USER_SENTIMENT_NEGATIVE || DEBUG) + final NotificationEntry entry = row.getEntry(); + if ((entry.getUserSentiment() == USER_SENTIMENT_NEGATIVE || DEBUG) && mIsShadeExpanded && !row.getIsNonblockable() - && ((!row.isChildInGroup() || row.isOnlyChildInGroup()) - && row.getNumUniqueChannels() <= 1)) { + && ((!row.isChildInGroup() || mGroupMembershipManager.isOnlyChildInGroup(entry)) + && row.getNumUniqueChannels() <= 1)) { // Dismiss any current blocking helper before continuing forward (only one can be shown // at a given time). dismissCurrentBlockingHelper(); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java index f80656706f37e..737cdeba797a6 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java @@ -281,17 +281,10 @@ public class StatusBarNotificationActivityStarter implements NotificationActivit // TODO: Some of this code may be able to move to NotificationEntryManager. removeHUN(row); - NotificationEntry parentToCancel = null; - if (shouldAutoCancel(entry.getSbn()) && mGroupMembershipManager.isOnlyChildInGroup(entry)) { - NotificationEntry summarySbn = mGroupMembershipManager.getLogicalGroupSummary(entry); - if (shouldAutoCancel(summarySbn.getSbn())) { - parentToCancel = summarySbn; - } - } - final NotificationEntry parentToCancelFinal = parentToCancel; + final Runnable runnable = () -> handleNotificationClickAfterPanelCollapsed( entry, row, controller, intent, - isActivityIntent, wasOccluded, parentToCancelFinal); + isActivityIntent, wasOccluded); if (showOverLockscreen) { mShadeController.addPostCollapseAction(runnable); @@ -312,8 +305,7 @@ public class StatusBarNotificationActivityStarter implements NotificationActivit RemoteInputController controller, PendingIntent intent, boolean isActivityIntent, - boolean wasOccluded, - NotificationEntry parentToCancelFinal) { + boolean wasOccluded) { String notificationKey = entry.getKey(); mLogger.logHandleClickAfterPanelCollapsed(notificationKey); @@ -373,22 +365,23 @@ public class StatusBarNotificationActivityStarter implements NotificationActivit NotificationLogger.getNotificationLocation(entry); final NotificationVisibility nv = NotificationVisibility.obtain(notificationKey, rank, count, true, location); + + // NMS will officially remove notification if the notification has FLAG_AUTO_CANCEL: mClickNotifier.onNotificationClick(notificationKey, nv); - if (!canBubble) { - if (parentToCancelFinal != null) { - // TODO: (b/145659174) remove - this cancels the parent if the notification clicked - // on will auto-cancel and is the only child in the group. This won't be - // necessary in the new pipeline due to group pruning in ShadeListBuilder. - removeNotification(parentToCancelFinal); - } + // TODO (b/162832756): delete these notification removals when migrating to the new + // pipeline; this is taken care of in {@link NotifCollection#tryRemoveNotification} + // which cancels lifetime extenders if the notification was dismissed by the user (ie: + // clicked or manually dismissed) + if (!canBubble && !mFeatureFlags.isNewNotifPipelineRenderingEnabled()) { if (shouldAutoCancel(entry.getSbn()) || mRemoteInputManager.isNotificationKeptForRemoteInputHistory( notificationKey)) { - // Automatically remove all notifications that we may have kept around longer + // manually call notification removal in order to cancel any lifetime extenders removeNotification(row.getEntry()); } } + mIsCollapsingToShowActivityOverLockscreen = false; } 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 359faba48f084..ce0f1220fc881 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 @@ -442,7 +442,7 @@ public class NotifCollectionTest extends SysuiTestCase { } @Test - public void testDismissingLifetimeExtendedSummaryDoesNotDismissChildren() { + public void testRetractingLifetimeExtendedSummaryDoesNotDismissChildren() { // GIVEN A notif group with one summary and two children mCollection.addNotificationLifetimeExtender(mExtender1); CollectionEvent notif1 = postNotif( @@ -460,15 +460,16 @@ public class NotifCollectionTest extends SysuiTestCase { NotificationEntry entry2 = mCollectionListener.getEntry(notif2.key); NotificationEntry entry3 = mCollectionListener.getEntry(notif3.key); - // GIVEN that the summary and one child are retracted, but both are lifetime-extended + // GIVEN that the summary and one child are retracted by the app, but both are + // lifetime-extended mExtender1.shouldExtendLifetime = true; - mNoMan.retractNotif(notif1.sbn, REASON_CANCEL); - mNoMan.retractNotif(notif2.sbn, REASON_CANCEL); + mNoMan.retractNotif(notif1.sbn, REASON_APP_CANCEL); + mNoMan.retractNotif(notif2.sbn, REASON_APP_CANCEL); assertEquals( new ArraySet<>(List.of(entry1, entry2, entry3)), new ArraySet<>(mCollection.getAllNotifs())); - // WHEN the summary is dismissed by the user + // WHEN the summary is retracted by the app mCollection.dismissNotification(entry1, defaultStats(entry1)); // THEN the summary is removed, but both children stick around @@ -479,6 +480,28 @@ public class NotifCollectionTest extends SysuiTestCase { assertEquals(NOT_DISMISSED, entry3.getDismissState()); } + @Test + public void testNMSReportsUserDismissalAlwaysRemovesNotif() throws RemoteException { + // GIVEN notifications are lifetime extended + mExtender1.shouldExtendLifetime = true; + CollectionEvent notif = postNotif(buildNotif(TEST_PACKAGE, 1, "myTag")); + CollectionEvent notif2 = postNotif(buildNotif(TEST_PACKAGE, 2, "myTag")); + NotificationEntry entry = mCollectionListener.getEntry(notif.key); + NotificationEntry entry2 = mCollectionListener.getEntry(notif2.key); + assertEquals( + new ArraySet<>(List.of(entry, entry2)), + new ArraySet<>(mCollection.getAllNotifs())); + + // WHEN the notifications are reported to be dismissed by the user by NMS + mNoMan.retractNotif(notif.sbn, REASON_CANCEL); + mNoMan.retractNotif(notif2.sbn, REASON_CLICK); + + // THEN the notifications are removed b/c they were dismissed by the user + assertEquals( + new ArraySet<>(List.of()), + new ArraySet<>(mCollection.getAllNotifs())); + } + @Test public void testDismissNotificationCallsDismissInterceptors() throws RemoteException { // GIVEN a collection with notifications with multiple dismiss interceptors @@ -833,13 +856,13 @@ public class NotifCollectionTest extends SysuiTestCase { NotifEvent notif2 = mNoMan.postNotif(buildNotif(TEST_PACKAGE2, 88)); NotificationEntry entry2 = mCollectionListener.getEntry(notif2.key); - // WHEN a notification is removed - mNoMan.retractNotif(notif2.sbn, REASON_CLICK); + // WHEN a notification is removed by the app + mNoMan.retractNotif(notif2.sbn, REASON_APP_CANCEL); // THEN each extender is asked whether to extend, even if earlier ones return true - verify(mExtender1).shouldExtendLifetime(entry2, REASON_CLICK); - verify(mExtender2).shouldExtendLifetime(entry2, REASON_CLICK); - verify(mExtender3).shouldExtendLifetime(entry2, REASON_CLICK); + verify(mExtender1).shouldExtendLifetime(entry2, REASON_APP_CANCEL); + verify(mExtender2).shouldExtendLifetime(entry2, REASON_APP_CANCEL); + verify(mExtender3).shouldExtendLifetime(entry2, REASON_APP_CANCEL); // THEN the entry is not removed assertTrue(mCollection.getAllNotifs().contains(entry2)); diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationBlockingHelperManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationBlockingHelperManagerTest.java index 5aeb43fbd9590..edb8776bcb021 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationBlockingHelperManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationBlockingHelperManagerTest.java @@ -49,6 +49,7 @@ import com.android.systemui.SysuiTestCase; import com.android.systemui.bubbles.BubbleController; import com.android.systemui.plugins.statusbar.NotificationMenuRowPlugin; import com.android.systemui.statusbar.notification.NotificationEntryManager; +import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager; import org.junit.Before; import org.junit.Test; @@ -71,6 +72,7 @@ public class NotificationBlockingHelperManagerTest extends SysuiTestCase { @Mock private NotificationEntryManager mEntryManager; @Mock private NotificationMenuRow mMenuRow; @Mock private NotificationMenuRowPlugin.MenuItem mMenuItem; + @Mock private GroupMembershipManager mGroupMembershipManager; @Before public void setUp() { @@ -89,7 +91,8 @@ public class NotificationBlockingHelperManagerTest extends SysuiTestCase { mHelper = new NotificationTestHelper(mContext, mDependency, TestableLooper.get(this)); mBlockingHelperManager = new NotificationBlockingHelperManager( - mContext, mGutsManager, mEntryManager, mock(MetricsLogger.class)); + mContext, mGutsManager, mEntryManager, mock(MetricsLogger.class), + mGroupMembershipManager); // By default, have the shade visible/expanded. mBlockingHelperManager.setNotificationShadeExpanded(1f); } @@ -185,6 +188,7 @@ public class NotificationBlockingHelperManagerTest extends SysuiTestCase { .build(); assertFalse(childRow.getIsNonblockable()); + when(mGroupMembershipManager.isOnlyChildInGroup(childRow.getEntry())).thenReturn(true); assertTrue(mBlockingHelperManager.perhapsShowBlockingHelper(childRow, mMenuRow)); verify(mGutsManager).openGuts(childRow, 0, 0, mMenuItem);