From b396864072f671f181b8add71e83d075f6608658 Mon Sep 17 00:00:00 2001 From: Sunny Goyal Date: Fri, 29 May 2020 14:07:54 -0700 Subject: [PATCH] Fixing loadTasksInBackground called twice on swipe up Bug: 157644889 Change-Id: Ia4413ea878f8ba731fbb7a8f61b7c0c0050f3811 --- .../android/quickstep/RecentTasksList.java | 83 ++++++++++++------- .../quickstep/RecentTasksListTest.java | 8 +- 2 files changed, 59 insertions(+), 32 deletions(-) diff --git a/quickstep/src/com/android/quickstep/RecentTasksList.java b/quickstep/src/com/android/quickstep/RecentTasksList.java index 2d9c56f673..70b4f20c2b 100644 --- a/quickstep/src/com/android/quickstep/RecentTasksList.java +++ b/quickstep/src/com/android/quickstep/RecentTasksList.java @@ -40,21 +40,20 @@ import java.util.function.Consumer; /** * Manages the recent task list from the system, caching it as necessary. */ -@TargetApi(Build.VERSION_CODES.P) +@TargetApi(Build.VERSION_CODES.R) public class RecentTasksList extends TaskStackChangeListener { + private static final TaskLoadResult INVALID_RESULT = new TaskLoadResult(-1, false, 0); + private final KeyguardManagerCompat mKeyguardManager; private final LooperExecutor mMainThreadExecutor; private final ActivityManagerWrapper mActivityManagerWrapper; // The list change id, increments as the task list changes in the system private int mChangeId; - // The last change id when the list was last loaded completely, must be <= the list change id - private int mLastLoadedId; - // The last change id was loaded with keysOnly = true - private boolean mLastLoadHadKeysOnly; - ArrayList mTasks = new ArrayList<>(); + private TaskLoadResult mResultsBg = INVALID_RESULT; + private TaskLoadResult mResultsUi = INVALID_RESULT; public RecentTasksList(LooperExecutor mainThreadExecutor, KeyguardManagerCompat keyguardManager, ActivityManagerWrapper activityManagerWrapper) { @@ -71,7 +70,7 @@ public class RecentTasksList extends TaskStackChangeListener { public void getTaskKeys(int numTasks, Consumer> callback) { // Kick off task loading in the background UI_HELPER_EXECUTOR.execute(() -> { - ArrayList tasks = loadTasksInBackground(numTasks, true /* loadKeysOnly */); + ArrayList tasks = loadTasksInBackground(numTasks, -1, true /* loadKeysOnly */); mMainThreadExecutor.execute(() -> callback.accept(tasks)); }); } @@ -85,26 +84,30 @@ public class RecentTasksList extends TaskStackChangeListener { */ public synchronized int getTasks(boolean loadKeysOnly, Consumer> callback) { final int requestLoadId = mChangeId; - Runnable resultCallback = callback == null - ? () -> { } - : () -> callback.accept(copyOf(mTasks)); - - if (mLastLoadedId == mChangeId && (!mLastLoadHadKeysOnly || loadKeysOnly)) { + if (mResultsUi.isValidForRequest(requestLoadId, loadKeysOnly)) { // The list is up to date, send the callback on the next frame, // so that requestID can be returned first. - mMainThreadExecutor.post(resultCallback); + if (callback != null) { + // Copy synchronously as the changeId might change by next frame + ArrayList result = copyOf(mResultsUi); + mMainThreadExecutor.post(() -> callback.accept(result)); + } + return requestLoadId; } // Kick off task loading in the background UI_HELPER_EXECUTOR.execute(() -> { - ArrayList tasks = loadTasksInBackground(Integer.MAX_VALUE, loadKeysOnly); - + if (!mResultsBg.isValidForRequest(requestLoadId, loadKeysOnly)) { + mResultsBg = loadTasksInBackground(Integer.MAX_VALUE, requestLoadId, loadKeysOnly); + } + TaskLoadResult loadResult = mResultsBg; mMainThreadExecutor.execute(() -> { - mTasks = tasks; - mLastLoadedId = requestLoadId; - mLastLoadHadKeysOnly = loadKeysOnly; - resultCallback.run(); + mResultsUi = loadResult; + if (callback != null) { + ArrayList result = copyOf(mResultsUi); + callback.accept(result); + } }); }); @@ -119,8 +122,8 @@ public class RecentTasksList extends TaskStackChangeListener { } @Override - public synchronized void onTaskStackChanged() { - mChangeId++; + public void onTaskStackChanged() { + invalidateLoadedTasks(); } @Override @@ -131,22 +134,28 @@ public class RecentTasksList extends TaskStackChangeListener { // callback (those are for changes to the active tasks), but the task list is still updated, // so we should also invalidate the change id to ensure we load a new list instead of // reusing a stale list. - mChangeId++; + invalidateLoadedTasks(); } @Override public void onTaskRemoved(int taskId) { - mTasks = loadTasksInBackground(Integer.MAX_VALUE, false); + invalidateLoadedTasks(); } + @Override - public synchronized void onActivityPinned(String packageName, int userId, int taskId, - int stackId) { - mChangeId++; + public void onActivityPinned(String packageName, int userId, int taskId, int stackId) { + invalidateLoadedTasks(); } @Override public synchronized void onActivityUnpinned() { + invalidateLoadedTasks(); + } + + private synchronized void invalidateLoadedTasks() { + UI_HELPER_EXECUTOR.execute(() -> mResultsBg = INVALID_RESULT); + mResultsUi = INVALID_RESULT; mChangeId++; } @@ -154,9 +163,8 @@ public class RecentTasksList extends TaskStackChangeListener { * Loads and creates a list of all the recent tasks. */ @VisibleForTesting - ArrayList loadTasksInBackground(int numTasks, boolean loadKeysOnly) { + TaskLoadResult loadTasksInBackground(int numTasks, int requestId, boolean loadKeysOnly) { int currentUserId = Process.myUserHandle().getIdentifier(); - ArrayList allTasks = new ArrayList<>(); List rawTasks = mActivityManagerWrapper.getRecentTasks(numTasks, currentUserId); // The raw tasks are given in most-recent to least-recent order, we need to reverse it @@ -173,6 +181,7 @@ public class RecentTasksList extends TaskStackChangeListener { } }; + TaskLoadResult allTasks = new TaskLoadResult(requestId, loadKeysOnly, rawTasks.size()); for (ActivityManager.RecentTaskInfo rawTask : rawTasks) { Task.TaskKey taskKey = new Task.TaskKey(rawTask); Task task; @@ -197,4 +206,22 @@ public class RecentTasksList extends TaskStackChangeListener { } return newTasks; } + + private static class TaskLoadResult extends ArrayList { + + final int mId; + + // If the result was loaded with keysOnly = true + final boolean mKeysOnly; + + TaskLoadResult(int id, boolean keysOnly, int size) { + super(size); + mId = id; + mKeysOnly = keysOnly; + } + + boolean isValidForRequest(int requestId, boolean loadKeysOnly) { + return mId == requestId && (!mKeysOnly || loadKeysOnly); + } + } } \ No newline at end of file diff --git a/quickstep/tests/src/com/android/quickstep/RecentTasksListTest.java b/quickstep/tests/src/com/android/quickstep/RecentTasksListTest.java index 34eb7f8177..79ddf7a399 100644 --- a/quickstep/tests/src/com/android/quickstep/RecentTasksListTest.java +++ b/quickstep/tests/src/com/android/quickstep/RecentTasksListTest.java @@ -58,9 +58,9 @@ public class RecentTasksListTest { } @Test - public void onTaskRemoved_reloadsAllTasks() { + public void onTaskRemoved_doesNotFetchTasks() { mRecentTasksList.onTaskRemoved(0); - verify(mockActivityManagerWrapper, times(1)) + verify(mockActivityManagerWrapper, times(0)) .getRecentTasks(anyInt(), anyInt()); } @@ -77,7 +77,7 @@ public class RecentTasksListTest { when(mockActivityManagerWrapper.getRecentTasks(anyInt(), anyInt())) .thenReturn(Collections.singletonList(recentTaskInfo)); - List taskList = mRecentTasksList.loadTasksInBackground(Integer.MAX_VALUE, true); + List taskList = mRecentTasksList.loadTasksInBackground(Integer.MAX_VALUE, -1, true); assertEquals(1, taskList.size()); assertNull(taskList.get(0).taskDescription.getLabel()); @@ -91,7 +91,7 @@ public class RecentTasksListTest { when(mockActivityManagerWrapper.getRecentTasks(anyInt(), anyInt())) .thenReturn(Collections.singletonList(recentTaskInfo)); - List taskList = mRecentTasksList.loadTasksInBackground(Integer.MAX_VALUE, false); + List taskList = mRecentTasksList.loadTasksInBackground(Integer.MAX_VALUE, -1, false); assertEquals(1, taskList.size()); assertEquals(taskDescription, taskList.get(0).taskDescription.getLabel());