From 693478bd5295e2cef3a9755af08585dc780daef2 Mon Sep 17 00:00:00 2001 From: Mark Renouf Date: Fri, 15 Jul 2022 14:46:33 +0000 Subject: [PATCH] Code cleanup of ScreenshotRequest and usages Make constructor visible for tests Make all fields final (no setters, so already effectively final) Remove mHasStatusBar and mHasNavBar (unused) Remove overload 'takeScreenshot' method without a source param Added missing @IntDef annotations to parameters Bug: 231957192 Test: atest ScreenshotHelperTest SystemActionPerformerTest Change-Id: Ief43bd0ba50dafac720ae18d00c3c99b8d31e2e1 --- .../internal/util/ScreenshotHelper.java | 198 +++++++----------- .../internal/util/ScreenshotHelperTest.java | 11 +- .../systemui/accessibility/SystemActions.java | 2 +- .../GlobalActionsDialogLite.java | 2 +- .../accessibility/SystemActionPerformer.java | 7 +- .../com/android/server/wm/DisplayPolicy.java | 2 - .../SystemActionPerformerTest.java | 7 +- 7 files changed, 95 insertions(+), 134 deletions(-) diff --git a/core/java/com/android/internal/util/ScreenshotHelper.java b/core/java/com/android/internal/util/ScreenshotHelper.java index f4f438b1f601d..8af2450f29efa 100644 --- a/core/java/com/android/internal/util/ScreenshotHelper.java +++ b/core/java/com/android/internal/util/ScreenshotHelper.java @@ -1,7 +1,6 @@ package com.android.internal.util; import static android.content.Intent.ACTION_USER_SWITCHED; -import static android.view.WindowManager.ScreenshotSource.SCREENSHOT_OTHER; import android.annotation.NonNull; import android.annotation.Nullable; @@ -29,6 +28,10 @@ import android.os.RemoteException; import android.os.UserHandle; import android.util.Log; import android.view.WindowManager; +import android.view.WindowManager.ScreenshotSource; +import android.view.WindowManager.ScreenshotType; + +import com.android.internal.annotations.VisibleForTesting; import java.util.Objects; import java.util.function.Consumer; @@ -42,24 +45,28 @@ public class ScreenshotHelper { * Describes a screenshot request (to make it easier to pass data through to the handler). */ public static class ScreenshotRequest implements Parcelable { - private int mSource; - private boolean mHasStatusBar; - private boolean mHasNavBar; - private Bundle mBitmapBundle; - private Rect mBoundsInScreen; - private Insets mInsets; - private int mTaskId; - private int mUserId; - private ComponentName mTopComponent; + private final int mSource; + private final Bundle mBitmapBundle; + private final Rect mBoundsInScreen; + private final Insets mInsets; + private final int mTaskId; + private final int mUserId; + private final ComponentName mTopComponent; - ScreenshotRequest(int source, boolean hasStatus, boolean hasNav) { + @VisibleForTesting + public ScreenshotRequest(int source) { mSource = source; - mHasStatusBar = hasStatus; - mHasNavBar = hasNav; + mBitmapBundle = null; + mBoundsInScreen = null; + mInsets = null; + mTaskId = -1; + mUserId = -1; + mTopComponent = null; } - ScreenshotRequest(int source, Bundle bitmapBundle, Rect boundsInScreen, Insets insets, - int taskId, int userId, ComponentName topComponent) { + @VisibleForTesting + public ScreenshotRequest(int source, Bundle bitmapBundle, Rect boundsInScreen, + Insets insets, int taskId, int userId, ComponentName topComponent) { mSource = source; mBitmapBundle = bitmapBundle; mBoundsInScreen = boundsInScreen; @@ -71,16 +78,21 @@ public class ScreenshotHelper { ScreenshotRequest(Parcel in) { mSource = in.readInt(); - mHasStatusBar = in.readBoolean(); - mHasNavBar = in.readBoolean(); - if (in.readInt() == 1) { mBitmapBundle = in.readBundle(getClass().getClassLoader()); - mBoundsInScreen = in.readParcelable(Rect.class.getClassLoader(), android.graphics.Rect.class); - mInsets = in.readParcelable(Insets.class.getClassLoader(), android.graphics.Insets.class); + mBoundsInScreen = in.readParcelable(Rect.class.getClassLoader(), Rect.class); + mInsets = in.readParcelable(Insets.class.getClassLoader(), Insets.class); mTaskId = in.readInt(); mUserId = in.readInt(); - mTopComponent = in.readParcelable(ComponentName.class.getClassLoader(), android.content.ComponentName.class); + mTopComponent = in.readParcelable(ComponentName.class.getClassLoader(), + ComponentName.class); + } else { + mBitmapBundle = null; + mBoundsInScreen = null; + mInsets = null; + mTaskId = -1; + mUserId = -1; + mTopComponent = null; } } @@ -88,14 +100,6 @@ public class ScreenshotHelper { return mSource; } - public boolean getHasStatusBar() { - return mHasStatusBar; - } - - public boolean getHasNavBar() { - return mHasNavBar; - } - public Bundle getBitmapBundle() { return mBitmapBundle; } @@ -112,7 +116,6 @@ public class ScreenshotHelper { return mTaskId; } - public int getUserId() { return mUserId; } @@ -129,8 +132,6 @@ public class ScreenshotHelper { @Override public void writeToParcel(Parcel dest, int flags) { dest.writeInt(mSource); - dest.writeBoolean(mHasStatusBar); - dest.writeBoolean(mHasNavBar); if (mBitmapBundle == null) { dest.writeInt(0); } else { @@ -144,7 +145,8 @@ public class ScreenshotHelper { } } - public static final @NonNull Parcelable.Creator CREATOR = + @NonNull + public static final Parcelable.Creator CREATOR = new Parcelable.Creator() { @Override @@ -254,113 +256,71 @@ public class ScreenshotHelper { /** * Request a screenshot be taken. - * + *

* Added to support reducing unit test duration; the method variant without a timeout argument * is recommended for general use. * - * @param screenshotType The type of screenshot, for example either - * {@link android.view.WindowManager#TAKE_SCREENSHOT_FULLSCREEN} - * or - * {@link android.view.WindowManager#TAKE_SCREENSHOT_SELECTED_REGION} - * @param hasStatus {@code true} if the status bar is currently showing. {@code false} - * if not. - * @param hasNav {@code true} if the navigation bar is currently showing. {@code - * false} if not. - * @param source The source of the screenshot request. One of - * {SCREENSHOT_GLOBAL_ACTIONS, SCREENSHOT_KEY_CHORD, - * SCREENSHOT_OVERVIEW, SCREENSHOT_OTHER} - * @param handler A handler used in case the screenshot times out - * @param completionConsumer Consumes `false` if a screenshot was not taken, and `true` if the - * screenshot was taken. + * @param screenshotType The type of screenshot, defined by {@link ScreenshotType} + * @param source The source of the screenshot request, defined by {@link ScreenshotSource} + * @param handler used to process messages received from the screenshot service + * @param completionConsumer receives the URI of the captured screenshot, once saved or + * null if no screenshot was saved */ - public void takeScreenshot(final int screenshotType, final boolean hasStatus, - final boolean hasNav, int source, @NonNull Handler handler, - @Nullable Consumer completionConsumer) { - ScreenshotRequest screenshotRequest = new ScreenshotRequest(source, hasStatus, hasNav); - takeScreenshot(screenshotType, SCREENSHOT_TIMEOUT_MS, handler, screenshotRequest, + public void takeScreenshot(@ScreenshotType int screenshotType, @ScreenshotSource int source, + @NonNull Handler handler, @Nullable Consumer completionConsumer) { + ScreenshotRequest screenshotRequest = new ScreenshotRequest(source); + takeScreenshot(screenshotType, handler, screenshotRequest, SCREENSHOT_TIMEOUT_MS, completionConsumer); } /** - * Request a screenshot be taken, with provided reason. - * - * @param screenshotType The type of screenshot, for example either - * {@link android.view.WindowManager#TAKE_SCREENSHOT_FULLSCREEN} - * or - * {@link android.view.WindowManager#TAKE_SCREENSHOT_SELECTED_REGION} - * @param hasStatus {@code true} if the status bar is currently showing. {@code false} - * if - * not. - * @param hasNav {@code true} if the navigation bar is currently showing. {@code - * false} - * if not. - * @param handler A handler used in case the screenshot times out - * @param completionConsumer Consumes `false` if a screenshot was not taken, and `true` if the - * screenshot was taken. - */ - public void takeScreenshot(final int screenshotType, final boolean hasStatus, - final boolean hasNav, @NonNull Handler handler, - @Nullable Consumer completionConsumer) { - takeScreenshot(screenshotType, hasStatus, hasNav, SCREENSHOT_TIMEOUT_MS, handler, - completionConsumer); - } - - /** - * Request a screenshot be taken with a specific timeout. - * + * Request a screenshot be taken. + *

* Added to support reducing unit test duration; the method variant without a timeout argument * is recommended for general use. * - * @param screenshotType The type of screenshot, for example either - * {@link android.view.WindowManager#TAKE_SCREENSHOT_FULLSCREEN} - * or - * {@link android.view.WindowManager#TAKE_SCREENSHOT_SELECTED_REGION} - * @param hasStatus {@code true} if the status bar is currently showing. {@code false} - * if - * not. - * @param hasNav {@code true} if the navigation bar is currently showing. {@code - * false} - * if not. - * @param timeoutMs If the screenshot hasn't been completed within this time period, - * the screenshot attempt will be cancelled and `completionConsumer` - * will be run. - * @param handler A handler used in case the screenshot times out - * @param completionConsumer Consumes `false` if a screenshot was not taken, and `true` if the - * screenshot was taken. + * @param screenshotType The type of screenshot, defined by {@link ScreenshotType} + * @param source The source of the screenshot request, defined by {@link ScreenshotSource} + * @param handler used to process messages received from the screenshot service + * @param timeoutMs time limit for processing, intended only for testing + * @param completionConsumer receives the URI of the captured screenshot, once saved or + * null if no screenshot was saved */ - public void takeScreenshot(final int screenshotType, final boolean hasStatus, - final boolean hasNav, long timeoutMs, @NonNull Handler handler, - @Nullable Consumer completionConsumer) { - ScreenshotRequest screenshotRequest = new ScreenshotRequest(SCREENSHOT_OTHER, hasStatus, - hasNav); - takeScreenshot(screenshotType, timeoutMs, handler, screenshotRequest, completionConsumer); + @VisibleForTesting + public void takeScreenshot(@ScreenshotType int screenshotType, @ScreenshotSource int source, + @NonNull Handler handler, long timeoutMs, @Nullable Consumer completionConsumer) { + ScreenshotRequest screenshotRequest = new ScreenshotRequest(source); + takeScreenshot(screenshotType, handler, screenshotRequest, timeoutMs, completionConsumer); } /** * Request that provided image be handled as if it was a screenshot. * - * @param screenshotBundle Bundle containing the buffer and color space of the screenshot. - * @param boundsInScreen The bounds in screen coordinates that the bitmap orginated from. - * @param insets The insets that the image was shown with, inside the screenbounds. - * @param taskId The taskId of the task that the screen shot was taken of. - * @param userId The userId of user running the task provided in taskId. - * @param topComponent The component name of the top component running in the task. - * @param handler A handler used in case the screenshot times out - * @param completionConsumer Consumes `false` if a screenshot was not taken, and `true` if the - * screenshot was taken. + * @param screenshotBundle Bundle containing the buffer and color space of the screenshot. + * @param boundsInScreen The bounds in screen coordinates that the bitmap originated from. + * @param insets The insets that the image was shown with, inside the screen bounds. + * @param taskId The taskId of the task that the screen shot was taken of. + * @param userId The userId of user running the task provided in taskId. + * @param topComponent The component name of the top component running in the task. + * @param source The source of the screenshot request, defined by {@link ScreenshotSource} + * @param handler A handler used in case the screenshot times out + * @param completionConsumer receives the URI of the captured screenshot, once saved or + * null if no screenshot was saved */ public void provideScreenshot(@NonNull Bundle screenshotBundle, @NonNull Rect boundsInScreen, - @NonNull Insets insets, int taskId, int userId, ComponentName topComponent, int source, - @NonNull Handler handler, @Nullable Consumer completionConsumer) { - ScreenshotRequest screenshotRequest = - new ScreenshotRequest(source, screenshotBundle, boundsInScreen, insets, taskId, - userId, topComponent); - takeScreenshot(WindowManager.TAKE_SCREENSHOT_PROVIDED_IMAGE, SCREENSHOT_TIMEOUT_MS, - handler, screenshotRequest, completionConsumer); + @NonNull Insets insets, int taskId, int userId, ComponentName topComponent, + @ScreenshotSource int source, @NonNull Handler handler, + @Nullable Consumer completionConsumer) { + ScreenshotRequest screenshotRequest = new ScreenshotRequest(source, screenshotBundle, + boundsInScreen, insets, taskId, userId, topComponent); + takeScreenshot(WindowManager.TAKE_SCREENSHOT_PROVIDED_IMAGE, handler, screenshotRequest, + SCREENSHOT_TIMEOUT_MS, + completionConsumer); } - private void takeScreenshot(final int screenshotType, long timeoutMs, @NonNull Handler handler, - ScreenshotRequest screenshotRequest, @Nullable Consumer completionConsumer) { + private void takeScreenshot(@ScreenshotType int screenshotType, @NonNull Handler handler, + ScreenshotRequest screenshotRequest, long timeoutMs, + @Nullable Consumer completionConsumer) { synchronized (mScreenshotLock) { final Runnable mScreenshotTimeout = () -> { diff --git a/core/tests/screenshothelpertests/src/com/android/internal/util/ScreenshotHelperTest.java b/core/tests/screenshothelpertests/src/com/android/internal/util/ScreenshotHelperTest.java index 4b8173732b4dd..fd4fb133ef125 100644 --- a/core/tests/screenshothelpertests/src/com/android/internal/util/ScreenshotHelperTest.java +++ b/core/tests/screenshothelpertests/src/com/android/internal/util/ScreenshotHelperTest.java @@ -80,13 +80,14 @@ public final class ScreenshotHelperTest { @Test public void testFullscreenScreenshot() { - mScreenshotHelper.takeScreenshot(TAKE_SCREENSHOT_FULLSCREEN, false, false, mHandler, null); + mScreenshotHelper.takeScreenshot(TAKE_SCREENSHOT_FULLSCREEN, + WindowManager.ScreenshotSource.SCREENSHOT_OTHER, mHandler, null); } @Test public void testSelectedRegionScreenshot() { - mScreenshotHelper.takeScreenshot(TAKE_SCREENSHOT_SELECTED_REGION, false, false, mHandler, - null); + mScreenshotHelper.takeScreenshot(TAKE_SCREENSHOT_SELECTED_REGION, + WindowManager.ScreenshotSource.SCREENSHOT_OTHER, mHandler, null); } @Test @@ -101,8 +102,10 @@ public final class ScreenshotHelperTest { long timeoutMs = 10; CountDownLatch lock = new CountDownLatch(1); - mScreenshotHelper.takeScreenshot(TAKE_SCREENSHOT_FULLSCREEN, false, false, timeoutMs, + mScreenshotHelper.takeScreenshot(TAKE_SCREENSHOT_FULLSCREEN, + WindowManager.ScreenshotSource.SCREENSHOT_OTHER, mHandler, + timeoutMs, uri -> { assertNull(uri); lock.countDown(); diff --git a/packages/SystemUI/src/com/android/systemui/accessibility/SystemActions.java b/packages/SystemUI/src/com/android/systemui/accessibility/SystemActions.java index 448b99b6e5d06..a1288b5319554 100644 --- a/packages/SystemUI/src/com/android/systemui/accessibility/SystemActions.java +++ b/packages/SystemUI/src/com/android/systemui/accessibility/SystemActions.java @@ -500,7 +500,7 @@ public class SystemActions extends CoreStartable { private void handleTakeScreenshot() { ScreenshotHelper screenshotHelper = new ScreenshotHelper(mContext); - screenshotHelper.takeScreenshot(WindowManager.TAKE_SCREENSHOT_FULLSCREEN, true, true, + screenshotHelper.takeScreenshot(WindowManager.TAKE_SCREENSHOT_FULLSCREEN, SCREENSHOT_ACCESSIBILITY_ACTIONS, new Handler(Looper.getMainLooper()), null); } diff --git a/packages/SystemUI/src/com/android/systemui/globalactions/GlobalActionsDialogLite.java b/packages/SystemUI/src/com/android/systemui/globalactions/GlobalActionsDialogLite.java index ab30db297fce9..ca65d12e87e6e 100644 --- a/packages/SystemUI/src/com/android/systemui/globalactions/GlobalActionsDialogLite.java +++ b/packages/SystemUI/src/com/android/systemui/globalactions/GlobalActionsDialogLite.java @@ -947,7 +947,7 @@ public class GlobalActionsDialogLite implements DialogInterface.OnDismissListene mHandler.postDelayed(new Runnable() { @Override public void run() { - mScreenshotHelper.takeScreenshot(TAKE_SCREENSHOT_FULLSCREEN, true, true, + mScreenshotHelper.takeScreenshot(TAKE_SCREENSHOT_FULLSCREEN, SCREENSHOT_GLOBAL_ACTIONS, mHandler, null); mMetricsLogger.action(MetricsEvent.ACTION_SCREENSHOT_POWER_MENU); mUiEventLogger.log(GlobalActionsEvent.GA_SCREENSHOT_PRESS); diff --git a/services/accessibility/java/com/android/server/accessibility/SystemActionPerformer.java b/services/accessibility/java/com/android/server/accessibility/SystemActionPerformer.java index 803177b6c0101..3fa0ab69d67c7 100644 --- a/services/accessibility/java/com/android/server/accessibility/SystemActionPerformer.java +++ b/services/accessibility/java/com/android/server/accessibility/SystemActionPerformer.java @@ -16,8 +16,6 @@ package com.android.server.accessibility; -import static android.view.WindowManager.ScreenshotSource.SCREENSHOT_ACCESSIBILITY_ACTIONS; - import android.accessibilityservice.AccessibilityService; import android.app.PendingIntent; import android.app.RemoteAction; @@ -34,6 +32,7 @@ import android.util.Slog; import android.view.InputDevice; import android.view.KeyCharacterMap; import android.view.KeyEvent; +import android.view.WindowManager; import android.view.accessibility.AccessibilityNodeInfo.AccessibilityAction; import com.android.internal.R; @@ -392,8 +391,8 @@ public class SystemActionPerformer { private boolean takeScreenshot() { ScreenshotHelper screenshotHelper = (mScreenshotHelperSupplier != null) ? mScreenshotHelperSupplier.get() : new ScreenshotHelper(mContext); - screenshotHelper.takeScreenshot(android.view.WindowManager.TAKE_SCREENSHOT_FULLSCREEN, - true, true, SCREENSHOT_ACCESSIBILITY_ACTIONS, + screenshotHelper.takeScreenshot(WindowManager.TAKE_SCREENSHOT_FULLSCREEN, + WindowManager.ScreenshotSource.SCREENSHOT_ACCESSIBILITY_ACTIONS, new Handler(Looper.getMainLooper()), null); return true; } diff --git a/services/core/java/com/android/server/wm/DisplayPolicy.java b/services/core/java/com/android/server/wm/DisplayPolicy.java index 14e2241112861..693f9c460ee63 100644 --- a/services/core/java/com/android/server/wm/DisplayPolicy.java +++ b/services/core/java/com/android/server/wm/DisplayPolicy.java @@ -2748,8 +2748,6 @@ public class DisplayPolicy { public void takeScreenshot(int screenshotType, int source) { if (mScreenshotHelper != null) { mScreenshotHelper.takeScreenshot(screenshotType, - getStatusBar() != null && getStatusBar().isVisible(), - getNavigationBar() != null && getNavigationBar().isVisible(), source, mHandler, null /* completionConsumer */); } } diff --git a/services/tests/servicestests/src/com/android/server/accessibility/SystemActionPerformerTest.java b/services/tests/servicestests/src/com/android/server/accessibility/SystemActionPerformerTest.java index 1d6ed038b86df..c15f6a9c3d66e 100644 --- a/services/tests/servicestests/src/com/android/server/accessibility/SystemActionPerformerTest.java +++ b/services/tests/servicestests/src/com/android/server/accessibility/SystemActionPerformerTest.java @@ -17,6 +17,7 @@ package com.android.server.accessibility; import static android.view.WindowManager.ScreenshotSource.SCREENSHOT_ACCESSIBILITY_ACTIONS; +import static android.view.WindowManager.TAKE_SCREENSHOT_FULLSCREEN; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.is; @@ -27,7 +28,6 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.never; @@ -301,8 +301,9 @@ public class SystemActionPerformerTest { mSystemActionPerformer.performSystemAction( AccessibilityService.GLOBAL_ACTION_TAKE_SCREENSHOT); verify(mMockScreenshotHelper).takeScreenshot( - eq(android.view.WindowManager.TAKE_SCREENSHOT_FULLSCREEN), anyBoolean(), - anyBoolean(), eq(SCREENSHOT_ACCESSIBILITY_ACTIONS), any(Handler.class), any()); + eq(TAKE_SCREENSHOT_FULLSCREEN), + eq(SCREENSHOT_ACCESSIBILITY_ACTIONS), + any(Handler.class), any()); } // PendingIntent is a final class and cannot be mocked. So we are using this