Reviewed error handling and notifications in downloads

AndroidManifest.xml

@@ -18,7 +18,7 @@
  -->
 <manifest package="eu.alefzero.owncloud"
     android:versionCode="1"
-    android:versionName="0.1.179B" xmlns:android="http://schemas.android.com/apk/res/android">
+    android:versionName="0.1.180B" xmlns:android="http://schemas.android.com/apk/res/android">
 
     <uses-permission android:name="android.permission.GET_ACCOUNTS" />
     <uses-permission android:name="android.permission.USE_CREDENTIALS" />

res/values/strings.xml

@@ -72,8 +72,12 @@
     <string name="uploader_info_dirname">Directory name</string>
     <string name="uploader_upload_succeed">Upload was successful</string>
     <string name="uploader_upload_failed">Upload failed: %1$d/%2$d files uploaded</string>
-    <string name="downloader_download_succeed">Download was successful</string>
+    <string name="downloader_download_in_progress_ticker">Downloading &#8230;</string>
-    <string name="downloader_download_failed">Download could not be completed</string>
+    <string name="downloader_download_in_progress_content">%1$d%% Downloading %2$s</string>
+    <string name="downloader_download_succeed_ticker">Download suceeded</string>
+    <string name="downloader_download_succeed_content">%1$s was successfully download</string>
+    <string name="downloader_download_failed_ticker">Download failed</string>
+    <string name="downloader_download_failed_content">Download of %1$s could not be completed</string>
     <string name="common_choose_account">Choose account</string>
     <string name="sync_string_contacts">Contacts</string>
     <string name="use_ssl">Use Secure Connection</string>

src/eu/alefzero/owncloud/files/services/FileDownloader.java

@@ -8,6 +8,8 @@ import java.util.Map;
 
 import android.accounts.Account;
 import android.accounts.AccountManager;
+import android.accounts.AuthenticatorException;
+import android.accounts.OperationCanceledException;
 import android.app.Notification;
 import android.app.NotificationManager;
 import android.app.PendingIntent;
@@ -22,6 +24,7 @@ import android.os.Looper;
 import android.os.Message;
 import android.os.Process;
 import android.util.Log;
+import android.util.LogPrinter;
 import android.widget.RemoteViews;
 import android.widget.Toast;
 import eu.alefzero.owncloud.R;
@@ -49,7 +52,7 @@ public class FileDownloader extends Service implements OnDatatransferProgressLis
     private String mRemotePath;
     private int mLastPercent;
     private long mTotalDownloadSize;
