From 221212c6f1d2a6c6b44c082a0fbc9c72563d90e8 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Mon, 2 May 2022 19:16:46 +0000 Subject: [PATCH 1/4] Add logging required to diagnose the multi-entry issues Bug: 230540148 Bug: 227254780 Test: atest StatusBarNotificationActivityStarterTest Change-Id: I2d39596f46dce196503c51e37b7eb28f805b9ee4 --- .../NotificationActivityStarter.java | 3 +- .../notification/NotificationClicker.java | 2 +- .../notification/NotificationClickerLogger.kt | 10 ++-- .../notification/NotificationUtils.java | 13 ++++- .../notification/NotificationUtils.kt | 31 +++++++++++ .../StatusBarNotificationActivityStarter.java | 27 +++++---- ...tusBarNotificationActivityStarterLogger.kt | 42 +++++++------- ...tusBarNotificationActivityStarterTest.java | 55 +++++++++++-------- 8 files changed, 114 insertions(+), 69 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationUtils.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationActivityStarter.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationActivityStarter.java index 129fa5a7cc179..0c341cc92f834 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationActivityStarter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationActivityStarter.java @@ -17,7 +17,6 @@ package com.android.systemui.statusbar.notification; import android.content.Intent; -import android.service.notification.StatusBarNotification; import android.view.View; import com.android.systemui.statusbar.notification.collection.NotificationEntry; @@ -29,7 +28,7 @@ import com.android.systemui.statusbar.notification.row.ExpandableNotificationRow */ public interface NotificationActivityStarter { /** Called when the user clicks on the surface of a notification. */ - void onNotificationClicked(StatusBarNotification sbn, ExpandableNotificationRow row); + void onNotificationClicked(NotificationEntry entry, ExpandableNotificationRow row); /** Called when the user clicks on a button in the notification guts which fires an intent. */ void startNotificationGutsIntent(Intent intent, int appUid, diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationClicker.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationClicker.java index 392145ad306a1..c3ce593b54fdf 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationClicker.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationClicker.java @@ -104,7 +104,7 @@ public final class NotificationClicker implements View.OnClickListener { mBubblesOptional.get().collapseStack(); } - mNotificationActivityStarter.onNotificationClicked(entry.getSbn(), row); + mNotificationActivityStarter.onNotificationClicked(entry, row); } private boolean isMenuVisible(ExpandableNotificationRow row) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationClickerLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationClickerLogger.kt index fbf033bd22912..ad3dfedcdb969 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationClickerLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationClickerLogger.kt @@ -27,7 +27,7 @@ class NotificationClickerLogger @Inject constructor( ) { fun logOnClick(entry: NotificationEntry) { buffer.log(TAG, LogLevel.DEBUG, { - str1 = entry.key + str1 = entry.logKey str2 = entry.ranking.channel.id }, { "CLICK $str1 (channel=$str2)" @@ -36,7 +36,7 @@ class NotificationClickerLogger @Inject constructor( fun logMenuVisible(entry: NotificationEntry) { buffer.log(TAG, LogLevel.DEBUG, { - str1 = entry.key + str1 = entry.logKey }, { "Ignoring click on $str1; menu is visible" }) @@ -44,7 +44,7 @@ class NotificationClickerLogger @Inject constructor( fun logParentMenuVisible(entry: NotificationEntry) { buffer.log(TAG, LogLevel.DEBUG, { - str1 = entry.key + str1 = entry.logKey }, { "Ignoring click on $str1; parent menu is visible" }) @@ -52,7 +52,7 @@ class NotificationClickerLogger @Inject constructor( fun logChildrenExpanded(entry: NotificationEntry) { buffer.log(TAG, LogLevel.DEBUG, { - str1 = entry.key + str1 = entry.logKey }, { "Ignoring click on $str1; children are expanded" }) @@ -60,7 +60,7 @@ class NotificationClickerLogger @Inject constructor( fun logGutsExposed(entry: NotificationEntry) { buffer.log(TAG, LogLevel.DEBUG, { - str1 = entry.key + str1 = entry.logKey }, { "Ignoring click on $str1; guts are exposed" }) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationUtils.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationUtils.java index c3cc97bc14a3f..7cfb1571ccf07 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationUtils.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationUtils.java @@ -24,7 +24,8 @@ import android.widget.ImageView; import com.android.internal.util.ContrastColorUtil; import com.android.systemui.R; -import com.android.systemui.statusbar.notification.collection.NotificationEntry; +import com.android.systemui.statusbar.notification.collection.ListEntry; +import com.android.systemui.util.Compile; /** * A util class for various reusable functions @@ -74,12 +75,18 @@ public class NotificationUtils { return (int) (dimensionPixelSize * factor); } + private static final boolean INCLUDE_HASH_CODE_IN_LIST_ENTRY_LOG_KEY = false; + /** Get the notification key, reformatted for logging, for the (optional) entry */ - public static String logKey(NotificationEntry entry) { + public static String logKey(ListEntry entry) { if (entry == null) { return "null"; } - return logKey(entry.getKey()); + if (Compile.IS_DEBUG && INCLUDE_HASH_CODE_IN_LIST_ENTRY_LOG_KEY) { + return logKey(entry.getKey()) + "@" + Integer.toHexString(entry.hashCode()); + } else { + return logKey(entry.getKey()); + } } /** Removes newlines from the notification key to prettify apps that have these in the tag */ diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationUtils.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationUtils.kt new file mode 100644 index 0000000000000..432bac49fa9b5 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationUtils.kt @@ -0,0 +1,31 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License + */ + +package com.android.systemui.statusbar.notification + +import android.service.notification.StatusBarNotification +import com.android.systemui.statusbar.notification.collection.ListEntry + +/** Get the notification key, reformatted for logging, for the (optional) entry */ +val ListEntry?.logKey: String? + get() = this?.let { NotificationUtils.logKey(it) } + +/** Get the notification key, reformatted for logging, for the (optional) sbn */ +val StatusBarNotification?.logKey: String? + get() = this?.key?.let { NotificationUtils.logKey(it) } + +/** Removes newlines from the notification key to prettify apps that have these in the tag */ +fun logKey(key: String?): String? = NotificationUtils.logKey(key) \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java index 87ca942edff2c..af4c81b4181d8 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java @@ -229,14 +229,13 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte /** * Called when a notification is clicked. * - * @param sbn notification that was clicked + * @param entry notification that was clicked * @param row row for that notification */ @Override - public void onNotificationClicked(StatusBarNotification sbn, ExpandableNotificationRow row) { - mLogger.logStartingActivityFromClick(sbn.getKey()); + public void onNotificationClicked(NotificationEntry entry, ExpandableNotificationRow row) { + mLogger.logStartingActivityFromClick(entry); - final NotificationEntry entry = row.getEntry(); if (mRemoteInputManager.isRemoteInputActive(entry) && !TextUtils.isEmpty(row.getActiveRemoteInputText())) { // We have an active remote input typed and the user clicked on the notification. @@ -244,7 +243,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte mRemoteInputManager.closeRemoteInputs(); return; } - Notification notification = sbn.getNotification(); + Notification notification = entry.getSbn().getNotification(); final PendingIntent intent = notification.contentIntent != null ? notification.contentIntent : notification.fullScreenIntent; @@ -254,7 +253,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte // The only valid case is Bubble notifications. Guard against other cases // entering here. if (intent == null && !isBubble) { - mLogger.logNonClickableNotification(sbn.getKey()); + mLogger.logNonClickableNotification(entry); return; } @@ -299,7 +298,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte boolean isActivityIntent, boolean animate, boolean showOverLockscreen) { - mLogger.logHandleClickAfterKeyguardDismissed(entry.getKey()); + mLogger.logHandleClickAfterKeyguardDismissed(entry); final Runnable runnable = () -> handleNotificationClickAfterPanelCollapsed( entry, row, intent, isActivityIntent, animate); @@ -326,7 +325,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte boolean isActivityIntent, boolean animate) { String notificationKey = entry.getKey(); - mLogger.logHandleClickAfterPanelCollapsed(notificationKey); + mLogger.logHandleClickAfterPanelCollapsed(entry); try { // The intent we are sending is for the application, which @@ -367,7 +366,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte } final boolean canBubble = entry.canBubble(); if (canBubble) { - mLogger.logExpandingBubble(notificationKey); + mLogger.logExpandingBubble(entry); removeHunAfterClick(row); expandBubbleStackOnMainThread(entry); mMainThreadHandler.post( @@ -489,7 +488,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte ExpandableNotificationRow row, boolean animate, boolean isActivityIntent) { - mLogger.logStartNotificationIntent(entry.getKey()); + mLogger.logStartNotificationIntent(entry); try { Runnable onFinishAnimationCallback = animate ? () -> mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry) @@ -515,7 +514,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte : getActivityOptions(mCentralSurfaces.getDisplayId(), adapter); int result = intent.sendAndReturnResult(mContext, 0, fillInIntent, null, null, null, options); - mLogger.logSendPendingIntent(entry.getKey(), intent, result); + mLogger.logSendPendingIntent(entry, intent, result); return result; }); } catch (PendingIntent.CanceledException e) { @@ -622,9 +621,9 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte void handleFullScreenIntent(NotificationEntry entry) { if (mNotificationInterruptStateProvider.shouldLaunchFullScreenIntentWhenAdded(entry)) { if (shouldSuppressFullScreenIntent(entry)) { - mLogger.logFullScreenIntentSuppressedByDnD(entry.getKey()); + mLogger.logFullScreenIntentSuppressedByDnD(entry); } else if (entry.getImportance() < NotificationManager.IMPORTANCE_HIGH) { - mLogger.logFullScreenIntentNotImportantEnough(entry.getKey()); + mLogger.logFullScreenIntentNotImportantEnough(entry); } else { // Stop screensaver if the notification has a fullscreen intent. // (like an incoming phone call) @@ -639,7 +638,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte // not immersive & a fullscreen alert should be shown final PendingIntent fullscreenIntent = entry.getSbn().getNotification().fullScreenIntent; - mLogger.logSendingFullScreenIntent(entry.getKey(), fullscreenIntent); + mLogger.logSendingFullScreenIntent(entry, fullscreenIntent); try { EventLog.writeEvent(EventLogTags.SYSUI_FULLSCREEN_NOTIFICATION, entry.getKey()); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterLogger.kt index 2fbe520a4b610..b9a1413ff7914 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterLogger.kt @@ -23,46 +23,48 @@ import com.android.systemui.log.LogLevel.ERROR import com.android.systemui.log.LogLevel.INFO import com.android.systemui.log.LogLevel.WARNING import com.android.systemui.log.dagger.NotifInteractionLog +import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.logKey import javax.inject.Inject class StatusBarNotificationActivityStarterLogger @Inject constructor( @NotifInteractionLog private val buffer: LogBuffer ) { - fun logStartingActivityFromClick(key: String) { + fun logStartingActivityFromClick(entry: NotificationEntry) { buffer.log(TAG, DEBUG, { - str1 = key + str1 = entry.logKey }, { "(1/5) onNotificationClicked: $str1" }) } - fun logHandleClickAfterKeyguardDismissed(key: String) { + fun logHandleClickAfterKeyguardDismissed(entry: NotificationEntry) { buffer.log(TAG, DEBUG, { - str1 = key + str1 = entry.logKey }, { "(2/5) handleNotificationClickAfterKeyguardDismissed: $str1" }) } - fun logHandleClickAfterPanelCollapsed(key: String) { + fun logHandleClickAfterPanelCollapsed(entry: NotificationEntry) { buffer.log(TAG, DEBUG, { - str1 = key + str1 = entry.logKey }, { "(3/5) handleNotificationClickAfterPanelCollapsed: $str1" }) } - fun logStartNotificationIntent(key: String) { + fun logStartNotificationIntent(entry: NotificationEntry) { buffer.log(TAG, INFO, { - str1 = key + str1 = entry.logKey }, { "(4/5) startNotificationIntent: $str1" }) } - fun logSendPendingIntent(key: String, pendingIntent: PendingIntent, result: Int) { + fun logSendPendingIntent(entry: NotificationEntry, pendingIntent: PendingIntent, result: Int) { buffer.log(TAG, INFO, { - str1 = key + str1 = entry.logKey str2 = pendingIntent.intent.toString() int1 = result }, { @@ -70,9 +72,9 @@ class StatusBarNotificationActivityStarterLogger @Inject constructor( }) } - fun logExpandingBubble(key: String) { + fun logExpandingBubble(entry: NotificationEntry) { buffer.log(TAG, DEBUG, { - str1 = key + str1 = entry.logKey }, { "Expanding bubble for $str1 (rather than firing intent)" }) @@ -86,33 +88,33 @@ class StatusBarNotificationActivityStarterLogger @Inject constructor( }) } - fun logNonClickableNotification(key: String) { + fun logNonClickableNotification(entry: NotificationEntry) { buffer.log(TAG, ERROR, { - str1 = key + str1 = entry.logKey }, { "onNotificationClicked called for non-clickable notification! $str1" }) } - fun logFullScreenIntentSuppressedByDnD(key: String) { + fun logFullScreenIntentSuppressedByDnD(entry: NotificationEntry) { buffer.log(TAG, DEBUG, { - str1 = key + str1 = entry.logKey }, { "No Fullscreen intent: suppressed by DND: $str1" }) } - fun logFullScreenIntentNotImportantEnough(key: String) { + fun logFullScreenIntentNotImportantEnough(entry: NotificationEntry) { buffer.log(TAG, DEBUG, { - str1 = key + str1 = entry.logKey }, { "No Fullscreen intent: not important enough: $str1" }) } - fun logSendingFullScreenIntent(key: String, pendingIntent: PendingIntent) { + fun logSendingFullScreenIntent(entry: NotificationEntry, pendingIntent: PendingIntent) { buffer.log(TAG, INFO, { - str1 = key + str1 = entry.logKey str2 = pendingIntent.intent.toString() }, { "Notification $str1 has fullScreenIntent; sending fullScreenIntent $str2" diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java index fa867e2796f7c..f1a6eba129525 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java @@ -265,15 +265,16 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { public void testOnNotificationClicked_keyGuardShowing() throws PendingIntent.CanceledException, RemoteException { // Given - StatusBarNotification sbn = mNotificationRow.getEntry().getSbn(); - sbn.getNotification().contentIntent = mContentIntent; - sbn.getNotification().flags |= Notification.FLAG_AUTO_CANCEL; + NotificationEntry entry = mNotificationRow.getEntry(); + Notification notification = entry.getSbn().getNotification(); + notification.contentIntent = mContentIntent; + notification.flags |= Notification.FLAG_AUTO_CANCEL; when(mKeyguardStateController.isShowing()).thenReturn(true); when(mCentralSurfaces.isOccluded()).thenReturn(true); // When - mNotificationActivityStarter.onNotificationClicked(sbn, mNotificationRow); + mNotificationActivityStarter.onNotificationClicked(entry, mNotificationRow); // Then verify(mShadeController, atLeastOnce()).collapsePanel(); @@ -285,22 +286,23 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { InOrder orderVerifier = Mockito.inOrder(mClickNotifier, mOnUserInteractionCallback); orderVerifier.verify(mClickNotifier).onNotificationClick( - eq(sbn.getKey()), any(NotificationVisibility.class)); + eq(entry.getKey()), any(NotificationVisibility.class)); // Notification calls dismiss callback to remove notification due to FLAG_AUTO_CANCEL - orderVerifier.verify(mOnUserInteractionCallback).onDismiss(mNotificationRow.getEntry(), + orderVerifier.verify(mOnUserInteractionCallback).onDismiss(entry, REASON_CLICK, null); } @Test public void testOnNotificationClicked_bubble_noContentIntent_noKeyGuard() throws RemoteException { - StatusBarNotification sbn = mBubbleNotificationRow.getEntry().getSbn(); + NotificationEntry entry = mBubbleNotificationRow.getEntry(); + StatusBarNotification sbn = entry.getSbn(); // Given sbn.getNotification().contentIntent = null; // When - mNotificationActivityStarter.onNotificationClicked(sbn, mBubbleNotificationRow); + mNotificationActivityStarter.onNotificationClicked(entry, mBubbleNotificationRow); // Then verify(mBubblesManager).expandStackAndSelectBubble(eq(mBubbleNotificationRow.getEntry())); @@ -311,7 +313,7 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { verify(mAssistManager).hideAssist(); verify(mClickNotifier).onNotificationClick( - eq(sbn.getKey()), any(NotificationVisibility.class)); + eq(entry.getKey()), any(NotificationVisibility.class)); // The content intent should NOT be sent on click. verifyZeroInteractions(mContentIntent); @@ -324,7 +326,8 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { @Test public void testOnNotificationClicked_bubble_noContentIntent_keyGuardShowing() throws RemoteException { - StatusBarNotification sbn = mBubbleNotificationRow.getEntry().getSbn(); + NotificationEntry entry = mBubbleNotificationRow.getEntry(); + StatusBarNotification sbn = entry.getSbn(); // Given sbn.getNotification().contentIntent = null; @@ -332,7 +335,7 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { when(mCentralSurfaces.isOccluded()).thenReturn(true); // When - mNotificationActivityStarter.onNotificationClicked(sbn, mBubbleNotificationRow); + mNotificationActivityStarter.onNotificationClicked(entry, mBubbleNotificationRow); // Then verify(mBubblesManager).expandStackAndSelectBubble(eq(mBubbleNotificationRow.getEntry())); @@ -342,7 +345,7 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { verify(mAssistManager).hideAssist(); verify(mClickNotifier).onNotificationClick( - eq(sbn.getKey()), any(NotificationVisibility.class)); + eq(entry.getKey()), any(NotificationVisibility.class)); // The content intent should NOT be sent on click. verifyZeroInteractions(mContentIntent); @@ -354,7 +357,8 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { @Test public void testOnNotificationClicked_bubble_withContentIntent_keyGuardShowing() throws RemoteException { - StatusBarNotification sbn = mBubbleNotificationRow.getEntry().getSbn(); + NotificationEntry entry = mBubbleNotificationRow.getEntry(); + StatusBarNotification sbn = entry.getSbn(); // Given sbn.getNotification().contentIntent = mContentIntent; @@ -362,7 +366,7 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { when(mCentralSurfaces.isOccluded()).thenReturn(true); // When - mNotificationActivityStarter.onNotificationClicked(sbn, mBubbleNotificationRow); + mNotificationActivityStarter.onNotificationClicked(entry, mBubbleNotificationRow); // Then verify(mBubblesManager).expandStackAndSelectBubble(mBubbleNotificationRow.getEntry()); @@ -372,7 +376,7 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { verify(mAssistManager).hideAssist(); verify(mClickNotifier).onNotificationClick( - eq(sbn.getKey()), any(NotificationVisibility.class)); + eq(entry.getKey()), any(NotificationVisibility.class)); // The content intent should NOT be sent on click. verify(mContentIntent).getIntent(); @@ -408,16 +412,17 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { @Test public void testNotifActivityStarterEventSourceStartEvent_onNotificationClicked() { + final NotificationEntry entry = mNotificationRow.getEntry(); NotifActivityLaunchEvents.Listener listener = mock(NotifActivityLaunchEvents.Listener.class); mLaunchEventsEmitter.registerListener(listener); - mNotificationActivityStarter - .onNotificationClicked(mNotificationRow.getEntry().getSbn(), mNotificationRow); - verify(listener).onStartLaunchNotifActivity(mNotificationRow.getEntry()); + mNotificationActivityStarter.onNotificationClicked(entry, mNotificationRow); + verify(listener).onStartLaunchNotifActivity(entry); } @Test public void testNotifActivityStarterEventSourceFinishEvent_dismissKeyguardCancelled() { + final NotificationEntry entry = mNotificationRow.getEntry(); NotifActivityLaunchEvents.Listener listener = mock(NotifActivityLaunchEvents.Listener.class); mLaunchEventsEmitter.registerListener(listener); @@ -428,34 +433,36 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { .when(mActivityStarter) .dismissKeyguardThenExecute(any(OnDismissAction.class), any(), anyBoolean()); mNotificationActivityStarter - .onNotificationClicked(mNotificationRow.getEntry().getSbn(), mNotificationRow); - verify(listener).onFinishLaunchNotifActivity(mNotificationRow.getEntry()); + .onNotificationClicked(entry, mNotificationRow); + verify(listener).onFinishLaunchNotifActivity(entry); } @Test public void testNotifActivityStarterEventSourceFinishEvent_postPanelCollapse() throws Exception { + final NotificationEntry entry = mNotificationRow.getEntry(); NotifActivityLaunchEvents.Listener listener = mock(NotifActivityLaunchEvents.Listener.class); mLaunchEventsEmitter.registerListener(listener); mNotificationActivityStarter - .onNotificationClicked(mNotificationRow.getEntry().getSbn(), mNotificationRow); + .onNotificationClicked(entry, mNotificationRow); ArgumentCaptor controllerCaptor = ArgumentCaptor.forClass(ActivityLaunchAnimator.Controller.class); verify(mActivityLaunchAnimator).startPendingIntentWithAnimation( controllerCaptor.capture(), anyBoolean(), any(), any()); controllerCaptor.getValue().onIntentStarted(false); - verify(listener).onFinishLaunchNotifActivity(mNotificationRow.getEntry()); + verify(listener).onFinishLaunchNotifActivity(entry); } @Test public void testNotifActivityStarterEventSourceFinishEvent_postPanelCollapse_noAnimate() { + final NotificationEntry entry = mNotificationRow.getEntry(); NotifActivityLaunchEvents.Listener listener = mock(NotifActivityLaunchEvents.Listener.class); mLaunchEventsEmitter.registerListener(listener); when(mCentralSurfaces.shouldAnimateLaunch(anyBoolean())).thenReturn(false); mNotificationActivityStarter - .onNotificationClicked(mNotificationRow.getEntry().getSbn(), mNotificationRow); - verify(listener).onFinishLaunchNotifActivity(mNotificationRow.getEntry()); + .onNotificationClicked(entry, mNotificationRow); + verify(listener).onFinishLaunchNotifActivity(entry); } } From 8b63613104f90500f54d1a8829766e4547d1697f Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Mon, 9 May 2022 19:37:23 +0000 Subject: [PATCH 2/4] Fix events so that VisualStabilityCoordinator blocks shade recalculation during activity launch animation Bug: 230540148 Bug: 227254780 Test: atest VisualStabilityCoordinatorTest Change-Id: Iacb95f8be09acf31faea7f71c4580dbb9fdc0825 --- .../statusbar/phone/NotificationPanelViewController.java | 4 ++-- .../statusbar/phone/StatusBarLaunchAnimatorController.kt | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelViewController.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelViewController.java index f15ea627af479..314ab2221b198 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelViewController.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotificationPanelViewController.java @@ -3377,9 +3377,9 @@ public class NotificationPanelViewController extends PanelViewController { @Override public void setIsLaunchAnimationRunning(boolean running) { - boolean wasRunning = isLaunchTransitionRunning(); + boolean wasRunning = mIsLaunchAnimationRunning; super.setIsLaunchAnimationRunning(running); - if (wasRunning != isLaunchTransitionRunning()) { + if (wasRunning != mIsLaunchAnimationRunning) { mPanelEventsEmitter.notifyLaunchingActivityChanged(running); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarLaunchAnimatorController.kt b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarLaunchAnimatorController.kt index 56b6dfc42ee35..c0922163903fe 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarLaunchAnimatorController.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarLaunchAnimatorController.kt @@ -20,7 +20,9 @@ class StatusBarLaunchAnimatorController( override fun onIntentStarted(willAnimate: Boolean) { delegate.onIntentStarted(willAnimate) - if (!willAnimate) { + if (willAnimate) { + centralSurfaces.notificationPanelViewController.setIsLaunchAnimationRunning(true) + } else { centralSurfaces.collapsePanelOnMainThread() } } @@ -51,6 +53,7 @@ class StatusBarLaunchAnimatorController( override fun onLaunchAnimationCancelled() { delegate.onLaunchAnimationCancelled() + centralSurfaces.notificationPanelViewController.setIsLaunchAnimationRunning(false) centralSurfaces.onLaunchAnimationCancelled(isLaunchForActivity) } } \ No newline at end of file From 4a206a416582e9242060176b4e3f8103e6b43473 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Wed, 11 May 2022 19:54:53 +0000 Subject: [PATCH 3/4] Fix NotificationEntry mismatch crashes when tapping notification. The root of these problems is holding onto a NotificationEntry instance and performing a dismiss or end to lifetime extension after a delay, during which apps or the system may cancel and re-post the given notification. The original fix to this by lifetime extending these notifications worked in the case where this closure around a NotificationEntry perfectly matched lifetime extension, but this was becoming a whack-a-mole of finding all the ways we might need to extend these notifications. Moreover, there was a bug in the implementation that used notification keys as the keys for the lifetime extension map, rather than NotificationEntry instances themselves, which caused b/227254780. This CL introduces the concept of reporting future dismissals to the NotifCollection (via OnUserInteractionCallback) which allows the NotifCollection to respond to system server's notification removal events by preventing the eventual dismissal call from crashing. This design is not the ideal end-game (see b/232260346) but rather preserves the existing call order into NotifCollection while also bailing out before any calls are made that would cause a crash. Test: atest ExpandableNotificationRowTest StatusBarNotificationActivityStarterTest NotifCollectionTest Fixes: 230540148 Fixes: 227254780 Bug: 232260346 Change-Id: Ia66ae5ade3fbfdd0436d54dcbeef720618622716 --- .../collection/NotifCollection.java | 157 +++++++++++++++++- .../coordinator/NotifCoordinators.kt | 2 +- .../OnUserInteractionCallbackImpl.java | 59 ++----- .../OnUserInteractionCallbackImplLegacy.java | 19 ++- .../notifcollection/NotifCollectionLogger.kt | 81 +++++++++ .../dagger/NotificationsModule.java | 3 +- .../row/ExpandableNotificationRow.java | 8 +- .../row/OnUserInteractionCallback.java | 33 ++-- .../StatusBarNotificationActivityStarter.java | 39 ++--- .../collection/NotifCollectionTest.java | 71 +++++++- .../row/ExpandableNotificationRowTest.java | 4 +- .../row/NotificationTestHelper.java | 7 + ...tusBarNotificationActivityStarterTest.java | 28 +++- 13 files changed, 381 insertions(+), 130 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java index bcd8e594ffbdf..6085096ee124e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifCollection.java @@ -18,9 +18,12 @@ package com.android.systemui.statusbar.notification.collection; import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL; import static android.service.notification.NotificationListenerService.REASON_APP_CANCEL_ALL; +import static android.service.notification.NotificationListenerService.REASON_ASSISTANT_CANCEL; import static android.service.notification.NotificationListenerService.REASON_CANCEL; import static android.service.notification.NotificationListenerService.REASON_CANCEL_ALL; import static android.service.notification.NotificationListenerService.REASON_CHANNEL_BANNED; +import static android.service.notification.NotificationListenerService.REASON_CHANNEL_REMOVED; +import static android.service.notification.NotificationListenerService.REASON_CLEAR_DATA; import static android.service.notification.NotificationListenerService.REASON_CLICK; import static android.service.notification.NotificationListenerService.REASON_ERROR; import static android.service.notification.NotificationListenerService.REASON_GROUP_OPTIMIZATION; @@ -36,9 +39,11 @@ import static android.service.notification.NotificationListenerService.REASON_TI import static android.service.notification.NotificationListenerService.REASON_UNAUTOBUNDLED; import static android.service.notification.NotificationListenerService.REASON_USER_STOPPED; +import static com.android.systemui.statusbar.notification.NotificationUtils.logKey; import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.DISMISSED; import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.NOT_DISMISSED; import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.PARENT_DISMISSED; +import static com.android.systemui.statusbar.notification.collection.notifcollection.NotifCollectionLoggerKt.cancellationReasonDebugString; import static java.util.Objects.requireNonNull; @@ -99,6 +104,7 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -143,6 +149,7 @@ public class NotifCollection implements Dumpable { private final Map mNotificationSet = new ArrayMap<>(); private final Collection mReadOnlyNotificationSet = Collections.unmodifiableCollection(mNotificationSet.values()); + private final HashMap mFutureDismissals = new HashMap<>(); @Nullable private CollectionReadyForBuildListener mBuildListener; private final List mNotifCollectionListeners = new ArrayList<>(); @@ -511,6 +518,7 @@ public class NotifCollection implements Dumpable { cancelDismissInterception(entry); mEventQueue.add(new EntryRemovedEvent(entry, entry.mCancellationReason)); mEventQueue.add(new CleanUpEntryEvent(entry)); + handleFutureDismissal(entry); return true; } else { return false; @@ -519,31 +527,32 @@ public class NotifCollection implements Dumpable { /** * Get the group summary entry - * @param group + * @param groupKey * @return */ @Nullable - public NotificationEntry getGroupSummary(String group) { + public NotificationEntry getGroupSummary(String groupKey) { return mNotificationSet .values() .stream() - .filter(it -> Objects.equals(it.getSbn().getGroup(), group)) + .filter(it -> Objects.equals(it.getSbn().getGroupKey(), groupKey)) .filter(it -> it.getSbn().getNotification().isGroupSummary()) .findFirst().orElse(null); } /** - * Checks if the entry is the only child in the logical group - * @param entry - * @return + * Checks if the entry is the only child in the logical group; + * it need not have a summary to qualify + * + * @param entry the entry to check */ public boolean isOnlyChildInGroup(NotificationEntry entry) { - String group = entry.getSbn().getGroup(); + String groupKey = entry.getSbn().getGroupKey(); return mNotificationSet.get(entry.getKey()) == entry && mNotificationSet .values() .stream() - .filter(it -> Objects.equals(it.getSbn().getGroup(), group)) + .filter(it -> Objects.equals(it.getSbn().getGroupKey(), groupKey)) .filter(it -> !it.getSbn().getNotification().isGroupSummary()) .count() == 1; } @@ -916,10 +925,139 @@ public class NotifCollection implements Dumpable { dispatchEventsAndRebuildList(); } + /** + * A method to alert the collection that an async operation is happening, at the end of which a + * dismissal request will be made. This method has the additional guarantee that if a parent + * notification exists for a single child, then that notification will also be dismissed. + * + * The runnable returned must be run at the end of the async operation to enact the cancellation + * + * @param entry the notification we want to dismiss + * @param cancellationReason the reason for the cancellation + * @param statsCreator the callback for generating the stats for an entry + * @return the runnable to be run when the dismissal is ready to happen + */ + public Runnable registerFutureDismissal(NotificationEntry entry, int cancellationReason, + DismissedByUserStatsCreator statsCreator) { + FutureDismissal dismissal = mFutureDismissals.get(entry.getKey()); + if (dismissal != null) { + mLogger.logFutureDismissalReused(dismissal); + return dismissal; + } + dismissal = new FutureDismissal(entry, cancellationReason, statsCreator); + mFutureDismissals.put(entry.getKey(), dismissal); + mLogger.logFutureDismissalRegistered(dismissal); + return dismissal; + } + + private void handleFutureDismissal(NotificationEntry entry) { + final FutureDismissal futureDismissal = mFutureDismissals.remove(entry.getKey()); + if (futureDismissal != null) { + futureDismissal.onSystemServerCancel(entry.mCancellationReason); + } + } + + /** A single method interface that callers can pass in when registering future dismissals */ + public interface DismissedByUserStatsCreator { + DismissedByUserStats createDismissedByUserStats(NotificationEntry entry); + } + + /** A class which tracks the double dismissal events coming in from both the system server and + * the ui */ + public class FutureDismissal implements Runnable { + private final NotificationEntry mEntry; + private final DismissedByUserStatsCreator mStatsCreator; + @Nullable + private final NotificationEntry mSummaryToDismiss; + private final String mLabel; + + private boolean mDidRun; + private boolean mDidSystemServerCancel; + + private FutureDismissal(NotificationEntry entry, @CancellationReason int cancellationReason, + DismissedByUserStatsCreator statsCreator) { + mEntry = entry; + mStatsCreator = statsCreator; + mSummaryToDismiss = fetchSummaryToDismiss(entry); + mLabel = ""; + } + + @Nullable + private NotificationEntry fetchSummaryToDismiss(NotificationEntry entry) { + if (isOnlyChildInGroup(entry)) { + String group = entry.getSbn().getGroupKey(); + NotificationEntry summary = getGroupSummary(group); + if (summary != null && summary.isDismissable()) return summary; + } + return null; + } + + /** called when the entry has been removed from the collection */ + public void onSystemServerCancel(@CancellationReason int cancellationReason) { + Assert.isMainThread(); + if (mDidSystemServerCancel) { + mLogger.logFutureDismissalDoubleCancelledByServer(this); + return; + } + mLogger.logFutureDismissalGotSystemServerCancel(this, cancellationReason); + mDidSystemServerCancel = true; + // TODO: Internally dismiss the summary now instead of waiting for onUiCancel + } + + private void onUiCancel() { + mFutureDismissals.remove(mEntry.getKey()); + final NotificationEntry currentEntry = getEntry(mEntry.getKey()); + // generate stats for the entry before dismissing summary, which could affect state + final DismissedByUserStats stats = mStatsCreator.createDismissedByUserStats(mEntry); + // dismiss the summary (if it exists) + if (mSummaryToDismiss != null) { + final NotificationEntry currentSummary = getEntry(mSummaryToDismiss.getKey()); + if (currentSummary == mSummaryToDismiss) { + mLogger.logFutureDismissalDismissing(this, "summary"); + dismissNotification(mSummaryToDismiss, + mStatsCreator.createDismissedByUserStats(mSummaryToDismiss)); + } else { + mLogger.logFutureDismissalMismatchedEntry(this, "summary", currentSummary); + } + } + // dismiss this entry (if it is still around) + if (mDidSystemServerCancel) { + mLogger.logFutureDismissalAlreadyCancelledByServer(this); + } else if (currentEntry == mEntry) { + mLogger.logFutureDismissalDismissing(this, "entry"); + dismissNotification(mEntry, stats); + } else { + mLogger.logFutureDismissalMismatchedEntry(this, "entry", currentEntry); + } + } + + /** called when the dismissal should be completed */ + @Override + public void run() { + Assert.isMainThread(); + if (mDidRun) { + mLogger.logFutureDismissalDoubleRun(this); + return; + } + mDidRun = true; + onUiCancel(); + } + + /** provides a debug label for this instance */ + public String getLabel() { + return mLabel; + } + } + @IntDef(prefix = { "REASON_" }, value = { REASON_NOT_CANCELED, REASON_UNKNOWN, REASON_CLICK, + REASON_CANCEL, REASON_CANCEL_ALL, REASON_ERROR, REASON_PACKAGE_CHANGED, @@ -937,6 +1075,9 @@ public class NotifCollection implements Dumpable { REASON_CHANNEL_BANNED, REASON_SNOOZED, REASON_TIMEOUT, + REASON_CHANNEL_REMOVED, + REASON_CLEAR_DATA, + REASON_ASSISTANT_CANCEL, }) @Retention(RetentionPolicy.SOURCE) public @interface CancellationReason {} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt index b24d2922adfbb..acb26a930689b 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt @@ -93,7 +93,7 @@ class NotifCoordinatorsImpl @Inject constructor( mCoordinators.add(shadeEventCoordinator) mCoordinators.add(viewConfigCoordinator) mCoordinators.add(visualStabilityCoordinator) - mCoordinators.add(activityLaunchAnimCoordinator) +// mCoordinators.add(activityLaunchAnimCoordinator) // NOTE: will delete in followup CL if (notifPipelineFlags.isSmartspaceDedupingEnabled()) { mCoordinators.add(smartspaceDedupingCoordinator) } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java index 3bd91b5c84806..7dd3672a6e306 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/inflation/OnUserInteractionCallbackImpl.java @@ -18,17 +18,17 @@ package com.android.systemui.statusbar.notification.collection.inflation; import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL; -import android.annotation.Nullable; import android.os.SystemClock; -import android.service.notification.NotificationListenerService; import android.service.notification.NotificationStats; +import androidx.annotation.NonNull; + import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.statusbar.notification.collection.NotifCollection; +import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.coordinator.VisualStabilityCoordinator; import com.android.systemui.statusbar.notification.collection.notifcollection.DismissedByUserStats; -import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager; import com.android.systemui.statusbar.notification.collection.render.NotificationVisibilityProvider; import com.android.systemui.statusbar.notification.row.OnUserInteractionCallback; import com.android.systemui.statusbar.policy.HeadsUpManager; @@ -43,54 +43,33 @@ public class OnUserInteractionCallbackImpl implements OnUserInteractionCallback private final HeadsUpManager mHeadsUpManager; private final StatusBarStateController mStatusBarStateController; private final VisualStabilityCoordinator mVisualStabilityCoordinator; - private final GroupMembershipManager mGroupMembershipManager; public OnUserInteractionCallbackImpl( NotificationVisibilityProvider visibilityProvider, NotifCollection notifCollection, HeadsUpManager headsUpManager, StatusBarStateController statusBarStateController, - VisualStabilityCoordinator visualStabilityCoordinator, - GroupMembershipManager groupMembershipManager + VisualStabilityCoordinator visualStabilityCoordinator ) { mVisibilityProvider = visibilityProvider; mNotifCollection = notifCollection; mHeadsUpManager = headsUpManager; mStatusBarStateController = statusBarStateController; mVisualStabilityCoordinator = visualStabilityCoordinator; - mGroupMembershipManager = groupMembershipManager; } - /** - * Callback triggered when a user: - * 1. Manually dismisses a notification {@see ExpandableNotificationRow}. - * 2. Clicks on a notification with flag {@link android.app.Notification#FLAG_AUTO_CANCEL}. - * {@see StatusBarNotificationActivityStarter} - */ - @Override - public void onDismiss( - NotificationEntry entry, - @NotificationListenerService.NotificationCancelReason int cancellationReason, - @Nullable NotificationEntry groupSummaryToDismiss - ) { + @NonNull + private DismissedByUserStats getDismissedByUserStats(NotificationEntry entry) { int dismissalSurface = NotificationStats.DISMISSAL_SHADE; if (mHeadsUpManager.isAlerting(entry.getKey())) { dismissalSurface = NotificationStats.DISMISSAL_PEEK; } else if (mStatusBarStateController.isDozing()) { dismissalSurface = NotificationStats.DISMISSAL_AOD; } - - if (groupSummaryToDismiss != null) { - onDismiss(groupSummaryToDismiss, cancellationReason, null); - } - - mNotifCollection.dismissNotification( - entry, - new DismissedByUserStats( - dismissalSurface, - DISMISS_SENTIMENT_NEUTRAL, - mVisibilityProvider.obtain(entry, true)) - ); + return new DismissedByUserStats( + dismissalSurface, + DISMISS_SENTIMENT_NEUTRAL, + mVisibilityProvider.obtain(entry, true)); } @Override @@ -100,19 +79,11 @@ public class OnUserInteractionCallbackImpl implements OnUserInteractionCallback SystemClock.uptimeMillis()); } - /** - * @param entry that is being dismissed - * @return the group summary to dismiss along with this entry if this is the last entry in - * the group. Else, returns null. - */ + @NonNull @Override - @Nullable - public NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) { - String group = entry.getSbn().getGroup(); - if (mNotifCollection.isOnlyChildInGroup(entry)) { - NotificationEntry summary = mNotifCollection.getGroupSummary(group); - if (summary != null && summary.isDismissable()) return summary; - } - return null; + public Runnable registerFutureDismissal(@NonNull NotificationEntry entry, + @CancellationReason int cancellationReason) { + return mNotifCollection.registerFutureDismissal( + entry, cancellationReason, this::getDismissedByUserStats); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java index 8daf8be0cc8ff..103b14b09e9c0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/legacy/OnUserInteractionCallbackImplLegacy.java @@ -18,12 +18,15 @@ package com.android.systemui.statusbar.notification.collection.legacy; import static android.service.notification.NotificationStats.DISMISS_SENTIMENT_NEUTRAL; -import android.annotation.Nullable; import android.service.notification.NotificationListenerService; import android.service.notification.NotificationStats; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; + import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.statusbar.notification.NotificationEntryManager; +import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason; import com.android.systemui.statusbar.notification.collection.NotificationEntry; import com.android.systemui.statusbar.notification.collection.notifcollection.DismissedByUserStats; import com.android.systemui.statusbar.notification.collection.render.GroupMembershipManager; @@ -68,8 +71,7 @@ public class OnUserInteractionCallbackImplLegacy implements OnUserInteractionCal * along with this dismissal. If null, does not additionally * dismiss any notifications. */ - @Override - public void onDismiss( + private void onDismiss( NotificationEntry entry, @NotificationListenerService.NotificationCancelReason int cancellationReason, @Nullable NotificationEntry groupSummaryToDismiss @@ -106,14 +108,21 @@ public class OnUserInteractionCallbackImplLegacy implements OnUserInteractionCal * @return the group summary to dismiss along with this entry if this is the last entry in * the group. Else, returns null. */ - @Override @Nullable - public NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) { + private NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry) { if (mGroupMembershipManager.isOnlyChildInGroup(entry)) { NotificationEntry groupSummary = mGroupMembershipManager.getLogicalGroupSummary(entry); return groupSummary.isDismissable() ? groupSummary : null; } return null; } + + @Override + @NonNull + public Runnable registerFutureDismissal(@NonNull NotificationEntry entry, + @CancellationReason int cancellationReason) { + NotificationEntry groupSummaryToDismiss = getGroupSummaryToDismiss(entry); + return () -> onDismiss(entry, cancellationReason, groupSummaryToDismiss); + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt index 7302de57e9551..7e7936717b84e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/notifcollection/NotifCollectionLogger.kt @@ -28,7 +28,9 @@ import com.android.systemui.log.LogLevel.WTF import com.android.systemui.log.dagger.NotificationLog import com.android.systemui.statusbar.notification.collection.NotifCollection import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason +import com.android.systemui.statusbar.notification.collection.NotifCollection.FutureDismissal import com.android.systemui.statusbar.notification.collection.NotificationEntry +import com.android.systemui.statusbar.notification.logKey import javax.inject.Inject fun cancellationReasonDebugString(@CancellationReason reason: Int) = @@ -36,6 +38,7 @@ fun cancellationReasonDebugString(@CancellationReason reason: Int) = -1 -> "REASON_NOT_CANCELED" // NotifCollection.REASON_NOT_CANCELED NotifCollection.REASON_UNKNOWN -> "REASON_UNKNOWN" NotificationListenerService.REASON_CLICK -> "REASON_CLICK" + NotificationListenerService.REASON_CANCEL -> "REASON_CANCEL" NotificationListenerService.REASON_CANCEL_ALL -> "REASON_CANCEL_ALL" NotificationListenerService.REASON_ERROR -> "REASON_ERROR" NotificationListenerService.REASON_PACKAGE_CHANGED -> "REASON_PACKAGE_CHANGED" @@ -53,6 +56,9 @@ fun cancellationReasonDebugString(@CancellationReason reason: Int) = NotificationListenerService.REASON_CHANNEL_BANNED -> "REASON_CHANNEL_BANNED" NotificationListenerService.REASON_SNOOZED -> "REASON_SNOOZED" NotificationListenerService.REASON_TIMEOUT -> "REASON_TIMEOUT" + NotificationListenerService.REASON_CHANNEL_REMOVED -> "REASON_CHANNEL_REMOVED" + NotificationListenerService.REASON_CLEAR_DATA -> "REASON_CLEAR_DATA" + NotificationListenerService.REASON_ASSISTANT_CANCEL -> "REASON_ASSISTANT_CANCEL" else -> "unknown" } @@ -241,6 +247,81 @@ class NotifCollectionLogger @Inject constructor( "ERROR suppressed due to initialization forgiveness: $str1" }) } + + fun logFutureDismissalReused(dismissal: FutureDismissal) { + buffer.log(TAG, INFO, { + str1 = dismissal.label + }, { + "Reusing existing registration: $str1" + }) + } + + fun logFutureDismissalRegistered(dismissal: FutureDismissal) { + buffer.log(TAG, DEBUG, { + str1 = dismissal.label + }, { + "Registered: $str1" + }) + } + + fun logFutureDismissalDoubleCancelledByServer(dismissal: FutureDismissal) { + buffer.log(TAG, WARNING, { + str1 = dismissal.label + }, { + "System server double cancelled: $str1" + }) + } + + fun logFutureDismissalDoubleRun(dismissal: FutureDismissal) { + buffer.log(TAG, WARNING, { + str1 = dismissal.label + }, { + "Double run: $str1" + }) + } + + fun logFutureDismissalAlreadyCancelledByServer(dismissal: FutureDismissal) { + buffer.log(TAG, DEBUG, { + str1 = dismissal.label + }, { + "Ignoring: entry already cancelled by server: $str1" + }) + } + + fun logFutureDismissalGotSystemServerCancel( + dismissal: FutureDismissal, + @CancellationReason cancellationReason: Int + ) { + buffer.log(TAG, DEBUG, { + str1 = dismissal.label + int1 = cancellationReason + }, { + "SystemServer cancelled: $str1 reason=${cancellationReasonDebugString(int1)}" + }) + } + + fun logFutureDismissalDismissing(dismissal: FutureDismissal, type: String) { + buffer.log(TAG, DEBUG, { + str1 = dismissal.label + str2 = type + }, { + "Dismissing $str2 for: $str1" + }) + } + + fun logFutureDismissalMismatchedEntry( + dismissal: FutureDismissal, + type: String, + latestEntry: NotificationEntry? + ) { + buffer.log(TAG, WARNING, { + str1 = dismissal.label + str2 = type + str3 = latestEntry.logKey + }, { + "Mismatch: current $str2 is $str3 for: $str1" + }) + } } private const val TAG = "NotifCollection" diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java index d96590a825479..94848e83a3608 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java @@ -350,8 +350,7 @@ public interface NotificationsModule { notifCollection.get(), headsUpManager, statusBarStateController, - visualStabilityCoordinator.get(), - groupMembershipManagerLazy.get()) + visualStabilityCoordinator.get()) : new OnUserInteractionCallbackImplLegacy( entryManager, visibilityProvider.get(), diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java index ed69e0609096e..af9d751ae545e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRow.java @@ -17,7 +17,6 @@ package com.android.systemui.statusbar.notification.row; import static android.app.Notification.Action.SEMANTIC_ACTION_MARK_CONVERSATION_AS_PRIORITY; -import static android.os.UserHandle.USER_SYSTEM; import static android.service.notification.NotificationListenerService.REASON_CANCEL; import static com.android.systemui.statusbar.notification.row.NotificationContentView.VISIBLE_TYPE_HEADSUP; @@ -34,8 +33,6 @@ import android.app.Notification; import android.app.NotificationChannel; import android.app.role.RoleManager; import android.content.Context; -import android.content.pm.PackageInfo; -import android.content.pm.PackageManager; import android.content.res.Configuration; import android.content.res.Resources; import android.graphics.Canvas; @@ -51,7 +48,6 @@ import android.os.Bundle; import android.os.RemoteException; import android.os.ServiceManager; import android.os.Trace; -import android.provider.Settings; import android.service.notification.StatusBarNotification; import android.util.ArraySet; import android.util.AttributeSet; @@ -112,7 +108,6 @@ import com.android.systemui.statusbar.notification.stack.ExpandableViewState; import com.android.systemui.statusbar.notification.stack.NotificationChildrenContainer; import com.android.systemui.statusbar.notification.stack.NotificationStackScrollLayout; import com.android.systemui.statusbar.notification.stack.SwipeableView; -import com.android.systemui.statusbar.phone.CentralSurfaces; import com.android.systemui.statusbar.phone.KeyguardBypassController; import com.android.systemui.statusbar.policy.HeadsUpManager; import com.android.systemui.statusbar.policy.InflatedSmartReplyState; @@ -1455,8 +1450,7 @@ public class ExpandableNotificationRow extends ActivatableNotificationView dismiss(fromAccessibility); if (mEntry.isDismissable()) { if (mOnUserInteractionCallback != null) { - mOnUserInteractionCallback.onDismiss(mEntry, REASON_CANCEL, - mOnUserInteractionCallback.getGroupSummaryToDismiss(mEntry)); + mOnUserInteractionCallback.registerFutureDismissal(mEntry, REASON_CANCEL).run(); } } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java index 94c5507cfbf42..98d43539841df 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/OnUserInteractionCallback.java @@ -16,8 +16,9 @@ package com.android.systemui.statusbar.notification.row; -import android.service.notification.NotificationListenerService; +import androidx.annotation.NonNull; +import com.android.systemui.statusbar.notification.collection.NotifCollection.CancellationReason; import com.android.systemui.statusbar.notification.collection.NotificationEntry; /** @@ -25,30 +26,24 @@ import com.android.systemui.statusbar.notification.collection.NotificationEntry; */ public interface OnUserInteractionCallback { - /** - * Handle a user interaction that triggers a notification dismissal. Called when a user clicks - * on an auto-cancelled notification or manually swipes to dismiss the notification. - * - * @param entry notification being dismissed - * @param cancellationReason reason for the cancellation - * @param groupSummaryToDismiss group summary to dismiss with `entry`. - */ - void onDismiss( - NotificationEntry entry, - @NotificationListenerService.NotificationCancelReason int cancellationReason, - NotificationEntry groupSummaryToDismiss); - /** * Triggered after a user has changed the importance of the notification via its * {@link NotificationGuts}. */ void onImportanceChanged(NotificationEntry entry); - /** - * @param entry being dismissed by the user - * @return group summary that should be dismissed along with `entry`. Can be null if no - * relevant group summary exists or the group summary should not be dismissed with `entry`. + * Called once it is known that a dismissal will take place for the given reason. + * This returns a Runnable which MUST be invoked when the dismissal is ready to be completed. + * + * Registering for future dismissal is typically done before notifying the NMS that a + * notification was clicked or dismissed, but the local dismissal may happen later. + * + * @param entry the entry being cancelled + * @param cancellationReason the reason for the cancellation + * @return the runnable to call when the dismissal can happen */ - NotificationEntry getGroupSummaryToDismiss(NotificationEntry entry); + @NonNull + Runnable registerFutureDismissal(@NonNull NotificationEntry entry, + @CancellationReason int cancellationReason); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java index af4c81b4181d8..eee7b2a41de49 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java @@ -380,22 +380,15 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte final NotificationVisibility nv = mVisibilityProvider.obtain(entry, true); - // retrieve the group summary to remove with this entry before we tell NMS the - // notification was clicked to avoid a race condition - final boolean shouldAutoCancel = shouldAutoCancel(entry.getSbn()); - final NotificationEntry summaryToRemove = shouldAutoCancel - ? mOnUserInteractionCallback.getGroupSummaryToDismiss(entry) : null; - - // inform NMS that the notification was clicked - mClickNotifier.onNotificationClick(notificationKey, nv); - - if (!canBubble && (shouldAutoCancel + if (!canBubble && (shouldAutoCancel(entry.getSbn()) || mRemoteInputManager.isNotificationKeptForRemoteInputHistory(notificationKey))) { + final Runnable locallyDismissNotification = + mOnUserInteractionCallback.registerFutureDismissal(entry, REASON_CLICK); // Immediately remove notification from visually showing. // We have to post the removal to the UI thread for synchronization. mMainThreadHandler.post(() -> { final Runnable removeNotification = () -> { - mOnUserInteractionCallback.onDismiss(entry, REASON_CLICK, summaryToRemove); + locallyDismissNotification.run(); if (!animate) { // If we're animating, this would be invoked after the activity launch // animation completes. Since we're not animating, the launch already @@ -417,6 +410,9 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte () -> mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry)); } + // inform NMS that the notification was clicked + mClickNotifier.onNotificationClick(notificationKey, nv); + mIsCollapsingToShowActivityOverLockscreen = false; } @@ -433,24 +429,14 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte // will focus follow operation only after drag-and-drop that notification. final NotificationVisibility nv = mVisibilityProvider.obtain(entry, true); - // retrieve the group summary to remove with this entry before we tell NMS the - // notification was clicked to avoid a race condition - final boolean shouldAutoCancel = shouldAutoCancel(entry.getSbn()); - final NotificationEntry summaryToRemove = shouldAutoCancel - ? mOnUserInteractionCallback.getGroupSummaryToDismiss(entry) : null; - String notificationKey = entry.getKey(); - // inform NMS that the notification was clicked - mClickNotifier.onNotificationClick(notificationKey, nv); - - if (shouldAutoCancel || mRemoteInputManager.isNotificationKeptForRemoteInputHistory( - notificationKey)) { + if (shouldAutoCancel(entry.getSbn()) + || mRemoteInputManager.isNotificationKeptForRemoteInputHistory(notificationKey)) { + final Runnable removeNotification = + mOnUserInteractionCallback.registerFutureDismissal(entry, REASON_CLICK); // Immediately remove notification from visually showing. // We have to post the removal to the UI thread for synchronization. mMainThreadHandler.post(() -> { - final Runnable removeNotification = () -> - mOnUserInteractionCallback.onDismiss( - entry, REASON_CLICK, summaryToRemove); if (mPresenter.isCollapsing()) { // To avoid lags we're only performing the remove // after the shade is collapsed @@ -461,6 +447,9 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte }); } + // inform NMS that the notification was clicked + mClickNotifier.onNotificationClick(notificationKey, nv); + mIsCollapsingToShowActivityOverLockscreen = false; } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java index 706800940fd12..958d54230f1c6 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifCollectionTest.java @@ -32,6 +32,8 @@ import static com.android.systemui.statusbar.notification.collection.Notificatio import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.NOT_DISMISSED; import static com.android.systemui.statusbar.notification.collection.NotificationEntry.DismissState.PARENT_DISMISSED; +import static com.google.common.truth.Truth.assertThat; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; @@ -47,6 +49,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import static java.util.Collections.singletonList; @@ -180,13 +183,14 @@ public class NotifCollectionTest extends SysuiTestCase { @Test public void testGetGroupSummary() { - assertEquals(null, mCollection.getGroupSummary("group")); - NotifEvent summary = mNoMan.postNotif( - buildNotif(TEST_PACKAGE, 0) - .setGroup(mContext, "group") - .setGroupSummary(mContext, true)); + final NotificationEntryBuilder entryBuilder = buildNotif(TEST_PACKAGE, 0) + .setGroup(mContext, "group") + .setGroupSummary(mContext, true); + final String groupKey = entryBuilder.build().getSbn().getGroupKey(); + assertEquals(null, mCollection.getGroupSummary(groupKey)); + NotifEvent summary = mNoMan.postNotif(entryBuilder); - final NotificationEntry entry = mCollection.getGroupSummary("group"); + final NotificationEntry entry = mCollection.getGroupSummary(groupKey); assertEquals(summary.key, entry.getKey()); assertEquals(summary.sbn, entry.getSbn()); assertEquals(summary.ranking, entry.getRanking()); @@ -194,9 +198,9 @@ public class NotifCollectionTest extends SysuiTestCase { @Test public void testIsOnlyChildInGroup() { - NotifEvent notif1 = mNoMan.postNotif( - buildNotif(TEST_PACKAGE, 1) - .setGroup(mContext, "group")); + final NotificationEntryBuilder entryBuilder = buildNotif(TEST_PACKAGE, 1) + .setGroup(mContext, "group"); + NotifEvent notif1 = mNoMan.postNotif(entryBuilder); final NotificationEntry entry = mCollection.getEntry(notif1.key); assertTrue(mCollection.isOnlyChildInGroup(entry)); @@ -1487,6 +1491,55 @@ public class NotifCollectionTest extends SysuiTestCase { verify(mCollectionListener, never()).onEntryUpdated(any(), anyBoolean()); } + @Test + public void testRegisterFutureDismissal() throws RemoteException { + // GIVEN a pipeline with one notification + NotifEvent notifEvent = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + NotificationEntry entry = requireNonNull(mCollection.getEntry(notifEvent.key)); + clearInvocations(mCollectionListener); + + // WHEN registering a future dismissal, nothing happens right away + final Runnable onDismiss = mCollection.registerFutureDismissal(entry, REASON_CLICK, + NotifCollectionTest::defaultStats); + verifyNoMoreInteractions(mCollectionListener); + + // WHEN finally dismissing + onDismiss.run(); + verify(mStatusBarService).onNotificationClear(any(), anyInt(), eq(notifEvent.key), + anyInt(), anyInt(), any()); + verifyNoMoreInteractions(mStatusBarService); + verifyNoMoreInteractions(mCollectionListener); + } + + @Test + public void testRegisterFutureDismissalWithRetractionAndRepost() { + // GIVEN a pipeline with one notification + NotifEvent notifEvent = mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + NotificationEntry entry = requireNonNull(mCollection.getEntry(notifEvent.key)); + clearInvocations(mCollectionListener); + + // WHEN registering a future dismissal, nothing happens right away + final Runnable onDismiss = mCollection.registerFutureDismissal(entry, REASON_CLICK, + NotifCollectionTest::defaultStats); + verifyNoMoreInteractions(mCollectionListener); + + // WHEN retracting the notification, and then reposting + mNoMan.retractNotif(notifEvent.sbn, REASON_CLICK); + mNoMan.postNotif(buildNotif(TEST_PACKAGE, 47, "myTag")); + clearInvocations(mCollectionListener); + + // KNOWING that the entry in the collection is different now + assertThat(mCollection.getEntry(notifEvent.key)).isNotSameInstanceAs(entry); + + // WHEN finally dismissing + onDismiss.run(); + + // VERIFY that nothing happens; the notification should not be removed + verifyNoMoreInteractions(mCollectionListener); + assertThat(mCollection.getEntry(notifEvent.key)).isNotNull(); + verifyNoMoreInteractions(mStatusBarService); + } + @Test public void testCannotDismissOngoingNotificationChildren() { // GIVEN an ongoing notification diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java index b3c34c1e4ff4e..f36ffe6fb198b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/ExpandableNotificationRowTest.java @@ -335,7 +335,7 @@ public class ExpandableNotificationRowTest extends SysuiTestCase { .build(); row.performDismiss(false); verify(mNotificationTestHelper.mOnUserInteractionCallback) - .onDismiss(any(), anyInt(), any()); + .registerFutureDismissal(any(), anyInt()); } @Test @@ -347,6 +347,6 @@ public class ExpandableNotificationRowTest extends SysuiTestCase { .build(); row.performDismiss(false); verify(mNotificationTestHelper.mOnUserInteractionCallback, never()) - .onDismiss(any(), anyInt(), any()); + .registerFutureDismissal(any(), anyInt()); } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java index d5ed37a570d25..7a8b329bec64d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java @@ -23,8 +23,11 @@ import static android.app.NotificationManager.IMPORTANCE_HIGH; import static com.android.systemui.statusbar.NotificationEntryHelper.modifyRanking; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import android.annotation.Nullable; import android.app.ActivityManager; @@ -121,6 +124,7 @@ public class NotificationTestHelper { private StatusBarStateController mStatusBarStateController; private final PeopleNotificationIdentifier mPeopleNotificationIdentifier; public final OnUserInteractionCallback mOnUserInteractionCallback; + public final Runnable mFutureDismissalRunnable; public NotificationTestHelper( Context context, @@ -182,6 +186,9 @@ public class NotificationTestHelper { mBindPipelineEntryListener = collectionListenerCaptor.getValue(); mPeopleNotificationIdentifier = mock(PeopleNotificationIdentifier.class); mOnUserInteractionCallback = mock(OnUserInteractionCallback.class); + mFutureDismissalRunnable = mock(Runnable.class); + when(mOnUserInteractionCallback.registerFutureDismissal(any(), anyInt())) + .thenReturn(mFutureDismissalRunnable); } /** diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java index f1a6eba129525..273b9a17ce1d4 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java @@ -93,6 +93,7 @@ import org.mockito.MockitoAnnotations; import org.mockito.stubbing.Answer; import java.util.ArrayList; +import java.util.List; import java.util.Optional; @SmallTest @@ -141,6 +142,8 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { @Mock private OnUserInteractionCallback mOnUserInteractionCallback; @Mock + private Runnable mFutureDismissalRunnable; + @Mock private StatusBarNotificationActivityStarter mNotificationActivityStarter; @Mock private ActivityLaunchAnimator mActivityLaunchAnimator; @@ -187,8 +190,8 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { when(mEntryManager.getVisibleNotifications()).thenReturn(mActiveNotifications); when(mStatusBarStateController.getState()).thenReturn(StatusBarState.SHADE); when(mNotifPipelineFlags.isNewPipelineEnabled()).thenReturn(false); - when(mOnUserInteractionCallback.getGroupSummaryToDismiss(mNotificationRow.getEntry())) - .thenReturn(null); + when(mOnUserInteractionCallback.registerFutureDismissal(eq(mNotificationRow.getEntry()), + anyInt())).thenReturn(mFutureDismissalRunnable); when(mVisibilityProvider.obtain(anyString(), anyBoolean())) .thenAnswer(invocation -> NotificationVisibility.obtain( invocation.getArgument(0), 0, 1, false)); @@ -264,6 +267,10 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { @Test public void testOnNotificationClicked_keyGuardShowing() throws PendingIntent.CanceledException, RemoteException { + // To get the order right, collect posted runnables and run them later + List runnables = new ArrayList<>(); + doAnswer(answerVoid(r -> runnables.add((Runnable) r))) + .when(mHandler).post(any(Runnable.class)); // Given NotificationEntry entry = mNotificationRow.getEntry(); Notification notification = entry.getSbn().getNotification(); @@ -275,6 +282,8 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { // When mNotificationActivityStarter.onNotificationClicked(entry, mNotificationRow); + // Run the collected runnables in fifo order, the way post() really does. + while (!runnables.isEmpty()) runnables.remove(0).run(); // Then verify(mShadeController, atLeastOnce()).collapsePanel(); @@ -284,12 +293,14 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { verify(mAssistManager).hideAssist(); - InOrder orderVerifier = Mockito.inOrder(mClickNotifier, mOnUserInteractionCallback); + InOrder orderVerifier = Mockito.inOrder(mClickNotifier, mOnUserInteractionCallback, + mFutureDismissalRunnable); + // Notification calls dismiss callback to remove notification due to FLAG_AUTO_CANCEL + orderVerifier.verify(mOnUserInteractionCallback) + .registerFutureDismissal(eq(entry), eq(REASON_CLICK)); orderVerifier.verify(mClickNotifier).onNotificationClick( eq(entry.getKey()), any(NotificationVisibility.class)); - // Notification calls dismiss callback to remove notification due to FLAG_AUTO_CANCEL - orderVerifier.verify(mOnUserInteractionCallback).onDismiss(entry, - REASON_CLICK, null); + orderVerifier.verify(mFutureDismissalRunnable).run(); } @Test @@ -319,8 +330,9 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { verifyZeroInteractions(mContentIntent); // Notification should not be cancelled. - verify(mOnUserInteractionCallback, never()).onDismiss(eq(mNotificationRow.getEntry()), - anyInt(), eq(null)); + verify(mOnUserInteractionCallback, never()) + .registerFutureDismissal(eq(mNotificationRow.getEntry()), anyInt()); + verify(mFutureDismissalRunnable, never()).run(); } @Test From 34b4ebd89a588219addbcc63b39d6f35e4352fda Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Thu, 12 May 2022 01:31:55 +0000 Subject: [PATCH 4/4] Remove the ActivityLaunchAnimCoordinator. This coordinator was recently added for the purpose of extending the lifetime of the notification entry while the launch animation played to prevent an entry instance mismatch crash when the app cancelled and reposted it on app launch. Unfortunately, this wasn't a scalable solution: 1) there was another launch scenario that wasn't covered by this animation, and 2) if the notification was auto-cancelled and reposted, this would not work because lifetime extenders are not allowed to extend the lifetime of user-dismissed notifications. That bug and these new crash cases were solved by alerting the NotifCollection when the animation starts, and then not dismissing the notification when it ends if it was already dismissed by the system server. That is implemented in Ia66ae5ade3fbfdd0436d54dcbeef720618622716, but makes this coordinator and the events it listened to unnecessary. Bug: 230540148 Bug: 227254780 Test: manually validate that crashes are still fixed Change-Id: I28f7226a30df7730dbae264bfd101397293a9a03 --- .../ActivityLaunchAnimCoordinator.kt | 95 ------------ .../coordinator/NotifCoordinators.kt | 2 - .../coordinator/dagger/CoordinatorsModule.kt | 2 - .../dagger/NotificationsModule.java | 2 - .../phone/NotifActivityLaunchEvents.kt | 39 ----- .../NotifActivityLaunchEventsModule.java | 30 ---- .../StatusBarNotificationActivityStarter.java | 67 +-------- .../ActivityLaunchAnimCoordinatorTest.kt | 138 ------------------ ...tusBarNotificationActivityStarterTest.java | 62 +------- 9 files changed, 5 insertions(+), 432 deletions(-) delete mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ActivityLaunchAnimCoordinator.kt delete mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/phone/NotifActivityLaunchEvents.kt delete mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/phone/NotifActivityLaunchEventsModule.java delete mode 100644 packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ActivityLaunchAnimCoordinatorTest.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ActivityLaunchAnimCoordinator.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ActivityLaunchAnimCoordinator.kt deleted file mode 100644 index b54163d29e800..0000000000000 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/ActivityLaunchAnimCoordinator.kt +++ /dev/null @@ -1,95 +0,0 @@ -/* - * Copyright (C) 2022 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.systemui.statusbar.notification.collection.coordinator - -import com.android.systemui.statusbar.notification.collection.NotifPipeline -import com.android.systemui.statusbar.notification.collection.NotificationEntry -import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorScope -import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender -import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender.OnEndLifetimeExtensionCallback -import com.android.systemui.statusbar.phone.NotifActivityLaunchEvents -import dagger.Binds -import dagger.Module -import javax.inject.Inject - -/** Extends the lifetime of notifications while their activity launch animation is playing. */ -interface ActivityLaunchAnimCoordinator : Coordinator - -/** Provides an [ActivityLaunchAnimCoordinator] to [CoordinatorScope]. */ -@Module(includes = [PrivateActivityStarterCoordinatorModule::class]) -object ActivityLaunchAnimCoordinatorModule - -@Module -private interface PrivateActivityStarterCoordinatorModule { - @Binds - fun bindCoordinator(impl: ActivityLaunchAnimCoordinatorImpl): ActivityLaunchAnimCoordinator -} - -/** - * Listens for [NotifActivityLaunchEvents], and then extends the lifetimes of any notifs while their - * launch animation is playing. - */ -@CoordinatorScope -private class ActivityLaunchAnimCoordinatorImpl @Inject constructor( - private val activityLaunchEvents: NotifActivityLaunchEvents -) : ActivityLaunchAnimCoordinator { - // Tracks notification launches, and whether or not their lifetimes are extended. - private val notifsLaunchingActivities = mutableMapOf() - - private var onEndLifetimeExtensionCallback: OnEndLifetimeExtensionCallback? = null - - override fun attach(pipeline: NotifPipeline) { - activityLaunchEvents.registerListener(activityStartEventListener) - pipeline.addNotificationLifetimeExtender(extender) - } - - private val activityStartEventListener = object : NotifActivityLaunchEvents.Listener { - override fun onStartLaunchNotifActivity(entry: NotificationEntry) { - notifsLaunchingActivities[entry.key] = false - } - - override fun onFinishLaunchNotifActivity(entry: NotificationEntry) { - if (notifsLaunchingActivities.remove(entry.key) == true) { - // If we were extending the lifetime of this notification, stop. - onEndLifetimeExtensionCallback?.onEndLifetimeExtension(extender, entry) - } - } - } - - private val extender = object : NotifLifetimeExtender { - override fun getName(): String = "ActivityStarterCoordinator" - - override fun setCallback(callback: OnEndLifetimeExtensionCallback) { - onEndLifetimeExtensionCallback = callback - } - - override fun maybeExtendLifetime(entry: NotificationEntry, reason: Int): Boolean { - if (entry.key in notifsLaunchingActivities) { - // Track that we're now extending this notif - notifsLaunchingActivities[entry.key] = true - return true - } - return false - } - - override fun cancelLifetimeExtension(entry: NotificationEntry) { - if (entry.key in notifsLaunchingActivities) { - notifsLaunchingActivities[entry.key] = false - } - } - } -} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt index acb26a930689b..0b3a0dc9dd580 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/NotifCoordinators.kt @@ -56,7 +56,6 @@ class NotifCoordinatorsImpl @Inject constructor( smartspaceDedupingCoordinator: SmartspaceDedupingCoordinator, viewConfigCoordinator: ViewConfigCoordinator, visualStabilityCoordinator: VisualStabilityCoordinator, - activityLaunchAnimCoordinator: ActivityLaunchAnimCoordinator ) : NotifCoordinators { private val mCoordinators: MutableList = ArrayList() @@ -93,7 +92,6 @@ class NotifCoordinatorsImpl @Inject constructor( mCoordinators.add(shadeEventCoordinator) mCoordinators.add(viewConfigCoordinator) mCoordinators.add(visualStabilityCoordinator) -// mCoordinators.add(activityLaunchAnimCoordinator) // NOTE: will delete in followup CL if (notifPipelineFlags.isSmartspaceDedupingEnabled()) { mCoordinators.add(smartspaceDedupingCoordinator) } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/dagger/CoordinatorsModule.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/dagger/CoordinatorsModule.kt index 8ecffcb7670a2..839cf0d7ef921 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/dagger/CoordinatorsModule.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/dagger/CoordinatorsModule.kt @@ -17,7 +17,6 @@ package com.android.systemui.statusbar.notification.collection.coordinator.dagger import com.android.systemui.dagger.SysUISingleton -import com.android.systemui.statusbar.notification.collection.coordinator.ActivityLaunchAnimCoordinatorModule import com.android.systemui.statusbar.notification.collection.coordinator.NotifCoordinators import com.android.systemui.statusbar.notification.collection.coordinator.NotifCoordinatorsImpl import dagger.Binds @@ -48,7 +47,6 @@ interface CoordinatorsSubcomponent { } @Module(includes = [ - ActivityLaunchAnimCoordinatorModule::class, ]) private abstract class InternalCoordinatorsModule { @Binds diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java index 94848e83a3608..c9c7fe9e0ab68 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java @@ -88,7 +88,6 @@ import com.android.systemui.statusbar.notification.stack.NotificationSectionsMan import com.android.systemui.statusbar.notification.stack.StackScrollAlgorithm; import com.android.systemui.statusbar.phone.CentralSurfaces; import com.android.systemui.statusbar.phone.KeyguardBypassController; -import com.android.systemui.statusbar.phone.NotifActivityLaunchEventsModule; import com.android.systemui.statusbar.phone.NotifPanelEventsModule; import com.android.systemui.statusbar.phone.ShadeController; import com.android.systemui.statusbar.policy.HeadsUpManager; @@ -111,7 +110,6 @@ import dagger.Provides; @Module(includes = { CoordinatorsModule.class, KeyguardNotificationVisibilityProviderModule.class, - NotifActivityLaunchEventsModule.class, NotifPanelEventsModule.class, NotifPipelineChoreographerModule.class, NotificationSectionHeadersModule.class, diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotifActivityLaunchEvents.kt b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotifActivityLaunchEvents.kt deleted file mode 100644 index f46d073382069..0000000000000 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotifActivityLaunchEvents.kt +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Copyright (C) 2022 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.systemui.statusbar.phone - -import com.android.systemui.statusbar.notification.collection.NotificationEntry - -/** Provides events about [android.app.Activity] launches from Notifications. */ -interface NotifActivityLaunchEvents { - - /** Registers a [Listener] to be invoked when notification activity launch events occur. */ - fun registerListener(listener: Listener) - - /** Unregisters a [Listener] previously registered via [registerListener] */ - fun unregisterListener(listener: Listener) - - /** Listener for events about [android.app.Activity] launches from Notifications. */ - interface Listener { - - /** Invoked when an activity has started launching from a notification. */ - fun onStartLaunchNotifActivity(entry: NotificationEntry) - - /** Invoked when an activity has finished launching. */ - fun onFinishLaunchNotifActivity(entry: NotificationEntry) - } -} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotifActivityLaunchEventsModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotifActivityLaunchEventsModule.java deleted file mode 100644 index 84ff538677b0d..0000000000000 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/NotifActivityLaunchEventsModule.java +++ /dev/null @@ -1,30 +0,0 @@ -/* - * Copyright (C) 2022 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.systemui.statusbar.phone; - -import com.android.systemui.dagger.SysUISingleton; - -import dagger.Binds; -import dagger.Module; - -/** Provides a {@link NotifActivityLaunchEvents} in {@link SysUISingleton} scope. */ -@Module -public abstract class NotifActivityLaunchEventsModule { - @Binds - abstract NotifActivityLaunchEvents bindLaunchEvents( - StatusBarNotificationActivityStarter.LaunchEventsEmitter impl); -} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java index eee7b2a41de49..cf776e3b60d12 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarter.java @@ -41,7 +41,6 @@ import android.text.TextUtils; import android.util.EventLog; import android.view.View; -import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; import com.android.internal.jank.InteractionJankMonitor; @@ -52,7 +51,6 @@ import com.android.systemui.ActivityIntentHelper; import com.android.systemui.EventLogTags; import com.android.systemui.animation.ActivityLaunchAnimator; import com.android.systemui.assist.AssistManager; -import com.android.systemui.dagger.SysUISingleton; import com.android.systemui.plugins.ActivityStarter; import com.android.systemui.plugins.statusbar.StatusBarStateController; import com.android.systemui.statusbar.CommandQueue; @@ -77,7 +75,6 @@ import com.android.systemui.statusbar.notification.row.OnUserInteractionCallback import com.android.systemui.statusbar.phone.dagger.CentralSurfacesComponent; import com.android.systemui.statusbar.policy.HeadsUpUtil; import com.android.systemui.statusbar.policy.KeyguardStateController; -import com.android.systemui.util.ListenerSet; import com.android.systemui.wmshell.BubblesManager; import java.util.Optional; @@ -131,7 +128,6 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte private final ActivityLaunchAnimator mActivityLaunchAnimator; private final NotificationLaunchAnimatorControllerProvider mNotificationAnimationProvider; private final OnUserInteractionCallback mOnUserInteractionCallback; - private final LaunchEventsEmitter mLaunchEventsEmitter; private boolean mIsCollapsingToShowActivityOverLockscreen; @@ -170,8 +166,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte NotificationPresenter presenter, NotificationPanelViewController panel, ActivityLaunchAnimator activityLaunchAnimator, - NotificationLaunchAnimatorControllerProvider notificationAnimationProvider, - LaunchEventsEmitter launchEventsEmitter) { + NotificationLaunchAnimatorControllerProvider notificationAnimationProvider) { mContext = context; mCommandQueue = commandQueue; mMainThreadHandler = mainThreadHandler; @@ -207,7 +202,6 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte mNotificationPanel = panel; mActivityLaunchAnimator = activityLaunchAnimator; mNotificationAnimationProvider = notificationAnimationProvider; - mLaunchEventsEmitter = launchEventsEmitter; if (!mNotifPipelineFlags.isNewPipelineEnabled()) { mEntryManager.addNotificationEntryListener(new NotificationEntryListener() { @@ -257,8 +251,6 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte return; } - mLaunchEventsEmitter.notifyStartLaunchNotifActivity(entry); - boolean isActivityIntent = intent != null && intent.isActivity() && !isBubble; final boolean willLaunchResolverActivity = isActivityIntent && mActivityIntentHelper.wouldLaunchResolverActivity(intent.getIntent(), @@ -286,7 +278,7 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte } else { mActivityStarter.dismissKeyguardThenExecute( postKeyguardAction, - () -> mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry), + null, willLaunchResolverActivity); } } @@ -369,8 +361,6 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte mLogger.logExpandingBubble(entry); removeHunAfterClick(row); expandBubbleStackOnMainThread(entry); - mMainThreadHandler.post( - () -> mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry)); } else { startNotificationIntent(intent, fillInIntent, entry, row, animate, isActivityIntent); } @@ -382,21 +372,11 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte if (!canBubble && (shouldAutoCancel(entry.getSbn()) || mRemoteInputManager.isNotificationKeptForRemoteInputHistory(notificationKey))) { - final Runnable locallyDismissNotification = + final Runnable removeNotification = mOnUserInteractionCallback.registerFutureDismissal(entry, REASON_CLICK); // Immediately remove notification from visually showing. // We have to post the removal to the UI thread for synchronization. mMainThreadHandler.post(() -> { - final Runnable removeNotification = () -> { - locallyDismissNotification.run(); - if (!animate) { - // If we're animating, this would be invoked after the activity launch - // animation completes. Since we're not animating, the launch already - // happened synchronously, so we notify the launch is complete here after - // onDismiss. - mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry); - } - }; if (mPresenter.isCollapsing()) { // To avoid lags we're only performing the remove after the shade is collapsed mShadeController.addPostCollapseAction(removeNotification); @@ -404,10 +384,6 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte removeNotification.run(); } }); - } else if (!canBubble && !animate) { - // Not animating, this is the end of the launch flow (see above comment for more info). - mMainThreadHandler.post( - () -> mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry)); } // inform NMS that the notification was clicked @@ -479,13 +455,9 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte boolean isActivityIntent) { mLogger.logStartNotificationIntent(entry); try { - Runnable onFinishAnimationCallback = animate - ? () -> mLaunchEventsEmitter.notifyFinishLaunchNotifActivity(entry) - : null; ActivityLaunchAnimator.Controller animationController = new StatusBarLaunchAnimatorController( - mNotificationAnimationProvider - .getAnimatorController(row, onFinishAnimationCallback), + mNotificationAnimationProvider.getAnimatorController(row, null), mCentralSurfaces, isActivityIntent); mActivityLaunchAnimator.startPendingIntentWithAnimation( @@ -673,35 +645,4 @@ class StatusBarNotificationActivityStarter implements NotificationActivityStarte return entry.shouldSuppressFullScreenIntent(); } - - @SysUISingleton - static class LaunchEventsEmitter implements NotifActivityLaunchEvents { - - private final ListenerSet mListeners = new ListenerSet<>(); - - @Inject - LaunchEventsEmitter() {} - - @Override - public void registerListener(@NonNull Listener listener) { - mListeners.addIfAbsent(listener); - } - - @Override - public void unregisterListener(@NonNull Listener listener) { - mListeners.remove(listener); - } - - private void notifyStartLaunchNotifActivity(NotificationEntry entry) { - for (Listener listener : mListeners) { - listener.onStartLaunchNotifActivity(entry); - } - } - - private void notifyFinishLaunchNotifActivity(NotificationEntry entry) { - for (Listener listener : mListeners) { - listener.onFinishLaunchNotifActivity(entry); - } - } - } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ActivityLaunchAnimCoordinatorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ActivityLaunchAnimCoordinatorTest.kt deleted file mode 100644 index c6c043aafb208..0000000000000 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/ActivityLaunchAnimCoordinatorTest.kt +++ /dev/null @@ -1,138 +0,0 @@ -/* - * Copyright (C) 2022 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.systemui.statusbar.notification.collection.coordinator - -import androidx.test.filters.SmallTest -import com.android.systemui.SysuiTestCase -import com.android.systemui.statusbar.notification.collection.NotifPipeline -import com.android.systemui.statusbar.notification.collection.NotificationEntry -import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorScope -import com.android.systemui.statusbar.notification.collection.notifcollection.NotifLifetimeExtender -import com.android.systemui.statusbar.phone.NotifActivityLaunchEvents -import com.android.systemui.util.mockito.mock -import com.android.systemui.util.mockito.withArgCaptor -import dagger.BindsInstance -import dagger.Component -import org.junit.Assert.assertFalse -import org.junit.Assert.assertTrue -import org.junit.Test -import org.mockito.Mockito.never -import org.mockito.Mockito.verify -import org.mockito.Mockito.`when` as whenever - -@SmallTest -class ActivityLaunchAnimCoordinatorTest : SysuiTestCase() { - - val activityLaunchEvents: NotifActivityLaunchEvents = mock() - val pipeline: NotifPipeline = mock() - - val coordinator: ActivityLaunchAnimCoordinator = - DaggerTestActivityStarterCoordinatorComponent - .factory() - .create(activityLaunchEvents) - .coordinator - - @Test - fun testNoLifetimeExtensionIfNoAssociatedActivityLaunch() { - coordinator.attach(pipeline) - val lifetimeExtender = withArgCaptor { - verify(pipeline).addNotificationLifetimeExtender(capture()) - } - val fakeEntry = mock().also { - whenever(it.key).thenReturn("0") - } - assertFalse(lifetimeExtender.maybeExtendLifetime(fakeEntry, 0)) - } - - @Test - fun testNoLifetimeExtensionIfAssociatedActivityLaunchAlreadyEnded() { - coordinator.attach(pipeline) - val lifetimeExtender = withArgCaptor { - verify(pipeline).addNotificationLifetimeExtender(capture()) - } - val eventListener = withArgCaptor { - verify(activityLaunchEvents).registerListener(capture()) - } - val fakeEntry = mock().also { - whenever(it.key).thenReturn("0") - } - eventListener.onStartLaunchNotifActivity(fakeEntry) - eventListener.onFinishLaunchNotifActivity(fakeEntry) - assertFalse(lifetimeExtender.maybeExtendLifetime(fakeEntry, 0)) - } - - @Test - fun testLifetimeExtensionWhileActivityLaunchInProgress() { - coordinator.attach(pipeline) - val lifetimeExtender = withArgCaptor { - verify(pipeline).addNotificationLifetimeExtender(capture()) - } - val eventListener = withArgCaptor { - verify(activityLaunchEvents).registerListener(capture()) - } - val onEndLifetimeExtensionCallback = - mock() - lifetimeExtender.setCallback(onEndLifetimeExtensionCallback) - - val fakeEntry = mock().also { - whenever(it.key).thenReturn("0") - } - eventListener.onStartLaunchNotifActivity(fakeEntry) - assertTrue(lifetimeExtender.maybeExtendLifetime(fakeEntry, 0)) - - eventListener.onFinishLaunchNotifActivity(fakeEntry) - verify(onEndLifetimeExtensionCallback).onEndLifetimeExtension(lifetimeExtender, fakeEntry) - } - - @Test - fun testCancelLifetimeExtensionDoesNotInvokeCallback() { - coordinator.attach(pipeline) - val lifetimeExtender = withArgCaptor { - verify(pipeline).addNotificationLifetimeExtender(capture()) - } - val eventListener = withArgCaptor { - verify(activityLaunchEvents).registerListener(capture()) - } - val onEndLifetimeExtensionCallback = - mock() - lifetimeExtender.setCallback(onEndLifetimeExtensionCallback) - - val fakeEntry = mock().also { - whenever(it.key).thenReturn("0") - } - eventListener.onStartLaunchNotifActivity(fakeEntry) - assertTrue(lifetimeExtender.maybeExtendLifetime(fakeEntry, 0)) - - lifetimeExtender.cancelLifetimeExtension(fakeEntry) - eventListener.onFinishLaunchNotifActivity(fakeEntry) - verify(onEndLifetimeExtensionCallback, never()) - .onEndLifetimeExtension(lifetimeExtender, fakeEntry) - } -} - -@CoordinatorScope -@Component(modules = [ActivityLaunchAnimCoordinatorModule::class]) -interface TestActivityStarterCoordinatorComponent { - val coordinator: ActivityLaunchAnimCoordinator - - @Component.Factory - interface Factory { - fun create( - @BindsInstance activityLaunchEvents: NotifActivityLaunchEvents - ): TestActivityStarterCoordinatorComponent - } -} diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java index 273b9a17ce1d4..ecea14c6a5224 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarNotificationActivityStarterTest.java @@ -85,7 +85,6 @@ import com.android.systemui.wmshell.BubblesManager; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.Mockito; @@ -149,7 +148,6 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { private ActivityLaunchAnimator mActivityLaunchAnimator; @Mock private InteractionJankMonitor mJankMonitor; - private StatusBarNotificationActivityStarter.LaunchEventsEmitter mLaunchEventsEmitter; private FakeExecutor mUiBgExecutor = new FakeExecutor(new FakeSystemClock()); private NotificationTestHelper mNotificationTestHelper; private ExpandableNotificationRow mNotificationRow; @@ -206,7 +204,6 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { NotificationListContainer.class), headsUpManager, mJankMonitor); - mLaunchEventsEmitter = new StatusBarNotificationActivityStarter.LaunchEventsEmitter(); mNotificationActivityStarter = new StatusBarNotificationActivityStarter( getContext(), @@ -242,8 +239,7 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { mock(NotificationPresenter.class), mock(NotificationPanelViewController.class), mActivityLaunchAnimator, - notificationAnimationProvider, - mLaunchEventsEmitter + notificationAnimationProvider ); // set up dismissKeyguardThenExecute to synchronously invoke the OnDismissAction arg @@ -421,60 +417,4 @@ public class StatusBarNotificationActivityStarterTest extends SysuiTestCase { // THEN display should try wake up for the full screen intent verify(mCentralSurfaces).wakeUpForFullScreenIntent(); } - - @Test - public void testNotifActivityStarterEventSourceStartEvent_onNotificationClicked() { - final NotificationEntry entry = mNotificationRow.getEntry(); - NotifActivityLaunchEvents.Listener listener = - mock(NotifActivityLaunchEvents.Listener.class); - mLaunchEventsEmitter.registerListener(listener); - mNotificationActivityStarter.onNotificationClicked(entry, mNotificationRow); - verify(listener).onStartLaunchNotifActivity(entry); - } - - @Test - public void testNotifActivityStarterEventSourceFinishEvent_dismissKeyguardCancelled() { - final NotificationEntry entry = mNotificationRow.getEntry(); - NotifActivityLaunchEvents.Listener listener = - mock(NotifActivityLaunchEvents.Listener.class); - mLaunchEventsEmitter.registerListener(listener); - // set up dismissKeyguardThenExecute to synchronously invoke the cancel runnable arg - doAnswer(answerVoid( - (OnDismissAction dismissAction, Runnable cancel, Boolean afterKeyguardGone) -> - cancel.run())) - .when(mActivityStarter) - .dismissKeyguardThenExecute(any(OnDismissAction.class), any(), anyBoolean()); - mNotificationActivityStarter - .onNotificationClicked(entry, mNotificationRow); - verify(listener).onFinishLaunchNotifActivity(entry); - } - - @Test - public void testNotifActivityStarterEventSourceFinishEvent_postPanelCollapse() - throws Exception { - final NotificationEntry entry = mNotificationRow.getEntry(); - NotifActivityLaunchEvents.Listener listener = - mock(NotifActivityLaunchEvents.Listener.class); - mLaunchEventsEmitter.registerListener(listener); - mNotificationActivityStarter - .onNotificationClicked(entry, mNotificationRow); - ArgumentCaptor controllerCaptor = - ArgumentCaptor.forClass(ActivityLaunchAnimator.Controller.class); - verify(mActivityLaunchAnimator).startPendingIntentWithAnimation( - controllerCaptor.capture(), anyBoolean(), any(), any()); - controllerCaptor.getValue().onIntentStarted(false); - verify(listener).onFinishLaunchNotifActivity(entry); - } - - @Test - public void testNotifActivityStarterEventSourceFinishEvent_postPanelCollapse_noAnimate() { - final NotificationEntry entry = mNotificationRow.getEntry(); - NotifActivityLaunchEvents.Listener listener = - mock(NotifActivityLaunchEvents.Listener.class); - mLaunchEventsEmitter.registerListener(listener); - when(mCentralSurfaces.shouldAnimateLaunch(anyBoolean())).thenReturn(false); - mNotificationActivityStarter - .onNotificationClicked(entry, mNotificationRow); - verify(listener).onFinishLaunchNotifActivity(entry); - } }