From 7af659da9e1296c0178924eb9f0fd32fd11939e7 Mon Sep 17 00:00:00 2001 From: Alex Chau Date: Mon, 15 May 2023 17:14:39 +0100 Subject: [PATCH] DisplayController should deep compare mPerDisplayBounds - In CHANGE_SUPPORTED_BOUNDS check, it uses Map.equals, but as the value is a primitive array WindowBounds[], equals check does not deep compare the arrays and may result in false negative - One example is when fontScale changes, DisplayController re-create bounds from WindowManagerProxy, result in new WindowBounds[] created, and the shallow compare results in unexpected CHANGE_SUPPORTED_BOUNDS Fix: 282736623 Test: DisplayControllerTest Change-Id: I3897595c58559192b951ecfee7c9f62a07dafe1f --- .../util/SystemWindowManagerProxy.java | 7 +- .../OrientationTouchTransformerTest.java | 4 +- .../quickstep/util/TaskViewSimulatorTest.java | 9 +- .../launcher3/util/DisplayController.java | 25 ++- .../util/MainThreadInitializedObject.java | 3 +- .../util/window/WindowManagerProxy.java | 16 +- .../launcher3/AbstractDeviceProfileTest.kt | 10 +- .../launcher3/util/DisplayControllerTest.kt | 160 ++++++++++++++++++ 8 files changed, 202 insertions(+), 32 deletions(-) create mode 100644 tests/src/com/android/launcher3/util/DisplayControllerTest.kt diff --git a/quickstep/src/com/android/quickstep/util/SystemWindowManagerProxy.java b/quickstep/src/com/android/quickstep/util/SystemWindowManagerProxy.java index a34888f684..c82cdb7c3c 100644 --- a/quickstep/src/com/android/quickstep/util/SystemWindowManagerProxy.java +++ b/quickstep/src/com/android/quickstep/util/SystemWindowManagerProxy.java @@ -28,6 +28,7 @@ import com.android.launcher3.util.WindowBounds; import com.android.launcher3.util.window.CachedDisplayInfo; import com.android.launcher3.util.window.WindowManagerProxy; +import java.util.List; import java.util.Set; /** @@ -53,15 +54,15 @@ public class SystemWindowManagerProxy extends WindowManagerProxy { } @Override - public ArrayMap estimateInternalDisplayBounds( + public ArrayMap> estimateInternalDisplayBounds( Context displayInfoContext) { - ArrayMap result = new ArrayMap<>(); + ArrayMap> result = new ArrayMap<>(); WindowManager windowManager = displayInfoContext.getSystemService(WindowManager.class); Set possibleMaximumWindowMetrics = windowManager.getPossibleMaximumWindowMetrics(DEFAULT_DISPLAY); for (WindowMetrics windowMetrics : possibleMaximumWindowMetrics) { CachedDisplayInfo info = getDisplayInfo(windowMetrics, Surface.ROTATION_0); - WindowBounds[] bounds = estimateWindowBounds(displayInfoContext, info); + List bounds = estimateWindowBounds(displayInfoContext, info); result.put(info, bounds); } return result; diff --git a/quickstep/tests/src/com/android/quickstep/OrientationTouchTransformerTest.java b/quickstep/tests/src/com/android/quickstep/OrientationTouchTransformerTest.java index 9c240f08c7..298dd6cae0 100644 --- a/quickstep/tests/src/com/android/quickstep/OrientationTouchTransformerTest.java +++ b/quickstep/tests/src/com/android/quickstep/OrientationTouchTransformerTest.java @@ -53,6 +53,8 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.MockitoAnnotations; +import java.util.List; + @SmallTest @RunWith(AndroidJUnit4.class) public class OrientationTouchTransformerTest { @@ -296,7 +298,7 @@ public class OrientationTouchTransformerTest { WindowManagerProxy wmProxy = mock(WindowManagerProxy.class); doReturn(cachedDisplayInfo).when(wmProxy).getDisplayInfo(any()); doReturn(windowBounds).when(wmProxy).getRealBounds(any(), any()); - ArrayMap internalDisplayBounds = new ArrayMap<>(); + ArrayMap> internalDisplayBounds = new ArrayMap<>(); doReturn(internalDisplayBounds).when(wmProxy).estimateInternalDisplayBounds(any()); return new DisplayController.Info( getApplicationContext(), wmProxy, new ArrayMap<>()); diff --git a/quickstep/tests/src/com/android/quickstep/util/TaskViewSimulatorTest.java b/quickstep/tests/src/com/android/quickstep/util/TaskViewSimulatorTest.java index 83602be72e..a54dc2da18 100644 --- a/quickstep/tests/src/com/android/quickstep/util/TaskViewSimulatorTest.java +++ b/quickstep/tests/src/com/android/quickstep/util/TaskViewSimulatorTest.java @@ -50,6 +50,9 @@ import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; +import java.util.ArrayList; +import java.util.List; + @SmallTest @RunWith(AndroidJUnit4.class) public class TaskViewSimulatorTest { @@ -150,7 +153,7 @@ public class TaskViewSimulatorTest { WindowBounds wm = new WindowBounds( new Rect(0, 0, mDisplaySize.x, mDisplaySize.y), mDisplayInsets); - WindowBounds[] allBounds = new WindowBounds[4]; + List allBounds = new ArrayList<>(4); for (int i = 0; i < 4; i++) { Rect boundsR = new Rect(wm.bounds); Rect insetsR = new Rect(wm.insets); @@ -158,7 +161,7 @@ public class TaskViewSimulatorTest { RotationUtils.rotateRect(insetsR, RotationUtils.deltaRotation(rotation, i)); RotationUtils.rotateRect(boundsR, RotationUtils.deltaRotation(rotation, i)); boundsR.set(0, 0, Math.abs(boundsR.width()), Math.abs(boundsR.height())); - allBounds[i] = new WindowBounds(boundsR, insetsR); + allBounds.add(new WindowBounds(boundsR, insetsR)); } WindowManagerProxy wmProxy = mock(WindowManagerProxy.class); @@ -166,7 +169,7 @@ public class TaskViewSimulatorTest { doReturn(wm).when(wmProxy).getRealBounds(any(), any()); doReturn(NavigationMode.NO_BUTTON).when(wmProxy).getNavigationMode(any()); - ArrayMap perDisplayBoundsCache = + ArrayMap> perDisplayBoundsCache = new ArrayMap<>(); perDisplayBoundsCache.put(cdi.normalize(), allBounds); diff --git a/src/com/android/launcher3/util/DisplayController.java b/src/com/android/launcher3/util/DisplayController.java index 776fe40f05..68ed78abda 100644 --- a/src/com/android/launcher3/util/DisplayController.java +++ b/src/com/android/launcher3/util/DisplayController.java @@ -53,8 +53,8 @@ import com.android.launcher3.util.window.WindowManagerProxy; import java.io.PrintWriter; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -105,7 +105,8 @@ public class DisplayController implements ComponentCallbacks, SafeCloseable { private final LauncherPrefs mPrefs; - private DisplayController(Context context) { + @VisibleForTesting + protected DisplayController(Context context) { mContext = context; mDM = context.getSystemService(DisplayManager.class); mPrefs = LauncherPrefs.get(context); @@ -323,7 +324,7 @@ public class DisplayController implements ComponentCallbacks, SafeCloseable { // WindowBounds public final WindowBounds realBounds; public final Set supportedBounds = new ArraySet<>(); - private final ArrayMap mPerDisplayBounds = + private final ArrayMap> mPerDisplayBounds = new ArrayMap<>(); public Info(Context displayInfoContext) { @@ -334,7 +335,7 @@ public class DisplayController implements ComponentCallbacks, SafeCloseable { // Used for testing public Info(Context displayInfoContext, WindowManagerProxy wmProxy, - Map perDisplayBoundsCache) { + Map> perDisplayBoundsCache) { CachedDisplayInfo displayInfo = wmProxy.getDisplayInfo(displayInfoContext); normalizedDisplayInfo = displayInfo.normalize(); rotation = displayInfo.rotation; @@ -348,7 +349,7 @@ public class DisplayController implements ComponentCallbacks, SafeCloseable { navigationMode = wmProxy.getNavigationMode(displayInfoContext); mPerDisplayBounds.putAll(perDisplayBoundsCache); - WindowBounds[] cachedValue = mPerDisplayBounds.get(normalizedDisplayInfo); + List cachedValue = mPerDisplayBounds.get(normalizedDisplayInfo); realBounds = wmProxy.getRealBounds(displayInfoContext, displayInfo); if (cachedValue == null) { @@ -366,22 +367,20 @@ public class DisplayController implements ComponentCallbacks, SafeCloseable { if (cachedValue != null) { // Verify that the real bounds are a match - WindowBounds expectedBounds = cachedValue[displayInfo.rotation]; + WindowBounds expectedBounds = cachedValue.get(displayInfo.rotation); if (!realBounds.equals(expectedBounds)) { - WindowBounds[] clone = new WindowBounds[4]; - System.arraycopy(cachedValue, 0, clone, 0, 4); - clone[displayInfo.rotation] = realBounds; + List clone = new ArrayList<>(cachedValue); + clone.set(displayInfo.rotation, realBounds); mPerDisplayBounds.put(normalizedDisplayInfo, clone); } } - mPerDisplayBounds.values().forEach( - windowBounds -> Collections.addAll(supportedBounds, windowBounds)); + mPerDisplayBounds.values().forEach(supportedBounds::addAll); if (DEBUG) { Log.d(TAG, "displayInfo: " + displayInfo); Log.d(TAG, "realBounds: " + realBounds); Log.d(TAG, "normalizedDisplayInfo: " + normalizedDisplayInfo); mPerDisplayBounds.forEach((key, value) -> Log.d(TAG, - "perDisplayBounds - " + key + ": " + Arrays.deepToString(value))); + "perDisplayBounds - " + key + ": " + value)); } } @@ -438,7 +437,7 @@ public class DisplayController implements ComponentCallbacks, SafeCloseable { pw.println(" navigationMode=" + info.navigationMode.name()); pw.println(" currentSize=" + info.currentSize); info.mPerDisplayBounds.forEach((key, value) -> pw.println( - " perDisplayBounds - " + key + ": " + Arrays.deepToString(value))); + " perDisplayBounds - " + key + ": " + value)); } /** diff --git a/src/com/android/launcher3/util/MainThreadInitializedObject.java b/src/com/android/launcher3/util/MainThreadInitializedObject.java index 6a4e528368..0899a22ba7 100644 --- a/src/com/android/launcher3/util/MainThreadInitializedObject.java +++ b/src/com/android/launcher3/util/MainThreadInitializedObject.java @@ -131,7 +131,8 @@ public class MainThreadInitializedObject { * Find a cached object from mObjectMap if we have already created one. If not, generate * an object using the provider. */ - private T getObject(MainThreadInitializedObject object, ObjectProvider provider) { + protected T getObject(MainThreadInitializedObject object, + ObjectProvider provider) { synchronized (mDestroyLock) { if (mDestroyed) { Log.e(TAG, "Static object access with a destroyed context"); diff --git a/src/com/android/launcher3/util/window/WindowManagerProxy.java b/src/com/android/launcher3/util/window/WindowManagerProxy.java index 4093bc913d..278a37e353 100644 --- a/src/com/android/launcher3/util/window/WindowManagerProxy.java +++ b/src/com/android/launcher3/util/window/WindowManagerProxy.java @@ -57,6 +57,9 @@ import com.android.launcher3.util.NavigationMode; import com.android.launcher3.util.ResourceBasedOverride; import com.android.launcher3.util.WindowBounds; +import java.util.ArrayList; +import java.util.List; + /** * Utility class for mocking some window manager behaviours */ @@ -90,11 +93,11 @@ public class WindowManagerProxy implements ResourceBasedOverride { * Returns a map of normalized info of internal displays to estimated window bounds * for that display */ - public ArrayMap estimateInternalDisplayBounds( + public ArrayMap> estimateInternalDisplayBounds( Context displayInfoContext) { CachedDisplayInfo info = getDisplayInfo(displayInfoContext).normalize(); - WindowBounds[] bounds = estimateWindowBounds(displayInfoContext, info); - ArrayMap result = new ArrayMap<>(); + List bounds = estimateWindowBounds(displayInfoContext, info); + ArrayMap> result = new ArrayMap<>(); result.put(info, bounds); return result; } @@ -200,7 +203,8 @@ public class WindowManagerProxy implements ResourceBasedOverride { /** * Returns a list of possible WindowBounds for the display keyed on the 4 surface rotations */ - protected WindowBounds[] estimateWindowBounds(Context context, CachedDisplayInfo displayInfo) { + protected List estimateWindowBounds(Context context, + CachedDisplayInfo displayInfo) { int densityDpi = context.getResources().getConfiguration().densityDpi; int rotation = displayInfo.rotation; Rect safeCutout = displayInfo.cutout; @@ -243,7 +247,7 @@ public class WindowManagerProxy implements ResourceBasedOverride { ? 0 : getDimenByName(systemRes, NAVBAR_LANDSCAPE_LEFT_RIGHT_SIZE); - WindowBounds[] result = new WindowBounds[4]; + List result = new ArrayList<>(4); Point tempSize = new Point(); for (int i = 0; i < 4; i++) { int rotationChange = deltaRotation(rotation, i); @@ -274,7 +278,7 @@ public class WindowManagerProxy implements ResourceBasedOverride { } else { insets.right = Math.max(insets.right, navbarWidth); } - result[i] = new WindowBounds(bounds, insets, i); + result.add(new WindowBounds(bounds, insets, i)); } return result; } diff --git a/tests/src/com/android/launcher3/AbstractDeviceProfileTest.kt b/tests/src/com/android/launcher3/AbstractDeviceProfileTest.kt index 01f494b202..a1c4f90d49 100644 --- a/tests/src/com/android/launcher3/AbstractDeviceProfileTest.kt +++ b/tests/src/com/android/launcher3/AbstractDeviceProfileTest.kt @@ -196,7 +196,7 @@ abstract class AbstractDeviceProfileTest { isGestureMode: Boolean, naturalX: Int, naturalY: Int - ): Array { + ): List { val buttonsNavHeight = Utilities.dpToPx(48f, deviceSpec.densityDpi) val rotation0Insets = @@ -231,7 +231,7 @@ abstract class AbstractDeviceProfileTest { if (isGestureMode) deviceSpec.gesturePx else 0 ) - return arrayOf( + return listOf( WindowBounds(Rect(0, 0, naturalX, naturalY), rotation0Insets, Surface.ROTATION_0), WindowBounds(Rect(0, 0, naturalY, naturalX), rotation90Insets, Surface.ROTATION_90), WindowBounds(Rect(0, 0, naturalX, naturalY), rotation180Insets, Surface.ROTATION_180), @@ -243,11 +243,11 @@ abstract class AbstractDeviceProfileTest { deviceSpec: DeviceSpec, naturalX: Int, naturalY: Int - ): Array { + ): List { val naturalInsets = Rect(0, deviceSpec.statusBarNaturalPx, 0, 0) val rotatedInsets = Rect(0, deviceSpec.statusBarRotatedPx, 0, 0) - return arrayOf( + return listOf( WindowBounds(Rect(0, 0, naturalX, naturalY), naturalInsets, Surface.ROTATION_0), WindowBounds(Rect(0, 0, naturalY, naturalX), rotatedInsets, Surface.ROTATION_90), WindowBounds(Rect(0, 0, naturalX, naturalY), naturalInsets, Surface.ROTATION_180), @@ -256,7 +256,7 @@ abstract class AbstractDeviceProfileTest { } private fun initializeCommonVars( - perDisplayBoundsCache: Map>, + perDisplayBoundsCache: Map>, displayInfo: CachedDisplayInfo, rotation: Int, isGestureMode: Boolean = true, diff --git a/tests/src/com/android/launcher3/util/DisplayControllerTest.kt b/tests/src/com/android/launcher3/util/DisplayControllerTest.kt new file mode 100644 index 0000000000..2f57634355 --- /dev/null +++ b/tests/src/com/android/launcher3/util/DisplayControllerTest.kt @@ -0,0 +1,160 @@ +/* + * Copyright (C) 2023 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.launcher3.util + +import android.content.Context +import android.content.res.Configuration +import android.content.res.Resources +import android.graphics.Point +import android.graphics.Rect +import android.hardware.display.DisplayManager +import android.util.ArrayMap +import android.util.DisplayMetrics +import android.view.Display +import android.view.Surface +import androidx.test.annotation.UiThreadTest +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.filters.SmallTest +import com.android.launcher3.LauncherPrefs +import com.android.launcher3.util.DisplayController.CHANGE_DENSITY +import com.android.launcher3.util.DisplayController.CHANGE_ROTATION +import com.android.launcher3.util.DisplayController.DisplayInfoChangeListener +import com.android.launcher3.util.MainThreadInitializedObject.SandboxContext +import com.android.launcher3.util.window.CachedDisplayInfo +import com.android.launcher3.util.window.WindowManagerProxy +import kotlin.math.min +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mock +import org.mockito.Mockito.doNothing +import org.mockito.Mockito.verify +import org.mockito.Mockito.`when` as whenever +import org.mockito.MockitoAnnotations +import org.mockito.stubbing.Answer + +/** Unit tests for {@link DisplayController} */ +@SmallTest +@RunWith(AndroidJUnit4::class) +class DisplayControllerTest { + + private val appContext: Context = ApplicationProvider.getApplicationContext() + + @Mock private lateinit var context: SandboxContext + @Mock private lateinit var windowManagerProxy: WindowManagerProxy + @Mock private lateinit var launcherPrefs: LauncherPrefs + @Mock private lateinit var displayManager: DisplayManager + @Mock private lateinit var display: Display + @Mock private lateinit var resources: Resources + @Mock private lateinit var displayInfoChangeListener: DisplayInfoChangeListener + + private lateinit var displayController: DisplayController + + private val width = 2208 + private val height = 1840 + private val inset = 110 + private val densityDpi = 420 + private val density = densityDpi / DisplayMetrics.DENSITY_DEFAULT.toFloat() + private val bounds = + arrayOf( + WindowBounds(Rect(0, 0, width, height), Rect(0, inset, 0, 0), Surface.ROTATION_0), + WindowBounds(Rect(0, 0, height, width), Rect(0, inset, 0, 0), Surface.ROTATION_90), + WindowBounds(Rect(0, 0, width, height), Rect(0, inset, 0, 0), Surface.ROTATION_180), + WindowBounds(Rect(0, 0, height, width), Rect(0, inset, 0, 0), Surface.ROTATION_270) + ) + private val configuration = + Configuration(appContext.resources.configuration).apply { + densityDpi = this@DisplayControllerTest.densityDpi + screenWidthDp = (bounds[0].bounds.width() / density).toInt() + screenHeightDp = (bounds[0].bounds.height() / density).toInt() + smallestScreenWidthDp = min(screenWidthDp, screenHeightDp) + } + + @Before + fun setUp() { + MockitoAnnotations.initMocks(this) + whenever(context.getObject(eq(WindowManagerProxy.INSTANCE), any())) + .thenReturn(windowManagerProxy) + whenever(context.getObject(eq(LauncherPrefs.INSTANCE), any())).thenReturn(launcherPrefs) + + // Mock WindowManagerProxy + val displayInfo = + CachedDisplayInfo(Point(width, height), Surface.ROTATION_0, Rect(0, 0, 0, 0)) + whenever(windowManagerProxy.getDisplayInfo(any())).thenReturn(displayInfo) + whenever(windowManagerProxy.estimateInternalDisplayBounds(any())) + .thenAnswer( + Answer { + // Always create a new copy of bounds + val perDisplayBounds = ArrayMap>() + perDisplayBounds[displayInfo] = bounds.toList() + return@Answer perDisplayBounds + } + ) + whenever(windowManagerProxy.getRealBounds(any(), any())).thenAnswer { i -> + bounds[i.getArgument(1).rotation] + } + + // Mock context + whenever(context.createWindowContext(any(), any(), nullable())).thenReturn(context) + whenever(context.getSystemService(eq(DisplayManager::class.java))) + .thenReturn(displayManager) + doNothing().`when`(context).registerComponentCallbacks(any()) + + // Mock display + whenever(display.rotation).thenReturn(displayInfo.rotation) + whenever(context.display).thenReturn(display) + whenever(displayManager.getDisplay(any())).thenReturn(display) + + // Mock resources + whenever(resources.configuration).thenReturn(configuration) + whenever(context.resources).thenReturn(resources) + + // Initialize DisplayController + displayController = DisplayController(context) + displayController.addChangeListener(displayInfoChangeListener) + } + + @Test + @UiThreadTest + fun testRotation() { + val displayInfo = + CachedDisplayInfo(Point(height, width), Surface.ROTATION_90, Rect(0, 0, 0, 0)) + whenever(windowManagerProxy.getDisplayInfo(any())).thenReturn(displayInfo) + whenever(display.rotation).thenReturn(displayInfo.rotation) + val configuration = + Configuration(configuration).apply { + screenWidthDp = configuration.screenHeightDp + screenHeightDp = configuration.screenWidthDp + } + whenever(resources.configuration).thenReturn(configuration) + + displayController.onConfigurationChanged(configuration) + + verify(displayInfoChangeListener).onDisplayInfoChanged(any(), any(), eq(CHANGE_ROTATION)) + } + + @Test + @UiThreadTest + fun testFontScale() { + val configuration = Configuration(configuration).apply { fontScale = 1.2f } + whenever(resources.configuration).thenReturn(configuration) + + displayController.onConfigurationChanged(configuration) + + verify(displayInfoChangeListener).onDisplayInfoChanged(any(), any(), eq(CHANGE_DENSITY)) + } +}