From 8945b5bcfceb3060fd6ff954d9ce1d703cf4a093 Mon Sep 17 00:00:00 2001 From: Ming-Shin Lu Date: Wed, 3 Aug 2022 13:51:30 +0800 Subject: [PATCH] Fix IME jumpcut when playing user IME animation after IME restarted There are 2 cases that will seeing IME jumpcut when IME insets animation is controlled by the app during IME prcocess being killed & restarted: 1) When IME process been killed for some reasons (e.g. force-stop package ), during IME restarting, DC#computeImeTarget will select the remote insets target as the control target and deliver IME leash to WM shell. If the focused window is not in multi-windowing mode, it may have a timing that IME surface may unexpectedly shown when starting insets animation from the remote. 2) Previously CL[1] fixed an IME surface visiblity issue when enabling shell-transition that related to somehow IME insets source visibility out of sync with the actual IME surface visiblity. The CL fix is setting IME surface alpha value according to the requested visiblity in InsetsSourceConsumer#setControl as a workaround. However, this may cause a side-effect that after IME restarted and leash updated, when the app requests showing IME animation, system calls setControl to the client to set the same leash after IME onPostLayout, then applyRequestedVisibilityToControl will be called before the insets controller calls setInsetsAndAlpha for the animation, if the IME requested visibility has visible, IME jumpcut will happen. To fix this issue: For 1), returning null control target in DC#computeImeTarget for the above special case. For 2), with CL[2] can fully fixed IME visiblity issue by letting the client know the initial visibility of an InsetsSourceControl and correctly animate insets when delivering the new leash or the visibility change of the InsetsSourceControl. for delivering same leash to request animation case, adding a check to call applyRequestedVisibilityToControl only when there is no running animation. [1]: Iaacdf5f57e68b928e2a19036cbd8a137cf320497 [2]: I2c02e97e191ebd83238c0c54908e861d200d4c8d Fix: 239808087 Bug: 209064170 Test: atest DisplayContentTests InsetsSourceConsumerTest Test: manual test steps in b/239808087#comment2 Test: manual test as steps in b/209064170 0) Enabling shell-transition 1) Launching camera app 2) Swiping camera app to launcher 3) Verify status bar appear animation works Change-Id: I242517a9214b36049d94e89d1ee63ffe505b91ac --- .../android/view/InsetsSourceConsumer.java | 4 +- .../view/InsetsSourceConsumerTest.java | 47 +++++++++++++++++-- .../com/android/server/wm/DisplayContent.java | 15 ++++-- .../server/wm/DisplayContentTests.java | 10 +++- 4 files changed, 66 insertions(+), 10 deletions(-) diff --git a/core/java/android/view/InsetsSourceConsumer.java b/core/java/android/view/InsetsSourceConsumer.java index d63c25a093829..5236fe772a7b8 100644 --- a/core/java/android/view/InsetsSourceConsumer.java +++ b/core/java/android/view/InsetsSourceConsumer.java @@ -176,7 +176,9 @@ public class InsetsSourceConsumer { // If we have a new leash, make sure visibility is up-to-date, even though we // didn't want to run an animation above. - applyRequestedVisibilityToControl(); + if (mController.getAnimationType(control.getType()) == ANIMATION_TYPE_NONE) { + applyRequestedVisibilityToControl(); + } // Remove the surface that owned by last control when it lost. if (!requestedVisible && lastControl == null) { diff --git a/core/tests/coretests/src/android/view/InsetsSourceConsumerTest.java b/core/tests/coretests/src/android/view/InsetsSourceConsumerTest.java index 2054b4fe9a354..8cf118c4b79a7 100644 --- a/core/tests/coretests/src/android/view/InsetsSourceConsumerTest.java +++ b/core/tests/coretests/src/android/view/InsetsSourceConsumerTest.java @@ -18,8 +18,10 @@ package android.view; import static android.view.InsetsController.ANIMATION_TYPE_NONE; import static android.view.InsetsController.ANIMATION_TYPE_USER; +import static android.view.InsetsSourceConsumer.ShowResult.SHOW_IMMEDIATELY; import static android.view.InsetsState.ITYPE_IME; import static android.view.InsetsState.ITYPE_STATUS_BAR; +import static android.view.WindowInsets.Type.ime; import static android.view.WindowInsets.Type.statusBars; import static junit.framework.Assert.assertEquals; @@ -28,6 +30,7 @@ import static junit.framework.TestCase.assertTrue; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; @@ -75,6 +78,7 @@ public class InsetsSourceConsumerTest { private boolean mRemoveSurfaceCalled = false; private InsetsController mController; private InsetsState mState; + private ViewRootImpl mViewRoot; @Before public void setup() { @@ -86,10 +90,9 @@ public class InsetsSourceConsumerTest { instrumentation.runOnMainSync(() -> { final Context context = instrumentation.getTargetContext(); // cannot mock ViewRootImpl since it's final. - final ViewRootImpl viewRootImpl = new ViewRootImpl(context, - context.getDisplayNoVerify()); + mViewRoot = new ViewRootImpl(context, context.getDisplayNoVerify()); try { - viewRootImpl.setView(new TextView(context), new LayoutParams(), null); + mViewRoot.setView(new TextView(context), new LayoutParams(), null); } catch (BadTokenException e) { // activity isn't running, lets ignore BadTokenException. } @@ -97,7 +100,7 @@ public class InsetsSourceConsumerTest { mSpyInsetsSource = Mockito.spy(new InsetsSource(ITYPE_STATUS_BAR)); mState.addSource(mSpyInsetsSource); - mController = new InsetsController(new ViewRootInsetsControllerHost(viewRootImpl)); + mController = new InsetsController(new ViewRootInsetsControllerHost(mViewRoot)); mConsumer = new InsetsSourceConsumer(ITYPE_STATUS_BAR, mState, () -> mMockTransaction, mController) { @Override @@ -207,4 +210,40 @@ public class InsetsSourceConsumerTest { }); } + + @Test + public void testWontUpdateImeLeashVisibility_whenAnimation() { + InstrumentationRegistry.getInstrumentation().runOnMainSync(() -> { + InsetsState state = new InsetsState(); + ViewRootInsetsControllerHost host = new ViewRootInsetsControllerHost(mViewRoot); + InsetsController insetsController = new InsetsController(host, (controller, type) -> { + if (type == ITYPE_IME) { + return new InsetsSourceConsumer(ITYPE_IME, state, + () -> mMockTransaction, controller) { + @Override + public int requestShow(boolean fromController) { + return SHOW_IMMEDIATELY; + } + }; + } + return new InsetsSourceConsumer(type, controller.getState(), Transaction::new, + controller); + }, host.getHandler()); + InsetsSourceConsumer imeConsumer = insetsController.getSourceConsumer(ITYPE_IME); + + // Initial IME insets source control with its leash. + imeConsumer.setControl(new InsetsSourceControl(ITYPE_IME, mLeash, + false /* initialVisible */, new Point(), Insets.NONE), new int[1], new int[1]); + reset(mMockTransaction); + + // Verify when the app requests controlling show IME animation, the IME leash + // visibility won't be updated when the consumer received the same leash in setControl. + insetsController.controlWindowInsetsAnimation(ime(), 0L, + null /* interpolator */, null /* cancellationSignal */, null /* listener */); + assertTrue(insetsController.getAnimationType(ITYPE_IME) == ANIMATION_TYPE_USER); + imeConsumer.setControl(new InsetsSourceControl(ITYPE_IME, mLeash, + true /* initialVisible */, new Point(), Insets.NONE), new int[1], new int[1]); + verify(mMockTransaction, never()).show(mLeash); + }); + } } diff --git a/services/core/java/com/android/server/wm/DisplayContent.java b/services/core/java/com/android/server/wm/DisplayContent.java index b54cd415d8df4..02146e340e74d 100644 --- a/services/core/java/com/android/server/wm/DisplayContent.java +++ b/services/core/java/com/android/server/wm/DisplayContent.java @@ -4416,13 +4416,20 @@ class DisplayContent extends RootDisplayArea implements WindowManagerPolicy.Disp */ @VisibleForTesting InsetsControlTarget computeImeControlTarget() { + if (mImeInputTarget == null) { + // A special case that if there is no IME input target while the IME is being killed, + // in case seeing unexpected IME surface visibility change when delivering the IME leash + // to the remote insets target during the IME restarting, but the focus window is not in + // multi-windowing mode, return null target until the next input target updated. + return null; + } + + final WindowState imeInputTarget = mImeInputTarget.getWindowState(); if (!isImeControlledByApp() && mRemoteInsetsControlTarget != null - || (mImeInputTarget != null - && getImeHostOrFallback(mImeInputTarget.getWindowState()) - == mRemoteInsetsControlTarget)) { + || getImeHostOrFallback(imeInputTarget) == mRemoteInsetsControlTarget) { return mRemoteInsetsControlTarget; } else { - return mImeInputTarget != null ? mImeInputTarget.getWindowState() : null; + return imeInputTarget; } } diff --git a/services/tests/wmtests/src/com/android/server/wm/DisplayContentTests.java b/services/tests/wmtests/src/com/android/server/wm/DisplayContentTests.java index 641a3adf337a9..bc1df4ebe3e3f 100644 --- a/services/tests/wmtests/src/com/android/server/wm/DisplayContentTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/DisplayContentTests.java @@ -1251,7 +1251,15 @@ public class DisplayContentTests extends WindowTestsBase { public void testComputeImeControlTarget() throws Exception { final DisplayContent dc = createNewDisplay(); dc.setRemoteInsetsController(createDisplayWindowInsetsController()); - dc.setImeInputTarget(createWindow(null, TYPE_BASE_APPLICATION, "app")); + dc.mCurrentFocus = createWindow(null, TYPE_BASE_APPLICATION, "app"); + + // Expect returning null IME control target when the focus window has not yet been the + // IME input target (e.g. IME is restarting) in fullscreen windowing mode. + dc.setImeInputTarget(null); + assertFalse(dc.mCurrentFocus.inMultiWindowMode()); + assertNull(dc.computeImeControlTarget()); + + dc.setImeInputTarget(dc.mCurrentFocus); dc.setImeLayeringTarget(dc.getImeInputTarget().getWindowState()); assertEquals(dc.getImeInputTarget().getWindowState(), dc.computeImeControlTarget()); }