From 62ac77294a73a3395f49eab3691c1e1094a1baae Mon Sep 17 00:00:00 2001 From: Dave Mankoff Date: Wed, 17 Nov 2021 09:54:44 -0500 Subject: [PATCH] Better support for Resource based flags. Introduce ResourceFlag class and ParcelableFlag classes, from which other flag types are based. ResourceFlags always pull there default values out of context.getResources. They can not, however, be directly serialized and sent to the flag app. For that, we now have ParcelableFlag. Prior to this change, we were not properly alerting the Flag app about the current value of flags when it asked. This change now also fixes that. All flags are coerced into ParcelableFlag before being sent to the app. Note that ResourceFlags perform runtime logic, even in release builds, meaing that the opportunity for optimizing them at compile time is much smaller. Tools such as proguard will not have the opportunity to strip them out entirely. Bug: 203548827 Test: atest SystemUITests Change-Id: I3c37ccae16c00237f9cffffc9c22580454ecb8cc --- .../src/com/android/systemui/flags/Flag.kt | 66 ++++++++++++------- .../com/android/systemui/flags/FlagManager.kt | 6 +- .../com/android/systemui/flags/FlagReader.kt | 2 + .../systemui/flags/FeatureFlagsDebug.java | 58 +++++++++++----- .../systemui/flags/FeatureFlagsRelease.java | 26 ++++++-- .../src/com/android/systemui/flags/Flags.java | 20 +++--- .../flags/FeatureFlagsReleaseTest.java | 33 ++++++++-- 7 files changed, 148 insertions(+), 63 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 9574101b466c8..b611c9659db89 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/Flag.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/Flag.kt @@ -16,28 +16,31 @@ package com.android.systemui.flags +import android.annotation.BoolRes +import android.annotation.IntegerRes +import android.annotation.StringRes import android.os.Parcel import android.os.Parcelable -interface Flag : Parcelable { +interface Flag { val id: Int +} + +interface ParcelableFlag : Flag, Parcelable { val default: T - val resourceOverride: Int - override fun describeContents() = 0 +} - fun hasResourceOverride(): Boolean { - return resourceOverride != -1 - } +interface ResourceFlag : Flag { + val resourceId: Int } // Consider using the "parcelize" kotlin library. data class BooleanFlag @JvmOverloads constructor( override val id: Int, - override val default: Boolean = false, - override val resourceOverride: Int = -1 -) : Flag { + override val default: Boolean = false +) : ParcelableFlag { companion object { @JvmField @@ -58,11 +61,15 @@ data class BooleanFlag @JvmOverloads constructor( } } +data class ResourceBooleanFlag constructor( + override val id: Int, + @BoolRes override val resourceId: Int +) : ResourceFlag + data class StringFlag @JvmOverloads constructor( override val id: Int, - override val default: String = "", - override val resourceOverride: Int = -1 -) : Flag { + override val default: String = "" +) : ParcelableFlag { companion object { @JvmField val CREATOR = object : Parcelable.Creator { @@ -82,11 +89,15 @@ data class StringFlag @JvmOverloads constructor( } } +data class ResourceStringFlag constructor( + override val id: Int, + @StringRes override val resourceId: Int +) : ResourceFlag + data class IntFlag @JvmOverloads constructor( override val id: Int, - override val default: Int = 0, - override val resourceOverride: Int = -1 -) : Flag { + override val default: Int = 0 +) : ParcelableFlag { companion object { @JvmField @@ -107,11 +118,15 @@ data class IntFlag @JvmOverloads constructor( } } +data class ResourceIntFlag constructor( + override val id: Int, + @IntegerRes override val resourceId: Int +) : ResourceFlag + data class LongFlag @JvmOverloads constructor( override val id: Int, - override val default: Long = 0, - override val resourceOverride: Int = -1 -) : Flag { + override val default: Long = 0 +) : ParcelableFlag { companion object { @JvmField @@ -134,9 +149,8 @@ data class LongFlag @JvmOverloads constructor( data class FloatFlag @JvmOverloads constructor( override val id: Int, - override val default: Float = 0f, - override val resourceOverride: Int = -1 -) : Flag { + override val default: Float = 0f +) : ParcelableFlag { companion object { @JvmField @@ -157,11 +171,15 @@ data class FloatFlag @JvmOverloads constructor( } } +data class ResourceFloatFlag constructor( + override val id: Int, + override val resourceId: Int +) : ResourceFlag + data class DoubleFlag @JvmOverloads constructor( override val id: Int, - override val default: Double = 0.0, - override val resourceOverride: Int = -1 -) : Flag { + override val default: Double = 0.0 +) : ParcelableFlag { companion object { @JvmField 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 e61cb5c9a53eb..b2ca2d7765d9f 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt @@ -60,7 +60,7 @@ class FlagManager constructor( object : BroadcastReceiver() { override fun onReceive(context: Context, intent: Intent) { val extras: Bundle? = getResultExtras(false) - val listOfFlags: java.util.ArrayList>? = + val listOfFlags: java.util.ArrayList>? = extras?.getParcelableArrayList(FIELD_FLAGS) if (listOfFlags != null) { completer.set(listOfFlags) @@ -108,6 +108,10 @@ class FlagManager constructor( } } + override fun isEnabled(flag: ResourceBooleanFlag): Boolean { + throw RuntimeException("Not implemented in FlagManager") + } + override fun addListener(listener: FlagReader.Listener) { synchronized(listeners) { val registerNeeded = listeners.isEmpty() 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 91a391272be7e..26c6a4b096fc8 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt @@ -24,6 +24,8 @@ interface FlagReader { return flag.default } + fun isEnabled(flag: ResourceBooleanFlag): Boolean + /** Returns a boolean value for the given flag. */ fun isEnabled(id: Int, def: Boolean): Boolean { return def diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java index 0ee47a7eea187..3f00b87188d3f 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java @@ -22,6 +22,7 @@ import static com.android.systemui.flags.FlagManager.FIELD_FLAGS; import static com.android.systemui.flags.FlagManager.FIELD_ID; import static com.android.systemui.flags.FlagManager.FIELD_VALUE; +import android.annotation.Nullable; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; @@ -30,7 +31,6 @@ 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; @@ -89,16 +89,18 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { public boolean isEnabled(BooleanFlag flag) { int id = flag.getId(); if (!mBooleanFlagCache.containsKey(id)) { - boolean def = flag.getDefault(); - if (flag.hasResourceOverride()) { - try { - def = isEnabledInOverlay(flag.getResourceOverride()); - } catch (Resources.NotFoundException e) { - // no-op - } - } + mBooleanFlagCache.put(id, isEnabled(id, flag.getDefault())); + } - mBooleanFlagCache.put(id, isEnabled(id, def)); + return mBooleanFlagCache.get(id); + } + + @Override + public boolean isEnabled(ResourceBooleanFlag flag) { + int id = flag.getId(); + if (!mBooleanFlagCache.containsKey(id)) { + mBooleanFlagCache.put( + id, isEnabled(id, mResources.getBoolean(flag.getResourceId()))); } return mBooleanFlagCache.get(id); @@ -111,6 +113,7 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { return result == null ? defaultValue : result; } + /** Returns the stored value or null if not set. */ private Boolean isEnabledInternal(int id) { try { @@ -121,10 +124,6 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { return null; } - private boolean isEnabledInOverlay(@BoolRes int resId) { - return mResources.getBoolean(resId); - } - /** Set whether a given {@link BooleanFlag} is enabled or not. */ public void setEnabled(int id, boolean value) { Boolean currentValue = isEnabledInternal(id); @@ -185,9 +184,19 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { } else if (ACTION_GET_FLAGS.equals(action)) { Map> knownFlagMap = Flags.collectFlags(); ArrayList> flags = new ArrayList<>(knownFlagMap.values()); + + // Convert all flags to parcelable flags. + ArrayList> pFlags = new ArrayList<>(); + for (Flag f : flags) { + ParcelableFlag pf = toParcelableFlag(f); + if (pf != null) { + pFlags.add(pf); + } + } + Bundle extras = getResultExtras(true); if (extras != null) { - extras.putParcelableArrayList(FIELD_FLAGS, flags); + extras.putParcelableArrayList(FIELD_FLAGS, pFlags); } } } @@ -215,6 +224,25 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { setEnabled(id, extras.getBoolean(FIELD_VALUE)); } } + + /** + * Ensures that the data we send to the app reflects the current state of the flags. + * + * Also converts an non-parcelable versions of the flags to their parcelable versions. + */ + @Nullable + private ParcelableFlag toParcelableFlag(Flag f) { + if (f instanceof BooleanFlag) { + return new BooleanFlag(f.getId(), isEnabled((BooleanFlag) f)); + } + if (f instanceof ResourceBooleanFlag) { + return new BooleanFlag(f.getId(), isEnabled((ResourceBooleanFlag) f)); + } + + // TODO: add support for other flag types. + Log.w(TAG, "Unsupported Flag Type. Please file a bug."); + return null; + } }; @Override diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java index bd6cb66570516..5b6404fa6a1f5 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java @@ -16,12 +16,14 @@ package com.android.systemui.flags; +import android.content.res.Resources; import android.util.SparseBooleanArray; 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 java.io.FileDescriptor; @@ -37,9 +39,11 @@ import javax.inject.Inject; */ @SysUISingleton public class FeatureFlagsRelease implements FeatureFlags, Dumpable { - SparseBooleanArray mAccessedFlags = new SparseBooleanArray(); + private final Resources mResources; + SparseBooleanArray mFlagCache = new SparseBooleanArray(); @Inject - public FeatureFlagsRelease(DumpManager dumpManager) { + public FeatureFlagsRelease(@Main Resources resources, DumpManager dumpManager) { + mResources = resources; dumpManager.registerDumpable("SysUIFlags", this); } @@ -54,19 +58,29 @@ public class FeatureFlagsRelease implements FeatureFlags, Dumpable { return isEnabled(flag.getId(), flag.getDefault()); } + @Override + public boolean isEnabled(ResourceBooleanFlag flag) { + int cacheIndex = mFlagCache.indexOfKey(flag.getId()); + if (cacheIndex < 0) { + return isEnabled(flag.getId(), mResources.getBoolean(flag.getResourceId())); + } + + return mFlagCache.valueAt(cacheIndex); + } + @Override public boolean isEnabled(int key, boolean defaultValue) { - mAccessedFlags.append(key, defaultValue); + mFlagCache.append(key, defaultValue); return defaultValue; } @Override public void dump(@NonNull FileDescriptor fd, @NonNull PrintWriter pw, @NonNull String[] args) { pw.println("can override: false"); - int size = mAccessedFlags.size(); + int size = mFlagCache.size(); for (int i = 0; i < size; i++) { - pw.println(" sysui_flag_" + mAccessedFlags.keyAt(i) - + ": " + mAccessedFlags.valueAt(i)); + pw.println(" sysui_flag_" + mFlagCache.keyAt(i) + + ": " + mFlagCache.valueAt(i)); } } } diff --git a/packages/SystemUI/src/com/android/systemui/flags/Flags.java b/packages/SystemUI/src/com/android/systemui/flags/Flags.java index 458cdc1f78147..c2934931662bf 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/Flags.java +++ b/packages/SystemUI/src/com/android/systemui/flags/Flags.java @@ -61,8 +61,8 @@ 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); + public static final ResourceBooleanFlag CHARGING_RIPPLE = + new ResourceBooleanFlag(203, R.bool.flag_charging_ripple); /***************************************/ // 300 - power menu @@ -77,8 +77,8 @@ 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); + public static final ResourceBooleanFlag SMARTSPACE = + new ResourceBooleanFlag(402, R.bool.flag_smartspace); /***************************************/ // 500 - quick settings @@ -88,11 +88,11 @@ 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); + public static final ResourceBooleanFlag PEOPLE_TILE = + new ResourceBooleanFlag(502, R.bool.flag_conversations); - public static final BooleanFlag QS_USER_DETAIL_SHORTCUT = - new BooleanFlag(503, false, R.bool.flag_lockscreen_qs_user_detail_shortcut); + public static final ResourceBooleanFlag QS_USER_DETAIL_SHORTCUT = + new ResourceBooleanFlag(503, R.bool.flag_lockscreen_qs_user_detail_shortcut); /***************************************/ // 600- status bar @@ -115,8 +115,8 @@ public class Flags { /***************************************/ // 800 - general visual/theme - public static final BooleanFlag MONET = - new BooleanFlag(800, true, R.bool.flag_monet); + public static final ResourceBooleanFlag MONET = + new ResourceBooleanFlag(800, R.bool.flag_monet); /***************************************/ // 900 - media diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java index 475dde204185e..dea42f969866b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java @@ -18,15 +18,14 @@ package com.android.systemui.flags; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; -import android.content.Context; +import android.content.res.Resources; import androidx.test.filters.SmallTest; @@ -51,13 +50,14 @@ import java.io.StringWriter; public class FeatureFlagsReleaseTest extends SysuiTestCase { FeatureFlagsRelease mFeatureFlagsRelease; + @Mock private Resources mResources; @Mock private DumpManager mDumpManager; @Before public void setup() { MockitoAnnotations.initMocks(this); - mFeatureFlagsRelease = new FeatureFlagsRelease(mDumpManager); + mFeatureFlagsRelease = new FeatureFlagsRelease(mResources, mDumpManager); } @After @@ -67,16 +67,35 @@ public class FeatureFlagsReleaseTest extends SysuiTestCase { verifyNoMoreInteractions(mDumpManager); } + @Test + public void testBooleanResourceFlag() { + int flagId = 213; + int flagResourceId = 3; + ResourceBooleanFlag flag = new ResourceBooleanFlag(flagId, flagResourceId); + when(mResources.getBoolean(flagResourceId)).thenReturn(true); + + assertThat(mFeatureFlagsRelease.isEnabled(flag)).isTrue(); + } + @Test public void testDump() { + int flagIdA = 213; + int flagIdB = 18; + int flagResourceId = 3; + BooleanFlag flagA = new BooleanFlag(flagIdA, true); + ResourceBooleanFlag flagB = new ResourceBooleanFlag(flagIdB, flagResourceId); + when(mResources.getBoolean(flagResourceId)).thenReturn(true); + // WHEN the flags have been accessed - assertFalse(mFeatureFlagsRelease.isEnabled(1, false)); - assertTrue(mFeatureFlagsRelease.isEnabled(2, true)); + assertThat(mFeatureFlagsRelease.isEnabled(1, false)).isFalse(); + assertThat(mFeatureFlagsRelease.isEnabled(flagA)).isTrue(); + assertThat(mFeatureFlagsRelease.isEnabled(flagB)).isTrue(); // THEN the dump contains the flags and the default values String dump = dumpToString(); assertThat(dump).contains(" sysui_flag_1: false\n"); - assertThat(dump).contains(" sysui_flag_2: true\n"); + assertThat(dump).contains(" sysui_flag_" + flagIdA + ": true\n"); + assertThat(dump).contains(" sysui_flag_" + flagIdB + ": true\n"); } private String dumpToString() {