From 6a2fc4a9080eeae9b1fb20f433117d7c3657151e Mon Sep 17 00:00:00 2001 From: Mady Mellor Date: Wed, 13 Jul 2022 13:35:54 -0700 Subject: [PATCH 1/3] Ignore updates to bubbles that aren't from the system The new notif pipeline code adds a concept of updates from the system vs from sysui. Currently bubbles is responding to all updates but I think we really only care about updates from the system (i.e. from NotificationManagerService). Bug: 238896626 Test: atest BubblesTest#testNonSystemUpdatesIgnored Test: atest --no-bazel-mode --iterations 20 PlatformScenarioTests:android.platform.test.scenario.sysui.bubble.ExpandAndDragToDismissTest (with vadim's CLs and a 2nd change to be uploaded) Change-Id: I3ffd6eb0f0a07448b2117924be514347c6e80b16 --- .../wm/shell/bubbles/BubbleController.java | 14 ++++++++----- .../com/android/wm/shell/bubbles/Bubbles.java | 3 ++- .../systemui/wmshell/BubblesManager.java | 9 ++++---- .../android/systemui/wmshell/BubblesTest.java | 21 ++++++++++++++++--- 4 files changed, 34 insertions(+), 13 deletions(-) diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java index 398261600dc7f..2b8c1eb455684 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java @@ -1082,7 +1082,8 @@ public class BubbleController implements ConfigurationChangeListener { } } - void inflateAndAdd(Bubble bubble, boolean suppressFlyout, boolean showInShade) { + @VisibleForTesting + public void inflateAndAdd(Bubble bubble, boolean suppressFlyout, boolean showInShade) { // Lazy init stack view when a bubble is created ensureStackViewCreated(); bubble.setInflateSynchronously(mInflateSynchronously); @@ -1111,7 +1112,10 @@ public class BubbleController implements ConfigurationChangeListener { } @VisibleForTesting - public void onEntryUpdated(BubbleEntry entry, boolean shouldBubbleUp) { + public void onEntryUpdated(BubbleEntry entry, boolean shouldBubbleUp, boolean fromSystem) { + if (!fromSystem) { + return; + } // shouldBubbleUp checks canBubble & for bubble metadata boolean shouldBubble = shouldBubbleUp && canLaunchInTaskView(mContext, entry); if (!shouldBubble && mBubbleData.hasAnyBubbleWithKey(entry.getKey())) { @@ -1174,7 +1178,7 @@ public class BubbleController implements ConfigurationChangeListener { mBubbleData.dismissBubbleWithKey(key, DISMISS_NO_BUBBLE_UP); } else if (entry != null && mTmpRanking.isBubble() && !isActive) { entry.setFlagBubble(true); - onEntryUpdated(entry, shouldBubbleUp); + onEntryUpdated(entry, shouldBubbleUp, /* fromSystem= */ true); } } } @@ -1773,9 +1777,9 @@ public class BubbleController implements ConfigurationChangeListener { } @Override - public void onEntryUpdated(BubbleEntry entry, boolean shouldBubbleUp) { + public void onEntryUpdated(BubbleEntry entry, boolean shouldBubbleUp, boolean fromSystem) { mMainExecutor.execute(() -> { - BubbleController.this.onEntryUpdated(entry, shouldBubbleUp); + BubbleController.this.onEntryUpdated(entry, shouldBubbleUp, fromSystem); }); } diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubbles.java b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubbles.java index cf792cda91b5c..37b96ffe5cd1e 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubbles.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubbles.java @@ -171,8 +171,9 @@ public interface Bubbles { * * @param entry the {@link BubbleEntry} by the notification. * @param shouldBubbleUp {@code true} if this notification should bubble up. + * @param fromSystem {@code true} if this update is from NotificationManagerService. */ - void onEntryUpdated(BubbleEntry entry, boolean shouldBubbleUp); + void onEntryUpdated(BubbleEntry entry, boolean shouldBubbleUp, boolean fromSystem); /** * Called when new notification entry removed. diff --git a/packages/SystemUI/src/com/android/systemui/wmshell/BubblesManager.java b/packages/SystemUI/src/com/android/systemui/wmshell/BubblesManager.java index e22a896227ef6..4c762702892a0 100644 --- a/packages/SystemUI/src/com/android/systemui/wmshell/BubblesManager.java +++ b/packages/SystemUI/src/com/android/systemui/wmshell/BubblesManager.java @@ -405,8 +405,8 @@ public class BubblesManager implements Dumpable { } @Override - public void onEntryUpdated(NotificationEntry entry) { - BubblesManager.this.onEntryUpdated(entry); + public void onEntryUpdated(NotificationEntry entry, boolean fromSystem) { + BubblesManager.this.onEntryUpdated(entry, fromSystem); } @Override @@ -444,9 +444,10 @@ public class BubblesManager implements Dumpable { } } - void onEntryUpdated(NotificationEntry entry) { + void onEntryUpdated(NotificationEntry entry, boolean fromSystem) { + boolean shouldBubble = mNotificationInterruptStateProvider.shouldBubbleUp(entry); mBubbles.onEntryUpdated(notifToBubbleEntry(entry), - mNotificationInterruptStateProvider.shouldBubbleUp(entry)); + shouldBubble, fromSystem); } void onEntryRemoved(NotificationEntry entry) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java b/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java index fee17c785ed28..25f83fcd4bcca 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java @@ -33,6 +33,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -621,7 +622,7 @@ public class BubblesTest extends SysuiTestCase { assertFalse(mBubbleData.getBubbleInStackWithKey(mRow.getKey()).showDot()); // Send update - mEntryListener.onEntryUpdated(mRow); + mEntryListener.onEntryUpdated(mRow, /* fromSystem= */ true); // Nothing should have changed // Notif is suppressed after expansion @@ -789,7 +790,7 @@ public class BubblesTest extends SysuiTestCase { @Test public void testAddNotif_notBubble() { mEntryListener.onEntryAdded(mNonBubbleNotifRow.getEntry()); - mEntryListener.onEntryUpdated(mNonBubbleNotifRow.getEntry()); + mEntryListener.onEntryUpdated(mNonBubbleNotifRow.getEntry(), /* fromSystem= */ true); assertThat(mBubbleController.hasBubbles()).isFalse(); } @@ -827,7 +828,7 @@ public class BubblesTest extends SysuiTestCase { NotificationListenerService.Ranking ranking = new RankingBuilder( mRow.getRanking()).setCanBubble(false).build(); mRow.setRanking(ranking); - mEntryListener.onEntryUpdated(mRow); + mEntryListener.onEntryUpdated(mRow, /* fromSystem= */ true); assertFalse(mBubbleController.hasBubbles()); verify(mDeleteIntent, never()).send(); @@ -1432,6 +1433,20 @@ public class BubblesTest extends SysuiTestCase { assertThat(mBubbleData.hasBubbleInStackWithKey(mBubbleEntry.getKey())).isFalse(); } + @Test + public void testNonSystemUpdatesIgnored() { + mEntryListener.onEntryAdded(mRow); + assertThat(mBubbleController.hasBubbles()).isTrue(); + + mEntryListener.onEntryUpdated(mRow, /* fromSystem= */ false); + mEntryListener.onEntryUpdated(mRow, /* fromSystem= */ false); + mEntryListener.onEntryUpdated(mRow, /* fromSystem= */ false); + + // Check that it wasn't inflated (1 because it would've been inflated via onEntryAdded) + verify(mBubbleController, times(1)).inflateAndAdd( + any(Bubble.class), anyBoolean(), anyBoolean()); + } + /** Creates a bubble using the userId and package. */ private Bubble createBubble(int userId, String pkg) { final UserHandle userHandle = new UserHandle(userId); From 55524d2a78d4f9f55d509c4f167fed4cfc0135a8 Mon Sep 17 00:00:00 2001 From: Mady Mellor Date: Fri, 22 Jul 2022 14:53:59 -0700 Subject: [PATCH 2/3] Don't bubble things when we shouldn't I haven't been able to repro these issues outside of tests on cuttlefish, but I think logically these fixes make sense & seem to resolve the test issue. - Ensure non-notifying updates to bubbles in the overflow don't clobber the FLAG_BUBBLE state - onRankingUpdate should only bubble stuff up if it's not active and not in the overflow (previously just not active) Bug: 238896626 Test: atest BubblesTest#testNonInterruptiveUpdate_doesntOverrideOverflowFlagBubble Test: atest --no-bazel-mode --iterations 20 PlatformScenarioTests:android.platform.test.scenario.sysui.bubble.ExpandAndDragToDismissTest (with vadim's changes & another CL) Change-Id: Id588c1130e62533a23dc931dfb4fe3c92125a905 --- .../wm/shell/bubbles/BubbleController.java | 5 +++- .../android/systemui/wmshell/BubblesTest.java | 29 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java index 2b8c1eb455684..9c3bc288a58f4 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java @@ -1061,6 +1061,9 @@ public class BubbleController implements ConfigurationChangeListener { && mBubbleData.hasOverflowBubbleWithKey(notif.getKey())) { // Update the bubble but don't promote it out of overflow Bubble b = mBubbleData.getOverflowBubbleWithKey(notif.getKey()); + if (notif.isBubble()) { + notif.setFlagBubble(false); + } b.setEntry(notif); } else if (mBubbleData.isSuppressedWithLocusId(notif.getLocusId())) { // Update the bubble but don't promote it out of overflow @@ -1176,7 +1179,7 @@ public class BubbleController implements ConfigurationChangeListener { // notification, so that the bubble will be re-created if shouldBubbleUp returns // true. mBubbleData.dismissBubbleWithKey(key, DISMISS_NO_BUBBLE_UP); - } else if (entry != null && mTmpRanking.isBubble() && !isActive) { + } else if (entry != null && mTmpRanking.isBubble() && !isActiveOrInOverflow) { entry.setFlagBubble(true); onEntryUpdated(entry, shouldBubbleUp, /* fromSystem= */ true); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java b/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java index 25f83fcd4bcca..ff95969f1e53d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java @@ -1433,6 +1433,35 @@ public class BubblesTest extends SysuiTestCase { assertThat(mBubbleData.hasBubbleInStackWithKey(mBubbleEntry.getKey())).isFalse(); } + /** + * Verifies that if a bubble is in the overflow and a non-interruptive notification update + * comes in for it with FLAG_BUBBLE that the flag is removed. + */ + @Test + public void testNonInterruptiveUpdate_doesntOverrideOverflowFlagBubble() { + mEntryListener.onEntryAdded(mRow); + mEntryListener.onEntryUpdated(mRow, /* fromSystem= */ true); + assertBubbleNotificationNotSuppressedFromShade(mBubbleEntry); + + // Dismiss the bubble so it's in the overflow + mBubbleController.removeBubble( + mRow.getKey(), Bubbles.DISMISS_USER_GESTURE); + assertThat(mBubbleData.hasOverflowBubbleWithKey(mRow.getKey())).isTrue(); + // Once it's in the overflow it's not actively a bubble (doesn't have FLAG_BUBBLE) + Bubble b = mBubbleData.getOverflowBubbleWithKey(mBubbleEntry.getKey()); + assertThat(b.isBubble()).isFalse(); + + // Send a non-notifying update that has FLAG_BUBBLE + mRow.getSbn().getNotification().flags = FLAG_BUBBLE; + assertThat(mRow.getSbn().getNotification().isBubbleNotification()).isTrue(); + mBubbleController.updateBubble(mBubbleEntry, + /* suppressFlyout= */ false, /* showInShade= */ true); + + // Verify that it still doesn't have FLAG_BUBBLE because it's in the overflow. + b = mBubbleData.getOverflowBubbleWithKey(mBubbleEntry.getKey()); + assertThat(b.isBubble()).isFalse(); + } + @Test public void testNonSystemUpdatesIgnored() { mEntryListener.onEntryAdded(mRow); From 5c5b6218e41cf3fafae745715a931ca9f4043707 Mon Sep 17 00:00:00 2001 From: Mady Mellor Date: Thu, 28 Jul 2022 10:27:42 -0700 Subject: [PATCH 3/3] If the update is non-interruptive don't re-bubble it I haven't been able to reproduce this outside of tests on acloud devices, but if you visit a bubble for the first time (causes an update to the notif) and then dismiss it sometimes the user dismiss can happen before we get the update back from notification manager service and the bubble will get re-bubbled incorrectly. To fix this, if a bubble is in the overflow (or active) and there's an update to the notification that is non-interruptive, we shouldn't re-bubble that content because there wasn't a new message, just some change on the notification. Bug: 238896626 Test: atest BubblesTest Test: atest --no-bazel-mode --iterations 20 PlatformScenarioTests:android.platform.test.scenario.sysui.bubble.ExpandAndDragToDismissTest (with vadim's changes & other CLc) Change-Id: If0b7b3d3d1875ddd1df6a1412db8c42456de35c5 --- .../com/android/wm/shell/bubbles/Bubble.java | 5 ++ .../wm/shell/bubbles/BubbleController.java | 26 ++++++++-- .../android/systemui/wmshell/BubblesTest.java | 51 +++++++++++++++++++ 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubble.java b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubble.java index 31fc6a5be5895..2c02006c8ca5d 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubble.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/Bubble.java @@ -452,6 +452,7 @@ public class Bubble implements BubbleViewProvider { */ void setEntry(@NonNull final BubbleEntry entry) { Objects.requireNonNull(entry); + boolean showingDotPreviously = showDot(); mLastUpdated = entry.getStatusBarNotification().getPostTime(); mIsBubble = entry.getStatusBarNotification().getNotification().isBubbleNotification(); mPackageName = entry.getStatusBarNotification().getPackageName(); @@ -498,6 +499,10 @@ public class Bubble implements BubbleViewProvider { mShouldSuppressNotificationDot = entry.shouldSuppressNotificationDot(); mShouldSuppressNotificationList = entry.shouldSuppressNotificationList(); mShouldSuppressPeek = entry.shouldSuppressPeek(); + if (showingDotPreviously != showDot()) { + // This will update the UI if needed + setShowDot(showDot()); + } } @Nullable diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java index 9c3bc288a58f4..8771ceb71d989 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleController.java @@ -1055,21 +1055,28 @@ public class BubbleController implements ConfigurationChangeListener { public void updateBubble(BubbleEntry notif, boolean suppressFlyout, boolean showInShade) { // If this is an interruptive notif, mark that it's interrupted mSysuiProxy.setNotificationInterruption(notif.getKey()); - if (!notif.getRanking().isTextChanged() + boolean isNonInterruptiveNotExpanding = !notif.getRanking().isTextChanged() && (notif.getBubbleMetadata() != null - && !notif.getBubbleMetadata().getAutoExpandBubble()) + && !notif.getBubbleMetadata().getAutoExpandBubble()); + if (isNonInterruptiveNotExpanding && mBubbleData.hasOverflowBubbleWithKey(notif.getKey())) { // Update the bubble but don't promote it out of overflow Bubble b = mBubbleData.getOverflowBubbleWithKey(notif.getKey()); if (notif.isBubble()) { notif.setFlagBubble(false); } - b.setEntry(notif); + updateNotNotifyingEntry(b, notif, showInShade); + } else if (mBubbleData.hasAnyBubbleWithKey(notif.getKey()) + && isNonInterruptiveNotExpanding) { + Bubble b = mBubbleData.getAnyBubbleWithkey(notif.getKey()); + if (b != null) { + updateNotNotifyingEntry(b, notif, showInShade); + } } else if (mBubbleData.isSuppressedWithLocusId(notif.getLocusId())) { // Update the bubble but don't promote it out of overflow Bubble b = mBubbleData.getSuppressedBubbleWithKey(notif.getKey()); if (b != null) { - b.setEntry(notif); + updateNotNotifyingEntry(b, notif, showInShade); } } else { Bubble bubble = mBubbleData.getOrCreateBubble(notif, null /* persistedBubble */); @@ -1079,12 +1086,23 @@ public class BubbleController implements ConfigurationChangeListener { if (bubble.shouldAutoExpand()) { bubble.setShouldAutoExpand(false); } + mImpl.mCachedState.updateBubbleSuppressedState(bubble); } else { inflateAndAdd(bubble, suppressFlyout, showInShade); } } } + void updateNotNotifyingEntry(Bubble b, BubbleEntry entry, boolean showInShade) { + boolean isBubbleSelected = Objects.equals(b, mBubbleData.getSelectedBubble()); + boolean isBubbleExpandedAndSelected = isStackExpanded() && isBubbleSelected; + b.setEntry(entry); + boolean suppress = isBubbleExpandedAndSelected || !showInShade || !b.showInShade(); + b.setSuppressNotification(suppress); + b.setShowDot(!isBubbleExpandedAndSelected); + mImpl.mCachedState.updateBubbleSuppressedState(b); + } + @VisibleForTesting public void inflateAndAdd(Bubble bubble, boolean suppressFlyout, boolean showInShade) { // Lazy init stack view when a bubble is created diff --git a/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java b/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java index ff95969f1e53d..18acf3f6ce531 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/wmshell/BubblesTest.java @@ -1433,6 +1433,57 @@ public class BubblesTest extends SysuiTestCase { assertThat(mBubbleData.hasBubbleInStackWithKey(mBubbleEntry.getKey())).isFalse(); } + /** + * Verifies that if a bubble is in the overflow and a non-interruptive notification update + * comes in for it, it stays in the overflow but the entry is updated. + */ + @Test + public void testNonInterruptiveUpdate_doesntBubbleFromOverflow() { + mEntryListener.onEntryAdded(mRow); + mEntryListener.onEntryUpdated(mRow, /* fromSystem= */ true); + assertBubbleNotificationNotSuppressedFromShade(mBubbleEntry); + + // Dismiss the bubble so it's in the overflow + mBubbleController.removeBubble( + mRow.getKey(), Bubbles.DISMISS_USER_GESTURE); + assertThat(mBubbleData.hasOverflowBubbleWithKey(mRow.getKey())).isTrue(); + + // Update the entry to not show in shade + setMetadataFlags(mRow, + Notification.BubbleMetadata.FLAG_SUPPRESS_NOTIFICATION, /* enableFlag= */ true); + mBubbleController.updateBubble(mBubbleEntry, + /* suppressFlyout= */ false, /* showInShade= */ true); + + // Check that the update was applied - shouldn't be show in shade + assertBubbleNotificationSuppressedFromShade(mBubbleEntry); + // Check that it wasn't inflated (1 because it would've been inflated via onEntryAdded) + verify(mBubbleController, times(1)).inflateAndAdd( + any(Bubble.class), anyBoolean(), anyBoolean()); + } + + /** + * Verifies that if a bubble is active, and a non-interruptive notification update comes in for + * it, it doesn't trigger a new inflate and add for that bubble. + */ + @Test + public void testNonInterruptiveUpdate_doesntTriggerInflate() { + mEntryListener.onEntryAdded(mRow); + mEntryListener.onEntryUpdated(mRow, /* fromSystem= */ true); + assertBubbleNotificationNotSuppressedFromShade(mBubbleEntry); + + // Update the entry to not show in shade + setMetadataFlags(mRow, + Notification.BubbleMetadata.FLAG_SUPPRESS_NOTIFICATION, /* enableFlag= */ true); + mBubbleController.updateBubble(mBubbleEntry, + /* suppressFlyout= */ false, /* showInShade= */ true); + + // Check that the update was applied - shouldn't be show in shade + assertBubbleNotificationSuppressedFromShade(mBubbleEntry); + // Check that it wasn't inflated (1 because it would've been inflated via onEntryAdded) + verify(mBubbleController, times(1)).inflateAndAdd( + any(Bubble.class), anyBoolean(), anyBoolean()); + } + /** * Verifies that if a bubble is in the overflow and a non-interruptive notification update * comes in for it with FLAG_BUBBLE that the flag is removed.