From 65d469643c14f3c28d6e04757ee119dd3759d9be Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Fri, 18 Mar 2022 15:48:14 -0400 Subject: [PATCH] Fix ongoing autogenerated summaries Fixes an issue where the autogroup summary wasn't being marked with the ongoing flag if the ongoing notifications were posted before the summary was created and then never updated Test: GroupHelperTest, manually look at "android" group when two system USB notifications are shown Bug: 225393724 Change-Id: Id7953bafa4f709bb053f24b72be3925465931ec5 --- .../server/notification/GroupHelper.java | 52 ++++++------ .../NotificationManagerService.java | 20 +++-- .../server/notification/GroupHelperTest.java | 82 +++++++++++-------- .../NotificationPermissionMigrationTest.java | 2 +- 4 files changed, 82 insertions(+), 74 deletions(-) diff --git a/services/core/java/com/android/server/notification/GroupHelper.java b/services/core/java/com/android/server/notification/GroupHelper.java index 4f2680904faee..273afcc9f7693 100644 --- a/services/core/java/com/android/server/notification/GroupHelper.java +++ b/services/core/java/com/android/server/notification/GroupHelper.java @@ -42,7 +42,7 @@ public class GroupHelper { private final int mAutoGroupAtCount; // count the number of ongoing notifications per group - // userId -> (package name -> (group Id -> (set of notification keys))) + // userId|packageName -> (set of ongoing notifications that aren't in an app group) final ArrayMap> mOngoingGroupCount = new ArrayMap<>(); @@ -55,52 +55,43 @@ public class GroupHelper { mCallback = callback; } - private String generatePackageGroupKey(int userId, String pkg, String group) { - return userId + "|" + pkg + "|" + group; + private String generatePackageKey(int userId, String pkg) { + return userId + "|" + pkg; } @VisibleForTesting - protected int getOngoingGroupCount(int userId, String pkg, String group) { - String key = generatePackageGroupKey(userId, pkg, group); + protected int getOngoingGroupCount(int userId, String pkg) { + String key = generatePackageKey(userId, pkg); return mOngoingGroupCount.getOrDefault(key, new ArraySet<>(0)).size(); } - private void addToOngoingGroupCount(StatusBarNotification sbn, boolean add) { - if (sbn.getNotification().isGroupSummary()) return; - if (!sbn.isOngoing() && add) return; - String group = sbn.getGroup(); - if (group == null) return; - int userId = sbn.getUser().getIdentifier(); - String key = generatePackageGroupKey(userId, sbn.getPackageName(), group); + private void updateOngoingGroupCount(StatusBarNotification sbn, boolean add) { + if (sbn.getNotification().isGroupSummary()) { + return; + } + String key = generatePackageKey(sbn.getUserId(), sbn.getPackageName()); ArraySet notifications = mOngoingGroupCount.getOrDefault(key, new ArraySet<>(0)); if (add) { notifications.add(sbn.getKey()); mOngoingGroupCount.put(key, notifications); } else { notifications.remove(sbn.getKey()); - // we dont need to put it back if it is default + // we don't need to put it back if it is default } - String combinedKey = generatePackageGroupKey(userId, sbn.getPackageName(), group); + boolean needsOngoingFlag = notifications.size() > 0; - mCallback.updateAutogroupSummary(userId, sbn.getPackageName(), needsOngoingFlag); + mCallback.updateAutogroupSummary(sbn.getUserId(), sbn.getPackageName(), needsOngoingFlag); } - public void onNotificationUpdated(StatusBarNotification childSbn, - boolean autogroupSummaryExists) { - if (childSbn.getGroup() != AUTOGROUP_KEY - || childSbn.getNotification().isGroupSummary()) return; - if (childSbn.isOngoing()) { - addToOngoingGroupCount(childSbn, true); - } else { - addToOngoingGroupCount(childSbn, false); - } + public void onNotificationUpdated(StatusBarNotification childSbn) { + updateOngoingGroupCount(childSbn, childSbn.isOngoing() && !childSbn.isAppGroup()); } public void onNotificationPosted(StatusBarNotification sbn, boolean autogroupSummaryExists) { - if (DEBUG) Log.i(TAG, "POSTED " + sbn.getKey()); try { + updateOngoingGroupCount(sbn, sbn.isOngoing() && !sbn.isAppGroup()); + List notificationsToGroup = new ArrayList<>(); - if (autogroupSummaryExists) addToOngoingGroupCount(sbn, true); if (!sbn.isAppGroup()) { // Not grouped by the app, add to the list of notifications for the app; // send grouping update if app exceeds the autogrouping limit. @@ -134,6 +125,7 @@ public class GroupHelper { // Grouped, but not by us. Send updates to un-autogroup, if we grouped it. maybeUngroup(sbn, false, sbn.getUserId()); } + } catch (Exception e) { Slog.e(TAG, "Failure processing new notification", e); } @@ -141,7 +133,7 @@ public class GroupHelper { public void onNotificationRemoved(StatusBarNotification sbn) { try { - addToOngoingGroupCount(sbn, false); + updateOngoingGroupCount(sbn, false); maybeUngroup(sbn, true, sbn.getUserId()); } catch (Exception e) { Slog.e(TAG, "Error processing canceled notification", e); @@ -189,7 +181,8 @@ public class GroupHelper { private void adjustAutogroupingSummary(int userId, String packageName, String triggeringKey, boolean summaryNeeded) { if (summaryNeeded) { - mCallback.addAutoGroupSummary(userId, packageName, triggeringKey); + mCallback.addAutoGroupSummary(userId, packageName, triggeringKey, + getOngoingGroupCount(userId, packageName) > 0); } else { mCallback.removeAutoGroupSummary(userId, packageName); } @@ -209,7 +202,8 @@ public class GroupHelper { protected interface Callback { void addAutoGroup(String key); void removeAutoGroup(String key); - void addAutoGroupSummary(int userId, String pkg, String triggeringKey); + void addAutoGroupSummary(int userId, String pkg, String triggeringKey, + boolean needsOngoingFlag); void removeAutoGroupSummary(int user, String pkg); void updateAutogroupSummary(int userId, String pkg, boolean needsOngoingFlag); } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 1885b5596e3e7..e0df7368db053 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -2557,8 +2557,10 @@ public class NotificationManagerService extends SystemService { } @Override - public void addAutoGroupSummary(int userId, String pkg, String triggeringKey) { - NotificationRecord r = createAutoGroupSummary(userId, pkg, triggeringKey); + public void addAutoGroupSummary(int userId, String pkg, String triggeringKey, + boolean needsOngoingFlag) { + NotificationRecord r = createAutoGroupSummary( + userId, pkg, triggeringKey, needsOngoingFlag); if (r != null) { final boolean isAppForeground = mActivityManager.getPackageImportance(pkg) == IMPORTANCE_FOREGROUND; @@ -5739,6 +5741,7 @@ public class NotificationManagerService extends SystemService { void removeAutogroupKeyLocked(String key) { NotificationRecord r = mNotificationsByKey.get(key); if (r == null) { + Slog.w(TAG, "Failed to remove autogroup " + key); return; } if (r.getSbn().getOverrideGroupKey() != null) { @@ -5778,7 +5781,8 @@ public class NotificationManagerService extends SystemService { } // Creates a 'fake' summary for a package that has exceeded the solo-notification limit. - NotificationRecord createAutoGroupSummary(int userId, String pkg, String triggeringKey) { + NotificationRecord createAutoGroupSummary(int userId, String pkg, String triggeringKey, + boolean needsOngoingFlag) { NotificationRecord summaryRecord = null; boolean isPermissionFixed = mPermissionHelper.isMigrationEnabled() ? mPermissionHelper.isPermissionFixed(pkg, userId) : false; @@ -5818,6 +5822,7 @@ public class NotificationManagerService extends SystemService { .setGroup(GroupHelper.AUTOGROUP_KEY) .setFlag(FLAG_AUTOGROUP_SUMMARY, true) .setFlag(Notification.FLAG_GROUP_SUMMARY, true) + .setFlag(FLAG_ONGOING_EVENT, needsOngoingFlag) .setColor(adjustedSbn.getNotification().color) .setLocalOnly(true) .build(); @@ -7356,17 +7361,16 @@ public class NotificationManagerService extends SystemService { mListeners.notifyPostedLocked(r, old); if ((oldSbn == null || !Objects.equals(oldSbn.getGroup(), n.getGroup())) && !isCritical(r)) { - mHandler.post(new Runnable() { - @Override - public void run() { + mHandler.post(() -> { + synchronized (mNotificationLock) { mGroupHelper.onNotificationPosted( n, hasAutoGroupSummaryLocked(n)); } }); } else if (oldSbn != null) { final NotificationRecord finalRecord = r; - mHandler.post(() -> mGroupHelper.onNotificationUpdated( - finalRecord.getSbn(), hasAutoGroupSummaryLocked(n))); + mHandler.post(() -> + mGroupHelper.onNotificationUpdated(finalRecord.getSbn())); } } else { Slog.e(TAG, "Not posting notification without small icon: " + notification); diff --git a/services/tests/uiservicestests/src/com/android/server/notification/GroupHelperTest.java b/services/tests/uiservicestests/src/com/android/server/notification/GroupHelperTest.java index 5458a5b84eea3..ff6c9769b69fb 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/GroupHelperTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/GroupHelperTest.java @@ -20,6 +20,7 @@ import static com.android.server.notification.GroupHelper.AUTOGROUP_KEY; import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertNotNull; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; @@ -53,7 +54,7 @@ import java.util.Map; public class GroupHelperTest extends UiServiceTestCase { private @Mock GroupHelper.Callback mCallback; - private final static int AUTOGROUP_AT_COUNT = 4; + private final static int AUTOGROUP_AT_COUNT = 7; private GroupHelper mGroupHelper; @Before @@ -88,7 +89,7 @@ public class GroupHelperTest extends UiServiceTestCase { false); } verify(mCallback, never()).addAutoGroupSummary( - eq(UserHandle.USER_SYSTEM), eq(pkg), anyString()); + eq(UserHandle.USER_SYSTEM), eq(pkg), anyString(), anyBoolean()); verify(mCallback, never()).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); @@ -105,7 +106,7 @@ public class GroupHelperTest extends UiServiceTestCase { mGroupHelper.onNotificationPosted( getSbn(pkg2, AUTOGROUP_AT_COUNT, "four", UserHandle.SYSTEM), false); verify(mCallback, never()).addAutoGroupSummary( - eq(UserHandle.USER_SYSTEM), eq(pkg), anyString()); + eq(UserHandle.USER_SYSTEM), eq(pkg), anyString(), anyBoolean()); verify(mCallback, never()).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); @@ -120,7 +121,8 @@ public class GroupHelperTest extends UiServiceTestCase { } mGroupHelper.onNotificationPosted( getSbn(pkg, AUTOGROUP_AT_COUNT, "four", UserHandle.ALL), false); - verify(mCallback, never()).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); + verify(mCallback, never()).addAutoGroupSummary( + anyInt(), eq(pkg), anyString(), anyBoolean()); verify(mCallback, never()).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); @@ -136,13 +138,12 @@ public class GroupHelperTest extends UiServiceTestCase { mGroupHelper.onNotificationPosted( getSbn(pkg, AUTOGROUP_AT_COUNT, "four", UserHandle.SYSTEM, "a"), false); verify(mCallback, never()).addAutoGroupSummary( - eq(UserHandle.USER_SYSTEM), eq(pkg), anyString()); + eq(UserHandle.USER_SYSTEM), eq(pkg), anyString(), anyBoolean()); verify(mCallback, never()).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); } - @Test public void testPostingOverLimit() throws Exception { final String pkg = "package"; @@ -150,7 +151,23 @@ public class GroupHelperTest extends UiServiceTestCase { mGroupHelper.onNotificationPosted( getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM), false); } - verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); + verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString(), eq(false)); + verify(mCallback, times(AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); + verify(mCallback, never()).removeAutoGroup(anyString()); + verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); + } + + @Test + public void testPostingOverLimit_addsOngoingFlag() throws Exception { + final String pkg = "package"; + for (int i = 0; i < AUTOGROUP_AT_COUNT; i++) { + StatusBarNotification sbn = getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM); + if (i == 0) { + sbn.getNotification().flags |= Notification.FLAG_ONGOING_EVENT; + } + mGroupHelper.onNotificationPosted(sbn, false); + } + verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString(), eq(true)); verify(mCallback, times(AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); @@ -178,7 +195,7 @@ public class GroupHelperTest extends UiServiceTestCase { int userId = UserHandle.SYSTEM.getIdentifier(); assertEquals(mGroupHelper.getOngoingGroupCount( - userId, pkg, AUTOGROUP_KEY), AUTOGROUP_AT_COUNT + 1); + userId, pkg), AUTOGROUP_AT_COUNT + 1); } @Test @@ -199,15 +216,14 @@ public class GroupHelperTest extends UiServiceTestCase { } notifications.get(0).getNotification().flags &= ~Notification.FLAG_ONGOING_EVENT; - - mGroupHelper.onNotificationUpdated(notifications.get(0), true); + mGroupHelper.onNotificationUpdated(notifications.get(0)); verify(mCallback, times(AUTOGROUP_AT_COUNT + 2)) .updateAutogroupSummary(anyInt(), anyString(), eq(true)); int userId = UserHandle.SYSTEM.getIdentifier(); assertEquals(mGroupHelper.getOngoingGroupCount( - userId, pkg, AUTOGROUP_KEY), AUTOGROUP_AT_COUNT); + userId, pkg), AUTOGROUP_AT_COUNT); } @Test @@ -229,18 +245,18 @@ public class GroupHelperTest extends UiServiceTestCase { notifications.get(0).getNotification().flags &= ~Notification.FLAG_ONGOING_EVENT; - mGroupHelper.onNotificationUpdated(notifications.get(0), true); + mGroupHelper.onNotificationUpdated(notifications.get(0)); notifications.get(0).getNotification().flags |= Notification.FLAG_ONGOING_EVENT; - mGroupHelper.onNotificationUpdated(notifications.get(0), true); + mGroupHelper.onNotificationUpdated(notifications.get(0)); verify(mCallback, times(AUTOGROUP_AT_COUNT + 3)) .updateAutogroupSummary(anyInt(), anyString(), eq(true)); int userId = UserHandle.SYSTEM.getIdentifier(); assertEquals(mGroupHelper.getOngoingGroupCount( - userId, pkg, AUTOGROUP_KEY), AUTOGROUP_AT_COUNT + 1); + userId, pkg), AUTOGROUP_AT_COUNT + 1); } @Test @@ -267,7 +283,7 @@ public class GroupHelperTest extends UiServiceTestCase { int userId = UserHandle.SYSTEM.getIdentifier(); assertEquals(mGroupHelper.getOngoingGroupCount( - userId, pkg, AUTOGROUP_KEY), AUTOGROUP_AT_COUNT); + userId, pkg), AUTOGROUP_AT_COUNT); } @@ -288,14 +304,14 @@ public class GroupHelperTest extends UiServiceTestCase { } notifications.get(0).getNotification().flags |= Notification.FLAG_ONGOING_EVENT; - mGroupHelper.onNotificationUpdated(notifications.get(0), true); + mGroupHelper.onNotificationUpdated(notifications.get(0)); verify(mCallback, times(1)) .updateAutogroupSummary(anyInt(), anyString(), eq(true)); int userId = UserHandle.SYSTEM.getIdentifier(); assertEquals(mGroupHelper.getOngoingGroupCount( - userId, pkg, AUTOGROUP_KEY), 1); + userId, pkg), 1); } @Test @@ -305,7 +321,7 @@ public class GroupHelperTest extends UiServiceTestCase { for (int i = 0; i < AUTOGROUP_AT_COUNT + 1; i++) { notifications.add(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM)); } - StatusBarNotification sbn = notifications.get(0); + StatusBarNotification sbn = notifications.get(AUTOGROUP_AT_COUNT); sbn.getNotification().flags |= Notification.FLAG_ONGOING_EVENT; sbn.setOverrideGroupKey(AUTOGROUP_KEY); @@ -319,7 +335,7 @@ public class GroupHelperTest extends UiServiceTestCase { int userId = UserHandle.SYSTEM.getIdentifier(); assertEquals(mGroupHelper.getOngoingGroupCount( - userId, pkg, AUTOGROUP_KEY), 1); + userId, pkg), 1); } @Test @@ -342,7 +358,7 @@ public class GroupHelperTest extends UiServiceTestCase { .updateAutogroupSummary(anyInt(), anyString(), eq(true)); int userId = UserHandle.SYSTEM.getIdentifier(); - assertEquals(mGroupHelper.getOngoingGroupCount(userId, pkg, AUTOGROUP_KEY), 0); + assertEquals(mGroupHelper.getOngoingGroupCount(userId, pkg), 0); } @@ -355,7 +371,7 @@ public class GroupHelperTest extends UiServiceTestCase { posted.add(sbn); mGroupHelper.onNotificationPosted(sbn, false); } - verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); + verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString(), eq(false)); verify(mCallback, times(AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); @@ -382,28 +398,22 @@ public class GroupHelperTest extends UiServiceTestCase { posted.add(sbn); mGroupHelper.onNotificationPosted(sbn, false); } - verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); + verify(mCallback, times(1)).addAutoGroupSummary( + anyInt(), eq(pkg), anyString(), anyBoolean()); verify(mCallback, times(AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); Mockito.reset(mCallback); - int i = 0; - for (i = 0; i < AUTOGROUP_AT_COUNT - 2; i++) { + for (int i = 0; i < AUTOGROUP_AT_COUNT; i++) { final StatusBarNotification sbn = getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM, "app group"); mGroupHelper.onNotificationPosted(sbn, false); + verify(mCallback, times(1)).removeAutoGroup(sbn.getKey()); + if (i < AUTOGROUP_AT_COUNT -1) { + verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); + } } - verify(mCallback, times(AUTOGROUP_AT_COUNT - 2)).removeAutoGroup(anyString()); - verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); - Mockito.reset(mCallback); - - for (; i < AUTOGROUP_AT_COUNT; i++) { - final StatusBarNotification sbn = - getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM, "app group"); - mGroupHelper.onNotificationPosted(sbn, false); - } - verify(mCallback, times(2)).removeAutoGroup(anyString()); verify(mCallback, times(1)).removeAutoGroupSummary(anyInt(), anyString()); } @@ -417,7 +427,7 @@ public class GroupHelperTest extends UiServiceTestCase { posted.add(sbn); mGroupHelper.onNotificationPosted(sbn, false); } - verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); + verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString(), eq(false)); verify(mCallback, times(AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); @@ -441,7 +451,7 @@ public class GroupHelperTest extends UiServiceTestCase { final StatusBarNotification sbn = getSbn(pkg, 5, String.valueOf(5), UserHandle.SYSTEM); posted.add(sbn); mGroupHelper.onNotificationPosted(sbn, true); - verify(mCallback, times(posted.size())).addAutoGroup(anyString()); + verify(mCallback, times(1)).addAutoGroup(sbn.getKey()); verify(mCallback, never()).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationPermissionMigrationTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationPermissionMigrationTest.java index 2ba587d21163b..0f6d5a56c6677 100755 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationPermissionMigrationTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationPermissionMigrationTest.java @@ -722,7 +722,7 @@ public class NotificationPermissionMigrationTest extends UiServiceTestCase { when(mPermissionHelper.isPermissionFixed(PKG, temp.getUserId())).thenReturn(true); NotificationRecord r = mService.createAutoGroupSummary( - temp.getUserId(), temp.getSbn().getPackageName(), temp.getKey()); + temp.getUserId(), temp.getSbn().getPackageName(), temp.getKey(), false); assertThat(r.isImportanceFixed()).isTrue(); }