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

Commit 2cfc95bf authored by Julia Tuttle's avatar Julia Tuttle
Browse files

HeadsUpViewBinder: Don't WTF if notif stops alerting after cleanup. 

Right now, a notification can go from heads up to not after it's removed
from the NotifCollection and cleaned up, and HeadsUpCoordinator will
happily ask HeadsUpViewVinder to unbind the heads up view, but then
RowContentBindStage will complain that there are no bind stage params
for the notification.

Long-term, we'd like to improve the clarity of the interactions between
a notification's lifetime in NotifCollection and in HeadsUpManager, but
short-term, this change keeps HeadsUpViewBinder from triggering a WTF
when a notification stops alerting after it was cleaned up.

Bug: 245537746
Test: atest HeadsUpViewBinderTest
Change-Id: I06a6a74c9946398cc4ed22544e5d205f6d74bd6a
parent aea723c8

packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinder.java

+12 −1
Original line number Diff line number Diff line
@@ -114,7 +114,18 @@ public class HeadsUpViewBinder { 
     */

    public void unbindHeadsUpView(NotificationEntry entry) {

        abortBindCallback(entry);

        mStage.getStageParams(entry).markContentViewsFreeable(FLAG_CONTENT_VIEW_HEADS_UP);



        // params may be null if the notification was already removed from the collection but we let

        // it stick around during a launch animation. In this case, the heads up view has already

        // been unbound, so we don't need to unbind it.

        // TODO(b/253081345): Change this back to getStageParams and remove null check.

        RowContentBindParams params = mStage.tryGetStageParams(entry);

        if (params == null) {

            mLogger.entryBindStageParamsNullOnUnbind(entry);

            return;

        }



        params.markContentViewsFreeable(FLAG_CONTENT_VIEW_HEADS_UP);

        mLogger.entryContentViewMarkedFreeable(entry);

        mStage.requestRebind(entry, e -> mLogger.entryUnbound(e));

    }

packages/SystemUI/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderLogger.kt

+8 −0
Original line number Diff line number Diff line
@@ -47,6 +47,14 @@ class HeadsUpViewBinderLogger @Inject constructor(@NotificationHeadsUpLog val bu 
            "start unbinding heads up entry $str1 "

        })

    }



    fun entryBindStageParamsNullOnUnbind(entry: NotificationEntry) {

        buffer.log(TAG, INFO, {

            str1 = entry.logKey

        }, {

            "heads up entry bind stage params null on unbind $str1 "

        })

    }

}



private const val TAG = "HeadsUpViewBinder"

packages/SystemUI/src/com/android/systemui/statusbar/notification/row/BindStage.java

+13 −1
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ import android.util.ArrayMap; 
import android.util.Log;



import androidx.annotation.NonNull;

import androidx.annotation.Nullable;



