Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit b7ce749a authored by Ioana Alexandru's avatar Ioana Alexandru
Browse files

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
parent e37b08f3

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinator.java

+53 −26
Original line number Diff line number Diff line
@@ -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,11 +56,58 @@ 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;

            }



            if (!Flags.notificationsBackgroundMediaIcons()) {

                inflateOrUpdateIcons(entry);

            }



            return true;

        }

    };



    private final NotifCollectionListener mCollectionListener = new NotifCollectionListener() {

        @Override

        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(@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(@NonNull NotificationEntry entry) {

            mIconsState.remove(entry);

        }

    };



    private void inflateOrUpdateIcons(NotificationEntry entry) {

        switch (mIconsState.getOrDefault(entry, STATE_ICONS_UNINFLATED)) {

            case STATE_ICONS_UNINFLATED:

                try {
@@ -80,30 +130,7 @@ public class MediaCoordinator implements Coordinator { 
                // do nothing

                break;

        }



            return true;

        }

    };



    private final NotifCollectionListener mCollectionListener = new NotifCollectionListener() {

        @Override

        public void onEntryInit(NotificationEntry entry) {

            mIconsState.put(entry, STATE_ICONS_UNINFLATED);

        }



        @Override

        public void onEntryUpdated(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);

    }

        }



        @Override

        public void onEntryCleanUp(NotificationEntry entry) {

            mIconsState.remove(entry);

        }

    };



    private void reportInflationError(NotificationEntry entry, Exception e) {

        // This is the same logic as in PreparationCoordinator; it doesn't handle media

packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/MediaCoordinatorTest.java

+62 −2
Original line number Diff line number Diff line
@@ -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);