From 0fef5ff26e88b0a5aae08e8e650acf4cb64218c9 Mon Sep 17 00:00:00 2001 From: Jeff Nainaparampil Date: Tue, 27 Dec 2022 21:14:53 +0000 Subject: [PATCH] [People Service] Fix `hasActiveNotifications` to track when notifications are modified Instead of tracking the number of active notifications, we track notifications for a package by their id. This is more robust and allows People Service to keep track of notifications that are updated, such as Messages which updates an existing notification for new incoming messages. This fixes an issue in which recent conversations are retained even if they do not contain active notifications. Bug: 263634015 Change-Id: I187161391a44f9e03840b89d88c4d3c083e1b9bc --- .../server/people/data/DataManager.java | 22 +++++---- .../server/people/data/DataManagerTest.java | 47 +++++++++++++++++-- 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/services/people/java/com/android/server/people/data/DataManager.java b/services/people/java/com/android/server/people/data/DataManager.java index bd0a814e172a7..eff9e8da9a76a 100644 --- a/services/people/java/com/android/server/people/data/DataManager.java +++ b/services/people/java/com/android/server/people/data/DataManager.java @@ -1162,9 +1162,9 @@ public class DataManager { private final int mUserId; - // Conversation package name + shortcut ID -> Number of active notifications + // Conversation package name + shortcut ID -> Keys of active notifications @GuardedBy("this") - private final Map, Integer> mActiveNotifCounts = new ArrayMap<>(); + private final Map, Set> mActiveNotifKeys = new ArrayMap<>(); private NotificationListener(int userId) { mUserId = userId; @@ -1178,8 +1178,10 @@ public class DataManager { String shortcutId = sbn.getNotification().getShortcutId(); PackageData packageData = getPackageIfConversationExists(sbn, conversationInfo -> { synchronized (this) { - mActiveNotifCounts.merge( - Pair.create(sbn.getPackageName(), shortcutId), 1, Integer::sum); + Set notificationKeys = mActiveNotifKeys.computeIfAbsent( + Pair.create(sbn.getPackageName(), shortcutId), + (unusedKey) -> new HashSet<>()); + notificationKeys.add(sbn.getKey()); } }); @@ -1218,12 +1220,12 @@ public class DataManager { Pair conversationKey = Pair.create(sbn.getPackageName(), shortcutId); synchronized (this) { - int count = mActiveNotifCounts.getOrDefault(conversationKey, 0) - 1; - if (count <= 0) { - mActiveNotifCounts.remove(conversationKey); + Set notificationKeys = mActiveNotifKeys.computeIfAbsent( + conversationKey, (unusedKey) -> new HashSet<>()); + notificationKeys.remove(sbn.getKey()); + if (notificationKeys.isEmpty()) { + mActiveNotifKeys.remove(conversationKey); cleanupCachedShortcuts(mUserId, MAX_CACHED_RECENT_SHORTCUTS); - } else { - mActiveNotifCounts.put(conversationKey, count); } } }); @@ -1289,7 +1291,7 @@ public class DataManager { } synchronized boolean hasActiveNotifications(String packageName, String shortcutId) { - return mActiveNotifCounts.containsKey(Pair.create(packageName, shortcutId)); + return mActiveNotifKeys.containsKey(Pair.create(packageName, shortcutId)); } } diff --git a/services/tests/servicestests/src/com/android/server/people/data/DataManagerTest.java b/services/tests/servicestests/src/com/android/server/people/data/DataManagerTest.java index 4d177acb99fb5..a27602d651189 100644 --- a/services/tests/servicestests/src/com/android/server/people/data/DataManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/people/data/DataManagerTest.java @@ -500,6 +500,7 @@ public final class DataManagerTest { // The cached conversations are above the limit because every conversation has active // notifications. To uncache one of them, the notifications for that conversation need to // be dismissed. + String notificationKey = ""; for (int i = 0; i < DataManager.MAX_CACHED_RECENT_SHORTCUTS + 1; i++) { String shortcutId = TEST_SHORTCUT_ID + i; ShortcutInfo shortcut = buildShortcutInfo(TEST_PKG_NAME, USER_ID_PRIMARY, shortcutId, @@ -507,11 +508,13 @@ public final class DataManagerTest { shortcut.setCached(ShortcutInfo.FLAG_CACHED_NOTIFICATIONS); mDataManager.addOrUpdateConversationInfo(shortcut); when(mNotification.getShortcutId()).thenReturn(shortcutId); - sendGenericNotification(); + notificationKey = String.format("notification-key-%d", i); + sendGenericNotificationWithKey(notificationKey); } // Post another notification for the last conversation. - sendGenericNotification(); + String otherNotificationKey = "other-notification-key"; + sendGenericNotificationWithKey(otherNotificationKey); // Removing one of the two notifications does not un-cache the shortcut. listenerService.onNotificationRemoved(mGenericSbn, null, @@ -520,6 +523,7 @@ public final class DataManagerTest { anyInt(), any(), anyString(), any(), anyInt(), anyInt()); // Removing the second notification un-caches the shortcut. + when(mGenericSbn.getKey()).thenReturn(notificationKey); listenerService.onNotificationRemoved(mGenericSbn, null, NotificationListenerService.REASON_CANCEL_ALL); verify(mShortcutServiceInternal).uncacheShortcuts( @@ -686,6 +690,35 @@ public final class DataManagerTest { assertThat(result.getStatuses()).containsExactly(cs); } + @Test + public void testGetConversation_trackActiveConversations() { + mDataManager.onUserUnlocked(USER_ID_PRIMARY); + assertThat(mDataManager.getConversation(TEST_PKG_NAME, USER_ID_PRIMARY, + TEST_SHORTCUT_ID)).isNull(); + ShortcutInfo shortcut = buildShortcutInfo(TEST_PKG_NAME, USER_ID_PRIMARY, TEST_SHORTCUT_ID, + buildPerson()); + shortcut.setCached(ShortcutInfo.FLAG_PINNED); + mDataManager.addOrUpdateConversationInfo(shortcut); + assertThat(mDataManager.getConversation(TEST_PKG_NAME, USER_ID_PRIMARY, + TEST_SHORTCUT_ID)).isNotNull(); + + sendGenericNotification(); + sendGenericNotification(); + ConversationChannel result = mDataManager.getConversation(TEST_PKG_NAME, USER_ID_PRIMARY, + TEST_SHORTCUT_ID); + assertTrue(result.hasActiveNotifications()); + + // Both generic notifications have the same notification key, so a single dismiss will + // remove both of them. + NotificationListenerService listenerService = + mDataManager.getNotificationListenerServiceForTesting(USER_ID_PRIMARY); + listenerService.onNotificationRemoved(mGenericSbn, null, + NotificationListenerService.REASON_CANCEL); + ConversationChannel resultTwo = mDataManager.getConversation(TEST_PKG_NAME, USER_ID_PRIMARY, + TEST_SHORTCUT_ID); + assertFalse(resultTwo.hasActiveNotifications()); + } + @Test public void testGetConversation_unsyncedShortcut() { mDataManager.onUserUnlocked(USER_ID_PRIMARY); @@ -1322,7 +1355,7 @@ public final class DataManagerTest { sendGenericNotification(); - mDataManager.getRecentConversations(USER_ID_PRIMARY); + mDataManager.getRecentConversations(USER_ID_PRIMARY); verify(mShortcutServiceInternal).getShortcuts( anyInt(), anyString(), anyLong(), anyString(), anyList(), any(), any(), @@ -1693,6 +1726,12 @@ public final class DataManagerTest { // "Sends" a notification to a non-customized notification channel - the notification channel // is something generic like "messages" and the notification has a shortcut id private void sendGenericNotification() { + sendGenericNotificationWithKey(GENERIC_KEY); + } + + // "Sends" a notification to a non-customized notification channel with the specified key. + private void sendGenericNotificationWithKey(String key) { + when(mGenericSbn.getKey()).thenReturn(key); when(mNotification.getChannelId()).thenReturn(PARENT_NOTIFICATION_CHANNEL_ID); doAnswer(invocationOnMock -> { NotificationListenerService.Ranking ranking = (NotificationListenerService.Ranking) @@ -1708,7 +1747,7 @@ public final class DataManagerTest { mParentNotificationChannel, null, null, true, 0, false, -1, false, null, null, false, false, false, null, 0, false, 0); return true; - }).when(mRankingMap).getRanking(eq(GENERIC_KEY), + }).when(mRankingMap).getRanking(eq(key), any(NotificationListenerService.Ranking.class)); NotificationListenerService listenerService = mDataManager.getNotificationListenerServiceForTesting(USER_ID_PRIMARY);