From bfdfe0ec428e30027cddd3391543f53d85ae509a Mon Sep 17 00:00:00 2001 From: Adrian Roos Date: Thu, 24 Sep 2020 14:22:11 +0200 Subject: [PATCH 1/2] DisplayImeController: Refactor for testability Bug: 162875596 Test: atest WMShellUnitTests Change-Id: I3118310a391592fcc72ea6ef8b4b8adccce2f14f --- .../wm/shell/common/DisplayImeController.java | 19 +++++++++++++------ .../wm/DisplaySystemBarsController.java | 4 +++- .../systemui/wmshell/TvWMShellModule.java | 7 +++++-- .../systemui/wmshell/WMShellModule.java | 6 ++++-- 4 files changed, 25 insertions(+), 11 deletions(-) diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/common/DisplayImeController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/common/DisplayImeController.java index c18e9ce761539..1471d1cbc74cd 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/common/DisplayImeController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/common/DisplayImeController.java @@ -43,6 +43,7 @@ import android.view.animation.PathInterpolator; import com.android.internal.view.IInputMethodManager; import java.util.ArrayList; +import java.util.concurrent.Executor; /** * Manages IME control at the display-level. This occurs when IME comes up in multi-window mode. @@ -62,15 +63,21 @@ public class DisplayImeController implements DisplayController.OnDisplaysChanged private static final int FLOATING_IME_BOTTOM_INSET = -80; protected final IWindowManager mWmService; - protected final Handler mHandler; + protected final Executor mExecutor; private final TransactionPool mTransactionPool; private final DisplayController mDisplayController; private final SparseArray mImePerDisplay = new SparseArray<>(); private final ArrayList mPositionProcessors = new ArrayList<>(); + @Deprecated public DisplayImeController(IWindowManager wmService, DisplayController displayController, Handler mainHandler, TransactionPool transactionPool) { - mHandler = mainHandler; + this(wmService, displayController, mainHandler::post, transactionPool); + } + + public DisplayImeController(IWindowManager wmService, DisplayController displayController, + Executor mainExecutor, TransactionPool transactionPool) { + mExecutor = mainExecutor; mWmService = wmService; mTransactionPool = transactionPool; mDisplayController = displayController; @@ -197,7 +204,7 @@ public class DisplayImeController implements DisplayController.OnDisplaysChanged @Override public void insetsChanged(InsetsState insetsState) { - mHandler.post(() -> { + mExecutor.execute(() -> { if (mInsetsState.equals(insetsState)) { return; } @@ -224,7 +231,7 @@ public class DisplayImeController implements DisplayController.OnDisplaysChanged continue; } if (activeControl.getType() == InsetsState.ITYPE_IME) { - mHandler.post(() -> { + mExecutor.execute(() -> { final Point lastSurfacePosition = mImeSourceControl != null ? mImeSourceControl.getSurfacePosition() : null; mImeSourceControl = activeControl; @@ -246,7 +253,7 @@ public class DisplayImeController implements DisplayController.OnDisplaysChanged return; } if (DEBUG) Slog.d(TAG, "Got showInsets for ime"); - mHandler.post(() -> startAnimation(true /* show */, false /* forceRestart */)); + mExecutor.execute(() -> startAnimation(true /* show */, false /* forceRestart */)); } @Override @@ -255,7 +262,7 @@ public class DisplayImeController implements DisplayController.OnDisplaysChanged return; } if (DEBUG) Slog.d(TAG, "Got hideInsets for ime"); - mHandler.post(() -> startAnimation(false /* show */, false /* forceRestart */)); + mExecutor.execute(() -> startAnimation(false /* show */, false /* forceRestart */)); } @Override diff --git a/packages/CarSystemUI/src/com/android/systemui/wm/DisplaySystemBarsController.java b/packages/CarSystemUI/src/com/android/systemui/wm/DisplaySystemBarsController.java index b113d29f00e66..f2ca4956be07f 100644 --- a/packages/CarSystemUI/src/com/android/systemui/wm/DisplaySystemBarsController.java +++ b/packages/CarSystemUI/src/com/android/systemui/wm/DisplaySystemBarsController.java @@ -50,6 +50,7 @@ public class DisplaySystemBarsController extends DisplayImeController { private final Context mContext; private final DisplayController mDisplayController; + private final Handler mHandler; private SparseArray mPerDisplaySparseArray; public DisplaySystemBarsController( @@ -58,9 +59,10 @@ public class DisplaySystemBarsController extends DisplayImeController { DisplayController displayController, @Main Handler mainHandler, TransactionPool transactionPool) { - super(wmService, displayController, mainHandler, transactionPool); + super(wmService, displayController, (r) -> mainHandler.post(r), transactionPool); mContext = context; mDisplayController = displayController; + mHandler = mainHandler; } @Override diff --git a/packages/SystemUI/src/com/android/systemui/wmshell/TvWMShellModule.java b/packages/SystemUI/src/com/android/systemui/wmshell/TvWMShellModule.java index 524eca389cebc..ddfd88c3e6283 100644 --- a/packages/SystemUI/src/com/android/systemui/wmshell/TvWMShellModule.java +++ b/packages/SystemUI/src/com/android/systemui/wmshell/TvWMShellModule.java @@ -31,6 +31,8 @@ import com.android.wm.shell.common.TransactionPool; import com.android.wm.shell.splitscreen.SplitScreen; import com.android.wm.shell.splitscreen.SplitScreenController; +import java.util.concurrent.Executor; + import dagger.Module; import dagger.Provides; @@ -44,9 +46,10 @@ public class TvWMShellModule { @SysUISingleton @Provides static DisplayImeController provideDisplayImeController(IWindowManager wmService, - DisplayController displayController, @Main Handler mainHandler, + DisplayController displayController, @Main Executor mainExecutor, TransactionPool transactionPool) { - return new DisplayImeController(wmService, displayController, mainHandler, transactionPool); + return new DisplayImeController(wmService, displayController, mainExecutor, + transactionPool); } @SysUISingleton diff --git a/packages/SystemUI/src/com/android/systemui/wmshell/WMShellModule.java b/packages/SystemUI/src/com/android/systemui/wmshell/WMShellModule.java index 16fb2cacc9508..14c744cb60bc9 100644 --- a/packages/SystemUI/src/com/android/systemui/wmshell/WMShellModule.java +++ b/packages/SystemUI/src/com/android/systemui/wmshell/WMShellModule.java @@ -44,6 +44,7 @@ import com.android.wm.shell.splitscreen.SplitScreen; import com.android.wm.shell.splitscreen.SplitScreenController; import java.util.Optional; +import java.util.concurrent.Executor; import dagger.Module; import dagger.Provides; @@ -58,9 +59,10 @@ public class WMShellModule { @SysUISingleton @Provides static DisplayImeController provideDisplayImeController(IWindowManager wmService, - DisplayController displayController, @Main Handler mainHandler, + DisplayController displayController, @Main Executor mainExecutor, TransactionPool transactionPool) { - return new DisplayImeController(wmService, displayController, mainHandler, transactionPool); + return new DisplayImeController(wmService, displayController, mainExecutor, + transactionPool); } @SysUISingleton From a3eedb3fa84a38acce5f3c2c26be59a75458f378 Mon Sep 17 00:00:00 2001 From: Adrian Roos Date: Thu, 24 Sep 2020 13:25:45 +0200 Subject: [PATCH 2/2] DisplayImeController: reapply visibility when leash changes Fixes an issue where the DisplayImeController did not re-apply the visibility if it receives a new leash. This lead to focusable IME dialogs not being visible sometimes upon relaunching the IME target activity, such as during rotation. Fixes: 162875596 Bug: 160672060 Test: Enable Braille keyboard, open Settings, click on search box, switch to Braille keyboard, on the confirmation dialog rotate the screen; verify the dialog does not disappear. Change-Id: Ia40a682ca8a9ba669454892e2b453f736c55929c --- .../wm/shell/common/DisplayImeController.java | 50 +++++++++-- .../common/DisplayImeControllerTest.java | 86 +++++++++++++++++++ .../server/wm/WindowManagerService.java | 2 +- 3 files changed, 132 insertions(+), 6 deletions(-) create mode 100644 libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/common/DisplayImeControllerTest.java diff --git a/libs/WindowManager/Shell/src/com/android/wm/shell/common/DisplayImeController.java b/libs/WindowManager/Shell/src/com/android/wm/shell/common/DisplayImeController.java index 1471d1cbc74cd..d810fb8257a9e 100644 --- a/libs/WindowManager/Shell/src/com/android/wm/shell/common/DisplayImeController.java +++ b/libs/WindowManager/Shell/src/com/android/wm/shell/common/DisplayImeController.java @@ -234,12 +234,22 @@ public class DisplayImeController implements DisplayController.OnDisplaysChanged mExecutor.execute(() -> { final Point lastSurfacePosition = mImeSourceControl != null ? mImeSourceControl.getSurfacePosition() : null; + final boolean positionChanged = + !activeControl.getSurfacePosition().equals(lastSurfacePosition); + final boolean leashChanged = + !haveSameLeash(mImeSourceControl, activeControl); mImeSourceControl = activeControl; - if (!activeControl.getSurfacePosition().equals(lastSurfacePosition) - && mAnimation != null) { - startAnimation(mImeShowing, true /* forceRestart */); - } else if (!mImeShowing) { - removeImeSurface(); + if (mAnimation != null) { + if (positionChanged) { + startAnimation(mImeShowing, true /* forceRestart */); + } + } else { + if (leashChanged) { + applyVisibilityToLeash(); + } + if (!mImeShowing) { + removeImeSurface(); + } } }); } @@ -247,6 +257,20 @@ public class DisplayImeController implements DisplayController.OnDisplaysChanged } } + private void applyVisibilityToLeash() { + SurfaceControl leash = mImeSourceControl.getLeash(); + if (leash != null) { + SurfaceControl.Transaction t = mTransactionPool.acquire(); + if (mImeShowing) { + t.show(leash); + } else { + t.hide(leash); + } + t.apply(); + mTransactionPool.release(t); + } + } + @Override public void showInsets(int types, boolean fromIme) { if ((types & WindowInsets.Type.ime()) == 0) { @@ -502,4 +526,20 @@ public class DisplayImeController implements DisplayController.OnDisplaysChanged return IInputMethodManager.Stub.asInterface( ServiceManager.getService(Context.INPUT_METHOD_SERVICE)); } + + private static boolean haveSameLeash(InsetsSourceControl a, InsetsSourceControl b) { + if (a == b) { + return true; + } + if (a == null || b == null) { + return false; + } + if (a.getLeash() == b.getLeash()) { + return true; + } + if (a.getLeash() == null || b.getLeash() == null) { + return false; + } + return a.getLeash().isSameSurface(b.getLeash()); + } } diff --git a/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/common/DisplayImeControllerTest.java b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/common/DisplayImeControllerTest.java new file mode 100644 index 0000000000000..080cddc58a092 --- /dev/null +++ b/libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/common/DisplayImeControllerTest.java @@ -0,0 +1,86 @@ +/* + * Copyright (C) 2020 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.wm.shell.common; + +import static android.view.Display.DEFAULT_DISPLAY; +import static android.view.InsetsState.ITYPE_IME; +import static android.view.Surface.ROTATION_0; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; + +import android.graphics.Point; +import android.view.InsetsSourceControl; +import android.view.InsetsState; +import android.view.SurfaceControl; + +import androidx.test.filters.SmallTest; + +import com.android.internal.view.IInputMethodManager; + +import org.junit.Before; +import org.junit.Test; + +@SmallTest +public class DisplayImeControllerTest { + + private SurfaceControl.Transaction mT; + private DisplayImeController.PerDisplay mPerDisplay; + private IInputMethodManager mMock; + + @Before + public void setUp() throws Exception { + mT = mock(SurfaceControl.Transaction.class); + mMock = mock(IInputMethodManager.class); + mPerDisplay = new DisplayImeController(null, null, Runnable::run, new TransactionPool() { + @Override + public SurfaceControl.Transaction acquire() { + return mT; + } + + @Override + public void release(SurfaceControl.Transaction t) { + } + }) { + @Override + public IInputMethodManager getImms() { + return mMock; + } + }.new PerDisplay(DEFAULT_DISPLAY, ROTATION_0); + } + + @Test + public void reappliesVisibilityToChangedLeash() { + verifyZeroInteractions(mT); + + mPerDisplay.mImeShowing = false; + mPerDisplay.insetsControlChanged(new InsetsState(), new InsetsSourceControl[] { + new InsetsSourceControl(ITYPE_IME, mock(SurfaceControl.class), new Point(0, 0)) + }); + + verify(mT).hide(any()); + + mPerDisplay.mImeShowing = true; + mPerDisplay.insetsControlChanged(new InsetsState(), new InsetsSourceControl[] { + new InsetsSourceControl(ITYPE_IME, mock(SurfaceControl.class), new Point(0, 0)) + }); + + verify(mT).show(any()); + } +} diff --git a/services/core/java/com/android/server/wm/WindowManagerService.java b/services/core/java/com/android/server/wm/WindowManagerService.java index a680213c52a7f..ecf6aaee746d8 100644 --- a/services/core/java/com/android/server/wm/WindowManagerService.java +++ b/services/core/java/com/android/server/wm/WindowManagerService.java @@ -6112,7 +6112,7 @@ public class WindowManagerService extends IWindowManager.Stub } if (inputMethodControlTarget != null) { pw.print(" inputMethodControlTarget in display# "); pw.print(displayId); - pw.print(' '); pw.println(inputMethodControlTarget.getWindow()); + pw.print(' '); pw.println(inputMethodControlTarget); } }); pw.print(" mInTouchMode="); pw.println(mInTouchMode);