Fix file deletion after the move to Model IDs. (da858bfc) · Commits · e / os / android_packages_apps_DocumentsUI

src/com/android/documentsui/dirlist/DirectoryFragment.java

+58 −12

Original line number	Diff line number	Diff line
		@@ -116,10 +116,12 @@ import com.android.documentsui.model.DocumentInfo;
		import com.android.documentsui.model.DocumentStack;
		import com.android.documentsui.model.RootInfo;
		import com.google.common.collect.Lists;
		import com.google.common.collect.Sets;

		import java.util.ArrayList;
		import java.util.Collections;
		import java.util.List;
		import java.util.Set;

		/**
		* Display the documents inside a single directory.
		@@ -844,8 +846,11 @@ public class DirectoryFragment extends Fragment {
		Context context = getActivity();
		String message = Shared.getQuantityString(context, R.plurals.deleting, selected.size());

		mModel.markForDeletion(selected);
		// Hide the files in the UI.
		final SparseArray<String> toDelete = mAdapter.hide(selected.getAll());

		// Show a snackbar informing the user that files will be deleted, and give them an option to
		// cancel.
		final Activity activity = getActivity();
		Snackbars.makeSnackbar(activity, message, Snackbar.LENGTH_LONG)
		.setAction(
		@@ -859,9 +864,12 @@ public class DirectoryFragment extends Fragment {
		@Override
		public void onDismissed(Snackbar snackbar, int event) {
		if (event == Snackbar.Callback.DISMISS_EVENT_ACTION) {
		mModel.undoDeletion();
		// If the delete was cancelled, just unhide the files.
		mAdapter.unhide(toDelete);
		} else {
		mModel.finalizeDeletion(
		// Actually kick off the delete.
		mModel.delete(
		selected,
		new Model.DeletionListener() {
		@Override
		public void onError() {
		@@ -1049,7 +1057,6 @@ public class DirectoryFragment extends Fragment {

		@Override
		public void onBindViewHolder(DocumentHolder holder, int position) {

		final Context context = getContext();
		final State state = getDisplayState();
		final RootsCache roots = DocumentsApplication.getRootsCache(context);
		@@ -1239,6 +1246,45 @@ public class DirectoryFragment extends Fragment {
		return mModelIds.get(adapterPosition);
		}

		/**
		* 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.
		*/
		public SparseArray<String> hide(String... 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)) {
		hiddenItems.put(i, mModelIds.remove(i));
		notifyItemRemoved(i);
		}
		}

		return hiddenItems;
		}

		/**
		* Unhides a set of previously hidden items.
		*
		* @param ids A sparse array of IDs from a previous call to {@link #hide}.
		*/
		public void unhide(SparseArray<String> ids) {
		// Proceed backwards through the list of items, because each addition causes the
		// positions of all subsequent items to change.
		for (int i = ids.size() - 1; i >= 0; --i) {
		int pos = ids.keyAt(i);
		String id = ids.get(pos);
		mModelIds.add(pos, id);
		notifyItemInserted(pos);
		}
		}
		}

		private static String formatTime(Context context, long when) {

src/com/android/documentsui/dirlist/Model.java

+20 −114

Original line number	Diff line number	Diff line
		@@ -19,7 +19,6 @@ package com.android.documentsui.dirlist;
		import static com.android.documentsui.Shared.DEBUG;
		import static com.android.documentsui.model.DocumentInfo.getCursorString;
		import static com.android.internal.util.Preconditions.checkNotNull;
		import static com.android.internal.util.Preconditions.checkState;

		import android.content.ContentProviderClient;
		import android.content.ContentResolver;
		@@ -34,7 +33,6 @@ import android.support.annotation.Nullable;
		import android.support.annotation.VisibleForTesting;
		import android.support.v7.widget.RecyclerView;
		import android.util.Log;
		import android.util.SparseArray;

		import com.android.documentsui.BaseActivity.DocumentContext;
		import com.android.documentsui.DirectoryResult;
		@@ -42,11 +40,9 @@ import com.android.documentsui.DocumentsApplication;
		import com.android.documentsui.RootCursorWrapper;
		import com.android.documentsui.dirlist.MultiSelectManager.Selection;
		import com.android.documentsui.model.DocumentInfo;
		import com.android.internal.annotations.GuardedBy;

		import java.util.ArrayList;
		import java.util.HashMap;
		import java.util.HashSet;
		import java.util.List;
		import java.util.Set;

		@@ -56,14 +52,9 @@ import java.util.Set;
		@VisibleForTesting
		public class Model implements DocumentContext {
		private static final String TAG = "Model";
		private RecyclerView.Adapter<?> mViewAdapter;
		private Context mContext;
		private int mCursorCount;
		private boolean mIsLoading;
		@GuardedBy("mPendingDelete")
		private Boolean mPendingDelete = false;
		@GuardedBy("mPendingDelete")
		private Set<String> mMarkedForDeletion = new HashSet<>();
		private List<UpdateListener> mUpdateListeners = new ArrayList<>();
		@Nullable private Cursor mCursor;
		@Nullable String info;
		@@ -72,7 +63,6 @@ public class Model implements DocumentContext {

		Model(Context context, RecyclerView.Adapter<?> viewAdapter) {
		mContext = context;
		mViewAdapter = viewAdapter;
		}

		/**
		@@ -142,9 +132,7 @@ public class Model implements DocumentContext {

		@VisibleForTesting
		int getItemCount() {
		synchronized(mPendingDelete) {
		return mCursorCount - mMarkedForDeletion.size();
		}
		return mCursorCount;
		}

		/**
		@@ -197,107 +185,24 @@ public class Model implements DocumentContext {
		return mCursor;
		}

		List<DocumentInfo> getDocumentsMarkedForDeletion() {
		// TODO(stable-id): This could be just a plain old selection now.
		synchronized (mPendingDelete) {
		final int size = mMarkedForDeletion.size();
		List<DocumentInfo> docs = new ArrayList<>(size);

		for (String id: mMarkedForDeletion) {
		Integer position = mPositions.get(id);
		checkState(position != null);
		mCursor.moveToPosition(position);
		final DocumentInfo doc = DocumentInfo.fromDirectoryCursor(mCursor);
		docs.add(doc);
		}
		return docs;
		}
		}

		/**
		* Marks the given files for deletion. This will remove them from the UI. Clients must then
		* call either {@link #undoDeletion()} or {@link #finalizeDeletion()} to cancel or confirm
		* the deletion, respectively. Only one deletion operation is allowed at a time.
		*
		* @param selected A selection representing the files to delete.
		*/
		void markForDeletion(Selection selected) {
		synchronized (mPendingDelete) {
		mPendingDelete = true;
		// Only one deletion operation at a time.
		checkState(mMarkedForDeletion.size() == 0);
		// There should never be more to delete than what exists.
		checkState(mCursorCount >= selected.size());

		// Adapter notifications must be sent in reverse order of adapter position. This is
		// because each removal causes subsequent item adapter positions to change.
		SparseArray<String> ids = new SparseArray<>();
		for (int i = ids.size() - 1; i >= 0; i--) {
		int pos = ids.keyAt(i);
		mMarkedForDeletion.add(ids.get(pos));
		mViewAdapter.notifyItemRemoved(pos);
		if (DEBUG) Log.d(TAG, "Scheduled " + pos + " for delete.");
		}
		}
		}

		/**
		* Cancels an ongoing deletion operation. All files currently marked for deletion will be
		* unmarked, and restored in the UI. See {@link #markForDeletion(Selection)}.
		*/
		void undoDeletion() {
		synchronized (mPendingDelete) {
		// Iterate over deleted items, temporarily marking them false in the deletion list, and
		// re-adding them to the UI.
		for (String id: mMarkedForDeletion) {
		Integer pos= mPositions.get(id);
		checkNotNull(pos);
		mMarkedForDeletion.remove(id);
		mViewAdapter.notifyItemInserted(pos);
		}
		resetDeleteData();
		}
		}

		private void resetDeleteData() {
		synchronized (mPendingDelete) {
		mPendingDelete = false;
		mMarkedForDeletion.clear();
		}
		}

		/**
		* Finalizes an ongoing deletion operation. All files currently marked for deletion will be
		* deleted. See {@link #markForDeletion(Selection)}.
		*
		* @param view The view which will be used to interact with the user (e.g. surfacing
		* snackbars) for errors, info, etc.
		*/
		void finalizeDeletion(DeletionListener listener) {
		synchronized (mPendingDelete) {
		if (mPendingDelete) {
		// Necessary to avoid b/25072545. Even when that's resolved, this
		// is a nice safe thing to day.
		mPendingDelete = false;
		public void delete(Selection selected, DeletionListener listener) {
		final ContentResolver resolver = mContext.getContentResolver();
		DeleteFilesTask task = new DeleteFilesTask(resolver, listener);
		task.execute();
		}
		}
		new DeleteFilesTask(resolver, listener).execute(selected);
		}

		/**
		* A Task which collects the DocumentInfo for documents that have been marked for deletion,
		* and actually deletes them.
		*/
		private class DeleteFilesTask extends AsyncTask<Void, Void, List<DocumentInfo>> {
		private class DeleteFilesTask extends AsyncTask<Selection, Void, Void> {
		private ContentResolver mResolver;
		private DeletionListener mListener;
		private boolean mHadTrouble;

		/**
		* @param resolver A ContentResolver for performing the actual file deletions.
		* @param errorCallback A Runnable that is executed in the event that one or more errors
		* occured while copying files. Execution will occur on the UI thread.
		* occurred while copying files. Execution will occur on the UI thread.
		*/
		public DeleteFilesTask(ContentResolver resolver, DeletionListener listener) {
		mResolver = resolver;
		@@ -305,17 +210,14 @@ public class Model implements DocumentContext {
		}

		@Override
		protected List<DocumentInfo> doInBackground(Void... params) {
		return getDocumentsMarkedForDeletion();
		}
		protected Void doInBackground(Selection... selected) {
		List<DocumentInfo> toDelete = getDocuments(selected[0]);
		mHadTrouble = false;

		@Override
		protected void onPostExecute(List<DocumentInfo> docs) {
		boolean hadTrouble = false;
		for (DocumentInfo doc : docs) {
		for (DocumentInfo doc : toDelete) {
		if (!doc.isDeleteSupported()) {
		Log.w(TAG, doc + " could not be deleted. Skipping...");
		hadTrouble = true;
		mHadTrouble = true;
		continue;
		}

		@@ -326,21 +228,25 @@ public class Model implements DocumentContext {
		mResolver, doc.derivedUri.getAuthority());
		DocumentsContract.deleteDocument(client, doc.derivedUri);
		} catch (Exception e) {
		Log.w(TAG, "Failed to delete " + doc);
		hadTrouble = true;
		Log.w(TAG, "Failed to delete " + doc, e);
		mHadTrouble = true;
		} finally {
		ContentProviderClient.releaseQuietly(client);
		}
		}

		if (hadTrouble) {
		return null;
		}

		@Override
		protected void onPostExecute(Void _) {
		if (mHadTrouble) {
		// TODO show which files failed? b/23720103
		mListener.onError();
		if (DEBUG) Log.d(TAG, "Deletion task completed. Some deletions failed.");
		} else {
		if (DEBUG) Log.d(TAG, "Deletion task completed successfully.");
		}
		resetDeleteData();

		mListener.onCompletion();
		}

src/com/android/documentsui/dirlist/MultiSelectManager.java

+13 −1

Original line number	Diff line number	Diff line
		@@ -140,10 +140,21 @@ public final class MultiSelectManager implements View.OnKeyListener {
		mEnvironment.registerDataObserver(
		new RecyclerView.AdapterDataObserver() {

		private List<String> mModelIds = new ArrayList<>();

		@Override
		public void onChanged() {
		// TODO(stable-id): This is causing b/22765812
		mSelection.clear();

		// TODO(stable-id): Improve this. It's currently super-inefficient,
		// performing a bunch of lookups and inserting into a List. Maybe just add
		// another method to the SelectionEnvironment to just grab the whole list at
		// once.
		mModelIds.clear();
		for (int i = 0; i < mEnvironment.getItemCount(); ++i) {
		mModelIds.add(mEnvironment.getModelIdFromAdapterPosition(i));
		}
		}

		@Override
		@@ -165,8 +176,9 @@ public final class MultiSelectManager implements View.OnKeyListener {
		int endPosition = startPosition + itemCount;
		// Remove any disappeared IDs from the selection.
		for (int i = startPosition; i < endPosition; i++) {
		String id = mEnvironment.getModelIdFromAdapterPosition(i);
		String id = mModelIds.get(i);
		mSelection.remove(id);
		mModelIds.remove(i);
		}
		}

tests/src/com/android/documentsui/dirlist/DirectoryFragmentModelTest.java→tests/src/com/android/documentsui/dirlist/ModelTest.java

+210 −0

Original line number	Diff line number	Diff line
		@@ -16,53 +16,58 @@

		package com.android.documentsui.dirlist;

		import static android.test.MoreAsserts.assertNotEqual;

		import android.content.ContentResolver;
		import android.content.Context;
		import android.content.ContextWrapper;
		import android.database.Cursor;
		import android.database.MatrixCursor;
		import android.net.Uri;
		import android.os.Bundle;
		import android.provider.DocumentsContract;
		import android.provider.DocumentsContract.Document;
		import android.support.v7.widget.RecyclerView;
		import android.test.AndroidTestCase;
		import android.test.mock.MockContentProvider;
		import android.test.mock.MockContentResolver;
		import android.test.suitebuilder.annotation.SmallTest;
		import android.view.ViewGroup;

		import com.android.documentsui.DirectoryResult;
		import com.android.documentsui.RootCursorWrapper;
		import com.android.documentsui.dirlist.MultiSelectManager.Selection;
		import com.android.documentsui.model.DocumentInfo;

		import java.util.ArrayList;
		import java.util.List;
		import java.util.concurrent.CountDownLatch;

		@SmallTest
		public class DirectoryFragmentModelTest extends AndroidTestCase {
		public class ModelTest extends AndroidTestCase {

		// Item count must be an even number (see setUp below)
		private static final int ITEM_COUNT = 10;
		private static final String AUTHORITY = "test_authority";
		private static final String[] COLUMNS = new String[]{
		RootCursorWrapper.COLUMN_AUTHORITY,
		Document.COLUMN_DOCUMENT_ID
		Document.COLUMN_DOCUMENT_ID,
		Document.COLUMN_FLAGS
		};
		private static Cursor cursor;

		private Context mContext;
		private Context context;
		private Model model;
		private TestContentProvider provider;

		public void setUp() {
		setupTestContext();

		// Make two sets of documents under two different authorities but with identical document
		// IDs.
		MatrixCursor c = new MatrixCursor(COLUMNS);
		for (int i = 0; i < ITEM_COUNT/2; ++i) {
		MatrixCursor.RowBuilder row0 = c.newRow();
		row0.add(RootCursorWrapper.COLUMN_AUTHORITY, "authority0");
		row0.add(Document.COLUMN_DOCUMENT_ID, Integer.toString(i));

		MatrixCursor.RowBuilder row1 = c.newRow();
		row1.add(RootCursorWrapper.COLUMN_AUTHORITY, "authority1");
		row1.add(Document.COLUMN_DOCUMENT_ID, Integer.toString(i));
		for (int i = 0; i < ITEM_COUNT; ++i) {
		MatrixCursor.RowBuilder row = c.newRow();
		row.add(RootCursorWrapper.COLUMN_AUTHORITY, AUTHORITY);
		row.add(Document.COLUMN_DOCUMENT_ID, Integer.toString(i));
		row.add(Document.COLUMN_FLAGS, Document.FLAG_SUPPORTS_DELETE);
		}
		cursor = c;

		@@ -70,7 +75,7 @@ public class DirectoryFragmentModelTest extends AndroidTestCase {
		r.cursor = cursor;

		// Instantiate the model with a dummy view adapter and listener that (for now) do nothing.
		model = new Model(mContext, new DummyAdapter());
		model = new Model(context, new DummyAdapter());
		model.addUpdateListener(new DummyListener());
		model.update(r);
		}
		@@ -80,32 +85,30 @@ public class DirectoryFragmentModelTest extends AndroidTestCase {
		assertEquals(ITEM_COUNT, model.getItemCount());
		}

		// Tests that the item count is correct after a deletion.
		public void testItemCount_WithDeletion() {
		// Simulate deleting 2 files.
		delete(2, 4);
		// Tests multiple authorities with clashing document IDs.
		public void testModelIdIsUnique() {
		MatrixCursor c0 = new MatrixCursor(COLUMNS);
		MatrixCursor c1 = new MatrixCursor(COLUMNS);

		assertEquals(ITEM_COUNT - 2, model.getItemCount());
		}

		// Tests that the item count is correct after a deletion is undone.
		public void testItemCount_WithUndoneDeletion() {
		// Simulate deleting 2 files.
		delete(0, 3);
		// Make two sets of items with the same IDs, under different authorities.
		final String AUTHORITY0 = "auth0";
		final String AUTHORITY1 = "auth1";
		for (int i = 0; i < ITEM_COUNT; ++i) {
		MatrixCursor.RowBuilder row0 = c0.newRow();
		row0.add(RootCursorWrapper.COLUMN_AUTHORITY, AUTHORITY0);
		row0.add(Document.COLUMN_DOCUMENT_ID, Integer.toString(i));

		// Undo the deletion
		model.undoDeletion();
		assertEquals(ITEM_COUNT, model.getItemCount());
		MatrixCursor.RowBuilder row1 = c1.newRow();
		row1.add(RootCursorWrapper.COLUMN_AUTHORITY, AUTHORITY1);
		row1.add(Document.COLUMN_DOCUMENT_ID, Integer.toString(i));
		}

		// Tests that the right things are marked for deletion.
		public void testMarkForDeletion() {
		delete(1, 3);

		List<DocumentInfo> docs = model.getDocumentsMarkedForDeletion();
		assertEquals(2, docs.size());
		assertEquals("1", docs.get(0).documentId);
		assertEquals("3", docs.get(1).documentId);
		for (int i = 0; i < ITEM_COUNT; ++i) {
		c0.moveToPosition(i);
		c1.moveToPosition(i);
		assertNotEqual(Model.createId(c0), Model.createId(c1));
		}
		}

		// Tests the base case for Model.getItem.
		@@ -117,45 +120,59 @@ public class DirectoryFragmentModelTest extends AndroidTestCase {
		}
		}

		// Tests that Model.getItem returns the right items after a deletion.
		public void testGetItem_WithDeletion() {
		// Tests that Model.delete works correctly.
		public void testDelete() throws Exception {
		// Simulate deleting 2 files.
		List<DocumentInfo> docsBefore = getDocumentInfo(2, 3);
		delete(2, 3);

		List<DocumentInfo> docs = getDocumentInfo(0, 1, 2);
		assertEquals("0", docs.get(0).documentId);
		assertEquals("1", docs.get(1).documentId);
		assertEquals("4", docs.get(2).documentId);
		}

		// Tests that Model.getItem returns the right items after a deletion is undone.
		public void testGetItem_WithCancelledDeletion() {
		delete(0, 1);
		model.undoDeletion();

		// Test that all documents are accounted for, in the right position.
		for (int i = 0; i < ITEM_COUNT; ++i) {
		assertEquals(Integer.toString(i), getDocumentInfo(i).get(0).documentId);
		}
		provider.assertWasDeleted(docsBefore.get(0));
		provider.assertWasDeleted(docsBefore.get(1));
		}

		private void setupTestContext() {
		final MockContentResolver resolver = new MockContentResolver();
		mContext = new ContextWrapper(getContext()) {
		context = new ContextWrapper(getContext()) {
		@Override
		public ContentResolver getContentResolver() {
		return resolver;
		}
		};
		provider = new TestContentProvider();
		resolver.addProvider(AUTHORITY, provider);
		}

		private Selection positionToSelection(int... positions) {
		Selection s = new Selection();
		// Construct a selection of the given positions.
		for (int p: positions) {
		cursor.moveToPosition(p);
		s.add(Model.createId(cursor));
		}
		return s;
		}

		private void delete(int... positions) {
		// model.markForDeletion(new Selection(positions));
		private void delete(int... positions) throws InterruptedException {
		Selection s = positionToSelection(positions);
		final CountDownLatch latch = new CountDownLatch(1);

		model.delete(
		s,
		new Model.DeletionListener() {
		@Override
		public void onError() {
		latch.countDown();
		}
		@Override
		void onCompletion() {
		latch.countDown();
		}
		});
		latch.await();
		}

		private List<DocumentInfo> getDocumentInfo(int... positions) {
		// return model.getDocuments(new Selection(positions));
		return new ArrayList<>();
		return model.getDocuments(positionToSelection(positions));
		}

		private static class DummyListener implements Model.UpdateListener {
		@@ -170,4 +187,24 @@ public class DirectoryFragmentModelTest extends AndroidTestCase {
		return null;
		}
		}

		private static class TestContentProvider extends MockContentProvider {
		List<Uri> mDeleted = new ArrayList<>();

		@Override
		public Bundle call(String method, String arg, Bundle extras) {
		// Intercept and log delete method calls.
		if (DocumentsContract.METHOD_DELETE_DOCUMENT.equals(method)) {
		final Uri documentUri = extras.getParcelable(DocumentsContract.EXTRA_URI);
		mDeleted.add(documentUri);
		return new Bundle();
		} else {
		return super.call(method, arg, extras);
		}
		}

		public void assertWasDeleted(DocumentInfo doc) {
		assertTrue(mDeleted.contains(doc.derivedUri));
		}
		}
		}