From 012cdce37b02dbc319d3a75f11a152159362b977 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 14 Sep 2015 07:43:19 +1000 Subject: [PATCH 1/4] Make closing of `Downloader`s more concise. The base `Downloader` class now wraps the `InputStream` returned by any child classes, in order to notify the child class when that stream is closed. This prevents each child class having to figure out a way to be notified of this. Also removed some dead code which was unused. --- .../src/org/fdroid/fdroid/RepoUpdater.java | 3 -- .../fdroid/net/BluetoothDownloader.java | 16 ++++---- .../src/org/fdroid/fdroid/net/Downloader.java | 40 ++++++++++++++++++- .../org/fdroid/fdroid/net/HttpDownloader.java | 32 ++------------- .../org/fdroid/fdroid/net/IconDownloader.java | 36 +---------------- 5 files changed, 50 insertions(+), 77 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java b/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java index b043a958f..456290e28 100644 --- a/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java +++ b/F-Droid/src/org/fdroid/fdroid/RepoUpdater.java @@ -108,7 +108,6 @@ public class RepoUpdater { } catch (IOException e) { if (downloader != null && downloader.getFile() != null) { downloader.getFile().delete(); - downloader.close(); } throw new UpdateException(repo, "Error getting index file from " + repo.address, e); @@ -133,8 +132,6 @@ public class RepoUpdater { // successful download, then we will have a file ready to use: processDownloadedFile(downloader.getFile(), downloader.getCacheTag()); } - - downloader.close(); } protected void processDownloadedFile(File downloadedFile, String cacheTag) throws UpdateException { diff --git a/F-Droid/src/org/fdroid/fdroid/net/BluetoothDownloader.java b/F-Droid/src/org/fdroid/fdroid/net/BluetoothDownloader.java index 218258d4a..6ce82cdfb 100644 --- a/F-Droid/src/org/fdroid/fdroid/net/BluetoothDownloader.java +++ b/F-Droid/src/org/fdroid/fdroid/net/BluetoothDownloader.java @@ -1,6 +1,7 @@ package org.fdroid.fdroid.net; import android.content.Context; +import android.support.annotation.Nullable; import android.util.Log; import org.apache.commons.io.input.BoundedInputStream; @@ -31,12 +32,11 @@ public class BluetoothDownloader extends Downloader { } @Override - public InputStream getInputStream() throws IOException { + protected InputStream getDownloadersInputStream() throws IOException { Request request = Request.createGET(sourcePath, connection); Response response = request.send(); fileDetails = response.toFileDetails(); - // TODO: Manage the dependency which includes this class better? // Right now, I only needed the one class from apache commons. // There are countless classes online which provide this functionality, @@ -53,10 +53,8 @@ public class BluetoothDownloader extends Downloader { /** * May return null if an error occurred while getting file details. - * TODO: Should we throw an exception? Everywhere else in this blue package throws IO exceptions weely-neely. - * Will probably require some thought as to how the API looks, with regards to all of the public methods - * and their signatures. */ + @Nullable public FileDetails getFileDetails() { if (fileDetails == null) { Utils.debugLog(TAG, "Going to Bluetooth \"server\" to get file details."); @@ -71,12 +69,14 @@ public class BluetoothDownloader extends Downloader { @Override public boolean hasChanged() { - return getFileDetails().getCacheTag() == null || getFileDetails().getCacheTag().equals(getCacheTag()); + FileDetails details = getFileDetails(); + return details == null || details.getCacheTag() == null || details.getCacheTag().equals(getCacheTag()); } @Override public int totalDownloadSize() { - return getFileDetails().getFileSize(); + FileDetails details = getFileDetails(); + return details != null ? details.getFileSize() : -1; } @Override @@ -94,7 +94,7 @@ public class BluetoothDownloader extends Downloader { } @Override - public void close() { + protected void close() { if (connection != null) connection.closeQuietly(); } diff --git a/F-Droid/src/org/fdroid/fdroid/net/Downloader.java b/F-Droid/src/org/fdroid/fdroid/net/Downloader.java index fcc74eef5..22daabee9 100644 --- a/F-Droid/src/org/fdroid/fdroid/net/Downloader.java +++ b/F-Droid/src/org/fdroid/fdroid/net/Downloader.java @@ -2,6 +2,7 @@ package org.fdroid.fdroid.net; import android.content.Context; import android.content.Intent; +import android.support.annotation.NonNull; import android.support.v4.content.LocalBroadcastManager; import org.fdroid.fdroid.Utils; @@ -35,9 +36,9 @@ public abstract class Downloader { protected int bytesRead; protected int totalBytes; - public abstract InputStream getInputStream() throws IOException; + protected abstract InputStream getDownloadersInputStream() throws IOException; - public abstract void close(); + protected abstract void close() throws IOException; Downloader(Context context, URL url, File destFile) throws FileNotFoundException, MalformedURLException { @@ -47,6 +48,10 @@ public abstract class Downloader { localBroadcastManager = LocalBroadcastManager.getInstance(context); } + public final InputStream getInputStream() throws IOException { + return new WrappedInputStream(getDownloadersInputStream()); + } + /** * 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 @@ -197,4 +202,35 @@ public abstract class Downloader { public int getTotalBytes() { return totalBytes; } + + /** + * Overrides every method in {@link InputStream} and delegates to the wrapped stream. + * The only difference is that when we call the {@link WrappedInputStream#close()} method, + * after delegating to the wrapped stream we invoke the {@link Downloader#close()} method + * on the {@link Downloader} which created this. + */ + private class WrappedInputStream extends InputStream { + + private InputStream toWrap; + + public WrappedInputStream(InputStream toWrap) { + super(); + this.toWrap = toWrap; + } + + @Override + public void close() throws IOException { + toWrap.close(); + Downloader.this.close(); + } + + @Override public int available() throws IOException { return toWrap.available(); } + @Override public void mark(int readlimit) { toWrap.mark(readlimit); } + @Override public boolean markSupported() { return toWrap.markSupported(); } + @Override public int read(@NonNull byte[] buffer) throws IOException { return toWrap.read(buffer); } + @Override public int read(@NonNull byte[] buffer, int byteOffset, int byteCount) throws IOException { return toWrap.read(buffer, byteOffset, byteCount); } + @Override public synchronized void reset() throws IOException { toWrap.reset(); } + @Override public long skip(long byteCount) throws IOException { return toWrap.skip(byteCount); } + @Override public int read() throws IOException { return toWrap.read(); } + } } diff --git a/F-Droid/src/org/fdroid/fdroid/net/HttpDownloader.java b/F-Droid/src/org/fdroid/fdroid/net/HttpDownloader.java index 7be60f056..a29e2bfcd 100644 --- a/F-Droid/src/org/fdroid/fdroid/net/HttpDownloader.java +++ b/F-Droid/src/org/fdroid/fdroid/net/HttpDownloader.java @@ -8,12 +8,10 @@ import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.Utils; import java.io.BufferedInputStream; -import java.io.BufferedReader; import java.io.File; import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; -import java.io.InputStreamReader; import java.net.HttpURLConnection; import java.net.InetSocketAddress; import java.net.MalformedURLException; @@ -30,7 +28,6 @@ public class HttpDownloader extends Downloader { protected static final String HEADER_FIELD_ETAG = "ETag"; protected HttpURLConnection connection; - private InputStream stream; private int statusCode = -1; HttpDownloader(Context context, URL url, File destFile) @@ -38,14 +35,6 @@ public class HttpDownloader extends Downloader { super(context, url, destFile); } - /** - * Calling this makes this downloader not download a file. Instead, it will - * only stream the file through the {@link HttpDownloader#getInputStream()} - */ - public HttpDownloader streamDontDownload() { - return this; - } - /** * Note: Doesn't follow redirects (as far as I'm aware). * {@link BaseImageDownloader#getStreamFromNetwork(String, Object)} has an implementation worth @@ -55,14 +44,9 @@ public class HttpDownloader extends Downloader { * @throws IOException */ - public InputStream getInputStream() throws IOException { + protected InputStream getDownloadersInputStream() throws IOException { setupConnection(); - stream = new BufferedInputStream(connection.getInputStream()); - return stream; - } - - public BufferedReader getBufferedReader() throws IOException { - return new BufferedReader(new InputStreamReader(getInputStream())); + return new BufferedInputStream(connection.getInputStream()); } // Get a remote file. Returns the HTTP response code. @@ -155,18 +139,8 @@ public class HttpDownloader extends Downloader { return this.statusCode != 304; } - public int getStatusCode() { - return statusCode; - } - + @Override public void close() { - try { - if (stream != null) - stream.close(); - } catch (IOException e) { - // ignore - } - connection.disconnect(); } } diff --git a/F-Droid/src/org/fdroid/fdroid/net/IconDownloader.java b/F-Droid/src/org/fdroid/fdroid/net/IconDownloader.java index a27d9da7a..c0389a390 100644 --- a/F-Droid/src/org/fdroid/fdroid/net/IconDownloader.java +++ b/F-Droid/src/org/fdroid/fdroid/net/IconDownloader.java @@ -4,11 +4,8 @@ import android.content.Context; import com.nostra13.universalimageloader.core.download.BaseImageDownloader; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; -import java.util.Locale; public class IconDownloader extends BaseImageDownloader { @@ -22,38 +19,7 @@ public class IconDownloader extends BaseImageDownloader { @Override public InputStream getStream(String imageUri, Object extra) throws IOException { - - Scheme scheme = Scheme.ofUri(imageUri); - - switch (scheme) { - case HTTP: - case HTTPS: - Downloader downloader = DownloaderFactory.create(context, imageUri); - return downloader.getInputStream(); - } - - //bluetooth isn't a scheme in the Scheme. library, so we can add a check here - if (imageUri.toLowerCase(Locale.ENGLISH).startsWith("bluetooth")) { - Downloader downloader = DownloaderFactory.create(context, imageUri); - - ByteArrayOutputStream baos = new ByteArrayOutputStream(); - InputStream is = downloader.getInputStream(); - - int b; - while ((b = is.read()) != -1) - baos.write(b); - - ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray()); - - downloader.close(); - - return bais; - - } - - - return super.getStream(imageUri, extra); - + return DownloaderFactory.create(context, imageUri).getInputStream(); } From f388f32fcf49332028ba011102e9d0b2810f0dfc Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sat, 10 Oct 2015 08:44:44 +1100 Subject: [PATCH 2/4] Don't use download manager for Bluetooth downloads. The F-Droid Bluetooth downloader must be used for these, as it is a custom protocol and Android download manager only understands HTTP and HTTPS. --- F-Droid/src/org/fdroid/fdroid/net/DownloaderFactory.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/F-Droid/src/org/fdroid/fdroid/net/DownloaderFactory.java b/F-Droid/src/org/fdroid/fdroid/net/DownloaderFactory.java index 5e9991c71..2ca730c25 100644 --- a/F-Droid/src/org/fdroid/fdroid/net/DownloaderFactory.java +++ b/F-Droid/src/org/fdroid/fdroid/net/DownloaderFactory.java @@ -108,6 +108,10 @@ public class DownloaderFactory { // We support onion addresses through our own downloader. return false; } + if (isBluetoothAddress(url)) { + // Completely differnet protocol not understood by the download manager. + return false; + } return hasDownloadManager(context); } From 3ad429aa6b9583581b4cb5e8f0c6796ea72ff152 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sat, 10 Oct 2015 08:52:14 +1100 Subject: [PATCH 3/4] Guard against null activity. Came across this whibluetooth swap. I think it is reasonable to guard against null activities here, because we are likely not releasing observers correctly, and thus the observer may receieve a notification when the activity is not attached. --- .../src/org/fdroid/fdroid/views/swap/SwapAppsView.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/views/swap/SwapAppsView.java b/F-Droid/src/org/fdroid/fdroid/views/swap/SwapAppsView.java index c69bee088..aee3a3a46 100644 --- a/F-Droid/src/org/fdroid/fdroid/views/swap/SwapAppsView.java +++ b/F-Droid/src/org/fdroid/fdroid/views/swap/SwapAppsView.java @@ -1,6 +1,7 @@ package org.fdroid.fdroid.views.swap; import android.annotation.TargetApi; +import android.app.Activity; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; @@ -330,9 +331,12 @@ public class SwapAppsView extends ListView implements private final ContentObserver appObserver = new ContentObserver(new Handler()) { @Override public void onChange(boolean selfChange) { - app = AppProvider.Helper.findById(getActivity().getContentResolver(), app.id); - apkToInstall = null; // Force lazy loading to fetch correct apk next time. - resetView(); + Activity activity = getActivity(); + if (activity != null) { + app = AppProvider.Helper.findById(getActivity().getContentResolver(), app.id); + apkToInstall = null; // Force lazy loading to fetch correct apk next time. + resetView(); + } } }; From 94410c5dbc9535e5d6df162d6577faab6cff72b1 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Mon, 12 Oct 2015 08:03:21 +1100 Subject: [PATCH 4/4] Code formatting to pass checkStyle. --- .../src/org/fdroid/fdroid/net/Downloader.java | 47 +++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/net/Downloader.java b/F-Droid/src/org/fdroid/fdroid/net/Downloader.java index 22daabee9..b7529e7e2 100644 --- a/F-Droid/src/org/fdroid/fdroid/net/Downloader.java +++ b/F-Droid/src/org/fdroid/fdroid/net/Downloader.java @@ -224,13 +224,44 @@ public abstract class Downloader { Downloader.this.close(); } - @Override public int available() throws IOException { return toWrap.available(); } - @Override public void mark(int readlimit) { toWrap.mark(readlimit); } - @Override public boolean markSupported() { return toWrap.markSupported(); } - @Override public int read(@NonNull byte[] buffer) throws IOException { return toWrap.read(buffer); } - @Override public int read(@NonNull byte[] buffer, int byteOffset, int byteCount) throws IOException { return toWrap.read(buffer, byteOffset, byteCount); } - @Override public synchronized void reset() throws IOException { toWrap.reset(); } - @Override public long skip(long byteCount) throws IOException { return toWrap.skip(byteCount); } - @Override public int read() throws IOException { return toWrap.read(); } + @Override + public int available() throws IOException { + return toWrap.available(); + } + + @Override + public void mark(int readlimit) { + toWrap.mark(readlimit); + } + + @Override + public boolean markSupported() { + return toWrap.markSupported(); + } + + @Override + public int read(@NonNull byte[] buffer) throws IOException { + return toWrap.read(buffer); + } + + @Override + public int read(@NonNull byte[] buffer, int byteOffset, int byteCount) throws IOException { + return toWrap.read(buffer, byteOffset, byteCount); + } + + @Override + public synchronized void reset() throws IOException { + toWrap.reset(); + } + + @Override + public long skip(long byteCount) throws IOException { + return toWrap.skip(byteCount); + } + + @Override + public int read() throws IOException { + return toWrap.read(); + } } }