From b49ad248f2f8f9d3417745a9d2bd2ab9566badf3 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Thu, 5 May 2022 15:06:17 +0000 Subject: [PATCH] When applying a RankingMap that lacks an entry, remove that entry from the NotifCollection. This fixes a bug after notification removed messages were dropped during moments of mass removal. The NotifCollection was detecting this edge case but ignoring it. Now we'll actually remove the notification and require an add event to get it back. Note: this feature is still flagged off by default. Bug: 216384850 Test: atest NotifCollectionTest Change-Id: Id897c8761e40fd1adedd7c9e5a7bd2f9467d13ed --- .../src/com/android/systemui/flags/Flags.java | 3 + .../notification/NotifPipelineFlags.kt | 5 +- .../collection/NotifCollection.java | 6 ++ .../collection/NoManSimulator.java | 5 ++ .../collection/NotifCollectionTest.java | 74 +++++++++++++++++++ 5 files changed, 92 insertions(+), 1 deletion(-) diff --git a/packages/SystemUI/src/com/android/systemui/flags/Flags.java b/packages/SystemUI/src/com/android/systemui/flags/Flags.java index 3ac0aa98300b0..56bb53b567be1 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/Flags.java +++ b/packages/SystemUI/src/com/android/systemui/flags/Flags.java @@ -60,6 +60,9 @@ public class Flags { public static final ResourceBooleanFlag NOTIFICATION_DRAG_TO_CONTENTS = new ResourceBooleanFlag(108, R.bool.config_notificationToContents); + public static final BooleanFlag REMOVE_UNRANKED_NOTIFICATIONS = + new BooleanFlag(109, false); + /***************************************/ // 200 - keyguard/lockscreen diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt index 478f7aa8ce09e..c4947ca2dd90c 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotifPipelineFlags.kt @@ -56,4 +56,7 @@ class NotifPipelineFlags @Inject constructor( fun isSmartspaceDedupingEnabled(): Boolean = featureFlags.isEnabled(Flags.SMARTSPACE) && featureFlags.isEnabled(Flags.SMARTSPACE_DEDUPING) -} \ No newline at end of file + + fun removeUnrankedNotifs(): Boolean = + featureFlags.isEnabled(Flags.REMOVE_UNRANKED_NOTIFICATIONS) +} 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 4e673c7f56fec..6a01df61705df 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 @@ -601,6 +601,12 @@ public class NotifCollection implements Dumpable, PipelineDumpable { ); mNotificationsWithoutRankings = currentEntriesWithoutRankings == null ? Collections.emptySet() : currentEntriesWithoutRankings.keySet(); + if (currentEntriesWithoutRankings != null && mNotifPipelineFlags.removeUnrankedNotifs()) { + for (NotificationEntry entry : currentEntriesWithoutRankings.values()) { + entry.mCancellationReason = REASON_UNKNOWN; + tryRemoveNotification(entry); + } + } mEventQueue.add(new RankingAppliedEvent()); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NoManSimulator.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NoManSimulator.java index 4507366e30736..ee7d558649314 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NoManSimulator.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NoManSimulator.java @@ -85,6 +85,11 @@ public class NoManSimulator { mRankings.put(key, ranking); } + /** This is for testing error cases: b/216384850 */ + public Ranking removeRankingWithoutEvent(String key) { + return mRankings.remove(key); + } + private RankingMap buildRankingMap() { return new RankingMap(mRankings.values().toArray(new Ranking[0])); } 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 958d54230f1c6..f286349971d27 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 @@ -1491,6 +1491,80 @@ public class NotifCollectionTest extends SysuiTestCase { verify(mCollectionListener, never()).onEntryUpdated(any(), anyBoolean()); } + @Test + public void testMissingRankingWhenRemovalFeatureIsDisabled() { + // GIVEN a pipeline with one two notifications + when(mNotifPipelineFlags.removeUnrankedNotifs()).thenReturn(false); + String key1 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 1, "myTag")).key; + String key2 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 2, "myTag")).key; + NotificationEntry entry1 = mCollectionListener.getEntry(key1); + NotificationEntry entry2 = mCollectionListener.getEntry(key2); + clearInvocations(mCollectionListener); + + // GIVEN the message for removing key1 gets does not reach NotifCollection + Ranking ranking1 = mNoMan.removeRankingWithoutEvent(key1); + // WHEN the message for removing key2 arrives + mNoMan.retractNotif(entry2.getSbn(), REASON_APP_CANCEL); + + // THEN only entry2 gets removed + verify(mCollectionListener).onEntryRemoved(eq(entry2), eq(REASON_APP_CANCEL)); + verify(mCollectionListener).onEntryCleanUp(eq(entry2)); + verify(mCollectionListener).onRankingApplied(); + verifyNoMoreInteractions(mCollectionListener); + verify(mLogger).logMissingRankings(eq(List.of(entry1)), eq(1), any()); + verify(mLogger, never()).logRecoveredRankings(any()); + clearInvocations(mCollectionListener, mLogger); + + // WHEN a ranking update includes key1 again + mNoMan.setRanking(key1, ranking1); + mNoMan.issueRankingUpdate(); + + // VERIFY that we do nothing but log the 'recovery' + verify(mCollectionListener).onRankingUpdate(any()); + verify(mCollectionListener).onRankingApplied(); + verifyNoMoreInteractions(mCollectionListener); + verify(mLogger, never()).logMissingRankings(any(), anyInt(), any()); + verify(mLogger).logRecoveredRankings(eq(List.of(key1))); + } + + @Test + public void testMissingRankingWhenRemovalFeatureIsEnabled() { + // GIVEN a pipeline with one two notifications + when(mNotifPipelineFlags.removeUnrankedNotifs()).thenReturn(true); + String key1 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 1, "myTag")).key; + String key2 = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 2, "myTag")).key; + NotificationEntry entry1 = mCollectionListener.getEntry(key1); + NotificationEntry entry2 = mCollectionListener.getEntry(key2); + clearInvocations(mCollectionListener); + + // GIVEN the message for removing key1 gets does not reach NotifCollection + Ranking ranking1 = mNoMan.removeRankingWithoutEvent(key1); + // WHEN the message for removing key2 arrives + mNoMan.retractNotif(entry2.getSbn(), REASON_APP_CANCEL); + + // THEN both entry1 and entry2 get removed + verify(mCollectionListener).onEntryRemoved(eq(entry2), eq(REASON_APP_CANCEL)); + verify(mCollectionListener).onEntryRemoved(eq(entry1), eq(REASON_UNKNOWN)); + verify(mCollectionListener).onEntryCleanUp(eq(entry2)); + verify(mCollectionListener).onEntryCleanUp(eq(entry1)); + verify(mCollectionListener).onRankingApplied(); + verifyNoMoreInteractions(mCollectionListener); + verify(mLogger).logMissingRankings(eq(List.of(entry1)), eq(1), any()); + verify(mLogger, never()).logRecoveredRankings(any()); + clearInvocations(mCollectionListener, mLogger); + + // WHEN a ranking update includes key1 again + mNoMan.setRanking(key1, ranking1); + mNoMan.issueRankingUpdate(); + + // VERIFY that we do nothing but log the 'recovery' + verify(mCollectionListener).onRankingUpdate(any()); + verify(mCollectionListener).onRankingApplied(); + verifyNoMoreInteractions(mCollectionListener); + verify(mLogger, never()).logMissingRankings(any(), anyInt(), any()); + verify(mLogger).logRecoveredRankings(eq(List.of(key1))); + } + @Test public void testRegisterFutureDismissal() throws RemoteException { // GIVEN a pipeline with one notification