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
Merged-In: Ia536cdf76805caa56ca1b6eaf2b3db83b6ecd94e
Change-Id: Ia536cdf76805caa56ca1b6eaf2b3db83b6ecd94e
(cherry picked from commit f8175d1a5b276386037006a77d1df132774e9ed8)

2 files changed, 109 insertions, 8 deletions

diff --git a/core/java/android/view/InsetsController.java b/core/java/android/view/InsetsController.java
index c383bc7a4d70..e9b7695e7b71 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(mLastDispatchedState, 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 801cd4ddb94e..af02b7bdbd90 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();