Refatoring, warnings clean-up, improved error logic

res/values/strings.xml

@@ -71,7 +71,7 @@
     <string name="uploader_wrn_no_content_title">No file to upload</string>
     <string name="uploader_wrn_unknown_content_title">The content can not be uploaded</string>
     <string name="uploader_wrn_no_content_text">Sorry, the received data does not content any file.</string>
-    <string name="uploader_error_forbidden_content">%1$s is not allowed to access the shared content</string>
+    <string name="uploader_error_forbidden_content">%1$s is not allowed to access the shared file</string>
     <string name="uploader_info_uploading">Uploading</string>
     <string name="file_list_seconds_ago">seconds ago</string>
     <string name="file_list_empty">Nothing in here. Upload something!</string>
@@ -449,6 +449,6 @@
     <string name="manage_space_error">Some files could not be deleted.</string>
 
     <string name="permission_storage_access">Additional permissions required to upload &amp; download files.</string>
-    <string name="local_file_not_found_toast">The file was not found locally</string>
+    <string name="local_file_not_found_toast">The file was not found in the local file system</string>
 
 </resources>

src/com/owncloud/android/ui/activity/Uploader.java

@@ -26,7 +26,6 @@ import android.accounts.AccountManager;
 import android.accounts.AuthenticatorException;
 import android.annotation.SuppressLint;
 import android.app.Dialog;
-import android.app.ProgressDialog;
 import android.content.BroadcastReceiver;
 import android.content.ContentResolver;
 import android.content.Context;
@@ -57,7 +56,6 @@ import android.widget.AdapterView.OnItemClickListener;
 import android.widget.Button;
 import android.widget.EditText;
 import android.widget.ListView;
-import android.widget.ProgressBar;
 import android.widget.Toast;
 
 import com.owncloud.android.MainApp;
@@ -81,7 +79,6 @@ import com.owncloud.android.utils.DisplayUtils;
 import com.owncloud.android.utils.ErrorMessageAdapter;
 import com.owncloud.android.utils.UriUtils;
 
-import java.io.File;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.LinkedList;
@@ -95,7 +92,7 @@ import java.util.Vector;
  */
 public class Uploader extends FileActivity
         implements OnItemClickListener, android.view.View.OnClickListener,
