Avoid side effects in MediaCoordinator notif filter.

Notification filtering shouldn't have side effects. Instead, we should
inflate/update the icons via NotifCollectionListener.

Test: checked that YTM media notification looks good (incl. on AOD) +
MediaCoordinatorTest
Bug: 315143160
Flag: ACONFIG com.android.systemui.notifications_background_media_icons DEVELOPMENT

Change-Id: Icfac69ecc4f2ba787f4bd5bd835409d0ca2498b1

2 files changed, 115 insertions, 28 deletions

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinator.java
index 0be4bde749f3..16af9d9c82ae 100644
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinator.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinator.java
@@ -22,7 +22,10 @@ import android.os.RemoteException;
 import android.service.notification.StatusBarNotification;
 import android.util.ArrayMap;
 
+import androidx.annotation.NonNull;
+
 import com.android.internal.statusbar.IStatusBarService;
+import com.android.systemui.Flags;
 import com.android.systemui.media.controls.util.MediaFeatureFlag;
 import com.android.systemui.statusbar.notification.InflationException;
 import com.android.systemui.statusbar.notification.collection.NotifPipeline;
@@ -53,32 +56,13 @@ public class MediaCoordinator implements Coordinator {
 
     private final NotifFilter mMediaFilter = new NotifFilter(TAG) {
         @Override
-        public boolean shouldFilterOut(NotificationEntry entry, long now) {
+        public boolean shouldFilterOut(@NonNull NotificationEntry entry, long now) {
             if (!mIsMediaFeatureEnabled || !isMediaNotification(entry.getSbn())) {
                 return false;
             }
 
-            switch (mIconsState.getOrDefault(entry, STATE_ICONS_UNINFLATED)) {
-                case STATE_ICONS_UNINFLATED:
-                    try {
-                        mIconManager.createIcons(entry);
-                        mIconsState.put(entry, STATE_ICONS_INFLATED);
-                    } catch (InflationException e) {
-                        reportInflationError(entry, e);
-                        mIconsState.put(entry, STATE_ICONS_ERROR);
-                    }
-                    break;
-                case STATE_ICONS_INFLATED:
-                    try {
-                        mIconManager.updateIcons(entry);
-                    } catch (InflationException e) {
-                        reportInflationError(entry, e);
-                        mIconsState.put(entry, STATE_ICONS_ERROR);
-                    }
-                    break;
-                case STATE_ICONS_ERROR:
-                    // do nothing
-                    break;
+            if (!Flags.notificationsBackgroundMediaIcons()) {
+                inflateOrUpdateIcons(entry);
             }
 
             return true;
@@ -87,24 +71,67 @@ public class MediaCoordinator implements Coordinator {
 
     private final NotifCollectionListener mCollectionListener = new NotifCollectionListener() {
         @Override
-        public void onEntryInit(NotificationEntry entry) {
-            mIconsState.put(entry, STATE_ICONS_UNINFLATED);
+        public void onEntryInit(@NonNull NotificationEntry entry) {
+            // We default to STATE_ICONS_UNINFLATED anyway, so there's no need to initialize it.
+            if (!Flags.notificationsBackgroundMediaIcons()) {
+                mIconsState.put(entry, STATE_ICONS_UNINFLATED);
+            }
+        }
+
+        @Override
+        public void onEntryAdded(@NonNull NotificationEntry entry) {
+            if (Flags.notificationsBackgroundMediaIcons()) {
+                if (isMediaNotification(entry.getSbn())) {
+                    inflateOrUpdateIcons(entry);
+                }
+            }
         }
 
         @Override
-        public void onEntryUpdated(NotificationEntry entry) {
+        public void onEntryUpdated(@NonNull NotificationEntry entry) {
             if (mIconsState.getOrDefault(entry, STATE_ICONS_UNINFLATED) == STATE_ICONS_ERROR) {
                 // The update may have fixed the inflation error, so give it another chance.
                 mIconsState.put(entry, STATE_ICONS_UNINFLATED);
             }
+
+            if (Flags.notificationsBackgroundMediaIcons()) {
+                if (isMediaNotification(entry.getSbn())) {
+                    inflateOrUpdateIcons(entry);
+                }
+            }
         }
 
         @Override
-        public void onEntryCleanUp(NotificationEntry entry) {
+        public void onEntryCleanUp(@NonNull NotificationEntry entry) {
             mIconsState.remove(entry);
         }
     };
 
+    private void inflateOrUpdateIcons(NotificationEntry entry) {
+        switch (mIconsState.getOrDefault(entry, STATE_ICONS_UNINFLATED)) {
+            case STATE_ICONS_UNINFLATED:
+                try {
+                    mIconManager.createIcons(entry);
+                    mIconsState.put(entry, STATE_ICONS_INFLATED);
+                } catch (InflationException e) {
+                    reportInflationError(entry, e);
+                    mIconsState.put(entry, STATE_ICONS_ERROR);
+                }
+                break;
+            case STATE_ICONS_INFLATED:
+                try {
+                    mIconManager.updateIcons(entry);
+                } catch (InflationException e) {
+                    reportInflationError(entry, e);
+                    mIconsState.put(entry, STATE_ICONS_ERROR);
+                }
+                break;
+            case STATE_ICONS_ERROR:
+                // do nothing
+                break;
+        }
+    }
+
     private void reportInflationError(NotificationEntry entry, Exception e) {
         // This is the same logic as in PreparationCoordinator; it doesn't handle media
         // notifications when the media feature is enabled since they aren't displayed in the shade,
diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinatorTest.java
index 590c902ba687..b548117684ad 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinatorTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinatorTest.java
@@ -19,6 +19,7 @@ package com.android.systemui.statusbar.notification.collection.coordinator;
 import static com.google.common.truth.Truth.assertThat;
 
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.clearInvocations;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.never;
@@ -28,12 +29,15 @@ import static org.mockito.Mockito.when;
 
 import android.app.Notification.MediaStyle;
 import android.media.session.MediaSession;
+import android.platform.test.annotations.DisableFlags;
+import android.platform.test.annotations.EnableFlags;
 import android.service.notification.NotificationListenerService;
 import android.testing.AndroidTestingRunner;
 
 import androidx.test.filters.SmallTest;
 
 import com.android.internal.statusbar.IStatusBarService;
+import com.android.systemui.Flags;
 import com.android.systemui.SysuiTestCase;
 import com.android.systemui.media.controls.util.MediaFeatureFlag;
 import com.android.systemui.statusbar.notification.InflationException;
@@ -153,7 +157,8 @@ public final class MediaCoordinatorTest extends SysuiTestCase {
     }
 
     @Test
-    public void inflateMediaNotificationIconsMediaEnabled() throws InflationException {
+    @DisableFlags(Flags.FLAG_NOTIFICATIONS_BACKGROUND_MEDIA_ICONS)
+    public void inflateMediaNotificationIconsMediaEnabled_old() throws InflationException {
         finishSetupWithMediaFeatureFlagEnabled(true);
 
         mListener.onEntryInit(mMediaEntry);
@@ -181,7 +186,37 @@ public final class MediaCoordinatorTest extends SysuiTestCase {
     }
 
     @Test
-    public void inflationException() throws InflationException {
+    @EnableFlags(Flags.FLAG_NOTIFICATIONS_BACKGROUND_MEDIA_ICONS)
+    public void inflateMediaNotificationIconsMediaEnabled_new() throws InflationException {
+        finishSetupWithMediaFeatureFlagEnabled(true);
+
+        mListener.onEntryInit(mMediaEntry);
+        mListener.onEntryAdded(mMediaEntry);
+        verify(mIconManager).createIcons(eq(mMediaEntry));
+        verify(mIconManager, never()).updateIcons(eq(mMediaEntry));
+        clearInvocations(mIconManager);
+
+        mFilter.shouldFilterOut(mMediaEntry, 0);
+        verify(mIconManager, never()).createIcons(eq(mMediaEntry));
+        verify(mIconManager, never()).updateIcons(eq(mMediaEntry));
+
+        mListener.onEntryUpdated(mMediaEntry);
+        verify(mIconManager, never()).createIcons(eq(mMediaEntry));
+        verify(mIconManager).updateIcons(eq(mMediaEntry));
+
+        mListener.onEntryRemoved(mMediaEntry, NotificationListenerService.REASON_CANCEL);
+        mListener.onEntryCleanUp(mMediaEntry);
+        clearInvocations(mIconManager);
+
+        mListener.onEntryInit(mMediaEntry);
+        mListener.onEntryAdded(mMediaEntry);
+        verify(mIconManager).createIcons(eq(mMediaEntry));
+        verify(mIconManager, never()).updateIcons(eq(mMediaEntry));
+    }
+
+    @Test
+    @DisableFlags(Flags.FLAG_NOTIFICATIONS_BACKGROUND_MEDIA_ICONS)
+    public void inflationException_old() throws InflationException {
         finishSetupWithMediaFeatureFlagEnabled(true);
 
         mListener.onEntryInit(mMediaEntry);
@@ -208,6 +243,31 @@ public final class MediaCoordinatorTest extends SysuiTestCase {
         verify(mIconManager, never()).updateIcons(eq(mMediaEntry));
     }
 
+    @Test
+    @EnableFlags(Flags.FLAG_NOTIFICATIONS_BACKGROUND_MEDIA_ICONS)
+    public void inflationException_new() throws InflationException {
+        finishSetupWithMediaFeatureFlagEnabled(true);
+
+        doThrow(InflationException.class).when(mIconManager).createIcons(eq(mMediaEntry));
+
+        mListener.onEntryInit(mMediaEntry);
+        mListener.onEntryAdded(mMediaEntry);
+        verify(mIconManager).createIcons(eq(mMediaEntry));
+        verify(mIconManager, never()).updateIcons(eq(mMediaEntry));
+        clearInvocations(mIconManager);
+
+        mListener.onEntryUpdated(mMediaEntry);
+        verify(mIconManager).createIcons(eq(mMediaEntry));
+        verify(mIconManager, never()).updateIcons(eq(mMediaEntry));
+        clearInvocations(mIconManager);
+
+        doNothing().when(mIconManager).createIcons(eq(mMediaEntry));
+
+        mListener.onEntryUpdated(mMediaEntry);
+        verify(mIconManager).createIcons(eq(mMediaEntry));
+        verify(mIconManager, never()).updateIcons(eq(mMediaEntry));
+    }
+
     private void finishSetupWithMediaFeatureFlagEnabled(boolean mediaFeatureFlagEnabled) {
         when(mMediaFeatureFlag.getEnabled()).thenReturn(mediaFeatureFlagEnabled);
         mCoordinator = new MediaCoordinator(mMediaFeatureFlag, mStatusBarService, mIconManager);