From 49af78d4b93e4a01f13f2e9d1dc4ce12098148f0 Mon Sep 17 00:00:00 2001 From: Bryce Lee Date: Mon, 23 Aug 2021 17:46:44 -0700 Subject: [PATCH] Request new communal surface on layout changes. This changelist causes a request to be made on every layout change to the communal SurfaceView. This fixes the issues where the SurfaceView is not resized properly. This change also ensures that subsequent identical show requests are ignored. Bug: 197574964 Test: CommunalSurfaceViewControllerTest#testLayoutChange Change-Id: I480b6ab5b9728f173cf4fd3e6042aa70ba60db89 --- .../communal/service/CommunalSourceImpl.java | 51 ++++++++++++++++--- .../CommunalSurfaceViewController.java | 27 +++++++--- .../CommunalSurfaceViewControllerTest.java | 45 +++++++++++++--- 3 files changed, 102 insertions(+), 21 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/communal/service/CommunalSourceImpl.java b/packages/SystemUI/src/com/android/systemui/communal/service/CommunalSourceImpl.java index d8bf2dc73c3dd..b8070abd90b83 100644 --- a/packages/SystemUI/src/com/android/systemui/communal/service/CommunalSourceImpl.java +++ b/packages/SystemUI/src/com/android/systemui/communal/service/CommunalSourceImpl.java @@ -38,6 +38,7 @@ import com.google.common.util.concurrent.ListenableFuture; import java.lang.ref.WeakReference; import java.util.ArrayList; +import java.util.Objects; import java.util.concurrent.Executor; import javax.inject.Inject; @@ -78,6 +79,45 @@ public class CommunalSourceImpl implements CommunalSource { } } + static class Request { + private final int mWidth; + private final int mHeight; + private final int mDisplayId; + private final IBinder mHostToken; + + Request(int width, int height, int displayId, IBinder hostToken) { + mWidth = width; + mHeight = height; + mDisplayId = displayId; + mHostToken = hostToken; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof Request)) return false; + Request request = (Request) o; + return mWidth == request.mWidth && mHeight == request.mHeight + && mDisplayId == request.mDisplayId && Objects.equals(mHostToken, + request.mHostToken); + } + + @Override + public int hashCode() { + return Objects.hash(mWidth, mHeight, mDisplayId, mHostToken); + } + + @Override + public String toString() { + return "Request{" + + "mWidth=" + mWidth + + ", mHeight=" + mHeight + + ", mDisplayId=" + mDisplayId + + ", mHostToken=" + mHostToken + + '}'; + } + } + // mConnected is initialized to true as it is presumed instances are constructed with valid // proxies. The source can never be reconnected once the proxy has died. Once this value // becomes false, the source will always report disconnected to registering callbacks. @@ -148,18 +188,15 @@ public class CommunalSourceImpl implements CommunalSource { * Called internally to request a new {@link android.view.SurfaceControlViewHost.SurfacePackage} * for showing communal content. * - * @param hostToken The HostToken necessary to generate a {@link SurfaceControlViewHost}. - * @param displayId The id of the display the surface will be shown on. - * @param width The width of the surface. - * @param height The height of the surface. + * @param request A request with the parameters for the new communal surface. * @return A future that returns the resulting * {@link android.view.SurfaceControlViewHost.SurfacePackage}. */ protected ListenableFuture requestCommunalSurface( - IBinder hostToken, int displayId, int width, int height) { + Request request) { return CallbackToFutureAdapter.getFuture(completer -> { - mSourceProxy.getCommunalSurface(hostToken, width, height, displayId, - new ICommunalSurfaceCallback.Stub() { + mSourceProxy.getCommunalSurface(request.mHostToken, request.mWidth, request.mHeight, + request.mDisplayId, new ICommunalSurfaceCallback.Stub() { @Override public void onSurface( SurfaceControlViewHost.SurfacePackage surfacePackage) { diff --git a/packages/SystemUI/src/com/android/systemui/communal/service/CommunalSurfaceViewController.java b/packages/SystemUI/src/com/android/systemui/communal/service/CommunalSurfaceViewController.java index d554b5e26d4f5..0de50297d04f4 100644 --- a/packages/SystemUI/src/com/android/systemui/communal/service/CommunalSurfaceViewController.java +++ b/packages/SystemUI/src/com/android/systemui/communal/service/CommunalSurfaceViewController.java @@ -37,6 +37,7 @@ import com.android.systemui.util.ViewController; import com.google.common.util.concurrent.ListenableFuture; +import java.util.Optional; import java.util.concurrent.Executor; /** @@ -64,6 +65,8 @@ public class CommunalSurfaceViewController extends ViewController { private int mCurrentState; + private Optional mLastRequest = Optional.empty(); + // The current in-flight request for a surface package. private ListenableFuture mCurrentSurfaceFuture; @@ -100,6 +103,9 @@ public class CommunalSurfaceViewController extends ViewController { mSurfaceViewTouchableRegion.set(left, top + topMargin, right, bottom - bottomMargin); updateTouchExclusion(); + + // Trigger showing (or hiding) surface based on new dimensions. + showSurface(); } }; @@ -149,7 +155,7 @@ public class CommunalSurfaceViewController extends ViewController { mCurrentState = newState; - showSurface(newState == STATE_CAN_SHOW_SURFACE); + showSurface(); updateTouchExclusion(); } @@ -167,25 +173,34 @@ public class CommunalSurfaceViewController extends ViewController { } } - private void showSurface(boolean show) { + private void showSurface() { mView.setWillNotDraw(false); - if (!show) { + if (mCurrentState != STATE_CAN_SHOW_SURFACE) { // If the surface is no longer showing, cancel any in-flight requests. if (mCurrentSurfaceFuture != null) { mCurrentSurfaceFuture.cancel(true); mCurrentSurfaceFuture = null; } + mLastRequest = Optional.empty(); mView.setWillNotDraw(true); return; } + final CommunalSourceImpl.Request request = new CommunalSourceImpl.Request( + mView.getMeasuredWidth(), mView.getMeasuredHeight(), + mView.getDisplay().getDisplayId(), mView.getHostToken()); + + if (mLastRequest.isPresent() && mLastRequest.get().equals(request)) { + return; + } + + mLastRequest = Optional.of(request); + // Since this method is only called when the state has changed, mCurrentSurfaceFuture should // be null here. - mCurrentSurfaceFuture = mSource.requestCommunalSurface(mView.getHostToken(), - mView.getDisplay().getDisplayId(), mView.getMeasuredWidth(), - mView.getMeasuredHeight()); + mCurrentSurfaceFuture = mSource.requestCommunalSurface(request); mCurrentSurfaceFuture.addListener(new Runnable() { @Override diff --git a/packages/SystemUI/tests/src/com/android/systemui/communal/service/CommunalSurfaceViewControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/communal/service/CommunalSurfaceViewControllerTest.java index c2d6b9aff96d9..cf2e029fabfcd 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/communal/service/CommunalSurfaceViewControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/communal/service/CommunalSurfaceViewControllerTest.java @@ -18,9 +18,9 @@ package com.android.systemui.communal.service; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -136,7 +136,7 @@ public class CommunalSurfaceViewControllerTest extends SysuiTestCase { mPackageFuture = SettableFuture.create(); - when(mCommunalSource.requestCommunalSurface(any(), anyInt(), anyInt(), anyInt())) + when(mCommunalSource.requestCommunalSurface(any())) .thenReturn(mPackageFuture); } @@ -144,19 +144,20 @@ public class CommunalSurfaceViewControllerTest extends SysuiTestCase { public void testSetSurfacePackage() { // There should be no requests without the proper state. verify(mCommunalSource, times(0)) - .requestCommunalSurface(any(), anyInt(), anyInt(), anyInt()); + .requestCommunalSurface(any()); // The full state must be present to make a request. mController.onViewAttached(); verify(mCommunalSource, times(0)) - .requestCommunalSurface(any(), anyInt(), anyInt(), anyInt()); + .requestCommunalSurface(any()); clearInvocations(mSurfaceView); // Request surface view once all conditions are met. mCallback.surfaceCreated(mSurfaceHolder); - verify(mCommunalSource) - .requestCommunalSurface(mHostToken, DISPLAY_ID, MEASURED_WIDTH, MEASURED_HEIGHT); + final CommunalSourceImpl.Request expectedRequest = new CommunalSourceImpl.Request( + MEASURED_WIDTH, MEASURED_HEIGHT, DISPLAY_ID, mHostToken); + verify(mCommunalSource).requestCommunalSurface(eq(expectedRequest)); when(mSurfaceView.isAttachedToWindow()).thenReturn(true); @@ -216,8 +217,9 @@ public class CommunalSurfaceViewControllerTest extends SysuiTestCase { mFakeExecutor.runAllReady(); clearInvocations(mSurfaceView); - verify(mCommunalSource, times(1)) - .requestCommunalSurface(mHostToken, DISPLAY_ID, MEASURED_WIDTH, MEASURED_HEIGHT); + final CommunalSourceImpl.Request expectedRequest = new CommunalSourceImpl.Request( + MEASURED_WIDTH, MEASURED_HEIGHT, DISPLAY_ID, mHostToken); + verify(mCommunalSource, times(1)).requestCommunalSurface(eq(expectedRequest)); mController.onViewDetached(); assertTrue(mPackageFuture.isCancelled()); @@ -270,4 +272,31 @@ public class CommunalSurfaceViewControllerTest extends SysuiTestCase { verify(mNotificationShadeWindowController) .setTouchExclusionRegion(eq(new Region())); } + + @Test + public void testLayoutChange() { + final int left = 0; + final int top = 0; + final int right = 200; + final int bottom = 100; + + givenSurfacePresent(); + + // Layout change should trigger a request to get new communal surface. + mLayoutChangeListener.onLayoutChange(mSurfaceView, left, top, right, bottom, 0, 0, 0, + 0); + // Note that the measured are preset and different than the layout input. + final CommunalSourceImpl.Request expectedRequest = + new CommunalSourceImpl.Request(MEASURED_WIDTH, MEASURED_HEIGHT, DISPLAY_ID, + mHostToken); + verify(mCommunalSource) + .requestCommunalSurface(eq(expectedRequest)); + + clearInvocations(mCommunalSource); + + // Subsequent matching layout change should not trigger any request. + mLayoutChangeListener.onLayoutChange(mSurfaceView, left, top, right, bottom, 0, 0, 0, + 0); + verify(mCommunalSource, never()).requestCommunalSurface(any()); + } }