-        CopyTmpFileAsyncTask.OnCopyTmpFileTaskListener {
+    CopyTmpFileAsyncTask.OnCopyTmpFilesTaskListener {
 
     private static final String TAG = Uploader.class.getSimpleName();
 
@@ -110,10 +107,7 @@ public class Uploader extends FileActivity
     private boolean mAccountSelected;
     private boolean mAccountSelectionShowing;
 
-    private int mNumCacheFile;
-
     private final static int DIALOG_NO_ACCOUNT = 0;
-    private final static int DIALOG_WAITING = 1;
     private final static int DIALOG_NO_STREAM = 2;
     private final static int DIALOG_MULTIPLE_ACCOUNT = 3;
     private final static int DIALOG_STREAM_UNKNOWN = 4;
@@ -124,8 +118,6 @@ public class Uploader extends FileActivity
     private final static String KEY_FILE = "FILE";
     private final static String KEY_ACCOUNT_SELECTED = "ACCOUNT_SELECTED";
     private final static String KEY_ACCOUNT_SELECTION_SHOWING = "ACCOUNT_SELECTION_SHOWING";
-    private final static String KEY_NUM_CACHE_FILE = "NUM_CACHE_FILE";
-    private final static String KEY_REMOTE_CACHE_DATA = "REMOTE_CACHE_DATA";
 
     private static final String DIALOG_WAIT_COPY_FILE = "DIALOG_WAIT_COPY_FILE";
 
@@ -134,17 +126,15 @@ public class Uploader extends FileActivity
         prepareStreamsToUpload();
 
         if (savedInstanceState == null) {
-            mParents = new Stack<String>();
+            mParents = new Stack<>();
             mAccountSelected = false;
             mAccountSelectionShowing = false;
-            mNumCacheFile = 0;
 
         } else {
             mParents = (Stack<String>) savedInstanceState.getSerializable(KEY_PARENTS);
             mFile = savedInstanceState.getParcelable(KEY_FILE);
             mAccountSelected = savedInstanceState.getBoolean(KEY_ACCOUNT_SELECTED);
             mAccountSelectionShowing = savedInstanceState.getBoolean(KEY_ACCOUNT_SELECTION_SHOWING);
-            mNumCacheFile = savedInstanceState.getInt(KEY_NUM_CACHE_FILE);
         }
 
         super.onCreate(savedInstanceState);
@@ -202,7 +192,6 @@ public class Uploader extends FileActivity
         outState.putParcelable(KEY_FILE, mFile);
         outState.putBoolean(KEY_ACCOUNT_SELECTED, mAccountSelected);
         outState.putBoolean(KEY_ACCOUNT_SELECTION_SHOWING, mAccountSelectionShowing);
-        outState.putInt(KEY_NUM_CACHE_FILE, mNumCacheFile);
         outState.putParcelable(FileActivity.EXTRA_ACCOUNT, getAccount());
 
         Log_OC.d(TAG, "onSaveInstanceState() end");
@@ -224,28 +213,13 @@ public class Uploader extends FileActivity
             @Override
             public boolean onKey(DialogInterface dialog, int keyCode, KeyEvent event) {
                 if (keyCode == KeyEvent.KEYCODE_BACK) {
-                    finish();
+                    Uploader.super.onBackPressed();
                     dialog.dismiss();
                 }
                 return true;
             }
         };
         switch (id) {
-        case DIALOG_WAITING:
-            final ProgressDialog pDialog = new ProgressDialog(this, R.style.ProgressDialogTheme);
-            pDialog.setIndeterminate(false);
-            pDialog.setCancelable(false);
-            pDialog.setMessage(getResources().getString(R.string.uploader_info_uploading));
-            pDialog.setOnShowListener(new DialogInterface.OnShowListener() {
-                @Override
-                public void onShow(DialogInterface dialog) {
-                    ProgressBar v = (ProgressBar) pDialog.findViewById(android.R.id.progress);
-                    v.getIndeterminateDrawable().setColorFilter(getResources().getColor(R.color.color_accent),
-                            android.graphics.PorterDuff.Mode.MULTIPLY);
-
-                }
-            });
-            return pDialog;
         case DIALOG_NO_ACCOUNT:
             builder.setIcon(R.drawable.ic_warning);
             builder.setTitle(R.string.uploader_wrn_no_account_title);
@@ -365,7 +339,6 @@ public class Uploader extends FileActivity
     public void onBackPressed() {
         if (mParents.size() <= 1) {
             super.onBackPressed();
-            return;
         } else {
             mParents.pop();
             String full_path = generatePath(mParents);
@@ -565,7 +538,7 @@ public class Uploader extends FileActivity
 
                     } else if (ContentResolver.SCHEME_FILE.equals(sourceUri.getScheme())) {
                         /// file: uris should point to a local file, should be safe let FileUploader handle them
-                        requestUpload(sourceUri.getPath(), remotePath); // TODO - CHECK PATH EXTRACTION
+                        requestUpload(sourceUri.getPath(), remotePath);
 
                     } else {
                         showDialog(DIALOG_STREAM_UNKNOWN);
@@ -589,12 +562,10 @@ public class Uploader extends FileActivity
 
     /**
      *
-     * @param sourceUris
-     * @param remotePaths
+     * @param sourceUris        Array of content:// URIs to the files to upload
+     * @param remotePaths       Array of absolute paths to set to the uploaded files
      */
     private void copyThenUpload(Uri[] sourceUris, String[] remotePaths) {
-        mNumCacheFile+= sourceUris.length;
-
         showWaitingCopyDialog();
 
         CopyTmpFileAsyncTask copyTask = new CopyTmpFileAsyncTask(this, this);
@@ -602,7 +573,8 @@ public class Uploader extends FileActivity
             CopyTmpFileAsyncTask.makeParamsToExecute(
                 getAccount(),
                 sourceUris,
-                remotePaths
+                remotePaths,
+                getContentResolver()
             )
         );
     }
@@ -832,29 +804,19 @@ public class Uploader extends FileActivity
             }
         }
     }
+
     /**
      * Process the result of CopyTmpFileAsyncTask
-     *
-     * @param numFiles
      */
     @Override