-    private long mCurrentDownlodSize;
+    private long mCurrentDownloadSize;
     private Notification mNotification;
     
     /**
@@ -112,61 +115,67 @@ public class FileDownloader extends Service implements OnDatatransferProgressLis
 
     @Override
     public int onStartCommand(Intent intent, int flags, int startId) {
-        if (!intent.hasExtra(EXTRA_ACCOUNT)
+        if (    !intent.hasExtra(EXTRA_ACCOUNT) ||
-                && !intent.hasExtra(EXTRA_FILE_PATH)) {
+                !intent.hasExtra(EXTRA_FILE_PATH) ||
+                !intent.hasExtra(EXTRA_REMOTE_PATH)
+           ) {
             Log.e(TAG, "Not enough information provided in intent");
             return START_STICKY;
         }
         mAccount = intent.getParcelableExtra(EXTRA_ACCOUNT);
         mFilePath = intent.getStringExtra(EXTRA_FILE_PATH);
         mRemotePath = intent.getStringExtra(EXTRA_REMOTE_PATH);
+        mTotalDownloadSize = intent.getLongExtra(EXTRA_FILE_SIZE, -1);
+        mCurrentDownloadSize = mLastPercent = 0;
+
         Message msg = mServiceHandler.obtainMessage();
         msg.arg1 = startId;
         mServiceHandler.sendMessage(msg);
-        mCurrentDownlodSize = mLastPercent = 0;
-        mTotalDownloadSize = intent.getLongExtra(EXTRA_FILE_SIZE, -1);
 
         return START_NOT_STICKY;
     }
 
     void downloadFile() {
+        boolean downloadResult = false;
+
+        /// prepare client object to send the request to the ownCloud server
         AccountManager am = (AccountManager) getSystemService(ACCOUNT_SERVICE);
-
-
         WebdavClient wdc = new WebdavClient(mAccount, getApplicationContext());
-        
         String username = mAccount.name.split("@")[0];
-        String password = "";
+        String password = null;
         try {
             password = am.blockingGetAuthToken(mAccount,
                     AccountAuthenticator.AUTH_TOKEN_TYPE, true);
         } catch (Exception e) {
-            e.printStackTrace();
+            Log.e(TAG, "Access to account credentials failed", e);
+            // TODO - check if that log prints the stack trace
+            sendFinalBroadcast(downloadResult, null);
             return;
         }
-
         wdc.setCredentials(username, password);
         wdc.allowSelfsignedCertificates();
         wdc.setDataTransferProgressListener(this);
 
-        mNotification = new Notification(R.drawable.icon, "Downloading file", System.currentTimeMillis());
         
+        /// download will be in a temporal file
+        File tmpFile = new File(getTemporalPath() + mAccount.name + mFilePath);
+        
+        /// create status notification to show the download progress
+        mNotification = new Notification(R.drawable.icon, getString(R.string.downloader_download_in_progress_ticker), System.currentTimeMillis());
         mNotification.flags |= Notification.FLAG_ONGOING_EVENT;
         mNotification.contentView = new RemoteViews(getApplicationContext().getPackageName(), R.layout.progressbar_layout);
         mNotification.contentView.setProgressBar(R.id.status_progress, 100, 0, mTotalDownloadSize == -1);
+        mNotification.contentView.setTextViewText(R.id.status_text, String.format(getString(R.string.downloader_download_in_progress_content), 0, tmpFile.getName()));
         mNotification.contentView.setImageViewResource(R.id.status_icon, R.drawable.icon);
         // dvelasco ; contentIntent MUST be assigned to avoid app crashes in versions previous to Android 4.x ;
         //              BUT an empty Intent is not a very elegant solution; something smart should happen when a user 'clicks' on a download in the notification bar
         mNotification.contentIntent = PendingIntent.getActivity(getApplicationContext(), 0, new Intent(), PendingIntent.FLAG_UPDATE_CURRENT);
+        mNotificationMngr.notify(R.string.downloader_download_in_progress_ticker, mNotification);
         
-        mNotificationMngr.notify(1, mNotification);
 
-        // download in a temporal file
+        /// perform the download
-        File tmpFile = new File(getTemporalPath() + mAccount.name + mFilePath);
         tmpFile.getParentFile().mkdirs();
         mDownloadsInProgress.put(buildRemoteName(mAccount.name, mRemotePath), tmpFile.getAbsolutePath());
-
-        boolean download_result = false;
         File newFile = null;
         if (wdc.downloadFile(mRemotePath, tmpFile)) {
             newFile = new File(getSavePath() + mAccount.name + mFilePath);
@@ -184,42 +193,57 @@ public class FileDownloader extends Service implements OnDatatransferProgressLis
                     new String[] {
                             mFilePath.substring(mFilePath.lastIndexOf('/') + 1),
                             mAccount.name });
-                download_result = true;
+                downloadResult = true;
             }
         }
-        
         mDownloadsInProgress.remove(buildRemoteName(mAccount.name, mRemotePath));
 
-        mNotificationMngr.cancel(1);
-        Intent end = new Intent(DOWNLOAD_FINISH_MESSAGE);
-        end.putExtra(EXTRA_DOWNLOAD_RESULT, download_result);
-        end.putExtra(ACCOUNT_NAME, mAccount.name);
-        end.putExtra(EXTRA_REMOTE_PATH, mRemotePath);
-        if (download_result) {
-            end.putExtra(EXTRA_FILE_PATH, newFile.getAbsolutePath());
-        }
-        sendBroadcast(end);
         
-        if (download_result) {
+        /// notify result
-            Toast.makeText(this, R.string.downloader_download_succeed , Toast.LENGTH_SHORT).show();
+        mNotificationMngr.cancel(R.string.downloader_download_in_progress_ticker);
-        } else {
+        int tickerId = (downloadResult) ? R.string.downloader_download_succeed_ticker : R.string.downloader_download_failed_ticker;
-            Toast.makeText(this, R.string.downloader_download_failed , Toast.LENGTH_SHORT).show();
+        int contentId = (downloadResult) ? R.string.downloader_download_succeed_content : R.string.downloader_download_failed_content;
-        }
+        Notification finalNotification = new Notification(R.drawable.icon, getString(tickerId), System.currentTimeMillis());
+        finalNotification.flags |= Notification.FLAG_AUTO_CANCEL;
+        // dvelasco ; contentIntent MUST be assigned to avoid app crashes in versions previous to Android 4.x ;
+        //              BUT an empty Intent is not a very elegant solution; something smart should happen when a user 'clicks' on a download in the notification bar
+        finalNotification.contentIntent = PendingIntent.getActivity(getApplicationContext(), 0, new Intent(), PendingIntent.FLAG_UPDATE_CURRENT);
+        finalNotification.setLatestEventInfo(getApplicationContext(), getString(tickerId), String.format(getString(contentId), tmpFile.getName()), finalNotification.contentIntent);
+        mNotificationMngr.notify(tickerId, finalNotification);
             
+        sendFinalBroadcast(downloadResult, (downloadResult)?newFile.getAbsolutePath():null);
     }
 
+    
     @Override
     public void transferProgress(long progressRate) {
-        mCurrentDownlodSize += progressRate;
+        mCurrentDownloadSize += progressRate;
-        int percent = (int)(100.0*((double)mCurrentDownlodSize)/((double)mTotalDownloadSize));
+        int percent = (int)(100.0*((double)mCurrentDownloadSize)/((double)mTotalDownloadSize));
         if (percent != mLastPercent) {
-          mNotification.contentView.setProgressBar(R.id.status_progress, 100, (int)(100*mCurrentDownlodSize/mTotalDownloadSize), mTotalDownloadSize == -1);
+          mNotification.contentView.setProgressBar(R.id.status_progress, 100, (int)(100*mCurrentDownloadSize/mTotalDownloadSize), mTotalDownloadSize == -1);
-          mNotification.contentView.setTextViewText(R.id.status_text, percent+"%");
+          mNotification.contentView.setTextViewText(R.id.status_text, String.format(getString(R.string.downloader_download_in_progress_content), percent, new File(mFilePath).getName()));
-          mNotificationMngr.notify(1, mNotification);
+          mNotificationMngr.notify(R.string.downloader_download_in_progress_ticker, mNotification);
         }
         
         mLastPercent = percent;
     }
     
 
+    /**
+     * Sends a broadcast in order to the interested activities can update their view
+     * 
+     * @param downloadResult        'True' if the download was successful
+     * @param newFilePath           Absolute path to the download file
+     */
+    private void sendFinalBroadcast(boolean downloadResult, String newFilePath) {
+        Intent end = new Intent(DOWNLOAD_FINISH_MESSAGE);
+        end.putExtra(EXTRA_DOWNLOAD_RESULT, downloadResult);
+        end.putExtra(ACCOUNT_NAME, mAccount.name);
+        end.putExtra(EXTRA_REMOTE_PATH, mRemotePath);
+        if (downloadResult) {
+            end.putExtra(EXTRA_FILE_PATH, newFilePath);
+        }
+        sendBroadcast(end);
+    }
+
 }

