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

Commit 4a6a45ef authored by Jeff DeCew's avatar Jeff DeCew
Browse files

Fix bug where FSI decision was not logged unless FSI was launching. 

* Change HeadsUpCoordinator's `onEntryAdded` logic so that `logFullScreenIntentDecision` is called unconditionally after `getFullScreenIntentDecision`.
* Change HeadsUpCoordinator's `onEntryUpdated` logic so each reconsideration can keep happening ONLY if the decision keeps being `NO_FSI_SUPPRESSED_ONLY_BY_DND`.
  * If the FSI should launch, in addition to launching it still calls `logFullScreenIntentDecision` and now also revokes the candidacy for reconsideration.
  * If the FSI should NOT launch (and the decision changed), it now calls `logFullScreenIntentDecision` and also revokes the candidacy for reconsideration.
* Fixes a bug & fragile code in `logFullScreenIntentDecision` where not all decision values were enumerated for logging.  The "human readable" string is gone, and replaced with a default case which logs the enum name.
* Adds some nullability annotations to the java interface.
  * Remove an unnecessary null check on the decision.
  * Convert an enum `.equals()` to `==`.

Bug: 265977861
Test: dumpsysui NotifInterruptLog
Test: atest HeadsUpCoordinatorTest NotificationInterruptStateProviderImplTest
Change-Id: Ie6d379008c9cd99cfcfe0f9da43e602a436a0d39
parent b457973c

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinator.kt

