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

Commit 7f94e9fe authored by Jeff DeCew's avatar Jeff DeCew
Browse files

Promote FeatureFlag best practices in notifications code & tests 

* Introduces a ViewRefactorFlag object which:
  * Avoids bugs from boolean flag fields in views being uninitialized
  * Provides helpers for asserting & checking the flag state
* Removes FeatureFlags from AmbientState
* Changes all uses of FeatureFlags mocks in com.android.systemui.statusbar.notification tests to FakeFeatureFlags
* Changes NotificationTestHelper to use constructor-provided FakeFeatureFlags; removes setter

Fixes: 290365128
Test: atest SystemUITests
Change-Id: Id16e9f59b800601dc04892477ede748c6ea2a8d3
parent c44cfb9c

packages/SystemUI/src/com/android/systemui/flags/ViewRefactorFlag.kt

0 → 100644
+99 −0
Original line number Diff line number Diff line 
/*

 * Copyright (C) 2023 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.flags



import android.util.Log

import com.android.systemui.Dependency



/**

 * This class promotes best practices for flag guarding System UI view refactors.

 * * [isEnabled] allows changing an implementation.

 * * [assertDisabled] allows authors to flag code as being "dead" when the flag gets enabled and

 *   ensure that it is not being invoked accidentally in the post-flag refactor.

 * * [expectEnabled] allows authors to guard new code with a "safe" alternative when invoked on

 *   flag-disabled builds, but with a check that should crash eng builds or tests when the

 *   expectation is violated.

 *

 * The constructors prefer that you provide a [FeatureFlags] instance, but does not require it,

 * falling back to [Dependency.get]. This fallback should ONLY be used to flag-guard code changes

 * inside views where injecting flag values after initialization can be error-prone.

 */

class ViewRefactorFlag

private constructor(

    private val injectedFlags: FeatureFlags?,

    private val flag: BooleanFlag,

    private val readFlagValue: (FeatureFlags) -> Boolean

) {

    @JvmOverloads

    constructor(

        flags: FeatureFlags? = null,

        flag: UnreleasedFlag

    ) : this(flags, flag, { it.isEnabled(flag) })



    @JvmOverloads

    constructor(

        flags: FeatureFlags? = null,

        flag: ReleasedFlag

    ) : this(flags, flag, { it.isEnabled(flag) })



    /** Whether the flag is enabled. Called to switch between an old behavior and a new behavior. */

    val isEnabled by lazy {

        @Suppress("DEPRECATION")

        val featureFlags = injectedFlags ?: Dependency.get(FeatureFlags::class.java)

        readFlagValue(featureFlags)

    }



    /**

     * Called to ensure code is only run when the flag is disabled. This will throw an exception if

     * the flag is enabled to ensure that the refactor author catches issues in testing.

     *

     * Example usage:

     * ```

     * public void setController(NotificationShelfController notificationShelfController) {

     *     mShelfRefactor.assertDisabled();

     *     mController = notificationShelfController;

     * }

     * ````

     */

    fun assertDisabled() = check(!isEnabled) { "Code path not supported when $flag is enabled." }



    /**

     * Called to ensure code is only run when the flag is enabled. This protects users from the

     * unintended behaviors caused by accidentally running new logic, while also crashing on an eng

     * build to ensure that the refactor author catches issues in testing.

     *

     * Example usage:

     * ```

     * public void setShelfIcons(NotificationIconContainer icons) {

     *     if (mShelfRefactor.expectEnabled()) {

     *         mShelfIcons = icons;

     *     }

     * }

     * ```

     */

    fun expectEnabled(): Boolean {

        if (!isEnabled) {

            val message = "Code path not supported when $flag is disabled."

            Log.wtf(TAG, message, Exception(message))

        }

        return isEnabled

    }



    private companion object {

        private const val TAG = "ViewRefactorFlag"

    }

}

packages/SystemUI/src/com/android/systemui/statusbar/LegacyNotificationShelfControllerImpl.java

+0 −2
Original line number Diff line number Diff line
@@ -19,7 +19,6 @@ package com.android.systemui.statusbar; 
import android.view.View;



import com.android.systemui.flags.FeatureFlags;

import com.android.systemui.flags.Flags;

import com.android.systemui.statusbar.notification.row.ActivatableNotificationViewController;

import com.android.systemui.statusbar.notification.row.dagger.NotificationRowScope;

