From b6478edb0945ef676fc1183f17a2d0e3c758deb7 Mon Sep 17 00:00:00 2001 From: Tiger Huang Date: Mon, 27 Jul 2020 23:46:30 +0800 Subject: [PATCH] Dispatch insets to client if mState is changed Previous logic in onStateChanged notifies insetsChanged based on the change of mLastDispatchedState, which can make us dispatch redundant insets changes to the app. In this CL, we only notifies insetsChanged if mState is really changed in onStateChanged -- we use the final mState (after updateState and applyLocalVisibilityOverride) to compare with the one before changing. Fix: 161924448 Test: atest InsetsControllerTest WindowInsetsControllerTests Test: Swipe up to home while IME open and see if there is any jank Change-Id: Ia536cdf76805caa56ca1b6eaf2b3db83b6ecd94e --- core/java/android/view/InsetsController.java | 16 +-- .../android/view/InsetsControllerTest.java | 101 +++++++++++++++++- 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/core/java/android/view/InsetsController.java b/core/java/android/view/InsetsController.java index c383bc7a4d700..7f45c044408a1 100644 --- a/core/java/android/view/InsetsController.java +++ b/core/java/android/view/InsetsController.java @@ -618,16 +618,20 @@ public class InsetsController implements WindowInsetsController, InsetsAnimation return false; } if (DEBUG) Log.d(TAG, "onStateChanged: " + state); - updateState(state); - - boolean localStateChanged = !mState.equals(mLastDispatchedState, - true /* excludingCaptionInsets */, true /* excludeInvisibleIme */); mLastDispatchedState.set(state, true /* copySources */); + final InsetsState lastState = new InsetsState(mState, true /* copySources */); + updateState(state); applyLocalVisibilityOverride(); - if (localStateChanged) { - if (DEBUG) Log.d(TAG, "onStateChanged, notifyInsetsChanged, send state to WM: " + mState); + + if (!mState.equals(lastState, true /* excludingCaptionInsets */, + true /* excludeInvisibleIme */)) { + if (DEBUG) Log.d(TAG, "onStateChanged, notifyInsetsChanged"); mHost.notifyInsetsChanged(); + } + if (!mState.equals(state, true /* excludingCaptionInsets */, + true /* excludeInvisibleIme */)) { + if (DEBUG) Log.d(TAG, "onStateChanged, send state to WM: " + mState); updateRequestedState(); } return true; diff --git a/core/tests/coretests/src/android/view/InsetsControllerTest.java b/core/tests/coretests/src/android/view/InsetsControllerTest.java index 801cd4ddb94e9..af02b7bdbd90e 100644 --- a/core/tests/coretests/src/android/view/InsetsControllerTest.java +++ b/core/tests/coretests/src/android/view/InsetsControllerTest.java @@ -27,6 +27,7 @@ import static android.view.InsetsState.ITYPE_NAVIGATION_BAR; import static android.view.InsetsState.ITYPE_STATUS_BAR; import static android.view.ViewRootImpl.NEW_INSETS_MODE_FULL; import static android.view.WindowInsets.Type.ime; +import static android.view.WindowInsets.Type.navigationBars; import static android.view.WindowInsets.Type.statusBars; import static android.view.WindowManager.LayoutParams.SOFT_INPUT_ADJUST_RESIZE; @@ -40,8 +41,11 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.notNull; +import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import android.content.Context; @@ -124,7 +128,7 @@ public class InsetsControllerTest { } mTestClock = new OffsettableClock(); mTestHandler = new TestHandler(null, mTestClock); - mTestHost = new TestHost(mViewRoot); + mTestHost = spy(new TestHost(mViewRoot)); mController = new InsetsController(mTestHost, (controller, type) -> { if (type == ITYPE_IME) { return new InsetsSourceConsumer(type, controller.getState(), @@ -745,6 +749,99 @@ public class InsetsControllerTest { }); } + @Test + public void testInsetsChangedCount_controlSystemBars() { + InstrumentationRegistry.getInstrumentation().runOnMainSync(() -> { + prepareControls(); + + // Hiding visible system bars should only causes insets change once for each bar. + clearInvocations(mTestHost); + mController.hide(statusBars() | navigationBars()); + verify(mTestHost, times(2)).notifyInsetsChanged(); + + // Sending the same insets state should not cause insets change. + // This simulates the callback from server after hiding system bars. + clearInvocations(mTestHost); + mController.onStateChanged(mController.getState()); + verify(mTestHost, never()).notifyInsetsChanged(); + + // Showing invisible system bars should only causes insets change once for each bar. + clearInvocations(mTestHost); + mController.show(statusBars() | navigationBars()); + verify(mTestHost, times(2)).notifyInsetsChanged(); + + // Sending the same insets state should not cause insets change. + // This simulates the callback from server after showing system bars. + clearInvocations(mTestHost); + mController.onStateChanged(mController.getState()); + verify(mTestHost, never()).notifyInsetsChanged(); + }); + } + + @Test + public void testInsetsChangedCount_controlIme() { + InstrumentationRegistry.getInstrumentation().runOnMainSync(() -> { + prepareControls(); + + // Showing invisible ime should only causes insets change once. + clearInvocations(mTestHost); + mController.show(ime(), true /* fromIme */); + verify(mTestHost, times(1)).notifyInsetsChanged(); + + // Sending the same insets state should not cause insets change. + // This simulates the callback from server after showing ime. + clearInvocations(mTestHost); + mController.onStateChanged(mController.getState()); + verify(mTestHost, never()).notifyInsetsChanged(); + + // Hiding visible ime should only causes insets change once. + clearInvocations(mTestHost); + mController.hide(ime()); + verify(mTestHost, times(1)).notifyInsetsChanged(); + + // Sending the same insets state should not cause insets change. + // This simulates the callback from server after hiding ime. + clearInvocations(mTestHost); + mController.onStateChanged(mController.getState()); + verify(mTestHost, never()).notifyInsetsChanged(); + }); + } + + @Test + public void testInsetsChangedCount_onStateChanged() { + InstrumentationRegistry.getInstrumentation().runOnMainSync(() -> { + final InsetsState localState = mController.getState(); + + // Changing status bar frame should cause notifyInsetsChanged. + clearInvocations(mTestHost); + InsetsState newState = new InsetsState(localState, true /* copySources */); + newState.getSource(ITYPE_STATUS_BAR).getFrame().bottom++; + mController.onStateChanged(newState); + verify(mTestHost, times(1)).notifyInsetsChanged(); + + // Changing status bar visibility should cause notifyInsetsChanged. + clearInvocations(mTestHost); + newState = new InsetsState(localState, true /* copySources */); + newState.getSource(ITYPE_STATUS_BAR).setVisible(false); + mController.onStateChanged(newState); + verify(mTestHost, times(1)).notifyInsetsChanged(); + + // Changing invisible IME frame should not cause notifyInsetsChanged. + clearInvocations(mTestHost); + newState = new InsetsState(localState, true /* copySources */); + newState.getSource(ITYPE_IME).getFrame().top--; + mController.onStateChanged(newState); + verify(mTestHost, never()).notifyInsetsChanged(); + + // Changing IME visibility should cause notifyInsetsChanged. + clearInvocations(mTestHost); + newState = new InsetsState(localState, true /* copySources */); + newState.getSource(ITYPE_IME).setVisible(true); + mController.onStateChanged(newState); + verify(mTestHost, times(1)).notifyInsetsChanged(); + }); + } + private void waitUntilNextFrame() throws Exception { final CountDownLatch latch = new CountDownLatch(1); Choreographer.getMainThreadInstance().postCallback(Choreographer.CALLBACK_COMMIT, @@ -777,7 +874,7 @@ public class InsetsControllerTest { return controls; } - private static class TestHost extends ViewRootInsetsControllerHost { + public static class TestHost extends ViewRootInsetsControllerHost { private InsetsState mModifiedState = new InsetsState();