From 7dd983d3e32986de4b934a3a59eb32d176e8493b Mon Sep 17 00:00:00 2001 From: Alexander Roederer Date: Thu, 15 Feb 2024 21:24:05 +0000 Subject: Update lifetime extended notif on second reply Add an update from system server to system ui when a notification that has already been lifetime extended has another direct reply. This ensures a second direct reply appears in the UI, even if the app doesn't update the notification (possibly because it thinks it's been cancelled after the first direct reply). Also moves the handling of the action click removal of lifetime extension up in the processing. This is important because otherwise removing the flag happens after the app has received the action, which can prevent it from canceling the notification (if that's what the app does in response to the action; applicable for "Mark as Read" and similar. Bug: 230652175 Test: atest NotificationManagerServiceTest.java, flash+build Flag: ACONFIG android.app.lifetime_extension_refactor DEVELOPMENT Change-Id: I000397bdfcc8a469f9f9b2365589e8237efc3640 --- .../statusbar/NotificationClickNotifier.kt | 11 +++- .../notification/NotificationManagerService.java | 57 +++++++++++++++++-- .../NotificationManagerServiceTest.java | 64 ++++++++++++++++++++++ 3 files changed, 124 insertions(+), 8 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationClickNotifier.kt b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationClickNotifier.kt index 692a9977c364..4ab78aab372d 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationClickNotifier.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationClickNotifier.kt @@ -1,8 +1,8 @@ package com.android.systemui.statusbar +import android.app.Flags.lifetimeExtensionRefactor import android.app.Notification import android.os.RemoteException -import android.util.Log import com.android.internal.statusbar.IStatusBarService import com.android.internal.statusbar.NotificationVisibility import com.android.systemui.dagger.SysUISingleton @@ -58,9 +58,14 @@ public class NotificationClickNotifier @Inject constructor( } catch (e: RemoteException) { // nothing } + if (lifetimeExtensionRefactor()) { + notifyListenersAboutInteraction(key) + } } - mainExecutor.execute { - notifyListenersAboutInteraction(key) + if (!lifetimeExtensionRefactor()) { + mainExecutor.execute { + notifyListenersAboutInteraction(key) + } } } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 53ae60b0cc76..83dd72ae0e97 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -1355,7 +1355,8 @@ public class NotificationManagerService extends SystemService { nv.recycle(); reportUserInteraction(r); mAssistants.notifyAssistantActionClicked(r, action, generatedByAssistant); - // Notifications that have been interacted with don't need to be lifetime extended. + // Notifications that have been interacted with should no longer be lifetime + // extended. if (lifetimeExtensionRefactor()) { r.getSbn().getNotification().flags &= ~FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY; } @@ -1524,9 +1525,32 @@ public class NotificationManagerService extends SystemService { @Override public void onNotificationDirectReplied(String key) { exitIdle(); + String packageName = null; + final int packageImportance; synchronized (mNotificationLock) { NotificationRecord r = mNotificationsByKey.get(key); if (r != null) { + packageName = r.getSbn().getPackageName(); + } + } + if (lifetimeExtensionRefactor() && packageName != null) { + packageImportance = getPackageImportanceWithIdentity(packageName); + } else { + packageImportance = IMPORTANCE_NONE; + } + synchronized (mNotificationLock) { + NotificationRecord r = mNotificationsByKey.get(key); + if (r != null) { + // If the notification is already marked as lifetime extended before we record + // the new direct reply, there must have been a previous lifetime extension + // event, and the app has already cancelled the notification, or does not + // respond to direct replies with updates. So we need to update System UI + // immediately. + if (lifetimeExtensionRefactor()) { + maybeNotifySystemUiListenerLifetimeExtendedLocked(r, + r.getSbn().getPackageName(), packageImportance); + } + r.recordDirectReplied(); mMetricsLogger.write(r.getLogMaker() .setCategory(MetricsEvent.NOTIFICATION_DIRECT_REPLY_ACTION) @@ -1557,10 +1581,31 @@ public class NotificationManagerService extends SystemService { @Override public void onNotificationSmartReplySent(String key, int replyIndex, CharSequence reply, int notificationLocation, boolean modifiedBeforeSending) { - + String packageName = null; + final int packageImportance; synchronized (mNotificationLock) { NotificationRecord r = mNotificationsByKey.get(key); if (r != null) { + packageName = r.getSbn().getPackageName(); + } + } + if (lifetimeExtensionRefactor() && packageName != null) { + packageImportance = getPackageImportanceWithIdentity(packageName); + } else { + packageImportance = IMPORTANCE_NONE; + } + synchronized (mNotificationLock) { + NotificationRecord r = mNotificationsByKey.get(key); + if (r != null) { + // If the notification is already marked as lifetime extended before we record + // the new direct reply, there must have been a previous lifetime extension + // event, and the app has already cancelled the notification, or does not + // respond to direct replies with updates. So we need to update System UI + // immediately. + if (lifetimeExtensionRefactor()) { + maybeNotifySystemUiListenerLifetimeExtendedLocked(r, + r.getSbn().getPackageName(), packageImportance); + } r.recordSmartReplied(); LogMaker logMaker = r.getLogMaker() .setCategory(MetricsEvent.SMART_REPLY_ACTION) @@ -7727,7 +7772,7 @@ public class NotificationManagerService extends SystemService { notification.flags &= ~FLAG_FSI_REQUESTED_BUT_DENIED; - // Apps should not create notifications that are lifetime extended. + // Apps cannot post notifications that are lifetime extended. if (lifetimeExtensionRefactor()) { notification.flags &= ~FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY; } @@ -11997,8 +12042,10 @@ public class NotificationManagerService extends SystemService { @Override public void onServiceAdded(ManagedServiceInfo info) { if (lifetimeExtensionRefactor()) { - // Only System or System UI can call registerSystemService, so if the caller is not - // system, we know it's system UI. + // Generally, only System or System UI should have the permissions to call + // registerSystemService. + // isCallerSystemorPhone tells us whether the caller is System. Then, if it's not + // the system, we know it's system UI. info.isSystemUi = !isCallerSystemOrPhone(); } final INotificationListener listener = (INotificationListener) info.service; 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 94d24a91bbc9..9a1129b42a28 100755 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -5840,6 +5840,30 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { mNotificationRecordLogger.event(0)); } + @Test + public void testStats_DirectReplyLifetimeExtendedPostsUpdate() throws Exception { + mSetFlagsRule.enableFlags(android.app.Flags.FLAG_LIFETIME_EXTENSION_REFACTOR); + final NotificationRecord r = generateNotificationRecord(mTestNotificationChannel); + r.getSbn().getNotification().flags |= FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY; + mService.addNotification(r); + + mService.mNotificationDelegate.onNotificationDirectReplied(r.getKey()); + waitForIdle(); + + assertThat(mService.getNotificationRecord(r.getKey()).getStats().hasDirectReplied()) + .isTrue(); + // Checks that a post update is sent. + verify(mWorkerHandler, times(1)) + .post(any(NotificationManagerService.PostNotificationRunnable.class)); + ArgumentCaptor captor = + ArgumentCaptor.forClass(NotificationRecord.class); + verify(mListeners, times(1)).prepareNotifyPostedLocked(captor.capture(), any(), + anyBoolean()); + assertThat(captor.getValue().getNotification().flags + & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY).isEqualTo( + FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY); + } + @Test public void testStats_updatedOnUserExpansion() throws Exception { NotificationRecord r = generateNotificationRecord(mTestNotificationChannel); @@ -8505,6 +8529,36 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { assertThat(r.getStats().hasSmartReplied()).isTrue(); } + @Test + public void testStats_SmartReplyAlreadyLifetimeExtendedPostsUpdate() throws Exception { + mSetFlagsRule.enableFlags(android.app.Flags.FLAG_LIFETIME_EXTENSION_REFACTOR); + final int replyIndex = 2; + final String reply = "Hello"; + final boolean modifiedBeforeSending = true; + final boolean generatedByAssistant = true; + + NotificationRecord r = generateNotificationRecord(mTestNotificationChannel); + r.getSbn().getNotification().flags |= FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY; + r.setSuggestionsGeneratedByAssistant(generatedByAssistant); + mService.addNotification(r); + + mService.mNotificationDelegate.onNotificationSmartReplySent( + r.getKey(), replyIndex, reply, NOTIFICATION_LOCATION_UNKNOWN, + modifiedBeforeSending); + waitForIdle(); + + // Checks that a post update is sent. + verify(mWorkerHandler, times(1)) + .post(any(NotificationManagerService.PostNotificationRunnable.class)); + ArgumentCaptor captor = + ArgumentCaptor.forClass(NotificationRecord.class); + verify(mListeners, times(1)).prepareNotifyPostedLocked(captor.capture(), any(), + anyBoolean()); + assertThat(captor.getValue().getNotification().flags + & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY).isEqualTo( + FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY); + } + @Test public void testOnNotificationActionClick() { final int actionIndex = 2; @@ -8536,6 +8590,8 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { final Notification.Action action = new Notification.Action.Builder(null, "text", PendingIntent.getActivity( mContext, 0, new Intent(), PendingIntent.FLAG_IMMUTABLE)).build(); + final boolean generatedByAssistant = false; + // Creates a notification marked as being lifetime extended. NotificationRecord r = generateNotificationRecord(mTestNotificationChannel); r.getSbn().getNotification().flags |= FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY; @@ -8549,6 +8605,14 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { // The flag is removed, so the notification is no longer lifetime extended. assertThat(r.getSbn().getNotification().flags & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY).isEqualTo(0); + + // The record is sent out without the flag. + ArgumentCaptor captor = + ArgumentCaptor.forClass(NotificationRecord.class); + verify(mAssistants, times(1)).notifyAssistantActionClicked( + captor.capture(), eq(action), eq(generatedByAssistant)); + assertThat(captor.getValue().getNotification().flags + & FLAG_LIFETIME_EXTENDED_BY_DIRECT_REPLY).isEqualTo(0); } @Test -- cgit v1.2.3-59-g8ed1b