From 55f00e7b5b5bbaa60a908c7bdbe280df491e5b76 Mon Sep 17 00:00:00 2001 From: Jay Aliomer Date: Tue, 28 Sep 2021 13:23:51 -0400 Subject: [PATCH] Remove flag plugin with flag manager with DI Test: FeatureFlagsTest Bug: 196602427 Change-Id: I43961a091f0f10ebd1a168f40703f0067d9f8a18 --- .../systemui/flags/FeatureFlagManager.java | 15 ++++- .../systemui/dagger/SystemUIModule.java | 9 +++ .../systemui/flags/FeatureFlagManager.java | 16 +++--- .../systemui/flags/FeatureFlagReader.java | 56 +++---------------- .../android/systemui/flags/FeatureFlags.java | 40 +------------ .../android/systemui/flags/FlagReader.java} | 37 +----------- .../com/android/systemui/flags/FlagWriter.kt | 21 +++++++ .../systemui/flags/FeatureFlagReaderTest.java | 3 +- .../systemui/flags/FeatureFlagsTest.java | 15 ++--- 9 files changed, 72 insertions(+), 140 deletions(-) rename packages/SystemUI/{plugin/src/com/android/systemui/plugins/FlagReaderPlugin.java => src/com/android/systemui/flags/FlagReader.java} (52%) create mode 100644 packages/SystemUI/src/com/android/systemui/flags/FlagWriter.kt 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 5c4f08edc784a..b3a6699ca646a 100644 --- a/packages/SystemUI/src-debug/com/android/systemui/flags/FeatureFlagManager.java +++ b/packages/SystemUI/src-debug/com/android/systemui/flags/FeatureFlagManager.java @@ -15,15 +15,19 @@ */ package com.android.systemui.flags; -import android.util.ArraySet; import com.android.systemui.dagger.SysUISingleton; +import javax.inject.Inject; + /** * Concrete implementation of the a Flag manager that returns default values for debug builds */ @SysUISingleton -public class FeatureFlagManager { +public class FeatureFlagManager implements FlagReader, FlagWriter { + @Inject + public FeatureFlagManager() {} + public boolean isEnabled(int key, boolean defaultValue) { return isEnabled(Integer.toString(key), defaultValue); } @@ -40,4 +44,9 @@ public class FeatureFlagManager { public void setEnabled(String key, boolean value) { // TODO } -} \ No newline at end of file + + public void addListener(Listener run) {} + + public void removeListener(Listener run) {} + +} diff --git a/packages/SystemUI/src/com/android/systemui/dagger/SystemUIModule.java b/packages/SystemUI/src/com/android/systemui/dagger/SystemUIModule.java index 8defe3c82303d..e57e9918e96a3 100644 --- a/packages/SystemUI/src/com/android/systemui/dagger/SystemUIModule.java +++ b/packages/SystemUI/src/com/android/systemui/dagger/SystemUIModule.java @@ -39,7 +39,10 @@ import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.demomode.dagger.DemoModeModule; import com.android.systemui.doze.dagger.DozeComponent; import com.android.systemui.dump.DumpManager; +import com.android.systemui.flags.FeatureFlagManager; import com.android.systemui.flags.FeatureFlags; +import com.android.systemui.flags.FlagReader; +import com.android.systemui.flags.FlagWriter; import com.android.systemui.fragments.FragmentService; import com.android.systemui.log.dagger.LogModule; import com.android.systemui.model.SysUiState; @@ -151,6 +154,12 @@ public abstract class SystemUIModule { return state; } + @Binds + abstract FlagReader provideFlagReader(FeatureFlagManager impl); + + @Binds + abstract FlagWriter provideFlagWriter(FeatureFlagManager impl); + @BindsOptionalOf abstract CommandQueue optionalCommandQueue(); diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagManager.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagManager.java index dc09f2aa24924..85baed4a221ca 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagManager.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagManager.java @@ -18,19 +18,21 @@ package com.android.systemui.flags; import com.android.systemui.dagger.SysUISingleton; +import javax.inject.Inject; + /** * Default implementation of the a Flag manager that returns default values for release builds */ @SysUISingleton -public class FeatureFlagManager { - public boolean getBoolean(int key, boolean defaultValue) { +public class FeatureFlagManager implements FlagReader, FlagWriter { + @Inject + public FeatureFlagManager() {} + public boolean isEnabled(String key, boolean defaultValue) { return defaultValue; } - public void setBoolean(int key, boolean value) {} - public boolean getBoolean(String key, boolean defaultValue) { + public boolean isEnabled(int key, boolean defaultValue) { return defaultValue; } - public void setBoolean(String key, boolean value) {} - public void addFlagChangedListener(Runnable run) {} - public void removeFlagUpdatedListener(Runnable run) {} + public void setEnabled(String key, boolean value) {} + public void setEnabled(int key, boolean value) {} } diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java index 3b1cc19fe3076..c87a7e4ff5cc1 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java @@ -16,7 +16,6 @@ package com.android.systemui.flags; -import android.content.Context; import android.content.res.Resources; import android.util.SparseArray; @@ -29,9 +28,6 @@ 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.plugins.FlagReaderPlugin; -import com.android.systemui.plugins.PluginListener; -import com.android.systemui.shared.plugins.PluginManager; import com.android.systemui.util.wrapper.BuildInfo; import java.io.FileDescriptor; @@ -65,70 +61,36 @@ import javax.inject.Inject; public class FeatureFlagReader implements Dumpable { private final Resources mResources; private final boolean mAreFlagsOverrideable; - private final PluginManager mPluginManager; private final SystemPropertiesHelper mSystemPropertiesHelper; private final SparseArray mCachedFlags = new SparseArray<>(); - private FlagReaderPlugin mPlugin = new FlagReaderPlugin(){}; + private FlagReader mFlagReader; @Inject public FeatureFlagReader( @Main Resources resources, BuildInfo build, DumpManager dumpManager, - PluginManager pluginManager, - SystemPropertiesHelper systemPropertiesHelper) { + SystemPropertiesHelper systemPropertiesHelper, + FlagReader reader) { mResources = resources; - mPluginManager = pluginManager; + mFlagReader = reader; mSystemPropertiesHelper = systemPropertiesHelper; mAreFlagsOverrideable = build.isDebuggable() && mResources.getBoolean(R.bool.are_flags_overrideable); - dumpManager.registerDumpable("FeatureFlags", this); - mPluginManager.addPluginListener(mPluginListener, FlagReaderPlugin.class); } - private final PluginListener mPluginListener = - new PluginListener() { - public void onPluginConnected(FlagReaderPlugin plugin, Context context) { - mPlugin = plugin; - } - - public void onPluginDisconnected(FlagReaderPlugin plugin) { - mPlugin = new FlagReaderPlugin() {}; - } - }; - boolean isEnabled(BooleanFlag flag) { - return mPlugin.isEnabled(flag.getId(), flag.getDefault()); + return mFlagReader.isEnabled(flag.getId(), flag.getDefault()); } - String getValue(StringFlag flag) { - return mPlugin.getValue(flag.getId(), flag.getDefault()); + void addListener(FlagReader.Listener listener) { + mFlagReader.addListener(listener); } - int getValue(IntFlag flag) { - return mPlugin.getValue(flag.getId(), flag.getDefault()); - } - - long getValue(LongFlag flag) { - return mPlugin.getValue(flag.getId(), flag.getDefault()); - } - - float getValue(FloatFlag flag) { - return mPlugin.getValue(flag.getId(), flag.getDefault()); - } - - double getValue(DoubleFlag flag) { - return mPlugin.getValue(flag.getId(), flag.getDefault()); - } - - void addListener(FlagReaderPlugin.Listener listener) { - mPlugin.addListener(listener); - } - - void removeListener(FlagReaderPlugin.Listener listener) { - mPlugin.removeListener(listener); + void removeListener(FlagReader.Listener listener) { + mFlagReader.removeListener(listener); } /** diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java index 3ff088358ab08..432d6c2f377a5 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java @@ -22,7 +22,6 @@ import android.util.FeatureFlagUtils; import com.android.internal.annotations.VisibleForTesting; import com.android.systemui.R; import com.android.systemui.dagger.SysUISingleton; -import com.android.systemui.plugins.FlagReaderPlugin; import java.util.ArrayList; import java.util.HashMap; @@ -51,7 +50,7 @@ public class FeatureFlags { flagReader.addListener(mListener); } - private final FlagReaderPlugin.Listener mListener = id -> { + private final FlagReader.Listener mListener = id -> { if (mListeners.containsKey(id) && mFlagMap.containsKey(id)) { mListeners.get(id).forEach(listener -> listener.onFlagChanged(mFlagMap.get(id))); } @@ -70,45 +69,8 @@ public class FeatureFlags { return mFlagReader.isEnabled(flag); } - /** - * @param flag The {@link StringFlag} of interest. - * @return The value of the flag. - */ - public String getValue(StringFlag flag) { - return mFlagReader.getValue(flag); - } - /** * @param flag The {@link IntFlag} of interest. - * @return The value of the flag. - */ - public int getValue(IntFlag flag) { - return mFlagReader.getValue(flag); - } - - /** - * @param flag The {@link LongFlag} of interest. - * @return The value of the flag. - */ - public long getValue(LongFlag flag) { - return mFlagReader.getValue(flag); - } - - /** - * @param flag The {@link FloatFlag} of interest. - * @return The value of the flag. - */ - public float getValue(FloatFlag flag) { - return mFlagReader.getValue(flag); - } - - /** - * @param flag The {@link DoubleFlag} of interest. - * @return The value of the flag. - */ - public double getValue(DoubleFlag flag) { - return mFlagReader.getValue(flag); - } /** Add a listener for a specific flag. */ public void addFlagListener(Flag flag, Listener listener) { diff --git a/packages/SystemUI/plugin/src/com/android/systemui/plugins/FlagReaderPlugin.java b/packages/SystemUI/src/com/android/systemui/flags/FlagReader.java similarity index 52% rename from packages/SystemUI/plugin/src/com/android/systemui/plugins/FlagReaderPlugin.java rename to packages/SystemUI/src/com/android/systemui/flags/FlagReader.java index ab1749978653d..1ae8c1f2a712a 100644 --- a/packages/SystemUI/plugin/src/com/android/systemui/plugins/FlagReaderPlugin.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FlagReader.java @@ -14,49 +14,18 @@ * limitations under the License. */ -package com.android.systemui.plugins; - -import com.android.systemui.plugins.annotations.ProvidesInterface; +package com.android.systemui.flags; /** - * Plugin for loading flag values from an alternate source of truth. + * Plugin for loading flag values */ -@ProvidesInterface(action = FlagReaderPlugin.ACTION, version = FlagReaderPlugin.VERSION) -public interface FlagReaderPlugin extends Plugin { - int VERSION = 1; - String ACTION = "com.android.systemui.flags.FLAG_READER_PLUGIN"; - +public interface FlagReader { /** Returns a boolean value for the given flag. */ default boolean isEnabled(int id, boolean def) { return def; } - /** Returns a string value for the given flag id. */ - default String getValue(int id, String def) { - return def; - } - - /** Returns a int value for the given flag. */ - default int getValue(int id, int def) { - return def; - } - - /** Returns a long value for the given flag. */ - default long getValue(int id, long def) { - return def; - } - - /** Returns a float value for the given flag. */ - default float getValue(int id, float def) { - return def; - } - - /** Returns a double value for the given flag. */ - default double getValue(int id, double def) { - return def; - } - /** Add a listener to be alerted when any flag changes. */ default void addListener(Listener listener) {} diff --git a/packages/SystemUI/src/com/android/systemui/flags/FlagWriter.kt b/packages/SystemUI/src/com/android/systemui/flags/FlagWriter.kt new file mode 100644 index 0000000000000..bacc66b39c587 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/flags/FlagWriter.kt @@ -0,0 +1,21 @@ +/* + * 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 + +interface FlagWriter { + fun setEnabled(key: Int, value: Boolean) {} +} \ No newline at end of file diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java index 9616042c5a82f..52d08dc44104d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java @@ -55,6 +55,7 @@ public class FeatureFlagReaderTest extends SysuiTestCase { @Mock private DumpManager mDumpManager; @Mock private PluginManager mPluginManager; @Mock private SystemPropertiesHelper mSystemPropertiesHelper; + @Mock private FlagReader mFlagReader; private FeatureFlagReader mReader; @@ -75,7 +76,7 @@ public class FeatureFlagReaderTest extends SysuiTestCase { when(mBuildInfo.isDebuggable()).thenReturn(isDebuggable); when(mResources.getBoolean(R.bool.are_flags_overrideable)).thenReturn(isOverrideable); mReader = new FeatureFlagReader( - mResources, mBuildInfo, mDumpManager, mPluginManager, mSystemPropertiesHelper); + mResources, mBuildInfo, mDumpManager, mSystemPropertiesHelper, mFlagReader); } @Test diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java index 1a961787ee796..a850f70ae3183 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsTest.java @@ -23,7 +23,6 @@ import static org.mockito.Mockito.verify; import androidx.test.filters.SmallTest; import com.android.systemui.SysuiTestCase; -import com.android.systemui.plugins.FlagReaderPlugin; import org.junit.Before; import org.junit.Test; @@ -51,11 +50,10 @@ public class FeatureFlagsTest extends SysuiTestCase { mFeatureFlags.addFlag(flag); // Assert and capture that a plugin listener was added. - ArgumentCaptor pluginListenerCaptor = - ArgumentCaptor.forClass(FlagReaderPlugin.Listener.class); - + ArgumentCaptor pluginListenerCaptor = + ArgumentCaptor.forClass(FlagReader.Listener.class); verify(mFeatureFlagReader).addListener(pluginListenerCaptor.capture()); - FlagReaderPlugin.Listener pluginListener = pluginListenerCaptor.getValue(); + FlagReader.Listener pluginListener = pluginListenerCaptor.getValue(); // Signal a change. No listeners, so no real effect. pluginListener.onFlagChanged(flag.getId()); @@ -81,11 +79,10 @@ public class FeatureFlagsTest extends SysuiTestCase { mFeatureFlags.addFlag(flag); // Assert and capture that a plugin listener was added. - ArgumentCaptor pluginListenerCaptor = - ArgumentCaptor.forClass(FlagReaderPlugin.Listener.class); - + ArgumentCaptor pluginListenerCaptor = + ArgumentCaptor.forClass(FlagReader.Listener.class); verify(mFeatureFlagReader).addListener(pluginListenerCaptor.capture()); - FlagReaderPlugin.Listener pluginListener = pluginListenerCaptor.getValue(); + FlagReader.Listener pluginListener = pluginListenerCaptor.getValue(); // Add a listener for the flag final Flag[] changedFlag = {null};