From 7277cee4c5c2fac8913233596e4f68de1806034f Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Mon, 14 Feb 2022 17:02:28 -0500 Subject: [PATCH] Add support for SystemProperty flags to SysUI. This will allow the flag flippin' app to change boolean system properties that launcher, settings, and system server can read. Some of these flags are already being read between the various systems and this gives a unified ux for changing the flags. Bug: 219067621 Test: manual Change-Id: I7d1a1e758915f43cac114054a2c2e055706ef3f3 --- .../src/com/android/systemui/flags/Flag.kt | 11 ++++ .../com/android/systemui/flags/FlagManager.kt | 6 +-- .../android/systemui/flags/FeatureFlags.kt | 3 ++ .../systemui/flags/FeatureFlagsDebug.java | 50 ++++++++++++++++--- .../systemui/flags/FeatureFlagsRelease.java | 19 ++++++- .../systemui/flags/SystemPropertiesHelper.kt | 8 +++ .../systemui/flags/FeatureFlagsDebugTest.kt | 26 ++++++++-- .../systemui/flags/FeatureFlagsReleaseTest.kt | 16 +++++- .../android/systemui/flags/FlagManagerTest.kt | 22 ++++---- 9 files changed, 133 insertions(+), 28 deletions(-) diff --git a/packages/SystemUI/shared/src/com/android/systemui/flags/Flag.kt b/packages/SystemUI/shared/src/com/android/systemui/flags/Flag.kt index b611c9659db89..6ad91612f99b6 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/Flag.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/Flag.kt @@ -35,6 +35,11 @@ interface ResourceFlag : Flag { val resourceId: Int } +interface SysPropFlag : Flag { + val name: String + val default: T +} + // Consider using the "parcelize" kotlin library. data class BooleanFlag @JvmOverloads constructor( @@ -66,6 +71,12 @@ data class ResourceBooleanFlag constructor( @BoolRes override val resourceId: Int ) : ResourceFlag +data class SysPropBooleanFlag constructor( + override val id: Int, + override val name: String, + override val default: Boolean = false +) : SysPropFlag + data class StringFlag @JvmOverloads constructor( override val id: Int, override val default: String = "" diff --git a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt index ec619dd6eea3d..149f6e8e7e9ed 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt @@ -54,7 +54,7 @@ class FlagManager constructor( * An action called on restart which takes as an argument whether the listeners requested * that the restart be suppressed */ - var restartAction: Consumer? = null + var onSettingsChangedAction: Consumer? = null var clearCacheAction: Consumer? = null private val listeners: MutableSet = mutableSetOf() private val settingsObserver: ContentObserver = SettingsObserver() @@ -154,11 +154,11 @@ class FlagManager constructor( val idStr = parts[parts.size - 1] val id = try { idStr.toInt() } catch (e: NumberFormatException) { return } clearCacheAction?.accept(id) - dispatchListenersAndMaybeRestart(id) + dispatchListenersAndMaybeRestart(id, onSettingsChangedAction) } } - fun dispatchListenersAndMaybeRestart(id: Int) { + fun dispatchListenersAndMaybeRestart(id: Int, restartAction: Consumer?) { val filteredListeners: List = synchronized(listeners) { listeners.mapNotNull { if (it.id == id) it.listener else null } } diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt index 96a90dfc7fe95..9d6e3c2951009 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt @@ -28,6 +28,9 @@ interface FeatureFlags : FlagListenable { /** Returns a boolean value for the given flag. */ fun isEnabled(flag: ResourceBooleanFlag): Boolean + /** Returns a boolean value for the given flag. */ + fun isEnabled(flag: SysPropBooleanFlag): Boolean + /** Returns a string value for the given flag. */ fun getString(flag: StringFlag): String diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java index df605990fa81a..677990437f3df 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java @@ -49,6 +49,7 @@ import java.util.ArrayList; import java.util.Map; import java.util.Objects; import java.util.TreeMap; +import java.util.function.Consumer; import java.util.function.Supplier; import javax.inject.Inject; @@ -69,6 +70,7 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { private final FlagManager mFlagManager; private final SecureSettings mSecureSettings; private final Resources mResources; + private final SystemPropertiesHelper mSystemProperties; private final Supplier>> mFlagsCollector; private final Map mBooleanFlagCache = new TreeMap<>(); private final Map mStringFlagCache = new TreeMap<>(); @@ -79,6 +81,7 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { FlagManager flagManager, Context context, SecureSettings secureSettings, + SystemPropertiesHelper systemProperties, @Main Resources resources, DumpManager dumpManager, @Nullable Supplier>> flagsCollector, @@ -86,11 +89,12 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { mFlagManager = flagManager; mSecureSettings = secureSettings; mResources = resources; + mSystemProperties = systemProperties; mFlagsCollector = flagsCollector != null ? flagsCollector : Flags::collectFlags; IntentFilter filter = new IntentFilter(); filter.addAction(ACTION_SET_FLAG); filter.addAction(ACTION_GET_FLAGS); - flagManager.setRestartAction(this::restartSystemUI); + flagManager.setOnSettingsChangedAction(this::restartSystemUI); flagManager.setClearCacheAction(this::removeFromCache); context.registerReceiver(mReceiver, filter, null, null, Context.RECEIVER_EXPORTED_UNAUDITED); @@ -121,6 +125,17 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { return mBooleanFlagCache.get(id); } + @Override + public boolean isEnabled(@NonNull SysPropBooleanFlag flag) { + int id = flag.getId(); + if (!mBooleanFlagCache.containsKey(id)) { + mBooleanFlagCache.put( + id, mSystemProperties.getBoolean(flag.getName(), flag.getDefault())); + } + + return mBooleanFlagCache.get(id); + } + @NonNull @Override public String getString(@NonNull StringFlag flag) { @@ -180,16 +195,28 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { mSecureSettings.putString(mFlagManager.idToSettingsKey(id), data); Log.i(TAG, "Set id " + id + " to " + value); removeFromCache(id); - mFlagManager.dispatchListenersAndMaybeRestart(id); + mFlagManager.dispatchListenersAndMaybeRestart(id, this::restartSystemUI); + } + + private void eraseFlag(Flag flag) { + if (flag instanceof SysPropFlag) { + mSystemProperties.erase(((SysPropFlag) flag).getName()); + dispatchListenersAndMaybeRestart(flag.getId(), this::restartAndroid); + } else { + eraseFlag(flag.getId()); + } } /** Erase a flag's overridden value if there is one. */ - public void eraseFlag(int id) { + private void eraseFlag(int id) { eraseInternal(id); removeFromCache(id); - mFlagManager.dispatchListenersAndMaybeRestart(id); + dispatchListenersAndMaybeRestart(id, this::restartSystemUI); } + private void dispatchListenersAndMaybeRestart(int id, Consumer restartAction) { + mFlagManager.dispatchListenersAndMaybeRestart(id, restartAction); + } /** Works just like {@link #eraseFlag(int)} except that it doesn't restart SystemUI. */ private void eraseInternal(int id) { // We can't actually "erase" things from sysprops, but we can set them to empty! @@ -217,7 +244,11 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { System.exit(0); } - private void restartAndroid() { + private void restartAndroid(boolean requestSuppress) { + if (requestSuppress) { + Log.i(TAG, "Android Restart Suppressed"); + return; + } Log.i(TAG, "Restarting Android"); try { mBarService.restart(); @@ -273,7 +304,7 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { Flag flag = flagMap.get(id); if (!extras.containsKey(EXTRA_VALUE)) { - eraseFlag(id); + eraseFlag(flag); return; } @@ -282,6 +313,10 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { setFlagValue(id, (Boolean) value, BooleanFlagSerializer.INSTANCE); } else if (flag instanceof ResourceBooleanFlag && value instanceof Boolean) { setFlagValue(id, (Boolean) value, BooleanFlagSerializer.INSTANCE); + } else if (flag instanceof SysPropBooleanFlag && value instanceof Boolean) { + // Store SysProp flags in SystemProperties where they can read by outside parties. + mSystemProperties.setBoolean( + ((SysPropBooleanFlag) flag).getName(), (Boolean) value); } else if (flag instanceof StringFlag && value instanceof String) { setFlagValue(id, (String) value, StringFlagSerializer.INSTANCE); } else if (flag instanceof ResourceStringFlag && value instanceof String) { @@ -306,6 +341,9 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { if (f instanceof ResourceBooleanFlag) { return new BooleanFlag(f.getId(), isEnabled((ResourceBooleanFlag) f)); } + if (f instanceof SysPropBooleanFlag) { + return new BooleanFlag(f.getId(), isEnabled((SysPropBooleanFlag) f)); + } // TODO: add support for other flag types. Log.w(TAG, "Unsupported Flag Type. Please file a bug."); diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java index 348a8e20122ea..1fb1acfae1cb3 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java @@ -43,11 +43,17 @@ import javax.inject.Inject; @SysUISingleton public class FeatureFlagsRelease implements FeatureFlags, Dumpable { private final Resources mResources; + private final SystemPropertiesHelper mSystemProperties; SparseBooleanArray mBooleanCache = new SparseBooleanArray(); SparseArray mStringCache = new SparseArray<>(); + @Inject - public FeatureFlagsRelease(@Main Resources resources, DumpManager dumpManager) { + public FeatureFlagsRelease( + @Main Resources resources, + SystemPropertiesHelper systemProperties, + DumpManager dumpManager) { mResources = resources; + mSystemProperties = systemProperties; dumpManager.registerDumpable("SysUIFlags", this); } @@ -72,6 +78,17 @@ public class FeatureFlagsRelease implements FeatureFlags, Dumpable { return mBooleanCache.valueAt(cacheIndex); } + @Override + public boolean isEnabled(SysPropBooleanFlag flag) { + int cacheIndex = mBooleanCache.indexOfKey(flag.getId()); + if (cacheIndex < 0) { + return isEnabled( + flag.getId(), mSystemProperties.getBoolean(flag.getName(), flag.getDefault())); + } + + return mBooleanCache.valueAt(cacheIndex); + } + private boolean isEnabled(int key, boolean defaultValue) { mBooleanCache.append(key, defaultValue); return defaultValue; diff --git a/packages/SystemUI/src/com/android/systemui/flags/SystemPropertiesHelper.kt b/packages/SystemUI/src/com/android/systemui/flags/SystemPropertiesHelper.kt index 1dc5a9f3adc5a..6c160975587b3 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/SystemPropertiesHelper.kt +++ b/packages/SystemUI/src/com/android/systemui/flags/SystemPropertiesHelper.kt @@ -34,6 +34,10 @@ open class SystemPropertiesHelper @Inject constructor() { return SystemProperties.getBoolean(name, default) } + fun setBoolean(name: String, value: Boolean) { + SystemProperties.set(name, if (value) "1" else "0") + } + fun set(name: String, value: String) { SystemProperties.set(name, value) } @@ -41,4 +45,8 @@ open class SystemPropertiesHelper @Inject constructor() { fun set(name: String, value: Int) { set(name, value.toString()) } + + fun erase(name: String) { + set(name, "") + } } \ No newline at end of file diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt index 4cc5673eaf978..23a5b2b2c5b7c 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt @@ -20,7 +20,7 @@ import android.content.Context import android.content.Intent import android.content.pm.PackageManager.NameNotFoundException import android.content.res.Resources -import androidx.test.filters.SmallTest +import android.test.suitebuilder.annotation.SmallTest import com.android.internal.statusbar.IStatusBarService import com.android.systemui.SysuiTestCase import com.android.systemui.dump.DumpManager @@ -35,6 +35,8 @@ import org.junit.Assert import org.junit.Before import org.junit.Test import org.mockito.Mock +import org.mockito.Mockito.anyBoolean +import org.mockito.Mockito.anyString import org.mockito.Mockito.inOrder import org.mockito.Mockito.times import org.mockito.Mockito.verify @@ -57,6 +59,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { @Mock private lateinit var mFlagManager: FlagManager @Mock private lateinit var mMockContext: Context @Mock private lateinit var mSecureSettings: SecureSettings + @Mock private lateinit var mSystemProperties: SystemPropertiesHelper @Mock private lateinit var mResources: Resources @Mock private lateinit var mDumpManager: DumpManager @Mock private lateinit var mBarService: IStatusBarService @@ -71,12 +74,13 @@ class FeatureFlagsDebugTest : SysuiTestCase() { mFlagManager, mMockContext, mSecureSettings, + mSystemProperties, mResources, mDumpManager, { mFlagMap }, mBarService ) - verify(mFlagManager).restartAction = any() + verify(mFlagManager).onSettingsChangedAction = any() mBroadcastReceiver = withArgCaptor { verify(mMockContext).registerReceiver(capture(), any(), nullable(), nullable(), any()) @@ -122,6 +126,22 @@ class FeatureFlagsDebugTest : SysuiTestCase() { } } + @Test + fun testReadSysPropBooleanFlag() { + whenever(mSystemProperties.getBoolean(anyString(), anyBoolean())).thenAnswer { + if ("b".equals(it.getArgument(0))) { + return@thenAnswer true + } + return@thenAnswer it.getArgument(1) + } + + assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(1, "a"))).isFalse() + assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(2, "b"))).isTrue() + assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(3, "c", true))).isTrue() + assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(4, "d", false))).isFalse() + assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(5, "e"))).isFalse() + } + @Test fun testReadStringFlag() { whenever(mFlagManager.readFlagValue(eq(3), any())).thenReturn("foo") @@ -259,7 +279,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { verify(mFlagManager, times(numReads)).readFlagValue(eq(id), any>()) verify(mFlagManager).idToSettingsKey(eq(id)) verify(mSecureSettings).putString(eq("key-$id"), eq(data)) - verify(mFlagManager).dispatchListenersAndMaybeRestart(eq(id)) + verify(mFlagManager).dispatchListenersAndMaybeRestart(eq(id), any()) }.verifyNoMoreInteractions() verifyNoMoreInteractions(mFlagManager, mSecureSettings) } diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt index b5e6602594eb4..ad304c49bd411 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt @@ -17,7 +17,7 @@ package com.android.systemui.flags import android.content.pm.PackageManager.NameNotFoundException import android.content.res.Resources -import androidx.test.filters.SmallTest +import android.test.suitebuilder.annotation.SmallTest import com.android.systemui.SysuiTestCase import com.android.systemui.dump.DumpManager import com.android.systemui.util.mockito.any @@ -44,12 +44,13 @@ class FeatureFlagsReleaseTest : SysuiTestCase() { private lateinit var mFeatureFlagsRelease: FeatureFlagsRelease @Mock private lateinit var mResources: Resources + @Mock private lateinit var mSystemProperties: SystemPropertiesHelper @Mock private lateinit var mDumpManager: DumpManager @Before fun setup() { MockitoAnnotations.initMocks(this) - mFeatureFlagsRelease = FeatureFlagsRelease(mResources, mDumpManager) + mFeatureFlagsRelease = FeatureFlagsRelease(mResources, mSystemProperties, mDumpManager) } @After @@ -86,6 +87,17 @@ class FeatureFlagsReleaseTest : SysuiTestCase() { } } + @Test + fun testSysPropBooleanFlag() { + val flagId = 213 + val flagName = "sys_prop_flag" + val flagDefault = true + + val flag = SysPropBooleanFlag(flagId, flagName, flagDefault) + whenever(mSystemProperties.getBoolean(flagName, flagDefault)).thenReturn(flagDefault) + assertThat(mFeatureFlagsRelease.isEnabled(flag)).isEqualTo(flagDefault) + } + @Test fun testDump() { val flag1 = BooleanFlag(1, true) diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt index 644bd21476116..a2eca81b04ed1 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt @@ -19,7 +19,7 @@ import android.content.Context import android.database.ContentObserver import android.net.Uri import android.os.Handler -import androidx.test.filters.SmallTest +import android.test.suitebuilder.annotation.SmallTest import com.android.systemui.SysuiTestCase import com.android.systemui.util.mockito.any import com.android.systemui.util.mockito.eq @@ -130,14 +130,14 @@ class FlagManagerTest : SysuiTestCase() { mFlagManager.addListener(BooleanFlag(1, true), listener1) mFlagManager.addListener(BooleanFlag(10, true), listener10) - mFlagManager.dispatchListenersAndMaybeRestart(1) + mFlagManager.dispatchListenersAndMaybeRestart(1, null) val flagEvent1 = withArgCaptor { verify(listener1).onFlagChanged(capture()) } assertThat(flagEvent1.flagId).isEqualTo(1) verifyNoMoreInteractions(listener1, listener10) - mFlagManager.dispatchListenersAndMaybeRestart(10) + mFlagManager.dispatchListenersAndMaybeRestart(10, null) val flagEvent10 = withArgCaptor { verify(listener10).onFlagChanged(capture()) } @@ -151,14 +151,14 @@ class FlagManagerTest : SysuiTestCase() { mFlagManager.addListener(BooleanFlag(1, true), listener) mFlagManager.addListener(BooleanFlag(10, true), listener) - mFlagManager.dispatchListenersAndMaybeRestart(1) + mFlagManager.dispatchListenersAndMaybeRestart(1, null) val flagEvent1 = withArgCaptor { verify(listener).onFlagChanged(capture()) } assertThat(flagEvent1.flagId).isEqualTo(1) verifyNoMoreInteractions(listener) - mFlagManager.dispatchListenersAndMaybeRestart(10) + mFlagManager.dispatchListenersAndMaybeRestart(10, null) val flagEvent10 = withArgCaptor { verify(listener, times(2)).onFlagChanged(capture()) } @@ -169,8 +169,7 @@ class FlagManagerTest : SysuiTestCase() { @Test fun testRestartWithNoListeners() { val restartAction = mock>() - mFlagManager.restartAction = restartAction - mFlagManager.dispatchListenersAndMaybeRestart(1) + mFlagManager.dispatchListenersAndMaybeRestart(1, restartAction) verify(restartAction).accept(eq(false)) verifyNoMoreInteractions(restartAction) } @@ -178,11 +177,10 @@ class FlagManagerTest : SysuiTestCase() { @Test fun testListenerCanSuppressRestart() { val restartAction = mock>() - mFlagManager.restartAction = restartAction mFlagManager.addListener(BooleanFlag(1, true)) { event -> event.requestNoRestart() } - mFlagManager.dispatchListenersAndMaybeRestart(1) + mFlagManager.dispatchListenersAndMaybeRestart(1, restartAction) verify(restartAction).accept(eq(true)) verifyNoMoreInteractions(restartAction) } @@ -190,11 +188,10 @@ class FlagManagerTest : SysuiTestCase() { @Test fun testListenerOnlySuppressesRestartForOwnFlag() { val restartAction = mock>() - mFlagManager.restartAction = restartAction mFlagManager.addListener(BooleanFlag(10, true)) { event -> event.requestNoRestart() } - mFlagManager.dispatchListenersAndMaybeRestart(1) + mFlagManager.dispatchListenersAndMaybeRestart(1, restartAction) verify(restartAction).accept(eq(false)) verifyNoMoreInteractions(restartAction) } @@ -202,14 +199,13 @@ class FlagManagerTest : SysuiTestCase() { @Test fun testRestartWhenNotAllListenersRequestSuppress() { val restartAction = mock>() - mFlagManager.restartAction = restartAction mFlagManager.addListener(BooleanFlag(10, true)) { event -> event.requestNoRestart() } mFlagManager.addListener(BooleanFlag(10, true)) { // do not request } - mFlagManager.dispatchListenersAndMaybeRestart(1) + mFlagManager.dispatchListenersAndMaybeRestart(1, restartAction) verify(restartAction).accept(eq(false)) verifyNoMoreInteractions(restartAction) }