From edd0575b4604f2e4773a4d0887db98aa091f1280 Mon Sep 17 00:00:00 2001 From: Steve McKay Date: Wed, 21 Oct 2015 14:38:54 -0700 Subject: [PATCH] Don't try to delete files twice. Also, apply delete in reverse order to placate RecyclerView. TODO: Refactor this whole stack...needs some love. Bug: 25091323, 24749296 Change-Id: I84e21c16423048bd50cd988eb1e2a36dc62b9d16 --- .../documentsui/DirectoryFragment.java | 139 +++++++++++------- .../documentsui/MultiSelectManager.java | 20 +++ .../DirectoryFragmentModelTest.java | 45 +----- 3 files changed, 109 insertions(+), 95 deletions(-) diff --git a/packages/DocumentsUI/src/com/android/documentsui/DirectoryFragment.java b/packages/DocumentsUI/src/com/android/documentsui/DirectoryFragment.java index a317ef2ea4319..45a890702c0fa 100644 --- a/packages/DocumentsUI/src/com/android/documentsui/DirectoryFragment.java +++ b/packages/DocumentsUI/src/com/android/documentsui/DirectoryFragment.java @@ -98,10 +98,12 @@ import com.android.documentsui.RecentsProvider.StateColumns; import com.android.documentsui.model.DocumentInfo; import com.android.documentsui.model.DocumentStack; import com.android.documentsui.model.RootInfo; +import com.android.internal.annotations.GuardedBy; import com.google.common.collect.Lists; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -1743,6 +1745,9 @@ public class DirectoryFragment extends Fragment { private Context mContext; private int mCursorCount; private boolean mIsLoading; + @GuardedBy("mPendingDelete") + private Boolean mPendingDelete = false; + @GuardedBy("mPendingDelete") private SparseBooleanArray mMarkedForDeletion = new SparseBooleanArray(); private UpdateListener mUpdateListener; @Nullable private Cursor mCursor; @@ -1787,35 +1792,39 @@ public class DirectoryFragment extends Fragment { } int getItemCount() { - return mCursorCount - mMarkedForDeletion.size(); + synchronized(mPendingDelete) { + return mCursorCount - mMarkedForDeletion.size(); + } } Cursor getItem(int position) { - // Items marked for deletion are masked out of the UI. To do this, for every marked - // item whose position is less than the requested item position, advance the requested - // position by 1. - final int originalPos = position; - final int size = mMarkedForDeletion.size(); - for (int i = 0; i < size; ++i) { - // It'd be more concise, but less efficient, to iterate over positions while calling - // mMarkedForDeletion.get. Instead, iterate over deleted entries. - if (mMarkedForDeletion.keyAt(i) <= position && mMarkedForDeletion.valueAt(i)) { - ++position; + synchronized(mPendingDelete) { + // Items marked for deletion are masked out of the UI. To do this, for every marked + // item whose position is less than the requested item position, advance the requested + // position by 1. + final int originalPos = position; + final int size = mMarkedForDeletion.size(); + for (int i = 0; i < size; ++i) { + // It'd be more concise, but less efficient, to iterate over positions while calling + // mMarkedForDeletion.get. Instead, iterate over deleted entries. + if (mMarkedForDeletion.keyAt(i) <= position && mMarkedForDeletion.valueAt(i)) { + ++position; + } } - } - if (DEBUG && position != originalPos) { - Log.d(TAG, "Item position adjusted for deletion. Original: " + originalPos - + " Adjusted: " + position); - } + if (DEBUG && position != originalPos) { + Log.d(TAG, "Item position adjusted for deletion. Original: " + originalPos + + " Adjusted: " + position); + } - if (position >= mCursorCount) { - throw new IndexOutOfBoundsException("Attempt to retrieve " + position + " of " + - mCursorCount + " items"); - } + if (position >= mCursorCount) { + throw new IndexOutOfBoundsException("Attempt to retrieve " + position + " of " + + mCursorCount + " items"); + } - mCursor.moveToPosition(position); - return mCursor; + mCursor.moveToPosition(position); + return mCursor; + } } private boolean isEmpty() { @@ -1848,17 +1857,19 @@ public class DirectoryFragment extends Fragment { } List getDocumentsMarkedForDeletion() { - final int size = mMarkedForDeletion.size(); - List docs = new ArrayList<>(size); + synchronized (mPendingDelete) { + final int size = mMarkedForDeletion.size(); + List docs = new ArrayList<>(size); - for (int i = 0; i < size; ++i) { - final int position = mMarkedForDeletion.keyAt(i); - checkState(position < mCursorCount); - mCursor.moveToPosition(position); - final DocumentInfo doc = DocumentInfo.fromDirectoryCursor(mCursor); - docs.add(doc); + for (int i = 0; i < size; ++i) { + final int position = mMarkedForDeletion.keyAt(i); + checkState(position < mCursorCount); + mCursor.moveToPosition(position); + final DocumentInfo doc = DocumentInfo.fromDirectoryCursor(mCursor); + docs.add(doc); + } + return docs; } - return docs; } /** @@ -1869,17 +1880,23 @@ public class DirectoryFragment extends Fragment { * @param selected A selection representing the files to delete. */ void markForDeletion(Selection selected) { - // 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()); + 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()); - final int size = selected.size(); - for (int i = 0; i < size; ++i) { - int position = selected.get(i); - if (DEBUG) Log.d(TAG, "Marked position " + position + " for deletion"); - mMarkedForDeletion.append(position, true); - mViewAdapter.notifyItemRemoved(position); + int[] positions = selected.getAll(); + Arrays.sort(positions); + + // Walk backwards through the set, since we're removing positions. + // Otherwise, positions would change after the first modification. + for (int p = positions.length - 1; p >= 0; p--) { + mMarkedForDeletion.append(positions[p], true); + mViewAdapter.notifyItemRemoved(positions[p]); + if (DEBUG) Log.d(TAG, "Scheduled " + positions[p] + " for delete."); + } } } @@ -1888,17 +1905,24 @@ public class DirectoryFragment extends Fragment { * unmarked, and restored in the UI. See {@link #markForDeletion(Selection)}. */ void undoDeletion() { - // Iterate over deleted items, temporarily marking them false in the deletion list, and - // re-adding them to the UI. - final int size = mMarkedForDeletion.size(); - for (int i = 0; i < size; ++i) { - final int position = mMarkedForDeletion.keyAt(i); - mMarkedForDeletion.put(position, false); - mViewAdapter.notifyItemInserted(position); + synchronized (mPendingDelete) { + // Iterate over deleted items, temporarily marking them false in the deletion list, and + // re-adding them to the UI. + final int size = mMarkedForDeletion.size(); + for (int i = 0; i < size; ++i) { + final int position = mMarkedForDeletion.keyAt(i); + mMarkedForDeletion.put(position, false); + mViewAdapter.notifyItemInserted(position); + } + resetDeleteData(); } + } - // Then, clear the deletion list. - mMarkedForDeletion.clear(); + private void resetDeleteData() { + synchronized (mPendingDelete) { + mPendingDelete = false; + mMarkedForDeletion.clear(); + } } /** @@ -1909,9 +1933,16 @@ public class DirectoryFragment extends Fragment { * snackbars) for errors, info, etc. */ void finalizeDeletion(DeletionListener listener) { - final ContentResolver resolver = mContext.getContentResolver(); - DeleteFilesTask task = new DeleteFilesTask(resolver, listener); - task.execute(); + synchronized (mPendingDelete) { + if (mPendingDelete) { + // Necessary to avoid b/25072545. Even when that's resolved, this + // is a nice safe thing to day. + mPendingDelete = false; + final ContentResolver resolver = mContext.getContentResolver(); + DeleteFilesTask task = new DeleteFilesTask(resolver, listener); + task.execute(); + } + } } /** @@ -1968,7 +1999,7 @@ public class DirectoryFragment extends Fragment { } else { if (DEBUG) Log.d(TAG, "Deletion task completed successfully."); } - mMarkedForDeletion.clear(); + resetDeleteData(); mListener.onCompletion(); } diff --git a/packages/DocumentsUI/src/com/android/documentsui/MultiSelectManager.java b/packages/DocumentsUI/src/com/android/documentsui/MultiSelectManager.java index b9c4da11ca09b..ef53d53c2f784 100644 --- a/packages/DocumentsUI/src/com/android/documentsui/MultiSelectManager.java +++ b/packages/DocumentsUI/src/com/android/documentsui/MultiSelectManager.java @@ -601,6 +601,14 @@ public final class MultiSelectManager implements View.OnKeyListener { mTotalSelection = new SparseBooleanArray(); } + @VisibleForTesting + public Selection(int... positions) { + this(); + for (int i = 0; i < positions.length; i++) { + add(positions[i]); + } + } + /** * @param position * @return true if the position is currently selected. @@ -623,6 +631,18 @@ public final class MultiSelectManager implements View.OnKeyListener { return mTotalSelection.keyAt(index); } + /** + * Returns an unordered array of selected positions. + */ + public int[] getAll() { + final int size = size(); + int[] positions = new int[size]; + for (int i = 0; i < size; i++) { + positions[i] = get(i); + } + return positions; + } + /** * @return size of the selection. */ diff --git a/packages/DocumentsUI/tests/src/com/android/documentsui/DirectoryFragmentModelTest.java b/packages/DocumentsUI/tests/src/com/android/documentsui/DirectoryFragmentModelTest.java index ace9e275c7766..36d880a068920 100644 --- a/packages/DocumentsUI/tests/src/com/android/documentsui/DirectoryFragmentModelTest.java +++ b/packages/DocumentsUI/tests/src/com/android/documentsui/DirectoryFragmentModelTest.java @@ -32,10 +32,6 @@ import com.android.documentsui.MultiSelectManager.Selection; import com.android.documentsui.model.DocumentInfo; import java.util.List; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; - - public class DirectoryFragmentModelTest extends AndroidTestCase { @@ -117,15 +113,6 @@ public class DirectoryFragmentModelTest extends AndroidTestCase { assertEquals("0", docs.get(0).documentId); assertEquals("1", docs.get(1).documentId); assertEquals("4", docs.get(2).documentId); - - TestDeletionListener testListener = new TestDeletionListener(); - model.finalizeDeletion(testListener); - testListener.waitForDone(); - - docs = getDocumentInfo(0, 1, 2); - assertEquals("0", docs.get(0).documentId); - assertEquals("1", docs.get(1).documentId); - assertEquals("2", docs.get(2).documentId); } // Tests that Model.getItem returns the right items after a deletion is undone. @@ -149,20 +136,12 @@ public class DirectoryFragmentModelTest extends AndroidTestCase { }; } - private void delete(int... items) { - Selection sel = new Selection(); - for (int item: items) { - sel.add(item); - } - model.markForDeletion(sel); + private void delete(int... positions) { + model.markForDeletion(new Selection(positions)); } - private List getDocumentInfo(int... items) { - Selection sel = new Selection(); - for (int item: items) { - sel.add(item); - } - return model.getDocuments(sel); + private List getDocumentInfo(int... positions) { + return model.getDocuments(new Selection(positions)); } private static class DummyListener extends Model.UpdateListener { @@ -177,20 +156,4 @@ public class DirectoryFragmentModelTest extends AndroidTestCase { return null; } } - - private static class TestDeletionListener extends Model.DeletionListener { - final CountDownLatch mSignal = new CountDownLatch(1); - - @Override - public void onCompletion() { - mSignal.countDown(); - } - - public void waitForDone() { - try { - boolean timeout = mSignal.await(10, TimeUnit.SECONDS); - assertTrue("Timed out waiting for deletion completion", timeout); - } catch (InterruptedException e) {} - } - } }