import com.android.systemui.statusbar.notification.stack.AmbientState;
@@ -52,7 +51,6 @@ public class LegacyNotificationShelfControllerImpl implements NotificationShelfC 
        mActivatableNotificationViewController = activatableNotificationViewController;

        mKeyguardBypassController = keyguardBypassController;

        mStatusBarStateController = statusBarStateController;

        mView.setSensitiveRevealAnimEnabled(featureFlags.isEnabled(Flags.SENSITIVE_REVEAL_ANIM));

        mOnAttachStateChangeListener = new View.OnAttachStateChangeListener() {

            @Override

            public void onViewAttachedToWindow(View v) {

packages/SystemUI/src/com/android/systemui/statusbar/NotificationShelf.java

+25 −48
Original line number Diff line number Diff line
@@ -24,7 +24,6 @@ import android.content.res.Resources; 
import android.graphics.Rect;

import android.util.AttributeSet;

import android.util.IndentingPrintWriter;

import android.util.Log;

import android.util.MathUtils;

import android.view.View;

import android.view.ViewGroup;
@@ -40,6 +39,8 @@ import com.android.internal.annotations.VisibleForTesting; 
import com.android.internal.policy.SystemBarUtils;

import com.android.systemui.R;

import com.android.systemui.animation.ShadeInterpolation;

import com.android.systemui.flags.Flags;

import com.android.systemui.flags.ViewRefactorFlag;

import com.android.systemui.plugins.statusbar.StatusBarStateController.StateListener;

import com.android.systemui.shade.transition.LargeScreenShadeInterpolator;

import com.android.systemui.statusbar.notification.NotificationUtils;
@@ -95,8 +96,10 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    private float mCornerAnimationDistance;

    private NotificationShelfController mController;

    private float mActualWidth = -1;

    private boolean mSensitiveRevealAnimEnabled;

    private boolean mShelfRefactorFlagEnabled;

    private final ViewRefactorFlag mSensitiveRevealAnim =

            new ViewRefactorFlag(Flags.SENSITIVE_REVEAL_ANIM);

    private final ViewRefactorFlag mShelfRefactor =

            new ViewRefactorFlag(Flags.NOTIFICATION_SHELF_REFACTOR);

    private boolean mCanModifyColorOfNotifications;

    private boolean mCanInteract;

    private NotificationStackScrollLayout mHostLayout;
@@ -130,7 +133,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 


    public void bind(AmbientState ambientState,

                     NotificationStackScrollLayoutController hostLayoutController) {

        assertRefactorFlagDisabled();

        mShelfRefactor.assertDisabled();

        mAmbientState = ambientState;

        mHostLayoutController = hostLayoutController;

        hostLayoutController.setOnNotificationRemovedListener((child, isTransferInProgress) -> {
@@ -140,7 +143,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 


    public void bind(AmbientState ambientState, NotificationStackScrollLayout hostLayout,

            NotificationRoundnessManager roundnessManager) {

        if (!checkRefactorFlagEnabled()) return;

        if (!mShelfRefactor.expectEnabled()) return;

        mAmbientState = ambientState;

        mHostLayout = hostLayout;

        mRoundnessManager = roundnessManager;
@@ -268,7 +271,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
        }



        final float stackEnd = ambientState.getStackY() + ambientState.getStackHeight();

        if (mSensitiveRevealAnimEnabled && viewState.hidden) {

        if (mSensitiveRevealAnim.isEnabled() && viewState.hidden) {

            // if the shelf is hidden, position it at the end of the stack (plus the clip

            // padding), such that when it appears animated, it will smoothly move in from the

            // bottom, without jump cutting any notifications
@@ -279,7 +282,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private int getSpeedBumpIndex() {

        if (mShelfRefactorFlagEnabled) {

        if (mShelfRefactor.isEnabled()) {

            return mHostLayout.getSpeedBumpIndex();

        } else {

            return mHostLayoutController.getSpeedBumpIndex();
@@ -413,7 +416,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
                    expandingAnimated, isLastChild, shelfClipStart);



            // TODO(b/172289889) scale mPaddingBetweenElements with expansion amount

            if ((!mSensitiveRevealAnimEnabled && ((isLastChild && !child.isInShelf())

            if ((!mSensitiveRevealAnim.isEnabled() && ((isLastChild && !child.isInShelf())

                    || backgroundForceHidden)) || aboveShelf) {

                notificationClipEnd = shelfStart + getIntrinsicHeight();

            } else {
@@ -462,7 +465,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
                // if the shelf is visible, but if the shelf is hidden, it causes incorrect curling.

                // notificationClipEnd handles the discrepancy between a visible and hidden shelf,

                // so we use that when on the keyguard (and while animating away) to reduce curling.

                final float keyguardSafeShelfStart = !mSensitiveRevealAnimEnabled

                final float keyguardSafeShelfStart = !mSensitiveRevealAnim.isEnabled()

                        && mAmbientState.isOnKeyguard() ? notificationClipEnd : shelfStart;

                updateCornerRoundnessOnScroll(anv, viewStart, keyguardSafeShelfStart);

            }
@@ -504,7 +507,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private ExpandableView getHostLayoutChildAt(int index) {

        if (mShelfRefactorFlagEnabled) {

        if (mShelfRefactor.isEnabled()) {

            return (ExpandableView) mHostLayout.getChildAt(index);

        } else {

            return mHostLayoutController.getChildAt(index);
@@ -512,7 +515,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private int getHostLayoutChildCount() {

        if (mShelfRefactorFlagEnabled) {

        if (mShelfRefactor.isEnabled()) {

            return mHostLayout.getChildCount();

        } else {

            return mHostLayoutController.getChildCount();
@@ -520,7 +523,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private boolean canModifyColorOfNotifications() {

        if (mShelfRefactorFlagEnabled) {

        if (mShelfRefactor.isEnabled()) {

            return mCanModifyColorOfNotifications && mAmbientState.isShadeExpanded();

        } else {

            return mController.canModifyColorOfNotifications();
@@ -583,7 +586,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private boolean isViewAffectedBySwipe(ExpandableView expandableView) {

        if (!mShelfRefactorFlagEnabled) {

        if (!mShelfRefactor.isEnabled()) {

            return mHostLayoutController.isViewAffectedBySwipe(expandableView);

        } else {

            return mRoundnessManager.isViewAffectedBySwipe(expandableView);
@@ -607,7 +610,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private View getHostLayoutTransientView(int index) {

        if (mShelfRefactorFlagEnabled) {

        if (mShelfRefactor.isEnabled()) {

            return mHostLayout.getTransientView(index);

        } else {

            return mHostLayoutController.getTransientView(index);
@@ -615,7 +618,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private int getHostLayoutTransientViewCount() {

        if (mShelfRefactorFlagEnabled) {

        if (mShelfRefactor.isEnabled()) {

            return mHostLayout.getTransientViewCount();

        } else {

            return mHostLayoutController.getTransientViewCount();
@@ -961,7 +964,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 


    @Override

    public void onStateChanged(int newState) {

        assertRefactorFlagDisabled();

        mShelfRefactor.assertDisabled();

        mStatusBarState = newState;

        updateInteractiveness();

    }
@@ -975,7 +978,7 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private boolean canInteract() {

        if (mShelfRefactorFlagEnabled) {

        if (mShelfRefactor.isEnabled()) {

            return mCanInteract;

        } else {

            return mStatusBarState == StatusBarState.KEYGUARD;
@@ -1018,32 +1021,18 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
        return false;

    }



    private void assertRefactorFlagDisabled() {

        if (mShelfRefactorFlagEnabled) {

            NotificationShelfController.throwIllegalFlagStateError(false);

        }

    }



    private boolean checkRefactorFlagEnabled() {

        if (!mShelfRefactorFlagEnabled) {

            Log.wtf(TAG,

                    "Code path not supported when Flags.NOTIFICATION_SHELF_REFACTOR is disabled.");

        }

        return mShelfRefactorFlagEnabled;

    }



    public void setController(NotificationShelfController notificationShelfController) {

        assertRefactorFlagDisabled();

        mShelfRefactor.assertDisabled();

        mController = notificationShelfController;

    }



    public void setCanModifyColorOfNotifications(boolean canModifyColorOfNotifications) {

        if (!checkRefactorFlagEnabled()) return;

        if (!mShelfRefactor.expectEnabled()) return;

        mCanModifyColorOfNotifications = canModifyColorOfNotifications;

    }



    public void setCanInteract(boolean canInteract) {

        if (!checkRefactorFlagEnabled()) return;

        if (!mShelfRefactor.expectEnabled()) return;

        mCanInteract = canInteract;

        updateInteractiveness();

    }
@@ -1053,27 +1042,15 @@ public class NotificationShelf extends ActivatableNotificationView implements St 
    }



    private int getIndexOfViewInHostLayout(ExpandableView child) {

        if (mShelfRefactorFlagEnabled) {

        if (mShelfRefactor.isEnabled()) {

            return mHostLayout.indexOfChild(child);

        } else {

            return mHostLayoutController.indexOfChild(child);

        }

    }



    /**

     * Set whether the sensitive reveal animation feature flag is enabled

     * @param enabled true if enabled

     */

    public void setSensitiveRevealAnimEnabled(boolean enabled) {

        mSensitiveRevealAnimEnabled = enabled;

    }



    public void setRefactorFlagEnabled(boolean enabled) {

        mShelfRefactorFlagEnabled = enabled;

    }



    public void requestRoundnessResetFor(ExpandableView child) {

        if (!checkRefactorFlagEnabled()) return;

        if (!mShelfRefactor.expectEnabled()) return;

        child.requestRoundnessReset(SHELF_SCROLL);

    }

packages/SystemUI/src/com/android/systemui/statusbar/NotificationShelfController.kt

+0 −28
Original line number Diff line number Diff line
@@ -16,11 +16,8 @@ 


package com.android.systemui.statusbar



import android.util.Log

import android.view.View

import android.view.View.OnClickListener

import com.android.systemui.flags.FeatureFlags

import com.android.systemui.flags.Flags

import com.android.systemui.statusbar.notification.row.ExpandableView

import com.android.systemui.statusbar.notification.stack.AmbientState

import com.android.systemui.statusbar.notification.stack.NotificationStackScrollLayout
@@ -49,29 +46,4 @@ interface NotificationShelfController { 


    /** @see View.setOnClickListener */

    fun setOnClickListener(listener: OnClickListener)



    companion object {

        @JvmStatic

        fun assertRefactorFlagDisabled(featureFlags: FeatureFlags) {

            if (featureFlags.isEnabled(Flags.NOTIFICATION_SHELF_REFACTOR)) {

                throwIllegalFlagStateError(expected = false)

            }

        }



        @JvmStatic

        fun checkRefactorFlagEnabled(featureFlags: FeatureFlags): Boolean =

            featureFlags.isEnabled(Flags.NOTIFICATION_SHELF_REFACTOR).also { enabled ->

                if (!enabled) {

                    Log.wtf("NotifShelf", getErrorMessage(expected = true))

                }

            }



        @JvmStatic

        fun throwIllegalFlagStateError(expected: Boolean): Nothing =

            error(getErrorMessage(expected))



        private fun getErrorMessage(expected: Boolean): String =

            "Code path not supported when Flags.NOTIFICATION_SHELF_REFACTOR is " +

                if (expected) "disabled" else "enabled"

    }

}

packages/SystemUI/src/com/android/systemui/statusbar/notification/Roundable.kt

+5 −6
Original line number Diff line number Diff line
@@ -3,10 +3,10 @@ package com.android.systemui.statusbar.notification 
import android.util.FloatProperty

import android.view.View

import androidx.annotation.FloatRange

import com.android.systemui.Dependency

import com.android.systemui.R

import com.android.systemui.flags.FeatureFlags

import com.android.systemui.flags.Flags

import com.android.systemui.flags.ViewRefactorFlag

import com.android.systemui.statusbar.notification.stack.AnimationProperties

import com.android.systemui.statusbar.notification.stack.StackStateAnimator

import kotlin.math.abs
@@ -46,14 +46,14 @@ interface Roundable { 
    @JvmDefault

    val topCornerRadius: Float

        get() =

            if (roundableState.newHeadsUpAnimFlagEnabled) roundableState.topCornerRadius

            if (roundableState.newHeadsUpAnim.isEnabled) roundableState.topCornerRadius

            else topRoundness * maxRadius



    /** Current bottom corner in pixel, based on [bottomRoundness] and [maxRadius] */

    @JvmDefault

    val bottomCornerRadius: Float

        get() =

            if (roundableState.newHeadsUpAnimFlagEnabled) roundableState.bottomCornerRadius

            if (roundableState.newHeadsUpAnim.isEnabled) roundableState.bottomCornerRadius

            else bottomRoundness * maxRadius



    /** Get and update the current radii */
@@ -335,13 +335,12 @@ constructor( 
    internal val targetView: View,

    private val roundable: Roundable,

    maxRadius: Float,

    private val featureFlags: FeatureFlags = Dependency.get(FeatureFlags::class.java)

    featureFlags: FeatureFlags? = null

) {

    internal var maxRadius = maxRadius

        private set



    internal val newHeadsUpAnimFlagEnabled

        get() = featureFlags.isEnabled(Flags.IMPROVED_HUN_ANIMATIONS)

    internal val newHeadsUpAnim = ViewRefactorFlag(featureFlags, Flags.IMPROVED_HUN_ANIMATIONS)



    /** Animatable for top roundness */

    private val topAnimatable = topAnimatable(roundable)