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 5d1c3e12ca39a..326a5f2582e16 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(); }