From b9db23114a4b5d1ebdc086dbfe444a4a82fb11e3 Mon Sep 17 00:00:00 2001 From: Charles Chen Date: Tue, 15 Mar 2022 14:31:06 +0000 Subject: [PATCH] Fix #testChangeFontScaleNoRelaunch flaky The test failed because we used public config diff to determine if Activity#onConfigurationChanged should be dispatched. However, the public config diff contained the change which wouldn't make Activity relaunch and also receive the callback. This CL uses the public diff, which filters the change without relaunching Activity, to determine if the callback should be dispatched. This CL also covers the case that the callback is still dispatched if small changes are handled. Test: atest ActivityThreadClientTest ConfigChangeTests Bug: 222067515 Bug: 222223467 Change-Id: I8aa80e9fd70fddc9f88a36e7dc2ffb2163b9ffa6 --- core/java/android/app/Activity.java | 2 +- core/java/android/app/ActivityThread.java | 46 +++++++++++++++--- .../activity/ActivityThreadClientTest.java | 47 +++++++++++++++++++ 3 files changed, 87 insertions(+), 8 deletions(-) diff --git a/core/java/android/app/Activity.java b/core/java/android/app/Activity.java index 8f348a4e81d69..51c52ad3bf032 100644 --- a/core/java/android/app/Activity.java +++ b/core/java/android/app/Activity.java @@ -872,7 +872,7 @@ public class Activity extends ContextThemeWrapper @UnsupportedAppUsage /*package*/ int mConfigChangeFlags; @UnsupportedAppUsage - /*package*/ Configuration mCurrentConfig; + /*package*/ Configuration mCurrentConfig = Configuration.EMPTY; private SearchManager mSearchManager; private MenuInflater mMenuInflater; diff --git a/core/java/android/app/ActivityThread.java b/core/java/android/app/ActivityThread.java index 64f0301fab22b..f4b6a27c5f65e 100644 --- a/core/java/android/app/ActivityThread.java +++ b/core/java/android/app/ActivityThread.java @@ -5883,19 +5883,17 @@ public final class ActivityThread extends ClientTransactionHandler final boolean movedToDifferentDisplay = isDifferentDisplay(activity.getDisplayId(), displayId); final Configuration currentConfig = activity.mCurrentConfig; - final int diff = (currentConfig == null) ? 0xffffffff - : currentConfig.diffPublicOnly(newConfig); + final int diff = currentConfig.diffPublicOnly(newConfig); final boolean hasPublicConfigChange = diff != 0; + final ActivityClientRecord r = getActivityClient(activityToken); // TODO(b/173090263): Use diff instead after the improvement of AssetManager and // ResourcesImpl constructions. final boolean shouldUpdateResources = hasPublicConfigChange || shouldUpdateResources(activityToken, currentConfig, newConfig, amOverrideConfig, movedToDifferentDisplay, hasPublicConfigChange); - final boolean shouldReportChange = hasPublicConfigChange - // If this activity doesn't handle any of the config changes, then don't bother - // calling onConfigurationChanged. Otherwise, report to the activity for the - // changes. - && (~activity.mActivityInfo.getRealConfigChanged() & diff) == 0; + final boolean shouldReportChange = shouldReportChange(diff, currentConfig, newConfig, + r != null ? r.mSizeConfigurations : null, + activity.mActivityInfo.getRealConfigChanged()); // Nothing significant, don't proceed with updating and reporting. if (!shouldUpdateResources) { return null; @@ -5942,6 +5940,40 @@ public final class ActivityThread extends ClientTransactionHandler return configToReport; } + /** + * Returns {@code true} if {@link Activity#onConfigurationChanged(Configuration)} should be + * dispatched. + * + * @param publicDiff Usually computed by {@link Configuration#diffPublicOnly(Configuration)}. + * This parameter is to prevent we compute it again. + * @param currentConfig The current configuration cached in {@link Activity#mCurrentConfig}. + * It is {@code null} before the first config update from the server side. + * @param newConfig The updated {@link Configuration} + * @param sizeBuckets The Activity's {@link SizeConfigurationBuckets} if not {@code null} + * @param handledConfigChanges Bit mask of configuration changes that the activity can handle + * @return {@code true} if the config change should be reported to the Activity + */ + @VisibleForTesting + public static boolean shouldReportChange(int publicDiff, @Nullable Configuration currentConfig, + @NonNull Configuration newConfig, @Nullable SizeConfigurationBuckets sizeBuckets, + int handledConfigChanges) { + // Don't report the change if there's no public diff between current and new config. + if (publicDiff == 0) { + return false; + } + final int diffWithBucket = SizeConfigurationBuckets.filterDiff(publicDiff, currentConfig, + newConfig, sizeBuckets); + // Compare to the diff which filter the change without crossing size buckets with + // {@code handledConfigChanges}. The small changes should not block Activity to receive + // its handled config updates. Also, if Activity handles all small changes, we should + // dispatch the updated config to it. + final int diff = diffWithBucket != 0 ? diffWithBucket : publicDiff; + // If this activity doesn't handle any of the config changes, then don't bother + // calling onConfigurationChanged. Otherwise, report to the activity for the + // changes. + return (~handledConfigChanges & diff) == 0; + } + public final void applyConfigurationToResources(Configuration config) { synchronized (mResourcesManager) { mResourcesManager.applyConfigurationToResources(config, null); diff --git a/core/tests/mockingcoretests/src/android/app/activity/ActivityThreadClientTest.java b/core/tests/mockingcoretests/src/android/app/activity/ActivityThreadClientTest.java index f8db069b99ce9..b006a1681a996 100644 --- a/core/tests/mockingcoretests/src/android/app/activity/ActivityThreadClientTest.java +++ b/core/tests/mockingcoretests/src/android/app/activity/ActivityThreadClientTest.java @@ -16,12 +16,15 @@ package android.app.activity; +import static android.app.ActivityThread.shouldReportChange; import static android.app.servertransaction.ActivityLifecycleItem.ON_CREATE; import static android.app.servertransaction.ActivityLifecycleItem.ON_DESTROY; import static android.app.servertransaction.ActivityLifecycleItem.ON_PAUSE; import static android.app.servertransaction.ActivityLifecycleItem.ON_RESUME; import static android.app.servertransaction.ActivityLifecycleItem.ON_START; import static android.app.servertransaction.ActivityLifecycleItem.ON_STOP; +import static android.content.pm.ActivityInfo.CONFIG_FONT_SCALE; +import static android.content.pm.ActivityInfo.CONFIG_SCREEN_SIZE; import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; @@ -31,6 +34,8 @@ import static com.android.dx.mockito.inline.extended.ExtendedMockito.mockitoSess import static com.android.dx.mockito.inline.extended.ExtendedMockito.spyOn; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.clearInvocations; @@ -56,6 +61,7 @@ import android.os.UserHandle; import android.platform.test.annotations.Presubmit; import android.testing.PollingCheck; import android.view.WindowManagerGlobal; +import android.window.SizeConfigurationBuckets; import androidx.test.annotation.UiThreadTest; import androidx.test.ext.junit.runners.AndroidJUnit4; @@ -195,6 +201,47 @@ public class ActivityThreadClientTest { } } + @Test + public void testShouldReportChange() { + final Configuration newConfig = new Configuration(); + final Configuration currentConfig = new Configuration(); + + assertFalse("Must not report change if no public diff", + shouldReportChange(0 /* publicDiff */, currentConfig, newConfig, + null /* sizeBuckets */, 0 /* handledConfigChanges */)); + + final int[] verticalThresholds = {100, 400}; + final SizeConfigurationBuckets buckets = new SizeConfigurationBuckets( + null /* horizontal */, + verticalThresholds, + null /* smallest */, + null /* screenLayoutSize */, + false /* screenLayoutLongSet */); + currentConfig.screenHeightDp = 200; + newConfig.screenHeightDp = 300; + + assertFalse("Must not report changes if the diff is small and not handled", + shouldReportChange(CONFIG_SCREEN_SIZE /* publicDiff */, currentConfig, + newConfig, buckets, CONFIG_FONT_SCALE /* handledConfigChanges */)); + + assertTrue("Must report changes if the small diff is handled", + shouldReportChange(CONFIG_SCREEN_SIZE /* publicDiff */, currentConfig, newConfig, + buckets, CONFIG_SCREEN_SIZE /* handledConfigChanges */)); + + currentConfig.fontScale = 0.8f; + newConfig.fontScale = 1.2f; + + assertTrue("Must report handled changes regardless of small unhandled change", + shouldReportChange(CONFIG_SCREEN_SIZE | CONFIG_FONT_SCALE /* publicDiff */, + currentConfig, newConfig, buckets, CONFIG_FONT_SCALE /* handledConfigChanges */)); + + newConfig.screenHeightDp = 500; + + assertFalse("Must not report changes if there's unhandled big changes", + shouldReportChange(CONFIG_SCREEN_SIZE | CONFIG_FONT_SCALE /* publicDiff */, + currentConfig, newConfig, buckets, CONFIG_FONT_SCALE /* handledConfigChanges */)); + } + private void recreateAndVerifyNoRelaunch(ActivityThread activityThread, TestActivity activity) { clearInvocations(activityThread); getInstrumentation().runOnMainSync(() -> activity.recreate());