From aa8fef22b728541b2413435adbe69164a647c4d8 Mon Sep 17 00:00:00 2001 From: Mady Mellor Date: Thu, 11 Apr 2019 13:36:40 -0700 Subject: [PATCH] Remove the bubble if the notification is no longer FLAG_BUBBLE Previously if a notification that was a bubble got updated such that it would fail our bubble criteria, we wouldn't actually remove the bubble we would just stop updating that bubble. This CL fixes it so that we'll remove the bubble in that case. This CL also factors all of the 'shouldBubble' logic into Notification InterruptionStateProvider. Fixes: 128459529 Test: CTSVerifier tests in other CL; atest BubbleControllerTest Change-Id: Ia2b53a25a28d53bd2bbaebe4fe89f50d8d46a9fb --- .../systemui/bubbles/BubbleController.java | 38 ++++++++-------- ...NotificationInterruptionStateProvider.java | 18 +++++++- .../bubbles/BubbleControllerTest.java | 43 +++++++++++++++++-- .../statusbar/NotificationTestHelper.java | 5 ++- 4 files changed, 77 insertions(+), 27 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/bubbles/BubbleController.java b/packages/SystemUI/src/com/android/systemui/bubbles/BubbleController.java index acdcfb2ea688d..9168bf3406bf0 100644 --- a/packages/SystemUI/src/com/android/systemui/bubbles/BubbleController.java +++ b/packages/SystemUI/src/com/android/systemui/bubbles/BubbleController.java @@ -83,7 +83,7 @@ public class BubbleController implements ConfigurationController.ConfigurationLi @Retention(SOURCE) @IntDef({DISMISS_USER_GESTURE, DISMISS_AGED, DISMISS_TASK_FINISHED, DISMISS_BLOCKED, - DISMISS_NOTIF_CANCEL, DISMISS_ACCESSIBILITY_ACTION}) + DISMISS_NOTIF_CANCEL, DISMISS_ACCESSIBILITY_ACTION, DISMISS_NO_LONGER_BUBBLE}) @interface DismissReason {} static final int DISMISS_USER_GESTURE = 1; @@ -92,6 +92,7 @@ public class BubbleController implements ConfigurationController.ConfigurationLi static final int DISMISS_BLOCKED = 4; static final int DISMISS_NOTIF_CANCEL = 5; static final int DISMISS_ACCESSIBILITY_ACTION = 6; + static final int DISMISS_NO_LONGER_BUBBLE = 7; static final int MAX_BUBBLES = 5; // TODO: actually enforce this @@ -126,8 +127,7 @@ public class BubbleController implements ConfigurationController.ConfigurationLi private final StatusBarWindowController mStatusBarWindowController; private StatusBarStateListener mStatusBarStateListener; - private final NotificationInterruptionStateProvider mNotificationInterruptionStateProvider = - Dependency.get(NotificationInterruptionStateProvider.class); + private final NotificationInterruptionStateProvider mNotificationInterruptionStateProvider; private INotificationManager mNotificationManagerService; @@ -183,15 +183,19 @@ public class BubbleController implements ConfigurationController.ConfigurationLi @Inject public BubbleController(Context context, StatusBarWindowController statusBarWindowController, - BubbleData data, ConfigurationController configurationController) { + BubbleData data, ConfigurationController configurationController, + NotificationInterruptionStateProvider interruptionStateProvider) { this(context, statusBarWindowController, data, null /* synchronizer */, - configurationController); + configurationController, interruptionStateProvider); } public BubbleController(Context context, StatusBarWindowController statusBarWindowController, BubbleData data, @Nullable BubbleStackView.SurfaceSynchronizer synchronizer, - ConfigurationController configurationController) { + ConfigurationController configurationController, + NotificationInterruptionStateProvider interruptionStateProvider) { mContext = context; + mNotificationInterruptionStateProvider = interruptionStateProvider; + configurationController.addCallback(this /* configurationListener */); mNotificationEntryManager = Dependency.get(NotificationEntryManager.class); @@ -380,7 +384,7 @@ public class BubbleController implements ConfigurationController.ConfigurationLi if (!areBubblesEnabled(mContext)) { return; } - if (shouldAutoBubbleForFlags(mContext, entry) || shouldBubble(entry)) { + if (mNotificationInterruptionStateProvider.shouldBubbleUp(entry)) { // TODO: handle group summaries? updateShowInShadeForSuppressNotification(entry); } @@ -391,7 +395,7 @@ public class BubbleController implements ConfigurationController.ConfigurationLi if (!areBubblesEnabled(mContext)) { return; } - if (entry.isBubble() && mNotificationInterruptionStateProvider.shouldBubbleUp(entry)) { + if (mNotificationInterruptionStateProvider.shouldBubbleUp(entry)) { updateBubble(entry); } } @@ -401,8 +405,11 @@ public class BubbleController implements ConfigurationController.ConfigurationLi if (!areBubblesEnabled(mContext)) { return; } - if (mNotificationInterruptionStateProvider.shouldBubbleUp(entry) - && alertAgain(entry, entry.notification.getNotification())) { + boolean shouldBubble = mNotificationInterruptionStateProvider.shouldBubbleUp(entry); + if (!shouldBubble && mBubbleData.hasBubbleWithKey(entry.key)) { + // It was previously a bubble but no longer a bubble -- lets remove it + removeBubble(entry.key, DISMISS_NO_LONGER_BUBBLE); + } else if (shouldBubble && alertAgain(entry, entry.notification.getNotification())) { updateShowInShadeForSuppressNotification(entry); entry.setBubbleDismissed(false); // updates come back as bubbles even if dismissed updateBubble(entry); @@ -528,17 +535,6 @@ public class BubbleController implements ConfigurationController.ConfigurationLi return mStackView; } - /** - * Whether the notification has been developer configured to bubble and is allowed by the user. - */ - @VisibleForTesting - protected boolean shouldBubble(NotificationEntry entry) { - StatusBarNotification n = entry.notification; - boolean hasOverlayIntent = n.getNotification().getBubbleMetadata() != null - && n.getNotification().getBubbleMetadata().getIntent() != null; - return hasOverlayIntent && entry.canBubble; - } - /** * Whether the notification should automatically bubble or not. Gated by secure settings flags. */ diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationInterruptionStateProvider.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationInterruptionStateProvider.java index 059c3f672b695..926d4b6a79b39 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationInterruptionStateProvider.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationInterruptionStateProvider.java @@ -147,7 +147,14 @@ public class NotificationInterruptionStateProvider { * @return true if the entry should bubble up, false otherwise */ public boolean shouldBubbleUp(NotificationEntry entry) { - StatusBarNotification sbn = entry.notification; + final StatusBarNotification sbn = entry.notification; + if (!entry.canBubble) { + if (DEBUG) { + Log.d(TAG, "No bubble up: not allowed to bubble: " + sbn.getKey()); + } + return false; + } + if (!entry.isBubble()) { if (DEBUG) { Log.d(TAG, "No bubble up: notification " + sbn.getKey() @@ -156,6 +163,15 @@ public class NotificationInterruptionStateProvider { return false; } + final Notification n = sbn.getNotification(); + if (n.getBubbleMetadata() == null || n.getBubbleMetadata().getIntent() == null) { + if (DEBUG) { + Log.d(TAG, "No bubble up: notification: " + sbn.getKey() + + " doesn't have valid metadata"); + } + return false; + } + if (!canHeadsUpCommon(entry)) { return false; } diff --git a/packages/SystemUI/tests/src/com/android/systemui/bubbles/BubbleControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/bubbles/BubbleControllerTest.java index 40a53571ddf0c..8b0ba9448d2b9 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/bubbles/BubbleControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/bubbles/BubbleControllerTest.java @@ -16,6 +16,7 @@ package com.android.systemui.bubbles; +import static android.app.Notification.FLAG_BUBBLE; import static android.content.Intent.FLAG_ACTIVITY_NEW_TASK; import static com.google.common.truth.Truth.assertThat; @@ -25,6 +26,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -47,15 +49,18 @@ import androidx.test.filters.SmallTest; import com.android.systemui.R; import com.android.systemui.SysuiTestCase; +import com.android.systemui.statusbar.NotificationPresenter; import com.android.systemui.statusbar.NotificationTestHelper; import com.android.systemui.statusbar.notification.NotificationEntryListener; import com.android.systemui.statusbar.notification.NotificationEntryManager; +import com.android.systemui.statusbar.notification.NotificationInterruptionStateProvider; import com.android.systemui.statusbar.notification.collection.NotificationData; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow; import com.android.systemui.statusbar.phone.DozeParameters; import com.android.systemui.statusbar.phone.StatusBarWindowController; import com.android.systemui.statusbar.policy.ConfigurationController; +import com.android.systemui.statusbar.policy.HeadsUpManager; import org.junit.Before; import org.junit.Test; @@ -138,7 +143,7 @@ public class BubbleControllerTest extends SysuiTestCase { // Some bubbles want to suppress notifs Notification.BubbleMetadata suppressNotifMetadata = - getBuilder().setSuppressInitialNotification(true).build(); + getBuilder().setSuppressNotification(true).build(); mSuppressNotifRow = mNotificationTestHelper.createBubble(suppressNotifMetadata, FOREGROUND_TEST_PKG_NAME); @@ -146,9 +151,15 @@ public class BubbleControllerTest extends SysuiTestCase { when(mNotificationEntryManager.getNotificationData()).thenReturn(mNotificationData); when(mNotificationData.getChannel(mRow.getEntry().key)).thenReturn(mRow.getEntry().channel); + TestableNotificationInterruptionStateProvider interruptionStateProvider = + new TestableNotificationInterruptionStateProvider(mContext); + interruptionStateProvider.setUpWithPresenter( + mock(NotificationPresenter.class), + mock(HeadsUpManager.class), + mock(NotificationInterruptionStateProvider.HeadsUpSuppressor.class)); mBubbleData = new BubbleData(mContext); mBubbleController = new TestableBubbleController(mContext, mStatusBarWindowController, - mBubbleData, mConfigurationController); + mBubbleData, mConfigurationController, interruptionStateProvider); mBubbleController.setBubbleStateChangeListener(mBubbleStateChangeListener); mBubbleController.setExpandListener(mBubbleExpandListener); @@ -487,12 +498,27 @@ public class BubbleControllerTest extends SysuiTestCase { verify(mDeleteIntent, times(2)).send(); } + @Test + public void testRemoveBubble_noLongerBubbleAfterUpdate() + throws PendingIntent.CanceledException { + mBubbleController.updateBubble(mRow.getEntry()); + assertTrue(mBubbleController.hasBubbles()); + + mRow.getEntry().notification.getNotification().flags &= ~FLAG_BUBBLE; + mEntryListener.onPreEntryUpdated(mRow.getEntry()); + + assertFalse(mBubbleController.hasBubbles()); + verify(mDeleteIntent, never()).send(); + } + static class TestableBubbleController extends BubbleController { // Let's assume surfaces can be synchronized immediately. TestableBubbleController(Context context, StatusBarWindowController statusBarWindowController, BubbleData data, - ConfigurationController configurationController) { - super(context, statusBarWindowController, data, Runnable::run, configurationController); + ConfigurationController configurationController, + NotificationInterruptionStateProvider interruptionStateProvider) { + super(context, statusBarWindowController, data, Runnable::run, + configurationController, interruptionStateProvider); } @Override @@ -501,6 +527,15 @@ public class BubbleControllerTest extends SysuiTestCase { } } + public static class TestableNotificationInterruptionStateProvider extends + NotificationInterruptionStateProvider { + + public TestableNotificationInterruptionStateProvider(Context context) { + super(context); + mUseHeadsUp = true; + } + } + /** * @return basic {@link android.app.Notification.BubbleMetadata.Builder} */ diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationTestHelper.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationTestHelper.java index 8cc1571b925ff..e4b90c54d5b31 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationTestHelper.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/NotificationTestHelper.java @@ -178,7 +178,10 @@ public class NotificationTestHelper { Notification n = createNotification(false /* isGroupSummary */, null /* groupKey */, bubbleMetadata); n.flags |= FLAG_BUBBLE; - return generateRow(n, pkg, UID, USER_HANDLE, 0 /* extraInflationFlags */, IMPORTANCE_HIGH); + ExpandableNotificationRow row = generateRow(n, pkg, UID, USER_HANDLE, + 0 /* extraInflationFlags */, IMPORTANCE_HIGH); + row.getEntry().canBubble = true; + return row; } /**