From 5346e090f5ccad448265ed8b98a02bbf5d9f4356 Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Mon, 8 Nov 2021 13:21:37 -0500 Subject: [PATCH] Move contents of FeatureFlags into FeatureFlagManager. The meaningful contents of FeatureFlags only relates to debug builds and has been moved into the debug version of FeatureFlagManager. Bug: 203548872 Test: atest SystemUITests && manual Change-Id: Ifcef0cfe4e2d07daec34dfc7a6cbe2d305b312b5 --- .../com/android/systemui/flags/FlagReader.kt | 5 + .../systemui/flags/FeatureFlagManager.java | 39 ++++- .../systemui/flags/FeatureFlagManager.java | 10 +- .../android/systemui/flags/FeatureFlags.java | 74 +--------- .../flags/FeatureFlagManagerTest.java | 7 +- .../systemui/flags/FeatureFlagsTest.java | 138 ------------------ 6 files changed, 47 insertions(+), 226 deletions(-) delete mode 100644 packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java diff --git a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt index ee6dea5364f49..91a391272be7e 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt @@ -19,6 +19,11 @@ package com.android.systemui.flags * Plugin for loading flag values */ interface FlagReader { + /** Returns a boolean value for the given flag. */ + fun isEnabled(flag: BooleanFlag): Boolean { + return flag.default + } + /** Returns a boolean value for the given flag. */ fun isEnabled(id: Int, def: Boolean): Boolean { return def diff --git a/packages/SystemUI/src-debug/com/android/systemui/flags/FeatureFlagManager.java b/packages/SystemUI/src-debug/com/android/systemui/flags/FeatureFlagManager.java index ef046193473d2..acfa3c84a4baf 100644 --- a/packages/SystemUI/src-debug/com/android/systemui/flags/FeatureFlagManager.java +++ b/packages/SystemUI/src-debug/com/android/systemui/flags/FeatureFlagManager.java @@ -26,13 +26,16 @@ import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; import android.content.IntentFilter; +import android.content.res.Resources; import android.os.Bundle; import android.util.Log; +import androidx.annotation.BoolRes; import androidx.annotation.NonNull; import com.android.systemui.Dumpable; import com.android.systemui.dagger.SysUISingleton; +import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.dump.DumpManager; import com.android.systemui.util.settings.SecureSettings; @@ -62,14 +65,19 @@ public class FeatureFlagManager implements FlagReader, FlagWriter, Dumpable { private final FlagManager mFlagManager; private final SecureSettings mSecureSettings; + private final Resources mResources; private final Map mBooleanFlagCache = new HashMap<>(); @Inject - public FeatureFlagManager(FlagManager flagManager, - SecureSettings secureSettings, Context context, + public FeatureFlagManager( + FlagManager flagManager, + Context context, + SecureSettings secureSettings, + @Main Resources resources, DumpManager dumpManager) { mFlagManager = flagManager; mSecureSettings = secureSettings; + mResources = resources; IntentFilter filter = new IntentFilter(); filter.addAction(ACTION_SET_FLAG); filter.addAction(ACTION_GET_FLAGS); @@ -77,17 +85,32 @@ public class FeatureFlagManager implements FlagReader, FlagWriter, Dumpable { dumpManager.registerDumpable(TAG, this); } - /** Return a {@link BooleanFlag}'s value. */ @Override - public boolean isEnabled(int id, boolean defaultValue) { + public boolean isEnabled(BooleanFlag flag) { + int id = flag.getId(); if (!mBooleanFlagCache.containsKey(id)) { - Boolean result = isEnabledInternal(id); - mBooleanFlagCache.put(id, result == null ? defaultValue : result); + boolean def = flag.getDefault(); + if (flag.hasResourceOverride()) { + try { + def = isEnabledInOverlay(flag.getResourceOverride()); + } catch (Resources.NotFoundException e) { + // no-op + } + } + + mBooleanFlagCache.put(id, isEnabled(id, def)); } return mBooleanFlagCache.get(id); } + /** Return a {@link BooleanFlag}'s value. */ + @Override + public boolean isEnabled(int id, boolean defaultValue) { + Boolean result = isEnabledInternal(id); + return result == null ? defaultValue : result; + } + /** Returns the stored value or null if not set. */ private Boolean isEnabledInternal(int id) { try { @@ -98,6 +121,10 @@ public class FeatureFlagManager implements FlagReader, FlagWriter, Dumpable { return null; } + private boolean isEnabledInOverlay(@BoolRes int resId) { + return mResources.getBoolean(resId); + } + /** Set whether a given {@link BooleanFlag} is enabled or not. */ @Override public void setEnabled(int id, boolean value) { diff --git a/packages/SystemUI/src-release/com/android/systemui/flags/FeatureFlagManager.java b/packages/SystemUI/src-release/com/android/systemui/flags/FeatureFlagManager.java index 6ff175f589a63..0934b32a71e43 100644 --- a/packages/SystemUI/src-release/com/android/systemui/flags/FeatureFlagManager.java +++ b/packages/SystemUI/src-release/com/android/systemui/flags/FeatureFlagManager.java @@ -16,7 +16,6 @@ package com.android.systemui.flags; -import android.content.Context; import android.util.SparseBooleanArray; import androidx.annotation.NonNull; @@ -24,7 +23,6 @@ import androidx.annotation.NonNull; import com.android.systemui.Dumpable; import com.android.systemui.dagger.SysUISingleton; import com.android.systemui.dump.DumpManager; -import com.android.systemui.util.settings.SecureSettings; import java.io.FileDescriptor; import java.io.PrintWriter; @@ -41,8 +39,7 @@ import javax.inject.Inject; public class FeatureFlagManager implements FlagReader, FlagWriter, Dumpable { SparseBooleanArray mAccessedFlags = new SparseBooleanArray(); @Inject - public FeatureFlagManager( - SecureSettings secureSettings, Context context, DumpManager dumpManager) { + public FeatureFlagManager(DumpManager dumpManager) { dumpManager.registerDumpable("SysUIFlags", this); } @@ -52,6 +49,11 @@ public class FeatureFlagManager implements FlagReader, FlagWriter, Dumpable { @Override public void removeListener(Listener run) {} + @Override + public boolean isEnabled(BooleanFlag flag) { + return isEnabled(flag.getId(), flag.getDefault()); + } + @Override public boolean isEnabled(int key, boolean defaultValue) { mAccessedFlags.append(key, defaultValue); diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java index d64f9b3b9fb6f..34f441510a7e0 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java @@ -17,22 +17,11 @@ 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.dagger.SysUISingleton; -import com.android.systemui.dagger.qualifiers.Main; - -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; import javax.inject.Inject; @@ -43,31 +32,13 @@ import javax.inject.Inject; */ @SysUISingleton public class FeatureFlags { - private final Resources mResources; private final FlagReader mFlagReader; private final Context mContext; - private final Map> mFlagMap = new HashMap<>(); - private final Map> mListeners = new HashMap<>(); - private final SparseArray mCachedFlags = new SparseArray<>(); @Inject - public FeatureFlags(@Main Resources resources, FlagReader flagReader, Context context) { - mResources = resources; + public FeatureFlags(FlagReader flagReader, Context context) { mFlagReader = flagReader; mContext = context; - - flagReader.addListener(mListener); - } - - private final FlagReader.Listener mListener = id -> { - if (mListeners.containsKey(id) && mFlagMap.containsKey(id)) { - mListeners.get(id).forEach(listener -> listener.onFlagChanged(mFlagMap.get(id))); - } - }; - - @VisibleForTesting - void addFlag(Flag flag) { - mFlagMap.put(flag.getId(), flag); } /** @@ -75,32 +46,7 @@ public class FeatureFlags { * @return The value of the flag. */ public boolean isEnabled(BooleanFlag 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); - } - - /** - * @param flag The {@link IntFlag} of interest. - - /** Add a listener for a specific flag. */ - public void addFlagListener(Flag flag, Listener listener) { - mListeners.putIfAbsent(flag.getId(), new ArrayList<>()); - mListeners.get(flag.getId()).add(listener); - mFlagMap.putIfAbsent(flag.getId(), flag); - } - - /** Remove a listener for a specific flag. */ - public void removeFlagListener(Flag flag, Listener listener) { - if (mListeners.containsKey(flag.getId())) { - mListeners.get(flag.getId()).remove(listener); - } + return mFlagReader.isEnabled(flag); } public void assertLegacyPipelineEnabled() { @@ -205,20 +151,4 @@ public class FeatureFlags { public static boolean isProviderModelSettingEnabled(Context context) { 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 { - /** */ - void onFlagChanged(Flag flag); - } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagManagerTest.java index 2fa32ba1fe75e..634763866d028 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagManagerTest.java @@ -53,8 +53,6 @@ import java.io.StringWriter; public class FeatureFlagManagerTest extends SysuiTestCase { FeatureFlagManager mFeatureFlagManager; - @Mock private FlagManager mFlagManager; - @Mock private SecureSettings mSecureSettings; @Mock private Context mContext; @Mock private DumpManager mDumpManager; @@ -62,14 +60,11 @@ public class FeatureFlagManagerTest extends SysuiTestCase { public void setup() { MockitoAnnotations.initMocks(this); - mFeatureFlagManager = new FeatureFlagManager(mSecureSettings, mContext, mDumpManager); + mFeatureFlagManager = new FeatureFlagManager(mDumpManager); } @After public void onFinished() { - // SecureSettings and Context are provided for constructor consistency with the - // debug version of the FeatureFlagManager, but should never be used. - verifyZeroInteractions(mSecureSettings, mContext); // The dump manager should be registered with even for the release version, but that's it. verify(mDumpManager).registerDumpable(anyString(), any()); verifyNoMoreInteractions(mDumpManager); diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java deleted file mode 100644 index 30e9b51ea47e6..0000000000000 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java +++ /dev/null @@ -1,138 +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 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; - -import com.android.systemui.SysuiTestCase; - -import org.junit.Before; -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 Resources mResources; - @Mock FlagReader mFeatureFlagReader; - - private FeatureFlags mFeatureFlags; - - @Before - public void setup() { - MockitoAnnotations.initMocks(this); - - when(mFeatureFlagReader.isEnabled(anyInt(), anyBoolean())).thenAnswer( - (Answer) invocation -> invocation.getArgument(1)); - - mFeatureFlags = new FeatureFlags(mResources, mFeatureFlagReader, getContext()); - } - - @Test - public void testAddListener() { - Flag flag = new BooleanFlag(1); - mFeatureFlags.addFlag(flag); - - // Assert and capture that a plugin listener was added. - ArgumentCaptor pluginListenerCaptor = - ArgumentCaptor.forClass(FlagReader.Listener.class); - verify(mFeatureFlagReader).addListener(pluginListenerCaptor.capture()); - FlagReader.Listener pluginListener = pluginListenerCaptor.getValue(); - - // Signal a change. No listeners, so no real effect. - pluginListener.onFlagChanged(flag.getId()); - - // Add a listener for the flag - final Flag[] changedFlag = {null}; - FeatureFlags.Listener listener = f -> changedFlag[0] = f; - mFeatureFlags.addFlagListener(flag, listener); - - // No changes seen yet. - assertThat(changedFlag[0]).isNull(); - - // Signal a change. - pluginListener.onFlagChanged(flag.getId()); - - // Assert that the change was for the correct flag. - assertThat(changedFlag[0]).isEqualTo(flag); - } - - @Test - public void testRemoveListener() { - Flag flag = new BooleanFlag(1); - mFeatureFlags.addFlag(flag); - - // Assert and capture that a plugin listener was added. - ArgumentCaptor pluginListenerCaptor = - ArgumentCaptor.forClass(FlagReader.Listener.class); - verify(mFeatureFlagReader).addListener(pluginListenerCaptor.capture()); - FlagReader.Listener pluginListener = pluginListenerCaptor.getValue(); - - // Add a listener for the flag - final Flag[] changedFlag = {null}; - FeatureFlags.Listener listener = f -> changedFlag[0] = f; - mFeatureFlags.addFlagListener(flag, listener); - - // Signal a change. - pluginListener.onFlagChanged(flag.getId()); - - // Assert that the change was for the correct flag. - assertThat(changedFlag[0]).isEqualTo(flag); - - changedFlag[0] = null; - - // Now remove the listener. - mFeatureFlags.removeFlagListener(flag, listener); - // Signal a change. - 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(); - } -}