[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
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 bd0a814..eff9e8d 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 @@
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<Pair<String, String>, Integer> mActiveNotifCounts = new ArrayMap<>();
+ private final Map<Pair<String, String>, Set<String>> mActiveNotifKeys = new ArrayMap<>();
private NotificationListener(int userId) {
mUserId = userId;
@@ -1178,8 +1178,10 @@
String shortcutId = sbn.getNotification().getShortcutId();
PackageData packageData = getPackageIfConversationExists(sbn, conversationInfo -> {
synchronized (this) {
- mActiveNotifCounts.merge(
- Pair.create(sbn.getPackageName(), shortcutId), 1, Integer::sum);
+ Set<String> notificationKeys = mActiveNotifKeys.computeIfAbsent(
+ Pair.create(sbn.getPackageName(), shortcutId),
+ (unusedKey) -> new HashSet<>());
+ notificationKeys.add(sbn.getKey());
}
});
@@ -1218,12 +1220,12 @@
Pair<String, String> conversationKey =
Pair.create(sbn.getPackageName(), shortcutId);
synchronized (this) {
- int count = mActiveNotifCounts.getOrDefault(conversationKey, 0) - 1;
- if (count <= 0) {
- mActiveNotifCounts.remove(conversationKey);
+ Set<String> 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 @@
}
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 4d177ac..a27602d 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 @@
// 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 @@
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 @@
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(
@@ -687,6 +691,35 @@
}
@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);
ShortcutInfo shortcut = buildShortcutInfo(TEST_PKG_NAME, USER_ID_PRIMARY, TEST_SHORTCUT_ID,
@@ -1322,7 +1355,7 @@
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 @@
// "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 @@
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);