From 543692372b0bcfba1f04bd650cfd69a3df8ceb34 Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Tue, 3 Jul 2018 10:43:35 -0400 Subject: [PATCH] Be more strict about triggering notification lights Don't trigger lights if they should be suppressed by notification flags. Don't log that notification lights happened if global or temporary settings have turned them off Test: runtest systemui-notification, verify notification_itnerruptiveness numbers for a fresh 3p app that uses lights Bug: 111069748 Change-Id: I68750bf39eb76ef0bda40ddacfd90e1be79e8575 (cherry picked from commit 28149f65f9f036f746957d1323efda20aed94ba3) --- .../NotificationManagerService.java | 71 ++++++- .../notification/BuzzBeepBlinkTest.java | 175 +++++++++++++++++- 2 files changed, 236 insertions(+), 10 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index dcd1e885ae430..3ed8e2e08e5b4 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -329,6 +329,8 @@ public class NotificationManagerService extends SystemService { private long[] mFallbackVibrationPattern; private boolean mUseAttentionLight; + boolean mHasLight = true; + boolean mLightEnabled; boolean mSystemReady; private boolean mDisableNotificationEffects; @@ -343,9 +345,9 @@ public class NotificationManagerService extends SystemService { private int mInterruptionFilter = NotificationListenerService.INTERRUPTION_FILTER_UNKNOWN; // for enabling and disabling notification pulse behavior - private boolean mScreenOn = true; + boolean mScreenOn = true; protected boolean mInCall = false; - private boolean mNotificationPulseEnabled; + boolean mNotificationPulseEnabled; private Uri mInCallNotificationUri; private AudioAttributes mInCallNotificationAudioAttributes; @@ -1198,7 +1200,8 @@ public class NotificationManagerService extends SystemService { ContentResolver resolver = getContext().getContentResolver(); if (uri == null || NOTIFICATION_LIGHT_PULSE_URI.equals(uri)) { boolean pulseEnabled = Settings.System.getIntForUser(resolver, - Settings.System.NOTIFICATION_LIGHT_PULSE, 0, UserHandle.USER_CURRENT) != 0; + Settings.System.NOTIFICATION_LIGHT_PULSE, 0, UserHandle.USER_CURRENT) + != 0; if (mNotificationPulseEnabled != pulseEnabled) { mNotificationPulseEnabled = pulseEnabled; updateNotificationPulse(); @@ -1460,6 +1463,8 @@ public class NotificationManagerService extends SystemService { mInCallNotificationVolume = resources.getFloat(R.dimen.config_inCallNotificationVolume); mUseAttentionLight = resources.getBoolean(R.bool.config_useAttentionLight); + mHasLight = + resources.getBoolean(com.android.internal.R.bool.config_intrusiveNotificationLed); // Don't start allowing notifications until the setup wizard has run once. // After that, including subsequent boots, init with notifications turned on. @@ -3825,6 +3830,7 @@ public class NotificationManagerService extends SystemService { pw.println(" "); } pw.println(" mUseAttentionLight=" + mUseAttentionLight); + pw.println(" mHasLight=" + mHasLight); pw.println(" mNotificationPulseEnabled=" + mNotificationPulseEnabled); pw.println(" mSoundNotificationKey=" + mSoundNotificationKey); pw.println(" mVibrateNotificationKey=" + mVibrateNotificationKey); @@ -4822,8 +4828,7 @@ public class NotificationManagerService extends SystemService { // light // release the light boolean wasShowLights = mLights.remove(key); - if (record.getLight() != null && aboveThreshold - && ((record.getSuppressedVisualEffects() & SUPPRESSED_EFFECT_LIGHTS) == 0)) { + if (canShowLightsLocked(record, aboveThreshold)) { mLights.add(key); updateLightsLocked(); if (mUseAttentionLight) { @@ -4834,7 +4839,19 @@ public class NotificationManagerService extends SystemService { updateLightsLocked(); } if (buzz || beep || blink) { - record.setInterruptive(true); + // Ignore summary updates because we don't display most of the information. + if (record.sbn.isGroup() && record.sbn.getNotification().isGroupSummary()) { + if (DEBUG_INTERRUPTIVENESS) { + Log.v(TAG, "INTERRUPTIVENESS: " + + record.getKey() + " is not interruptive: summary"); + } + } else { + if (DEBUG_INTERRUPTIVENESS) { + Log.v(TAG, "INTERRUPTIVENESS: " + + record.getKey() + " is interruptive: alerted"); + } + record.setInterruptive(true); + } MetricsLogger.action(record.getLogMaker() .setCategory(MetricsEvent.NOTIFICATION_ALERT) .setType(MetricsEvent.TYPE_OPEN) @@ -4843,12 +4860,50 @@ public class NotificationManagerService extends SystemService { } } + @GuardedBy("mNotificationLock") + boolean canShowLightsLocked(final NotificationRecord record, boolean aboveThreshold) { + // device lacks light + if (!mHasLight) { + return false; + } + // user turned lights off globally + if (!mNotificationPulseEnabled) { + return false; + } + // the notification/channel has no light + if (record.getLight() == null) { + return false; + } + // unimportant notification + if (!aboveThreshold) { + return false; + } + // suppressed due to DND + if ((record.getSuppressedVisualEffects() & SUPPRESSED_EFFECT_LIGHTS) != 0) { + return false; + } + // Suppressed because it's a silent update + final Notification notification = record.getNotification(); + if (record.isUpdate && (notification.flags & Notification.FLAG_ONLY_ALERT_ONCE) != 0) { + return false; + } + // Suppressed because another notification in its group handles alerting + if (record.sbn.isGroup() && record.getNotification().suppressAlertingDueToGrouping()) { + return false; + } + // not if in call or the screen's on + if (mInCall || mScreenOn) { + return false; + } + + return true; + } + @GuardedBy("mNotificationLock") boolean shouldMuteNotificationLocked(final NotificationRecord record) { // Suppressed because it's a silent update final Notification notification = record.getNotification(); - if(record.isUpdate - && (notification.flags & Notification.FLAG_ONLY_ALERT_ONCE) != 0) { + if (record.isUpdate && (notification.flags & Notification.FLAG_ONLY_ALERT_ONCE) != 0) { return true; } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/BuzzBeepBlinkTest.java b/services/tests/uiservicestests/src/com/android/server/notification/BuzzBeepBlinkTest.java index 78099996a1a01..bdba3d5cd6779 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/BuzzBeepBlinkTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/BuzzBeepBlinkTest.java @@ -19,7 +19,9 @@ import static android.app.Notification.GROUP_ALERT_ALL; import static android.app.Notification.GROUP_ALERT_CHILDREN; import static android.app.Notification.GROUP_ALERT_SUMMARY; 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.Policy.SUPPRESSED_EFFECT_LIGHTS; import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertNull; @@ -149,6 +151,9 @@ public class BuzzBeepBlinkTest extends UiServiceTestCase { mService.setFallbackVibrationPattern(FALLBACK_VIBRATION_PATTERN); mService.setUsageStats(mUsageStats); mService.setAccessibilityManager(accessibilityManager); + mService.mScreenOn = false; + mService.mInCall = false; + mService.mNotificationPulseEnabled = true; } // @@ -216,8 +221,13 @@ public class BuzzBeepBlinkTest extends UiServiceTestCase { } private NotificationRecord getLightsNotification() { + return getNotificationRecord(mId, false /* insistent */, false /* once */, + false /* noisy */, false /* buzzy*/, true /* lights */); + } + + private NotificationRecord getLightsOnceNotification() { return getNotificationRecord(mId, false /* insistent */, true /* once */, - false /* noisy */, true /* buzzy*/, true /* lights */); + false /* noisy */, false /* buzzy*/, true /* lights */); } private NotificationRecord getCustomLightsNotification() { @@ -244,6 +254,12 @@ public class BuzzBeepBlinkTest extends UiServiceTestCase { groupKey, groupAlertBehavior, false); } + private NotificationRecord getLightsNotificationRecord(String groupKey, + int groupAlertBehavior) { + return getNotificationRecord(mId, false, false, false, false, true /*lights*/, true, true, + true, groupKey, groupAlertBehavior, false); + } + private NotificationRecord getNotificationRecord(int id, boolean insistent, boolean once, boolean noisy, boolean buzzy, boolean lights, boolean defaultVibration, boolean defaultSound, boolean defaultLights, String groupKey, int groupAlertBehavior, @@ -369,6 +385,10 @@ public class BuzzBeepBlinkTest extends UiServiceTestCase { verify(mVibrator, never()).cancel(); } + private void verifyNeverLights() { + verify(mLight, never()).setFlashing(anyInt(), anyInt(), anyInt(), anyInt()); + } + private void verifyLights() { verify(mLight, times(1)).setFlashing(anyInt(), anyInt(), anyInt(), anyInt()); } @@ -712,7 +732,8 @@ public class BuzzBeepBlinkTest extends UiServiceTestCase { mService.buzzBeepBlinkLocked(summary); verifyBeepLooped(); - assertTrue(summary.isInterruptive()); + // summaries are never interruptive for notification counts + assertFalse(summary.isInterruptive()); } @Test @@ -990,6 +1011,156 @@ public class BuzzBeepBlinkTest extends UiServiceTestCase { verify(mAccessibilityService, times(1)).sendAccessibilityEvent(any(), anyInt()); } + @Test + public void testLightsScreenOn() { + mService.mScreenOn = true; + NotificationRecord r = getLightsNotification(); + mService.buzzBeepBlinkLocked(r); + verifyNeverLights(); + assertFalse(r.isInterruptive()); + } + + @Test + public void testLightsInCall() { + mService.mInCall = true; + NotificationRecord r = getLightsNotification(); + mService.buzzBeepBlinkLocked(r); + verifyNeverLights(); + assertFalse(r.isInterruptive()); + } + + @Test + public void testLightsSilentUpdate() { + NotificationRecord r = getLightsOnceNotification(); + mService.buzzBeepBlinkLocked(r); + verifyLights(); + assertTrue(r.isInterruptive()); + + r = getLightsOnceNotification(); + r.isUpdate = true; + mService.buzzBeepBlinkLocked(r); + // checks that lights happened once, i.e. this new call didn't trigger them again + verifyLights(); + assertFalse(r.isInterruptive()); + } + + @Test + public void testLightsUnimportant() { + NotificationRecord r = getLightsNotification(); + r.setImportance(IMPORTANCE_LOW, "testing"); + mService.buzzBeepBlinkLocked(r); + verifyNeverLights(); + assertFalse(r.isInterruptive()); + } + + @Test + public void testLightsNoLights() { + NotificationRecord r = getQuietNotification(); + mService.buzzBeepBlinkLocked(r); + verifyNeverLights(); + assertFalse(r.isInterruptive()); + } + + @Test + public void testLightsNoLightOnDevice() { + mService.mHasLight = false; + NotificationRecord r = getLightsNotification(); + mService.buzzBeepBlinkLocked(r); + verifyNeverLights(); + assertFalse(r.isInterruptive()); + } + + @Test + public void testLightsLightsOffGlobally() { + mService.mNotificationPulseEnabled = false; + NotificationRecord r = getLightsNotification(); + mService.buzzBeepBlinkLocked(r); + verifyNeverLights(); + assertFalse(r.isInterruptive()); + } + + @Test + public void testLightsDndIntercepted() { + NotificationRecord r = getLightsNotification(); + r.setSuppressedVisualEffects(SUPPRESSED_EFFECT_LIGHTS); + mService.buzzBeepBlinkLocked(r); + verifyNeverLights(); + assertFalse(r.isInterruptive()); + } + + @Test + public void testGroupAlertSummaryNoLightsChild() { + NotificationRecord child = getLightsNotificationRecord("a", GROUP_ALERT_SUMMARY); + + mService.buzzBeepBlinkLocked(child); + + verifyNeverLights(); + assertFalse(child.isInterruptive()); + } + + @Test + public void testGroupAlertSummaryLightsSummary() { + NotificationRecord summary = getLightsNotificationRecord("a", GROUP_ALERT_SUMMARY); + summary.getNotification().flags |= Notification.FLAG_GROUP_SUMMARY; + + mService.buzzBeepBlinkLocked(summary); + + verifyLights(); + // summaries should never count for interruptiveness counts + assertFalse(summary.isInterruptive()); + } + + @Test + public void testGroupAlertSummaryLightsNonGroupChild() { + NotificationRecord nonGroup = getLightsNotificationRecord(null, GROUP_ALERT_SUMMARY); + + mService.buzzBeepBlinkLocked(nonGroup); + + verifyLights(); + assertTrue(nonGroup.isInterruptive()); + } + + @Test + public void testGroupAlertChildNoLightsSummary() { + NotificationRecord summary = getLightsNotificationRecord("a", GROUP_ALERT_CHILDREN); + summary.getNotification().flags |= Notification.FLAG_GROUP_SUMMARY; + + mService.buzzBeepBlinkLocked(summary); + + verifyNeverLights(); + assertFalse(summary.isInterruptive()); + } + + @Test + public void testGroupAlertChildLightsChild() { + NotificationRecord child = getLightsNotificationRecord("a", GROUP_ALERT_CHILDREN); + + mService.buzzBeepBlinkLocked(child); + + verifyLights(); + assertTrue(child.isInterruptive()); + } + + @Test + public void testGroupAlertChildLightsNonGroupSummary() { + NotificationRecord nonGroup = getLightsNotificationRecord(null, GROUP_ALERT_CHILDREN); + + mService.buzzBeepBlinkLocked(nonGroup); + + verifyLights(); + assertTrue(nonGroup.isInterruptive()); + } + + @Test + public void testGroupAlertAllLightsGroup() { + NotificationRecord group = getLightsNotificationRecord("a", GROUP_ALERT_ALL); + + mService.buzzBeepBlinkLocked(group); + + verifyLights(); + assertTrue(group.isInterruptive()); + } + static class VibrateRepeatMatcher implements ArgumentMatcher { private final int mRepeatIndex;