From 4c9612be3089e0c3458a61094bc288956b5f1d3a Mon Sep 17 00:00:00 2001 From: Stefan Andonian Date: Wed, 22 Feb 2023 00:00:03 +0000 Subject: [PATCH 1/2] Revert "Revert "Migrate IDP_GRID_NAME usage to LauncherPrefs"" This reverts commit df11959779ca08a48308d9e390eb5b3fdb4bbf35. Bug: 269569568 Test: In follow-up CL, everything works on device and manual and unit tests verified original bug from this CL is not present. Change-Id: I125bbbfd6442cd3fa1e88c9109de536b88981dc6 --- .../launcher3/InvariantDeviceProfile.java | 13 ++--- src/com/android/launcher3/LauncherPrefs.kt | 52 ++++++++++--------- .../android/launcher3/LauncherPrefsTest.kt | 2 +- 3 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/com/android/launcher3/InvariantDeviceProfile.java b/src/com/android/launcher3/InvariantDeviceProfile.java index 604c1b87e6..2fb0fa6207 100644 --- a/src/com/android/launcher3/InvariantDeviceProfile.java +++ b/src/com/android/launcher3/InvariantDeviceProfile.java @@ -16,6 +16,7 @@ package com.android.launcher3; +import static com.android.launcher3.LauncherPrefs.GRID_NAME; import static com.android.launcher3.Utilities.dpiFromPx; import static com.android.launcher3.config.FeatureFlags.ENABLE_DEVICE_PROFILE_LOGGING; import static com.android.launcher3.config.FeatureFlags.ENABLE_TWO_PANEL_HOME; @@ -93,8 +94,6 @@ public class InvariantDeviceProfile { public static final int TYPE_MULTI_DISPLAY = 1; public static final int TYPE_TABLET = 2; - private static final String KEY_IDP_GRID_NAME = "idp_grid_name"; - private static final float ICON_SIZE_DEFINED_IN_APP_DP = 48; // Constants that affects the interpolation curve between statically defined device profile @@ -207,8 +206,7 @@ public class InvariantDeviceProfile { String gridName = getCurrentGridName(context); String newGridName = initGrid(context, gridName); if (!newGridName.equals(gridName)) { - LauncherPrefs.getPrefs(context).edit().putString(KEY_IDP_GRID_NAME, newGridName) - .apply(); + LauncherPrefs.get(context).put(GRID_NAME, newGridName); } new DeviceGridState(this).writeToPrefs(context); @@ -316,7 +314,7 @@ public class InvariantDeviceProfile { } public static String getCurrentGridName(Context context) { - return LauncherPrefs.getPrefs(context).getString(KEY_IDP_GRID_NAME, null); + return LauncherPrefs.get(context).get(GRID_NAME); } private String initGrid(Context context, String gridName) { @@ -458,9 +456,8 @@ public class InvariantDeviceProfile { public void setCurrentGrid(Context context, String gridName) { - Context appContext = context.getApplicationContext(); - LauncherPrefs.getPrefs(appContext).edit().putString(KEY_IDP_GRID_NAME, gridName).apply(); - MAIN_EXECUTOR.execute(() -> onConfigChanged(appContext)); + LauncherPrefs.get(context).put(GRID_NAME, gridName); + MAIN_EXECUTOR.execute(() -> onConfigChanged(context.getApplicationContext())); } private Object[] toModelState() { diff --git a/src/com/android/launcher3/LauncherPrefs.kt b/src/com/android/launcher3/LauncherPrefs.kt index 2e07e3022a..befaa64d50 100644 --- a/src/com/android/launcher3/LauncherPrefs.kt +++ b/src/com/android/launcher3/LauncherPrefs.kt @@ -4,6 +4,8 @@ import android.content.Context import android.content.SharedPreferences import android.content.SharedPreferences.OnSharedPreferenceChangeListener import androidx.annotation.VisibleForTesting +import com.android.launcher3.LauncherFiles.DEVICE_PREFERENCES_KEY +import com.android.launcher3.LauncherFiles.SHARED_PREFERENCES_KEY import com.android.launcher3.allapps.WorkProfileManager import com.android.launcher3.model.DeviceGridState import com.android.launcher3.pm.InstallSessionHelper @@ -20,11 +22,10 @@ import com.android.launcher3.util.Themes class LauncherPrefs(private val context: Context) { /** Wrapper around `getInner` for a `ContextualItem` */ - fun get(item: ContextualItem): T = - getInner(item, item.defaultValueFromContext(context)) + fun get(item: ContextualItem): T = getInner(item, item.defaultValueFromContext(context)) /** Wrapper around `getInner` for an `Item` */ - fun get(item: ConstantItem): T = getInner(item, item.defaultValue) + fun get(item: ConstantItem): T = getInner(item, item.defaultValue) /** * Retrieves the value for an [Item] from [SharedPreferences]. It handles method typing via the @@ -32,11 +33,11 @@ class LauncherPrefs(private val context: Context) { * `String`, `Boolean`, `Float`, `Int`, `Long`, or `Set`. */ @Suppress("IMPLICIT_CAST_TO_ANY", "UNCHECKED_CAST") - private fun getInner(item: Item, default: T): T { + private fun getInner(item: Item, default: T): T { val sp = context.getSharedPreferences(item.sharedPrefFile, Context.MODE_PRIVATE) - return when (default::class.java) { - String::class.java -> sp.getString(item.sharedPrefKey, default as String) + return when (item.type) { + String::class.java -> sp.getString(item.sharedPrefKey, default as? String) Boolean::class.java, java.lang.Boolean::class.java -> sp.getBoolean(item.sharedPrefKey, default as Boolean) Int::class.java, @@ -45,11 +46,10 @@ class LauncherPrefs(private val context: Context) { java.lang.Float::class.java -> sp.getFloat(item.sharedPrefKey, default as Float) Long::class.java, java.lang.Long::class.java -> sp.getLong(item.sharedPrefKey, default as Long) - Set::class.java -> sp.getStringSet(item.sharedPrefKey, default as Set) + Set::class.java -> sp.getStringSet(item.sharedPrefKey, default as? Set) else -> throw IllegalArgumentException( - "item type: ${default::class.java}" + - " is not compatible with sharedPref methods" + "item type: ${item.type}" + " is not compatible with sharedPref methods" ) } as T @@ -224,39 +224,36 @@ class LauncherPrefs(private val context: Context) { backedUpItem(RestoreDbTask.RESTORED_DEVICE_TYPE, InvariantDeviceProfile.TYPE_PHONE) @JvmField val APP_WIDGET_IDS = backedUpItem(RestoreDbTask.APPWIDGET_IDS, "") @JvmField val OLD_APP_WIDGET_IDS = backedUpItem(RestoreDbTask.APPWIDGET_OLD_IDS, "") + @JvmField val GRID_NAME = ConstantItem("idp_grid_name", true, null, String::class.java) @JvmField val ALLOW_ROTATION = - backedUpItem(RotationHelper.ALLOW_ROTATION_PREFERENCE_KEY) { + backedUpItem(RotationHelper.ALLOW_ROTATION_PREFERENCE_KEY, Boolean::class.java) { RotationHelper.getAllowRotationDefaultValue(DisplayController.INSTANCE.get(it).info) } @VisibleForTesting @JvmStatic fun backedUpItem(sharedPrefKey: String, defaultValue: T): ConstantItem = - ConstantItem(sharedPrefKey, LauncherFiles.SHARED_PREFERENCES_KEY, defaultValue) + ConstantItem(sharedPrefKey, true, defaultValue) @JvmStatic fun backedUpItem( sharedPrefKey: String, + type: Class, defaultValueFromContext: (c: Context) -> T - ): ContextualItem = - ContextualItem( - sharedPrefKey, - LauncherFiles.SHARED_PREFERENCES_KEY, - defaultValueFromContext - ) + ): ContextualItem = ContextualItem(sharedPrefKey, true, defaultValueFromContext, type) @VisibleForTesting @JvmStatic fun nonRestorableItem(sharedPrefKey: String, defaultValue: T): ConstantItem = - ConstantItem(sharedPrefKey, LauncherFiles.DEVICE_PREFERENCES_KEY, defaultValue) + ConstantItem(sharedPrefKey, false, defaultValue) @Deprecated("Don't use shared preferences directly. Use other LauncherPref methods.") @JvmStatic fun getPrefs(context: Context): SharedPreferences { // Use application context for shared preferences, so we use single cached instance return context.applicationContext.getSharedPreferences( - LauncherFiles.SHARED_PREFERENCES_KEY, + SHARED_PREFERENCES_KEY, Context.MODE_PRIVATE ) } @@ -266,7 +263,7 @@ class LauncherPrefs(private val context: Context) { fun getDevicePrefs(context: Context): SharedPreferences { // Use application context for shared preferences, so we use a single cached instance return context.applicationContext.getSharedPreferences( - LauncherFiles.DEVICE_PREFERENCES_KEY, + DEVICE_PREFERENCES_KEY, Context.MODE_PRIVATE ) } @@ -275,21 +272,26 @@ class LauncherPrefs(private val context: Context) { abstract class Item { abstract val sharedPrefKey: String - abstract val sharedPrefFile: String + abstract val isBackedUp: Boolean + abstract val type: Class<*> + val sharedPrefFile: String = if (isBackedUp) SHARED_PREFERENCES_KEY else DEVICE_PREFERENCES_KEY fun to(value: T): Pair = Pair(this, value) } data class ConstantItem( override val sharedPrefKey: String, - override val sharedPrefFile: String, - val defaultValue: T + override val isBackedUp: Boolean, + val defaultValue: T, + // The default value can be null. If so, the type needs to be explicitly stated, or else NPE + override val type: Class = defaultValue!!::class.java ) : Item() data class ContextualItem( override val sharedPrefKey: String, - override val sharedPrefFile: String, - private val defaultSupplier: (c: Context) -> T + override val isBackedUp: Boolean, + private val defaultSupplier: (c: Context) -> T, + override val type: Class ) : Item() { private var default: T? = null diff --git a/tests/src/com/android/launcher3/LauncherPrefsTest.kt b/tests/src/com/android/launcher3/LauncherPrefsTest.kt index 31e8d3099b..d40a7bcf8c 100644 --- a/tests/src/com/android/launcher3/LauncherPrefsTest.kt +++ b/tests/src/com/android/launcher3/LauncherPrefsTest.kt @@ -13,7 +13,7 @@ import org.junit.runner.RunWith private val TEST_BOOLEAN_ITEM = LauncherPrefs.nonRestorableItem("1", false) private val TEST_STRING_ITEM = LauncherPrefs.nonRestorableItem("2", "( ͡❛ ͜ʖ ͡❛)") private val TEST_INT_ITEM = LauncherPrefs.nonRestorableItem("3", -1) -private val TEST_CONTEXTUAL_ITEM = LauncherPrefs.backedUpItem("4") { true } +private val TEST_CONTEXTUAL_ITEM = ContextualItem("4", true, { true }, Boolean::class.java) @SmallTest @RunWith(AndroidJUnit4::class) From 246eeaaea1a1bf74103fc61c2365acc267904120 Mon Sep 17 00:00:00 2001 From: Stefan Andonian Date: Tue, 21 Feb 2023 22:14:47 +0000 Subject: [PATCH 2/2] Add unit test and fix bug related to "Migrate IDP_GRID_NAME usage to LauncherPrefs" CL. The issue was that kotlin initializes class variables before contructor variables, but allows constructor variables to be referenced by the class variables before they are initialized. This led to `isBackedUp` always being false, and then the SharedPreference store always and only checking if values were in DEVICE_PREFERENCES rather than the standard backed up preferences. The fix is to make the class variable sharedPrefFile lazily retrieved. That way, isBackedUp will have been initialized and behave correctly. Bug: 269569568 Test: Everything works on device and manual and unit tests verified original bug is not present. Change-Id: I8ab4a5752886ce8f4a2988295fa61c8a2c38ec7c --- src/com/android/launcher3/LauncherPrefs.kt | 3 ++- .../com/android/launcher3/LauncherPrefsTest.kt | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/com/android/launcher3/LauncherPrefs.kt b/src/com/android/launcher3/LauncherPrefs.kt index befaa64d50..e5a1879b4f 100644 --- a/src/com/android/launcher3/LauncherPrefs.kt +++ b/src/com/android/launcher3/LauncherPrefs.kt @@ -274,7 +274,8 @@ abstract class Item { abstract val sharedPrefKey: String abstract val isBackedUp: Boolean abstract val type: Class<*> - val sharedPrefFile: String = if (isBackedUp) SHARED_PREFERENCES_KEY else DEVICE_PREFERENCES_KEY + val sharedPrefFile: String + get() = if (isBackedUp) SHARED_PREFERENCES_KEY else DEVICE_PREFERENCES_KEY fun to(value: T): Pair = Pair(this, value) } diff --git a/tests/src/com/android/launcher3/LauncherPrefsTest.kt b/tests/src/com/android/launcher3/LauncherPrefsTest.kt index d40a7bcf8c..e8372b1f24 100644 --- a/tests/src/com/android/launcher3/LauncherPrefsTest.kt +++ b/tests/src/com/android/launcher3/LauncherPrefsTest.kt @@ -15,6 +15,9 @@ private val TEST_STRING_ITEM = LauncherPrefs.nonRestorableItem("2", "( ͡❛  private val TEST_INT_ITEM = LauncherPrefs.nonRestorableItem("3", -1) private val TEST_CONTEXTUAL_ITEM = ContextualItem("4", true, { true }, Boolean::class.java) +private const val TEST_DEFAULT_VALUE = "default" +private const val TEST_PREF_KEY = "test_pref_key" + @SmallTest @RunWith(AndroidJUnit4::class) class LauncherPrefsTest { @@ -151,4 +154,16 @@ class LauncherPrefsTest { fun get_contextualItem_returnsCorrectDefault() { assertThat(launcherPrefs.get(TEST_CONTEXTUAL_ITEM)).isTrue() } + + @Test + fun getItemSharedPrefFile_forNonRestorableItem_isCorrect() { + val nonRestorableItem = LauncherPrefs.nonRestorableItem(TEST_PREF_KEY, TEST_DEFAULT_VALUE) + assertThat(nonRestorableItem.sharedPrefFile).isEqualTo(LauncherFiles.DEVICE_PREFERENCES_KEY) + } + + @Test + fun getItemSharedPrefFile_forBackedUpItem_isCorrect() { + val backedUpItem = LauncherPrefs.backedUpItem(TEST_PREF_KEY, TEST_DEFAULT_VALUE) + assertThat(backedUpItem.sharedPrefFile).isEqualTo(LauncherFiles.SHARED_PREFERENCES_KEY) + } }