From b3c68ff47a57301d690f792ebbe98f9cbc58f053 Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Tue, 22 May 2018 14:58:39 -0400 Subject: Change notification interruption calculation Updates that change notification text will only be counted if the user sees the update, so apps that are silently keeping their notification data fresh will not be punished. Test: runtest systemui-notification Change-Id: I3d494417e92296ad9a1742db2ab949132ebac18f Fixes: 78643290 --- .../notification/NotificationManagerService.java | 9 ++-- .../server/notification/NotificationRecord.java | 17 ++++++++ .../NotificationManagerServiceTest.java | 48 +++++++++++++++++++++- .../notification/NotificationRecordTest.java | 41 ++++++++++++++++++ 4 files changed, 111 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index e2e6f6dbad79..8f7fa1b2a7f7 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -821,6 +821,7 @@ public class NotificationManagerService extends SystemService { } } r.setVisibility(true, nv.rank, nv.count); + maybeRecordInterruptionLocked(r); nv.recycle(); } // Note that we might receive this event after notifications @@ -1928,11 +1929,12 @@ public class NotificationManagerService extends SystemService { @GuardedBy("mNotificationLock") protected void maybeRecordInterruptionLocked(NotificationRecord r) { - if (r.isInterruptive()) { + if (r.isInterruptive() && !r.hasRecordedInterruption()) { mAppUsageStats.reportInterruptiveNotification(r.sbn.getPackageName(), r.getChannel().getId(), getRealUserId(r.sbn.getUserId())); logRecentLocked(r); + r.setRecordedInterruption(true); } } @@ -2669,6 +2671,7 @@ public class NotificationManagerService extends SystemService { if (DBG) Slog.d(TAG, "Marking notification as seen " + keys[i]); reportSeen(r); r.setSeen(); + maybeRecordInterruptionLocked(r); } } } @@ -4440,7 +4443,7 @@ public class NotificationManagerService extends SystemService { if (index < 0) { mNotificationList.add(r); mUsageStats.registerPostedByApp(r); - r.setInterruptive(isVisuallyInterruptive(null, r)); + r.setInterruptive(true); } else { old = mNotificationList.get(index); mNotificationList.set(index, r); @@ -4449,7 +4452,7 @@ public class NotificationManagerService extends SystemService { notification.flags |= old.getNotification().flags & FLAG_FOREGROUND_SERVICE; r.isUpdate = true; - r.setInterruptive(isVisuallyInterruptive(old, r)); + r.setTextChanged(isVisuallyInterruptive(old, r)); } mNotificationsByKey.put(n.getKey(), r); diff --git a/services/core/java/com/android/server/notification/NotificationRecord.java b/services/core/java/com/android/server/notification/NotificationRecord.java index 2aec3eaf757c..0f3f44eae2d3 100644 --- a/services/core/java/com/android/server/notification/NotificationRecord.java +++ b/services/core/java/com/android/server/notification/NotificationRecord.java @@ -158,6 +158,8 @@ public final class NotificationRecord { private final NotificationStats mStats; private int mUserSentiment; private boolean mIsInterruptive; + private boolean mTextChanged; + private boolean mRecordedInterruption; private int mNumberOfSmartRepliesAdded; private boolean mHasSeenSmartReplies; /** @@ -839,6 +841,9 @@ public final class NotificationRecord { /** Mark the notification as seen by the user. */ public void setSeen() { mStats.setSeen(); + if (mTextChanged) { + mIsInterruptive = true; + } } public void setAuthoritativeRank(int authoritativeRank) { @@ -935,6 +940,18 @@ public final class NotificationRecord { mIsInterruptive = interruptive; } + public void setTextChanged(boolean textChanged) { + mTextChanged = textChanged; + } + + public void setRecordedInterruption(boolean recorded) { + mRecordedInterruption = recorded; + } + + public boolean hasRecordedInterruption() { + return mRecordedInterruption; + } + public boolean isInterruptive() { return mIsInterruptive; } 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 3dde7f1b59b3..41b4718e6343 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -91,10 +91,12 @@ import android.os.Build; import android.os.Bundle; import android.os.IBinder; import android.os.Process; +import android.os.RemoteException; import android.os.UserHandle; import android.provider.MediaStore; import android.provider.Settings.Secure; import android.service.notification.Adjustment; +import android.service.notification.INotificationListener; import android.service.notification.NotificationListenerService; import android.service.notification.NotificationStats; import android.service.notification.NotifyingApp; @@ -160,6 +162,8 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Mock private NotificationUsageStats mUsageStats; @Mock + private UsageStatsManagerInternal mAppUsageStats; + @Mock private AudioManager mAudioManager; @Mock ActivityManager mActivityManager; @@ -276,7 +280,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { mPackageManager, mPackageManagerClient, mockLightsManager, mListeners, mAssistants, mConditionProviders, mCompanionMgr, mSnoozeHelper, mUsageStats, mPolicyFile, mActivityManager, - mGroupHelper, mAm, mock(UsageStatsManagerInternal.class), + mGroupHelper, mAm, mAppUsageStats, mock(DevicePolicyManagerInternal.class)); } catch (SecurityException e) { if (!e.getMessage().contains("Permission Denial: not allowed to send broadcast")) { @@ -3019,4 +3023,46 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { assertEquals(true, mService.canUseManagedServices("d")); } + + @Test + public void testOnNotificationVisibilityChanged_triggersInterruptionUsageStat() { + final NotificationRecord r = generateNotificationRecord( + mTestNotificationChannel, 1, null, true); + r.setTextChanged(true); + mService.addNotification(r); + + mService.mNotificationDelegate.onNotificationVisibilityChanged(new NotificationVisibility[] + {NotificationVisibility.obtain(r.getKey(), 1, 1, true)}, + new NotificationVisibility[]{}); + + verify(mAppUsageStats).reportInterruptiveNotification(anyString(), anyString(), anyInt()); + } + + @Test + public void testSetNotificationsShownFromListener_triggersInterruptionUsageStat() + throws RemoteException { + final NotificationRecord r = generateNotificationRecord( + mTestNotificationChannel, 1, null, true); + r.setTextChanged(true); + mService.addNotification(r); + + mBinderService.setNotificationsShownFromListener(null, new String[] {r.getKey()}); + + verify(mAppUsageStats).reportInterruptiveNotification(anyString(), anyString(), anyInt()); + } + + @Test + public void testMybeRecordInterruptionLocked_doesNotRecordTwice() + throws RemoteException { + final NotificationRecord r = generateNotificationRecord( + mTestNotificationChannel, 1, null, true); + r.setInterruptive(true); + mService.addNotification(r); + + mService.maybeRecordInterruptionLocked(r); + mService.maybeRecordInterruptionLocked(r); + + verify(mAppUsageStats, times(1)).reportInterruptiveNotification( + anyString(), anyString(), anyInt()); + } } 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 e3289ab3e66e..c1868e7da921 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordTest.java @@ -602,4 +602,45 @@ public class NotificationRecordTest extends UiServiceTestCase { record.setIsAppImportanceLocked(false); assertEquals(false, record.getIsAppImportanceLocked()); } + + @Test + public void testIsInterruptive_textChanged_notSeen() { + StatusBarNotification sbn = getNotification(false /*preO */, false /* noisy */, + false /* defaultSound */, false /* buzzy */, false /* defaultBuzz */, + false /* lights */, false /* defaultLights */, null /* group */); + NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); + + assertEquals(false, record.isInterruptive()); + + record.setTextChanged(true); + assertEquals(false, record.isInterruptive()); + } + + @Test + public void testIsInterruptive_textChanged_seen() { + StatusBarNotification sbn = getNotification(false /*preO */, false /* noisy */, + false /* defaultSound */, false /* buzzy */, false /* defaultBuzz */, + false /* lights */, false /* defaultLights */, null /* group */); + NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); + + assertEquals(false, record.isInterruptive()); + + record.setTextChanged(true); + record.setSeen(); + assertEquals(true, record.isInterruptive()); + } + + @Test + public void testIsInterruptive_textNotChanged_seen() { + StatusBarNotification sbn = getNotification(false /*preO */, false /* noisy */, + false /* defaultSound */, false /* buzzy */, false /* defaultBuzz */, + false /* lights */, false /* defaultLights */, null /* group */); + NotificationRecord record = new NotificationRecord(mMockContext, sbn, channel); + + assertEquals(false, record.isInterruptive()); + + record.setTextChanged(false); + record.setSeen(); + assertEquals(false, record.isInterruptive()); + } } -- cgit v1.2.3-59-g8ed1b