-    public void onTmpFileCopied(int numFiles) {
-
+    public void onTmpFilesCopied(ResultCode result) {
         dismissWaitingCopyDialog();
+        if (result != ResultCode.OK) {
+            showDialog(DIALOG_STREAM_UNKNOWN);  // TODO BETTER ERROR DIALOGS!
 
-        if(mNumCacheFile != numFiles) {
-            String message = String.format(
-                getString(R.string.uploader_error_forbidden_content),
-                getString(R.string.app_name)
-            );
-            Toast.makeText(this, message, Toast.LENGTH_LONG).show();
-            Log_OC.d(TAG, message);
+        } else {
+            finish();
         }
-
-        mNumCacheFile = 0;
-
-        finish();
-
     }
 
     /**

src/com/owncloud/android/utils/CopyTmpFileAsyncTask.java

@@ -28,10 +28,12 @@ import android.widget.Toast;
 
 import com.owncloud.android.R;
 import com.owncloud.android.files.services.FileUploader;
+import com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCode;
 import com.owncloud.android.lib.common.utils.Log_OC;
 import com.owncloud.android.operations.UploadFileOperation;
 
 import java.io.File;
+import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.InputStream;
 import java.lang.ref.WeakReference;
@@ -39,7 +41,7 @@ import java.lang.ref.WeakReference;
 /**
  * AsyncTask to copy a file from a uri in a temporal file
  */
-public class CopyTmpFileAsyncTask  extends AsyncTask<Object, Void, Integer> {
+public class CopyTmpFileAsyncTask  extends AsyncTask<Object, Void, ResultCode> {
 
     private final String TAG = CopyTmpFileAsyncTask.class.getSimpleName();
 
@@ -48,149 +50,179 @@ public class CopyTmpFileAsyncTask  extends AsyncTask<Object, Void, Integer> {
      *
      * Just packages the received parameters in correct order, doesn't check anything about them.
      *
+     * @param   account             OC account to upload the shared files.
+     * @param   sourceUris          Array of "content://" URIs to the files to be uploaded.
+     * @param   remotePaths         Array of absolute paths in the OC account to set to the uploaded files.
+     * @param   contentResolver     {@link ContentResolver} instance with appropriate permissions to open the
+     *                              URIs in 'sourceUris'.
+     *
+     * Handling this parameter in {@link #doInBackground(Object[])} keeps an indirect reference to the
+     * caller Activity, what is technically wrong, since it will be held in memory
+     * (with all its associated resources) until the task finishes even though the user leaves the Activity.
+     *
+     * But we really, really, really want that the files are copied to temporary files in the OC folder and then
+     * uploaded, even if the user gets bored of waiting while the copy finishes. And we can't forward the job to
+     * another {@link Context}, because if any of the content:// URIs is constrained by a TEMPORARY READ PERMISSION,
+     * trying to open it will fail with a {@link SecurityException} after the user leaves the Uploader Activity. We
+     * really tried it.
+     *
+     * So we are doomed to leak here for the best interest of the user. Please, don't do similar in other places.
+     *
+     * Any idea to prevent this while keeping the functionality will be welcome.
+     *
      * @return  Correct array of parameters to be passed to {@link #execute(Object[])}
      */
-    public final static Object[] makeParamsToExecute(
+    public static Object[] makeParamsToExecute(
         Account account,
         Uri[] sourceUris,
-        String[] remotePaths
+        String[] remotePaths,
+        ContentResolver contentResolver
     ) {
 
         return new Object[] {
             account,
             sourceUris,
-            remotePaths
+            remotePaths,
+            contentResolver
         };
     }
 
 
     /**
-     * Listener in main thread to be notified when the task ends. Held in a WeakReference assuming that it's
+     * Listener in main thread to be notified when the task ends. Held in a WeakReference assuming that its
      * lifespan is associated with an Activity context, that could be finished by the user before the AsyncTask
      * ends.
      */
-    private final WeakReference<OnCopyTmpFileTaskListener> mListener;
+    private final WeakReference<OnCopyTmpFilesTaskListener> mListener;
 
     /**
      * Reference to application context, used to access app resources. Holding it should not be a problem,
      * since it needs to exist until the end of the AsyncTask although the caller Activity were finished
      * before.
      */
-    private final Context mContext;
+    private final Context mAppContext;
 
 
-    public CopyTmpFileAsyncTask(OnCopyTmpFileTaskListener listener, Context context) {
-        mListener = new WeakReference<OnCopyTmpFileTaskListener>(listener);
-        mContext = context;
+    public CopyTmpFileAsyncTask(
+        OnCopyTmpFilesTaskListener listener,
+        Context context
+    ) {
+        mListener = new WeakReference<>(listener);
+        mAppContext = context.getApplicationContext();
     }
 
     /**
-     * Params for execute:
-     * - Uri: uri of file
-     * - String: path for saving the file into the app
-     * - int: index of upload
-     * - String: accountName
-     * - ContentResolver: content resolver
+     * @param params    Params to execute the task; see
+     *                  {@link #makeParamsToExecute(Account, Uri[], String[], ContentResolver)}
+     *                  for further details.
      */
     @Override
-    protected Integer doInBackground(Object[] params) {
+    protected ResultCode doInBackground(Object[] params) {
+
+        ResultCode result = ResultCode.UNKNOWN_ERROR;
 
-        int numFiles = 0;
+        InputStream inputStream = null;
+        FileOutputStream outputStream = null;
+        String fullTempPath = null;
+        Uri currentUri = null;
 
-        if (params != null && params.length == 3) {
+        try {
             Account account = (Account) params[0];
             Uri[] uris = (Uri[]) params[1];
             String[] remotePaths = (String[]) params[2];
+            ContentResolver leakedContentResolver = (ContentResolver) params[3];
 
-            InputStream inputStream = null;
-            FileOutputStream outputStream = null;
-            String fullTempPath = null;
+            String currentRemotePath;
 
-            Uri actualUri = null;
-            String actualRemotePath = null;
+            for (int i = 0; i < uris.length; i++) {
+                currentUri = uris[i];
+                currentRemotePath = remotePaths[i];
 
-            ContentResolver contentResolver = mContext.getContentResolver();
-            // TODO: test that it's safe for URLs with temporary access;
-            //      alternative: receive InputStream in another parameter
+                fullTempPath = FileStorageUtils.getTemporalPath(account.name) + currentRemotePath;
+                inputStream = leakedContentResolver.openInputStream(currentUri);
+                File cacheFile = new File(fullTempPath);
+                File tempDir = cacheFile.getParentFile();
+                if (!tempDir.exists()) {
+                    tempDir.mkdirs();
+                }
+                cacheFile.createNewFile();
+                outputStream = new FileOutputStream(fullTempPath);
+                byte[] buffer = new byte[4096];
 
-            try {
-                for(int i=0; i < uris.length; i++) {
-                    actualUri = uris[i];
-                    actualRemotePath = remotePaths[i];
+                int count;
+                while ((count = inputStream.read(buffer)) > 0) {
+                    outputStream.write(buffer, 0, count);
+                }
 
-                    fullTempPath = FileStorageUtils.getTemporalPath(account.name) + actualRemotePath;
-                    inputStream = contentResolver.openInputStream(actualUri);
-                    File cacheFile = new File(fullTempPath);
-                    File tempDir = cacheFile.getParentFile();
-                    if (!tempDir.exists()) {
-                        tempDir.mkdirs();
-                    }
-                    cacheFile.createNewFile();
-                    outputStream = new FileOutputStream(fullTempPath);
-                    byte[] buffer = new byte[4096];
+                requestUpload(
+                    account,
+                    fullTempPath,
+                    currentRemotePath,
+                    leakedContentResolver.getType(currentUri)
+                );
+                fullTempPath = null;
+            }
 
-                    int count = 0;
+            result = ResultCode.OK;
 
-                    while ((count = inputStream.read(buffer)) > 0) {
-                        outputStream.write(buffer, 0, count);
-                    }
+        } catch (ArrayIndexOutOfBoundsException e) {
+            Log_OC.e(TAG, "Wrong number of arguments received ", e);
 
-                    requestUpload(
-                        account,
-                        fullTempPath,
-                        actualRemotePath,
-                        contentResolver.getType(actualUri)
-                    );
-                    numFiles++;
-                }
+        } catch (ClassCastException e) {
+            Log_OC.e(TAG, "Wrong parameter received ", e);
 
-            } catch (Exception e) {
-                Log_OC.e(TAG, "Exception while copying " + actualUri.toString() + " to temporary file", e);
+        } catch (FileNotFoundException e) {
+            Log_OC.e(TAG, "Could not find source file " + currentUri, e);
+            result = ResultCode.LOCAL_FILE_NOT_FOUND;
 
-                // clean
-                if (fullTempPath != null) {
-                    File f = new File(fullTempPath);
-                    if (f.exists()) {
-                        if (!f.delete()) {
-                            Log_OC.e(TAG, "Could not delete temporary file " + fullTempPath);
-                        }
-                    }
-                }
+        } catch (SecurityException e) {
+            Log_OC.e(TAG, "Not enough permissions to read source file " + currentUri, e);
+            result = ResultCode.FORBIDDEN;
+
+        } catch (Exception e) {
+            Log_OC.e(TAG, "Exception while copying " + currentUri + " to temporary file", e);
+            result =  ResultCode.LOCAL_STORAGE_NOT_COPIED;
 
-            } finally {
-                if (inputStream != null) {
-                    try {
-                        inputStream.close();
-                    } catch (Exception e) {
-                        Log_OC.w(TAG, "Ignoring exception of inputStream closure");
+            // clean
+            if (fullTempPath != null) {
+                File f = new File(fullTempPath);
+                if (f.exists()) {
+                    if (!f.delete()) {
+                        Log_OC.e(TAG, "Could not delete temporary file " + fullTempPath);
                     }
                 }
+            }
 
-                if (outputStream != null) {
-                    try {
-                        outputStream.close();
-                    } catch (Exception e) {
-                        Log_OC.w(TAG, "Ignoring exception of outStream closure");
-                    }
+        } finally {
+            if (inputStream != null) {
+                try {
+                    inputStream.close();
+                } catch (Exception e) {
+                    Log_OC.w(TAG, "Ignoring exception of inputStream closure");
                 }
             }
 
-        } else {
-            throw new IllegalArgumentException("Error in parameters number");
+            if (outputStream != null) {
+                try {
+                    outputStream.close();
+                } catch (Exception e) {
+                    Log_OC.w(TAG, "Ignoring exception of outStream closure");
+                }
+            }
         }
 
-        return numFiles;
+        return result;
     }
 
     private void requestUpload(Account account, String localPath, String remotePath, String mimeType) {
         FileUploader.UploadRequester requester = new FileUploader.UploadRequester();
         requester.uploadNewFile(
-            mContext,
+            mAppContext,
             account,
             localPath,
             remotePath,
             FileUploader.LOCAL_BEHAVIOUR_MOVE,  // the copy was already done, let's take advantage and move it
-            // into the OC folder so that the folder was not
+                                                // into the OC folder so that appears as downloaded
             mimeType,
             false,      // do not create parent folder if not existent
             UploadFileOperation.CREATED_BY_USER // TODO , different category?
@@ -198,20 +230,42 @@ public class CopyTmpFileAsyncTask  extends AsyncTask<Object, Void, Integer> {
     }
 
     @Override
-    protected void onPostExecute(Integer numFiles) {
-        OnCopyTmpFileTaskListener listener = mListener.get();
+    protected void onPostExecute(ResultCode result) {
+        OnCopyTmpFilesTaskListener listener = mListener.get();
         if (listener!= null) {
-            listener.onTmpFileCopied(numFiles.intValue());
+            listener.onTmpFilesCopied(result);
+
         } else {
             Log_OC.i(TAG, "User left Uploader activity before the temporal copies were finished ");
+            if (result != ResultCode.OK) {
+                // if the user left the app, report background error in a Toast
+                int messageId;
+                switch (result) {
+                    case LOCAL_FILE_NOT_FOUND:
+                        messageId = R.string.copy_file_not_found;
+                        break;
+                    case LOCAL_STORAGE_NOT_COPIED:
+                        messageId = R.string.copy_file_error;
+                        break;
+                    case FORBIDDEN:
+                        messageId = R.string.uploader_error_forbidden_content;
+                        break;
+                    default:
+                        messageId = R.string.common_error_unknown;
+                }
+                String message = String.format(
+                    mAppContext.getString(messageId),
+                    mAppContext.getString(R.string.app_name)
+                );
+                Toast.makeText(mAppContext, message, Toast.LENGTH_LONG).show();
+            }
         }
     }
 
     /*
      * Interface to retrieve data from recognition task
      */
-    public interface OnCopyTmpFileTaskListener{
-
-        void onTmpFileCopied(int numFiles);
+    public interface OnCopyTmpFilesTaskListener {
+        void onTmpFilesCopied(ResultCode result);
     }
 }