From b2c63c6dc71b2aa1b5087ee08f5b708cc6f03560 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sat, 24 May 2014 08:42:49 +0930 Subject: [PATCH] More robust multi-threaded downloading. ApkDownloaders keep track of a unique id. This id is passed through with each event coming from the downloader. When progress events are received, if they don't have the same id as the current downloader, they are ignored. When returning to the app details screen after leaving, if a download completed in the mean time, automatically show the install screen. Otherwise, when you, e.g. return to your devices home screen during a download, the download keeps occuring in the background, but we are unable to make use of the resulting downloaded file. --- src/org/fdroid/fdroid/AppDetails.java | 58 +++++++++++++++++--- src/org/fdroid/fdroid/net/ApkDownloader.java | 31 ++++++++++- 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index e56e6ce79..fca4c0b10 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -433,13 +433,18 @@ public class AppDetails extends ListActivity implements ProgressListener { AppProvider.getContentUri(app.id), true, myAppObserver); - if (downloadHandler != null) { - downloadHandler.setProgressListener(this); + if (downloadHandler != null) { + if (downloadHandler.isComplete()) { + downloadCompleteInstallApk(); + } else { + downloadHandler.setProgressListener(this); - // Show the progress dialog, if for no other reason than to prevent them attempting - // to download again (i.e. we force them to touch 'cancel' before they can access - // the rest of the activity). - updateProgressDialog(); + // Show the progress dialog, if for no other reason than to prevent them attempting + // to download again (i.e. we force them to touch 'cancel' before they can access + // the rest of the activity). + Log.d(TAG, "Showing dialog to user after resuming app details view, because a download was previously in progress"); + updateProgressDialog(); + } } updateViews(); @@ -447,6 +452,29 @@ public class AppDetails extends ListActivity implements ProgressListener { MenuManager.create(this).invalidateOptionsMenu(); } + /** + * Remove progress listener, suppress progress dialog, set downloadHandler to null. + */ + private void cleanUpFinishedDownload() { + if (downloadHandler != null) { + downloadHandler.removeProgressListener(); + removeProgressDialog(); + downloadHandler = null; + } + } + + /** + * Once the download completes successfully, call this method to start the install process + * with the file that was downloaded. + */ + private void downloadCompleteInstallApk() { + if (downloadHandler != null) { + assert downloadHandler.isComplete(); + installApk(downloadHandler.localFile(), downloadHandler.getApk().id); + cleanUpFinishedDownload(); + } + } + @Override protected void onPause() { if (myAppObserver != null) { @@ -491,6 +519,7 @@ public class AppDetails extends ListActivity implements ProgressListener { if (downloadHandler != null) { if (!inProcessOfChangingConfiguration) { downloadHandler.cancel(); + cleanUpFinishedDownload(); } } inProcessOfChangingConfiguration = false; @@ -1024,6 +1053,7 @@ public class AppDetails extends ListActivity implements ProgressListener { updateProgressDialog(); } } + private void installApk(File file, String packageName) { setProgressBarIndeterminateVisibility(true); @@ -1120,7 +1150,7 @@ public class AppDetails extends ListActivity implements ProgressListener { Log.d(TAG, "User clicked 'cancel' on download, attempting to interrupt download thread."); if (downloadHandler != null) { downloadHandler.cancel(); - downloadHandler = null; + cleanUpFinishedDownload(); } else { Log.e(TAG, "Tried to cancel, but the downloadHandler doesn't exist."); } @@ -1175,6 +1205,18 @@ public class AppDetails extends ListActivity implements ProgressListener { @Override public void onProgress(Event event) { + if (downloadHandler == null || !downloadHandler.isEventFromThis(event)) { + // Choose not to respond to events from previous downloaders. + // We don't even care if we receive "cancelled" events or the like, because + // we dealt with cancellations in the onCancel listener of the dialog, + // rather than waiting to receive the event here. We try and be careful in + // the download thread to make sure that we check for cancellations before + // sending events, but it is not possible to be perfect, because the interruption + // which triggers the download can happen after the check to see if + Log.d(TAG, "Discarding downloader event \"" + event.type + "\" as it is from an old (probably cancelled) downloader."); + return; + } + boolean finished = false; if (event.type.equals(Downloader.EVENT_PROGRESS)) { updateProgressDialog(event.progress, event.total); @@ -1188,7 +1230,7 @@ public class AppDetails extends ListActivity implements ProgressListener { Toast.makeText(this, text, Toast.LENGTH_LONG).show(); finished = true; } else if (event.type.equals(ApkDownloader.EVENT_APK_DOWNLOAD_COMPLETE)) { - installApk(downloadHandler.localFile(), downloadHandler.getApk().id); + downloadCompleteInstallApk(); finished = true; } diff --git a/src/org/fdroid/fdroid/net/ApkDownloader.java b/src/org/fdroid/fdroid/net/ApkDownloader.java index 2dffe9507..baddfd5ef 100644 --- a/src/org/fdroid/fdroid/net/ApkDownloader.java +++ b/src/org/fdroid/fdroid/net/ApkDownloader.java @@ -48,6 +48,9 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { public static final int ERROR_DOWNLOAD_FAILED = 102; public static final int ERROR_UNKNOWN = 103; + private static final String EVENT_SOURCE_ID = "sourceId"; + private static long downloadIdCounter = 0; + /** * Used as a key to pass data through with an error event, explaining the type of event. */ @@ -61,6 +64,9 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { private AsyncDownloadWrapper dlWrapper = null; private int progress = 0; private int totalSize = 0; + private boolean isComplete = false; + + private long id = ++downloadIdCounter; public void setProgressListener(ProgressListener listener) { this.listener = listener; @@ -83,6 +89,15 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { return localFile; } + /** + * When stopping/starting downloaders multiple times (on different threads), it can + * get weird whereby different threads are sending progress events. It is important + * to be able to see which downloader these progress events are coming from. + */ + public boolean isEventFromThis(Event event) { + return event.getData().containsKey(EVENT_SOURCE_ID) && event.getData().getLong(EVENT_SOURCE_ID) == id; + } + public String getRemoteAddress() { return repoAddress + "/" + curApk.apkName.replace(" ", "%20"); } @@ -131,6 +146,15 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { } } + private void sendCompleteMessage() { + isComplete = true; + sendMessage(EVENT_APK_DOWNLOAD_COMPLETE); + } + + public boolean isComplete() { + return this.isComplete; + } + /** * If the download successfully spins up a new thread to start downloading, then we return * true, otherwise false. This is useful, e.g. when we use a cached version, and so don't @@ -140,7 +164,7 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { // Can we use the cached version? if (verifyOrDeleteCachedVersion()) { - sendMessage(EVENT_APK_DOWNLOAD_COMPLETE); + sendCompleteMessage(); return false; } @@ -181,6 +205,8 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { progress = event.progress; } + event.getData().putLong(EVENT_SOURCE_ID, id); + if (listener != null) { listener.onProgress(event); } @@ -214,7 +240,7 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { } Log.d("FDroid", "Download finished: " + localFile); - sendMessage(EVENT_APK_DOWNLOAD_COMPLETE); + sendCompleteMessage(); } @Override @@ -232,7 +258,6 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { * listener (to prevent */ public void cancel() { - removeProgressListener(); if (dlWrapper != null) { dlWrapper.attemptCancel(); }