From 2c80f1a758f7fc6c98b76e0d85bd1b387dbac1ea Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sun, 23 Feb 2014 18:08:24 +1100 Subject: [PATCH 01/17] Split net.Downloader into abstract Downloader and concrete HttpDownloader. This will allow the more general, non HTTP related stuff (progress events, stream copying) to occur in a separate base class. HTTP specific stuff (HTTP status codes, etag cache checking) is done in the HTTPDownloader base class. --- src/org/fdroid/fdroid/net/Downloader.java | 107 ++++------------ src/org/fdroid/fdroid/net/HttpDownloader.java | 115 ++++++++++++++++++ .../fdroid/fdroid/updater/RepoUpdater.java | 11 +- 3 files changed, 142 insertions(+), 91 deletions(-) create mode 100644 src/org/fdroid/fdroid/net/HttpDownloader.java diff --git a/src/org/fdroid/fdroid/net/Downloader.java b/src/org/fdroid/fdroid/net/Downloader.java index 27fc6bb79..922256c3f 100644 --- a/src/org/fdroid/fdroid/net/Downloader.java +++ b/src/org/fdroid/fdroid/net/Downloader.java @@ -5,24 +5,18 @@ import java.net.*; import android.content.*; import org.fdroid.fdroid.*; -public class Downloader { +public abstract class Downloader { - private static final String HEADER_IF_NONE_MATCH = "If-None-Match"; - private static final String HEADER_FIELD_ETAG = "ETag"; - - private URL sourceUrl; private OutputStream outputStream; private ProgressListener progressListener = null; private ProgressListener.Event progressEvent = null; - private String eTag = null; private final File outputFile; - private HttpURLConnection connection; - private int statusCode = -1; + + public abstract InputStream inputStream() throws IOException; // The context is required for opening the file to write to. - public Downloader(String source, String destFile, Context ctx) + public Downloader(String destFile, Context ctx) throws FileNotFoundException, MalformedURLException { - sourceUrl = new URL(source); outputStream = ctx.openFileOutput(destFile, Context.MODE_PRIVATE); outputFile = new File(ctx.getFilesDir() + File.separator + destFile); } @@ -32,16 +26,14 @@ public class Downloader { * you are done*. * @see org.fdroid.fdroid.net.Downloader#getFile() */ - public Downloader(String source, Context ctx) throws IOException { + public Downloader(Context ctx) throws IOException { // http://developer.android.com/guide/topics/data/data-storage.html#InternalCache outputFile = File.createTempFile("dl-", "", ctx.getCacheDir()); outputStream = new FileOutputStream(outputFile); - sourceUrl = new URL(source); } - public Downloader(String source, OutputStream output) + public Downloader(OutputStream output) throws MalformedURLException { - sourceUrl = new URL(source); outputStream = output; outputFile = null; } @@ -61,82 +53,25 @@ public class Downloader { return outputFile; } - /** - * Only available after downloading a file. - */ - public int getStatusCode() { - return statusCode; - } - - /** - * If you ask for the eTag before calling download(), you will get the - * same one you passed in (if any). If you call it after download(), you - * will get the new eTag from the server, or null if there was none. - */ - public String getETag() { - return eTag; - } - - /** - * If this eTag matches that returned by the server, then no download will - * take place, and a status code of 304 will be returned by download(). - */ - public void setETag(String eTag) { - this.eTag = eTag; - } - - // Get a remote file. Returns the HTTP response code. - // If 'etag' is not null, it's passed to the server as an If-None-Match - // header, in which case expect a 304 response if nothing changed. - // In the event of a 200 response ONLY, 'retag' (which should be passed - // empty) may contain an etag value for the response, or it may be left - // empty if none was available. - public int download() throws IOException { - connection = (HttpURLConnection)sourceUrl.openConnection(); - setupCacheCheck(); - statusCode = connection.getResponseCode(); - if (statusCode == 200) { - setupProgressListener(); - InputStream input = null; - try { - input = connection.getInputStream(); - Utils.copy(input, outputStream, - progressListener, progressEvent); - } finally { - Utils.closeQuietly(outputStream); - Utils.closeQuietly(input); - } - updateCacheCheck(); - } - return statusCode; - } - - protected void setupCacheCheck() { - if (eTag != null) { - connection.setRequestProperty(HEADER_IF_NONE_MATCH, eTag); - } - } - - protected void updateCacheCheck() { - eTag = connection.getHeaderField(HEADER_FIELD_ETAG); - } - - protected void setupProgressListener() { + private void setupProgressListener() { if (progressListener != null && progressEvent != null) { - // Testing in the emulator for me, showed that figuring out the - // filesize took about 1 to 1.5 seconds. - // To put this in context, downloading a repo of: - // - 400k takes ~6 seconds - // - 5k takes ~3 seconds - // on my connection. I think the 1/1.5 seconds is worth it, - // because as the repo grows, the tradeoff will - // become more worth it. - progressEvent.total = connection.getContentLength(); + progressEvent.total = totalDownloadSize(); } } - public boolean hasChanged() { - return this.statusCode == 200; + protected abstract int totalDownloadSize(); + + public void download() throws IOException { + setupProgressListener(); + InputStream input = null; + try { + input = inputStream(); + Utils.copy(input, outputStream, + progressListener, progressEvent); + } finally { + Utils.closeQuietly(outputStream); + Utils.closeQuietly(input); + } } } diff --git a/src/org/fdroid/fdroid/net/HttpDownloader.java b/src/org/fdroid/fdroid/net/HttpDownloader.java new file mode 100644 index 000000000..363ff5f49 --- /dev/null +++ b/src/org/fdroid/fdroid/net/HttpDownloader.java @@ -0,0 +1,115 @@ +package org.fdroid.fdroid.net; + +import android.content.Context; + +import java.io.*; +import java.net.HttpURLConnection; +import java.net.MalformedURLException; +import java.net.URL; + +public class HttpDownloader extends Downloader { + + private static final String HEADER_IF_NONE_MATCH = "If-None-Match"; + private static final String HEADER_FIELD_ETAG = "ETag"; + + private URL sourceUrl; + private String eTag = null; + private HttpURLConnection connection; + private int statusCode = -1; + + // The context is required for opening the file to write to. + public HttpDownloader(String source, String destFile, Context ctx) + throws FileNotFoundException, MalformedURLException { + super(destFile, ctx); + sourceUrl = new URL(source); + } + + /** + * Downloads to a temporary file, which *you must delete yourself when + * you are done*. + * @see org.fdroid.fdroid.net.HttpDownloader#getFile() + */ + public HttpDownloader(String source, Context ctx) throws IOException { + super(ctx); + sourceUrl = new URL(source); + } + + public HttpDownloader(String source, OutputStream output) + throws MalformedURLException { + super(output); + sourceUrl = new URL(source); + } + + public InputStream inputStream() throws IOException { + return connection.getInputStream(); + } + + // Get a remote file. Returns the HTTP response code. + // If 'etag' is not null, it's passed to the server as an If-None-Match + // header, in which case expect a 304 response if nothing changed. + // In the event of a 200 response ONLY, 'retag' (which should be passed + // empty) may contain an etag value for the response, or it may be left + // empty if none was available. + public int downloadHttpFile() throws IOException { + connection = (HttpURLConnection)sourceUrl.openConnection(); + setupCacheCheck(); + statusCode = connection.getResponseCode(); + if (statusCode == 200) { + download(); + updateCacheCheck(); + } + return statusCode; + } + + /** + * Only available after downloading a file. + */ + public int getStatusCode() { + return statusCode; + } + + /** + * If you ask for the eTag before calling download(), you will get the + * same one you passed in (if any). If you call it after download(), you + * will get the new eTag from the server, or null if there was none. + */ + public String getETag() { + return eTag; + } + + /** + * If this eTag matches that returned by the server, then no download will + * take place, and a status code of 304 will be returned by download(). + */ + public void setETag(String eTag) { + this.eTag = eTag; + } + + + protected void setupCacheCheck() { + if (eTag != null) { + connection.setRequestProperty(HEADER_IF_NONE_MATCH, eTag); + } + } + + protected void updateCacheCheck() { + eTag = connection.getHeaderField(HEADER_FIELD_ETAG); + } + + // Testing in the emulator for me, showed that figuring out the + // filesize took about 1 to 1.5 seconds. + // To put this in context, downloading a repo of: + // - 400k takes ~6 seconds + // - 5k takes ~3 seconds + // on my connection. I think the 1/1.5 seconds is worth it, + // because as the repo grows, the tradeoff will + // become more worth it. + protected int totalDownloadSize() { + return connection.getContentLength(); + } + + public boolean hasChanged() { + return this.statusCode == 200; + } + +} diff --git a/src/org/fdroid/fdroid/updater/RepoUpdater.java b/src/org/fdroid/fdroid/updater/RepoUpdater.java index 486798154..a5817cc50 100644 --- a/src/org/fdroid/fdroid/updater/RepoUpdater.java +++ b/src/org/fdroid/fdroid/updater/RepoUpdater.java @@ -12,6 +12,7 @@ import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.RepoProvider; import org.fdroid.fdroid.net.Downloader; +import org.fdroid.fdroid.net.HttpDownloader; import org.xml.sax.InputSource; import org.xml.sax.SAXException; import org.xml.sax.XMLReader; @@ -84,11 +85,11 @@ abstract public class RepoUpdater { protected abstract String getIndexAddress(); - protected Downloader downloadIndex() throws UpdateException { + protected HttpDownloader downloadIndex() throws UpdateException { Bundle progressData = createProgressData(repo.address); - Downloader downloader = null; + HttpDownloader downloader = null; try { - downloader = new Downloader(getIndexAddress(), context); + downloader = new HttpDownloader(getIndexAddress(), context); downloader.setETag(repo.lastetag); if (isInteractive()) { @@ -98,7 +99,7 @@ abstract public class RepoUpdater { downloader.setProgressListener(progressListener, event); } - int status = downloader.download(); + int status = downloader.downloadHttpFile(); if (status == 304) { // The index is unchanged since we last read it. We just mark @@ -169,7 +170,7 @@ abstract public class RepoUpdater { File indexFile = null; try { - Downloader downloader = downloadIndex(); + HttpDownloader downloader = downloadIndex(); hasChanged = downloader.hasChanged(); if (hasChanged) { From c4b0eb9b510feb9b6ff4484c313dac67ab5311f6 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 24 Feb 2014 19:54:57 -0500 Subject: [PATCH 02/17] .Downloader --> .ApkDownloader to distinguish from .net.Downloader org.fdroid.froid.Downloader is only used for downloading APKs, so name it appropriately. refs #2598 https://dev.guardianproject.info/issues/2598 --- .../fdroid/fdroid/{Downloader.java => ApkDownloader.java} | 4 ++-- src/org/fdroid/fdroid/AppDetails.java | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) rename src/org/fdroid/fdroid/{Downloader.java => ApkDownloader.java} (98%) diff --git a/src/org/fdroid/fdroid/Downloader.java b/src/org/fdroid/fdroid/ApkDownloader.java similarity index 98% rename from src/org/fdroid/fdroid/Downloader.java rename to src/org/fdroid/fdroid/ApkDownloader.java index 22bcac17e..0e99249b6 100644 --- a/src/org/fdroid/fdroid/Downloader.java +++ b/src/org/fdroid/fdroid/ApkDownloader.java @@ -29,7 +29,7 @@ import java.net.URL; import android.util.Log; import org.fdroid.fdroid.data.Apk; -public class Downloader extends Thread { +public class ApkDownloader extends Thread { private Apk curapk; private String repoaddress; @@ -53,7 +53,7 @@ public class Downloader extends Thread { // Constructor - creates a Downloader to download the given Apk, // which must have its detail populated. - Downloader(Apk apk, String repoaddress, File destdir) { + ApkDownloader(Apk apk, String repoaddress, File destdir) { curapk = apk; this.repoaddress = repoaddress; this.destdir = destdir; diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index 96e82b6bf..7638242b5 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -1072,14 +1072,14 @@ public class AppDetails extends ListActivity { // Handler used to update the progress dialog while downloading. private class DownloadHandler extends Handler { - private Downloader download; + private ApkDownloader download; private ProgressDialog pd; private boolean updating; private String id; public DownloadHandler(Apk apk, String repoaddress, File destdir) { id = apk.id; - download = new Downloader(apk, repoaddress, destdir); + download = new ApkDownloader(apk, repoaddress, destdir); download.start(); startUpdates(); } @@ -1106,7 +1106,7 @@ public class AppDetails extends ListActivity { if (pd != null) pd.dismiss(); String text; - if (download.getErrorType() == Downloader.Error.CORRUPT) + if (download.getErrorType() == ApkDownloader.Error.CORRUPT) text = getString(R.string.corrupt_download); else text = download.getErrorMessage(); From a44ce0e4e780a3d11762f22759d57e50ab05dc87 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 20 May 2014 05:34:08 +0930 Subject: [PATCH 03/17] Rough guess at what ApkDownloader refactor could look like. --- .gitignore | 2 +- src/org/fdroid/fdroid/ApkDownloader.java | 147 +++++------------- src/org/fdroid/fdroid/AppDetails.java | 112 +++++-------- src/org/fdroid/fdroid/net/Downloader.java | 31 +++- src/org/fdroid/fdroid/net/HttpDownloader.java | 7 + 5 files changed, 105 insertions(+), 194 deletions(-) diff --git a/.gitignore b/.gitignore index 22273c3f4..9708725fd 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,6 @@ /build.xml *~ /.idea/ -/*.iml +*.iml out /.settings/ diff --git a/src/org/fdroid/fdroid/ApkDownloader.java b/src/org/fdroid/fdroid/ApkDownloader.java index 0e99249b6..bef04e314 100644 --- a/src/org/fdroid/fdroid/ApkDownloader.java +++ b/src/org/fdroid/fdroid/ApkDownloader.java @@ -20,37 +20,29 @@ package org.fdroid.fdroid; -import java.io.File; -import java.io.FileOutputStream; -import java.io.InputStream; -import java.io.OutputStream; -import java.net.URL; +import java.io.*; import android.util.Log; import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.net.HttpDownloader; public class ApkDownloader extends Thread { + public static final int EVENT_APK_DOWNLOAD_COMPLETE = 100; + public static final int EVENT_ERROR_HASH_MISMATCH = 101; + public static final int EVENT_ERROR_DOWNLOAD_FAILED = 102; + public static final int EVENT_ERROR_UNKNOWN = 103; private Apk curapk; private String repoaddress; - private String filename; private File destdir; private File localfile; - public static enum Status { - STARTING, RUNNING, ERROR, DONE, CANCELLED - } + private ProgressListener listener; - public static enum Error { - CORRUPT, UNKNOWN + public void setProgressListener(ProgressListener listener) { + this.listener = listener; } - private Status status = Status.STARTING; - private Error error; - private int progress; - private int max; - private String errorMessage; - // Constructor - creates a Downloader to download the given Apk, // which must have its detail populated. ApkDownloader(Apk apk, String repoaddress, File destdir) { @@ -59,50 +51,19 @@ public class ApkDownloader extends Thread { this.destdir = destdir; } - public synchronized Status getStatus() { - return status; - } - - // Current progress and maximum value for progress dialog - public synchronized int getProgress() { - return progress; - } - - public synchronized int getMax() { - return max; - } - - // Error code and error message, only valid if status is ERROR - public synchronized Error getErrorType() { - return error; - } - - public synchronized String getErrorMessage() { - return errorMessage; - } - - // The URL being downloaded or path to a cached file - public synchronized String remoteFile() { - return filename; - } - // The downloaded APK. Valid only when getStatus() has returned STATUS.DONE. public File localFile() { return localfile; } - // The APK being downloaded - public synchronized Apk getApk() { - return curapk; + public String remoteFile() { + return repoaddress + "/" + curapk.apkName.replace(" ", "%20"); } @Override public void run() { + localfile = new File(destdir, curapk.apkName); - InputStream input = null; - OutputStream output = null; - String apkname = curapk.apkName; - localfile = new File(destdir, apkname); try { // See if we already have this apk cached... @@ -111,12 +72,7 @@ public class ApkDownloader extends Thread { Hasher hash = new Hasher(curapk.hashType, localfile); if (hash.match(curapk.hash)) { Log.d("FDroid", "Using cached apk at " + localfile); - synchronized (this) { - progress = 1; - max = 1; - status = Status.DONE; - return; - } + return; } else { Log.d("FDroid", "Not using cached apk at " + localfile); localfile.delete(); @@ -124,72 +80,45 @@ public class ApkDownloader extends Thread { } // If we haven't got the apk locally, we'll have to download it... - String remotefile; - remotefile = repoaddress + "/" + apkname.replace(" ", "%20"); - Log.d("FDroid", "Downloading apk from " + remotefile); - synchronized (this) { - filename = remotefile; - progress = 0; - max = curapk.size; - status = Status.RUNNING; - } - input = new URL(remotefile).openStream(); - output = new FileOutputStream(localfile); - byte data[] = new byte[Utils.BUFFER_SIZE]; - while (true) { - if (isInterrupted()) { - Log.d("FDroid", "Download cancelled!"); - break; - } - int count = input.read(data); - if (count == -1) { - break; - } - output.write(data, 0, count); - synchronized (this) { - progress += count; - } - } + HttpDownloader downloader = new HttpDownloader(remoteFile(), localfile); + downloader.setProgressListener(listener); - if (isInterrupted()) { - localfile.delete(); - synchronized (this) { - status = Status.CANCELLED; - } + Log.d("FDroid", "Downloading apk from " + remoteFile()); + int httpStatus = downloader.downloadHttpFile(); + + if (httpStatus != 200 || !localfile.exists()) { + sendProgress(EVENT_ERROR_DOWNLOAD_FAILED); return; } + Hasher hash = new Hasher(curapk.hashType, localfile); if (!hash.match(curapk.hash)) { - synchronized (this) { - Log.d("FDroid", "Downloaded file hash of " + hash.getHash() - + " did not match repo's " + curapk.hash); - // No point keeping a bad file, whether we're - // caching or not. - localfile.delete(); - error = Error.CORRUPT; - errorMessage = null; - status = Status.ERROR; - return; - } + Log.d("FDroid", "Downloaded file hash of " + hash.getHash() + + " did not match repo's " + curapk.hash); + // No point keeping a bad file, whether we're + // caching or not. + localfile.delete(); + sendProgress(EVENT_ERROR_HASH_MISMATCH); + return; } } catch (Exception e) { Log.e("FDroid", "Download failed:\n" + Log.getStackTraceString(e)); - synchronized (this) { + if (localfile.exists()) { localfile.delete(); - error = Error.UNKNOWN; - errorMessage = e.toString(); - status = Status.ERROR; - return; } - } finally { - Utils.closeQuietly(output); - Utils.closeQuietly(input); + sendProgress(EVENT_ERROR_UNKNOWN); + return; } Log.d("FDroid", "Download finished: " + localfile); - synchronized (this) { - status = Status.DONE; + sendProgress(EVENT_APK_DOWNLOAD_COMPLETE); + } + + private void sendProgress(int type) { + if (listener != null) { + listener.onProgress(new ProgressListener.Event(type)); } } + } diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index 7638242b5..63af2b9d7 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -26,6 +26,7 @@ import org.fdroid.fdroid.data.*; import org.fdroid.fdroid.installer.Installer; import org.fdroid.fdroid.installer.Installer.AndroidNotCompatibleException; import org.fdroid.fdroid.installer.Installer.InstallerCallback; +import org.fdroid.fdroid.net.Downloader; import org.xml.sax.XMLReader; import android.app.AlertDialog; @@ -394,10 +395,6 @@ public class AppDetails extends ListActivity { updateViews(); MenuManager.create(this).invalidateOptionsMenu(); - - if (downloadHandler != null) { - downloadHandler.startUpdates(); - } } @Override @@ -405,9 +402,6 @@ public class AppDetails extends ListActivity { if (myAppObserver != null) { getContentResolver().unregisterContentObserver(myAppObserver); } - if (downloadHandler != null) { - downloadHandler.stopUpdates(); - } if (app != null && (app.ignoreAllUpdates != startingIgnoreAll || app.ignoreThisUpdate != startingIgnoreThis)) { setIgnoreUpdates(app.id, app.ignoreAllUpdates, app.ignoreThisUpdate); @@ -1039,7 +1033,7 @@ public class AppDetails extends ListActivity { shareIntent.setType("text/plain"); shareIntent.putExtra(Intent.EXTRA_SUBJECT, app.name); - shareIntent.putExtra(Intent.EXTRA_TEXT, app.name+" ("+app.summary+") - https://f-droid.org/app/"+app.id); + shareIntent.putExtra(Intent.EXTRA_TEXT, app.name + " (" + app.summary + ") - https://f-droid.org/app/" + app.id); startActivity(Intent.createChooser(shareIntent, getString(R.string.menu_share))); } @@ -1071,81 +1065,61 @@ public class AppDetails extends ListActivity { } // Handler used to update the progress dialog while downloading. - private class DownloadHandler extends Handler { + private class DownloadHandler extends Handler implements ProgressListener { private ApkDownloader download; private ProgressDialog pd; - private boolean updating; private String id; public DownloadHandler(Apk apk, String repoaddress, File destdir) { id = apk.id; download = new ApkDownloader(apk, repoaddress, destdir); download.start(); - startUpdates(); } public DownloadHandler(DownloadHandler oldHandler) { if (oldHandler != null) { download = oldHandler.download; } - startUpdates(); } - public boolean updateProgress() { + public void onProgress(ProgressListener.Event event) { boolean finished = false; - switch (download.getStatus()) { - case RUNNING: - if (pd == null) { - pd = createProgressDialog(download.remoteFile(), - download.getProgress(), download.getMax()); - } else { - pd.setProgress(download.getProgress()); - } - break; - case ERROR: - if (pd != null) - pd.dismiss(); - String text; - if (download.getErrorType() == ApkDownloader.Error.CORRUPT) - text = getString(R.string.corrupt_download); - else - text = download.getErrorMessage(); - Toast.makeText(AppDetails.this, text, Toast.LENGTH_LONG).show(); - finished = true; - break; - case DONE: - if (pd != null) - pd.dismiss(); - installApk(download.localFile(), id); - finished = true; - break; - case CANCELLED: - Toast.makeText(AppDetails.this, - getString(R.string.download_cancelled), - Toast.LENGTH_SHORT).show(); - finished = true; - break; - default: - break; - } - return finished; - } + switch (event.type) { + case Downloader.EVENT_PROGRESS: + if (pd == null) { + pd = createProgressDialog(download.remoteFile(), + event.progress, event.total); + } else { + pd.setProgress(event.progress); + } + break; - public void startUpdates() { - if (!updating) { - updating = true; - sendEmptyMessage(0); - } - } + case ApkDownloader.EVENT_ERROR_DOWNLOAD_FAILED: + case ApkDownloader.EVENT_ERROR_HASH_MISMATCH: + case ApkDownloader.EVENT_ERROR_UNKNOWN: + String text; + if (event.type == ApkDownloader.EVENT_ERROR_HASH_MISMATCH) + text = getString(R.string.corrupt_download); + else + text = getString(R.string.details_notinstalled); + Toast.makeText(AppDetails.this, text, Toast.LENGTH_LONG).show(); + finished = true; + break; - public void stopUpdates() { - updating = false; - removeMessages(0); + case ApkDownloader.EVENT_APK_DOWNLOAD_COMPLETE: + installApk(download.localFile(), id); + finished = true; + break; + + } + + if (finished) { + destroy(); + } } public void cancel() { - if (download != null) - download.interrupt(); + // TODO: Re-implement... } public void destroy() { @@ -1155,24 +1129,10 @@ public class AppDetails extends ListActivity { pd.dismiss(); pd = null; } - // Cancel any scheduled updates so that we don't - // accidentally recreate the progress dialog. - stopUpdates(); - } - - // Repeatedly run updateProgress() until it's finished. - @Override - public void handleMessage(Message msg) { - if (download == null) - return; - boolean finished = updateProgress(); - if (finished) - download = null; - else - sendMessageDelayed(obtainMessage(), 50); } } + @Override protected void onActivityResult(int requestCode, int resultCode, Intent data) { // handle cases for install manager first diff --git a/src/org/fdroid/fdroid/net/Downloader.java b/src/org/fdroid/fdroid/net/Downloader.java index 922256c3f..304e3640b 100644 --- a/src/org/fdroid/fdroid/net/Downloader.java +++ b/src/org/fdroid/fdroid/net/Downloader.java @@ -10,15 +10,28 @@ public abstract class Downloader { private OutputStream outputStream; private ProgressListener progressListener = null; private ProgressListener.Event progressEvent = null; - private final File outputFile; + private File outputFile; + + public static final int EVENT_PROGRESS = 1; public abstract InputStream inputStream() throws IOException; // The context is required for opening the file to write to. public Downloader(String destFile, Context ctx) throws FileNotFoundException, MalformedURLException { - outputStream = ctx.openFileOutput(destFile, Context.MODE_PRIVATE); - outputFile = new File(ctx.getFilesDir() + File.separator + destFile); + this(new File(ctx.getFilesDir() + File.separator + destFile)); + } + + // The context is required for opening the file to write to. + public Downloader(Context ctx) throws IOException { + this(File.createTempFile("dl-", "", ctx.getCacheDir())); + } + + public Downloader(File destFile) + throws FileNotFoundException, MalformedURLException { + // http://developer.android.com/guide/topics/data/data-storage.html#InternalCache + outputFile = destFile; + outputStream = new FileOutputStream(outputFile); } /** @@ -26,10 +39,7 @@ public abstract class Downloader { * you are done*. * @see org.fdroid.fdroid.net.Downloader#getFile() */ - public Downloader(Context ctx) throws IOException { - // http://developer.android.com/guide/topics/data/data-storage.html#InternalCache - outputFile = File.createTempFile("dl-", "", ctx.getCacheDir()); - outputStream = new FileOutputStream(outputFile); + public Downloader(File destFile, Context ctx) throws IOException { } public Downloader(OutputStream output) @@ -38,6 +48,11 @@ public abstract class Downloader { outputFile = null; } + public void setProgressListener(ProgressListener listener) { + this.progressListener = listener; + this.progressEvent = new ProgressListener.Event(EVENT_PROGRESS, totalDownloadSize()); + } + public void setProgressListener(ProgressListener progressListener, ProgressListener.Event progressEvent) { this.progressListener = progressListener; @@ -61,7 +76,7 @@ public abstract class Downloader { protected abstract int totalDownloadSize(); - public void download() throws IOException { + protected void download() throws IOException { setupProgressListener(); InputStream input = null; try { diff --git a/src/org/fdroid/fdroid/net/HttpDownloader.java b/src/org/fdroid/fdroid/net/HttpDownloader.java index 363ff5f49..74347ae0b 100644 --- a/src/org/fdroid/fdroid/net/HttpDownloader.java +++ b/src/org/fdroid/fdroid/net/HttpDownloader.java @@ -24,6 +24,13 @@ public class HttpDownloader extends Downloader { sourceUrl = new URL(source); } + // The context is required for opening the file to write to. + public HttpDownloader(String source, File destFile) + throws FileNotFoundException, MalformedURLException { + super(destFile); + sourceUrl = new URL(source); + } + /** * Downloads to a temporary file, which *you must delete yourself when * you are done*. From d9f9b682d481aa98949a9ef99b8997cfd6520e31 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 25 Feb 2014 14:56:48 -0500 Subject: [PATCH 04/17] simplify ProgressListener.Event creation and use This aims to simplify the creation and use of Event objects among classes that implement the ProgressListener Interface. This is also needed in order to split out the downloading support from various places and put it all into a centralized refs #2598 https://dev.guardianproject.info/issues/2598 https://gitorious.org/f-droid/fdroidclient/merge_requests/29 --- src/org/fdroid/fdroid/ApkDownloader.java | 12 ++++-- src/org/fdroid/fdroid/ProgressListener.java | 39 +++++++++++-------- src/org/fdroid/fdroid/RepoXMLHandler.java | 6 +-- src/org/fdroid/fdroid/UpdateService.java | 5 +-- src/org/fdroid/fdroid/net/Downloader.java | 5 --- .../fdroid/fdroid/updater/RepoUpdater.java | 16 +------- 6 files changed, 37 insertions(+), 46 deletions(-) diff --git a/src/org/fdroid/fdroid/ApkDownloader.java b/src/org/fdroid/fdroid/ApkDownloader.java index bef04e314..429d8e9fc 100644 --- a/src/org/fdroid/fdroid/ApkDownloader.java +++ b/src/org/fdroid/fdroid/ApkDownloader.java @@ -24,9 +24,11 @@ import java.io.*; import android.util.Log; import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.HttpDownloader; public class ApkDownloader extends Thread { + private static final String TAG = "ApkDownloader"; public static final int EVENT_APK_DOWNLOAD_COMPLETE = 100; public static final int EVENT_ERROR_HASH_MISMATCH = 101; @@ -80,11 +82,15 @@ public class ApkDownloader extends Thread { } // If we haven't got the apk locally, we'll have to download it... + String remoteAddress = remoteFile(); + HttpDownloader downloader = new HttpDownloader(remoteAddress, localfile); - HttpDownloader downloader = new HttpDownloader(remoteFile(), localfile); - downloader.setProgressListener(listener); + if (listener != null) { + downloader.setProgressListener(listener, + new ProgressListener.Event(Downloader.EVENT_PROGRESS, remoteAddress)); + } - Log.d("FDroid", "Downloading apk from " + remoteFile()); + Log.d(TAG, "Downloading apk from " + remoteAddress); int httpStatus = downloader.downloadHttpFile(); if (httpStatus != 200 || !localfile.exists()) { diff --git a/src/org/fdroid/fdroid/ProgressListener.java b/src/org/fdroid/fdroid/ProgressListener.java index 5550c1492..c7371f0ee 100644 --- a/src/org/fdroid/fdroid/ProgressListener.java +++ b/src/org/fdroid/fdroid/ProgressListener.java @@ -1,8 +1,10 @@ + package org.fdroid.fdroid; import android.os.Bundle; import android.os.Parcel; import android.os.Parcelable; +import android.text.TextUtils; public interface ProgressListener { @@ -14,6 +16,7 @@ public interface ProgressListener { public static class Event implements Parcelable { public static final int NO_VALUE = Integer.MIN_VALUE; + public static final String PROGRESS_DATA_REPO = "repo"; public final int type; public final Bundle data; @@ -29,27 +32,18 @@ public interface ProgressListener { this(type, NO_VALUE, NO_VALUE, null); } - public Event(int type, Bundle data) { - this(type, NO_VALUE, NO_VALUE, data); + public Event(int type, String repoAddress) { + this(type, NO_VALUE, NO_VALUE, repoAddress); } - public Event(int type, int progress) { - this(type, progress, NO_VALUE, null); - } - - public Event(int type, int progress, Bundle data) { - this(type, NO_VALUE, NO_VALUE, data); - } - - public Event(int type, int progress, int total) { - this(type, progress, total, null); - } - - public Event(int type, int progress, int total, Bundle data) { + public Event(int type, int progress, int total, String repoAddress) { this.type = type; this.progress = progress; this.total = total; - this.data = data == null ? new Bundle() : data; + if (TextUtils.isEmpty(repoAddress)) + this.data = new Bundle(); + else + this.data = createProgressData(repoAddress); } @Override @@ -68,7 +62,8 @@ public interface ProgressListener { public static final Parcelable.Creator CREATOR = new Parcelable.Creator() { @Override public Event createFromParcel(Parcel in) { - return new Event(in.readInt(), in.readInt(), in.readInt(), in.readBundle()); + return new Event(in.readInt(), in.readInt(), in.readInt(), + in.readBundle().getString(PROGRESS_DATA_REPO)); } @Override @@ -77,6 +72,16 @@ public interface ProgressListener { } }; + public String getRepoAddress() { + return data.getString(PROGRESS_DATA_REPO); + } + + public static Bundle createProgressData(String repoAddress) { + Bundle data = new Bundle(); + data.putString(PROGRESS_DATA_REPO, repoAddress); + return data; + } + } } diff --git a/src/org/fdroid/fdroid/RepoXMLHandler.java b/src/org/fdroid/fdroid/RepoXMLHandler.java index 1c7fb5009..541dfff2e 100644 --- a/src/org/fdroid/fdroid/RepoXMLHandler.java +++ b/src/org/fdroid/fdroid/RepoXMLHandler.java @@ -19,7 +19,6 @@ package org.fdroid.fdroid; -import android.os.Bundle; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.Repo; @@ -279,12 +278,11 @@ public class RepoXMLHandler extends DefaultHandler { } else if (localName.equals("application") && curapp == null) { curapp = new App(); curapp.id = attributes.getValue("", "id"); - Bundle progressData = RepoUpdater.createProgressData(repo.address); progressCounter ++; progressListener.onProgress( new ProgressListener.Event( - RepoUpdater.PROGRESS_TYPE_PROCESS_XML, progressCounter, - totalAppCount, progressData)); + RepoUpdater.PROGRESS_TYPE_PROCESS_XML, + progressCounter, totalAppCount, repo.address)); } else if (localName.equals("package") && curapp != null && curapk == null) { curapk = new Apk(); diff --git a/src/org/fdroid/fdroid/UpdateService.java b/src/org/fdroid/fdroid/UpdateService.java index 471b120f2..06cd30a1a 100644 --- a/src/org/fdroid/fdroid/UpdateService.java +++ b/src/org/fdroid/fdroid/UpdateService.java @@ -194,7 +194,7 @@ public class UpdateService extends IntentService implements ProgressListener { if (message != null && message.length() > 0) resultData.putString(RESULT_MESSAGE, message); if (event == null) - event = new Event(statusCode); + event = new ProgressListener.Event(statusCode); resultData.putParcelable(RESULT_EVENT, event); receiver.send(statusCode, resultData); } @@ -675,14 +675,13 @@ public class UpdateService extends IntentService implements ProgressListener { @Override public void onProgress(ProgressListener.Event event) { String message = ""; + String repoAddress = event.getRepoAddress(); if (event.type == RepoUpdater.PROGRESS_TYPE_DOWNLOAD) { - String repoAddress = event.data.getString(RepoUpdater.PROGRESS_DATA_REPO); String downloadedSize = Utils.getFriendlySize( event.progress ); String totalSize = Utils.getFriendlySize( event.total ); int percent = (int)((double)event.progress/event.total * 100); message = getString(R.string.status_download, repoAddress, downloadedSize, totalSize, percent); } else if (event.type == RepoUpdater.PROGRESS_TYPE_PROCESS_XML) { - String repoAddress = event.data.getString(RepoUpdater.PROGRESS_DATA_REPO); message = getString(R.string.status_processing_xml, repoAddress, event.progress, event.total); } sendStatus(STATUS_INFO, message); diff --git a/src/org/fdroid/fdroid/net/Downloader.java b/src/org/fdroid/fdroid/net/Downloader.java index 304e3640b..fac02f64c 100644 --- a/src/org/fdroid/fdroid/net/Downloader.java +++ b/src/org/fdroid/fdroid/net/Downloader.java @@ -48,11 +48,6 @@ public abstract class Downloader { outputFile = null; } - public void setProgressListener(ProgressListener listener) { - this.progressListener = listener; - this.progressEvent = new ProgressListener.Event(EVENT_PROGRESS, totalDownloadSize()); - } - public void setProgressListener(ProgressListener progressListener, ProgressListener.Event progressEvent) { this.progressListener = progressListener; diff --git a/src/org/fdroid/fdroid/updater/RepoUpdater.java b/src/org/fdroid/fdroid/updater/RepoUpdater.java index a5817cc50..3382368cb 100644 --- a/src/org/fdroid/fdroid/updater/RepoUpdater.java +++ b/src/org/fdroid/fdroid/updater/RepoUpdater.java @@ -2,7 +2,6 @@ package org.fdroid.fdroid.updater; import android.content.ContentValues; import android.content.Context; -import android.os.Bundle; import android.util.Log; import org.fdroid.fdroid.ProgressListener; import org.fdroid.fdroid.RepoXMLHandler; @@ -11,7 +10,6 @@ import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.RepoProvider; -import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.HttpDownloader; import org.xml.sax.InputSource; import org.xml.sax.SAXException; @@ -30,7 +28,6 @@ abstract public class RepoUpdater { public static final int PROGRESS_TYPE_DOWNLOAD = 1; public static final int PROGRESS_TYPE_PROCESS_XML = 2; - public static final String PROGRESS_DATA_REPO = "repo"; public static RepoUpdater createUpdaterFor(Context ctx, Repo repo) { if (repo.fingerprint == null && repo.pubkey == null) { @@ -86,17 +83,14 @@ abstract public class RepoUpdater { protected abstract String getIndexAddress(); protected HttpDownloader downloadIndex() throws UpdateException { - Bundle progressData = createProgressData(repo.address); HttpDownloader downloader = null; try { downloader = new HttpDownloader(getIndexAddress(), context); downloader.setETag(repo.lastetag); if (isInteractive()) { - ProgressListener.Event event = - new ProgressListener.Event( - RepoUpdater.PROGRESS_TYPE_DOWNLOAD, progressData); - downloader.setProgressListener(progressListener, event); + downloader.setProgressListener(progressListener, + new ProgressListener.Event(PROGRESS_TYPE_DOWNLOAD, repo.address)); } int status = downloader.downloadHttpFile(); @@ -139,12 +133,6 @@ abstract public class RepoUpdater { return downloader; } - public static Bundle createProgressData(String repoAddress) { - Bundle data = new Bundle(); - data.putString(PROGRESS_DATA_REPO, repoAddress); - return data; - } - private int estimateAppCount(File indexFile) { int count = -1; try { From 9588f30778e37687c604802f7beb63077160a11e Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 25 Feb 2014 15:00:55 -0500 Subject: [PATCH 05/17] normalize null checks for ProgressListeners This removes the RepoUpdater.isInteractive() method to normalize the code used to check whether a ProgressListener is null throughout the project. --- src/org/fdroid/fdroid/updater/RepoUpdater.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/org/fdroid/fdroid/updater/RepoUpdater.java b/src/org/fdroid/fdroid/updater/RepoUpdater.java index 3382368cb..357b75dfa 100644 --- a/src/org/fdroid/fdroid/updater/RepoUpdater.java +++ b/src/org/fdroid/fdroid/updater/RepoUpdater.java @@ -66,10 +66,6 @@ abstract public class RepoUpdater { return apks; } - public boolean isInteractive() { - return progressListener != null; - } - /** * For example, you may want to unzip a jar file to get the index inside, * or if the file is not compressed, you can just return a reference to @@ -88,7 +84,7 @@ abstract public class RepoUpdater { downloader = new HttpDownloader(getIndexAddress(), context); downloader.setETag(repo.lastetag); - if (isInteractive()) { + if (progressListener != null) { // interactive session, show progress downloader.setProgressListener(progressListener, new ProgressListener.Event(PROGRESS_TYPE_DOWNLOAD, repo.address)); } @@ -171,7 +167,7 @@ abstract public class RepoUpdater { XMLReader reader = parser.getXMLReader(); RepoXMLHandler handler = new RepoXMLHandler(repo, progressListener); - if (isInteractive()) { + if (progressListener != null) { // Only bother spending the time to count the expected apps // if we can show that to the user... handler.setTotalAppCount(estimateAppCount(indexFile)); From 19e722a9d7ef800983021a94a448d17bfdd42691 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 20 May 2014 05:39:02 +0930 Subject: [PATCH 06/17] wire up ProgressListener to new ApkDownloader structure Conflicts: src/org/fdroid/fdroid/AppDetails.java --- src/org/fdroid/fdroid/AppDetails.java | 96 +++++++++++++++++---------- 1 file changed, 62 insertions(+), 34 deletions(-) diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index 63af2b9d7..c1096af58 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -19,32 +19,28 @@ package org.fdroid.fdroid; -import android.content.*; -import android.widget.*; - -import org.fdroid.fdroid.data.*; -import org.fdroid.fdroid.installer.Installer; -import org.fdroid.fdroid.installer.Installer.AndroidNotCompatibleException; -import org.fdroid.fdroid.installer.Installer.InstallerCallback; -import org.fdroid.fdroid.net.Downloader; -import org.xml.sax.XMLReader; - +import android.app.Activity; import android.app.AlertDialog; import android.app.ListActivity; import android.app.ProgressDialog; import android.bluetooth.BluetoothAdapter; +import android.content.ContentValues; +import android.content.Context; +import android.content.DialogInterface; +import android.content.Intent; +import android.content.SharedPreferences; +import android.content.pm.PackageInfo; +import android.content.pm.PackageManager; +import android.content.pm.PackageManager.NameNotFoundException; +import android.content.pm.Signature; +import android.database.ContentObserver; +import android.graphics.Bitmap; import android.net.Uri; import android.os.Bundle; import android.os.Handler; -import android.os.Message; import android.preference.PreferenceManager; import android.support.v4.app.NavUtils; import android.support.v4.view.MenuItemCompat; -import android.content.pm.PackageManager; -import android.content.pm.PackageInfo; -import android.content.pm.Signature; -import android.content.pm.PackageManager.NameNotFoundException; -import android.database.ContentObserver; import android.text.Editable; import android.text.Html; import android.text.Html.TagHandler; @@ -59,16 +55,30 @@ import android.view.SubMenu; import android.view.View; import android.view.ViewGroup; import android.view.Window; -import android.graphics.Bitmap; - +import android.widget.ArrayAdapter; +import android.widget.ImageView; +import android.widget.LinearLayout; +import android.widget.ListView; +import android.widget.TextView; +import android.widget.Toast; import com.nostra13.universalimageloader.core.DisplayImageOptions; import com.nostra13.universalimageloader.core.ImageLoader; import com.nostra13.universalimageloader.core.assist.ImageScaleType; - import org.fdroid.fdroid.Utils.CommaSeparatedList; import org.fdroid.fdroid.compat.ActionBarCompat; import org.fdroid.fdroid.compat.MenuManager; import org.fdroid.fdroid.compat.PackageManagerCompat; +import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.data.ApkProvider; +import org.fdroid.fdroid.data.App; +import org.fdroid.fdroid.data.AppProvider; +import org.fdroid.fdroid.data.Repo; +import org.fdroid.fdroid.data.RepoProvider; +import org.fdroid.fdroid.installer.Installer; +import org.fdroid.fdroid.installer.Installer.AndroidNotCompatibleException; +import org.fdroid.fdroid.installer.Installer.InstallerCallback; +import org.fdroid.fdroid.net.Downloader; +import org.xml.sax.XMLReader; import java.io.File; import java.security.NoSuchAlgorithmException; @@ -99,7 +109,7 @@ public class AppDetails extends ListActivity { // observer to update view when package has been installed/deleted AppObserver myAppObserver; - class AppObserver extends ContentObserver { + class AppObserver extends ContentObserver { public AppObserver(Handler handler) { super(handler); } @@ -906,6 +916,7 @@ public class AppDetails extends ListActivity { // Install the version of this app denoted by 'app.curApk'. private void install(final Apk apk) { + final Activity activity = this; String [] projection = { RepoProvider.DataColumns.ADDRESS }; Repo repo = RepoProvider.Helper.findById(this, apk.repo, projection); if (repo == null || repo.address == null) { @@ -921,9 +932,8 @@ public class AppDetails extends ListActivity { @Override public void onClick(DialogInterface dialog, int whichButton) { - downloadHandler = new DownloadHandler(apk, - repoaddress, Utils - .getApkCacheDir(getBaseContext())); + downloadHandler = new DownloadHandler(activity, apk, + repoaddress, Utils.getApkCacheDir(getBaseContext())); } }); ask_alrt.setNegativeButton(getString(R.string.no), @@ -952,7 +962,7 @@ public class AppDetails extends ListActivity { alert.show(); return; } - downloadHandler = new DownloadHandler(apk, repoaddress, + downloadHandler = new DownloadHandler(activity, apk, repoaddress, Utils.getApkCacheDir(getBaseContext())); } private void installApk(File file, String packageName) { @@ -1066,13 +1076,17 @@ public class AppDetails extends ListActivity { // Handler used to update the progress dialog while downloading. private class DownloadHandler extends Handler implements ProgressListener { + private static final String TAG = "AppDetails.DownloadHandler"; + private Activity activity; private ApkDownloader download; private ProgressDialog pd; private String id; - public DownloadHandler(Apk apk, String repoaddress, File destdir) { + public DownloadHandler(Activity activity, Apk apk, String repoaddress, File destdir) { + this.activity = activity; id = apk.id; download = new ApkDownloader(apk, repoaddress, destdir); + download.setProgressListener(this); download.start(); } @@ -1082,27 +1096,41 @@ public class AppDetails extends ListActivity { } } - public void onProgress(ProgressListener.Event event) { + public void onProgress(final ProgressListener.Event event) { boolean finished = false; switch (event.type) { case Downloader.EVENT_PROGRESS: - if (pd == null) { - pd = createProgressDialog(download.remoteFile(), - event.progress, event.total); - } else { - pd.setProgress(event.progress); - } + activity.runOnUiThread(new Runnable() { + + @Override + public void run() { + // this must be on the main UI thread + if (pd == null) { + pd = createProgressDialog(download.remoteFile(), + event.progress, event.total); + } else { + pd.setProgress(event.progress); + } + } + }); break; case ApkDownloader.EVENT_ERROR_DOWNLOAD_FAILED: case ApkDownloader.EVENT_ERROR_HASH_MISMATCH: case ApkDownloader.EVENT_ERROR_UNKNOWN: - String text; + final String text; if (event.type == ApkDownloader.EVENT_ERROR_HASH_MISMATCH) text = getString(R.string.corrupt_download); else text = getString(R.string.details_notinstalled); - Toast.makeText(AppDetails.this, text, Toast.LENGTH_LONG).show(); + activity.runOnUiThread(new Runnable() { + + @Override + public void run() { + // this must be on the main UI thread + Toast.makeText(AppDetails.this, text, Toast.LENGTH_LONG).show(); + } + }); finished = true; break; From 084a048740312cc30496b6da6a4e4fbf7e304304 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 25 Feb 2014 20:23:41 -0500 Subject: [PATCH 07/17] ApkDownloader: rename remoteFile() to getRemoteAddress() Since it doesn't return a java.io.File but a String with the remote address --- src/org/fdroid/fdroid/ApkDownloader.java | 4 ++-- src/org/fdroid/fdroid/AppDetails.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/org/fdroid/fdroid/ApkDownloader.java b/src/org/fdroid/fdroid/ApkDownloader.java index 429d8e9fc..006148fa5 100644 --- a/src/org/fdroid/fdroid/ApkDownloader.java +++ b/src/org/fdroid/fdroid/ApkDownloader.java @@ -58,7 +58,7 @@ public class ApkDownloader extends Thread { return localfile; } - public String remoteFile() { + public String getRemoteAddress() { return repoaddress + "/" + curapk.apkName.replace(" ", "%20"); } @@ -82,7 +82,7 @@ public class ApkDownloader extends Thread { } // If we haven't got the apk locally, we'll have to download it... - String remoteAddress = remoteFile(); + String remoteAddress = getRemoteAddress(); HttpDownloader downloader = new HttpDownloader(remoteAddress, localfile); if (listener != null) { diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index c1096af58..2c117d850 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -1106,7 +1106,7 @@ public class AppDetails extends ListActivity { public void run() { // this must be on the main UI thread if (pd == null) { - pd = createProgressDialog(download.remoteFile(), + pd = createProgressDialog(download.getRemoteAddress(), event.progress, event.total); } else { pd.setProgress(event.progress); From 91f2f1014efa2928cf278b647f5492384b7bae30 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 25 Feb 2014 21:40:10 -0500 Subject: [PATCH 08/17] SKETCH - DOES NOT WORK: move HTTP code into HttpDownloader This is the beginning of moving to a Downloader class and subclasses that are used everywhere for downloading APKs, indexes, and icons. I think that the ETag value can probably be used everywhere as the unique ID for testing whether to download content, so I moved that to the super class Downloader. But that's just a guess as of now... --- src/org/fdroid/fdroid/ApkDownloader.java | 11 +-- src/org/fdroid/fdroid/net/Downloader.java | 48 +++++++++-- src/org/fdroid/fdroid/net/HttpDownloader.java | 84 ++++++++++--------- .../fdroid/fdroid/updater/RepoUpdater.java | 50 ++++------- 4 files changed, 108 insertions(+), 85 deletions(-) diff --git a/src/org/fdroid/fdroid/ApkDownloader.java b/src/org/fdroid/fdroid/ApkDownloader.java index 006148fa5..4070e0ecc 100644 --- a/src/org/fdroid/fdroid/ApkDownloader.java +++ b/src/org/fdroid/fdroid/ApkDownloader.java @@ -20,13 +20,14 @@ package org.fdroid.fdroid; -import java.io.*; - import android.util.Log; + import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.HttpDownloader; +import java.io.File; + public class ApkDownloader extends Thread { private static final String TAG = "ApkDownloader"; @@ -83,7 +84,7 @@ public class ApkDownloader extends Thread { // If we haven't got the apk locally, we'll have to download it... String remoteAddress = getRemoteAddress(); - HttpDownloader downloader = new HttpDownloader(remoteAddress, localfile); + Downloader downloader = new HttpDownloader(remoteAddress, localfile); if (listener != null) { downloader.setProgressListener(listener, @@ -91,9 +92,9 @@ public class ApkDownloader extends Thread { } Log.d(TAG, "Downloading apk from " + remoteAddress); - int httpStatus = downloader.downloadHttpFile(); + downloader.download(); - if (httpStatus != 200 || !localfile.exists()) { + if (!localfile.exists()) { sendProgress(EVENT_ERROR_DOWNLOAD_FAILED); return; } diff --git a/src/org/fdroid/fdroid/net/Downloader.java b/src/org/fdroid/fdroid/net/Downloader.java index fac02f64c..61b600ab0 100644 --- a/src/org/fdroid/fdroid/net/Downloader.java +++ b/src/org/fdroid/fdroid/net/Downloader.java @@ -1,17 +1,29 @@ package org.fdroid.fdroid.net; -import java.io.*; -import java.net.*; -import android.content.*; -import org.fdroid.fdroid.*; +import android.content.Context; +import android.util.Log; + +import org.fdroid.fdroid.ProgressListener; +import org.fdroid.fdroid.Utils; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.MalformedURLException; public abstract class Downloader { + private static final String TAG = "Downloader"; private OutputStream outputStream; private ProgressListener progressListener = null; private ProgressListener.Event progressEvent = null; private File outputFile; + protected String eTag = null; + public static final int EVENT_PROGRESS = 1; public abstract InputStream inputStream() throws IOException; @@ -50,10 +62,28 @@ public abstract class Downloader { public void setProgressListener(ProgressListener progressListener, ProgressListener.Event progressEvent) { + Log.i(TAG, "setProgressListener(ProgressListener listener, ProgressListener.Event progressEvent)"); this.progressListener = progressListener; this.progressEvent = progressEvent; } + /** + * If you ask for the eTag before calling download(), you will get the + * same one you passed in (if any). If you call it after download(), you + * will get the new eTag from the server, or null if there was none. + */ + public String getETag() { + return eTag; + } + + /** + * If this eTag matches that returned by the server, then no download will + * take place, and a status code of 304 will be returned by download(). + */ + public void setETag(String eTag) { + this.eTag = eTag; + } + /** * Only available if you passed a context object into the constructor * (rather than an outputStream, which may or may not be associated with @@ -63,15 +93,19 @@ public abstract class Downloader { return outputFile; } + public abstract boolean hasChanged(); + + public abstract int totalDownloadSize(); + private void setupProgressListener() { + Log.i(TAG, "setupProgressListener"); if (progressListener != null && progressEvent != null) { progressEvent.total = totalDownloadSize(); } } - protected abstract int totalDownloadSize(); - - protected void download() throws IOException { + public void download() throws IOException { + Log.i(TAG, "download"); setupProgressListener(); InputStream input = null; try { diff --git a/src/org/fdroid/fdroid/net/HttpDownloader.java b/src/org/fdroid/fdroid/net/HttpDownloader.java index 74347ae0b..6fe79e7a7 100644 --- a/src/org/fdroid/fdroid/net/HttpDownloader.java +++ b/src/org/fdroid/fdroid/net/HttpDownloader.java @@ -1,19 +1,26 @@ package org.fdroid.fdroid.net; import android.content.Context; +import android.util.Log; -import java.io.*; +import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.net.HttpURLConnection; import java.net.MalformedURLException; import java.net.URL; +import javax.net.ssl.SSLHandshakeException; + public class HttpDownloader extends Downloader { + private static final String TAG = "HttpDownloader"; private static final String HEADER_IF_NONE_MATCH = "If-None-Match"; private static final String HEADER_FIELD_ETAG = "ETag"; private URL sourceUrl; - private String eTag = null; private HttpURLConnection connection; private int statusCode = -1; @@ -57,49 +64,48 @@ public class HttpDownloader extends Downloader { // In the event of a 200 response ONLY, 'retag' (which should be passed // empty) may contain an etag value for the response, or it may be left // empty if none was available. - public int downloadHttpFile() throws IOException { - connection = (HttpURLConnection)sourceUrl.openConnection(); - setupCacheCheck(); - statusCode = connection.getResponseCode(); - if (statusCode == 200) { - download(); - updateCacheCheck(); + @Override + public void download() throws IOException { + try { + connection = (HttpURLConnection)sourceUrl.openConnection(); + setupCacheCheck(); + statusCode = connection.getResponseCode(); + Log.i(TAG, "download " + statusCode); + if (statusCode == 304) { + // The index is unchanged since we last read it. We just mark + // everything that came from this repo as being updated. + Log.d("FDroid", "Repo index for " + sourceUrl + + " is up to date (by etag)"); + } else if (statusCode == 200) { + download(); + updateCacheCheck(); + } else { + // Is there any code other than 200 which still returns + // content? Just in case, lets try to clean up. + if (getFile() != null) { + getFile().delete(); + } + throw new IOException( + "Failed to update repo " + sourceUrl + + " - HTTP response " + statusCode); + } + } catch (SSLHandshakeException e) { + // TODO this should be handled better + throw new IOException( + "A problem occurred while establishing an SSL " + + "connection. If this problem persists, AND you have a " + + "very old device, you could try using http instead of " + + "https for the repo URL." + Log.getStackTraceString(e) ); } - return statusCode; } - /** - * Only available after downloading a file. - */ - public int getStatusCode() { - return statusCode; - } - - /** - * If you ask for the eTag before calling download(), you will get the - * same one you passed in (if any). If you call it after download(), you - * will get the new eTag from the server, or null if there was none. - */ - public String getETag() { - return eTag; - } - - /** - * If this eTag matches that returned by the server, then no download will - * take place, and a status code of 304 will be returned by download(). - */ - public void setETag(String eTag) { - this.eTag = eTag; - } - - - protected void setupCacheCheck() { + private void setupCacheCheck() { if (eTag != null) { connection.setRequestProperty(HEADER_IF_NONE_MATCH, eTag); } } - protected void updateCacheCheck() { + private void updateCacheCheck() { eTag = connection.getHeaderField(HEADER_FIELD_ETAG); } @@ -111,10 +117,12 @@ public class HttpDownloader extends Downloader { // on my connection. I think the 1/1.5 seconds is worth it, // because as the repo grows, the tradeoff will // become more worth it. - protected int totalDownloadSize() { + @Override + public int totalDownloadSize() { return connection.getContentLength(); } + @Override public boolean hasChanged() { return this.statusCode == 200; } diff --git a/src/org/fdroid/fdroid/updater/RepoUpdater.java b/src/org/fdroid/fdroid/updater/RepoUpdater.java index 357b75dfa..2186d50a1 100644 --- a/src/org/fdroid/fdroid/updater/RepoUpdater.java +++ b/src/org/fdroid/fdroid/updater/RepoUpdater.java @@ -3,6 +3,7 @@ package org.fdroid.fdroid.updater; import android.content.ContentValues; import android.content.Context; import android.util.Log; + import org.fdroid.fdroid.ProgressListener; import org.fdroid.fdroid.RepoXMLHandler; import org.fdroid.fdroid.Utils; @@ -10,19 +11,25 @@ import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.RepoProvider; +import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.HttpDownloader; import org.xml.sax.InputSource; import org.xml.sax.SAXException; import org.xml.sax.XMLReader; +import java.io.BufferedReader; +import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileReader; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Date; +import java.util.List; + import javax.net.ssl.SSLHandshakeException; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; -import java.io.*; -import java.util.ArrayList; -import java.util.Date; -import java.util.List; abstract public class RepoUpdater { @@ -78,8 +85,8 @@ abstract public class RepoUpdater { protected abstract String getIndexAddress(); - protected HttpDownloader downloadIndex() throws UpdateException { - HttpDownloader downloader = null; + protected Downloader downloadIndex() throws UpdateException { + Downloader downloader = null; try { downloader = new HttpDownloader(getIndexAddress(), context); downloader.setETag(repo.lastetag); @@ -89,34 +96,7 @@ abstract public class RepoUpdater { new ProgressListener.Event(PROGRESS_TYPE_DOWNLOAD, repo.address)); } - int status = downloader.downloadHttpFile(); - - if (status == 304) { - // The index is unchanged since we last read it. We just mark - // everything that came from this repo as being updated. - Log.d("FDroid", "Repo index for " + repo.address - + " is up to date (by etag)"); - } else if (status == 200) { - // Nothing needed to be done here... - } else { - // Is there any code other than 200 which still returns - // content? Just in case, lets try to clean up. - if (downloader.getFile() != null) { - downloader.getFile().delete(); - } - throw new UpdateException( - repo, - "Failed to update repo " + repo.address + - " - HTTP response " + status); - } - } catch (SSLHandshakeException e) { - throw new UpdateException( - repo, - "A problem occurred while establishing an SSL " + - "connection. If this problem persists, AND you have a " + - "very old device, you could try using http instead of " + - "https for the repo URL.", - e ); + downloader.download(); } catch (IOException e) { if (downloader != null && downloader.getFile() != null) { downloader.getFile().delete(); @@ -154,7 +134,7 @@ abstract public class RepoUpdater { File indexFile = null; try { - HttpDownloader downloader = downloadIndex(); + Downloader downloader = downloadIndex(); hasChanged = downloader.hasChanged(); if (hasChanged) { From 0e0e042cd01047f170bb7195de175cd1446b584c Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 28 Feb 2014 09:11:34 +1100 Subject: [PATCH 09/17] Change DownloadHandler from using runOnUiThread() to send/handleMessage(). It still executes the download on another thread, and receives progress events on that thread. However it now posts those events to the main UI thread for them to get handled. Also refactored Downloader/HttpDownloader a tad further. Some caching stuff was pushed down from HttpDownloader -> Downloader (renamed from eTag to just cacheTag). HttpDownloader.download() was recursively calling itself, so I made the base class download() method abstract, and instead the stream downloading is done in "protected void downloadFromStream()" Other minor stuff: * Prefixed some log TAGs with package names, to make them easier to grep. * Removed a little bit of unused code (parameter to DownloadHandler). --- src/org/fdroid/fdroid/AppDetails.java | 67 ++++++++++--------- src/org/fdroid/fdroid/net/Downloader.java | 30 ++++++--- src/org/fdroid/fdroid/net/HttpDownloader.java | 46 ++++++------- .../fdroid/fdroid/updater/RepoUpdater.java | 13 +++- 4 files changed, 87 insertions(+), 69 deletions(-) diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index 2c117d850..351a33c80 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -38,6 +38,7 @@ import android.graphics.Bitmap; import android.net.Uri; import android.os.Bundle; import android.os.Handler; +import android.os.Message; import android.preference.PreferenceManager; import android.support.v4.app.NavUtils; import android.support.v4.view.MenuItemCompat; @@ -932,8 +933,8 @@ public class AppDetails extends ListActivity { @Override public void onClick(DialogInterface dialog, int whichButton) { - downloadHandler = new DownloadHandler(activity, apk, - repoaddress, Utils.getApkCacheDir(getBaseContext())); + downloadHandler = new DownloadHandler( + apk, repoaddress, Utils.getApkCacheDir(getBaseContext())); } }); ask_alrt.setNegativeButton(getString(R.string.no), @@ -962,8 +963,8 @@ public class AppDetails extends ListActivity { alert.show(); return; } - downloadHandler = new DownloadHandler(activity, apk, repoaddress, - Utils.getApkCacheDir(getBaseContext())); + downloadHandler = new DownloadHandler( + apk, repoaddress, Utils.getApkCacheDir(getBaseContext())); } private void installApk(File file, String packageName) { setProgressBarIndeterminateVisibility(true); @@ -1076,14 +1077,12 @@ public class AppDetails extends ListActivity { // Handler used to update the progress dialog while downloading. private class DownloadHandler extends Handler implements ProgressListener { - private static final String TAG = "AppDetails.DownloadHandler"; - private Activity activity; + private static final String TAG = "org.fdroid.fdroid.AppDetails.DownloadHandler"; private ApkDownloader download; private ProgressDialog pd; private String id; - public DownloadHandler(Activity activity, Apk apk, String repoaddress, File destdir) { - this.activity = activity; + public DownloadHandler(Apk apk, String repoaddress, File destdir) { id = apk.id; download = new ApkDownloader(apk, repoaddress, destdir); download.setProgressListener(this); @@ -1096,23 +1095,22 @@ public class AppDetails extends ListActivity { } } - public void onProgress(final ProgressListener.Event event) { + private static final String MSG_EVENT_DATA = "msgEvent"; + + /** + * Subclasses must implement this to receive messages. + */ + public void handleMessage(Message msg) { + ProgressListener.Event event = msg.getData().getParcelable(MSG_EVENT_DATA); boolean finished = false; switch (event.type) { case Downloader.EVENT_PROGRESS: - activity.runOnUiThread(new Runnable() { - - @Override - public void run() { - // this must be on the main UI thread - if (pd == null) { - pd = createProgressDialog(download.getRemoteAddress(), - event.progress, event.total); - } else { - pd.setProgress(event.progress); - } - } - }); + if (pd == null) { + pd = createProgressDialog(download.getRemoteAddress(), + event.progress, event.total); + } else { + pd.setProgress(event.progress); + } break; case ApkDownloader.EVENT_ERROR_DOWNLOAD_FAILED: @@ -1123,14 +1121,8 @@ public class AppDetails extends ListActivity { text = getString(R.string.corrupt_download); else text = getString(R.string.details_notinstalled); - activity.runOnUiThread(new Runnable() { - - @Override - public void run() { - // this must be on the main UI thread - Toast.makeText(AppDetails.this, text, Toast.LENGTH_LONG).show(); - } - }); + // this must be on the main UI thread + Toast.makeText(AppDetails.this, text, Toast.LENGTH_LONG).show(); finished = true; break; @@ -1138,7 +1130,6 @@ public class AppDetails extends ListActivity { installApk(download.localFile(), id); finished = true; break; - } if (finished) { @@ -1146,6 +1137,20 @@ public class AppDetails extends ListActivity { } } + /** + * We receive events on the download thread, and then post them to + * whatever thread the DownloadHandler was run on (in our case, the UI + * thread). + * @param event + */ + public void onProgress(final ProgressListener.Event event) { + Message message = new Message(); + Bundle bundle = new Bundle(1); + bundle.putParcelable(MSG_EVENT_DATA, event); + message.setData(bundle); + sendMessage(message); + } + public void cancel() { // TODO: Re-implement... } diff --git a/src/org/fdroid/fdroid/net/Downloader.java b/src/org/fdroid/fdroid/net/Downloader.java index 61b600ab0..c26ef7eb2 100644 --- a/src/org/fdroid/fdroid/net/Downloader.java +++ b/src/org/fdroid/fdroid/net/Downloader.java @@ -15,14 +15,14 @@ import java.io.OutputStream; import java.net.MalformedURLException; public abstract class Downloader { - private static final String TAG = "Downloader"; + private static final String TAG = "org.fdroid.fdroid.net.Downloader"; private OutputStream outputStream; private ProgressListener progressListener = null; private ProgressListener.Event progressEvent = null; private File outputFile; - protected String eTag = null; + protected String cacheTag = null; public static final int EVENT_PROGRESS = 1; @@ -68,20 +68,24 @@ public abstract class Downloader { } /** - * If you ask for the eTag before calling download(), you will get the + * If you ask for the cacheTag before calling download(), you will get the * same one you passed in (if any). If you call it after download(), you - * will get the new eTag from the server, or null if there was none. + * will get the new cacheTag from the server, or null if there was none. */ - public String getETag() { - return eTag; + public String getCacheTag() { + return cacheTag; } /** - * If this eTag matches that returned by the server, then no download will + * If this cacheTag matches that returned by the server, then no download will * take place, and a status code of 304 will be returned by download(). */ - public void setETag(String eTag) { - this.eTag = eTag; + public void setCacheTag(String cacheTag) { + this.cacheTag = cacheTag; + } + + protected boolean wantToCheckCache() { + return cacheTag != null; } /** @@ -104,8 +108,12 @@ public abstract class Downloader { } } - public void download() throws IOException { - Log.i(TAG, "download"); + public abstract void download() throws IOException; + + public abstract boolean isCached(); + + protected void downloadFromStream() throws IOException { + Log.d(TAG, "Downloading from stream"); setupProgressListener(); InputStream input = null; try { diff --git a/src/org/fdroid/fdroid/net/HttpDownloader.java b/src/org/fdroid/fdroid/net/HttpDownloader.java index 6fe79e7a7..3f7920716 100644 --- a/src/org/fdroid/fdroid/net/HttpDownloader.java +++ b/src/org/fdroid/fdroid/net/HttpDownloader.java @@ -11,11 +11,12 @@ import java.io.OutputStream; import java.net.HttpURLConnection; import java.net.MalformedURLException; import java.net.URL; +import java.net.UnknownHostException; import javax.net.ssl.SSLHandshakeException; public class HttpDownloader extends Downloader { - private static final String TAG = "HttpDownloader"; + private static final String TAG = "org.fdroid.fdroid.net.HttpDownloader"; private static final String HEADER_IF_NONE_MATCH = "If-None-Match"; private static final String HEADER_FIELD_ETAG = "ETag"; @@ -68,29 +69,22 @@ public class HttpDownloader extends Downloader { public void download() throws IOException { try { connection = (HttpURLConnection)sourceUrl.openConnection(); - setupCacheCheck(); - statusCode = connection.getResponseCode(); - Log.i(TAG, "download " + statusCode); - if (statusCode == 304) { - // The index is unchanged since we last read it. We just mark - // everything that came from this repo as being updated. - Log.d("FDroid", "Repo index for " + sourceUrl - + " is up to date (by etag)"); - } else if (statusCode == 200) { - download(); - updateCacheCheck(); + + if (wantToCheckCache()) { + setupCacheCheck(); + Log.i(TAG, "Checking cached status of " + sourceUrl); + statusCode = connection.getResponseCode(); + } + + if (isCached()) { + Log.i(TAG, sourceUrl + " is cached, so not downloading (HTTP " + statusCode + ")"); } else { - // Is there any code other than 200 which still returns - // content? Just in case, lets try to clean up. - if (getFile() != null) { - getFile().delete(); - } - throw new IOException( - "Failed to update repo " + sourceUrl + - " - HTTP response " + statusCode); + Log.i(TAG, "Downloading from " + sourceUrl); + downloadFromStream(); + updateCacheCheck(); } } catch (SSLHandshakeException e) { - // TODO this should be handled better + // TODO this should be handled better, it is not internationalised here. throw new IOException( "A problem occurred while establishing an SSL " + "connection. If this problem persists, AND you have a " + @@ -99,14 +93,18 @@ public class HttpDownloader extends Downloader { } } + public boolean isCached() { + return wantToCheckCache() && statusCode == 304; + } + private void setupCacheCheck() { - if (eTag != null) { - connection.setRequestProperty(HEADER_IF_NONE_MATCH, eTag); + if (cacheTag != null) { + connection.setRequestProperty(HEADER_IF_NONE_MATCH, cacheTag); } } private void updateCacheCheck() { - eTag = connection.getHeaderField(HEADER_FIELD_ETAG); + cacheTag = connection.getHeaderField(HEADER_FIELD_ETAG); } // Testing in the emulator for me, showed that figuring out the diff --git a/src/org/fdroid/fdroid/updater/RepoUpdater.java b/src/org/fdroid/fdroid/updater/RepoUpdater.java index 2186d50a1..6f1ce3164 100644 --- a/src/org/fdroid/fdroid/updater/RepoUpdater.java +++ b/src/org/fdroid/fdroid/updater/RepoUpdater.java @@ -26,7 +26,6 @@ import java.util.ArrayList; import java.util.Date; import java.util.List; -import javax.net.ssl.SSLHandshakeException; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; @@ -89,7 +88,7 @@ abstract public class RepoUpdater { Downloader downloader = null; try { downloader = new HttpDownloader(getIndexAddress(), context); - downloader.setETag(repo.lastetag); + downloader.setCacheTag(repo.lastetag); if (progressListener != null) { // interactive session, show progress downloader.setProgressListener(progressListener, @@ -97,6 +96,14 @@ abstract public class RepoUpdater { } downloader.download(); + + if (downloader.isCached()) { + // The index is unchanged since we last read it. We just mark + // everything that came from this repo as being updated. + Log.d("FDroid", "Repo index for " + getIndexAddress() + + " is up to date (by etag)"); + } + } catch (IOException e) { if (downloader != null && downloader.getFile() != null) { downloader.getFile().delete(); @@ -160,7 +167,7 @@ abstract public class RepoUpdater { reader.parse(is); apps = handler.getApps(); apks = handler.getApks(); - updateRepo(handler, downloader.getETag()); + updateRepo(handler, downloader.getCacheTag()); } } catch (SAXException e) { throw new UpdateException( From 3ed2cde207ea506c430493ba20c490c1fb72bdb0 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 20 May 2014 05:42:25 +0930 Subject: [PATCH 10/17] Added AsyncDownloadWrapper and used it in AppDetails to download apks. This has a very different interface to the Downloader class, because a handful of operations need to be run off the main thread (not just download, but also totalDownloadSize and perhaps cache tag related stuff). The AsyncDownloadWrapper provides its own listener, more specific than just the progress listener, which invokes functions at different points in time (e.g. download failed, completed, cancelled). Refactored progress events to use string types instead of ints. This way, it is less likely that two different events from different processes will clash with each other. Conflicts: src/org/fdroid/fdroid/AppDetails.java --- src/org/fdroid/fdroid/ApkDownloader.java | 222 ++++++++++++------ src/org/fdroid/fdroid/AppDetails.java | 194 +++++++-------- src/org/fdroid/fdroid/ProgressListener.java | 36 ++- src/org/fdroid/fdroid/RepoXMLHandler.java | 5 +- src/org/fdroid/fdroid/UpdateService.java | 51 ++-- .../fdroid/net/AsyncDownloadWrapper.java | 133 +++++++++++ src/org/fdroid/fdroid/net/Downloader.java | 90 +++++-- src/org/fdroid/fdroid/net/HttpDownloader.java | 4 +- .../fdroid/fdroid/updater/RepoUpdater.java | 13 +- .../views/fragments/RepoDetailsFragment.java | 2 +- .../views/fragments/RepoListFragment.java | 4 +- 11 files changed, 497 insertions(+), 257 deletions(-) create mode 100644 src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java diff --git a/src/org/fdroid/fdroid/ApkDownloader.java b/src/org/fdroid/fdroid/ApkDownloader.java index 4070e0ecc..e8a58d046 100644 --- a/src/org/fdroid/fdroid/ApkDownloader.java +++ b/src/org/fdroid/fdroid/ApkDownloader.java @@ -20,27 +20,42 @@ package org.fdroid.fdroid; +import android.os.Bundle; import android.util.Log; import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.net.AsyncDownloadWrapper; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.HttpDownloader; import java.io.File; +import java.io.IOException; +import java.net.MalformedURLException; +import java.security.NoSuchAlgorithmException; -public class ApkDownloader extends Thread { - private static final String TAG = "ApkDownloader"; +public class ApkDownloader implements AsyncDownloadWrapper.Listener { - public static final int EVENT_APK_DOWNLOAD_COMPLETE = 100; - public static final int EVENT_ERROR_HASH_MISMATCH = 101; - public static final int EVENT_ERROR_DOWNLOAD_FAILED = 102; - public static final int EVENT_ERROR_UNKNOWN = 103; - private Apk curapk; - private String repoaddress; - private File destdir; - private File localfile; + private static final String TAG = "org.fdroid.fdroid.ApkDownloader"; + + public static final String EVENT_APK_DOWNLOAD_COMPLETE = "apkDownloadComplete"; + public static final String EVENT_APK_DOWNLOAD_CANCELLED = "apkDownloadCancelled"; + public static final String EVENT_ERROR = "apkDownloadError"; + + public static final int ERROR_HASH_MISMATCH = 101; + public static final int ERROR_DOWNLOAD_FAILED = 102; + public static final int ERROR_UNKNOWN = 103; + + /** + * Used as a key to pass data through with an error event, explaining the type of event. + */ + public static final String EVENT_DATA_ERROR_TYPE = "apkDownloadErrorType"; + + private Apk curApk; + private String repoAddress; + private File localFile; private ProgressListener listener; + private AsyncDownloadWrapper dlWrapper = null; public void setProgressListener(ProgressListener listener) { this.listener = listener; @@ -48,84 +63,153 @@ public class ApkDownloader extends Thread { // Constructor - creates a Downloader to download the given Apk, // which must have its detail populated. - ApkDownloader(Apk apk, String repoaddress, File destdir) { - curapk = apk; - this.repoaddress = repoaddress; - this.destdir = destdir; + ApkDownloader(Apk apk, String repoAddress, File destDir) { + curApk = apk; + this.repoAddress = repoAddress; + localFile = new File(destDir, curApk.apkName); } // The downloaded APK. Valid only when getStatus() has returned STATUS.DONE. public File localFile() { - return localfile; + return localFile; } public String getRemoteAddress() { - return repoaddress + "/" + curapk.apkName.replace(" ", "%20"); + return repoAddress + "/" + curApk.apkName.replace(" ", "%20"); } - @Override - public void run() { - localfile = new File(destdir, curapk.apkName); - + private Hasher createHasher() { + Hasher hasher; try { + hasher = new Hasher(curApk.hashType, localFile); + } catch (NoSuchAlgorithmException e) { + Log.e("FDroid", "Error verifying hash of cached apk at " + localFile + ". " + + "I don't understand what the " + curApk.hashType + " hash algorithm is :("); + hasher = null; + } + return hasher; + } - // See if we already have this apk cached... - if (localfile.exists()) { - // We do - if its hash matches, we'll use it... - Hasher hash = new Hasher(curapk.hashType, localfile); - if (hash.match(curapk.hash)) { - Log.d("FDroid", "Using cached apk at " + localfile); - return; - } else { - Log.d("FDroid", "Not using cached apk at " + localfile); - localfile.delete(); - } + private boolean hashMatches() { + if (!localFile.exists()) { + return false; + } + Hasher hasher = createHasher(); + return hasher != null && hasher.match(curApk.hash); + } + + /** + * If an existing cached version exists, and matches the hash of the apk we + * want to download, then we will return true. Otherwise, we return false + * (and remove the cached file - if it exists and didn't match the correct hash). + */ + private boolean verifyOrDeleteCachedVersion() { + if (localFile.exists()) { + if (hashMatches()) { + Log.d("FDroid", "Using cached apk at " + localFile); + return true; + } else { + Log.d("FDroid", "Not using cached apk at " + localFile); + deleteLocalFile(); } + } + return false; + } - // If we haven't got the apk locally, we'll have to download it... - String remoteAddress = getRemoteAddress(); - Downloader downloader = new HttpDownloader(remoteAddress, localfile); + private void deleteLocalFile() { + if (localFile != null && localFile.exists()) { + localFile.delete(); + } + } - if (listener != null) { - downloader.setProgressListener(listener, - new ProgressListener.Event(Downloader.EVENT_PROGRESS, remoteAddress)); - } + public void download() { - Log.d(TAG, "Downloading apk from " + remoteAddress); - downloader.download(); - - if (!localfile.exists()) { - sendProgress(EVENT_ERROR_DOWNLOAD_FAILED); - return; - } - - Hasher hash = new Hasher(curapk.hashType, localfile); - if (!hash.match(curapk.hash)) { - Log.d("FDroid", "Downloaded file hash of " + hash.getHash() - + " did not match repo's " + curapk.hash); - // No point keeping a bad file, whether we're - // caching or not. - localfile.delete(); - sendProgress(EVENT_ERROR_HASH_MISMATCH); - return; - } - } catch (Exception e) { - Log.e("FDroid", "Download failed:\n" + Log.getStackTraceString(e)); - if (localfile.exists()) { - localfile.delete(); - } - sendProgress(EVENT_ERROR_UNKNOWN); + // Can we use the cached version? + if (verifyOrDeleteCachedVersion()) { + sendMessage(EVENT_APK_DOWNLOAD_COMPLETE); return; } - Log.d("FDroid", "Download finished: " + localfile); - sendProgress(EVENT_APK_DOWNLOAD_COMPLETE); - } + String remoteAddress = getRemoteAddress(); + Log.d(TAG, "Downloading apk from " + remoteAddress); - private void sendProgress(int type) { - if (listener != null) { - listener.onProgress(new ProgressListener.Event(type)); + try { + + Downloader downloader = new HttpDownloader(remoteAddress, localFile); + dlWrapper = new AsyncDownloadWrapper(downloader, this); + dlWrapper.download(); + + } catch (MalformedURLException e) { + onErrorDownloading(e.getLocalizedMessage()); + } catch (IOException e) { + onErrorDownloading(e.getLocalizedMessage()); } } -} + private void sendMessage(String type) { + sendProgressEvent(new ProgressListener.Event(type)); + } + + private void sendError(int errorType) { + Bundle data = new Bundle(1); + data.putInt(EVENT_DATA_ERROR_TYPE, errorType); + sendProgressEvent(new Event(EVENT_ERROR, data)); + } + + private void sendProgressEvent(Event event) { + if (listener != null) { + listener.onProgress(event); + } + } + + @Override + public void onReceiveTotalDownloadSize(int size) { + // Do nothing... + // Rather, we will obtain the total download size from the progress events + // when they start coming through. + } + + @Override + public void onReceiveCacheTag(String cacheTag) { + // Do nothing... + } + + @Override + public void onErrorDownloading(String localisedExceptionDetails) { + Log.e("FDroid", "Download failed: " + localisedExceptionDetails); + sendError(ERROR_DOWNLOAD_FAILED); + deleteLocalFile(); + } + + @Override + public void onDownloadComplete() { + + if (!verifyOrDeleteCachedVersion()) { + sendError(ERROR_HASH_MISMATCH); + return; + } + + Log.d("FDroid", "Download finished: " + localFile); + sendMessage(EVENT_APK_DOWNLOAD_COMPLETE); + } + + @Override + public void onDownloadCancelled() { + sendMessage(EVENT_APK_DOWNLOAD_CANCELLED); + } + + @Override + public void onProgress(Event event) { + sendProgressEvent(event); + } + + public void cancel() { + if (dlWrapper != null) { + dlWrapper.attemptCancel(); + } + } + + public Apk getApk() { + return curApk; + } +} \ No newline at end of file diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index 351a33c80..5362dc2fa 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -38,7 +38,6 @@ import android.graphics.Bitmap; import android.net.Uri; import android.os.Bundle; import android.os.Handler; -import android.os.Message; import android.preference.PreferenceManager; import android.support.v4.app.NavUtils; import android.support.v4.view.MenuItemCompat; @@ -86,7 +85,7 @@ import java.security.NoSuchAlgorithmException; import java.util.Iterator; import java.util.List; -public class AppDetails extends ListActivity { +public class AppDetails extends ListActivity implements ProgressListener { private static final String TAG = "AppDetails"; public static final int REQUEST_ENABLE_BLUETOOTH = 2; @@ -96,6 +95,7 @@ public class AppDetails extends ListActivity { private FDroidApp fdroidApp; private ApkListAdapter adapter; + private ProgressDialog progressDialog; private static class ViewHolder { TextView version; @@ -281,7 +281,7 @@ public class AppDetails extends ListActivity { private App app; private String appid; private PackageManager mPm; - private DownloadHandler downloadHandler; + private ApkDownloader downloadHandler; private boolean stateRetained; private boolean startingIgnoreAll; @@ -441,23 +441,35 @@ public class AppDetails extends ListActivity { @Override protected void onDestroy() { + // TODO: Generally need to verify the downloader stuff plays well with orientation changes... if (downloadHandler != null) { if (!stateRetained) downloadHandler.cancel(); - downloadHandler.destroy(); + removeProgressDialog(); } super.onDestroy(); } + private void removeProgressDialog() { + if (progressDialog != null) { + progressDialog.dismiss(); + progressDialog = null; + } + } + // Copy all relevant state from an old instance. This is used in // place of reset(), so it must initialize all fields normally set // there. private void copyState(AppDetails old) { + // TODO: Reimplement copyState with the new downloader stuff... But really, if we start to use fragments for + // this view, then it will probably not be relevant any more... + /* if (old.downloadHandler != null) downloadHandler = new DownloadHandler(old.downloadHandler); app = old.app; mInstalledSignature = old.mInstalledSignature; mInstalledSigID = old.mInstalledSigID; + */ } // Reset the display and list contents. Used when entering the activity, and @@ -932,9 +944,8 @@ public class AppDetails extends ListActivity { new DialogInterface.OnClickListener() { @Override public void onClick(DialogInterface dialog, - int whichButton) { - downloadHandler = new DownloadHandler( - apk, repoaddress, Utils.getApkCacheDir(getBaseContext())); + int whichButton) { + startDownload(apk, repoaddress); } }); ask_alrt.setNegativeButton(getString(R.string.no), @@ -963,8 +974,14 @@ public class AppDetails extends ListActivity { alert.show(); return; } - downloadHandler = new DownloadHandler( - apk, repoaddress, Utils.getApkCacheDir(getBaseContext())); + startDownload(apk, repoaddress); + } + + private void startDownload(Apk apk, String repoAddress) { + downloadHandler = new ApkDownloader(apk, repoAddress, Utils.getApkCacheDir(getBaseContext())); + getProgressDialog(downloadHandler.getRemoteAddress()).show(); + downloadHandler.setProgressListener(this); + downloadHandler.download(); } private void installApk(File file, String packageName) { setProgressBarIndeterminateVisibility(true); @@ -1049,123 +1066,70 @@ public class AppDetails extends ListActivity { startActivity(Intent.createChooser(shareIntent, getString(R.string.menu_share))); } - private ProgressDialog createProgressDialog(String file, int p, int max) { - final ProgressDialog pd = new ProgressDialog(this); - pd.setProgressStyle(ProgressDialog.STYLE_HORIZONTAL); - pd.setMessage(getString(R.string.download_server) + ":\n " + file); - pd.setMax(max); - pd.setProgress(p); - pd.setCancelable(true); - pd.setCanceledOnTouchOutside(false); - pd.setOnCancelListener(new DialogInterface.OnCancelListener() { - @Override - public void onCancel(DialogInterface dialog) { - downloadHandler.cancel(); - } - }); - pd.setButton(DialogInterface.BUTTON_NEUTRAL, - getString(R.string.cancel), - new DialogInterface.OnClickListener() { - @Override - public void onClick(DialogInterface dialog, int which) { - pd.cancel(); + private ProgressDialog getProgressDialog(String file) { + if (progressDialog == null) { + final ProgressDialog pd = new ProgressDialog(this); + pd.setProgressStyle(ProgressDialog.STYLE_HORIZONTAL); + pd.setMessage(getString(R.string.download_server) + ":\n " + file); + pd.setCancelable(true); + pd.setCanceledOnTouchOutside(false); + pd.setIndeterminate(true); // This will get overridden on the first progress event we receive. + pd.setOnCancelListener(new DialogInterface.OnCancelListener() { + @Override + public void onCancel(DialogInterface dialog) { + downloadHandler.cancel(); + } + }); + pd.setButton(DialogInterface.BUTTON_NEUTRAL, + getString(R.string.cancel), + new DialogInterface.OnClickListener() { + @Override + public void onClick(DialogInterface dialog, int which) { + pd.cancel(); + } } - }); - pd.show(); - return pd; + ); + progressDialog = pd; + } + return progressDialog; } - // Handler used to update the progress dialog while downloading. - private class DownloadHandler extends Handler implements ProgressListener { - private static final String TAG = "org.fdroid.fdroid.AppDetails.DownloadHandler"; - private ApkDownloader download; - private ProgressDialog pd; - private String id; + private void updateProgressDialog(int progress, int total) { + ProgressDialog pd = getProgressDialog(downloadHandler.getRemoteAddress()); + pd.setIndeterminate(false); + pd.setProgress(progress); + pd.setMax(total); + } - public DownloadHandler(Apk apk, String repoaddress, File destdir) { - id = apk.id; - download = new ApkDownloader(apk, repoaddress, destdir); - download.setProgressListener(this); - download.start(); + @Override + public void onProgress(Event event) { + boolean finished = false; + if (event.type.equals(Downloader.EVENT_PROGRESS)) { + updateProgressDialog(event.progress, event.total); + } else if (event.type.equals(ApkDownloader.EVENT_ERROR)) { + final String text; + if (event.getData().getInt(ApkDownloader.EVENT_DATA_ERROR_TYPE) == ApkDownloader.ERROR_HASH_MISMATCH) + text = getString(R.string.corrupt_download); + else + text = getString(R.string.details_notinstalled); + // this must be on the main UI thread + 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); + finished = true; + } else if (event.type.equals(ApkDownloader.EVENT_APK_DOWNLOAD_CANCELLED)) { + Toast.makeText(this, getString(R.string.download_cancelled), Toast.LENGTH_LONG).show(); + finished = true; } - public DownloadHandler(DownloadHandler oldHandler) { - if (oldHandler != null) { - download = oldHandler.download; - } - } - - private static final String MSG_EVENT_DATA = "msgEvent"; - - /** - * Subclasses must implement this to receive messages. - */ - public void handleMessage(Message msg) { - ProgressListener.Event event = msg.getData().getParcelable(MSG_EVENT_DATA); - boolean finished = false; - switch (event.type) { - case Downloader.EVENT_PROGRESS: - if (pd == null) { - pd = createProgressDialog(download.getRemoteAddress(), - event.progress, event.total); - } else { - pd.setProgress(event.progress); - } - break; - - case ApkDownloader.EVENT_ERROR_DOWNLOAD_FAILED: - case ApkDownloader.EVENT_ERROR_HASH_MISMATCH: - case ApkDownloader.EVENT_ERROR_UNKNOWN: - final String text; - if (event.type == ApkDownloader.EVENT_ERROR_HASH_MISMATCH) - text = getString(R.string.corrupt_download); - else - text = getString(R.string.details_notinstalled); - // this must be on the main UI thread - Toast.makeText(AppDetails.this, text, Toast.LENGTH_LONG).show(); - finished = true; - break; - - case ApkDownloader.EVENT_APK_DOWNLOAD_COMPLETE: - installApk(download.localFile(), id); - finished = true; - break; - } - - if (finished) { - destroy(); - } - } - - /** - * We receive events on the download thread, and then post them to - * whatever thread the DownloadHandler was run on (in our case, the UI - * thread). - * @param event - */ - public void onProgress(final ProgressListener.Event event) { - Message message = new Message(); - Bundle bundle = new Bundle(1); - bundle.putParcelable(MSG_EVENT_DATA, event); - message.setData(bundle); - sendMessage(message); - } - - public void cancel() { - // TODO: Re-implement... - } - - public void destroy() { + if (finished) { // The dialog can't be dismissed when it's not displayed, // so do it when the activity is being destroyed. - if (pd != null) { - pd.dismiss(); - pd = null; - } + removeProgressDialog(); } } - @Override protected void onActivityResult(int requestCode, int resultCode, Intent data) { // handle cases for install manager first diff --git a/src/org/fdroid/fdroid/ProgressListener.java b/src/org/fdroid/fdroid/ProgressListener.java index c7371f0ee..1b408d81d 100644 --- a/src/org/fdroid/fdroid/ProgressListener.java +++ b/src/org/fdroid/fdroid/ProgressListener.java @@ -18,7 +18,7 @@ public interface ProgressListener { public static final int NO_VALUE = Integer.MIN_VALUE; public static final String PROGRESS_DATA_REPO = "repo"; - public final int type; + public final String type; public final Bundle data; // These two are not final, so that you can create a template Event, @@ -28,22 +28,19 @@ public interface ProgressListener { public int progress; public int total; - public Event(int type) { + public Event(String type) { this(type, NO_VALUE, NO_VALUE, null); } - public Event(int type, String repoAddress) { - this(type, NO_VALUE, NO_VALUE, repoAddress); + public Event(String type, Bundle data) { + this(type, NO_VALUE, NO_VALUE, data); } - public Event(int type, int progress, int total, String repoAddress) { + public Event(String type, int progress, int total, Bundle data) { this.type = type; this.progress = progress; this.total = total; - if (TextUtils.isEmpty(repoAddress)) - this.data = new Bundle(); - else - this.data = createProgressData(repoAddress); + this.data = (data == null) ? new Bundle() : data; } @Override @@ -53,7 +50,7 @@ public interface ProgressListener { @Override public void writeToParcel(Parcel dest, int flags) { - dest.writeInt(type); + dest.writeString(type); dest.writeInt(progress); dest.writeInt(total); dest.writeBundle(data); @@ -62,8 +59,7 @@ public interface ProgressListener { public static final Parcelable.Creator CREATOR = new Parcelable.Creator() { @Override public Event createFromParcel(Parcel in) { - return new Event(in.readInt(), in.readInt(), in.readInt(), - in.readBundle().getString(PROGRESS_DATA_REPO)); + return new Event(in.readString(), in.readInt(), in.readInt(), in.readBundle()); } @Override @@ -72,16 +68,16 @@ public interface ProgressListener { } }; - public String getRepoAddress() { - return data.getString(PROGRESS_DATA_REPO); - } - - public static Bundle createProgressData(String repoAddress) { - Bundle data = new Bundle(); - data.putString(PROGRESS_DATA_REPO, repoAddress); + /** + * Can help to provide context to the listener about what process is causing the event. + * For example, the repo updater uses one listener to listen to multiple downloaders. + * When it receives an event, it doesn't know which repo download is causing the event, + * so we pass that through to the downloader when we set the progress listener. This way, + * we can ask the event for the name of the repo. + */ + public Bundle getData() { return data; } - } } diff --git a/src/org/fdroid/fdroid/RepoXMLHandler.java b/src/org/fdroid/fdroid/RepoXMLHandler.java index 541dfff2e..f85a8eafc 100644 --- a/src/org/fdroid/fdroid/RepoXMLHandler.java +++ b/src/org/fdroid/fdroid/RepoXMLHandler.java @@ -19,6 +19,7 @@ package org.fdroid.fdroid; +import android.os.Bundle; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.Repo; @@ -279,10 +280,12 @@ public class RepoXMLHandler extends DefaultHandler { curapp = new App(); curapp.id = attributes.getValue("", "id"); progressCounter ++; + Bundle data = new Bundle(1); + data.putString(RepoUpdater.PROGRESS_DATA_REPO_ADDRESS, repo.address); progressListener.onProgress( new ProgressListener.Event( RepoUpdater.PROGRESS_TYPE_PROCESS_XML, - progressCounter, totalAppCount, repo.address)); + progressCounter, totalAppCount, data)); } else if (localName.equals("package") && curapp != null && curapk == null) { curapk = new Apk(); diff --git a/src/org/fdroid/fdroid/UpdateService.java b/src/org/fdroid/fdroid/UpdateService.java index 06cd30a1a..bf7fd8b71 100644 --- a/src/org/fdroid/fdroid/UpdateService.java +++ b/src/org/fdroid/fdroid/UpdateService.java @@ -33,6 +33,7 @@ import android.text.TextUtils; import android.util.Log; import android.widget.Toast; import org.fdroid.fdroid.data.*; +import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.updater.RepoUpdater; import java.util.*; @@ -47,6 +48,14 @@ public class UpdateService extends IntentService implements ProgressListener { public static final int STATUS_ERROR = 2; public static final int STATUS_INFO = 3; + // I don't like that I've had to dupliacte the statuses above with strings here, however + // one method of communication/notification is using ResultReceiver (int status codes) + // while the other uses progress events (string event types). + public static final String EVENT_COMPLETE_WITH_CHANGES = "repoUpdateComplete (changed)"; + public static final String EVENT_COMPLETE_AND_SAME = "repoUpdateComplete (not changed)"; + public static final String EVENT_ERROR = "repoUpdateError"; + public static final String EVENT_INFO = "repoUpdateInfo"; + public static final String EXTRA_RECEIVER = "receiver"; public static final String EXTRA_ADDRESS = "address"; @@ -97,28 +106,31 @@ public class UpdateService extends IntentService implements ProgressListener { return this; } + private void forwardEvent(String type) { + if (listener != null) { + listener.onProgress(new Event(type)); + } + } + @Override protected void onReceiveResult(int resultCode, Bundle resultData) { String message = resultData.getString(UpdateService.RESULT_MESSAGE); boolean finished = false; if (resultCode == UpdateService.STATUS_ERROR) { + forwardEvent(EVENT_ERROR); Toast.makeText(context, message, Toast.LENGTH_LONG).show(); finished = true; - } else if (resultCode == UpdateService.STATUS_COMPLETE_WITH_CHANGES - || resultCode == UpdateService.STATUS_COMPLETE_AND_SAME) { + } else if (resultCode == UpdateService.STATUS_COMPLETE_WITH_CHANGES) { + forwardEvent(EVENT_COMPLETE_WITH_CHANGES); + finished = true; + } else if (resultCode == UpdateService.STATUS_COMPLETE_AND_SAME) { + forwardEvent(EVENT_COMPLETE_AND_SAME); finished = true; } else if (resultCode == UpdateService.STATUS_INFO) { + forwardEvent(EVENT_INFO); dialog.setMessage(message); } - // Forward the progress event on to anybody else who'd like to know. - if (listener != null) { - Parcelable event = resultData.getParcelable(UpdateService.RESULT_EVENT); - if (event != null && event instanceof Event) { - listener.onProgress((Event)event); - } - } - if (finished && dialog.isShowing()) try { dialog.dismiss(); @@ -185,17 +197,10 @@ public class UpdateService extends IntentService implements ProgressListener { } protected void sendStatus(int statusCode, String message) { - sendStatus(statusCode, message, null); - } - - protected void sendStatus(int statusCode, String message, Event event) { if (receiver != null) { Bundle resultData = new Bundle(); if (message != null && message.length() > 0) resultData.putString(RESULT_MESSAGE, message); - if (event == null) - event = new ProgressListener.Event(statusCode); - resultData.putParcelable(RESULT_EVENT, event); receiver.send(statusCode, resultData); } } @@ -675,13 +680,15 @@ public class UpdateService extends IntentService implements ProgressListener { @Override public void onProgress(ProgressListener.Event event) { String message = ""; - String repoAddress = event.getRepoAddress(); - if (event.type == RepoUpdater.PROGRESS_TYPE_DOWNLOAD) { - String downloadedSize = Utils.getFriendlySize( event.progress ); - String totalSize = Utils.getFriendlySize( event.total ); + // TODO: Switch to passing through Bundles of data with the event, rather than a repo address. They are + // now much more general purpose then just repo downloading. + String repoAddress = event.getData().getString(RepoUpdater.PROGRESS_DATA_REPO_ADDRESS); + if (event.type.equals(Downloader.EVENT_PROGRESS)) { + String downloadedSize = Utils.getFriendlySize(event.progress); + String totalSize = Utils.getFriendlySize(event.total); int percent = (int)((double)event.progress/event.total * 100); message = getString(R.string.status_download, repoAddress, downloadedSize, totalSize, percent); - } else if (event.type == RepoUpdater.PROGRESS_TYPE_PROCESS_XML) { + } else if (event.type.equals(RepoUpdater.PROGRESS_TYPE_PROCESS_XML)) { message = getString(R.string.status_processing_xml, repoAddress, event.progress, event.total); } sendStatus(STATUS_INFO, message); diff --git a/src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java b/src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java new file mode 100644 index 000000000..01aba30cb --- /dev/null +++ b/src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java @@ -0,0 +1,133 @@ +package org.fdroid.fdroid.net; + +import android.os.Bundle; +import android.os.Handler; +import android.os.Message; +import android.util.Log; +import org.fdroid.fdroid.ProgressListener; + +import java.io.IOException; + +public class AsyncDownloadWrapper extends Handler { + + private static final String TAG = "org.fdroid.fdroid.net.AsyncDownloadWrapper"; + + private static final int MSG_PROGRESS = 1; + private static final int MSG_DOWNLOAD_COMPLETE = 2; + private static final int MSG_DOWNLOAD_CANCELLED = 3; + private static final int MSG_ERROR = 4; + private static final String MSG_DATA = "data"; + + private Downloader downloader; + private Listener listener; + private DownloadThread downloadThread = null; + + /** + * Normally the listener would be provided using a setListener method. + * However for the purposes of this async downloader, it doesn't make + * sense to have an async task without any way to notify the outside + * world about completion. Therefore, we require the listener as a + * parameter to the constructor. + */ + public AsyncDownloadWrapper(Downloader downloader, Listener listener) { + this.downloader = downloader; + this.listener = listener; + } + + public void fetchTotalDownloadSize() { + int size = downloader.totalDownloadSize(); + listener.onReceiveTotalDownloadSize(size); + } + + public void fetchCacheTag() { + String cacheTag = downloader.getCacheTag(); + listener.onReceiveCacheTag(cacheTag); + } + + public void download() { + downloadThread = new DownloadThread(); + downloadThread.start(); + } + + public void attemptCancel() { + if (downloadThread != null) { + downloadThread.interrupt(); + } + } + + public static class NotDownloadingException extends Exception { + public NotDownloadingException(String message) { + super(message); + } + } + + public void cancelDownload() throws NotDownloadingException { + if (downloadThread == null) { + throw new RuntimeException("Can't cancel download, it hasn't started yet."); + } else if (!downloadThread.isAlive()) { + throw new RuntimeException("Can't cancel download, it is already finished."); + } + + downloadThread.interrupt(); + } + + public void handleMessage(Message message) { + if (message.arg1 == MSG_PROGRESS) { + Bundle data = message.getData(); + ProgressListener.Event event = data.getParcelable(MSG_DATA); + listener.onProgress(event); + } else if (message.arg1 == MSG_DOWNLOAD_COMPLETE) { + listener.onDownloadComplete(); + } else if (message.arg1 == MSG_DOWNLOAD_CANCELLED) { + listener.onDownloadCancelled(); + } else if (message.arg1 == MSG_ERROR) { + listener.onErrorDownloading(message.getData().getString(MSG_DATA)); + } + } + + public interface Listener extends ProgressListener { + public void onReceiveTotalDownloadSize(int size); + public void onReceiveCacheTag(String cacheTag); + public void onErrorDownloading(String localisedExceptionDetails); + public void onDownloadComplete(); + public void onDownloadCancelled(); + } + + private class DownloadThread extends Thread implements ProgressListener { + + public void run() { + try { + downloader.setProgressListener(this); + downloader.download(); + sendMessage(MSG_DOWNLOAD_COMPLETE); + } catch (InterruptedException e) { + sendMessage(MSG_DOWNLOAD_CANCELLED); + } catch (IOException e) { + Log.e(TAG, e.getMessage() + ": " + Log.getStackTraceString(e)); + Bundle data = new Bundle(1); + data.putString(MSG_DATA, e.getLocalizedMessage()); + Message message = new Message(); + message.arg1 = MSG_ERROR; + message.setData(data); + AsyncDownloadWrapper.this.sendMessage(message); + } + } + + private void sendMessage(int messageType) { + Message message = new Message(); + message.arg1 = messageType; + AsyncDownloadWrapper.this.sendMessage(message); + } + + @Override + public void onProgress(Event event) { + Message message = new Message(); + Bundle data = new Bundle(); + data.putParcelable(MSG_DATA, event); + message.setData(data); + message.arg1 = MSG_PROGRESS; + AsyncDownloadWrapper.this.sendMessage(message); + } + } + +} diff --git a/src/org/fdroid/fdroid/net/Downloader.java b/src/org/fdroid/fdroid/net/Downloader.java index c26ef7eb2..8f28a252a 100644 --- a/src/org/fdroid/fdroid/net/Downloader.java +++ b/src/org/fdroid/fdroid/net/Downloader.java @@ -1,6 +1,7 @@ package org.fdroid.fdroid.net; import android.content.Context; +import android.os.Bundle; import android.util.Log; import org.fdroid.fdroid.ProgressListener; @@ -15,16 +16,16 @@ import java.io.OutputStream; import java.net.MalformedURLException; public abstract class Downloader { + private static final String TAG = "org.fdroid.fdroid.net.Downloader"; - private OutputStream outputStream; - private ProgressListener progressListener = null; - private ProgressListener.Event progressEvent = null; - private File outputFile; + private ProgressListener progressListener = null; + private Bundle eventData = null; + private File outputFile; protected String cacheTag = null; - public static final int EVENT_PROGRESS = 1; + public static final String EVENT_PROGRESS = "downloadProgress"; public abstract InputStream inputStream() throws IOException; @@ -52,6 +53,7 @@ public abstract class Downloader { * @see org.fdroid.fdroid.net.Downloader#getFile() */ public Downloader(File destFile, Context ctx) throws IOException { + // TODO: Reimplement (is it still necessary? In what context was it being used before?) } public Downloader(OutputStream output) @@ -60,11 +62,13 @@ public abstract class Downloader { outputFile = null; } - public void setProgressListener(ProgressListener progressListener, - ProgressListener.Event progressEvent) { - Log.i(TAG, "setProgressListener(ProgressListener listener, ProgressListener.Event progressEvent)"); - this.progressListener = progressListener; - this.progressEvent = progressEvent; + public void setProgressListener(ProgressListener listener) { + setProgressListener(listener, null); + } + + public void setProgressListener(ProgressListener listener, Bundle eventData) { + this.progressListener = listener; + this.eventData = eventData; } /** @@ -101,29 +105,75 @@ public abstract class Downloader { public abstract int totalDownloadSize(); - private void setupProgressListener() { - Log.i(TAG, "setupProgressListener"); - if (progressListener != null && progressEvent != null) { - progressEvent.total = totalDownloadSize(); - } + /** + * Helper function for synchronous downloads (i.e. those *not* using AsyncDownloadWrapper), + * which don't really want to bother dealing with an InterruptedException. + * The InterruptedException thrown from download() is there to enable cancelling asynchronous + * downloads, but regular synchronous downloads cannot be cancelled because download() will + * block until completed. + * @throws IOException + */ + public void downloadUninterrupted() throws IOException { + try { + download(); + } catch (InterruptedException ignored) {} } - public abstract void download() throws IOException; + public abstract void download() throws IOException, InterruptedException; public abstract boolean isCached(); - protected void downloadFromStream() throws IOException { + protected void downloadFromStream() throws IOException, InterruptedException { Log.d(TAG, "Downloading from stream"); - setupProgressListener(); InputStream input = null; try { input = inputStream(); - Utils.copy(input, outputStream, - progressListener, progressEvent); + copyInputToOutputStream(inputStream()); } finally { Utils.closeQuietly(outputStream); Utils.closeQuietly(input); } } + protected void copyInputToOutputStream(InputStream input) throws IOException, InterruptedException { + + byte[] buffer = new byte[Utils.BUFFER_SIZE]; + int bytesRead = 0; + int totalBytes = totalDownloadSize(); + sendProgress(bytesRead, totalBytes); + while (true) { + + // In a synchronous download (the usual usage of the Downloader interface), + // you will not be able to interrupt this because the thread will block + // after you have called download(). However if you use the AsyncDownloadWrapper, + // then it will use this mechanism to cancel the download. + if (Thread.interrupted()) { + // TODO: Do we need to provide more information to whoever needs it, + // so they can, for example, remove any partially created files? + Log.d(TAG, "Received interrupt, cancelling download"); + throw new InterruptedException(); + } + + int count = input.read(buffer); + bytesRead += count; + sendProgress(bytesRead, totalBytes); + if (count == -1) { + Log.d(TAG, "Finished downloading from stream"); + break; + } + outputStream.write(buffer, 0, count); + } + outputStream.flush(); + } + + protected void sendProgress(int bytesRead, int totalBytes) { + sendProgress(new ProgressListener.Event(EVENT_PROGRESS, bytesRead, totalBytes, eventData)); + } + + protected void sendProgress(ProgressListener.Event event) { + if (progressListener != null) { + progressListener.onProgress(event); + } + } + } diff --git a/src/org/fdroid/fdroid/net/HttpDownloader.java b/src/org/fdroid/fdroid/net/HttpDownloader.java index 3f7920716..177fa6de1 100644 --- a/src/org/fdroid/fdroid/net/HttpDownloader.java +++ b/src/org/fdroid/fdroid/net/HttpDownloader.java @@ -66,7 +66,7 @@ public class HttpDownloader extends Downloader { // empty) may contain an etag value for the response, or it may be left // empty if none was available. @Override - public void download() throws IOException { + public void download() throws IOException, InterruptedException { try { connection = (HttpURLConnection)sourceUrl.openConnection(); @@ -122,7 +122,7 @@ public class HttpDownloader extends Downloader { @Override public boolean hasChanged() { - return this.statusCode == 200; + return this.statusCode != 304; } } diff --git a/src/org/fdroid/fdroid/updater/RepoUpdater.java b/src/org/fdroid/fdroid/updater/RepoUpdater.java index 6f1ce3164..40864f881 100644 --- a/src/org/fdroid/fdroid/updater/RepoUpdater.java +++ b/src/org/fdroid/fdroid/updater/RepoUpdater.java @@ -2,6 +2,7 @@ package org.fdroid.fdroid.updater; import android.content.ContentValues; import android.content.Context; +import android.os.Bundle; import android.util.Log; import org.fdroid.fdroid.ProgressListener; @@ -32,8 +33,9 @@ import javax.xml.parsers.SAXParserFactory; abstract public class RepoUpdater { - public static final int PROGRESS_TYPE_DOWNLOAD = 1; - public static final int PROGRESS_TYPE_PROCESS_XML = 2; + public static final String PROGRESS_TYPE_PROCESS_XML = "processingXml"; + + public static final String PROGRESS_DATA_REPO_ADDRESS = "repoAddress"; public static RepoUpdater createUpdaterFor(Context ctx, Repo repo) { if (repo.fingerprint == null && repo.pubkey == null) { @@ -91,11 +93,12 @@ abstract public class RepoUpdater { downloader.setCacheTag(repo.lastetag); if (progressListener != null) { // interactive session, show progress - downloader.setProgressListener(progressListener, - new ProgressListener.Event(PROGRESS_TYPE_DOWNLOAD, repo.address)); + Bundle data = new Bundle(1); + data.putString(PROGRESS_DATA_REPO_ADDRESS, getIndexAddress()); + downloader.setProgressListener(progressListener, data); } - downloader.download(); + downloader.downloadUninterrupted(); if (downloader.isCached()) { // The index is unchanged since we last read it. We just mark diff --git a/src/org/fdroid/fdroid/views/fragments/RepoDetailsFragment.java b/src/org/fdroid/fdroid/views/fragments/RepoDetailsFragment.java index 0a3ff5d2f..471a6a654 100644 --- a/src/org/fdroid/fdroid/views/fragments/RepoDetailsFragment.java +++ b/src/org/fdroid/fdroid/views/fragments/RepoDetailsFragment.java @@ -203,7 +203,7 @@ public class RepoDetailsFragment extends Fragment { UpdateService.updateRepoNow(repo.address, getActivity()).setListener(new ProgressListener() { @Override public void onProgress(Event event) { - if (event.type == UpdateService.STATUS_COMPLETE_WITH_CHANGES) { + if (event.type.equals(UpdateService.EVENT_COMPLETE_WITH_CHANGES)) { repo = loadRepoDetails(); updateView((ViewGroup)getView()); } diff --git a/src/org/fdroid/fdroid/views/fragments/RepoListFragment.java b/src/org/fdroid/fdroid/views/fragments/RepoListFragment.java index d5b23c5ac..f98a18834 100644 --- a/src/org/fdroid/fdroid/views/fragments/RepoListFragment.java +++ b/src/org/fdroid/fdroid/views/fragments/RepoListFragment.java @@ -219,8 +219,8 @@ public class RepoListFragment extends ListFragment UpdateService.updateNow(getActivity()).setListener(new ProgressListener() { @Override public void onProgress(Event event) { - if (event.type == UpdateService.STATUS_COMPLETE_AND_SAME || - event.type == UpdateService.STATUS_COMPLETE_WITH_CHANGES) { + if (event.type.equals(UpdateService.EVENT_COMPLETE_AND_SAME) || + event.type.equals(UpdateService.EVENT_COMPLETE_WITH_CHANGES)) { // No need to prompt to update any more, we just did it! changed = false; } From 11ddf0478c835a397ce55f28ca6dd32dddf2c53b Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sun, 30 Mar 2014 22:57:07 +1100 Subject: [PATCH 11/17] Better handling of cancelling of downloads. One thing that still annoys me is that indeterminate progress dialog still shows "0/100" due to the number formatter being used to display the values in the TextView. Solution (from http://stackoverflow.com/a/6784180) is to either make a custom dialog, or at the very least, in API 11 or higher we can set the number formatter to display nothing. Don't show the full download path of repo. The full path includes the path to the index.jar, as well as ?clientVersion=*. This is undesirable, the main purpose of even showing where we are downloading from is to differentiate between multiple repos being updated at once. --- src/org/fdroid/fdroid/AppDetails.java | 10 ++-- src/org/fdroid/fdroid/ProgressListener.java | 1 - src/org/fdroid/fdroid/net/Downloader.java | 48 ++++++++++++++----- .../fdroid/fdroid/updater/RepoUpdater.java | 2 +- 4 files changed, 44 insertions(+), 17 deletions(-) diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index 5362dc2fa..cfc4b15c3 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -1073,11 +1073,16 @@ public class AppDetails extends ListActivity implements ProgressListener { pd.setMessage(getString(R.string.download_server) + ":\n " + file); pd.setCancelable(true); pd.setCanceledOnTouchOutside(false); - pd.setIndeterminate(true); // This will get overridden on the first progress event we receive. + + // The indeterminate-ness will get overridden on the first progress event we receive. + pd.setIndeterminate(true); + pd.setOnCancelListener(new DialogInterface.OnCancelListener() { @Override public void onCancel(DialogInterface dialog) { downloadHandler.cancel(); + progressDialog = null; + Toast.makeText(AppDetails.this, getString(R.string.download_cancelled), Toast.LENGTH_LONG).show(); } }); pd.setButton(DialogInterface.BUTTON_NEUTRAL, @@ -1118,9 +1123,6 @@ public class AppDetails extends ListActivity implements ProgressListener { } else if (event.type.equals(ApkDownloader.EVENT_APK_DOWNLOAD_COMPLETE)) { installApk(downloadHandler.localFile(), downloadHandler.getApk().id); finished = true; - } else if (event.type.equals(ApkDownloader.EVENT_APK_DOWNLOAD_CANCELLED)) { - Toast.makeText(this, getString(R.string.download_cancelled), Toast.LENGTH_LONG).show(); - finished = true; } if (finished) { diff --git a/src/org/fdroid/fdroid/ProgressListener.java b/src/org/fdroid/fdroid/ProgressListener.java index 1b408d81d..6af2daf4b 100644 --- a/src/org/fdroid/fdroid/ProgressListener.java +++ b/src/org/fdroid/fdroid/ProgressListener.java @@ -16,7 +16,6 @@ public interface ProgressListener { public static class Event implements Parcelable { public static final int NO_VALUE = Integer.MIN_VALUE; - public static final String PROGRESS_DATA_REPO = "repo"; public final String type; public final Bundle data; diff --git a/src/org/fdroid/fdroid/net/Downloader.java b/src/org/fdroid/fdroid/net/Downloader.java index 8f28a252a..9be48bd20 100644 --- a/src/org/fdroid/fdroid/net/Downloader.java +++ b/src/org/fdroid/fdroid/net/Downloader.java @@ -128,11 +128,41 @@ public abstract class Downloader { InputStream input = null; try { input = inputStream(); + + // Getting the input stream is slow(ish) for HTTP downloads, so we'll check if + // we were interrupted before proceeding to the download. + throwExceptionIfInterrupted(); + copyInputToOutputStream(inputStream()); } finally { Utils.closeQuietly(outputStream); Utils.closeQuietly(input); } + + // Even if we have completely downloaded the file, we should probably respect + // the wishes of the user who wanted to cancel us. + throwExceptionIfInterrupted(); + } + + /** + * In a synchronous download (the usual usage of the Downloader interface), + * you will not be able to interrupt this because the thread will block + * after you have called download(). However if you use the AsyncDownloadWrapper, + * then it will use this mechanism to cancel the download. + * + * After every network operation that could take a while, we will check if an + * interrupt occured during that blocking operation. The goal is to ensure we + * don't move onto another slow, network operation if we have cancelled the + * download. + * @throws InterruptedException + */ + private void throwExceptionIfInterrupted() throws InterruptedException { + if (Thread.interrupted()) { + // TODO: Do we need to provide more information to whoever needs it, + // so they can, for example, remove any partially created files? + Log.d(TAG, "Received interrupt, cancelling download"); + throw new InterruptedException(); + } } protected void copyInputToOutputStream(InputStream input) throws IOException, InterruptedException { @@ -140,21 +170,17 @@ public abstract class Downloader { byte[] buffer = new byte[Utils.BUFFER_SIZE]; int bytesRead = 0; int totalBytes = totalDownloadSize(); + + // Getting the total download size could potentially take time, depending on how + // it is implemented, so we may as well check this before we proceed. + throwExceptionIfInterrupted(); + sendProgress(bytesRead, totalBytes); while (true) { - // In a synchronous download (the usual usage of the Downloader interface), - // you will not be able to interrupt this because the thread will block - // after you have called download(). However if you use the AsyncDownloadWrapper, - // then it will use this mechanism to cancel the download. - if (Thread.interrupted()) { - // TODO: Do we need to provide more information to whoever needs it, - // so they can, for example, remove any partially created files? - Log.d(TAG, "Received interrupt, cancelling download"); - throw new InterruptedException(); - } - int count = input.read(buffer); + throwExceptionIfInterrupted(); + bytesRead += count; sendProgress(bytesRead, totalBytes); if (count == -1) { diff --git a/src/org/fdroid/fdroid/updater/RepoUpdater.java b/src/org/fdroid/fdroid/updater/RepoUpdater.java index 40864f881..eab4291d7 100644 --- a/src/org/fdroid/fdroid/updater/RepoUpdater.java +++ b/src/org/fdroid/fdroid/updater/RepoUpdater.java @@ -94,7 +94,7 @@ abstract public class RepoUpdater { if (progressListener != null) { // interactive session, show progress Bundle data = new Bundle(1); - data.putString(PROGRESS_DATA_REPO_ADDRESS, getIndexAddress()); + data.putString(PROGRESS_DATA_REPO_ADDRESS, repo.address); downloader.setProgressListener(progressListener, data); } From 1e1be1e73bb855e56f849aacbd61de338fc60909 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 20 May 2014 05:51:56 +0930 Subject: [PATCH 12/17] Support orientation changes during app download. Keep track of the downloader after an orientation change, and make sure that there is always UI feedback beeing shown. Removed resetRequired after merge conflict (the content observer now deals with this for us in a much nicer way). Conflicts: src/org/fdroid/fdroid/AppDetails.java --- src/org/fdroid/fdroid/ApkDownloader.java | 28 ++- src/org/fdroid/fdroid/AppDetails.java | 245 ++++++++++++++--------- 2 files changed, 177 insertions(+), 96 deletions(-) diff --git a/src/org/fdroid/fdroid/ApkDownloader.java b/src/org/fdroid/fdroid/ApkDownloader.java index e8a58d046..07c34f5d5 100644 --- a/src/org/fdroid/fdroid/ApkDownloader.java +++ b/src/org/fdroid/fdroid/ApkDownloader.java @@ -56,13 +56,17 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { private ProgressListener listener; private AsyncDownloadWrapper dlWrapper = null; + private int progress = 0; + private int totalSize = 0; public void setProgressListener(ProgressListener listener) { this.listener = listener; } - // Constructor - creates a Downloader to download the given Apk, - // which must have its detail populated. + public void removeProgressListener() { + setProgressListener(null); + } + ApkDownloader(Apk apk, String repoAddress, File destDir) { curApk = apk; this.repoAddress = repoAddress; @@ -157,6 +161,13 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { } private void sendProgressEvent(Event event) { + if (event.type.equals(Downloader.EVENT_PROGRESS)) { + // Keep a copy of these ourselves, so people can interrogate us for the + // info (in addition to receiving events with the info). + totalSize = event.total; + progress = event.progress; + } + if (listener != null) { listener.onProgress(event); } @@ -203,7 +214,12 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { sendProgressEvent(event); } + /** + * Attempts to cancel the download (if in progress) and also removes the progress + * listener (to prevent + */ public void cancel() { + removeProgressListener(); if (dlWrapper != null) { dlWrapper.attemptCancel(); } @@ -212,4 +228,12 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { public Apk getApk() { return curApk; } + + public int getProgress() { + return progress; + } + + public int getTotalSize() { + return totalSize; + } } \ No newline at end of file diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index cfc4b15c3..3baf3c3b8 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -24,11 +24,7 @@ import android.app.AlertDialog; import android.app.ListActivity; import android.app.ProgressDialog; import android.bluetooth.BluetoothAdapter; -import android.content.ContentValues; -import android.content.Context; -import android.content.DialogInterface; -import android.content.Intent; -import android.content.SharedPreferences; +import android.content.*; import android.content.pm.PackageInfo; import android.content.pm.PackageManager; import android.content.pm.PackageManager.NameNotFoundException; @@ -68,12 +64,7 @@ import org.fdroid.fdroid.Utils.CommaSeparatedList; import org.fdroid.fdroid.compat.ActionBarCompat; import org.fdroid.fdroid.compat.MenuManager; import org.fdroid.fdroid.compat.PackageManagerCompat; -import org.fdroid.fdroid.data.Apk; -import org.fdroid.fdroid.data.ApkProvider; -import org.fdroid.fdroid.data.App; -import org.fdroid.fdroid.data.AppProvider; -import org.fdroid.fdroid.data.Repo; -import org.fdroid.fdroid.data.RepoProvider; +import org.fdroid.fdroid.data.*; import org.fdroid.fdroid.installer.Installer; import org.fdroid.fdroid.installer.Installer.AndroidNotCompatibleException; import org.fdroid.fdroid.installer.Installer.InstallerCallback; @@ -86,7 +77,7 @@ import java.util.Iterator; import java.util.List; public class AppDetails extends ListActivity implements ProgressListener { - private static final String TAG = "AppDetails"; + private static final String TAG = "org.fdroid.fdroid.AppDetails"; public static final int REQUEST_ENABLE_BLUETOOTH = 2; @@ -122,7 +113,7 @@ public class AppDetails extends ListActivity implements ProgressListener { @Override public void onChange(boolean selfChange, Uri uri) { - if (!reset()) { + if (!reset(app.id)) { AppDetails.this.finish(); return; } @@ -279,10 +270,8 @@ public class AppDetails extends ListActivity implements ProgressListener { private static final int SEND_VIA_BLUETOOTH = Menu.FIRST + 15; private App app; - private String appid; private PackageManager mPm; private ApkDownloader downloadHandler; - private boolean stateRetained; private boolean startingIgnoreAll; private int startingIgnoreThis; @@ -294,6 +283,68 @@ public class AppDetails extends ListActivity implements ProgressListener { private DisplayImageOptions displayImageOptions; private Installer installer; + /** + * Stores relevant data that we want to keep track of when destroying the activity + * with the expectation of it being recreated straight away (e.g. after an + * orientation change). One of the major things is that we want the download thread + * to stay active, but for it not to trigger any UI stuff (e.g. progress dialogs) + * between the activity being destroyed and recreated. + */ + private static class ConfigurationChangeHelper { + + public ApkDownloader downloader; + public App app; + + public ConfigurationChangeHelper(ApkDownloader downloader, App app) { + this.downloader = downloader; + this.app = app; + } + } + + private boolean inProcessOfChangingConfiguration = false; + + /** + * Attempt to extract the appId from the intent which launched this activity. + * Various different intents could cause us to show this activity, such as: + *
    + *
  • market://details?id=[app_id]
    • + *
  • https://f-droid.org/app/[app_id]
    • + *
  • fdroid.app:[app_id]
    • + *
+ * @return May return null, if we couldn't find the appId. In this case, you will + * probably want to do something drastic like finish the activity and show some + * feedback to the user (this method will not do that, it will just return + * null). + */ + private String getAppIdFromIntent() { + Intent i = getIntent(); + Uri data = i.getData(); + String appId = null; + if (data != null) { + if (data.isHierarchical()) { + if (data.getHost() != null && data.getHost().equals("details")) { + // market://details?id=app.id + appId = data.getQueryParameter("id"); + } else { + // https://f-droid.org/app/app.id + appId = data.getLastPathSegment(); + if (appId != null && appId.equals("app")) { + appId = null; + } + } + } else { + // fdroid.app:app.id + appId = data.getEncodedSchemeSpecificPart(); + } + Log.d("FDroid", "AppDetails launched from link, for '" + appId + "'"); + } else if (!i.hasExtra(EXTRA_APPID)) { + Log.e("FDroid", "No application ID in AppDetails!?"); + } else { + appId = i.getStringExtra(EXTRA_APPID); + } + return appId; + } + @Override protected void onCreate(Bundle savedInstanceState) { requestWindowFeature(Window.FEATURE_INDETERMINATE_PROGRESS); @@ -319,43 +370,27 @@ public class AppDetails extends ListActivity implements ProgressListener { // for reason why. ActionBarCompat.create(this).setDisplayHomeAsUpEnabled(true); - Intent i = getIntent(); - Uri data = i.getData(); - if (data != null) { - if (data.isHierarchical()) { - if (data.getHost() != null && data.getHost().equals("details")) { - // market://details?id=app.id - appid = data.getQueryParameter("id"); - } else { - // https://f-droid.org/app/app.id - appid = data.getLastPathSegment(); - if (appid != null && appid.equals("app")) appid = null; - } - } else { - // fdroid.app:app.id - appid = data.getEncodedSchemeSpecificPart(); - } - Log.d("FDroid", "AppDetails launched from link, for '" + appid + "'"); - } else if (!i.hasExtra(EXTRA_APPID)) { - Log.d("FDroid", "No application ID in AppDetails!?"); - } else { - appid = i.getStringExtra(EXTRA_APPID); - } - - if (i.hasExtra(EXTRA_FROM)) { - setTitle(i.getStringExtra(EXTRA_FROM)); + if (getIntent().hasExtra(EXTRA_FROM)) { + setTitle(getIntent().getStringExtra(EXTRA_FROM)); } mPm = getPackageManager(); + installer = Installer.getActivityInstaller(this, mPm, myInstallerCallback); // Get the preferences we're going to use in this Activity... - AppDetails old = (AppDetails) getLastNonConfigurationInstance(); - if (old != null) { - copyState(old); + ConfigurationChangeHelper previousData = (ConfigurationChangeHelper)getLastNonConfigurationInstance(); + if (previousData != null) { + Log.d(TAG, "Recreating view after configuration change."); + downloadHandler = previousData.downloader; + if (downloadHandler != null) { + Log.d(TAG, "Download was in progress before the configuration change, so we will start to listen to its events again."); + } + app = previousData.app; + setApp(app); } else { - if (!reset()) { + if (!reset(getAppIdFromIntent())) { finish(); return; } @@ -398,11 +433,15 @@ public class AppDetails extends ListActivity implements ProgressListener { AppProvider.getContentUri(app.id), true, myAppObserver); - - if (!reset()) { - finish(); - return; + if (downloadHandler != null) { + 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(); } + updateViews(); MenuManager.create(this).invalidateOptionsMenu(); @@ -415,8 +454,16 @@ public class AppDetails extends ListActivity implements ProgressListener { } if (app != null && (app.ignoreAllUpdates != startingIgnoreAll || app.ignoreThisUpdate != startingIgnoreThis)) { + Log.d(TAG, "Updating 'ignore updates', as it has changed since we started the activity..."); setIgnoreUpdates(app.id, app.ignoreAllUpdates, app.ignoreThisUpdate); } + + if (downloadHandler != null) { + downloadHandler.removeProgressListener(); + } + + removeProgressDialog(); + super.onPause(); } @@ -435,18 +482,18 @@ public class AppDetails extends ListActivity implements ProgressListener { @Override public Object onRetainNonConfigurationInstance() { - stateRetained = true; - return this; + inProcessOfChangingConfiguration = true; + return new ConfigurationChangeHelper(downloadHandler, app); } @Override protected void onDestroy() { - // TODO: Generally need to verify the downloader stuff plays well with orientation changes... if (downloadHandler != null) { - if (!stateRetained) + if (!inProcessOfChangingConfiguration) { downloadHandler.cancel(); - removeProgressDialog(); + } } + inProcessOfChangingConfiguration = false; super.onDestroy(); } @@ -457,55 +504,50 @@ public class AppDetails extends ListActivity implements ProgressListener { } } - // Copy all relevant state from an old instance. This is used in - // place of reset(), so it must initialize all fields normally set - // there. - private void copyState(AppDetails old) { - // TODO: Reimplement copyState with the new downloader stuff... But really, if we start to use fragments for - // this view, then it will probably not be relevant any more... - /* - if (old.downloadHandler != null) - downloadHandler = new DownloadHandler(old.downloadHandler); - app = old.app; - mInstalledSignature = old.mInstalledSignature; - mInstalledSigID = old.mInstalledSigID; - */ - } - // Reset the display and list contents. Used when entering the activity, and // also when something has been installed/uninstalled. // Return true if the app was found, false otherwise. - private boolean reset() { + private boolean reset(String appId) { - Log.d("FDroid", "Getting application details for " + appid); - app = null; + Log.d("FDroid", "Getting application details for " + appId); + App newApp = null; - if (appid != null && appid.length() > 0) { - app = AppProvider.Helper.findById(getContentResolver(), appid); + if (appId != null && appId.length() > 0) { + newApp = AppProvider.Helper.findById(getContentResolver(), appId); } - if (app == null) { - Toast toast = Toast.makeText(this, - getString(R.string.no_such_app), Toast.LENGTH_LONG); - toast.show(); + setApp(newApp); + + return this.app != null; + } + + /** + * If passed null, this will show a message to the user ("Could not find app ..." or something + * like that) and then finish the activity. + */ + private void setApp(App newApp) { + + if (newApp == null) { + Toast.makeText(this, getString(R.string.no_such_app), Toast.LENGTH_LONG).show(); finish(); - return false; + return; } + app = newApp; + startingIgnoreAll = app.ignoreAllUpdates; startingIgnoreThis = app.ignoreThisUpdate; // Get the signature of the installed package... mInstalledSignature = null; mInstalledSigID = null; + if (app.isInstalled()) { - PackageManager pm = getBaseContext().getPackageManager(); + PackageManager pm = getPackageManager(); try { - PackageInfo pi = pm.getPackageInfo(appid, - PackageManager.GET_SIGNATURES); + PackageInfo pi = pm.getPackageInfo(app.id, PackageManager.GET_SIGNATURES); mInstalledSignature = pi.signatures[0]; - Hasher hash = new Hasher("MD5", mInstalledSignature - .toCharsString().getBytes()); + Hasher hash = new Hasher("MD5", mInstalledSignature.toCharsString().getBytes()); mInstalledSigID = hash.getHash(); } catch (NameNotFoundException e) { Log.d("FDroid", "Failed to get installed signature"); @@ -514,7 +556,6 @@ public class AppDetails extends ListActivity implements ProgressListener { mInstalledSignature = null; } } - return true; } private void startViews() { @@ -528,10 +569,10 @@ public class AppDetails extends ListActivity implements ProgressListener { headerView.removeAllViews(); if (landparent != null) { landparent.addView(infoView); - Log.d("FDroid", "Setting landparent infoview"); + Log.d("FDroid", "Setting up landscape view"); } else { headerView.addView(infoView); - Log.d("FDroid", "Setting header infoview"); + Log.d("FDroid", "Setting up portrait view"); } // Set the icon... @@ -627,8 +668,7 @@ public class AppDetails extends ListActivity implements ProgressListener { if (permissionName.equals("ACCESS_SUPERUSER")) { sb.append("\t• Full permissions to all device features and storage\n"); } else { - Log.d("FDroid", "Permission not yet available: " - +permissionName); + Log.d("FDroid", "Permission not yet available: " + permissionName); } } } @@ -979,9 +1019,9 @@ public class AppDetails extends ListActivity implements ProgressListener { private void startDownload(Apk apk, String repoAddress) { downloadHandler = new ApkDownloader(apk, repoAddress, Utils.getApkCacheDir(getBaseContext())); - getProgressDialog(downloadHandler.getRemoteAddress()).show(); downloadHandler.setProgressListener(this); downloadHandler.download(); + updateProgressDialog(); } private void installApk(File file, String packageName) { setProgressBarIndeterminateVisibility(true); @@ -1081,6 +1121,7 @@ public class AppDetails extends ListActivity implements ProgressListener { @Override public void onCancel(DialogInterface dialog) { downloadHandler.cancel(); + downloadHandler = null; progressDialog = null; Toast.makeText(AppDetails.this, getString(R.string.download_cancelled), Toast.LENGTH_LONG).show(); } @@ -1099,11 +1140,28 @@ public class AppDetails extends ListActivity implements ProgressListener { return progressDialog; } + /** + * Looks at the current downloadHandler and finds it's size and progress. + * This is in comparison to {@link org.fdroid.fdroid.AppDetails#updateProgressDialog(int, int)}, + * which is used when you have the details from a freshly received + * {@link org.fdroid.fdroid.ProgressListener.Event}. + */ + private void updateProgressDialog() { + updateProgressDialog(downloadHandler.getProgress(), downloadHandler.getTotalSize()); + } + private void updateProgressDialog(int progress, int total) { ProgressDialog pd = getProgressDialog(downloadHandler.getRemoteAddress()); - pd.setIndeterminate(false); - pd.setProgress(progress); - pd.setMax(total); + if (total > 0) { + pd.setIndeterminate(false); + pd.setProgress(progress); + pd.setMax(total); + } else { + pd.setIndeterminate(true); + pd.setProgress(progress); + pd.setMax(0); + } + pd.show(); } @Override @@ -1126,9 +1184,8 @@ public class AppDetails extends ListActivity implements ProgressListener { } if (finished) { - // The dialog can't be dismissed when it's not displayed, - // so do it when the activity is being destroyed. removeProgressDialog(); + downloadHandler = null; } } From 75586ec4e5261c86f54ff5d0a9a1f823e6a5f7ec Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 20 May 2014 06:13:15 +0930 Subject: [PATCH 13/17] Moved ApkDownloader to .net package This is where it belongs, along with the other downloader code. --- src/org/fdroid/fdroid/AppDetails.java | 1 + src/org/fdroid/fdroid/{ => net}/ApkDownloader.java | 10 ++++------ 2 files changed, 5 insertions(+), 6 deletions(-) rename src/org/fdroid/fdroid/{ => net}/ApkDownloader.java (96%) diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index 3baf3c3b8..0281ab08e 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -68,6 +68,7 @@ import org.fdroid.fdroid.data.*; import org.fdroid.fdroid.installer.Installer; import org.fdroid.fdroid.installer.Installer.AndroidNotCompatibleException; import org.fdroid.fdroid.installer.Installer.InstallerCallback; +import org.fdroid.fdroid.net.ApkDownloader; import org.fdroid.fdroid.net.Downloader; import org.xml.sax.XMLReader; diff --git a/src/org/fdroid/fdroid/ApkDownloader.java b/src/org/fdroid/fdroid/net/ApkDownloader.java similarity index 96% rename from src/org/fdroid/fdroid/ApkDownloader.java rename to src/org/fdroid/fdroid/net/ApkDownloader.java index 07c34f5d5..c2f4aa475 100644 --- a/src/org/fdroid/fdroid/ApkDownloader.java +++ b/src/org/fdroid/fdroid/net/ApkDownloader.java @@ -18,15 +18,13 @@ * MA 02110-1301, USA. */ -package org.fdroid.fdroid; +package org.fdroid.fdroid.net; import android.os.Bundle; import android.util.Log; - +import org.fdroid.fdroid.Hasher; +import org.fdroid.fdroid.ProgressListener; import org.fdroid.fdroid.data.Apk; -import org.fdroid.fdroid.net.AsyncDownloadWrapper; -import org.fdroid.fdroid.net.Downloader; -import org.fdroid.fdroid.net.HttpDownloader; import java.io.File; import java.io.IOException; @@ -35,7 +33,7 @@ import java.security.NoSuchAlgorithmException; public class ApkDownloader implements AsyncDownloadWrapper.Listener { - private static final String TAG = "org.fdroid.fdroid.ApkDownloader"; + private static final String TAG = "org.fdroid.fdroid.net.ApkDownloader"; public static final String EVENT_APK_DOWNLOAD_COMPLETE = "apkDownloadComplete"; public static final String EVENT_APK_DOWNLOAD_CANCELLED = "apkDownloadCancelled"; From d9e5b070541ae8963b259f3cbe4d51cc3e899b89 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 20 May 2014 08:59:27 +0930 Subject: [PATCH 14/17] Removed unneeded todo's, add docs. --- src/org/fdroid/fdroid/net/ApkDownloader.java | 9 ++++++++- src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java | 12 ++++++++++++ src/org/fdroid/fdroid/net/Downloader.java | 11 ----------- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/org/fdroid/fdroid/net/ApkDownloader.java b/src/org/fdroid/fdroid/net/ApkDownloader.java index c2f4aa475..27d052baf 100644 --- a/src/org/fdroid/fdroid/net/ApkDownloader.java +++ b/src/org/fdroid/fdroid/net/ApkDownloader.java @@ -31,6 +31,11 @@ import java.io.IOException; import java.net.MalformedURLException; import java.security.NoSuchAlgorithmException; +/** + * Downloads and verifies (against the Apk.hash) the apk file. + * If the file has previously been downloaded, it will make use of that + * instead, without going to the network to download a new one. + */ public class ApkDownloader implements AsyncDownloadWrapper.Listener { private static final String TAG = "org.fdroid.fdroid.net.ApkDownloader"; @@ -71,7 +76,9 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { localFile = new File(destDir, curApk.apkName); } - // The downloaded APK. Valid only when getStatus() has returned STATUS.DONE. + /** + * The downloaded APK. Valid only when getStatus() has returned STATUS.DONE. + */ public File localFile() { return localFile; } diff --git a/src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java b/src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java index 01aba30cb..5e250aaa4 100644 --- a/src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java +++ b/src/org/fdroid/fdroid/net/AsyncDownloadWrapper.java @@ -8,6 +8,13 @@ import org.fdroid.fdroid.ProgressListener; import java.io.IOException; +/** + * Given a {@link org.fdroid.fdroid.net.Downloader}, this wrapper will conduct the download operation on a + * separate thread. All progress/status/error/etc events will be forwarded from that thread to the thread + * that {@link AsyncDownloadWrapper#download()} was invoked on. If you want to respond with UI feedback + * to these events, it is important that you execute the download method of this class from the UI thread. + * That way, all forwarded events will be handled on that thread. + */ public class AsyncDownloadWrapper extends Handler { private static final String TAG = "org.fdroid.fdroid.net.AsyncDownloadWrapper"; @@ -71,6 +78,11 @@ public class AsyncDownloadWrapper extends Handler { downloadThread.interrupt(); } + /** + * Receives "messages" from the download thread, and passes them onto the + * relevant {@link org.fdroid.fdroid.net.AsyncDownloadWrapper.Listener} + * @param message + */ public void handleMessage(Message message) { if (message.arg1 == MSG_PROGRESS) { Bundle data = message.getData(); diff --git a/src/org/fdroid/fdroid/net/Downloader.java b/src/org/fdroid/fdroid/net/Downloader.java index 9be48bd20..8c9f7cf77 100644 --- a/src/org/fdroid/fdroid/net/Downloader.java +++ b/src/org/fdroid/fdroid/net/Downloader.java @@ -47,15 +47,6 @@ public abstract class Downloader { outputStream = new FileOutputStream(outputFile); } - /** - * Downloads to a temporary file, which *you must delete yourself when - * you are done*. - * @see org.fdroid.fdroid.net.Downloader#getFile() - */ - public Downloader(File destFile, Context ctx) throws IOException { - // TODO: Reimplement (is it still necessary? In what context was it being used before?) - } - public Downloader(OutputStream output) throws MalformedURLException { outputStream = output; @@ -158,8 +149,6 @@ public abstract class Downloader { */ private void throwExceptionIfInterrupted() throws InterruptedException { if (Thread.interrupted()) { - // TODO: Do we need to provide more information to whoever needs it, - // so they can, for example, remove any partially created files? Log.d(TAG, "Received interrupt, cancelling download"); throw new InterruptedException(); } From 2f7d6f6452cb7ce44baeb8bdba90deb4b431af24 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 20 May 2014 10:27:13 +1000 Subject: [PATCH 15/17] Made constructor of ApkDownloader public. Previously it was in the org.fdroid.fdroid package, along with most other things. As such, it only need a package-local constructor. However, now it has moved to the .net subpackage, and needs to be made public. --- src/org/fdroid/fdroid/net/ApkDownloader.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/org/fdroid/fdroid/net/ApkDownloader.java b/src/org/fdroid/fdroid/net/ApkDownloader.java index 27d052baf..4d87609a6 100644 --- a/src/org/fdroid/fdroid/net/ApkDownloader.java +++ b/src/org/fdroid/fdroid/net/ApkDownloader.java @@ -70,7 +70,7 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { setProgressListener(null); } - ApkDownloader(Apk apk, String repoAddress, File destDir) { + public ApkDownloader(Apk apk, String repoAddress, File destDir) { curApk = apk; this.repoAddress = repoAddress; localFile = new File(destDir, curApk.apkName); @@ -241,4 +241,4 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { public int getTotalSize() { return totalSize; } -} \ No newline at end of file +} From 009c8a6be5a479d2901070aa265c4f1e5a3e07b2 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 22 May 2014 21:43:00 +0930 Subject: [PATCH 16/17] Don't show progress in AppDetails when using a cached download. The ApkDownloader now returns true or false depending on whether it is using a cached version on disk, or hitting the network. I was also a little more defensive about checking for if downloadHandler is null before deciding to show a progress dialog. --- src/org/fdroid/fdroid/AppDetails.java | 50 ++++++++++++-------- src/org/fdroid/fdroid/net/ApkDownloader.java | 12 ++++- 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/src/org/fdroid/fdroid/AppDetails.java b/src/org/fdroid/fdroid/AppDetails.java index 0281ab08e..e56e6ce79 100644 --- a/src/org/fdroid/fdroid/AppDetails.java +++ b/src/org/fdroid/fdroid/AppDetails.java @@ -425,7 +425,6 @@ public class AppDetails extends ListActivity implements ProgressListener { @Override protected void onResume() { - Log.d(TAG, "onresume"); super.onResume(); // register observer to know when install status changes @@ -1021,8 +1020,9 @@ public class AppDetails extends ListActivity implements ProgressListener { private void startDownload(Apk apk, String repoAddress) { downloadHandler = new ApkDownloader(apk, repoAddress, Utils.getApkCacheDir(getBaseContext())); downloadHandler.setProgressListener(this); - downloadHandler.download(); - updateProgressDialog(); + if (downloadHandler.download()) { + updateProgressDialog(); + } } private void installApk(File file, String packageName) { setProgressBarIndeterminateVisibility(true); @@ -1052,10 +1052,6 @@ public class AppDetails extends ListActivity implements ProgressListener { @Override public void run() { if (operation == Installer.InstallerCallback.OPERATION_INSTALL) { - if (downloadHandler != null) { - downloadHandler = null; - } - PackageManagerCompat.setInstaller(mPm, app.id); } @@ -1121,8 +1117,13 @@ public class AppDetails extends ListActivity implements ProgressListener { pd.setOnCancelListener(new DialogInterface.OnCancelListener() { @Override public void onCancel(DialogInterface dialog) { - downloadHandler.cancel(); - downloadHandler = null; + Log.d(TAG, "User clicked 'cancel' on download, attempting to interrupt download thread."); + if (downloadHandler != null) { + downloadHandler.cancel(); + downloadHandler = null; + } else { + Log.e(TAG, "Tried to cancel, but the downloadHandler doesn't exist."); + } progressDialog = null; Toast.makeText(AppDetails.this, getString(R.string.download_cancelled), Toast.LENGTH_LONG).show(); } @@ -1148,21 +1149,28 @@ public class AppDetails extends ListActivity implements ProgressListener { * {@link org.fdroid.fdroid.ProgressListener.Event}. */ private void updateProgressDialog() { - updateProgressDialog(downloadHandler.getProgress(), downloadHandler.getTotalSize()); + if (downloadHandler != null) { + updateProgressDialog(downloadHandler.getProgress(), downloadHandler.getTotalSize()); + } } private void updateProgressDialog(int progress, int total) { - ProgressDialog pd = getProgressDialog(downloadHandler.getRemoteAddress()); - if (total > 0) { - pd.setIndeterminate(false); - pd.setProgress(progress); - pd.setMax(total); - } else { - pd.setIndeterminate(true); - pd.setProgress(progress); - pd.setMax(0); + if (downloadHandler != null) { + ProgressDialog pd = getProgressDialog(downloadHandler.getRemoteAddress()); + if (total > 0) { + pd.setIndeterminate(false); + pd.setProgress(progress); + pd.setMax(total); + } else { + pd.setIndeterminate(true); + pd.setProgress(progress); + pd.setMax(0); + } + if (!pd.isShowing()) { + Log.d(TAG, "Showing progress dialog for download."); + pd.show(); + } } - pd.show(); } @Override @@ -1200,7 +1208,7 @@ public class AppDetails extends ListActivity implements ProgressListener { switch (requestCode) { case REQUEST_ENABLE_BLUETOOTH: fdroidApp.sendViaBluetooth(this, resultCode, app.id); - break; + break; } } } diff --git a/src/org/fdroid/fdroid/net/ApkDownloader.java b/src/org/fdroid/fdroid/net/ApkDownloader.java index 4d87609a6..2dffe9507 100644 --- a/src/org/fdroid/fdroid/net/ApkDownloader.java +++ b/src/org/fdroid/fdroid/net/ApkDownloader.java @@ -131,12 +131,17 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { } } - public void download() { + /** + * 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 + * want to bother with progress dialogs et al. + */ + public boolean download() { // Can we use the cached version? if (verifyOrDeleteCachedVersion()) { sendMessage(EVENT_APK_DOWNLOAD_COMPLETE); - return; + return false; } String remoteAddress = getRemoteAddress(); @@ -147,12 +152,15 @@ public class ApkDownloader implements AsyncDownloadWrapper.Listener { Downloader downloader = new HttpDownloader(remoteAddress, localFile); dlWrapper = new AsyncDownloadWrapper(downloader, this); dlWrapper.download(); + return true; } catch (MalformedURLException e) { onErrorDownloading(e.getLocalizedMessage()); } catch (IOException e) { onErrorDownloading(e.getLocalizedMessage()); } + + return false; } private void sendMessage(String type) { From b2c63c6dc71b2aa1b5087ee08f5b708cc6f03560 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sat, 24 May 2014 08:42:49 +0930 Subject: [PATCH 17/17] 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(); }