Improve how the download manager reports paused statuses.
Steve Howard [Wed, 29 Sep 2010 23:48:01 +0000 (16:48 -0700)]
This change makes the download manager report more detail when a
download is paused.  Rather than always reporting status
RUNNING_PAUSED, there are now four different statuses:

 * paused by the app
 * waiting to retry after a network error
 * waiting for network connectivity
 * queued for wifi due to size limits

This allows a few improvements:

 * code deciding when to run a download can be improved and cleaned up
   (I've taken some extra steps in cleaning up this particular code)
 * notification code no longer has to rely on the in-memory-only
   "mPausedReason" member of DownloadInfo; instead, it knows from the
   status that the download is queued for wifi, and can display the
   appropriate string.  This moves the string fetching out into the
   UI-specific logic and is a sign that this is really the right way
   to do things.

And finally, the real motivation for this change: I've changed the
meaning of "Queued" in the downloads UI so it now means "Queued for
WiFi'.  This is what was originally intended, I'd misunderstood.  What
was formerly known as "Queued", a download that hadn't started, is now
displayed as "In progress" (it's always a transient state so it's
basically meaningless anyway).  Otherwise it remains the same (in
particular, downloads paused for other reasons are still reported as
"In progress").

I've also increased some of the logging in DownloadThread a bit, as
this change initally introduced some bugs that were impossible to
track down without that logging.  There have been other bug reports
that were impossible to diagnose and these few extra log statements
should really help, without cluttering logs too much.  I've taken care
to avoid potentially introducing any PII into the logs.

Change-Id: Id0b8d65fc8e4406ad7ffa1439ffc22a0281b051f

src/com/android/providers/downloads/DownloadInfo.java
src/com/android/providers/downloads/DownloadNotification.java
src/com/android/providers/downloads/DownloadService.java
src/com/android/providers/downloads/DownloadThread.java
tests/src/com/android/providers/downloads/DownloadManagerFunctionalTest.java
tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
ui/res/values/strings.xml
ui/src/com/android/providers/downloads/ui/DownloadAdapter.java
ui/src/com/android/providers/downloads/ui/DownloadList.java

index a204895..b6f478c 100644 (file)
@@ -227,7 +227,6 @@ public class DownloadInfo {
     public String mTitle;
     public String mDescription;
     public int mBypassRecommendedSizeLimit;
-    public String mPausedReason;
 
     public int mFuzz;
 
@@ -275,10 +274,12 @@ public class DownloadInfo {
     }
 
     /**
-     * Returns the time when a download should be restarted. Must only
-     * be called when numFailed > 0.
+     * Returns the time when a download should be restarted.
      */
-    public long restartTime() {
+    public long restartTime(long now) {
+        if (mNumFailed == 0) {
+            return now;
+        }
         if (mRetryAfter > 0) {
             return mLastMod + mRetryAfter;
         }
@@ -291,67 +292,29 @@ public class DownloadInfo {
      * Returns whether this download (which the download manager hasn't seen yet)
      * should be started.
      */
-    public boolean isReadyToStart(long now) {
+    private boolean isReadyToStart(long now) {
+        if (mHasActiveThread) {
+            // already running
+            return false;
+        }
         if (mControl == Downloads.Impl.CONTROL_PAUSED) {
             // the download is paused, so it's not going to start
             return false;
         }
-        if (mStatus == 0) {
-            // status hasn't been initialized yet, this is a new download
-            return true;
-        }
-        if (mStatus == Downloads.Impl.STATUS_PENDING) {
-            // download is explicit marked as ready to start
-            return true;
-        }
-        if (mStatus == Downloads.Impl.STATUS_RUNNING) {
-            // download was interrupted (process killed, loss of power) while it was running,
-            //     without a chance to update the database
-            return true;
-        }
-        if (mStatus == Downloads.Impl.STATUS_RUNNING_PAUSED) {
-            if (mNumFailed == 0) {
-                // download is waiting for network connectivity to return before it can resume
+        switch (mStatus) {
+            case 0: // status hasn't been initialized yet, this is a new download
+            case Downloads.Impl.STATUS_PENDING: // download is explicit marked as ready to start
+            case Downloads.Impl.STATUS_RUNNING: // download interrupted (process killed etc) while
+                                                // running, without a chance to update the database
                 return true;
-            }
-            if (restartTime() < now) {
-                // download was waiting for a delayed restart, and the delay has expired
-                return true;
-            }
-        }
-        return false;
-    }
 
-    /**
-     * Returns whether this download (which the download manager has already seen
-     * and therefore potentially started) should be restarted.
-     *
-     * In a nutshell, this returns true if the download isn't already running
-     * but should be, and it can know whether the download is already running
-     * by checking the status.
-     */
-    public boolean isReadyToRestart(long now) {
-        if (mControl == Downloads.Impl.CONTROL_PAUSED) {
-            // the download is paused, so it's not going to restart
-            return false;
-        }
-        if (mStatus == 0) {
-            // download hadn't been initialized yet
-            return true;
-        }
-        if (mStatus == Downloads.Impl.STATUS_PENDING) {
-            // download is explicit marked as ready to start
-            return true;
-        }
-        if (mStatus == Downloads.Impl.STATUS_RUNNING_PAUSED) {
-            if (mNumFailed == 0) {
-                // download is waiting for network connectivity to return before it can resume
+            case Downloads.Impl.STATUS_WAITING_FOR_NETWORK:
+            case Downloads.Impl.STATUS_QUEUED_FOR_WIFI:
                 return checkCanUseNetwork() == NETWORK_OK;
-            }
-            if (restartTime() < now) {
-                // download was waiting for a delayed restart, and the delay has expired
-                return true;
-            }
+
+            case Downloads.Impl.STATUS_WAITING_TO_RETRY:
+                // download was waiting for a delayed restart
+                return restartTime(now) <= now;
         }
         return false;
     }
@@ -450,7 +413,11 @@ public class DownloadInfo {
         return NETWORK_OK;
     }
 
-    void start(long now) {
+    void startIfReady(long now) {
+        if (!isReadyToStart(now)) {
+            return;
+        }
+
         if (Constants.LOGV) {
             Log.v(Constants.TAG, "Service spawning thread to handle download " + mId);
         }
@@ -521,13 +488,10 @@ public class DownloadInfo {
         if (Downloads.Impl.isStatusCompleted(mStatus)) {
             return -1;
         }
-        if (mStatus != Downloads.Impl.STATUS_RUNNING_PAUSED) {
-            return 0;
-        }
-        if (mNumFailed == 0) {
+        if (mStatus != Downloads.Impl.STATUS_WAITING_TO_RETRY) {
             return 0;
         }
-        long when = restartTime();
+        long when = restartTime(now);
         if (when <= now) {
             return 0;
         }
@@ -545,8 +509,6 @@ public class DownloadInfo {
     }
 
     void notifyPauseDueToSize(boolean isWifiRequired) {
-        mPausedReason = mContext.getResources().getString(
-                R.string.notification_need_wifi_for_size);
         Intent intent = new Intent(Intent.ACTION_VIEW);
         intent.setData(getAllDownloadsUri());
         intent.setClassName(SizeLimitActivity.class.getPackage().getName(),
index 90c8693..4d615df 100644 (file)
@@ -138,8 +138,10 @@ class DownloadNotification {
                 item.addItem(title, progress, max);
                 mNotifications.put(packageName, item);
             }
-            if (hasPausedReason(download) && item.mPausedText == null) {
-                item.mPausedText = download.mPausedReason;
+            if (download.mStatus == Downloads.Impl.STATUS_QUEUED_FOR_WIFI
+                    && item.mPausedText == null) {
+                item.mPausedText = mContext.getResources().getString(
+                        R.string.notification_need_wifi_for_size);
             }
         }
 
@@ -205,10 +207,6 @@ class DownloadNotification {
         }
     }
 
-    private boolean hasPausedReason(DownloadInfo download) {
-        return download.mStatus == Downloads.STATUS_RUNNING_PAUSED && download.mPausedReason != null;
-    }
-
     private void updateCompletedNotification(Collection<DownloadInfo> downloads) {
         for (DownloadInfo download : downloads) {
             if (!isCompleteAndVisible(download)) {
index c79ecc4..8fbcf4b 100644 (file)
@@ -439,10 +439,7 @@ public class DownloadService extends Service {
             info.logVerboseInfo();
         }
 
-        if (info.isReadyToStart(now)) {
-            info.start(now);
-        }
-
+        info.startIfReady(now);
         return info;
     }
 
@@ -466,9 +463,7 @@ public class DownloadService extends Service {
             mSystemFacade.cancelNotification(info.mId);
         }
 
-        if (info.isReadyToRestart(now)) {
-            info.start(now);
-        }
+        info.startIfReady(now);
     }
 
     /**
index 12ddfa6..b384771 100644 (file)
@@ -141,7 +141,6 @@ public class DownloadThread extends Thread {
         AndroidHttpClient client = null;
         PowerManager.WakeLock wakeLock = null;
         int finalStatus = Downloads.Impl.STATUS_UNKNOWN_ERROR;
-        mInfo.mPausedReason = null;
 
         try {
             PowerManager pm = (PowerManager) mContext.getSystemService(Context.POWER_SERVICE);
@@ -157,6 +156,7 @@ public class DownloadThread extends Thread {
 
             boolean finished = false;
             while(!finished) {
+                Log.i(Constants.TAG, "Initiating request for download " + mInfo.mId);
                 HttpGet request = new HttpGet(state.mRequestUri);
                 try {
                     executeDownload(state, client, request);
@@ -175,25 +175,19 @@ public class DownloadThread extends Thread {
             finalizeDestinationFile(state);
             finalStatus = Downloads.Impl.STATUS_SUCCESS;
         } catch (StopRequest error) {
-            if (Constants.LOGV) {
-                Log.v(Constants.TAG, "Aborting request for " + mInfo.mUri, error);
-            }
+            // remove the cause before printing, in case it contains PII
+            Log.w(Constants.TAG, "Aborting request for download " + mInfo.mId, removeCause(error));
             finalStatus = error.mFinalStatus;
             // fall through to finally block
         } catch (FileNotFoundException ex) {
-            Log.d(Constants.TAG, "FileNotFoundException for " + state.mFilename + " : " +  ex);
+            Log.w(Constants.TAG, "FileNotFoundException for " + state.mFilename, ex);
             finalStatus = Downloads.Impl.STATUS_FILE_ERROR;
             // falls through to the code that reports an error
-        } catch (RuntimeException ex) { //sometimes the socket code throws unchecked exceptions
-            if (Constants.LOGV) {
-                Log.d(Constants.TAG, "Exception for " + mInfo.mUri, ex);
-            } else if (Config.LOGD) {
-                Log.d(Constants.TAG, "Exception for id " + mInfo.mId, ex);
-            }
+        } catch (Throwable ex) { //sometimes the socket code throws unchecked exceptions
+            Log.w(Constants.TAG, "Exception for id " + mInfo.mId, ex);
             finalStatus = Downloads.Impl.STATUS_UNKNOWN_ERROR;
             // falls through to the code that reports an error
         } finally {
-            mInfo.mHasActiveThread = false;
             if (wakeLock != null) {
                 wakeLock.release();
                 wakeLock = null;
@@ -206,10 +200,20 @@ public class DownloadThread extends Thread {
             notifyDownloadCompleted(finalStatus, state.mCountRetry, state.mRetryAfter,
                                     state.mRedirectCount, state.mGotData, state.mFilename,
                                     state.mNewUri, state.mMimeType);
+            mInfo.mHasActiveThread = false;
         }
     }
 
     /**
+     * @return an identical StopRequest but with the cause removed.
+     */
+    private StopRequest removeCause(StopRequest error) {
+        StopRequest newException = new StopRequest(error.mFinalStatus);
+        newException.setStackTrace(error.getStackTrace());
+        return newException;
+    }
+
+    /**
      * Fully execute a single download request - setup and send the request, handle the response,
      * and transfer the data to the destination file.
      */
@@ -242,12 +246,15 @@ public class DownloadThread extends Thread {
     private void checkConnectivity(State state) throws StopRequest {
         int networkUsable = mInfo.checkCanUseNetwork();
         if (networkUsable != DownloadInfo.NETWORK_OK) {
+            int status = Downloads.Impl.STATUS_WAITING_FOR_NETWORK;
             if (networkUsable == DownloadInfo.NETWORK_UNUSABLE_DUE_TO_SIZE) {
+                status = Downloads.Impl.STATUS_QUEUED_FOR_WIFI;
                 mInfo.notifyPauseDueToSize(true);
             } else if (networkUsable == DownloadInfo.NETWORK_RECOMMENDED_UNUSABLE_DUE_TO_SIZE) {
+                status = Downloads.Impl.STATUS_QUEUED_FOR_WIFI;
                 mInfo.notifyPauseDueToSize(false);
             }
-            throw new StopRequest(Downloads.Impl.STATUS_RUNNING_PAUSED);
+            throw new StopRequest(status);
         }
     }
 
@@ -385,7 +392,7 @@ public class DownloadThread extends Thread {
                 if (Constants.LOGV) {
                     Log.v(Constants.TAG, "paused " + mInfo.mUri);
                 }
-                throw new StopRequest(Downloads.Impl.STATUS_RUNNING_PAUSED);
+                throw new StopRequest(Downloads.Impl.STATUS_PAUSED_BY_APP);
             }
         }
         if (mInfo.mStatus == Downloads.Impl.STATUS_CANCELED) {
@@ -766,7 +773,7 @@ public class DownloadThread extends Thread {
                // ignored - retryAfter stays 0 in this case.
            }
         }
-        throw new StopRequest(Downloads.Impl.STATUS_RUNNING_PAUSED);
+        throw new StopRequest(Downloads.Impl.STATUS_WAITING_TO_RETRY);
     }
 
     /**
@@ -801,10 +808,10 @@ public class DownloadThread extends Thread {
         }
 
         if (!Helpers.isNetworkAvailable(mSystemFacade)) {
-            return Downloads.Impl.STATUS_RUNNING_PAUSED;
+            return Downloads.Impl.STATUS_WAITING_FOR_NETWORK;
         } else if (mInfo.mNumFailed < Constants.MAX_RETRIES) {
             state.mCountRetry = true;
-            return Downloads.Impl.STATUS_RUNNING_PAUSED;
+            return Downloads.Impl.STATUS_WAITING_TO_RETRY;
         } else {
             Log.d(Constants.TAG, "reached max retries: " + message + " for " + mInfo.mId);
             return Downloads.Impl.STATUS_HTTP_DATA_ERROR;
index 350c63d..0cb63e0 100644 (file)
@@ -82,7 +82,7 @@ public class DownloadManagerFunctionalTest extends AbstractDownloadManagerFuncti
         downloadUri = requestDownload("/path");
         updateDownload(downloadUri, Downloads.COLUMN_DESTINATION,
                        Integer.toString(Downloads.DESTINATION_CACHE_PARTITION_NOROAMING));
-        runUntilStatus(downloadUri, Downloads.STATUS_RUNNING_PAUSED);
+        runUntilStatus(downloadUri, Downloads.Impl.STATUS_WAITING_FOR_NETWORK);
 
         // ...and pick up when we're off roaming
         enqueueResponse(HTTP_OK, FILE_CONTENT);
index ab7d0e4..cad01df 100644 (file)
@@ -531,7 +531,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         Download download = enqueueRequest(getRequest().setDestinationUri(destination));
         download.runUntilStatus(DownloadManager.STATUS_FAILED);
         assertEquals(DownloadManager.ERROR_FILE_ALREADY_EXISTS,
-                     download.getLongField(DownloadManager.COLUMN_ERROR_CODE));
+                     download.getLongField(DownloadManager.COLUMN_REASON));
     }
 
     public void testEmptyFields() throws Exception {
@@ -542,7 +542,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         assertEquals(0, download.getLongField(DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR));
         assertEquals(-1, download.getLongField(DownloadManager.COLUMN_TOTAL_SIZE_BYTES));
         // just ensure no exception is thrown
-        download.getLongField(DownloadManager.COLUMN_ERROR_CODE);
+        download.getLongField(DownloadManager.COLUMN_REASON);
     }
 
     public void testRestart() throws Exception {
@@ -567,7 +567,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         Download download = enqueueRequest(getRequest());
         download.runUntilStatus(DownloadManager.STATUS_FAILED);
         assertEquals(expectedErrorCode,
-                     download.getLongField(DownloadManager.COLUMN_ERROR_CODE));
+                     download.getLongField(DownloadManager.COLUMN_REASON));
     }
 
     /**
index bb3654a..6bd9f2c 100644 (file)
@@ -38,9 +38,9 @@
 
     <!-- Status messages -->
 
-    <!-- Status indicating that the download has not yet begun.  Appears for an individual item in
-         the download list.  [CHAR LIMIT=11] -->
-    <string name="download_pending">Queued</string>
+    <!-- Status indicating that the download has been queued to start in the future.  Appears for an
+         individual item in the download list.  [CHAR LIMIT=11] -->
+    <string name="download_queued">Queued</string>
     <!-- Status indicating that the system is currently downloading the file.  Appears for an
          individual item in the download list.  [CHAR LIMIT=11] -->
     <string name="download_running">In progress</string>
index c7d4c2c..9c57253 100644 (file)
@@ -57,6 +57,7 @@ public class DownloadAdapter extends CursorAdapter {
     private int mTitleColumnId;
     private int mDescriptionColumnId;
     private int mStatusColumnId;
+    private int mReasonColumnId;
     private int mTotalBytesColumnId;
     private int mMediaTypeColumnId;
     private int mDateColumnId;
@@ -76,6 +77,7 @@ public class DownloadAdapter extends CursorAdapter {
         mTitleColumnId = cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_TITLE);
         mDescriptionColumnId = cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_DESCRIPTION);
         mStatusColumnId = cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_STATUS);
+        mReasonColumnId = cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_REASON);
         mTotalBytesColumnId = cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_TOTAL_SIZE_BYTES);
         mMediaTypeColumnId = cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_MEDIA_TYPE);
         mDateColumnId =
@@ -150,11 +152,15 @@ public class DownloadAdapter extends CursorAdapter {
                 return R.string.download_success;
 
             case DownloadManager.STATUS_PENDING:
-                return R.string.download_pending;
-
             case DownloadManager.STATUS_RUNNING:
-            case DownloadManager.STATUS_PAUSED:
                 return R.string.download_running;
+
+            case DownloadManager.STATUS_PAUSED:
+                if (mCursor.getInt(mReasonColumnId) == DownloadManager.PAUSED_QUEUED_FOR_WIFI) {
+                    return R.string.download_queued;
+                } else {
+                    return R.string.download_running;
+                }
         }
         throw new IllegalStateException("Unknown status: " + mCursor.getInt(mStatusColumnId));
     }
index 4b3547c..bb33153 100644 (file)
@@ -85,7 +85,7 @@ public class DownloadList extends Activity
     private int mIdColumnId;
     private int mLocalUriColumnId;
     private int mMediaTypeColumnId;
-    private int mErrorCodeColumndId;
+    private int mReasonColumndId;
 
     private boolean mIsSortedBySize = false;
     private Set<Long> mSelectedIds = new HashSet<Long>();
@@ -94,8 +94,9 @@ public class DownloadList extends Activity
      * We keep track of when a dialog is being displayed for a pending download, because if that
      * download starts running, we want to immediately hide the dialog.
      */
-    private Long mPendingDownloadId = null;
-    private AlertDialog mPendingDialog;
+    private Long mQueuedDownloadId = null;
+    private AlertDialog mQueuedDialog;
+
 
     private class MyContentObserver extends ContentObserver {
         public MyContentObserver() {
@@ -145,8 +146,8 @@ public class DownloadList extends Activity
                     mDateSortedCursor.getColumnIndexOrThrow(DownloadManager.COLUMN_LOCAL_URI);
             mMediaTypeColumnId =
                     mDateSortedCursor.getColumnIndexOrThrow(DownloadManager.COLUMN_MEDIA_TYPE);
-            mErrorCodeColumndId =
-                    mDateSortedCursor.getColumnIndexOrThrow(DownloadManager.COLUMN_ERROR_CODE);
+            mReasonColumndId =
+                    mDateSortedCursor.getColumnIndexOrThrow(DownloadManager.COLUMN_REASON);
 
             mDateSortedAdapter = new DateSortedDownloadAdapter(this, mDateSortedCursor, this);
             mDateOrderedListView.setAdapter(mDateSortedAdapter);
@@ -359,19 +360,23 @@ public class DownloadList extends Activity
         long id = cursor.getInt(mIdColumnId);
         switch (cursor.getInt(mStatusColumnId)) {
             case DownloadManager.STATUS_PENDING:
-                mPendingDownloadId = id;
-                mPendingDialog = new AlertDialog.Builder(this)
-                        .setTitle(R.string.dialog_title_not_available)
-                        .setMessage(R.string.dialog_queued_body)
-                        .setPositiveButton(R.string.keep_queued_download, null)
-                        .setNegativeButton(R.string.remove_download, getDeleteClickHandler(id))
-                        .setOnCancelListener(this)
-                        .show();
+            case DownloadManager.STATUS_RUNNING:
+                sendRunningDownloadClickedBroadcast(id);
                 break;
 
-            case DownloadManager.STATUS_RUNNING:
             case DownloadManager.STATUS_PAUSED:
-                sendRunningDownloadClickedBroadcast(id);
+                if (isPausedForWifi(cursor)) {
+                    mQueuedDownloadId = id;
+                    mQueuedDialog = new AlertDialog.Builder(this)
+                            .setTitle(R.string.dialog_title_not_available)
+                            .setMessage(R.string.dialog_queued_body)
+                            .setPositiveButton(R.string.keep_queued_download, null)
+                            .setNegativeButton(R.string.remove_download, getDeleteClickHandler(id))
+                            .setOnCancelListener(this)
+                            .show();
+                } else {
+                    sendRunningDownloadClickedBroadcast(id);
+                }
                 break;
 
             case DownloadManager.STATUS_SUCCESSFUL:
@@ -388,7 +393,7 @@ public class DownloadList extends Activity
      * @return the appropriate error message for the failed download pointed to by cursor
      */
     private String getErrorMessage(Cursor cursor) {
-        switch (cursor.getInt(mErrorCodeColumndId)) {
+        switch (cursor.getInt(mReasonColumndId)) {
             case DownloadManager.ERROR_FILE_ALREADY_EXISTS:
                 if (isOnExternalStorage(cursor)) {
                     return getString(R.string.dialog_file_already_exists);
@@ -616,13 +621,18 @@ public class DownloadList extends Activity
     void handleDownloadsChanged() {
         checkSelectionForDeletedEntries();
 
-        if (mPendingDownloadId != null && moveToDownload(mPendingDownloadId)) {
-            if (mDateSortedCursor.getInt(mStatusColumnId) != DownloadManager.STATUS_PENDING) {
-                mPendingDialog.cancel();
+        if (mQueuedDownloadId != null && moveToDownload(mQueuedDownloadId)) {
+            if (mDateSortedCursor.getInt(mStatusColumnId) != DownloadManager.STATUS_PAUSED
+                    || !isPausedForWifi(mDateSortedCursor)) {
+                mQueuedDialog.cancel();
             }
         }
     }
 
+    private boolean isPausedForWifi(Cursor cursor) {
+        return cursor.getInt(mReasonColumndId) == DownloadManager.PAUSED_QUEUED_FOR_WIFI;
+    }
+
     /**
      * Check if any of the selected downloads have been deleted from the downloads database, and
      * remove such downloads from the selection.
@@ -662,7 +672,7 @@ public class DownloadList extends Activity
      */
     @Override
     public void onCancel(DialogInterface dialog) {
-        mPendingDownloadId = null;
-        mPendingDialog = null;
+        mQueuedDownloadId = null;
+        mQueuedDialog = null;
     }
 }