From 27c0a96cb5f73e402ffd1a64f06688950ff66ffb Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Mon, 10 Dec 2018 12:37:28 -0500 Subject: [PATCH] Updates to Adjustment.KEY_IMPORTANCE handling - Do not run signal extractors on enqueued notifications - the assistant isn't told that information and the information that the extractors base their decisions on it's final. Instead, extract signals in the post runnable, before the sort/before we generate the notification update for listeners - Ignore importance adjustments sent from onNotificationEnqueued that arrive after the notification was posted, because it's too late to cancel the notification or silence it. - Also fix a bug in the handling of importance adjustments that are sent via adjustNotification: *listen* to the comment that says 'any field that can be changed in an extractor needs to be handled in handleRankingSort' Change-Id: I1603d6c5701fcc16f038231e22e21565dd366572 Fixes: 120601946 Test: atest, manual --- .../service/notification/Adjustment.java | 17 +++-- .../NotificationManagerService.java | 23 ++++-- .../notification/NotificationRecord.java | 45 +++++++----- .../NotificationManagerServiceTest.java | 71 +++++++++++++++---- .../notification/NotificationRecordTest.java | 3 + 5 files changed, 117 insertions(+), 42 deletions(-) diff --git a/core/java/android/service/notification/Adjustment.java b/core/java/android/service/notification/Adjustment.java index 518f8ed11a267..de532b74375f6 100644 --- a/core/java/android/service/notification/Adjustment.java +++ b/core/java/android/service/notification/Adjustment.java @@ -80,11 +80,18 @@ public final class Adjustment implements Parcelable { * Data type: int, one of importance values e.g. * {@link android.app.NotificationManager#IMPORTANCE_MIN}. * - * If used from - * {@link NotificationAssistantService#onNotificationEnqueued(StatusBarNotification)}, it can - * block a notification from appearing or silence it. If used from - * {@link NotificationAssistantService#adjustNotification(Adjustment)}, it can visually - * demote a notification. + *

