Merge "Prioritize delete jobs by adding them to a separate thread pool." into nyc-andromeda-dev
diff --git a/src/com/android/documentsui/dirlist/DirectoryFragment.java b/src/com/android/documentsui/dirlist/DirectoryFragment.java
index 1a1fe61..efe76df 100644
--- a/src/com/android/documentsui/dirlist/DirectoryFragment.java
+++ b/src/com/android/documentsui/dirlist/DirectoryFragment.java
@@ -869,12 +869,7 @@
(TextView) mInflater.inflate(R.layout.dialog_delete_confirmation, null);
message.setText(generateDeleteMessage(docs));
- // This "insta-hides" files that are being deleted, because
- // the delete operation may be not execute immediately (it
- // may be queued up on the FileOperationService.)
- // To hide the files locally, we call the hide method on the adapter
- // ...which a live object...cannot be parceled.
- // For that reason, for now, we implement this dialog NOT
+ // For now, we implement this dialog NOT
// as a fragment (which can survive rotation and have its own state),
// but as a simple runtime dialog. So rotating a device with an
// active delete dialog...results in that dialog disappearing.
@@ -895,10 +890,7 @@
} else {
Log.w(TAG, "Action mode is null before deleting documents.");
}
- // Hide the files in the UI...since the operation
- // might be queued up on FileOperationService.
- // We're walking a line here.
- mAdapter.hide(selected.getAll());
+
FileOperations.delete(
getActivity(), docs, srcParent, getDisplayState().stack);
}
diff --git a/src/com/android/documentsui/dirlist/DocumentsAdapter.java b/src/com/android/documentsui/dirlist/DocumentsAdapter.java
index 0bbecf9..4b35447 100644
--- a/src/com/android/documentsui/dirlist/DocumentsAdapter.java
+++ b/src/com/android/documentsui/dirlist/DocumentsAdapter.java
@@ -23,7 +23,6 @@
import android.provider.DocumentsContract.Document;
import android.support.v7.widget.GridLayoutManager;
import android.support.v7.widget.RecyclerView;
-import android.util.SparseArray;
import com.android.documentsui.State;
@@ -65,15 +64,6 @@
abstract String getModelId(int position);
/**
- * Hides a set of items from the associated RecyclerView.
- *
- * @param ids The Model IDs of the items to hide.
- * @return A SparseArray that maps the hidden IDs to their old positions. This can be used
- * to {@link #unhide} the items if necessary.
- */
- abstract public SparseArray<String> hide(String... ids);
-
- /**
* Returns a class that yields the span size for a particular element. This is
* primarily useful in {@link SectionBreakDocumentsAdapterWrapper} where
* we adjust sizes.
diff --git a/src/com/android/documentsui/dirlist/ModelBackedDocumentsAdapter.java b/src/com/android/documentsui/dirlist/ModelBackedDocumentsAdapter.java
index ca3b2e2..2c1a221 100644
--- a/src/com/android/documentsui/dirlist/ModelBackedDocumentsAdapter.java
+++ b/src/com/android/documentsui/dirlist/ModelBackedDocumentsAdapter.java
@@ -25,11 +25,9 @@
import android.database.Cursor;
import android.provider.DocumentsContract.Document;
import android.util.Log;
-import android.util.SparseArray;
import android.view.ViewGroup;
import com.android.documentsui.State;
-import com.google.common.collect.Sets;
import java.util.ArrayList;
import java.util.HashSet;
@@ -166,26 +164,6 @@
}
@Override
- public SparseArray<String> hide(String... ids) {
- if (DEBUG) Log.d(TAG, "Hiding ids: " + ids);
- Set<String> toHide = Sets.newHashSet(ids);
-
- // Proceed backwards through the list of items, because each removal causes the
- // positions of all subsequent items to change.
- SparseArray<String> hiddenItems = new SparseArray<>();
- for (int i = mModelIds.size() - 1; i >= 0; --i) {
- String id = mModelIds.get(i);
- if (toHide.contains(id)) {
- mHiddenIds.add(id);
- hiddenItems.put(i, mModelIds.remove(i));
- notifyItemRemoved(i);
- }
- }
-
- return hiddenItems;
- }
-
- @Override
public List<String> getModelIds() {
return mModelIds;
}
diff --git a/src/com/android/documentsui/dirlist/SectionBreakDocumentsAdapterWrapper.java b/src/com/android/documentsui/dirlist/SectionBreakDocumentsAdapterWrapper.java
index b698059..55a6de7 100644
--- a/src/com/android/documentsui/dirlist/SectionBreakDocumentsAdapterWrapper.java
+++ b/src/com/android/documentsui/dirlist/SectionBreakDocumentsAdapterWrapper.java
@@ -20,7 +20,6 @@
import android.database.Cursor;
import android.support.v7.widget.GridLayoutManager;
import android.support.v7.widget.RecyclerView.AdapterDataObserver;
-import android.util.SparseArray;
import android.view.ViewGroup;
import android.widget.Space;
@@ -162,13 +161,6 @@
}
@Override
- public SparseArray<String> hide(String... ids) {
- // NOTE: We hear about these changes and adjust break position
- // in our AdapterDataObserver.
- return mDelegate.hide(ids);
- }
-
- @Override
List<String> getModelIds() {
return mDelegate.getModelIds();
}
diff --git a/src/com/android/documentsui/services/FileOperationService.java b/src/com/android/documentsui/services/FileOperationService.java
index 871e135..5685f86 100644
--- a/src/com/android/documentsui/services/FileOperationService.java
+++ b/src/com/android/documentsui/services/FileOperationService.java
@@ -38,17 +38,14 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.ScheduledFuture;
-import java.util.concurrent.ScheduledThreadPoolExecutor;
-import java.util.concurrent.TimeUnit;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
import javax.annotation.concurrent.GuardedBy;
public class FileOperationService extends Service implements Job.Listener {
- private static final int DEFAULT_DELAY = 0;
- private static final int MAX_DELAY = 10 * 1000; // ten seconds
private static final int POOL_SIZE = 2; // "pool size", not *max* "pool size".
private static final int NOTIFICATION_ID_PROGRESS = 0;
private static final int NOTIFICATION_ID_FAILURE = 1;
@@ -57,7 +54,6 @@
public static final String TAG = "FileOperationService";
public static final String EXTRA_JOB_ID = "com.android.documentsui.JOB_ID";
- public static final String EXTRA_DELAY = "com.android.documentsui.DELAY";
public static final String EXTRA_OPERATION = "com.android.documentsui.OPERATION";
public static final String EXTRA_CANCEL = "com.android.documentsui.CANCEL";
public static final String EXTRA_SRC_LIST = "com.android.documentsui.SRC_LIST";
@@ -87,7 +83,10 @@
// The executor and job factory are visible for testing and non-final
// so we'll have a way to inject test doubles from the test. It's
// a sub-optimal arrangement.
- @VisibleForTesting ScheduledExecutorService executor;
+ @VisibleForTesting ExecutorService executor;
+
+ // Use a separate thread pool to prioritize deletions
+ @VisibleForTesting ExecutorService deletionExecutor;
@VisibleForTesting Factory jobFactory;
private PowerManager mPowerManager;
@@ -103,7 +102,11 @@
public void onCreate() {
// Allow tests to pre-set these with test doubles.
if (executor == null) {
- executor = new ScheduledThreadPoolExecutor(POOL_SIZE);
+ executor = Executors.newFixedThreadPool(POOL_SIZE);
+ }
+
+ if (deletionExecutor == null) {
+ deletionExecutor = Executors.newCachedThreadPool();
}
if (jobFactory == null) {
@@ -171,11 +174,8 @@
}
assert(job != null);
- int delay = intent.getIntExtra(EXTRA_DELAY, DEFAULT_DELAY);
- assert(delay <= MAX_DELAY);
- if (DEBUG) Log.d(
- TAG, "Scheduling job " + job.id + " to run in " + delay + " milliseconds.");
- ScheduledFuture<?> future = executor.schedule(job, delay, TimeUnit.MILLISECONDS);
+ if (DEBUG) Log.d(TAG, "Scheduling job " + job.id + ".");
+ Future<?> future = getExecutorService(operationType).submit(job);
mRunning.put(jobId, new JobRecord(job, future));
}
@@ -200,14 +200,6 @@
JobRecord record = mRunning.get(jobId);
if (record != null) {
record.job.cancel();
-
- // If the job hasn't been started, cancel it and explicitly clean up.
- // If it *has* been started, we wait for it to recognize this, then
- // allow it stop working in an orderly fashion.
- if (record.future.getDelay(TimeUnit.MILLISECONDS) > 0) {
- record.future.cancel(false);
- onFinished(record.job);
- }
}
}
@@ -257,6 +249,18 @@
}
}
+ private ExecutorService getExecutorService(@OpType int operationType) {
+ switch (operationType) {
+ case OPERATION_COPY:
+ case OPERATION_MOVE:
+ return executor;
+ case OPERATION_DELETE:
+ return deletionExecutor;
+ default:
+ throw new UnsupportedOperationException();
+ }
+ }
+
@GuardedBy("mRunning")
private void deleteJob(Job job) {
if (DEBUG) Log.d(TAG, "deleteJob: " + job.id);
@@ -333,9 +337,9 @@
private static final class JobRecord {
private final Job job;
- private final ScheduledFuture<?> future;
+ private final Future<?> future;
- public JobRecord(Job job, ScheduledFuture<?> future) {
+ public JobRecord(Job job, Future<?> future) {
this.job = job;
this.future = future;
}
diff --git a/tests/Android.mk b/tests/Android.mk
index 31b7c49..6472bf6 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -8,7 +8,7 @@
LOCAL_SRC_FILES := $(call all-java-files-under, src)
LOCAL_JAVA_LIBRARIES := android-support-v4 android.test.runner
-LOCAL_STATIC_JAVA_LIBRARIES := mockito-target ub-uiautomator espresso-core
+LOCAL_STATIC_JAVA_LIBRARIES := mockito-target ub-uiautomator espresso-core guava
LOCAL_PACKAGE_NAME := DocumentsUITests
LOCAL_INSTRUMENTATION_FOR := DocumentsUI
diff --git a/tests/src/com/android/documentsui/dirlist/ModelBackedDocumentsAdapterTest.java b/tests/src/com/android/documentsui/dirlist/ModelBackedDocumentsAdapterTest.java
index 7ab51ba..236621f 100644
--- a/tests/src/com/android/documentsui/dirlist/ModelBackedDocumentsAdapterTest.java
+++ b/tests/src/com/android/documentsui/dirlist/ModelBackedDocumentsAdapterTest.java
@@ -21,13 +21,10 @@
import android.support.v7.widget.RecyclerView;
import android.test.AndroidTestCase;
import android.test.suitebuilder.annotation.SmallTest;
-import android.util.SparseArray;
import android.view.ViewGroup;
import com.android.documentsui.State;
-import java.util.List;
-
@SmallTest
public class ModelBackedDocumentsAdapterTest extends AndroidTestCase {
@@ -66,13 +63,6 @@
assertEquals(mModel.getItemCount(), mAdapter.getItemCount());
}
- // Tests that the item count is correct.
- public void testHide_ItemCount() {
- String[] ids = mModel.getModelIds();
- mAdapter.hide(ids[0], ids[1]);
- assertEquals(mModel.getItemCount() - 2, mAdapter.getItemCount());
- }
-
private final class TestEnvironment implements DocumentsAdapter.Environment {
private final Context testContext;
diff --git a/tests/src/com/android/documentsui/dirlist/TestDocumentsAdapter.java b/tests/src/com/android/documentsui/dirlist/TestDocumentsAdapter.java
index e170dbb..4df0fa1 100644
--- a/tests/src/com/android/documentsui/dirlist/TestDocumentsAdapter.java
+++ b/tests/src/com/android/documentsui/dirlist/TestDocumentsAdapter.java
@@ -16,7 +16,6 @@
package com.android.documentsui.dirlist;
-import android.util.SparseArray;
import android.view.ViewGroup;
import java.util.ArrayList;
@@ -56,11 +55,6 @@
}
@Override
- public SparseArray<String> hide(String... ids) {
- throw new UnsupportedOperationException();
- }
-
- @Override
public DocumentHolder onCreateViewHolder(ViewGroup parent, int viewType) {
throw new UnsupportedOperationException();
}
diff --git a/tests/src/com/android/documentsui/services/FileOperationServiceTest.java b/tests/src/com/android/documentsui/services/FileOperationServiceTest.java
index 96d963f..f385776 100644
--- a/tests/src/com/android/documentsui/services/FileOperationServiceTest.java
+++ b/tests/src/com/android/documentsui/services/FileOperationServiceTest.java
@@ -17,6 +17,7 @@
package com.android.documentsui.services;
import static com.android.documentsui.services.FileOperationService.OPERATION_COPY;
+import static com.android.documentsui.services.FileOperationService.OPERATION_DELETE;
import static com.android.documentsui.services.FileOperations.createBaseIntent;
import static com.android.documentsui.services.FileOperations.createJobId;
import static com.google.android.collect.Lists.newArrayList;
@@ -47,6 +48,7 @@
private FileOperationService mService;
private TestScheduledExecutorService mExecutor;
+ private TestScheduledExecutorService mDeletionExecutor;
private TestJobFactory mJobFactory;
public FileOperationServiceTest() {
@@ -59,6 +61,7 @@
setupService(); // must be called first for our test setup to work correctly.
mExecutor = new TestScheduledExecutorService();
+ mDeletionExecutor = new TestScheduledExecutorService();
mJobFactory = new TestJobFactory();
// Install test doubles.
@@ -67,36 +70,62 @@
assertNull(mService.executor);
mService.executor = mExecutor;
+ assertNull(mService.deletionExecutor);
+ mService.deletionExecutor = mDeletionExecutor;
+
assertNull(mService.jobFactory);
mService.jobFactory = mJobFactory;
}
- public void testRunsJobs() throws Exception {
+ public void testRunsCopyJobs() throws Exception {
startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
mExecutor.runAll();
- mJobFactory.assertAllJobsStarted();
+ mJobFactory.assertAllCopyJobsStarted();
}
- public void testRunsJobs_AfterExceptionInJobCreation() throws Exception {
+ public void testRunsCopyJobs_AfterExceptionInJobCreation() throws Exception {
startService(createCopyIntent(new ArrayList<DocumentInfo>(), BETA_DOC));
startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
mJobFactory.assertJobsCreated(1);
mExecutor.runAll();
- mJobFactory.assertAllJobsStarted();
+ mJobFactory.assertAllCopyJobsStarted();
}
- public void testRunsJobs_AfterFailure() throws Exception {
+ public void testRunsCopyJobs_AfterFailure() throws Exception {
startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
- mJobFactory.jobs.get(0).fail(ALPHA_DOC);
+ mJobFactory.copyJobs.get(0).fail(ALPHA_DOC);
mExecutor.runAll();
- mJobFactory.assertAllJobsStarted();
+ mJobFactory.assertAllCopyJobsStarted();
+ }
+
+ public void testRunsCopyJobs_notRunsDeleteJobs() throws Exception {
+ startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
+ startService(createDeleteIntent(newArrayList(GAMMA_DOC)));
+
+ mExecutor.runAll();
+ mJobFactory.assertNoDeleteJobsStarted();
+ }
+
+ public void testRunsDeleteJobs() throws Exception {
+ startService(createDeleteIntent(newArrayList(ALPHA_DOC)));
+
+ mDeletionExecutor.runAll();
+ mJobFactory.assertAllDeleteJobsStarted();
+ }
+
+ public void testRunsDeleteJobs_NotRunsCopyJobs() throws Exception {
+ startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
+ startService(createDeleteIntent(newArrayList(GAMMA_DOC)));
+
+ mDeletionExecutor.runAll();
+ mJobFactory.assertNoCopyJobsStarted();
}
public void testHoldsWakeLockWhileWorking() throws Exception {
@@ -136,7 +165,7 @@
startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
- mJobFactory.jobs.get(0).fail(ALPHA_DOC);
+ mJobFactory.copyJobs.get(0).fail(ALPHA_DOC);
mExecutor.runAll();
shutdownService();
@@ -148,8 +177,8 @@
startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
- mJobFactory.jobs.get(0).fail(ALPHA_DOC);
- mJobFactory.jobs.get(1).fail(GAMMA_DOC);
+ mJobFactory.copyJobs.get(0).fail(ALPHA_DOC);
+ mJobFactory.copyJobs.get(1).fail(GAMMA_DOC);
mExecutor.runAll();
shutdownService();
@@ -165,6 +194,12 @@
return createBaseIntent(OPERATION_COPY, getContext(), createJobId(), files, stack);
}
+ private Intent createDeleteIntent(ArrayList<DocumentInfo> files) {
+ DocumentStack stack = new DocumentStack();
+
+ return createBaseIntent(OPERATION_DELETE, getContext(), createJobId(), files, stack);
+ }
+
private static DocumentInfo createDoc(String name) {
// Doesn't need to be valid content Uri, just some urly looking thing.
Uri uri = new Uri.Builder()
@@ -184,16 +219,35 @@
private final class TestJobFactory extends Job.Factory {
- final List<TestJob> jobs = new ArrayList<>();
+ final List<TestJob> copyJobs = new ArrayList<>();
+ final List<TestJob> deleteJobs = new ArrayList<>();
- void assertAllJobsStarted() {
- for (TestJob job : jobs) {
+ void assertAllCopyJobsStarted() {
+ for (TestJob job : copyJobs) {
job.assertStarted();
}
}
+ void assertAllDeleteJobsStarted() {
+ for (TestJob job : deleteJobs) {
+ job.assertStarted();
+ }
+ }
+
+ void assertNoCopyJobsStarted() {
+ for (TestJob job : copyJobs) {
+ job.assertNotStarted();
+ }
+ }
+
+ void assertNoDeleteJobsStarted() {
+ for (TestJob job : deleteJobs) {
+ job.assertNotStarted();
+ }
+ }
+
void assertJobsCreated(int expected) {
- assertEquals(expected, jobs.size());
+ assertEquals(expected, copyJobs.size() + deleteJobs.size());
}
@Override
@@ -205,7 +259,21 @@
}
TestJob job = new TestJob(service, appContext, listener, OPERATION_COPY, id, stack);
- jobs.add(job);
+ copyJobs.add(job);
+ return job;
+ }
+
+ @Override
+ Job createDelete(Context service, Context appContext, Listener listener, String id,
+ DocumentStack stack, List<DocumentInfo> srcs, DocumentInfo srcParent) {
+
+ if (srcs.isEmpty()) {
+ throw new RuntimeException("Empty srcs not supported!");
+ }
+
+ TestJob job = new TestJob(service, appContext, listener, OPERATION_DELETE, id, stack);
+ deleteJobs.add(job);
+
return job;
}
}
diff --git a/tests/src/com/android/documentsui/services/TestJob.java b/tests/src/com/android/documentsui/services/TestJob.java
index 9147a57..c78f582 100644
--- a/tests/src/com/android/documentsui/services/TestJob.java
+++ b/tests/src/com/android/documentsui/services/TestJob.java
@@ -16,6 +16,7 @@
package com.android.documentsui.services;
+import static junit.framework.Assert.assertFalse;
import static junit.framework.Assert.assertTrue;
import android.app.Notification;
@@ -45,6 +46,10 @@
assertTrue(mStarted);
}
+ void assertNotStarted() {
+ assertFalse(mStarted);
+ }
+
void fail(DocumentInfo doc) {
onFileFailed(doc);
}
diff --git a/tests/src/com/android/documentsui/services/TestScheduledExecutorService.java b/tests/src/com/android/documentsui/services/TestScheduledExecutorService.java
index 4d417cf..7bbe70a 100644
--- a/tests/src/com/android/documentsui/services/TestScheduledExecutorService.java
+++ b/tests/src/com/android/documentsui/services/TestScheduledExecutorService.java
@@ -80,7 +80,7 @@
@Override
public Future<?> submit(Runnable task) {
- throw new UnsupportedOperationException();
+ return schedule(task, 0, TimeUnit.MILLISECONDS);
}
@Override