src/eu/alefzero/owncloud/ui/fragment/FileDetailFragment.java

@@ -218,7 +218,6 @@ public class FileDetailFragment extends SherlockFragment implements
                 i.putExtra(FileDownloader.EXTRA_FILE_SIZE, mFile.getFileLength());
                 
                 // update ui 
-                Toast.makeText(getActivity(), "Downloading", Toast.LENGTH_LONG).show();
                 setButtonsForDownloading();
                 
                 getActivity().startService(i);

src/eu/alefzero/webdav/WebdavClient.java

@@ -137,24 +137,25 @@ public class WebdavClient extends HttpClient {
      * Downloads a file in remoteFilepath to the local targetPath.
      * 
      * @param remoteFilepath    Path to the file in the remote server, URL DECODED. 
-     * @param targetPath        Local path to save the downloaded file.
+     * @param targetFile        Local path to save the downloaded file.
      * @return                  'True' when the file is successfully downloaded.
      */
-    public boolean downloadFile(String remoteFilepath, File targetPath) {
+    public boolean downloadFile(String remoteFilepath, File targetFile) {
         boolean ret = false;
+        boolean caughtException = false;
         GetMethod get = new GetMethod(mUri.toString() + WebdavUtils.encodePath(remoteFilepath));
 
         // get.setHeader("Host", mUri.getHost());
         // get.setHeader("User-Agent", "Android-ownCloud");
 
+        int status = -1;
         try {
-            int status = executeMethod(get, 0);
+            status = executeMethod(get);
-            Log.e(TAG, "status return: " + status);
             if (status == HttpStatus.SC_OK) {
-                targetPath.createNewFile();
+                targetFile.createNewFile();
                 BufferedInputStream bis = new BufferedInputStream(
                         get.getResponseBodyAsStream());
-                FileOutputStream fos = new FileOutputStream(targetPath);
+                FileOutputStream fos = new FileOutputStream(targetFile);
 
                 byte[] bytes = new byte[4096];
                 int readResult;
@@ -166,11 +167,28 @@ public class WebdavClient extends HttpClient {
                 ret = true;
             }
             
-        } catch (Throwable e) {
+        } catch (HttpException e) {
-            e.printStackTrace();
+            Log.e(TAG, "HTTP exception downloading " + remoteFilepath, e);
-            targetPath.delete();
+            caughtException = true;
-        }
 
+        } catch (IOException e) {
+            Log.e(TAG, "I/O exception downloading " + remoteFilepath, e);
+            caughtException = true;
+
+        } catch (Exception e) {
+            Log.e(TAG, "Unexpected exception downloading " + remoteFilepath, e);
+            caughtException = true;
+            
+        } finally {
+            if (!ret) {
+                if (!caughtException) {
+                    Log.e(TAG, "Download of " + remoteFilepath + " to " + targetFile + " failed with HTTP status " + status);
+                }
+                if (targetFile.exists()) {
+                    targetFile.delete();
+                }
+            }
+        }
         return ret;
     }