If used from + * {@link NotificationAssistantService#onNotificationEnqueued(StatusBarNotification)}, and + * received before the notification is posted, it can block a notification from appearing or + * silence it. Importance adjustments received too late from + * {@link NotificationAssistantService#onNotificationEnqueued(StatusBarNotification)} will be + * ignored. + *

+ *

If used from + * {@link NotificationAssistantService#adjustNotification(Adjustment)}, it can + * visually demote or cancel a notification, but use this with care if they notification was + * recently posted because the notification may already have made noise. + *

*/ public static final String KEY_IMPORTANCE = "key_importance"; diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 2d2f9e3ca84f3..fc364c01aa4fa 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -3621,12 +3621,19 @@ public class NotificationManagerService extends SystemService { && mAssistants.isSameUser(token, r.getUserId())) { applyAdjustment(r, adjustment); r.applyAdjustments(); + // importance is checked at the beginning of the + // PostNotificationRunnable, before the signal extractors are run, so + // calculate the final importance here + r.calculateImportance(); foundEnqueued = true; break; } } if (!foundEnqueued) { // adjustment arrived too late to apply to enqueued; apply to posted + // However, since the notification is now posted and may have alerted, + // ignore any importance related adjustments + adjustment.getSignals().remove(Adjustment.KEY_IMPORTANCE); applyAdjustmentFromAssistant(token, adjustment); } } @@ -3656,8 +3663,12 @@ public class NotificationManagerService extends SystemService { NotificationRecord r = mNotificationsByKey.get(adjustment.getKey()); if (r != null && mAssistants.isSameUser(token, r.getUserId())) { applyAdjustment(r, adjustment); - r.applyImportanceFromAdjustments(); - if (r.getImportance() == IMPORTANCE_NONE) { + // If the assistant has blocked the notification, cancel it + // This will trigger a sort, so we don't have to explicitly ask for + // one here. + if (adjustment.getSignals().containsKey(Adjustment.KEY_IMPORTANCE) + && adjustment.getSignals().getInt(Adjustment.KEY_IMPORTANCE) + == IMPORTANCE_NONE) { cancelNotificationsFromListener(token, new String[]{r.getKey()}); } else { needsSort = true; @@ -4758,7 +4769,6 @@ public class NotificationManagerService extends SystemService { enqueueStatus); } - mRankingHelper.extractSignals(r); // tell the assistant service about the notification if (mAssistants.isEnabled()) { mAssistants.onNotificationEnqueuedLocked(r); @@ -4842,7 +4852,7 @@ public class NotificationManagerService extends SystemService { | Notification.FLAG_NO_CLEAR; } - applyZenModeLocked(r); + mRankingHelper.extractSignals(r); mRankingHelper.sort(mNotificationList); if (!r.isHidden()) { @@ -5627,6 +5637,7 @@ public class NotificationManagerService extends SystemService { ArrayList suppressVisuallyBefore = new ArrayList<>(N); ArrayList> systemSmartActionsBefore = new ArrayList<>(N); ArrayList> smartRepliesBefore = new ArrayList<>(N); + int[] importancesBefore = new int[N]; for (int i = 0; i < N; i++) { final NotificationRecord r = mNotificationList.get(i); orderBefore.add(r.getKey()); @@ -5640,6 +5651,7 @@ public class NotificationManagerService extends SystemService { suppressVisuallyBefore.add(r.getSuppressedVisualEffects()); systemSmartActionsBefore.add(r.getSystemGeneratedSmartActions()); smartRepliesBefore.add(r.getSmartReplies()); + importancesBefore[i] = r.getImportance(); mRankingHelper.extractSignals(r); } mRankingHelper.sort(mNotificationList); @@ -5657,7 +5669,8 @@ public class NotificationManagerService extends SystemService { r.getSuppressedVisualEffects()) || !Objects.equals(systemSmartActionsBefore.get(i), r.getSystemGeneratedSmartActions()) - || !Objects.equals(smartRepliesBefore.get(i), r.getSmartReplies())) { + || !Objects.equals(smartRepliesBefore.get(i), r.getSmartReplies()) + || importancesBefore[i] != r.getImportance()) { mHandler.scheduleSendRankingUpdate(); return; } diff --git a/services/core/java/com/android/server/notification/NotificationRecord.java b/services/core/java/com/android/server/notification/NotificationRecord.java index 1a9257cf17fc3..50810ccc8cbcb 100644 --- a/services/core/java/com/android/server/notification/NotificationRecord.java +++ b/services/core/java/com/android/server/notification/NotificationRecord.java @@ -21,10 +21,8 @@ import static android.app.NotificationManager.IMPORTANCE_HIGH; import static android.app.NotificationManager.IMPORTANCE_LOW; import static android.app.NotificationManager.IMPORTANCE_MIN; import static android.app.NotificationManager.IMPORTANCE_UNSPECIFIED; -import static android.service.notification.NotificationListenerService.Ranking - .USER_SENTIMENT_NEUTRAL; -import static android.service.notification.NotificationListenerService.Ranking - .USER_SENTIMENT_POSITIVE; +import static android.service.notification.NotificationListenerService.Ranking.USER_SENTIMENT_NEUTRAL; +import static android.service.notification.NotificationListenerService.Ranking.USER_SENTIMENT_POSITIVE; import android.annotation.Nullable; import android.app.ActivityManager; @@ -609,6 +607,17 @@ public final class NotificationRecord { mIsAppImportanceLocked, this.sbn.getNotification()); } + public boolean hasAdjustment(String key) { + synchronized (mAdjustments) { + for (Adjustment adjustment : mAdjustments) { + if (adjustment.getSignals().containsKey(key)) { + return true; + } + } + } + return false; + } + public void addAdjustment(Adjustment adjustment) { synchronized (mAdjustments) { mAdjustments.add(adjustment); @@ -669,18 +678,6 @@ public final class NotificationRecord { .addTaggedData(MetricsEvent.ADJUSTMENT_KEY_SMART_REPLIES, getSmartReplies().size())); } - } - applyImportanceFromAdjustments(); - } - } - - /** - * Update importance from the adjustment. - */ - public void applyImportanceFromAdjustments() { - synchronized (mAdjustments) { - for (Adjustment adjustment : mAdjustments) { - Bundle signals = adjustment.getSignals(); if (signals.containsKey(Adjustment.KEY_IMPORTANCE)) { int importance = signals.getInt(Adjustment.KEY_IMPORTANCE); importance = Math.max(IMPORTANCE_UNSPECIFIED, importance); @@ -752,6 +749,8 @@ public final class NotificationRecord { */ public void setSystemImportance(int importance) { mSystemImportance = importance; + // System importance is only changed in enqueue, so it's ok for us to calculate the + // importance directly instead of waiting for signal extractor. calculateImportance(); } @@ -762,7 +761,16 @@ public final class NotificationRecord { */ public void setAssistantImportance(int importance) { mAssistantImportance = importance; - calculateImportance(); + // Unlike the system importance, the assistant importance can change on posted + // notifications, so don't calculateImportance() here, but wait for the signal extractors. + } + + /** + * Returns the importance set by the assistant, or IMPORTANCE_UNSPECIFIED if the assistant + * hasn't set it. + */ + public int getAssistantImportance() { + return mAssistantImportance; } /** @@ -774,7 +782,8 @@ public final class NotificationRecord { if (getChannel().hasUserSetImportance()) { mImportanceExplanation = "user"; } - if (!getChannel().hasUserSetImportance() && mAssistantImportance != IMPORTANCE_UNSPECIFIED) { + if (!getChannel().hasUserSetImportance() + && mAssistantImportance != IMPORTANCE_UNSPECIFIED) { mImportance = mAssistantImportance; mImportanceExplanation = "asst"; } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java index 95561110b707a..527a1eef7b0fe 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -20,6 +20,7 @@ import static android.app.ActivityManager.RunningAppProcessInfo.IMPORTANCE_FOREG import static android.app.ActivityManager.RunningAppProcessInfo.IMPORTANCE_GONE; import static android.app.Notification.FLAG_FOREGROUND_SERVICE; import static android.app.NotificationManager.EXTRA_BLOCKED_STATE; +import static android.app.NotificationManager.IMPORTANCE_DEFAULT; import static android.app.NotificationManager.IMPORTANCE_HIGH; import static android.app.NotificationManager.IMPORTANCE_LOW; import static android.app.NotificationManager.IMPORTANCE_MAX; @@ -183,7 +184,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { Resources mResources; private NotificationChannel mTestNotificationChannel = new NotificationChannel( - TEST_CHANNEL_ID, TEST_CHANNEL_ID, NotificationManager.IMPORTANCE_DEFAULT); + TEST_CHANNEL_ID, TEST_CHANNEL_ID, IMPORTANCE_DEFAULT); @Mock private NotificationListeners mListeners; @Mock private NotificationAssistants mAssistants; @@ -430,7 +431,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testCreateNotificationChannels_SingleChannel() throws Exception { final NotificationChannel channel = - new NotificationChannel("id", "name", NotificationManager.IMPORTANCE_DEFAULT); + new NotificationChannel("id", "name", IMPORTANCE_DEFAULT); mBinderService.createNotificationChannels(PKG, new ParceledListSlice(Arrays.asList(channel))); final NotificationChannel createdChannel = @@ -452,9 +453,9 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testCreateNotificationChannels_TwoChannels() throws Exception { final NotificationChannel channel1 = - new NotificationChannel("id1", "name", NotificationManager.IMPORTANCE_DEFAULT); + new NotificationChannel("id1", "name", IMPORTANCE_DEFAULT); final NotificationChannel channel2 = - new NotificationChannel("id2", "name", NotificationManager.IMPORTANCE_DEFAULT); + new NotificationChannel("id2", "name", IMPORTANCE_DEFAULT); mBinderService.createNotificationChannels(PKG, new ParceledListSlice(Arrays.asList(channel1, channel2))); assertTrue(mBinderService.getNotificationChannel(PKG, "id1") != null); @@ -465,7 +466,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { public void testCreateNotificationChannels_SecondCreateDoesNotChangeImportance() throws Exception { final NotificationChannel channel = - new NotificationChannel("id", "name", NotificationManager.IMPORTANCE_DEFAULT); + new NotificationChannel("id", "name", IMPORTANCE_DEFAULT); mBinderService.createNotificationChannels(PKG, new ParceledListSlice(Arrays.asList(channel))); @@ -476,14 +477,14 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { new ParceledListSlice(Arrays.asList(dupeChannel))); final NotificationChannel createdChannel = mBinderService.getNotificationChannel(PKG, "id"); - assertEquals(NotificationManager.IMPORTANCE_DEFAULT, createdChannel.getImportance()); + assertEquals(IMPORTANCE_DEFAULT, createdChannel.getImportance()); } @Test public void testCreateNotificationChannels_SecondCreateAllowedToDowngradeImportance() throws Exception { final NotificationChannel channel = - new NotificationChannel("id", "name", NotificationManager.IMPORTANCE_DEFAULT); + new NotificationChannel("id", "name", IMPORTANCE_DEFAULT); mBinderService.createNotificationChannels(PKG, new ParceledListSlice(Arrays.asList(channel))); @@ -501,7 +502,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { public void testCreateNotificationChannels_CannotDowngradeImportanceIfAlreadyUpdated() throws Exception { final NotificationChannel channel = - new NotificationChannel("id", "name", NotificationManager.IMPORTANCE_DEFAULT); + new NotificationChannel("id", "name", IMPORTANCE_DEFAULT); mBinderService.createNotificationChannels(PKG, new ParceledListSlice(Arrays.asList(channel))); @@ -524,14 +525,14 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { public void testCreateNotificationChannels_IdenticalChannelsInListIgnoresSecond() throws Exception { final NotificationChannel channel1 = - new NotificationChannel("id", "name", NotificationManager.IMPORTANCE_DEFAULT); + new NotificationChannel("id", "name", IMPORTANCE_DEFAULT); final NotificationChannel channel2 = new NotificationChannel("id", "name", IMPORTANCE_HIGH); mBinderService.createNotificationChannels(PKG, new ParceledListSlice(Arrays.asList(channel1, channel2))); final NotificationChannel createdChannel = mBinderService.getNotificationChannel(PKG, "id"); - assertEquals(NotificationManager.IMPORTANCE_DEFAULT, createdChannel.getImportance()); + assertEquals(IMPORTANCE_DEFAULT, createdChannel.getImportance()); } @Test @@ -756,13 +757,18 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testBlockedNotifications_blockedByAssistant() throws Exception { when(mPackageManager.isPackageSuspendedForUser(anyString(), anyInt())).thenReturn(false); + when(mAssistants.isSameUser(any(), anyInt())).thenReturn(true); NotificationChannel channel = new NotificationChannel("id", "name", NotificationManager.IMPORTANCE_HIGH); NotificationRecord r = generateNotificationRecord(channel); mService.addEnqueuedNotification(r); - r.setAssistantImportance(IMPORTANCE_NONE); + Bundle bundle = new Bundle(); + bundle.putInt(Adjustment.KEY_IMPORTANCE, IMPORTANCE_NONE); + Adjustment adjustment = new Adjustment( + r.sbn.getPackageName(), r.getKey(), bundle, "", r.getUser().getIdentifier()); + mBinderService.applyEnqueuedAdjustmentFromAssistant(null, adjustment); NotificationManagerService.PostNotificationRunnable runnable = mService.new PostNotificationRunnable(r.getKey()); @@ -1814,7 +1820,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { mListener = mock(ManagedServices.ManagedServiceInfo.class); when(mListener.enabledAndUserMatches(anyInt())).thenReturn(false); when(mListeners.checkServiceTokenLocked(any())).thenReturn(mListener); -try { + try { mBinderService.getNotificationChannelGroupsFromPrivilegedListener( null, PKG, Process.myUserHandle()); fail("listeners that don't have a companion device shouldn't be able to call this"); @@ -2617,7 +2623,7 @@ try { } @Test - public void testAssistantIBlockingTriggersCancel() throws Exception { + public void testAssistantBlockingTriggersCancel() throws Exception { final NotificationRecord r = generateNotificationRecord(mTestNotificationChannel); mService.addNotification(r); NotificationManagerService.WorkerHandler handler = mock( @@ -2656,6 +2662,43 @@ try { assertEquals(USER_SENTIMENT_NEGATIVE, r.getUserSentiment()); } + @Test + public void testApplyEnqueuedAdjustmentFromAssistant_importance_onTime() throws Exception { + final NotificationRecord r = generateNotificationRecord(mTestNotificationChannel); + mService.addEnqueuedNotification(r); + NotificationManagerService.WorkerHandler handler = mock( + NotificationManagerService.WorkerHandler.class); + mService.setHandler(handler); + when(mAssistants.isSameUser(eq(null), anyInt())).thenReturn(true); + + Bundle signals = new Bundle(); + signals.putInt(Adjustment.KEY_IMPORTANCE, IMPORTANCE_LOW); + Adjustment adjustment = new Adjustment( + r.sbn.getPackageName(), r.getKey(), signals, "", r.getUser().getIdentifier()); + mBinderService.applyEnqueuedAdjustmentFromAssistant(null, adjustment); + + assertEquals(IMPORTANCE_LOW, r.getImportance()); + } + + @Test + public void testApplyEnqueuedAdjustmentFromAssistant_importance_tooLate() throws Exception { + final NotificationRecord r = generateNotificationRecord(mTestNotificationChannel); + mService.addNotification(r); + NotificationManagerService.WorkerHandler handler = mock( + NotificationManagerService.WorkerHandler.class); + mService.setHandler(handler); + when(mAssistants.isSameUser(eq(null), anyInt())).thenReturn(true); + + Bundle signals = new Bundle(); + signals.putInt(Adjustment.KEY_IMPORTANCE, IMPORTANCE_LOW); + Adjustment adjustment = new Adjustment( + r.sbn.getPackageName(), r.getKey(), signals, "", r.getUser().getIdentifier()); + mBinderService.applyEnqueuedAdjustmentFromAssistant(null, adjustment); + + assertEquals(IMPORTANCE_DEFAULT, r.getImportance()); + assertFalse(r.hasAdjustment(Adjustment.KEY_IMPORTANCE)); + } + @Test public void testApplyEnqueuedAdjustmentFromAssistant_crossUser() throws Exception { final NotificationRecord r = generateNotificationRecord(mTestNotificationChannel); @@ -2856,7 +2899,7 @@ try { @Test public void updateUriPermissions_update() throws Exception { NotificationChannel c = new NotificationChannel( - TEST_CHANNEL_ID, TEST_CHANNEL_ID, NotificationManager.IMPORTANCE_DEFAULT); + TEST_CHANNEL_ID, TEST_CHANNEL_ID, IMPORTANCE_DEFAULT); c.setSound(null, Notification.AUDIO_ATTRIBUTES_DEFAULT); Message message1 = new Message("", 0, ""); message1.setData("", diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java index 8690110ca66a3..0eeeeed0b227b 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java @@ -760,6 +760,7 @@ public class NotificationRecordTest extends UiServiceTestCase { NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); record.setAssistantImportance(IMPORTANCE_LOW); + record.calculateImportance(); assertEquals(IMPORTANCE_LOW, record.getImportance()); // assistant ignored if user expressed preference @@ -767,6 +768,7 @@ public class NotificationRecordTest extends UiServiceTestCase { channel.lockFields(USER_LOCKED_IMPORTANCE); record.setAssistantImportance(IMPORTANCE_LOW); + record.calculateImportance(); assertEquals(channel.getImportance(), record.getImportance()); } @@ -779,6 +781,7 @@ public class NotificationRecordTest extends UiServiceTestCase { NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); record.setAssistantImportance(IMPORTANCE_LOW); + record.calculateImportance(); assertEquals(IMPORTANCE_LOW, record.getImportance()); record.updateNotificationChannel(