+15 −5
Original line number Diff line number Diff line
@@ -389,11 +389,11 @@ class HeadsUpCoordinator @Inject constructor( 
            // First check whether this notification should launch a full screen intent, and

            // launch it if needed.

            val fsiDecision = mNotificationInterruptStateProvider.getFullScreenIntentDecision(entry)

            if (fsiDecision != null && fsiDecision.shouldLaunch) {

            mNotificationInterruptStateProvider.logFullScreenIntentDecision(entry, fsiDecision)

            if (fsiDecision.shouldLaunch) {

                mLaunchFullScreenIntentProvider.launchFullScreenIntent(entry)

            } else if (mFlags.fsiOnDNDUpdate() &&

                fsiDecision.equals(FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)) {

                fsiDecision == FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND) {

                // If DND was the only reason this entry was suppressed, note it for potential

                // reconsideration on later ranking updates.

                addForFSIReconsideration(entry, mSystemClock.currentTimeMillis())
@@ -514,14 +514,24 @@ class HeadsUpCoordinator @Inject constructor( 
                        mNotificationInterruptStateProvider.getFullScreenIntentDecision(entry)

                    if (decision.shouldLaunch) {

                        // Log both the launch of the full screen and also that this was via a

                        // ranking update.

                        mLogger.logEntryUpdatedToFullScreen(entry.key)

                        // ranking update, and finally revoke candidacy for FSI reconsideration

                        mLogger.logEntryUpdatedToFullScreen(entry.key, decision.name)

                        mNotificationInterruptStateProvider.logFullScreenIntentDecision(

                            entry, decision)

                        mLaunchFullScreenIntentProvider.launchFullScreenIntent(entry)

                        mFSIUpdateCandidates.remove(entry.key)



                        // if we launch the FSI then this is no longer a candidate for HUN

                        continue

                    } else if (decision == FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND) {

                        // decision has not changed; no need to log

                    } else {

                        // some other condition is now blocking FSI; log that and revoke candidacy

                        // for FSI reconsideration

                        mLogger.logEntryDisqualifiedFromFullScreen(entry.key, decision.name)

                        mNotificationInterruptStateProvider.logFullScreenIntentDecision(

                            entry, decision)

                        mFSIUpdateCandidates.remove(entry.key)

                    }

                }

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinatorLogger.kt

+12 −2
Original line number Diff line number Diff line
@@ -70,11 +70,21 @@ class HeadsUpCoordinatorLogger constructor( 
        })

    }



    fun logEntryUpdatedToFullScreen(key: String) {

    fun logEntryUpdatedToFullScreen(key: String, reason: String) {

        buffer.log(TAG, LogLevel.DEBUG, {

            str1 = key

            str2 = reason

        }, {

            "updating entry to launch full screen intent: $str1 because $str2"

        })

    }



    fun logEntryDisqualifiedFromFullScreen(key: String, reason: String) {

        buffer.log(TAG, LogLevel.DEBUG, {

            str1 = key

            str2 = reason

        }, {

            "updating entry to launch full screen intent: $str1"

            "updated entry no longer qualifies for full screen intent: $str1 because $str2"

        })

    }

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

+4 −1
Original line number Diff line number Diff line
@@ -16,6 +16,8 @@ 


package com.android.systemui.statusbar.notification.interruption;



import androidx.annotation.NonNull;



import com.android.systemui.statusbar.notification.collection.NotificationEntry;



/**
@@ -153,7 +155,8 @@ public interface NotificationInterruptStateProvider { 
     * @param entry the entry to evaluate

     * @return FullScreenIntentDecision representing the decision for whether to show the intent

     */

    FullScreenIntentDecision getFullScreenIntentDecision(NotificationEntry entry);

    @NonNull

    FullScreenIntentDecision getFullScreenIntentDecision(@NonNull NotificationEntry entry);



    /**

     * Write the full screen launch decision for the given entry to logs.

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

+15 −34
Original line number Diff line number Diff line
@@ -35,6 +35,8 @@ import android.service.dreams.IDreamManager; 
import android.service.notification.StatusBarNotification;

import android.util.Log;



import androidx.annotation.NonNull;



import com.android.internal.annotations.VisibleForTesting;

import com.android.internal.logging.UiEvent;

import com.android.internal.logging.UiEventLogger;
@@ -232,6 +234,7 @@ public class NotificationInterruptStateProviderImpl implements NotificationInter 
    // suppressor.

    //

    // If the entry was not suppressed by DND, just returns the given decision.

    @NonNull

    private FullScreenIntentDecision getDecisionGivenSuppression(FullScreenIntentDecision decision,

            boolean suppressedByDND) {

        if (suppressedByDND) {
@@ -243,7 +246,7 @@ public class NotificationInterruptStateProviderImpl implements NotificationInter 
    }



    @Override

    public FullScreenIntentDecision getFullScreenIntentDecision(NotificationEntry entry) {

    public FullScreenIntentDecision getFullScreenIntentDecision(@NonNull NotificationEntry entry) {

        if (entry.getSbn().getNotification().fullScreenIntent == null) {

            if (entry.isStickyAndNotDemoted()) {

                return FullScreenIntentDecision.NO_FSI_SHOW_STICKY_HUN;
@@ -336,52 +339,30 @@ public class NotificationInterruptStateProviderImpl implements NotificationInter 
        final int uid = entry.getSbn().getUid();

        final String packageName = entry.getSbn().getPackageName();

        switch (decision) {

            case NO_FSI_SHOW_STICKY_HUN:

                mLogger.logNoFullscreen(entry, "Permission denied, show sticky HUN");

                return;

            case NO_FULL_SCREEN_INTENT:

                return;

            case NO_FSI_SUPPRESSED_BY_DND:

            case NO_FSI_SUPPRESSED_ONLY_BY_DND:

                mLogger.logNoFullscreen(entry, "Suppressed by DND");

                return;

            case NO_FSI_NOT_IMPORTANT_ENOUGH:

                mLogger.logNoFullscreen(entry, "Not important enough");

                // explicitly prevent logging for this (frequent) case

                return;

            case NO_FSI_SUPPRESSIVE_GROUP_ALERT_BEHAVIOR:

                android.util.EventLog.writeEvent(0x534e4554, "231322873", uid,

                        "groupAlertBehavior");

                mUiEventLogger.log(FSI_SUPPRESSED_SUPPRESSIVE_GROUP_ALERT_BEHAVIOR, uid,

                        packageName);

                mLogger.logNoFullscreenWarning(entry, "GroupAlertBehavior will prevent HUN");

                return;

            case FSI_DEVICE_NOT_INTERACTIVE:

                mLogger.logFullscreen(entry, "Device is not interactive");

                return;

            case FSI_DEVICE_IS_DREAMING:

                mLogger.logFullscreen(entry, "Device is dreaming");

                return;

            case FSI_KEYGUARD_SHOWING:

                mLogger.logFullscreen(entry, "Keyguard is showing");

                return;

            case NO_FSI_EXPECTED_TO_HUN:

                mLogger.logNoFullscreen(entry, "Expected to HUN");

                return;

            case FSI_KEYGUARD_OCCLUDED:

                mLogger.logFullscreen(entry,

                        "Expected not to HUN while keyguard occluded");

                return;

            case FSI_LOCKED_SHADE:

                mLogger.logFullscreen(entry, "Keyguard is showing and not occluded");

                mLogger.logNoFullscreenWarning(entry,

                        decision + ": GroupAlertBehavior will prevent HUN");

                return;

            case NO_FSI_NO_HUN_OR_KEYGUARD:

                android.util.EventLog.writeEvent(0x534e4554, "231322873", uid,

                        "no hun or keyguard");

                mUiEventLogger.log(FSI_SUPPRESSED_NO_HUN_OR_KEYGUARD, uid, packageName);

                mLogger.logNoFullscreenWarning(entry, "Expected not to HUN while not on keyguard");

                mLogger.logNoFullscreenWarning(entry,

                        decision + ": Expected not to HUN while not on keyguard");

                return;

            case FSI_EXPECTED_NOT_TO_HUN:

                mLogger.logFullscreen(entry, "Expected not to HUN");

            default:

                if (decision.shouldLaunch) {

                    mLogger.logFullscreen(entry, decision.name());

                } else {

                    mLogger.logNoFullscreen(entry, decision.name());

                }

        }

    }

packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/HeadsUpCoordinatorTest.kt

+157 −4
Original line number Diff line number Diff line
@@ -22,6 +22,7 @@ import android.testing.TestableLooper.RunWithLooper 
import androidx.test.filters.SmallTest

import com.android.systemui.SysuiTestCase

import com.android.systemui.dump.logcatLogBuffer

import com.android.systemui.flags.Flags

import com.android.systemui.statusbar.NotificationRemoteInputManager

import com.android.systemui.statusbar.notification.NotifPipelineFlags

import com.android.systemui.statusbar.notification.collection.GroupEntryBuilder
@@ -58,6 +59,7 @@ import org.junit.Before 
import org.junit.Test

import org.junit.runner.RunWith

import org.mockito.ArgumentMatchers.anyString

import org.mockito.BDDMockito.clearInvocations

import org.mockito.BDDMockito.given

import org.mockito.Mockito.never

import org.mockito.Mockito.times
@@ -166,6 +168,12 @@ class HeadsUpCoordinatorTest : SysuiTestCase() { 
        mGroupChild1 = mHelper.createChildNotification(GROUP_ALERT_ALL, 1, "child", 350)

        mGroupChild2 = mHelper.createChildNotification(GROUP_ALERT_ALL, 2, "child", 250)

        mGroupChild3 = mHelper.createChildNotification(GROUP_ALERT_ALL, 3, "child", 150)



        // Set the default FSI decision

        setShouldFullScreen(any(), FullScreenIntentDecision.NO_FULL_SCREEN_INTENT)



        // Run tests with default feature flag state

        whenever(mFlags.fsiOnDNDUpdate()).thenReturn(Flags.FSI_ON_DND_UPDATE.default)

    }



    @Test
@@ -809,6 +817,39 @@ class HeadsUpCoordinatorTest : SysuiTestCase() { 
        verify(mHeadsUpManager, never()).showNotification(any())

    }



    @Test

    fun onEntryAdded_whenLaunchingFSI_doesLogDecision() {

        // GIVEN A new notification can FSI

        setShouldFullScreen(mEntry, FullScreenIntentDecision.FSI_DEVICE_NOT_INTERACTIVE)

        mCollectionListener.onEntryAdded(mEntry)



        verify(mLaunchFullScreenIntentProvider).launchFullScreenIntent(mEntry)

        verify(mNotificationInterruptStateProvider).logFullScreenIntentDecision(

                mEntry, FullScreenIntentDecision.FSI_DEVICE_NOT_INTERACTIVE)

    }



    @Test

    fun onEntryAdded_whenNotLaunchingFSI_doesLogDecision() {

        // GIVEN A new notification can't FSI

        setShouldFullScreen(mEntry, FullScreenIntentDecision.NO_FULL_SCREEN_INTENT)

        mCollectionListener.onEntryAdded(mEntry)



        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())

        verify(mNotificationInterruptStateProvider).logFullScreenIntentDecision(

                mEntry, FullScreenIntentDecision.NO_FULL_SCREEN_INTENT)

    }



    @Test

    fun onEntryAdded_whenNotLaunchingFSIBecauseOfDnd_doesLogDecision() {

        // GIVEN A new notification can't FSI because of DND

        setShouldFullScreen(mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        mCollectionListener.onEntryAdded(mEntry)



        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())

        verify(mNotificationInterruptStateProvider).logFullScreenIntentDecision(

                mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

    }



    @Test

    fun testOnRankingApplied_noFSIOnUpdateWhenFlagOff() {

        // Ensure the feature flag is off
@@ -818,13 +859,22 @@ class HeadsUpCoordinatorTest : SysuiTestCase() { 
        setShouldFullScreen(mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        mCollectionListener.onEntryAdded(mEntry)



        // Verify that this causes a log

        verify(mNotificationInterruptStateProvider).logFullScreenIntentDecision(

                mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        clearInvocations(mNotificationInterruptStateProvider)



        // and it is then updated to allow full screen

        setShouldFullScreen(mEntry, FullScreenIntentDecision.FSI_DEVICE_NOT_INTERACTIVE)

        whenever(mNotifPipeline.allNotifs).thenReturn(listOf(mEntry))

        mCollectionListener.onRankingApplied()



        // THEN it should not full screen because the feature is off

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(mEntry)

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())



        // VERIFY that no additional logging happens either

        verify(mNotificationInterruptStateProvider, never())

                .logFullScreenIntentDecision(any(), any())

    }



    @Test
@@ -836,8 +886,11 @@ class HeadsUpCoordinatorTest : SysuiTestCase() { 
        setShouldFullScreen(mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        mCollectionListener.onEntryAdded(mEntry)



        // at this point, it should not have full screened

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(mEntry)

        // at this point, it should not have full screened, but should have logged

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())

        verify(mNotificationInterruptStateProvider).logFullScreenIntentDecision(mEntry,

                FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        clearInvocations(mNotificationInterruptStateProvider)



        // and it is then updated to allow full screen AND HUN

        setShouldFullScreen(mEntry, FullScreenIntentDecision.FSI_DEVICE_NOT_INTERACTIVE)
@@ -847,10 +900,110 @@ class HeadsUpCoordinatorTest : SysuiTestCase() { 
        mBeforeTransformGroupsListener.onBeforeTransformGroups(listOf(mEntry))

        mBeforeFinalizeFilterListener.onBeforeFinalizeFilter(listOf(mEntry))



        // THEN it should full screen but it should NOT HUN

        // THEN it should full screen and log but it should NOT HUN

        verify(mLaunchFullScreenIntentProvider).launchFullScreenIntent(mEntry)

        verify(mHeadsUpViewBinder, never()).bindHeadsUpView(any(), any())

        verify(mHeadsUpManager, never()).showNotification(any())

        verify(mNotificationInterruptStateProvider).logFullScreenIntentDecision(mEntry,

                FullScreenIntentDecision.FSI_DEVICE_NOT_INTERACTIVE)

        clearInvocations(mNotificationInterruptStateProvider)



        // WHEN ranking updates again and the pipeline reruns

        clearInvocations(mLaunchFullScreenIntentProvider)

        mCollectionListener.onRankingApplied()

        mBeforeTransformGroupsListener.onBeforeTransformGroups(listOf(mEntry))

        mBeforeFinalizeFilterListener.onBeforeFinalizeFilter(listOf(mEntry))



        // VERIFY that the FSI does not launch again or log

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())

        verify(mNotificationInterruptStateProvider, never())

                .logFullScreenIntentDecision(any(), any())

    }



    @Test

    fun testOnRankingApplied_withOnlyDndSuppressionAllowsFsiLater() {

        // Turn on the feature

        whenever(mFlags.fsiOnDNDUpdate()).thenReturn(true)



        // GIVEN that mEntry was previously suppressed from full-screen only by DND

        setShouldFullScreen(mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        mCollectionListener.onEntryAdded(mEntry)



        // at this point, it should not have full screened, but should have logged

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())

        verify(mNotificationInterruptStateProvider).logFullScreenIntentDecision(mEntry,

                FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        clearInvocations(mNotificationInterruptStateProvider)



        // ranking is applied with only DND blocking FSI

        setShouldFullScreen(mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        mCollectionListener.onRankingApplied()

        mBeforeTransformGroupsListener.onBeforeTransformGroups(listOf(mEntry))

        mBeforeFinalizeFilterListener.onBeforeFinalizeFilter(listOf(mEntry))



        // THEN it should still not yet full screen or HUN

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())

        verify(mHeadsUpViewBinder, never()).bindHeadsUpView(any(), any())

        verify(mHeadsUpManager, never()).showNotification(any())



        // Same decision as before; is not logged

        verify(mNotificationInterruptStateProvider, never())

                .logFullScreenIntentDecision(any(), any())

        clearInvocations(mNotificationInterruptStateProvider)



        // and it is then updated to allow full screen AND HUN

        setShouldFullScreen(mEntry, FullScreenIntentDecision.FSI_DEVICE_NOT_INTERACTIVE)

        setShouldHeadsUp(mEntry)

        whenever(mNotifPipeline.allNotifs).thenReturn(listOf(mEntry))

        mCollectionListener.onRankingApplied()

        mBeforeTransformGroupsListener.onBeforeTransformGroups(listOf(mEntry))

        mBeforeFinalizeFilterListener.onBeforeFinalizeFilter(listOf(mEntry))



        // THEN it should full screen and log but it should NOT HUN

        verify(mLaunchFullScreenIntentProvider).launchFullScreenIntent(mEntry)

        verify(mHeadsUpViewBinder, never()).bindHeadsUpView(any(), any())

        verify(mHeadsUpManager, never()).showNotification(any())

        verify(mNotificationInterruptStateProvider).logFullScreenIntentDecision(mEntry,

                FullScreenIntentDecision.FSI_DEVICE_NOT_INTERACTIVE)

        clearInvocations(mNotificationInterruptStateProvider)

    }



    @Test

    fun testOnRankingApplied_newNonFullScreenAnswerInvalidatesCandidate() {

        // Turn on the feature

        whenever(mFlags.fsiOnDNDUpdate()).thenReturn(true)



        // GIVEN that mEntry was previously suppressed from full-screen only by DND

        whenever(mNotifPipeline.allNotifs).thenReturn(listOf(mEntry))

        setShouldFullScreen(mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_ONLY_BY_DND)

        mCollectionListener.onEntryAdded(mEntry)



        // at this point, it should not have full screened

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(mEntry)



        // now some other condition blocks FSI in addition to DND

        setShouldFullScreen(mEntry, FullScreenIntentDecision.NO_FSI_SUPPRESSED_BY_DND)

        mCollectionListener.onRankingApplied()

        mBeforeTransformGroupsListener.onBeforeTransformGroups(listOf(mEntry))

        mBeforeFinalizeFilterListener.onBeforeFinalizeFilter(listOf(mEntry))



        // THEN it should NOT full screen or HUN

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())

        verify(mHeadsUpViewBinder, never()).bindHeadsUpView(any(), any())

        verify(mHeadsUpManager, never()).showNotification(any())



        // NOW the DND logic changes and FSI and HUN are available

        clearInvocations(mLaunchFullScreenIntentProvider)

        setShouldFullScreen(mEntry, FullScreenIntentDecision.FSI_DEVICE_NOT_INTERACTIVE)

        setShouldHeadsUp(mEntry)

        mCollectionListener.onRankingApplied()

        mBeforeTransformGroupsListener.onBeforeTransformGroups(listOf(mEntry))

        mBeforeFinalizeFilterListener.onBeforeFinalizeFilter(listOf(mEntry))



        // VERIFY that the FSI didn't happen, but that we do HUN

        verify(mLaunchFullScreenIntentProvider, never()).launchFullScreenIntent(any())

        finishBind(mEntry)

        verify(mHeadsUpManager).showNotification(mEntry)

    }



    @Test