From 720ae87d6b8ce51ad2145eeac9ef487a0234031d Mon Sep 17 00:00:00 2001 From: Chris Li Date: Mon, 18 Apr 2022 15:52:13 +0800 Subject: Allow delete empty TaskFragment in PiP We ignore WCT for TaskFragment in PiP, but in case the empty TaskFragment is not removed before the Task enters PiP, we should allow organizer to remove it. Also make sure we have all requests in one WCT during onTaskFragmentInfoChanged. Bug: 225371112 Test: atest WmTests:TaskFragmentOrganizerControllerTest Test: atest WMJetpackUnitTests:SplitControllerTest Test: atest WMJetpackUnitTests:TaskFragmentContainerTest Change-Id: I718bbb7f6c18c35048c671ed3c131eaf536cf265 --- .../extensions/embedding/SplitController.java | 38 +++++++-- .../extensions/embedding/SplitPresenter.java | 20 ++--- .../embedding/TaskFragmentContainer.java | 12 +-- .../extensions/embedding/SplitControllerTest.java | 36 +++++++- .../embedding/TaskFragmentContainerTest.java | 97 ++++++++++++++++++++++ .../server/wm/WindowOrganizerController.java | 5 +- .../wm/TaskFragmentOrganizerControllerTest.java | 8 ++ 7 files changed, 183 insertions(+), 33 deletions(-) create mode 100644 libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/extensions/embedding/TaskFragmentContainerTest.java diff --git a/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitController.java b/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitController.java index 33a41ecd49fa..e20cef2bec4e 100644 --- a/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitController.java +++ b/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitController.java @@ -60,7 +60,8 @@ import java.util.function.Consumer; public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmentCallback, ActivityEmbeddingComponent { - private final SplitPresenter mPresenter; + @VisibleForTesting + final SplitPresenter mPresenter; // Currently applied split configuration. private final List mSplitRules = new ArrayList<>(); @@ -149,6 +150,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen return; } + final WindowContainerTransaction wct = new WindowContainerTransaction(); final boolean wasInPip = isInPictureInPicture(container); container.setInfo(taskFragmentInfo); final boolean isInPip = isInPictureInPicture(container); @@ -159,13 +161,13 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen // Do not finish the dependents if the last activity is reparented to PiP. // Instead, the original split should be cleanup, and the dependent may be expanded // to fullscreen. - cleanupForEnterPip(container); - mPresenter.cleanupContainer(container, false /* shouldFinishDependent */); + cleanupForEnterPip(wct, container); + mPresenter.cleanupContainer(container, false /* shouldFinishDependent */, wct); } else { // Do not finish the dependents if this TaskFragment was cleared due to launching // activity in the Task. final boolean shouldFinishDependent = !taskFragmentInfo.isTaskClearedForReuse(); - mPresenter.cleanupContainer(container, shouldFinishDependent); + mPresenter.cleanupContainer(container, shouldFinishDependent, wct); } } else if (wasInPip && isInPip) { // No update until exit PIP. @@ -174,12 +176,13 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen // Enter PIP. // All overrides will be cleanup. container.setLastRequestedBounds(null /* bounds */); - cleanupForEnterPip(container); + cleanupForEnterPip(wct, container); } else if (wasInPip) { // Exit PIP. // Updates the presentation of the container. Expand or launch placeholder if needed. - mPresenter.updateContainer(container); + updateContainer(wct, container); } + mPresenter.applyTransaction(wct); updateCallbackIfNecessary(); } @@ -188,7 +191,15 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen final TaskFragmentContainer container = getContainer(taskFragmentInfo.getFragmentToken()); if (container != null) { // Cleanup if the TaskFragment vanished is not requested by the organizer. - mPresenter.cleanupContainer(container, true /* shouldFinishDependent */); + removeContainer(container); + // Make sure the top container is updated. + final TaskFragmentContainer newTopContainer = getTopActiveContainer( + container.getTaskId()); + if (newTopContainer != null) { + final WindowContainerTransaction wct = new WindowContainerTransaction(); + updateContainer(wct, newTopContainer); + mPresenter.applyTransaction(wct); + } updateCallbackIfNecessary(); } cleanupTaskFragment(taskFragmentInfo.getFragmentToken()); @@ -452,7 +463,8 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen } /** Cleanups all the dependencies when the TaskFragment is entering PIP. */ - private void cleanupForEnterPip(@NonNull TaskFragmentContainer container) { + private void cleanupForEnterPip(@NonNull WindowContainerTransaction wct, + @NonNull TaskFragmentContainer container) { final int taskId = container.getTaskId(); final TaskContainer taskContainer = mTaskContainers.get(taskId); if (taskContainer == null) { @@ -482,7 +494,9 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen // If there is any TaskFragment split with the PIP TaskFragment, update their presentations // since the split is dismissed. // We don't want to close any of them even if they are dependencies of the PIP TaskFragment. - mPresenter.updateContainers(containersToUpdate); + for (TaskFragmentContainer containerToUpdate : containersToUpdate) { + updateContainer(wct, containerToUpdate); + } } /** @@ -502,6 +516,7 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen // TaskFragment there. taskContainer.mFinishedContainer.add(container.getTaskFragmentToken()); + // Cleanup any split references. final List containersToRemove = new ArrayList<>(); for (SplitContainer splitContainer : taskContainer.mSplitContainers) { if (container.equals(splitContainer.getSecondaryContainer()) @@ -510,6 +525,11 @@ public class SplitController implements JetpackTaskFragmentOrganizer.TaskFragmen } } taskContainer.mSplitContainers.removeAll(containersToRemove); + + // Cleanup any dependent references. + for (TaskFragmentContainer containerToUpdate : taskContainer.mContainers) { + containerToUpdate.removeContainerToFinishOnExit(container); + } } /** diff --git a/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitPresenter.java b/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitPresenter.java index b55c16e3b45d..1b49585ed7dc 100644 --- a/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitPresenter.java +++ b/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/SplitPresenter.java @@ -36,7 +36,6 @@ import androidx.annotation.IntDef; import androidx.annotation.NonNull; import androidx.annotation.Nullable; -import java.util.Collection; import java.util.concurrent.Executor; /** @@ -73,16 +72,12 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { } /** - * Updates the presentation of the provided containers. + * Deletes the specified container and all other associated and dependent containers in the same + * transaction. */ - void updateContainers(@NonNull Collection containers) { - if (containers.isEmpty()) { - return; - } + void cleanupContainer(@NonNull TaskFragmentContainer container, boolean shouldFinishDependent) { final WindowContainerTransaction wct = new WindowContainerTransaction(); - for (TaskFragmentContainer container : containers) { - mController.updateContainer(wct, container); - } + cleanupContainer(container, shouldFinishDependent, wct); applyTransaction(wct); } @@ -90,9 +85,8 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { * Deletes the specified container and all other associated and dependent containers in the same * transaction. */ - void cleanupContainer(@NonNull TaskFragmentContainer container, boolean shouldFinishDependent) { - final WindowContainerTransaction wct = new WindowContainerTransaction(); - + void cleanupContainer(@NonNull TaskFragmentContainer container, boolean shouldFinishDependent, + @NonNull WindowContainerTransaction wct) { container.finish(shouldFinishDependent, this, wct, mController); final TaskFragmentContainer newTopContainer = mController.getTopActiveContainer( @@ -100,8 +94,6 @@ class SplitPresenter extends JetpackTaskFragmentOrganizer { if (newTopContainer != null) { mController.updateContainer(wct, newTopContainer); } - - applyTransaction(wct); } /** diff --git a/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/TaskFragmentContainer.java b/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/TaskFragmentContainer.java index 20c929b26acb..871f545d203a 100644 --- a/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/TaskFragmentContainer.java +++ b/libs/WindowManager/Jetpack/src/androidx/window/extensions/embedding/TaskFragmentContainer.java @@ -257,7 +257,8 @@ class TaskFragmentContainer { // Finish dependent containers for (TaskFragmentContainer container : mContainersToFinishOnExit) { - if (controller.shouldRetainAssociatedContainer(this, container)) { + if (container.mIsFinished + || controller.shouldRetainAssociatedContainer(this, container)) { continue; } container.finish(true /* shouldFinishDependent */, presenter, @@ -267,18 +268,13 @@ class TaskFragmentContainer { // Finish associated activities for (Activity activity : mActivitiesToFinishOnExit) { - if (controller.shouldRetainAssociatedActivity(this, activity)) { + if (activity.isFinishing() + || controller.shouldRetainAssociatedActivity(this, activity)) { continue; } activity.finish(); } mActivitiesToFinishOnExit.clear(); - - // Finish activities that were being re-parented to this container. - for (Activity activity : mPendingAppearedActivities) { - activity.finish(); - } - mPendingAppearedActivities.clear(); } boolean isFinished() { diff --git a/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/extensions/embedding/SplitControllerTest.java b/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/extensions/embedding/SplitControllerTest.java index a26a4b657d68..72519dc6da5f 100644 --- a/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/extensions/embedding/SplitControllerTest.java +++ b/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/extensions/embedding/SplitControllerTest.java @@ -17,13 +17,20 @@ package androidx.window.extensions.embedding; import static com.android.dx.mockito.inline.extended.ExtendedMockito.spyOn; +import static com.android.dx.mockito.inline.extended.ExtendedMockito.verify; import static com.google.common.truth.Truth.assertWithMessage; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import android.app.Activity; +import android.content.res.Configuration; +import android.content.res.Resources; import android.platform.test.annotations.Presubmit; +import android.window.TaskFragmentInfo; import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.filters.SmallTest; @@ -32,12 +39,14 @@ import androidx.window.extensions.embedding.SplitController.TaskContainer; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; /** * Test class for {@link SplitController}. * * Build/Install/Run: - * atest WMJetpackUnitTests:SplitController + * atest WMJetpackUnitTests:SplitControllerTest */ @Presubmit @SmallTest @@ -45,12 +54,24 @@ import org.junit.runner.RunWith; public class SplitControllerTest { private static final int TASK_ID = 10; + @Mock + private Activity mActivity; + @Mock + private Resources mActivityResources; + @Mock + private TaskFragmentInfo mInfo; private SplitController mSplitController; + private SplitPresenter mSplitPresenter; @Before public void setUp() { + MockitoAnnotations.initMocks(this); mSplitController = new SplitController(); + mSplitPresenter = mSplitController.mPresenter; spyOn(mSplitController); + spyOn(mSplitPresenter); + doReturn(mActivityResources).when(mActivity).getResources(); + doReturn(new Configuration()).when(mActivityResources).getConfiguration(); } @Test @@ -83,4 +104,17 @@ public class SplitControllerTest { assertWithMessage("Must return null because tf1 has no running activity.") .that(mSplitController.getTopActiveContainer(TASK_ID)).isNull(); } + + @Test + public void testOnTaskFragmentVanished() { + final TaskFragmentContainer tf = mSplitController.newContainer(mActivity, TASK_ID); + doReturn(tf.getTaskFragmentToken()).when(mInfo).getFragmentToken(); + + // The TaskFragment has been removed in the server, we only need to cleanup the reference. + mSplitController.onTaskFragmentVanished(mInfo); + + verify(mSplitPresenter, never()).deleteTaskFragment(any(), any()); + verify(mSplitController).removeContainer(tf); + verify(mActivity, never()).finish(); + } } diff --git a/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/extensions/embedding/TaskFragmentContainerTest.java b/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/extensions/embedding/TaskFragmentContainerTest.java new file mode 100644 index 000000000000..97896c2c0a57 --- /dev/null +++ b/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/extensions/embedding/TaskFragmentContainerTest.java @@ -0,0 +1,97 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package androidx.window.extensions.embedding; + +import static com.android.dx.mockito.inline.extended.ExtendedMockito.verify; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; + +import android.app.Activity; +import android.platform.test.annotations.Presubmit; +import android.window.TaskFragmentInfo; +import android.window.WindowContainerTransaction; + +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.SmallTest; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import java.util.ArrayList; + +/** + * Test class for {@link TaskFragmentContainer}. + * + * Build/Install/Run: + * atest WMJetpackUnitTests:TaskFragmentContainerTest + */ +@Presubmit +@SmallTest +@RunWith(AndroidJUnit4.class) +public class TaskFragmentContainerTest { + private static final int TASK_ID = 10; + + @Mock + private SplitPresenter mPresenter; + @Mock + private SplitController mController; + @Mock + private Activity mActivity; + @Mock + private TaskFragmentInfo mInfo; + + @Before + public void setup() { + MockitoAnnotations.initMocks(this); + } + + @Test + public void testFinish() { + final TaskFragmentContainer container = new TaskFragmentContainer(mActivity, TASK_ID); + final WindowContainerTransaction wct = new WindowContainerTransaction(); + + // Only remove the activity, but not clear the reference until appeared. + container.finish(true /* shouldFinishDependent */, mPresenter, wct, mController); + + verify(mActivity).finish(); + verify(mPresenter, never()).deleteTaskFragment(any(), any()); + verify(mController, never()).removeContainer(any()); + + // Calling twice should not finish activity again. + clearInvocations(mActivity); + container.finish(true /* shouldFinishDependent */, mPresenter, wct, mController); + + verify(mActivity, never()).finish(); + verify(mPresenter, never()).deleteTaskFragment(any(), any()); + verify(mController, never()).removeContainer(any()); + + // Remove all references after the container has appeared in server. + doReturn(new ArrayList<>()).when(mInfo).getActivities(); + container.setInfo(mInfo); + container.finish(true /* shouldFinishDependent */, mPresenter, wct, mController); + + verify(mActivity, never()).finish(); + verify(mPresenter).deleteTaskFragment(wct, container.getTaskFragmentToken()); + verify(mController).removeContainer(container); + } +} diff --git a/services/core/java/com/android/server/wm/WindowOrganizerController.java b/services/core/java/com/android/server/wm/WindowOrganizerController.java index c1c8b81f2c32..67f7ff701981 100644 --- a/services/core/java/com/android/server/wm/WindowOrganizerController.java +++ b/services/core/java/com/android/server/wm/WindowOrganizerController.java @@ -1522,7 +1522,10 @@ class WindowOrganizerController extends IWindowOrganizerController.Stub sendTaskFragmentOperationFailure(organizer, errorCallbackToken, exception); return 0; } - if (taskFragment.isEmbeddedTaskFragmentInPip()) { + if (taskFragment.isEmbeddedTaskFragmentInPip() + // When the Task enters PiP before the organizer removes the empty TaskFragment, we + // should allow it to do the cleanup. + && taskFragment.getTopNonFinishingActivity() != null) { final Throwable exception = new IllegalArgumentException( "Not allowed to delete TaskFragment in PIP Task"); sendTaskFragmentOperationFailure(organizer, errorCallbackToken, exception); diff --git a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java index d135de0fc921..a297608af480 100644 --- a/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java +++ b/services/tests/wmtests/src/com/android/server/wm/TaskFragmentOrganizerControllerTest.java @@ -527,6 +527,14 @@ public class TaskFragmentOrganizerControllerTest extends WindowTestsBase { verify(mAtm.mWindowOrganizerController).sendTaskFragmentOperationFailure(eq(mIOrganizer), eq(errorToken), any(IllegalArgumentException.class)); assertNotNull(mAtm.mWindowOrganizerController.getTaskFragment(mFragmentToken)); + + // Allow organizer to delete empty TaskFragment for cleanup. + final Task task = mTaskFragment.getTask(); + mTaskFragment.removeChild(mTaskFragment.getTopMostActivity()); + mAtm.mWindowOrganizerController.applyTransaction(mTransaction); + + assertNull(mAtm.mWindowOrganizerController.getTaskFragment(mFragmentToken)); + assertNull(task.getTopChild()); } @Test -- cgit v1.2.3-59-g8ed1b