Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 99f1dc3d authored by Steve McKay's avatar Steve McKay
Browse files

Improve error handling. 

Recover gracefully when a uri cannot be resolved.
Include Uris in information presented in error dialog.
Update DeleteJob and CopyJob to have shared uri resolution code.
Break out error handling to provide clearer and more granular error info.
Update Metrics to be sure we including error information about URIs we failed to resolve.

Bug: 33938336
Test: New coverage in JobErrorHandlingTest.

Change-Id: Idf34882a561ec5cb90170f291683bdc752188b57
parent 1dfad7f2

src/com/android/documentsui/Metrics.java

+35 −14
Original line number Diff line number Diff line
@@ -392,7 +392,9 @@ public final class Metrics { 
            @OpType int operationType,

            List<DocumentInfo> srcs,

            @Nullable DocumentInfo dst) {

        ProviderCounts counts = countProviders(srcs, dst);



        ProviderCounts counts = new ProviderCounts();

        countProviders(counts, srcs, dst);



        if (counts.intraProvider > 0) {

            logIntraProviderFileOps(context, dst.authority, operationType);
@@ -438,8 +440,13 @@ public final class Metrics { 
     * @param failedFiles

     */

    public static void logFileOperationErrors(Context context, @OpType int operationType,

            List<DocumentInfo> failedFiles) {

        ProviderCounts counts = countProviders(failedFiles, null);

            List<DocumentInfo> failedFiles, List<Uri> failedUris) {



        ProviderCounts counts = new ProviderCounts();

        countProviders(counts, failedFiles, null);



        // TODO: Report URI errors separate from file operation errors.

        countProviders(counts, failedUris);



        @FileOp int opCode = FILEOP_OTHER_ERROR;

        switch (operationType) {
@@ -826,20 +833,34 @@ public final class Metrics { 
     * the dst document (if a dst is provided), how many come from system providers, and how many

     * come from external 3rd-party providers.

     */

    private static ProviderCounts countProviders(

            List<DocumentInfo> srcs, @Nullable DocumentInfo dst) {

        ProviderCounts counts = new ProviderCounts();

    private static void countProviders(

            ProviderCounts counts, List<DocumentInfo> srcs, @Nullable DocumentInfo dst) {

        for (DocumentInfo doc: srcs) {

            if (dst != null && doc.authority.equals(dst.authority)) {

            countForAuthority(counts, doc.authority, dst);

        }

    }



    /**

     * Count the given uris and provide a tally of how many come from the same provider as

     * the dst document (if a dst is provided), how many come from system providers, and how many

     * come from external 3rd-party providers.

     */

    private static void countProviders(ProviderCounts counts, List<Uri> uris) {

        for (Uri uri: uris) {

            countForAuthority(counts, uri.getAuthority(), null);

        }

    }



    private static void countForAuthority(

            ProviderCounts counts, String authority, @Nullable DocumentInfo dst) {

        if (dst != null && authority.equals(dst.authority)) {

            counts.intraProvider++;

            } else if (isSystemProvider(doc.authority)){

        } else if (isSystemProvider(authority)){

            counts.systemProvider++;

        } else {

            counts.externalProvider++;

        }

    }

        return counts;

    }



    private static class ProviderCounts {

        int intraProvider;

src/com/android/documentsui/OperationDialogFragment.java

+20 −9
Original line number Diff line number Diff line
@@ -17,13 +17,14 @@ 
package com.android.documentsui;



import android.annotation.IntDef;

import android.app.AlertDialog;

import android.app.Dialog;

import android.app.DialogFragment;

import android.app.FragmentManager;

import android.app.FragmentTransaction;

import android.content.DialogInterface;

import android.net.Uri;

import android.os.Bundle;

import android.support.v7.app.AlertDialog;

import android.text.Html;



import com.android.documentsui.base.DocumentInfo;
@@ -34,7 +35,6 @@ import com.android.documentsui.services.FileOperationService.OpType; 
import java.lang.annotation.Retention;

import java.lang.annotation.RetentionPolicy;

import java.util.ArrayList;



/**

 * Alert dialog for operation dialogs.

 */
@@ -55,13 +55,18 @@ public class OperationDialogFragment extends DialogFragment { 


    private static final String TAG = "OperationDialogFragment";



    public static void show(FragmentManager fm, @DialogType int dialogType,

            ArrayList<DocumentInfo> failedSrcList, DocumentStack dstStack,

    public static void show(

            FragmentManager fm,

            @DialogType int dialogType,

            ArrayList<DocumentInfo> failedSrcList,

            ArrayList<DocumentInfo> uriList,

            DocumentStack dstStack,

            @OpType int operationType) {



        final Bundle args = new Bundle();

        args.putInt(FileOperationService.EXTRA_DIALOG_TYPE, dialogType);

        args.putInt(FileOperationService.EXTRA_OPERATION_TYPE, operationType);

        args.putParcelableArrayList(FileOperationService.EXTRA_SRC_LIST, failedSrcList);

        args.putParcelableArrayList(FileOperationService.EXTRA_FAILED_DOCS, failedSrcList);



        final FragmentTransaction ft = fm.beginTransaction();

        final OperationDialogFragment fragment = new OperationDialogFragment();
@@ -79,8 +84,10 @@ public class OperationDialogFragment extends DialogFragment { 
              getArguments().getInt(FileOperationService.EXTRA_DIALOG_TYPE);

        final @OpType int operationType =

              getArguments().getInt(FileOperationService.EXTRA_OPERATION_TYPE);

        final ArrayList<DocumentInfo> srcList = getArguments().getParcelableArrayList(

                FileOperationService.EXTRA_SRC_LIST);

        final ArrayList<Uri> uriList = getArguments().getParcelableArrayList(

                FileOperationService.EXTRA_FAILED_URIS);

        final ArrayList<DocumentInfo> docList = getArguments().getParcelableArrayList(

                FileOperationService.EXTRA_FAILED_DOCS);



        final AlertDialog.Builder builder = new AlertDialog.Builder(getActivity());

        String messageFormat;
@@ -111,10 +118,14 @@ public class OperationDialogFragment extends DialogFragment { 
        }



        final StringBuilder list = new StringBuilder("<p>");

        for (DocumentInfo documentInfo : srcList) {

            list.append(String.format("&#8226; %s<br>", Html.escapeHtml(documentInfo.displayName)));

        for (DocumentInfo documentInfo : docList) {

            list.append("&#8226; " + Html.escapeHtml(documentInfo.displayName) + "<br>");

        }

        for (Uri uri : uriList) {

            list.append("&#8226; " + uri.toSafeString() + "<br>");

        }

        list.append("</p>");



        builder.setMessage(Html.fromHtml(String.format(messageFormat, list.toString())));

        builder.setPositiveButton(

                R.string.close,

src/com/android/documentsui/files/FilesActivity.java

+6 −3
Original line number Diff line number Diff line
@@ -154,12 +154,15 @@ public class FilesActivity extends BaseActivity implements ActionHandler.Addons 
            final int opType = intent.getIntExtra(

                    FileOperationService.EXTRA_OPERATION_TYPE,

                    FileOperationService.OPERATION_COPY);

            final ArrayList<DocumentInfo> srcList =

                    intent.getParcelableArrayListExtra(FileOperationService.EXTRA_SRC_LIST);

            final ArrayList<DocumentInfo> docList =

                    intent.getParcelableArrayListExtra(FileOperationService.EXTRA_FAILED_DOCS);

            final ArrayList<DocumentInfo> uriList =

                    intent.getParcelableArrayListExtra(FileOperationService.EXTRA_FAILED_URIS);

            OperationDialogFragment.show(

                    getFragmentManager(),

                    dialogType,

                    srcList,

                    docList,

                    uriList,

                    mState.stack,

                    opType);

        }

src/com/android/documentsui/services/CopyJob.java

+25 −61
Original line number Diff line number Diff line
@@ -35,7 +35,6 @@ import android.app.Notification; 
import android.app.Notification.Builder;

import android.app.PendingIntent;

import android.content.ContentProviderClient;

import android.content.ContentResolver;

import android.content.Context;

import android.content.Intent;

import android.content.res.AssetFileDescriptor;
@@ -67,18 +66,16 @@ import java.io.IOException; 
import java.io.InputStream;

import java.text.NumberFormat;

import java.util.ArrayList;

import java.util.List;



class CopyJob extends Job {

class CopyJob extends ResolvedResourcesJob {



    private static final String TAG = "CopyJob";



    final List<DocumentInfo> mSrcs;

    final ArrayList<DocumentInfo> convertedFiles = new ArrayList<>();



    private long mStartTime = -1;



    private long mBatchSize;

    private long mBytesRequired;

    private volatile long mBytesCopied;

    // Speed estimation

    private long mBytesCopiedSample;
@@ -99,9 +96,6 @@ class CopyJob extends Job { 
        super(service, listener, id, opType, destination, srcs);



        assert(srcs.getItemCount() > 0);



        // delay the initialization of it to setUp() because it may be IO extensive.

        mSrcs = new ArrayList<>(srcs.getItemCount());

    }



    @Override
@@ -121,8 +115,8 @@ class CopyJob extends Job { 
    Notification getProgressNotification(@StringRes int msgId) {

        updateRemainingTimeEstimate();



        if (mBatchSize >= 0) {

            double completed = (double) this.mBytesCopied / mBatchSize;

        if (mBytesRequired >= 0) {

            double completed = (double) this.mBytesCopied / mBytesRequired;

            mProgressBuilder.setProgress(100, (int) (completed * 100), false);

            mProgressBuilder.setSubText(

                    NumberFormat.getPercentInstance().format(completed));
@@ -171,7 +165,7 @@ class CopyJob extends Job { 
        }



        if (mSampleTime > 0 && mSpeed > 0) {

            mRemainingTime = ((mBatchSize - bytesCopied) * 1000) / mSpeed;

            mRemainingTime = ((mBytesRequired - bytesCopied) * 1000) / mSpeed;

        } else {

            mRemainingTime = 0;

        }
@@ -211,10 +205,7 @@ class CopyJob extends Job { 


    @Override

    boolean setUp() {

        try {

            buildDocumentList();

        } catch (ResourceException e) {

            Log.e(TAG, "Failed to get the list of docs.", e);

        if (!super.setUp()) {

            return false;

        }
@@ -224,10 +215,10 @@ class CopyJob extends Job { 
        }



        try {

            mBatchSize = calculateSize(mSrcs);

            mBytesRequired = calculateBytesRequired();

        } catch (ResourceException e) {

            Log.w(TAG, "Failed to calculate total size. Copying without progress.", e);

            mBatchSize = -1;

            mBytesRequired = -1;

        }



        // Check if user has canceled this task. We should check it again here as user cancels
@@ -246,8 +237,8 @@ class CopyJob extends Job { 
        mStartTime = elapsedRealtime();

        DocumentInfo srcInfo;

        DocumentInfo dstInfo = stack.peek();

        for (int i = 0; i < mSrcs.size() && !isCanceled(); ++i) {

            srcInfo = mSrcs.get(i);

        for (int i = 0; i < mResolvedDocs.size() && !isCanceled(); ++i) {

            srcInfo = mResolvedDocs.get(i);



            if (DEBUG) Log.d(TAG,

                    "Copying " + srcInfo.displayName + " (" + srcInfo.derivedUri + ")"
@@ -265,39 +256,12 @@ class CopyJob extends Job { 
                onFileFailed(srcInfo);

            }

        }

        Metrics.logFileOperation(service, operationType, mSrcs, dstInfo);

    }



    private void buildDocumentList() throws ResourceException {

        try {

            final ContentResolver resolver = appContext.getContentResolver();

            final Iterable<Uri> uris = srcs.getUris(appContext);



            int docProcessed = 0;

            for (Uri uri : uris) {

                DocumentInfo doc = DocumentInfo.fromUri(resolver, uri);

                if (canCopy(doc, stack.getRoot())) {

                    mSrcs.add(doc);

                } else {

                    onFileFailed(doc);

        Metrics.logFileOperation(service, operationType, mResolvedDocs, dstInfo);

    }

                ++docProcessed;



                if (isCanceled()) {

                    return;

                }

            }



            // If docProcessed is different than the count claimed by UrisSupplier, add the number

            // to failedFileCount.

            failedFileCount += (srcs.getItemCount() - docProcessed);

        } catch(IOException e) {

            failedFileCount += srcs.getItemCount();

            throw new ResourceException("Failed to open the list of docs to copy.", e);

        }

    }



    private static boolean canCopy(DocumentInfo doc, RootInfo root) {

    @Override

    boolean isEligibleDoc(DocumentInfo doc, RootInfo root) {

        // Can't copy folders to downloads, because we don't show folders there.

        return !root.isDownloads() || !doc.isDirectory();

    }
@@ -307,7 +271,7 @@ class CopyJob extends Job { 
     * @return true if the root has enough space or doesn't provide free space info; otherwise false

     */

    boolean checkSpace() {

        return checkSpace(mBatchSize);

        return verifySpaceAvailable(mBytesRequired);

    }



    /**
@@ -315,10 +279,10 @@ class CopyJob extends Job { 
     * @param batchSize the total size of files

     * @return true if the root has enough space or doesn't provide free space info; otherwise false

     */

    final boolean checkSpace(long batchSize) {

    final boolean verifySpaceAvailable(long batchSize) {

        // Default to be true because if batchSize or available space is invalid, we still let the

        // copy start anyway.

        boolean result = true;

        boolean available = true;

        if (batchSize >= 0) {

            RootsCache cache = DocumentsApplication.getRootsCache(appContext);
@@ -327,18 +291,18 @@ class CopyJob extends Job { 
            // stale.

            root = cache.getRootOneshot(root.authority, root.rootId, true);

            if (root.availableBytes >= 0) {

                result = (batchSize <= root.availableBytes);

                available = (batchSize <= root.availableBytes);

            } else {

                Log.w(TAG, root.toString() + " doesn't provide available bytes.");

            }

        }



        if (!result) {

            failedFileCount += mSrcs.size();

            failedFiles.addAll(mSrcs);

        if (!available) {

            failureCount = mResolvedDocs.size();

            failedDocs.addAll(mResolvedDocs);

        }



        return result;

        return available;

    }



    @Override
@@ -626,14 +590,13 @@ class CopyJob extends Job { 
     * Calculates the cumulative size of all the documents in the list. Directories are recursed

     * into and totaled up.

     *

     * @param srcs

     * @return Size in bytes.

     * @throws ResourceException

     */

    private long calculateSize(List<DocumentInfo> srcs) throws ResourceException {

    private long calculateBytesRequired() throws ResourceException {

        long result = 0;



        for (DocumentInfo src : srcs) {

        for (DocumentInfo src : mResolvedDocs) {

            if (src.isDirectory()) {

                // Directories need to be recursed into.

                try {
@@ -719,7 +682,8 @@ class CopyJob extends Job { 
                .append("CopyJob")

                .append("{")

                .append("id=" + id)

                .append(", docs=" + srcs)

                .append(", uris=" + mResourceUris)

                .append(", docs=" + mResolvedDocs)

                .append(", destination=" + stack)

                .append("}")

                .toString();

src/com/android/documentsui/services/DeleteJob.java

+37 −45
Original line number Diff line number Diff line
@@ -26,22 +26,21 @@ import android.content.Context; 
import android.net.Uri;

import android.util.Log;



import com.android.documentsui.clipping.UrisSupplier;

import com.android.documentsui.Metrics;

import com.android.documentsui.R;

import com.android.documentsui.base.DocumentInfo;

import com.android.documentsui.base.DocumentStack;

import com.android.documentsui.clipping.UrisSupplier;



import java.io.FileNotFoundException;



import javax.annotation.Nullable;

import java.io.IOException;

import java.util.ArrayList;

import java.util.List;



final class DeleteJob extends Job {

final class DeleteJob extends ResolvedResourcesJob {



    private static final String TAG = "DeleteJob";



    private final Uri mSrcParent;

    private final Uri mParentUri;



    private volatile int mDocsProcessed = 0;

    /**
@@ -54,7 +53,7 @@ final class DeleteJob extends Job { 
    DeleteJob(Context service, Listener listener, String id, DocumentStack stack,

            UrisSupplier srcs, @Nullable Uri srcParent) {

        super(service, listener, id, OPERATION_DELETE, stack, srcs);

        mSrcParent = srcParent;

        mParentUri = srcParent;

    }



    @Override
@@ -73,9 +72,10 @@ final class DeleteJob extends Job { 


    @Override

    public Notification getProgressNotification() {

        mProgressBuilder.setProgress(srcs.getItemCount(), mDocsProcessed, false);

        mProgressBuilder.setProgress(mResourceUris.getItemCount(), mDocsProcessed, false);

        String format = service.getString(R.string.delete_progress);

        mProgressBuilder.setSubText(String.format(format, mDocsProcessed, srcs.getItemCount()));

        mProgressBuilder.setSubText(

                String.format(format, mDocsProcessed, mResourceUris.getItemCount()));



        mProgressBuilder.setContentText(null);
@@ -95,44 +95,35 @@ final class DeleteJob extends Job { 


    @Override

    void start() {

        try {

            final List<DocumentInfo> srcs = new ArrayList<>(this.srcs.getItemCount());

        ContentResolver resolver = appContext.getContentResolver();



            final Iterable<Uri> uris = this.srcs.getUris(appContext);



            final ContentResolver resolver = appContext.getContentResolver();

            final DocumentInfo srcParent =

                mSrcParent != null

                    ? DocumentInfo.fromUri(resolver, mSrcParent)

        DocumentInfo parentDoc;

        try {

            parentDoc = mParentUri != null

                ? DocumentInfo.fromUri(resolver, mParentUri)

                : null;

            for (Uri uri : uris) {

                DocumentInfo doc = DocumentInfo.fromUri(resolver, uri);

                srcs.add(doc);

        } catch (FileNotFoundException e) {

          Log.e(TAG, "Failed to resolve parent from Uri: " + mParentUri + ". Cannot continue.", e);

          failureCount += this.mResourceUris.getItemCount();

          return;

        }



        for (DocumentInfo doc : mResolvedDocs) {

            if (DEBUG) Log.d(TAG, "Deleting document @ " + doc.derivedUri);

            try {

                    deleteDocument(doc, srcParent);



                    if (isCanceled()) {

                        // Canceled, dump the rest of the work. Deleted docs are not recoverable.

                        return;

                    }

                deleteDocument(doc, parentDoc);

            } catch (ResourceException e) {

                Log.e(TAG, "Failed to delete document @ " + doc.derivedUri, e);

                onFileFailed(doc);

            }



                ++mDocsProcessed;

            mDocsProcessed++;

            if (isCanceled()) {

                return;

            }



            // If mDocProcessed is different than the count claimed by UrisSupplier, add the number

            // to failedFileCount.

            failedFileCount += (this.srcs.getItemCount() - mDocsProcessed);

            Metrics.logFileOperation(service, operationType, srcs, null);

        } catch(IOException e) {

            Log.e(TAG, "Failed to get list of docs or parent source.", e);

            failedFileCount += srcs.getItemCount();

        }



        Metrics.logFileOperation(service, operationType, mResolvedDocs, null);

    }



    @Override
@@ -141,8 +132,9 @@ final class DeleteJob extends Job { 
                .append("DeleteJob")

                .append("{")

                .append("id=" + id)

                .append(", docs=" + srcs)

                .append(", srcParent=" + mSrcParent)

                .append(", uris=" + mResourceUris)

                .append(", docs=" + mResolvedDocs)

                .append(", srcParent=" + mParentUri)

                .append(", location=" + stack)

                .append("}")

                .toString();