import com.android.systemui.statusbar.notification.collection.NotificationEntry;
@@ -64,7 +65,7 @@ public abstract class BindStage<Params> extends BindRequester { 
     * Get the stage parameters for the entry. Clients should use this to modify how the stage

     * handles the notification content.

     */

    public final Params getStageParams(@NonNull NotificationEntry entry) {

    public final @NonNull Params getStageParams(@NonNull NotificationEntry entry) {

        Params params = mContentParams.get(entry);

        if (params == null) {

            // TODO: This should throw an exception but there are some cases of re-entrant calls
@@ -79,6 +80,17 @@ public abstract class BindStage<Params> extends BindRequester { 
        return params;

    }



    // TODO(b/253081345): Remove this method.

    /**

     * Get the stage parameters for the entry, or null if there are no stage parameters for the

     * entry.

     *

     * @see #getStageParams(NotificationEntry)

     */

    public final @Nullable Params tryGetStageParams(@NonNull NotificationEntry entry) {

        return mContentParams.get(entry);

    }



    /**

     * Create a params entry for the notification for this stage.

     */

packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/interruption/HeadsUpViewBinderTest.java

+30 −0
Original line number Diff line number Diff line
@@ -91,6 +91,8 @@ public class HeadsUpViewBinderTest extends SysuiTestCase { 
        verifyNoMoreInteractions(mLogger);

        clearInvocations(mLogger);



        when(mBindStage.tryGetStageParams(eq(mEntry))).thenReturn(new RowContentBindParams());



        mViewBinder.unbindHeadsUpView(mEntry);

        verify(mLogger).entryContentViewMarkedFreeable(eq(mEntry));

        verifyNoMoreInteractions(mLogger);
@@ -139,6 +141,8 @@ public class HeadsUpViewBinderTest extends SysuiTestCase { 
        verifyNoMoreInteractions(mLogger);

        clearInvocations(mLogger);



        when(mBindStage.tryGetStageParams(eq(mEntry))).thenReturn(new RowContentBindParams());



        mViewBinder.unbindHeadsUpView(mEntry);

        verify(mLogger).currentOngoingBindingAborted(eq(mEntry));

        verify(mLogger).entryContentViewMarkedFreeable(eq(mEntry));
@@ -150,4 +154,30 @@ public class HeadsUpViewBinderTest extends SysuiTestCase { 
        verifyNoMoreInteractions(mLogger);

        clearInvocations(mLogger);

    }



    @Test

    public void testLoggingForLateUnbindFlow() {

        AtomicReference<NotifBindPipeline.BindCallback> callback = new AtomicReference<>();

        when(mBindStage.requestRebind(any(), any())).then(i -> {

            callback.set(i.getArgument(1));

            return new CancellationSignal();

        });



        mViewBinder.bindHeadsUpView(mEntry, null);

        verify(mLogger).startBindingHun(eq(mEntry));

        verifyNoMoreInteractions(mLogger);

        clearInvocations(mLogger);



        callback.get().onBindFinished(mEntry);

        verify(mLogger).entryBoundSuccessfully(eq(mEntry));

        verifyNoMoreInteractions(mLogger);

        clearInvocations(mLogger);



        when(mBindStage.tryGetStageParams(eq(mEntry))).thenReturn(null);



        mViewBinder.unbindHeadsUpView(mEntry);

        verify(mLogger).entryBindStageParamsNullOnUnbind(eq(mEntry));

        verifyNoMoreInteractions(mLogger);

        clearInvocations(mLogger);

    }

}

packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/RowContentBindStageTest.java

+66 −0
Original line number Diff line number Diff line
@@ -21,6 +21,10 @@ import static com.android.systemui.statusbar.notification.row.NotificationRowCon 
import static com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.FLAG_CONTENT_VIEW_EXPANDED;

import static com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.FLAG_CONTENT_VIEW_HEADS_UP;



import static junit.framework.Assert.assertEquals;

import static junit.framework.Assert.assertNotNull;

import static junit.framework.Assert.assertNotSame;

import static junit.framework.Assert.assertNull;

import static junit.framework.Assert.assertTrue;



import static org.mockito.ArgumentMatchers.any;
@@ -31,6 +35,7 @@ import static org.mockito.Mockito.verify; 


import android.testing.AndroidTestingRunner;

import android.testing.TestableLooper;

import android.util.Log;



import androidx.test.filters.SmallTest;
@@ -100,6 +105,67 @@ public class RowContentBindStageTest extends SysuiTestCase { 
        verify(mBinder).unbindContent(eq(mEntry), any(), eq(flags));

    }



    class CountingWtfHandler implements Log.TerribleFailureHandler {

        private Log.TerribleFailureHandler mOldHandler = null;

        private int mWtfCount = 0;



        public void register() {

            mOldHandler = Log.setWtfHandler(this);

        }



        public void unregister() {

            Log.setWtfHandler(mOldHandler);

            mOldHandler = null;

        }



        @Override

        public void onTerribleFailure(String tag, Log.TerribleFailure what, boolean system) {

            mWtfCount++;

        }



        public int getWtfCount() {

            return mWtfCount;

        }

    }



    @Test

    public void testGetStageParamsAfterCleanUp() {

        // GIVEN an entry whose params have already been deleted.

        RowContentBindParams originalParams = mRowContentBindStage.getStageParams(mEntry);

        mRowContentBindStage.deleteStageParams(mEntry);



        // WHEN a caller calls getStageParams.

        CountingWtfHandler countingWtfHandler = new CountingWtfHandler();

        countingWtfHandler.register();



        RowContentBindParams blankParams = mRowContentBindStage.getStageParams(mEntry);



        countingWtfHandler.unregister();



        // THEN getStageParams logs a WTF and returns blank params created to avoid a crash.

        assertEquals(1, countingWtfHandler.getWtfCount());

        assertNotNull(blankParams);

        assertNotSame(originalParams, blankParams);

    }



    @Test

    public void testTryGetStageParamsAfterCleanUp() {

        // GIVEN an entry whose params have already been deleted.

        mRowContentBindStage.deleteStageParams(mEntry);



        // WHEN a caller calls getStageParams.

        CountingWtfHandler countingWtfHandler = new CountingWtfHandler();

        countingWtfHandler.register();



        RowContentBindParams nullParams = mRowContentBindStage.tryGetStageParams(mEntry);



        countingWtfHandler.unregister();



        // THEN getStageParams does NOT log a WTF and returns null to indicate missing params.

        assertEquals(0, countingWtfHandler.getWtfCount());

        assertNull(nullParams);

    }



    @Test

    public void testRebindAllContentViews() {

        // GIVEN a view with content bound.