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

Commit 194f30e4 authored by Ikram Gabiyev's avatar Ikram Gabiyev
Browse files

[PiP2] Do not allow multiple on-idle runnables 

Earlier, it was actually possible to schedule multiple
onIdlePipTransitionStateRunnables if done in quick succession.
Effectively, the first runnable could get posted, the local cache would
be cleared, and if that posted runnable did not have enough time to
actually execute, we could remain in an idle state briefly.
This means any follow-up setOnIdlePipTransitionStateRunnable() call
would still go through and post the callback onto the message queue as
well. This can introduce race conditions in PipTransitionState.

Instead, we wanna enforce one on-idle runnable scheduled at a time,
by clearing out the previously scheduled on-idle runnables from the msg
queue as needed.

We should also allow setting the state to SCHEDULED_BOUNDS_CHANGE only
when in a valid idle PiP state.

Lastly, we are adding a sanity check wtf log to finishTransition()
in case we end up in an UNDEFINED PipTransitionState.

Bug: 405394597
Flag: com.android.wm.shell.enable_pip2
Test: atest PipTransitionStateTest
Change-Id: Ic3a1cb187e0b778ac755d449e02b15e894556bbc
parent ed71cd15

libs/WindowManager/Shell/src/com/android/wm/shell/pip2/phone/PipTransition.java

