Merge "Remove FeatureFlagReader. Allow resource overlays." into sc-v2-dev

packages/SystemUI/Android.bp

@@ -50,17 +50,6 @@ java_library {
     srcs: ["src/com/android/systemui/EventLogTags.logtags"],
 }
 
-java_library {
-    name: "SystemUI-flags",
-    srcs: [
-        "src/com/android/systemui/flags/Flags.java",
-    ],
-    libs: [
-        "SystemUI-flag-types",
-    ],
-    static_kotlin_stdlib: false,
-}
-
 filegroup {
     name: "ReleaseJavaFiles",
     srcs: [
@@ -126,7 +115,6 @@ android_library {
         "iconloader_base",
         "SystemUI-tags",
         "SystemUI-proto",
-        "SystemUI-flags",
         "monet",
         "dagger2",
         "jsr330",

packages/SystemUI/shared/Android.bp

@@ -45,9 +45,6 @@ android_library {
         ":wm_shell-aidls",
         ":wm_shell_util-sources",
     ],
-    libs: [
-        "SystemUI-flags",
-    ],
     static_libs: [
         "PluginCoreLib",
         "androidx.dynamicanimation_dynamicanimation",
@@ -75,7 +72,6 @@ java_library {
     ],
     static_kotlin_stdlib: false,
     libs: [
-        "SystemUI-flags",
         "androidx.concurrent_concurrent-futures",
     ],
     static_libs: [

packages/SystemUI/shared/src/com/android/systemui/flags/Flag.kt

@@ -22,15 +22,21 @@ import android.os.Parcelable
 interface Flag<T> : Parcelable {
     val id: Int
     val default: T
+    val resourceOverride: Int
 
     override fun describeContents() = 0
+
+    fun hasResourceOverride(): Boolean {
+        return resourceOverride != -1
+    }
 }
 
 // Consider using the "parcelize" kotlin library.
 
 data class BooleanFlag @JvmOverloads constructor(
     override val id: Int,
-    override val default: Boolean = false
+    override val default: Boolean = false,
+    override val resourceOverride: Int = -1
 ) : Flag<Boolean> {
 
     companion object {
@@ -54,7 +60,8 @@ data class BooleanFlag @JvmOverloads constructor(
 
 data class StringFlag @JvmOverloads constructor(
     override val id: Int,
-    override val default: String = ""
+    override val default: String = "",
+    override val resourceOverride: Int = -1
 ) : Flag<String> {
     companion object {
         @JvmField
@@ -77,7 +84,8 @@ data class StringFlag @JvmOverloads constructor(
 
 data class IntFlag @JvmOverloads constructor(
     override val id: Int,
-    override val default: Int = 0
+    override val default: Int = 0,
+    override val resourceOverride: Int = -1
 ) : Flag<Int> {
 
     companion object {
@@ -101,7 +109,8 @@ data class IntFlag @JvmOverloads constructor(
 
 data class LongFlag @JvmOverloads constructor(
     override val id: Int,
-    override val default: Long = 0
+    override val default: Long = 0,
+    override val resourceOverride: Int = -1
 ) : Flag<Long> {
 
     companion object {
@@ -125,7 +134,8 @@ data class LongFlag @JvmOverloads constructor(
 
 data class FloatFlag @JvmOverloads constructor(
     override val id: Int,
-    override val default: Float = 0f
+    override val default: Float = 0f,
+    override val resourceOverride: Int = -1
 ) : Flag<Float> {
 
     companion object {
@@ -149,7 +159,8 @@ data class FloatFlag @JvmOverloads constructor(
 
 data class DoubleFlag @JvmOverloads constructor(
     override val id: Int,
-    override val default: Double = 0.0
+    override val default: Double = 0.0,
+    override val resourceOverride: Int = -1
 ) : Flag<Double> {
 
     companion object {

packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java

@@ -1,175 +0,0 @@
-/*
- * Copyright (C) 2021 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.content.res.Resources;
-import android.util.SparseArray;
-
-import androidx.annotation.BoolRes;
-import androidx.annotation.NonNull;
-import androidx.annotation.Nullable;
-
-import com.android.systemui.Dumpable;
-import com.android.systemui.R;
-import com.android.systemui.dagger.SysUISingleton;
-import com.android.systemui.dagger.qualifiers.Main;
-import com.android.systemui.dump.DumpManager;
-import com.android.systemui.util.wrapper.BuildInfo;
-
-import java.io.FileDescriptor;
-import java.io.PrintWriter;
-import java.util.ArrayList;
-
-import javax.inject.Inject;
-/**
- * Reads and caches feature flags for quick access
- *
- * Feature flags must be defined as boolean resources. For example:t
- *
- * {@code
- *  <bool name="flag_foo_bar_baz">false</bool>
- * }
- *
- * It is strongly recommended that the name of the resource begin with "flag_".
- *
- * Flags can be overridden via adb on development builds. For example, to override the flag from the
- * previous example, do the following:
- *
- * {@code
- *  $ adb shell setprop persist.systemui.flag_foo_bar_baz 1
- * }
- *
- * Note that all storage keys begin with "flag_", even if their associated resId does not.
- *
- * Calls to this class should probably be wrapped by a method in {@link FeatureFlags}.
- */
-@SysUISingleton
-public class FeatureFlagReader implements Dumpable {
-    private final Resources mResources;
-    private final boolean mAreFlagsOverrideable;
-    private final SystemPropertiesHelper mSystemPropertiesHelper;
-    private final SparseArray<CachedFlag> mCachedFlags = new SparseArray<>();
-
-    private final FlagReader mFlagReader;
-
-    @Inject
-    public FeatureFlagReader(
-            @Main Resources resources,
-            BuildInfo build,
-            DumpManager dumpManager,
-            SystemPropertiesHelper systemPropertiesHelper,
-            FlagReader reader) {
-        mResources = resources;
-        mFlagReader = reader;
-        mSystemPropertiesHelper = systemPropertiesHelper;
-        mAreFlagsOverrideable =
-                build.isDebuggable() && mResources.getBoolean(R.bool.are_flags_overrideable);
-        dumpManager.registerDumpable("FeatureFlags", this);
-    }
-
-    boolean isEnabled(BooleanFlag flag) {
-        return mFlagReader.isEnabled(flag.getId(), flag.getDefault());
-    }
-
-    void addListener(FlagReader.Listener listener) {
-        mFlagReader.addListener(listener);
-    }
-
-    void removeListener(FlagReader.Listener listener) {
-        mFlagReader.removeListener(listener);
-    }
-
-    /**
-     * Returns true if the specified feature flag has been enabled.
-     *
-     * @param resId The backing boolean resource that determines the value of the flag. This value
-     *              can be overridden via DeviceConfig on development builds.
-     */
-    public boolean isEnabled(@BoolRes int resId) {
-        synchronized (mCachedFlags) {
-            CachedFlag cachedFlag = mCachedFlags.get(resId);
-
-            if (cachedFlag == null) {
-                String name = resourceIdToFlagName(resId);
-                boolean value = mResources.getBoolean(resId);
-                if (mAreFlagsOverrideable) {
-                    value = mSystemPropertiesHelper.getBoolean(flagNameToStorageKey(name), value);
-                }
-
-                cachedFlag = new CachedFlag(name, value);
-                mCachedFlags.put(resId, cachedFlag);
-            }
-
-            return cachedFlag.value;
-        }
-    }
-
-    private String resourceIdToFlagName(@BoolRes int resId) {
-        String resName = mResources.getResourceEntryName(resId);
-        if (resName.startsWith(RESNAME_PREFIX)) {
-            resName = resName.substring(RESNAME_PREFIX.length());
-        }
-        return resName;
-    }
-
-    private String flagNameToStorageKey(String flagName) {
-        if (flagName.startsWith(STORAGE_KEY_PREFIX)) {
-            return flagName;
-        } else {
-            return STORAGE_KEY_PREFIX + flagName;
-        }
-    }
-
-    @Nullable
-    private String storageKeyToFlagName(String configName) {
-        if (configName.startsWith(STORAGE_KEY_PREFIX)) {
-            return configName.substring(STORAGE_KEY_PREFIX.length());
-        } else {
-            return null;
-        }
-    }
-
-    @Override
-    public void dump(@NonNull FileDescriptor fd, @NonNull PrintWriter pw, @NonNull String[] args) {
-        ArrayList<String> flagStrings = new ArrayList<>(mCachedFlags.size());
-        for (int i = 0; i < mCachedFlags.size(); i++) {
-            int key = mCachedFlags.keyAt(i);
-            // get the object by the key.
-            CachedFlag flag = mCachedFlags.get(key);
-            flagStrings.add("  " + RESNAME_PREFIX + flag.name + ": " + flag.value + "\n");
-        }
-        flagStrings.sort(String.CASE_INSENSITIVE_ORDER);
-        pw.println("AreFlagsOverrideable: " + mAreFlagsOverrideable);
-        pw.println("Cached FeatureFlags:");
-        for (String flagString : flagStrings) {
-            pw.print(flagString);
-        }
-    }
-
-    private static class CachedFlag {
-        public final String name;
-        public final boolean value;
-
-        private CachedFlag(String name, boolean value) {
-            this.name = name;
-            this.value = value;
-        }
-    }
-
-    private static final String STORAGE_KEY_PREFIX = "persist.systemui.flag_";
-    private static final String RESNAME_PREFIX = "flag_";
-}

packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java

@@ -17,13 +17,17 @@
 package com.android.systemui.flags;
 
 import android.content.Context;
+import android.content.res.Resources;
 import android.util.FeatureFlagUtils;
 import android.util.Log;
+import android.util.SparseArray;
 import android.widget.Toast;
 
+import androidx.annotation.BoolRes;
+
 import com.android.internal.annotations.VisibleForTesting;
-import com.android.systemui.R;
 import com.android.systemui.dagger.SysUISingleton;
+import com.android.systemui.dagger.qualifiers.Main;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -39,13 +43,16 @@ import javax.inject.Inject;
  */
 @SysUISingleton
 public class FeatureFlags {
-    private final FeatureFlagReader mFlagReader;
+    private final Resources mResources;
+    private final FlagReader mFlagReader;
     private final Context mContext;
     private final Map<Integer, Flag<?>> mFlagMap = new HashMap<>();
     private final Map<Integer, List<Listener>> mListeners = new HashMap<>();
+    private final SparseArray<Boolean> mCachedFlags = new SparseArray<>();
 
     @Inject
-    public FeatureFlags(FeatureFlagReader flagReader, Context context) {
+    public FeatureFlags(@Main Resources resources, FlagReader flagReader, Context context) {
+        mResources = resources;
         mFlagReader = flagReader;
         mContext = context;
 
@@ -59,7 +66,7 @@ public class FeatureFlags {
     };
 
     @VisibleForTesting
-    void addFlag(Flag flag) {
+    void addFlag(Flag<?> flag) {
         mFlagMap.put(flag.getId(), flag);
     }
 
@@ -68,7 +75,15 @@ public class FeatureFlags {
      * @return The value of the flag.
      */
     public boolean isEnabled(BooleanFlag flag) {
-        return mFlagReader.isEnabled(flag);
+        boolean def = flag.getDefault();
+        if (flag.hasResourceOverride()) {
+            try {
+                def = isEnabledInOverlay(flag.getResourceOverride());
+            } catch (Resources.NotFoundException e) {
+                // no-op
+            }
+        }
+        return mFlagReader.isEnabled(flag.getId(), def);
     }
 
     /**
@@ -118,13 +133,11 @@ public class FeatureFlags {
     }
 
     public boolean isPeopleTileEnabled() {
-        // TODO(b/202860494): different resource overlays have different values.
+        return isEnabled(Flags.PEOPLE_TILE);
-        return mFlagReader.isEnabled(R.bool.flag_conversations);
     }
 
     public boolean isMonetEnabled() {
-        // TODO(b/202860494): used in wallpaper picker. Always true, maybe delete.
+        return isEnabled(Flags.MONET);
-        return mFlagReader.isEnabled(R.bool.flag_monet);
     }
 
     public boolean isPMLiteEnabled() {
@@ -132,8 +145,7 @@ public class FeatureFlags {
     }
 
     public boolean isChargingRippleEnabled() {
-        // TODO(b/202860494): different resource overlays have different values.
+        return isEnabled(Flags.CHARGING_RIPPLE);
-        return mFlagReader.isEnabled(R.bool.flag_charging_ripple);
     }
 
     public boolean isOngoingCallStatusBarChipEnabled() {
@@ -150,8 +162,7 @@ public class FeatureFlags {
     }
 
     public boolean isSmartspaceEnabled() {
-        // TODO(b/202860494): different resource overlays have different values.
+        return isEnabled(Flags.SMARTSPACE);
-        return mFlagReader.isEnabled(R.bool.flag_smartspace);
     }
 
     public boolean isSmartspaceDedupingEnabled() {
@@ -195,6 +206,16 @@ public class FeatureFlags {
         return FeatureFlagUtils.isEnabled(context, FeatureFlagUtils.SETTINGS_PROVIDER_MODEL);
     }
 
+    private boolean isEnabledInOverlay(@BoolRes int resId) {
+        synchronized (mCachedFlags) {
+            if (!mCachedFlags.contains(resId)) {
+                mCachedFlags.put(resId, mResources.getBoolean(resId));
+            }
+
+            return mCachedFlags.get(resId);
+        }
+    }
+
     /** Simple interface for beinga alerted when a specific flag changes value. */
     public interface Listener {
         /** */

packages/SystemUI/src/com/android/systemui/flags/Flags.java

@@ -16,6 +16,8 @@
 
 package com.android.systemui.flags;
 
+import com.android.systemui.R;
+
 import java.lang.reflect.Field;
 import java.util.HashMap;
 import java.util.Map;
@@ -47,7 +49,6 @@ public class Flags {
     public static final BooleanFlag NOTIFICATION_UPDATES =
             new BooleanFlag(102, true);
 
-
     /***************************************/
     // 200 - keyguard/lockscreen
     public static final BooleanFlag KEYGUARD_LAYOUT =
@@ -59,6 +60,9 @@ public class Flags {
     public static final BooleanFlag NEW_UNLOCK_SWIPE_ANIMATION =
             new BooleanFlag(202, true);
 
+    public static final BooleanFlag CHARGING_RIPPLE =
+            new BooleanFlag(203, false, R.bool.flag_charging_ripple);
+
     /***************************************/
     // 300 - power menu
     public static final BooleanFlag POWER_MENU_LITE =
@@ -72,6 +76,9 @@ public class Flags {
     public static final BooleanFlag SMARTSPACE_SHARED_ELEMENT_TRANSITION_ENABLED =
             new BooleanFlag(401, false);
 
+    public static final BooleanFlag SMARTSPACE =
+            new BooleanFlag(402, false, R.bool.flag_smartspace);
+
     /***************************************/
     // 500 - quick settings
     public static final BooleanFlag NEW_USER_SWITCHER =
@@ -80,6 +87,9 @@ public class Flags {
     public static final BooleanFlag COMBINED_QS_HEADERS =
             new BooleanFlag(501, false);
 
+    public static final BooleanFlag PEOPLE_TILE =
+            new BooleanFlag(502, false, R.bool.flag_conversations);
+
     /***************************************/
     // 600- status bar
     public static final BooleanFlag COMBINED_STATUS_BAR_SIGNAL_ICONS =
@@ -96,6 +106,11 @@ public class Flags {
     public static final BooleanFlag ONGOING_CALL_IN_IMMERSIVE_CHIP_TAP =
             new BooleanFlag(702, true);
 
+    /***************************************/
+    // 800 - general visual/theme
+    public static final BooleanFlag MONET =
+            new BooleanFlag(800, true, R.bool.flag_monet);
+
     // Pay no attention to the reflection behind the curtain.
     // ========================== Curtain ==========================
     // |                                                           |

packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java

@@ -1,148 +0,0 @@
-/*
- * Copyright (C) 2021 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 static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.ArgumentMatchers.anyBoolean;
-import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
-
-import android.content.res.Resources;
-
-import androidx.annotation.BoolRes;
-import androidx.test.filters.SmallTest;
-
-import com.android.systemui.R;
-import com.android.systemui.SysuiTestCase;
-import com.android.systemui.dump.DumpManager;
-import com.android.systemui.shared.plugins.PluginManager;
-import com.android.systemui.util.wrapper.BuildInfo;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.mockito.Mock;
-import org.mockito.MockitoAnnotations;
-
-@SmallTest
-public class FeatureFlagReaderTest extends SysuiTestCase {
-    @Mock private Resources mResources;
-    @Mock private BuildInfo mBuildInfo;
-    @Mock private DumpManager mDumpManager;
-    @Mock private SystemPropertiesHelper mSystemPropertiesHelper;
-    @Mock private FlagReader mFlagReader;
-
-    private FeatureFlagReader mReader;
-
-    @Before
-    public void setUp() {
-        MockitoAnnotations.initMocks(this);
-
-        when(mSystemPropertiesHelper.getBoolean(anyString(), anyBoolean()))
-                .thenAnswer(invocation -> invocation.getArgument(1));
-
-        defineFlag(FLAG_RESID_0, false);
-        defineFlag(FLAG_RESID_1, true);
-
-        initialize(true, true);
-    }
-
-    private void initialize(boolean isDebuggable, boolean isOverrideable) {
-        when(mBuildInfo.isDebuggable()).thenReturn(isDebuggable);
-        when(mResources.getBoolean(R.bool.are_flags_overrideable)).thenReturn(isOverrideable);
-        mReader = new FeatureFlagReader(
-                mResources, mBuildInfo, mDumpManager, mSystemPropertiesHelper, mFlagReader);
-    }
-
-    @Test
-    public void testCantOverrideIfNotDebuggable() {
-        // GIVEN that the build is not debuggable
-        initialize(false, true);
-
-        // GIVEN that a flag has been overridden to true
-        overrideFlag(FLAG_RESID_0, true);
-
-        // THEN the flag is still false
-        assertFalse(mReader.isEnabled(FLAG_RESID_0));
-    }
-
-    @Test
-    public void testCantOverrideIfNotOverrideable() {
-        // GIVEN that flags are not overrideable
-        initialize(true, false);
-
-        // GIVEN that a flag has been overridden to true
-        overrideFlag(FLAG_RESID_0, true);
-
-        // THEN the flag is still false
-        assertFalse(mReader.isEnabled(FLAG_RESID_0));
-    }
-
-    @Test
-    public void testReadFlags() {
-        assertFalse(mReader.isEnabled(FLAG_RESID_0));
-        assertTrue(mReader.isEnabled(FLAG_RESID_1));
-    }
-
-    @Test
-    public void testOverrideFlags() {
-        // GIVEN that flags are overridden
-        overrideFlag(FLAG_RESID_0, true);
-        overrideFlag(FLAG_RESID_1, false);
-
-        // THEN the reader returns the overridden values
-        assertTrue(mReader.isEnabled(FLAG_RESID_0));
-        assertFalse(mReader.isEnabled(FLAG_RESID_1));
-    }
-
-    @Test
-    public void testThatFlagReadsAreCached() {
-        // GIVEN that a flag is overridden
-        overrideFlag(FLAG_RESID_0, true);
-
-        // WHEN the flag is queried many times
-        mReader.isEnabled(FLAG_RESID_0);
-        mReader.isEnabled(FLAG_RESID_0);
-        mReader.isEnabled(FLAG_RESID_0);
-        mReader.isEnabled(FLAG_RESID_0);
-
-        // THEN the underlying resource and override are only queried once
-        verify(mResources, times(1)).getBoolean(FLAG_RESID_0);
-        verify(mSystemPropertiesHelper, times(1))
-                .getBoolean(fakeStorageKey(FLAG_RESID_0), false);
-    }
-
-    private void defineFlag(int resId, boolean value) {
-        when(mResources.getBoolean(resId)).thenReturn(value);
-        when(mResources.getResourceEntryName(resId)).thenReturn(fakeStorageKey(resId));
-    }
-
-    private void overrideFlag(int resId, boolean value) {
-        when(mSystemPropertiesHelper.getBoolean(eq(fakeStorageKey(resId)), anyBoolean()))
-                .thenReturn(value);
-    }
-
-    private String fakeStorageKey(@BoolRes int resId) {
-        return "persist.systemui.flag_testname_" + resId;
-    }
-
-    private static final int FLAG_RESID_0 = 47;
-    private static final int FLAG_RESID_1 = 48;
-}

packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java

@@ -18,7 +18,12 @@ package com.android.systemui.flags;
 
 import static com.google.common.truth.Truth.assertThat;
 
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import android.content.res.Resources;
 
 import androidx.test.filters.SmallTest;
 
@@ -29,11 +34,13 @@ import org.junit.Test;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
+import org.mockito.stubbing.Answer;
 
 @SmallTest
 public class FeatureFlagsTest extends SysuiTestCase {
 
-    @Mock FeatureFlagReader mFeatureFlagReader;
+    @Mock Resources mResources;
+    @Mock FlagReader mFeatureFlagReader;
 
     private FeatureFlags mFeatureFlags;
 
@@ -41,7 +48,10 @@ public class FeatureFlagsTest extends SysuiTestCase {
     public void setup() {
         MockitoAnnotations.initMocks(this);
 
-        mFeatureFlags = new FeatureFlags(mFeatureFlagReader, getContext());
+        when(mFeatureFlagReader.isEnabled(anyInt(), anyBoolean())).thenAnswer(
+                (Answer<Boolean>) invocation -> invocation.getArgument(1));
+
+        mFeatureFlags = new FeatureFlags(mResources, mFeatureFlagReader, getContext());
     }
 
     @Test
@@ -103,6 +113,26 @@ public class FeatureFlagsTest extends SysuiTestCase {
         pluginListener.onFlagChanged(flag.getId());
         // Assert that the change was not triggered
         assertThat(changedFlag[0]).isNull();
+    }
 
+    @Test
+    public void testBooleanDefault() {
+        BooleanFlag flag = new BooleanFlag(1, true);
+
+        mFeatureFlags.addFlag(flag);
+
+        assertThat(mFeatureFlags.isEnabled(flag)).isTrue();
+    }
+
+    @Test
+    public void testBooleanResourceOverlay() {
+        int resourceId = 12;
+        BooleanFlag flag = new BooleanFlag(1, false, resourceId);
+        when(mResources.getBoolean(resourceId)).thenReturn(true);
+        when(mResources.getResourceEntryName(resourceId)).thenReturn("flag");
+
+        mFeatureFlags.addFlag(flag);
+
+        assertThat(mFeatureFlags.isEnabled(flag)).isTrue();
     }
 }