From a13b3e257418b722282b1c4b5aaef316503bc127 Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Thu, 10 Aug 2017 16:58:54 -0400 Subject: [PATCH] Group new free notis if autogroup summary exists In the case where an app posted enough notifications to get an autogrpup summary, but then canceled most of them, add all new group-less notifications to the autogroup summary even if the total number of notifications currently posted by the app is < the autogroup trigger number Test: runtest systemui-notification, cts Change-Id: I6f7a8a0140abcff2b0f78eacfbf23cfab12897fa Fixes: 64158706 --- .../server/notification/GroupHelper.java | 8 +- .../NotificationManagerService.java | 14 ++-- .../server/notification/GroupHelperTest.java | 79 +++++++++++++++---- .../NotificationManagerServiceTest.java | 6 +- 4 files changed, 80 insertions(+), 27 deletions(-) diff --git a/services/core/java/com/android/server/notification/GroupHelper.java b/services/core/java/com/android/server/notification/GroupHelper.java index 57c558cddb548..ce805aadec777 100644 --- a/services/core/java/com/android/server/notification/GroupHelper.java +++ b/services/core/java/com/android/server/notification/GroupHelper.java @@ -38,14 +38,14 @@ public class GroupHelper { private final Callback mCallback; // Map of user : . Only contains notifications that are not - // groupd by the app (aka no group or sort key). + // grouped by the app (aka no group or sort key). Map>> mUngroupedNotifications = new HashMap<>(); public GroupHelper(Callback callback) {; mCallback = callback; } - public void onNotificationPosted(StatusBarNotification sbn) { + public void onNotificationPosted(StatusBarNotification sbn, boolean autogroupSummaryExists) { if (DEBUG) Log.i(TAG, "POSTED " + sbn.getKey()); try { List notificationsToGroup = new ArrayList<>(); @@ -68,7 +68,8 @@ public class GroupHelper { notificationsForPackage.add(sbn.getKey()); ungroupedNotificationsByUser.put(sbn.getPackageName(), notificationsForPackage); - if (notificationsForPackage.size() >= AUTOGROUP_AT_COUNT) { + if (notificationsForPackage.size() >= AUTOGROUP_AT_COUNT + || autogroupSummaryExists) { notificationsToGroup.addAll(notificationsForPackage); } } @@ -120,6 +121,7 @@ public class GroupHelper { // If the status change of this notification has brought the number of loose // notifications to zero, remove the summary and un-autogroup. if (notificationsForPackage.size() == 0) { + ungroupedNotificationsByUser.remove(sbn.getPackageName()); removeSummary = true; } } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 704978b9616f8..cc9f183fb69ee 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -187,7 +187,6 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.File; import java.io.FileDescriptor; -import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; @@ -2051,9 +2050,7 @@ public class NotificationManagerService extends SystemService { private StatusBarNotification sanitizeSbn(String pkg, int userId, StatusBarNotification sbn) { - if (sbn.getPackageName().equals(pkg) && sbn.getUserId() == userId - && (sbn.getNotification().flags - & Notification.FLAG_AUTOGROUP_SUMMARY) == 0) { + if (sbn.getPackageName().equals(pkg) && sbn.getUserId() == userId) { // We could pass back a cloneLight() but clients might get confused and // try to send this thing back to notify() again, which would not work // very well. @@ -3061,6 +3058,12 @@ public class NotificationManagerService extends SystemService { } } + @GuardedBy("mNotificationLock") + private boolean hasAutoGroupSummaryLocked(StatusBarNotification sbn) { + ArrayMap summaries = mAutobundledSummaries.get(sbn.getUserId()); + return summaries != null && summaries.containsKey(sbn.getPackageName()); + } + // Posts a 'fake' summary for a package that has exceeded the solo-notification limit. private void createAutoGroupSummary(int userId, String pkg, String triggeringKey) { NotificationRecord summaryRecord = null; @@ -3836,7 +3839,8 @@ public class NotificationManagerService extends SystemService { mHandler.post(new Runnable() { @Override public void run() { - mGroupHelper.onNotificationPosted(n); + mGroupHelper.onNotificationPosted( + n, hasAutoGroupSummaryLocked(n)); } }); } diff --git a/services/tests/notification/src/com/android/server/notification/GroupHelperTest.java b/services/tests/notification/src/com/android/server/notification/GroupHelperTest.java index 8dd177921022d..f75c648f3c3e5 100644 --- a/services/tests/notification/src/com/android/server/notification/GroupHelperTest.java +++ b/services/tests/notification/src/com/android/server/notification/GroupHelperTest.java @@ -15,6 +15,9 @@ */ package com.android.server.notification; +import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertNotNull; + import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; @@ -29,18 +32,16 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; -import android.app.AlarmManager; import android.app.Notification; -import android.app.NotificationChannel; -import android.app.NotificationManager; -import android.app.PendingIntent; import android.os.UserHandle; import android.service.notification.StatusBarNotification; import android.support.test.runner.AndroidJUnit4; import android.test.suitebuilder.annotation.SmallTest; import java.util.ArrayList; +import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; @SmallTest @RunWith(AndroidJUnit4.class) @@ -77,7 +78,8 @@ public class GroupHelperTest extends NotificationTestCase { public void testNoGroup_postingUnderLimit() throws Exception { final String pkg = "package"; for (int i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT - 1; i++) { - mGroupHelper.onNotificationPosted(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM)); + mGroupHelper.onNotificationPosted(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM), + false); } verify(mCallback, never()).addAutoGroupSummary( eq(UserHandle.USER_SYSTEM), eq(pkg), anyString()); @@ -91,10 +93,11 @@ public class GroupHelperTest extends NotificationTestCase { final String pkg = "package"; final String pkg2 = "package2"; for (int i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT - 1; i++) { - mGroupHelper.onNotificationPosted(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM)); + mGroupHelper.onNotificationPosted(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM), + false); } mGroupHelper.onNotificationPosted( - getSbn(pkg2, GroupHelper.AUTOGROUP_AT_COUNT, "four", UserHandle.SYSTEM)); + getSbn(pkg2, GroupHelper.AUTOGROUP_AT_COUNT, "four", UserHandle.SYSTEM), false); verify(mCallback, never()).addAutoGroupSummary( eq(UserHandle.USER_SYSTEM), eq(pkg), anyString()); verify(mCallback, never()).addAutoGroup(anyString()); @@ -106,10 +109,12 @@ public class GroupHelperTest extends NotificationTestCase { public void testNoGroup_multiUser() throws Exception { final String pkg = "package"; for (int i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT - 1; i++) { - mGroupHelper.onNotificationPosted(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM)); + mGroupHelper.onNotificationPosted(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM), + false); } mGroupHelper.onNotificationPosted( - getSbn(pkg, GroupHelper.AUTOGROUP_AT_COUNT, "four", UserHandle.ALL)); + getSbn(pkg, GroupHelper.AUTOGROUP_AT_COUNT, "four", UserHandle.ALL), + false); verify(mCallback, never()).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); verify(mCallback, never()).addAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroup(anyString()); @@ -120,10 +125,12 @@ public class GroupHelperTest extends NotificationTestCase { public void testNoGroup_someAreGrouped() throws Exception { final String pkg = "package"; for (int i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT - 1; i++) { - mGroupHelper.onNotificationPosted(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM)); + mGroupHelper.onNotificationPosted( + getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM), false); } mGroupHelper.onNotificationPosted( - getSbn(pkg, GroupHelper.AUTOGROUP_AT_COUNT, "four", UserHandle.SYSTEM, "a")); + getSbn(pkg, GroupHelper.AUTOGROUP_AT_COUNT, "four", UserHandle.SYSTEM, "a"), + false); verify(mCallback, never()).addAutoGroupSummary( eq(UserHandle.USER_SYSTEM), eq(pkg), anyString()); verify(mCallback, never()).addAutoGroup(anyString()); @@ -136,7 +143,8 @@ public class GroupHelperTest extends NotificationTestCase { public void testPostingOverLimit() throws Exception { final String pkg = "package"; for (int i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT; i++) { - mGroupHelper.onNotificationPosted(getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM)); + mGroupHelper.onNotificationPosted( + getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM), false); } verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); verify(mCallback, times(GroupHelper.AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); @@ -151,7 +159,7 @@ public class GroupHelperTest extends NotificationTestCase { for (int i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT; i++) { final StatusBarNotification sbn = getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM); posted.add(sbn); - mGroupHelper.onNotificationPosted(sbn); + mGroupHelper.onNotificationPosted(sbn, false); } verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); verify(mCallback, times(GroupHelper.AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); @@ -178,7 +186,7 @@ public class GroupHelperTest extends NotificationTestCase { for (int i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT; i++) { final StatusBarNotification sbn = getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM); posted.add(sbn); - mGroupHelper.onNotificationPosted(sbn); + mGroupHelper.onNotificationPosted(sbn, false); } verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); verify(mCallback, times(GroupHelper.AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); @@ -190,7 +198,7 @@ public class GroupHelperTest extends NotificationTestCase { for (i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT - 2; i++) { final StatusBarNotification sbn = getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM, "app group"); - mGroupHelper.onNotificationPosted(sbn); + mGroupHelper.onNotificationPosted(sbn, false); } verify(mCallback, times(GroupHelper.AUTOGROUP_AT_COUNT - 2)).removeAutoGroup(anyString()); verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); @@ -199,9 +207,48 @@ public class GroupHelperTest extends NotificationTestCase { for (; i < GroupHelper.AUTOGROUP_AT_COUNT; i++) { final StatusBarNotification sbn = getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM, "app group"); - mGroupHelper.onNotificationPosted(sbn); + mGroupHelper.onNotificationPosted(sbn, false); } verify(mCallback, times(2)).removeAutoGroup(anyString()); verify(mCallback, times(1)).removeAutoGroupSummary(anyInt(), anyString()); } + + @Test + public void testNewNotificationsAddedToAutogroup_ifOriginalNotificationsCanceled() + throws Exception { + final String pkg = "package"; + List posted = new ArrayList<>(); + for (int i = 0; i < GroupHelper.AUTOGROUP_AT_COUNT; i++) { + final StatusBarNotification sbn = getSbn(pkg, i, String.valueOf(i), UserHandle.SYSTEM); + posted.add(sbn); + mGroupHelper.onNotificationPosted(sbn, false); + } + verify(mCallback, times(1)).addAutoGroupSummary(anyInt(), eq(pkg), anyString()); + verify(mCallback, times(GroupHelper.AUTOGROUP_AT_COUNT)).addAutoGroup(anyString()); + verify(mCallback, never()).removeAutoGroup(anyString()); + verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); + Mockito.reset(mCallback); + + for (int i = posted.size() - 2; i >= 0; i--) { + mGroupHelper.onNotificationRemoved(posted.remove(i)); + } + verify(mCallback, never()).removeAutoGroup(anyString()); + verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); + Mockito.reset(mCallback); + + // only one child remains + Map> ungroupedForUser = + mGroupHelper.mUngroupedNotifications.get(UserHandle.USER_SYSTEM); + assertNotNull(ungroupedForUser); + assertEquals(1, ungroupedForUser.get(pkg).size()); + + // Add new notification; it should be autogrouped even though the total count is + // < AUTOGROUP_AT_COUNT + 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, never()).removeAutoGroup(anyString()); + verify(mCallback, never()).removeAutoGroupSummary(anyInt(), anyString()); + } } diff --git a/services/tests/notification/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/notification/src/com/android/server/notification/NotificationManagerServiceTest.java index 9acf5194286fc..04b42f1ee3121 100644 --- a/services/tests/notification/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/notification/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -1339,7 +1339,7 @@ public class NotificationManagerServiceTest extends NotificationTestCase { runnable.run(); waitForIdle(); - verify(mGroupHelper, times(1)).onNotificationPosted(any()); + verify(mGroupHelper, times(1)).onNotificationPosted(any(), anyBoolean()); } @Test @@ -1355,7 +1355,7 @@ public class NotificationManagerServiceTest extends NotificationTestCase { runnable.run(); waitForIdle(); - verify(mGroupHelper, times(1)).onNotificationPosted(any()); + verify(mGroupHelper, times(1)).onNotificationPosted(any(), anyBoolean()); } @Test @@ -1371,7 +1371,7 @@ public class NotificationManagerServiceTest extends NotificationTestCase { runnable.run(); waitForIdle(); - verify(mGroupHelper, never()).onNotificationPosted(any()); + verify(mGroupHelper, never()).onNotificationPosted(any(), anyBoolean()); } @Test