Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2d511d7f authored by Matt Casey's avatar Matt Casey Committed by Android (Google) Code Review
Browse files

Merge "Fix Race in ScreenshotController" into 24D1-dev

parents 12c947f9 0322e156

packages/SystemUI/src/com/android/systemui/screenshot/ScreenshotController.java

+22 −29
Original line number Diff line number Diff line
@@ -47,7 +47,6 @@ import android.content.res.Configuration; 
import android.graphics.Bitmap;

import android.graphics.Insets;

import android.graphics.Rect;

import android.hardware.display.DisplayManager;

import android.net.Uri;

import android.os.Process;

import android.os.UserHandle;
@@ -209,8 +208,7 @@ public class ScreenshotController { 
    @Nullable

    private final ScreenshotSoundController mScreenshotSoundController;

    private final PhoneWindow mWindow;

    private final DisplayManager mDisplayManager;

    private final int mDisplayId;

    private final Display mDisplay;

    private final ScrollCaptureExecutor mScrollCaptureExecutor;

    private final ScreenshotNotificationSmartActionsProvider

            mScreenshotNotificationSmartActionsProvider;
@@ -250,7 +248,6 @@ public class ScreenshotController { 
    @AssistedInject

    ScreenshotController(

            Context context,

            DisplayManager displayManager,

            WindowManager windowManager,

            FeatureFlags flags,

            ScreenshotViewProxy.Factory viewProxyFactory,
@@ -272,12 +269,14 @@ public class ScreenshotController { 
            AssistContentRequester assistContentRequester,

            MessageContainerController messageContainerController,

            Provider<ScreenshotSoundController> screenshotSoundController,

            @Assisted int displayId,

            @Assisted Display display,

            @Assisted boolean showUIOnExternalDisplay

    ) {

        mScreenshotSmartActions = screenshotSmartActions;

        mWindowManager = windowManager;

        mActionsProviderFactory = actionsProviderFactory;

        mNotificationsController = screenshotNotificationsControllerFactory.create(displayId);

        mNotificationsController = screenshotNotificationsControllerFactory.create(

                display.getDisplayId());

        mUiEventLogger = uiEventLogger;

        mImageExporter = imageExporter;

        mImageCapture = imageCapture;
@@ -290,12 +289,8 @@ public class ScreenshotController { 


        mScreenshotHandler = timeoutHandler;

        mScreenshotHandler.setDefaultTimeoutMillis(SCREENSHOT_CORNER_DEFAULT_TIMEOUT_MILLIS);





        mDisplayId = displayId;

        mDisplayManager = displayManager;

        mWindowManager = windowManager;

        final Context displayContext = context.createDisplayContext(getDisplay());

        mDisplay = display;

        final Context displayContext = context.createDisplayContext(display);

        mContext = (WindowContext) displayContext.createWindowContext(TYPE_SCREENSHOT, null);

        mFlags = flags;

        mActionIntentExecutor = actionIntentExecutor;
@@ -303,7 +298,7 @@ public class ScreenshotController { 
        mMessageContainerController = messageContainerController;

        mAssistContentRequester = assistContentRequester;



        mViewProxy = viewProxyFactory.getProxy(mContext, mDisplayId);

        mViewProxy = viewProxyFactory.getProxy(mContext, mDisplay.getDisplayId());



        mScreenshotHandler.setOnTimeoutRunnable(() -> {

            if (DEBUG_UI) {
@@ -329,7 +324,7 @@ public class ScreenshotController { 
                });



        // Sound is only reproduced from the controller of the default display.

        if (displayId == Display.DEFAULT_DISPLAY) {

        if (display.getDisplayId() == Display.DEFAULT_DISPLAY) {

            mScreenshotSoundController = screenshotSoundController.get();

        } else {

            mScreenshotSoundController = null;
@@ -357,7 +352,7 @@ public class ScreenshotController { 
        if (screenshot.getType() == WindowManager.TAKE_SCREENSHOT_FULLSCREEN) {

            Rect bounds = getFullScreenRect();

            screenshot.setBitmap(

                    mImageCapture.captureDisplay(mDisplayId, bounds));

                    mImageCapture.captureDisplay(mDisplay.getDisplayId(), bounds));

            screenshot.setScreenBounds(bounds);

        }
@@ -460,7 +455,7 @@ public class ScreenshotController { 
    }



    private boolean shouldShowUi() {

        return mDisplayId == Display.DEFAULT_DISPLAY || mShowUIOnExternalDisplay;

        return mDisplay.getDisplayId() == Display.DEFAULT_DISPLAY || mShowUIOnExternalDisplay;

    }



    void prepareViewForNewScreenshot(@NonNull ScreenshotData screenshot, String oldPackageName) {
@@ -623,7 +618,7 @@ public class ScreenshotController { 


    private void requestScrollCapture(UserHandle owner) {

        mScrollCaptureExecutor.requestScrollCapture(

                mDisplayId,

                mDisplay.getDisplayId(),

                mWindow.getDecorView().getWindowToken(),

                (response) -> {

                    mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_LONG_SCREENSHOT_IMPRESSION,
@@ -646,7 +641,8 @@ public class ScreenshotController { 
        }

        mUiEventLogger.log(ScreenshotEvent.SCREENSHOT_LONG_SCREENSHOT_REQUESTED, 0,

                response.getPackageName());

        Bitmap newScreenshot = mImageCapture.captureDisplay(mDisplayId, getFullScreenRect());

        Bitmap newScreenshot = mImageCapture.captureDisplay(mDisplay.getDisplayId(),

                getFullScreenRect());

        if (newScreenshot == null) {

            Log.e(TAG, "Failed to capture current screenshot for scroll transition!");

            return;
@@ -824,7 +820,8 @@ public class ScreenshotController { 
    private void saveScreenshotInBackground(

            ScreenshotData screenshot, UUID requestId, Consumer<Uri> finisher) {

        ListenableFuture<ImageExporter.Result> future = mImageExporter.export(mBgExecutor,

                requestId, screenshot.getBitmap(), screenshot.getUserOrDefault(), mDisplayId);

                requestId, screenshot.getBitmap(), screenshot.getUserOrDefault(),

                mDisplay.getDisplayId());

        future.addListener(() -> {

            try {

                ImageExporter.Result result = future.get();
@@ -866,7 +863,7 @@ public class ScreenshotController { 
        data.mActionsReadyListener = actionsReadyListener;

        data.mQuickShareActionsReadyListener = quickShareActionsReadyListener;

        data.owner = owner;

        data.displayId = mDisplayId;

        data.displayId = mDisplay.getDisplayId();



        if (mSaveInBgTask != null) {

            // just log success/failure for the pre-existing screenshot
@@ -991,13 +988,9 @@ public class ScreenshotController { 
        }

    }



    private Display getDisplay() {

        return mDisplayManager.getDisplay(mDisplayId);

    }



    private Rect getFullScreenRect() {

        DisplayMetrics displayMetrics = new DisplayMetrics();

        getDisplay().getRealMetrics(displayMetrics);

        mDisplay.getRealMetrics(displayMetrics);

        return new Rect(0, 0, displayMetrics.widthPixels, displayMetrics.heightPixels);

    }
@@ -1033,10 +1026,10 @@ public class ScreenshotController { 
        /**

         * Creates an instance of the controller for that specific displayId.

         *

         * @param displayId:               display to capture

         * @param showUIOnExternalDisplay: Whether the UI should be shown if this is an external

         * @param display                 Display to capture.

         * @param showUIOnExternalDisplay Whether the UI should be shown if this is an external

         *                                display.

         */

        ScreenshotController create(int displayId, boolean showUIOnExternalDisplay);

        ScreenshotController create(Display display, boolean showUIOnExternalDisplay);

    }

}

packages/SystemUI/src/com/android/systemui/screenshot/TakeScreenshotExecutor.kt

+13 −10
Original line number Diff line number Diff line
@@ -52,11 +52,13 @@ constructor( 
        onSaved: (Uri?) -> Unit,

        requestCallback: RequestCallback

    ) {

        val displayIds = getDisplaysToScreenshot(screenshotRequest.type)

        val displays = getDisplaysToScreenshot(screenshotRequest.type)

        val resultCallbackWrapper = MultiResultCallbackWrapper(requestCallback)

        displayIds.forEach { displayId: Int ->

        displays.forEach { display: Display ->

            val displayId = display.displayId

            Log.d(TAG, "Executing screenshot for display $displayId")

            dispatchToController(

                display,

                rawScreenshotData = ScreenshotData.fromRequest(screenshotRequest, displayId),

                onSaved =

                    if (displayId == Display.DEFAULT_DISPLAY) {
@@ -69,6 +71,7 @@ constructor( 


    /** All logging should be triggered only by this method. */

    private suspend fun dispatchToController(

        display: Display,

        rawScreenshotData: ScreenshotData,

        onSaved: (Uri?) -> Unit,

        callback: RequestCallback
@@ -88,8 +91,7 @@ constructor( 
        logScreenshotRequested(screenshotData)

        Log.d(TAG, "Screenshot request: $screenshotData")

        try {

            getScreenshotController(screenshotData.displayId)

                .handleScreenshot(screenshotData, onSaved, callback)

            getScreenshotController(display).handleScreenshot(screenshotData, onSaved, callback)

        } catch (e: IllegalStateException) {

            Log.e(TAG, "Error while ScreenshotController was handling ScreenshotData!", e)

            onFailedScreenshotRequest(screenshotData, callback)
@@ -119,12 +121,13 @@ constructor( 
        callback.reportError()

    }



    private suspend fun getDisplaysToScreenshot(requestType: Int): List<Int> {

    private suspend fun getDisplaysToScreenshot(requestType: Int): List<Display> {

        val allDisplays = displays.first()

        return if (requestType == TAKE_SCREENSHOT_PROVIDED_IMAGE) {

            // If this is a provided image, let's show the UI on the default display only.

            listOf(Display.DEFAULT_DISPLAY)

            allDisplays.filter { it.displayId == Display.DEFAULT_DISPLAY }

        } else {

            displays.first().filter { it.type in ALLOWED_DISPLAY_TYPES }.map { it.displayId }

            allDisplays.filter { it.type in ALLOWED_DISPLAY_TYPES }

        }

    }
@@ -158,9 +161,9 @@ constructor( 
        screenshotControllers.clear()

    }



    private fun getScreenshotController(id: Int): ScreenshotController {

        return screenshotControllers.computeIfAbsent(id) {

            screenshotControllerFactory.create(id, /* showUIOnExternalDisplay= */ false)

    private fun getScreenshotController(display: Display): ScreenshotController {

        return screenshotControllers.computeIfAbsent(display.displayId) {

            screenshotControllerFactory.create(display, /* showUIOnExternalDisplay= */ false)

        }

    }

packages/SystemUI/src/com/android/systemui/screenshot/TakeScreenshotService.java

+5 −2
Original line number Diff line number Diff line
@@ -37,6 +37,7 @@ import android.content.ComponentName; 
import android.content.Context;

import android.content.Intent;

import android.content.IntentFilter;

import android.hardware.display.DisplayManager;

import android.net.Uri;

import android.os.Handler;

import android.os.IBinder;
@@ -116,7 +117,8 @@ public class TakeScreenshotService extends Service { 
            UiEventLogger uiEventLogger,

            ScreenshotNotificationsController.Factory notificationsControllerFactory,

            Context context, @Background Executor bgExecutor, FeatureFlags featureFlags,

            RequestProcessor processor, Provider<TakeScreenshotExecutor> takeScreenshotExecutor) {

            RequestProcessor processor, Provider<TakeScreenshotExecutor> takeScreenshotExecutor,

            DisplayManager displayManager) {

        if (DEBUG_SERVICE) {

            Log.d(TAG, "new " + this);

        }
@@ -134,7 +136,8 @@ public class TakeScreenshotService extends Service { 
            mScreenshot = null;

        } else {

            mScreenshot = screenshotControllerFactory.create(

                    Display.DEFAULT_DISPLAY, /* showUIOnExternalDisplay= */ false);

                    displayManager.getDisplay(

                            Display.DEFAULT_DISPLAY), /* showUIOnExternalDisplay= */ false);

        }

    }

packages/SystemUI/tests/src/com/android/systemui/screenshot/TakeScreenshotExecutorTest.kt

+14 −8
Original line number Diff line number Diff line
@@ -69,8 +69,9 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 


    @Before

    fun setUp() {

        whenever(controllerFactory.create(eq(0), any())).thenReturn(controller0)

        whenever(controllerFactory.create(eq(1), any())).thenReturn(controller1)

        whenever(controllerFactory.create(any(), any())).thenAnswer {

            if (it.getArgument<Display>(0).displayId == 0) controller0 else controller1

        }

        whenever(notificationControllerFactory.create(eq(0))).thenReturn(notificationsController0)

        whenever(notificationControllerFactory.create(eq(1))).thenReturn(notificationsController1)

    }
@@ -78,12 +79,14 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 
    @Test

    fun executeScreenshots_severalDisplays_callsControllerForEachOne() =

        testScope.runTest {

            setDisplays(display(TYPE_INTERNAL, id = 0), display(TYPE_EXTERNAL, id = 1))

            val internalDisplay = display(TYPE_INTERNAL, id = 0)

            val externalDisplay = display(TYPE_EXTERNAL, id = 1)

            setDisplays(internalDisplay, externalDisplay)

            val onSaved = { _: Uri? -> }

            screenshotExecutor.executeScreenshots(createScreenshotRequest(), onSaved, callback)



            verify(controllerFactory).create(eq(0), any())

            verify(controllerFactory).create(eq(1), any())

            verify(controllerFactory).create(eq(internalDisplay), any())

            verify(controllerFactory).create(eq(externalDisplay), any())



            val capturer = ArgumentCaptor<ScreenshotData>()
@@ -107,7 +110,9 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 
    @Test

    fun executeScreenshots_providedImageType_callsOnlyDefaultDisplayController() =

        testScope.runTest {

            setDisplays(display(TYPE_INTERNAL, id = 0), display(TYPE_EXTERNAL, id = 1))

            val internalDisplay = display(TYPE_INTERNAL, id = 0)

            val externalDisplay = display(TYPE_EXTERNAL, id = 1)

            setDisplays(internalDisplay, externalDisplay)

            val onSaved = { _: Uri? -> }

            screenshotExecutor.executeScreenshots(

                createScreenshotRequest(TAKE_SCREENSHOT_PROVIDED_IMAGE),
@@ -115,8 +120,8 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 
                callback

            )



            verify(controllerFactory).create(eq(0), any())

            verify(controllerFactory, never()).create(eq(1), any())

            verify(controllerFactory).create(eq(internalDisplay), any())

            verify(controllerFactory, never()).create(eq(externalDisplay), any())



            val capturer = ArgumentCaptor<ScreenshotData>()
@@ -473,6 +478,7 @@ class TakeScreenshotExecutorTest : SysuiTestCase() { 
        var processed: ScreenshotData? = null

        var toReturn: ScreenshotData? = null

        var shouldThrowException = false



        override suspend fun process(screenshot: ScreenshotData): ScreenshotData {

            if (shouldThrowException) throw RequestProcessorException("")

            processed = screenshot

packages/SystemUI/tests/src/com/android/systemui/screenshot/TakeScreenshotServiceTest.kt

+5 −1
Original line number Diff line number Diff line
@@ -20,6 +20,7 @@ import android.app.admin.DevicePolicyManager 
import android.app.admin.DevicePolicyResources.Strings.SystemUi.SCREENSHOT_BLOCKED_BY_ADMIN

import android.app.admin.DevicePolicyResourcesManager

import android.content.ComponentName

import android.hardware.display.DisplayManager

import android.os.UserHandle

import android.os.UserManager

import android.testing.AndroidTestingRunner
@@ -38,6 +39,7 @@ import com.android.systemui.screenshot.TakeScreenshotService.RequestCallback 
import com.android.systemui.util.mockito.any

import com.android.systemui.util.mockito.eq

import com.android.systemui.util.mockito.mock

import com.android.systemui.util.mockito.nullable

import com.android.systemui.util.mockito.whenever

import java.util.function.Consumer

import org.junit.Assert.assertEquals
@@ -68,6 +70,7 @@ class TakeScreenshotServiceTest : SysuiTestCase() { 
    private val notificationsControllerFactory = mock<ScreenshotNotificationsController.Factory>()

    private val notificationsController = mock<ScreenshotNotificationsController>()

    private val callback = mock<RequestCallback>()

    private val displayManager = mock<DisplayManager>()



    private val eventLogger = UiEventLoggerFake()

    private val flags = FakeFeatureFlags()
@@ -87,7 +90,7 @@ class TakeScreenshotServiceTest : SysuiTestCase() { 
            )

            .thenReturn(false)

        whenever(userManager.isUserUnlocked).thenReturn(true)

        whenever(controllerFactory.create(any(), any())).thenReturn(controller)

        whenever(controllerFactory.create(nullable<Display>(), any())).thenReturn(controller)

        whenever(notificationsControllerFactory.create(any())).thenReturn(notificationsController)



        // Stub request processor as a synchronous no-op for tests with the flag enabled
@@ -331,6 +334,7 @@ class TakeScreenshotServiceTest : SysuiTestCase() { 
                flags,

                requestProcessor,

                { takeScreenshotExecutor },

                displayManager,

            )

        service.attach(

            mContext,