From 5eac15cf85efee2f5128435d35f1f9838cd431db 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 Merged-In: I43961a091f0f10ebd1a168f40703f0067d9f8a18 --- .../systemui/flags/FeatureFlagManager.java | 15 +++- .../systemui/dagger/SystemUIModule.java | 9 ++ .../systemui/flags/FeatureFlagManager.java | 16 ++-- .../systemui/flags/FeatureFlagReader.java | 86 ++++++++----------- .../android/systemui/flags/FeatureFlags.java | 40 +-------- .../android/systemui/flags/FlagReader.java} | 37 +------- .../com/android/systemui/flags/FlagWriter.kt | 21 +++++ .../systemui/flags/FeatureFlagReaderTest.java | 6 +- .../systemui/flags/FeatureFlagsTest.java | 15 ++-- 9 files changed, 102 insertions(+), 143 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 3b459d1427d5b..f9331280ece35 100644 --- a/packages/SystemUI/src/com/android/systemui/dagger/SystemUIModule.java +++ b/packages/SystemUI/src/com/android/systemui/dagger/SystemUIModule.java @@ -38,7 +38,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; @@ -149,6 +152,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 d4d01c8d97b59..e78646a4839d4 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagReader.java @@ -16,26 +16,29 @@ package com.android.systemui.flags; -import android.content.Context; 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.plugins.FlagReaderPlugin; -import com.android.systemui.plugins.PluginListener; -import com.android.systemui.shared.plugins.PluginManager; +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: + * Feature flags must be defined as boolean resources. For example:t * * {@code * false @@ -55,71 +58,39 @@ import javax.inject.Inject; * Calls to this class should probably be wrapped by a method in {@link FeatureFlags}. */ @SysUISingleton -public class FeatureFlagReader { +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 final FlagReader mFlagReader; @Inject public FeatureFlagReader( @Main Resources resources, BuildInfo build, - PluginManager pluginManager, - SystemPropertiesHelper systemPropertiesHelper) { + DumpManager dumpManager, + SystemPropertiesHelper systemPropertiesHelper, + FlagReader reader) { mResources = resources; - mPluginManager = pluginManager; + mFlagReader = reader; mSystemPropertiesHelper = systemPropertiesHelper; mAreFlagsOverrideable = build.isDebuggable() && mResources.getBoolean(R.bool.are_flags_overrideable); - - mPluginManager.addPluginListener(mPluginListener, FlagReaderPlugin.class); + dumpManager.registerDumpable("FeatureFlags", this); } - 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); } /** @@ -172,6 +143,23 @@ public class FeatureFlagReader { } } + @Override + public void dump(@NonNull FileDescriptor fd, @NonNull PrintWriter pw, @NonNull String[] args) { + ArrayList 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; diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java index e2cef43a2cf8f..105e7f514dc40 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 7bc5f86510a11..fc6f3fd1d9c65 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagReaderTest.java @@ -32,6 +32,7 @@ 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; @@ -44,8 +45,9 @@ import org.mockito.MockitoAnnotations; public class FeatureFlagReaderTest extends SysuiTestCase { @Mock private Resources mResources; @Mock private BuildInfo mBuildInfo; - @Mock private PluginManager mPluginManager; + @Mock private DumpManager mDumpManager; @Mock private SystemPropertiesHelper mSystemPropertiesHelper; + @Mock private FlagReader mFlagReader; private FeatureFlagReader mReader; @@ -66,7 +68,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, 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};