+7 −0
Original line number Diff line number Diff line
@@ -966,6 +966,13 @@ public class PipTransition extends PipTransitionController implements 
                nextState = PipTransitionState.EXITED_PIP;

                break;

        }



        if (nextState == PipTransitionState.UNDEFINED) {

            Log.wtf(TAG, String.format("""

                        PipTransitionState resolved to an undefined state in finishTransition().

                        callers=%s""", Debug.getCallers(4)));

        }



        mPipTransitionState.setState(nextState);



        if (mFinishCallback != null) {

libs/WindowManager/Shell/src/com/android/wm/shell/pip2/phone/PipTransitionState.java

+16 −6
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ import android.app.TaskInfo; 
import android.graphics.Rect;

import android.os.Bundle;

import android.os.Handler;

import android.os.Message;

import android.view.SurfaceControl;

import android.window.WindowContainerToken;
@@ -260,24 +261,33 @@ public class PipTransitionState { 
     *

     * <p>We only allow for one callback to be scheduled to avoid cases with multiple transitions

     * being scheduled. For instance, if user double taps and IME shows, this would

     * schedule a bounds change transition for IME appearing. But if some other transition would

     * want to animate PiP before the scheduled callback executes, we would rather want to replace

     * the existing callback with a new one, to avoid multiple animations

     * as soon as we are idle.</p>

     * schedule a bounds change transition for IME appearing.</p>

     *

     * <p>Only on-idle runnable can be scheduled at a time, so attempting to schedule a new one

     * in quick succession should remove the previous one from the message queue.</p>

     */

    public void setOnIdlePipTransitionStateRunnable(

            @Nullable Runnable onIdlePipTransitionStateRunnable) {

        mMainHandler.removeMessages(PipTransitionState.class.hashCode());

        mOnIdlePipTransitionStateRunnable = onIdlePipTransitionStateRunnable;

        maybeRunOnIdlePipTransitionStateCallback();

    }



    private void maybeRunOnIdlePipTransitionStateCallback() {

        if (mOnIdlePipTransitionStateRunnable != null && isPipStateIdle()) {

            mMainHandler.post(mOnIdlePipTransitionStateRunnable);

            final Message msg = mMainHandler.obtainMessage(PipTransitionState.class.hashCode());

            msg.setCallback(mOnIdlePipTransitionStateRunnable);

            mMainHandler.sendMessage(msg);

            mOnIdlePipTransitionStateRunnable = null;

        }

    }



    @VisibleForTesting

    @Nullable

    Runnable getOnIdlePipTransitionStateRunnable() {

        return mOnIdlePipTransitionStateRunnable;

    }



    /**

     * Adds a {@link PipTransitionStateChangedListener} for future PiP transition state updates.

     */
@@ -398,7 +408,7 @@ public class PipTransitionState { 
                //   started playing yet

                // - there is no drag-to-desktop gesture in progress; otherwise the PiP resize

                //   transition will block the drag-to-desktop transitions from finishing

                return isInPip() && !mPipDesktopState.isDragToDesktopInProgress();

                return isPipStateIdle() && !mPipDesktopState.isDragToDesktopInProgress();

            default:

                return true;

        }

libs/WindowManager/Shell/tests/unittest/src/com/android/wm/shell/pip2/phone/PipTransitionStateTest.java

+85 −0
Original line number Diff line number Diff line
@@ -16,10 +16,20 @@ 


package com.android.wm.shell.pip2.phone;



import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyInt;

import static org.mockito.ArgumentMatchers.argThat;

import static org.mockito.ArgumentMatchers.eq;

import static org.mockito.Mockito.clearInvocations;

import static org.mockito.Mockito.mock;

import static org.mockito.Mockito.never;

import static org.mockito.Mockito.times;

import static org.mockito.Mockito.verify;

import static org.mockito.Mockito.when;



import android.os.Bundle;

import android.os.Handler;

import android.os.Message;

import android.os.Parcelable;

import android.testing.AndroidTestingRunner;
@@ -159,4 +169,79 @@ public class PipTransitionStateTest extends ShellTestCase { 
        Assert.assertFalse(mPipTransitionState.shouldTransitionToState(

                PipTransitionState.SCHEDULED_BOUNDS_CHANGE));

    }



    @Test

    public void shouldTransitionToState_scheduledBoundsChangeWhileChangingBounds_returnsFalse() {

        Bundle extra = new Bundle();

        extra.putParcelable(EXTRA_ENTRY_KEY, mEmptyParcelable);

        mPipTransitionState.setState(PipTransitionState.CHANGING_PIP_BOUNDS, extra);



        Assert.assertFalse(mPipTransitionState.shouldTransitionToState(

                PipTransitionState.SCHEDULED_BOUNDS_CHANGE));

    }



    @Test

    public void testResetSameState_scheduledBoundsChange_doNotDispatchStateChanged() {

        Bundle extra = new Bundle();

        extra.putParcelable(EXTRA_ENTRY_KEY, mEmptyParcelable);

        mPipTransitionState.setState(PipTransitionState.SCHEDULED_BOUNDS_CHANGE, extra);



        mStateChangedListener = mock(PipTransitionState.PipTransitionStateChangedListener.class);

        verify(mStateChangedListener, never())

                .onPipTransitionStateChanged(anyInt(), anyInt(), any());



        mPipTransitionState.addPipTransitionStateChangedListener(mStateChangedListener);

        mPipTransitionState.setState(PipTransitionState.SCHEDULED_BOUNDS_CHANGE, extra);

    }



    @Test

    public void testResetOnIdlePipTransitionStateRunnable_whileIdle_removePrevRunnable() {

        when(mMainHandler.obtainMessage(anyInt())).thenAnswer(invocation ->

                new Message().setWhat(invocation.getArgument(0)));



        // pick an idle ENTERED_PIP state

        mPipTransitionState.setState(PipTransitionState.ENTERED_PIP);



        int what = PipTransitionState.class.hashCode();



        final Runnable firstOnIdleRunnable = () -> {};



        mPipTransitionState.setOnIdlePipTransitionStateRunnable(firstOnIdleRunnable);

        verify(mMainHandler, times(1)).removeMessages(eq(what));

        verify(mMainHandler, times(1))

                .sendMessage(argThat(msg -> msg.getCallback() == firstOnIdleRunnable));



        clearInvocations(mMainHandler);



        final Runnable secondOnIdleRunnable = () -> {};

        mPipTransitionState.setOnIdlePipTransitionStateRunnable(secondOnIdleRunnable);

        verify(mMainHandler, times(1)).removeMessages(eq(what));

        verify(mMainHandler, times(1))

                .sendMessage(argThat(msg -> msg.getCallback() == secondOnIdleRunnable));

    }



    @Test

    public void testSetOnIdlePipTransitionStateRunnable_notIdle_postAndClearRunnableOnceIdle() {

        when(mMainHandler.obtainMessage(anyInt())).thenAnswer(invocation ->

                new Message().setWhat(invocation.getArgument(0)));



        // pick a non-idle state

        Bundle extra = new Bundle();

        extra.putParcelable(EXTRA_ENTRY_KEY, mEmptyParcelable);

        mPipTransitionState.setState(PipTransitionState.CHANGING_PIP_BOUNDS, extra);



        final Runnable onIdleRunnable = () -> {};

        mPipTransitionState.setOnIdlePipTransitionStateRunnable(onIdleRunnable);



        verify(mMainHandler, never()).sendMessage(any());



        // advance to an idle state

        mPipTransitionState.setState(PipTransitionState.CHANGED_PIP_BOUNDS);



        verify(mMainHandler, times(1))

                .sendMessage(argThat(msg -> msg.getCallback() == onIdleRunnable));



        Assert.assertNull("onIdle runnable not cleared",

                mPipTransitionState.getOnIdlePipTransitionStateRunnable());

    }

}