From e72ba25fe58d02b77e735c273f97d6553ec69a2c Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 5 May 2016 11:10:05 +1000 Subject: [PATCH 01/17] Call stopService to ensure notification goes away. Always show download notifications. Addresses a bug found in MR !273 whereby removing `stopForeground` results in a persistent "Downloading ..." notification even though it was cancelled. In the process of doing this, it also addresses / Fixes #621 by ensuring that all downloads of apks are done in a foreground service, regardless of the preference used for foreground updater service updating. --- .../fdroid/fdroid/net/DownloaderService.java | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index 0f53a4693..ce1b05c3f 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -43,7 +43,6 @@ import android.util.Log; import org.fdroid.fdroid.AppDetails; import org.fdroid.fdroid.FDroid; -import org.fdroid.fdroid.Preferences; import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.App; @@ -260,10 +259,8 @@ public class DownloaderService extends Service { final String packageName = intent.getStringExtra(EXTRA_PACKAGE_NAME); sendBroadcast(uri, Downloader.ACTION_STARTED, localFile); - if (Preferences.get().isUpdateNotificationEnabled()) { - Notification notification = createNotification(intent.getDataString(), intent.getStringExtra(EXTRA_PACKAGE_NAME)).build(); - startForeground(NOTIFY_DOWNLOADING, notification); - } + Notification notification = createNotification(intent.getDataString(), packageName).build(); + startForeground(NOTIFY_DOWNLOADING, notification); try { downloader = DownloaderFactory.create(this, uri, localFile); @@ -276,13 +273,11 @@ public class DownloaderService extends Service { intent.putExtra(Downloader.EXTRA_TOTAL_BYTES, totalBytes); localBroadcastManager.sendBroadcast(intent); - if (Preferences.get().isUpdateNotificationEnabled()) { - NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - Notification notification = createNotification(uri.toString(), packageName) - .setProgress(totalBytes, bytesRead, false) - .build(); - nm.notify(NOTIFY_DOWNLOADING, notification); - } + NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + Notification notification = createNotification(uri.toString(), packageName) + .setProgress(totalBytes, bytesRead, false) + .build(); + nm.notify(NOTIFY_DOWNLOADING, notification); } }); downloader.download(); @@ -298,6 +293,7 @@ public class DownloaderService extends Service { if (downloader != null) { downloader.close(); } + stopForeground(true); } downloader = null; } From 3a1fcfd2261c61b33bb50e46310ee6bbcb988222 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 5 May 2016 15:17:22 +1000 Subject: [PATCH 02/17] Move `Utils#getApkUrl(String repoAddress, Apk apk)` to `Apk#getUrl()` Added assertions that both apkName and repoAddress need to be populated in order to call `getUrl()`. Also verified that this is the case for all usages of this method, which it should be. All `Apk` objects which currently have `getUrl()` called on them are loaded using the `ApkProvider.Helper.findById()` method without specifying which columns to load (which defaults to all). --- .../java/org/fdroid/fdroid/AppDetails.java | 21 ++++--------------- .../java/org/fdroid/fdroid/UpdateService.java | 10 ++++----- .../main/java/org/fdroid/fdroid/Utils.java | 5 ----- .../main/java/org/fdroid/fdroid/data/Apk.java | 7 +++++++ .../fdroid/views/swap/SwapAppsView.java | 2 +- .../views/swap/SwapWorkflowActivity.java | 2 +- 6 files changed, 17 insertions(+), 30 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index 88d17a270..acf535c08 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -815,9 +815,6 @@ public class AppDetails extends AppCompatActivity { return; } - final String repoaddress = getRepoAddress(apk); - if (repoaddress == null) return; - if (!apk.compatible) { AlertDialog.Builder builder = new AlertDialog.Builder(this); builder.setMessage(R.string.installIncompatible); @@ -826,7 +823,7 @@ public class AppDetails extends AppCompatActivity { @Override public void onClick(DialogInterface dialog, int whichButton) { - startDownload(apk, repoaddress); + startDownload(apk); } }); builder.setNegativeButton(R.string.no, @@ -855,21 +852,11 @@ public class AppDetails extends AppCompatActivity { alert.show(); return; } - startDownload(apk, repoaddress); + startDownload(apk); } - @Nullable - private String getRepoAddress(Apk apk) { - final String[] projection = {RepoProvider.DataColumns.ADDRESS}; - Repo repo = RepoProvider.Helper.findById(this, apk.repo, projection); - if (repo == null || repo.address == null) { - return null; - } - return repo.address; - } - - private void startDownload(Apk apk, String repoAddress) { - activeDownloadUrlString = Utils.getApkUrl(repoAddress, apk); + private void startDownload(Apk apk) { + activeDownloadUrlString = apk.getUrl(); registerDownloaderReceivers(); headerFragment.startProgress(); DownloaderService.queue(this, apk.packageName, activeDownloadUrlString); diff --git a/app/src/main/java/org/fdroid/fdroid/UpdateService.java b/app/src/main/java/org/fdroid/fdroid/UpdateService.java index c3cadf50b..1be4092fd 100644 --- a/app/src/main/java/org/fdroid/fdroid/UpdateService.java +++ b/app/src/main/java/org/fdroid/fdroid/UpdateService.java @@ -393,7 +393,7 @@ public class UpdateService extends IntentService implements ProgressListener { // now that downloading the index is done, start downloading updates if (changes && fdroidPrefs.isAutoDownloadEnabled()) { - autoDownloadUpdates(repo.address); + autoDownloadUpdates(); } } @@ -485,7 +485,7 @@ public class UpdateService extends IntentService implements ProgressListener { return inboxStyle; } - private void autoDownloadUpdates(String repoAddress) { + private void autoDownloadUpdates() { Cursor cursor = getContentResolver().query( AppProvider.getCanUpdateUri(), new String[]{ @@ -496,10 +496,8 @@ public class UpdateService extends IntentService implements ProgressListener { cursor.moveToFirst(); for (int i = 0; i < cursor.getCount(); i++) { App app = new App(cursor); - Apk apk = ApkProvider.Helper.find(this, app.packageName, app.suggestedVersionCode, new String[]{ - ApkProvider.DataColumns.NAME, - }); - String urlString = Utils.getApkUrl(repoAddress, apk); + Apk apk = ApkProvider.Helper.find(this, app.packageName, app.suggestedVersionCode); + String urlString = apk.getUrl(); DownloaderService.queue(this, app.packageName, urlString); cursor.moveToNext(); } diff --git a/app/src/main/java/org/fdroid/fdroid/Utils.java b/app/src/main/java/org/fdroid/fdroid/Utils.java index d0dfa9b1a..e19408a77 100644 --- a/app/src/main/java/org/fdroid/fdroid/Utils.java +++ b/app/src/main/java/org/fdroid/fdroid/Utils.java @@ -39,7 +39,6 @@ import com.nostra13.universalimageloader.utils.StorageUtils; import org.apache.commons.io.FileUtils; import org.fdroid.fdroid.compat.FileCompat; -import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.SanitizedFile; import org.xml.sax.XMLReader; @@ -434,10 +433,6 @@ public final class Utils { return new Locale(languageTag); } - public static String getApkUrl(String repoAddress, Apk apk) { - return repoAddress + "/" + apk.apkName.replace(" ", "%20"); - } - public static final class CommaSeparatedList implements Iterable { private final String value; diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index 387312940..ee68ede09 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -121,6 +121,13 @@ public class Apk extends ValueObject implements Comparable { } } + public String getUrl() { + if (repoAddress == null || apkName == null) { + throw new IllegalStateException("Apk needs to have both ApkProvider.DataColumns.REPO_ADDRESS and ApkProvider.DataColumns.NAME set in order to calculate URL."); + } + return repoAddress + "/" + apkName.replace(" ", "%20"); + } + @Override public String toString() { return packageName + " (version " + versionCode + ")"; diff --git a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java index 009a0399b..d7b42505d 100644 --- a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java +++ b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapAppsView.java @@ -297,7 +297,7 @@ public class SwapAppsView extends ListView implements // TODO: Unregister receivers correctly... Apk apk = getApkToInstall(); - String url = Utils.getApkUrl(apk.repoAddress, apk); + String url = apk.getUrl(); localBroadcastManager = LocalBroadcastManager.getInstance(getActivity()); localBroadcastManager.registerReceiver(appListViewResetReceiver, diff --git a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java index a1585a512..2b063872f 100644 --- a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java +++ b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java @@ -778,7 +778,7 @@ public class SwapWorkflowActivity extends AppCompatActivity { public void install(@NonNull final App app) { final Apk apk = ApkProvider.Helper.find(this, app.packageName, app.suggestedVersionCode); - String urlString = Utils.getApkUrl(apk.repoAddress, apk); + String urlString = apk.getUrl(); downloadCompleteReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { From 2c7033e3673717704ecdf556bb8771930de126e2 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 5 May 2016 12:02:52 +1000 Subject: [PATCH 03/17] Ask for active downloading url in on resume. Fixes #624. The `AppDetails` activity was not correctly asking for the active download url string when being resumed. This change recalculates the value when being resumed now. --- .../java/org/fdroid/fdroid/AppDetails.java | 23 +++++++++++++------ .../fdroid/fdroid/net/DownloaderService.java | 3 +++ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index acf535c08..f63ad894e 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -426,7 +426,12 @@ public class AppDetails extends AppCompatActivity { @Override protected void onResumeFragments() { + // Must be called before super.onResumeFragments(), as the fragments depend on the active + // url being correctly set in order to know whether or not to show the download progress bar. + calcActiveDownloadUrlString(app.packageName); + super.onResumeFragments(); + headerFragment = (AppDetailsHeaderFragment) getSupportFragmentManager().findFragmentById(R.id.header); refreshApkList(); supportInvalidateOptionsMenu(); @@ -582,13 +587,7 @@ public class AppDetails extends AppCompatActivity { Utils.debugLog(TAG, "Getting application details for " + packageName); App newApp = null; - String urlString = getPreferences(MODE_PRIVATE).getString(packageName, null); - if (DownloaderService.isQueuedOrActive(urlString)) { - activeDownloadUrlString = urlString; - } else { - // this URL is no longer active, remove it - PreferencesCompat.apply(getPreferences(MODE_PRIVATE).edit().remove(packageName)); - } + calcActiveDownloadUrlString(packageName); if (!TextUtils.isEmpty(packageName)) { newApp = AppProvider.Helper.findByPackageName(getContentResolver(), packageName); @@ -599,6 +598,16 @@ public class AppDetails extends AppCompatActivity { return this.app != null; } + private void calcActiveDownloadUrlString(String packageName) { + String urlString = getPreferences(MODE_PRIVATE).getString(packageName, null); + if (DownloaderService.isQueuedOrActive(urlString)) { + activeDownloadUrlString = urlString; + } else { + // this URL is no longer active, remove it + PreferencesCompat.apply(getPreferences(MODE_PRIVATE).edit().remove(packageName)); + } + } + /** * If passed null, this will show a message to the user ("Could not find app ..." or something * like that) and then finish the activity. diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index ce1b05c3f..46205d2e7 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -389,6 +389,9 @@ public class DownloaderService extends Service { if (TextUtils.isEmpty(urlString)) { //NOPMD - suggests unreadable format return false; } + if (serviceHandler == null) { + return false; // this service is not even running + } return serviceHandler.hasMessages(urlString.hashCode()) || isActive(urlString); } From 37ba565f5b00d8e98c143bc7107bbcf14ce879a8 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 5 May 2016 12:20:30 +1000 Subject: [PATCH 04/17] Make logging more informative. Before it said: > "doDownload for http://... false" Now it says: > "Starting download for http://... (is resumable: false)" --- app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java index 5cc5f75ea..5b71d6d7a 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java @@ -157,7 +157,7 @@ public class HttpDownloader extends Downloader { if (isCached()) { Utils.debugLog(TAG, sourceUrl + " is cached, so not downloading (HTTP " + statusCode + ")"); } else { - Utils.debugLog(TAG, "doDownload for " + sourceUrl + " " + resumable); + Utils.debugLog(TAG, "Need to download " + sourceUrl + " (is resumable: " + resumable + ")"); downloadFromStream(8192, resumable); updateCacheCheck(); } From 67e66a7b0cb79f14917e78edfe9b8b53756d3652 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 6 May 2016 12:27:47 +0200 Subject: [PATCH 05/17] InstallManagerService skeleton which checks cache before installing DownloaderService is structured to be as simple as possible, and as tightly matched to the downloading lifecycle as possible, with a single queue for all requests to avoid downloads competing for bandwidth. This does not represent the possibilities of the whole install process. For example, downloading can happen in parallel with checking the cache, and if an APK is fully cached, there is no need for it to go through the DownloaderService queue. This also lays the groundwork towards simplifying DownloaderService even more, by moving the Notification handling to InstallManagerService. That will provide a single place to manage all aspects of the Notifications that has a lifecycle that is longer than the Notifications, unlike an Activity or DownloaderService. --- app/src/main/AndroidManifest.xml | 3 + .../java/org/fdroid/fdroid/AppDetails.java | 5 +- .../java/org/fdroid/fdroid/UpdateService.java | 4 +- .../main/java/org/fdroid/fdroid/Utils.java | 11 ++ .../installer/InstallManagerService.java | 115 ++++++++++++++++++ .../fdroid/fdroid/net/DownloaderService.java | 4 +- .../views/swap/SwapWorkflowActivity.java | 3 +- 7 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index ced498306..7b4b8cdd5 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -447,6 +447,9 @@ android:exported="false" /> + diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index f63ad894e..abcd19523 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -37,7 +37,6 @@ import android.os.Build; import android.os.Bundle; import android.os.Handler; import android.support.annotation.NonNull; -import android.support.annotation.Nullable; import android.support.v4.app.Fragment; import android.support.v4.app.ListFragment; import android.support.v4.app.NavUtils; @@ -86,8 +85,8 @@ import org.fdroid.fdroid.data.ApkProvider; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.AppProvider; import org.fdroid.fdroid.data.InstalledAppProvider; -import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.RepoProvider; +import org.fdroid.fdroid.installer.InstallManagerService; import org.fdroid.fdroid.installer.Installer; import org.fdroid.fdroid.installer.Installer.InstallFailedException; import org.fdroid.fdroid.installer.Installer.InstallerCallback; @@ -868,7 +867,7 @@ public class AppDetails extends AppCompatActivity { activeDownloadUrlString = apk.getUrl(); registerDownloaderReceivers(); headerFragment.startProgress(); - DownloaderService.queue(this, apk.packageName, activeDownloadUrlString); + InstallManagerService.queue(this, app, apk); } private void removeApk(String packageName) { diff --git a/app/src/main/java/org/fdroid/fdroid/UpdateService.java b/app/src/main/java/org/fdroid/fdroid/UpdateService.java index 1be4092fd..0cb83a178 100644 --- a/app/src/main/java/org/fdroid/fdroid/UpdateService.java +++ b/app/src/main/java/org/fdroid/fdroid/UpdateService.java @@ -48,6 +48,7 @@ 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.InstallManagerService; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; @@ -497,8 +498,7 @@ public class UpdateService extends IntentService implements ProgressListener { for (int i = 0; i < cursor.getCount(); i++) { App app = new App(cursor); Apk apk = ApkProvider.Helper.find(this, app.packageName, app.suggestedVersionCode); - String urlString = apk.getUrl(); - DownloaderService.queue(this, app.packageName, urlString); + InstallManagerService.queue(this, app, apk); cursor.moveToNext(); } cursor.close(); diff --git a/app/src/main/java/org/fdroid/fdroid/Utils.java b/app/src/main/java/org/fdroid/fdroid/Utils.java index e19408a77..fb1178374 100644 --- a/app/src/main/java/org/fdroid/fdroid/Utils.java +++ b/app/src/main/java/org/fdroid/fdroid/Utils.java @@ -334,6 +334,17 @@ public final class Utils { return apkCacheDir; } + /** + * Get the full path for where an APK URL will be downloaded into. + */ + public static SanitizedFile getApkDownloadPath(Context context, Uri uri) { + File dir = new File(Utils.getApkCacheDir(context), uri.getHost() + "-" + uri.getPort()); + if (!dir.exists()) { + dir.mkdirs(); + } + return new SanitizedFile(dir, uri.getLastPathSegment()); + } + /** * Recursively delete files in {@code dir} that were last modified * {@code secondsAgo} seconds ago, e.g. when it was downloaded. diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java new file mode 100644 index 000000000..9e651b52b --- /dev/null +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -0,0 +1,115 @@ +package org.fdroid.fdroid.installer; + +import android.app.Service; +import android.content.Context; +import android.content.Intent; +import android.net.Uri; +import android.os.IBinder; +import android.support.v4.content.LocalBroadcastManager; + +import org.fdroid.fdroid.Utils; +import org.fdroid.fdroid.data.Apk; +import org.fdroid.fdroid.data.App; +import org.fdroid.fdroid.net.Downloader; +import org.fdroid.fdroid.net.DownloaderService; + +import java.io.File; +import java.util.HashMap; + +/** + * Manages the whole process when a background update triggers an install or the user + * requests an APK to be installed. It handles checking whether the APK is cached, + * downloading it, putting up and maintaining a {@link Notification}, and more. + *

+ * Data is sent via {@link Intent}s so that Android handles the message queuing + * and {@link Service} lifecycle for us, although it adds one layer of redirection + * between the static method to send the {@code Intent} and the method to + * actually process it. + *

+ * The full URL for the APK file to download is also used as the unique ID to + * represent the download itself throughout F-Droid. This follows the model + * of {@link Intent#setData(Uri)}, where the core data of an {@code Intent} is + * a {@code Uri}. The full download URL is guaranteed to be unique since it + * points to files on a filesystem, where there cannot be multiple files with + * the same name. This provides a unique ID beyond just {@code packageName} + * and {@code versionCode} since there could be different copies of the same + * APK on different servers, signed by different keys, or even different builds. + *

    + *
  • for a {@link Uri} ID, use {@code Uri}, {@link Intent#getData()} + *
  • for a {@code String} ID, use {@code urlString}, {@link Uri#toString()}, or + * {@link Intent#getDataString()} + *
  • for an {@code int} ID, use {@link String#hashCode()} + *

+ */ +public class InstallManagerService extends Service { + public static final String TAG = "InstallManagerService"; + + private static final String ACTION_INSTALL = "org.fdroid.fdroid.InstallManagerService.action.INSTALL"; + + /** + * The collection of APKs that are actively going through this whole process. + */ + private static final HashMap ACTIVE_APKS = new HashMap(3); + + private LocalBroadcastManager localBroadcastManager; + + /** + * This service does not use binding, so no need to implement this method + */ + @Override + public IBinder onBind(Intent intent) { + return null; + } + + @Override + public void onCreate() { + super.onCreate(); + Utils.debugLog(TAG, "creating Service"); + localBroadcastManager = LocalBroadcastManager.getInstance(this); + } + + @Override + public int onStartCommand(Intent intent, int flags, int startId) { + Utils.debugLog(TAG, "onStartCommand " + intent); + String urlString = intent.getDataString(); + Apk apk = ACTIVE_APKS.get(urlString); + File apkFilePath = Utils.getApkDownloadPath(this, intent.getData()); + long apkFileSize = apkFilePath.length(); + if (!apkFilePath.exists() || apkFileSize < apk.size) { + Utils.debugLog(TAG, "download " + urlString + " " + apkFilePath); + DownloaderService.queue(this, apk.packageName, urlString); + } else if (apkFileSize == apk.size) { + Utils.debugLog(TAG, "skip download, we have it, straight to install " + urlString + " " + apkFilePath); + sendBroadcast(intent.getData(), Downloader.ACTION_STARTED, apkFilePath); + sendBroadcast(intent.getData(), Downloader.ACTION_COMPLETE, apkFilePath); + } else { + Utils.debugLog(TAG, " delete and download again " + urlString + " " + apkFilePath); + apkFilePath.delete(); + DownloaderService.queue(this, apk.packageName, urlString); + } + return START_REDELIVER_INTENT; // if killed before completion, retry Intent + } + + private void sendBroadcast(Uri uri, String action, File file) { + Intent intent = new Intent(action); + intent.setData(uri); + intent.putExtra(Downloader.EXTRA_DOWNLOAD_PATH, file.getAbsolutePath()); + localBroadcastManager.sendBroadcast(intent); + } + + /** + * Install an APK, checking the cache and downloading if necessary before starting the process. + * All notifications are sent as an {@link Intent} via local broadcasts to be received by + * + * @param context this app's {@link Context} + */ + public static void queue(Context context, App app, Apk apk) { + String urlString = apk.getUrl(); + Utils.debugLog(TAG, "queue " + app.packageName + " " + apk.versionCode + " from " + urlString); + ACTIVE_APKS.put(urlString, apk); + Intent intent = new Intent(context, InstallManagerService.class); + intent.setAction(ACTION_INSTALL); + intent.setData(Uri.parse(urlString)); + context.startService(intent); + } +} diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index 46205d2e7..e5685ac49 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -253,9 +253,7 @@ public class DownloaderService extends Service { */ protected void handleIntent(Intent intent) { final Uri uri = intent.getData(); - File downloadDir = new File(Utils.getApkCacheDir(this), uri.getHost() + "-" + uri.getPort()); - downloadDir.mkdirs(); - final SanitizedFile localFile = new SanitizedFile(downloadDir, uri.getLastPathSegment()); + final SanitizedFile localFile = Utils.getApkDownloadPath(this, uri); final String packageName = intent.getStringExtra(EXTRA_PACKAGE_NAME); sendBroadcast(uri, Downloader.ACTION_STARTED, localFile); diff --git a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java index 2b063872f..6b0b46aaf 100644 --- a/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java +++ b/app/src/main/java/org/fdroid/fdroid/views/swap/SwapWorkflowActivity.java @@ -41,6 +41,7 @@ import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.ApkProvider; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.NewRepoConfig; +import org.fdroid.fdroid.installer.InstallManagerService; import org.fdroid.fdroid.installer.Installer; import org.fdroid.fdroid.localrepo.LocalRepoManager; import org.fdroid.fdroid.localrepo.SwapService; @@ -788,7 +789,7 @@ public class SwapWorkflowActivity extends AppCompatActivity { }; localBroadcastManager.registerReceiver(downloadCompleteReceiver, DownloaderService.getIntentFilter(urlString, Downloader.ACTION_COMPLETE)); - DownloaderService.queue(this, app.packageName, urlString); + InstallManagerService.queue(this, app, apk); } private void handleDownloadComplete(File apkFile, String packageName, String urlString) { From 08988f2369efc9352648c079d3a177b480c6fc03 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 6 May 2016 12:48:26 +0200 Subject: [PATCH 06/17] move all downloading notifications to InstallManagerService This keeps DownloaderService tightly focused on downloading, and makes it a lot easier to manage Notifications since InstallManagerService's lifecycle lasts as long as the Notifications, unlike DownloaderService. --- .../installer/InstallManagerService.java | 165 +++++++++++++++++- .../fdroid/fdroid/net/DownloaderService.java | 117 +------------ 2 files changed, 167 insertions(+), 115 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index 9e651b52b..7c3a725b8 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -1,15 +1,27 @@ package org.fdroid.fdroid.installer; +import android.app.Notification; +import android.app.NotificationManager; +import android.app.PendingIntent; import android.app.Service; +import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; +import android.content.pm.PackageManager; import android.net.Uri; import android.os.IBinder; +import android.support.annotation.Nullable; +import android.support.v4.app.NotificationCompat; +import android.support.v4.app.TaskStackBuilder; import android.support.v4.content.LocalBroadcastManager; +import org.fdroid.fdroid.AppDetails; +import org.fdroid.fdroid.FDroid; +import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.App; +import org.fdroid.fdroid.data.AppProvider; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; @@ -45,12 +57,19 @@ public class InstallManagerService extends Service { public static final String TAG = "InstallManagerService"; private static final String ACTION_INSTALL = "org.fdroid.fdroid.InstallManagerService.action.INSTALL"; + private static final int NOTIFY_DOWNLOADING = 0x2344; /** * The collection of APKs that are actively going through this whole process. */ private static final HashMap ACTIVE_APKS = new HashMap(3); + /** + * The array of active {@link BroadcastReceiver}s for each active APK. The key is the + * download URL, as in {@link Apk#getUrl()} or {@code urlString}. + */ + private final HashMap receivers = new HashMap(3); + private LocalBroadcastManager localBroadcastManager; /** @@ -73,11 +92,17 @@ public class InstallManagerService extends Service { Utils.debugLog(TAG, "onStartCommand " + intent); String urlString = intent.getDataString(); Apk apk = ACTIVE_APKS.get(urlString); + + Notification notification = createNotification(intent.getDataString(), apk.packageName).build(); + startForeground(NOTIFY_DOWNLOADING, notification); + + registerDownloaderReceivers(urlString); + File apkFilePath = Utils.getApkDownloadPath(this, intent.getData()); long apkFileSize = apkFilePath.length(); if (!apkFilePath.exists() || apkFileSize < apk.size) { Utils.debugLog(TAG, "download " + urlString + " " + apkFilePath); - DownloaderService.queue(this, apk.packageName, urlString); + DownloaderService.queue(this, urlString); } else if (apkFileSize == apk.size) { Utils.debugLog(TAG, "skip download, we have it, straight to install " + urlString + " " + apkFilePath); sendBroadcast(intent.getData(), Downloader.ACTION_STARTED, apkFilePath); @@ -85,7 +110,7 @@ public class InstallManagerService extends Service { } else { Utils.debugLog(TAG, " delete and download again " + urlString + " " + apkFilePath); apkFilePath.delete(); - DownloaderService.queue(this, apk.packageName, urlString); + DownloaderService.queue(this, urlString); } return START_REDELIVER_INTENT; // if killed before completion, retry Intent } @@ -97,6 +122,142 @@ public class InstallManagerService extends Service { localBroadcastManager.sendBroadcast(intent); } + private void unregisterDownloaderReceivers(String urlString) { + for (BroadcastReceiver receiver : receivers.get(urlString)) { + localBroadcastManager.unregisterReceiver(receiver); + } + } + + private void registerDownloaderReceivers(String urlString) { + BroadcastReceiver startedReceiver = new BroadcastReceiver() { + @Override + public void onReceive(Context context, Intent intent) { + } + }; + BroadcastReceiver progressReceiver = new BroadcastReceiver() { + @Override + public void onReceive(Context context, Intent intent) { + String urlString = intent.getDataString(); + Apk apk = ACTIVE_APKS.get(urlString); + int bytesRead = intent.getIntExtra(Downloader.EXTRA_BYTES_READ, 0); + int totalBytes = intent.getIntExtra(Downloader.EXTRA_TOTAL_BYTES, 0); + NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + Notification notification = createNotification(urlString, apk.packageName) + .setProgress(totalBytes, bytesRead, false) + .build(); + nm.notify(NOTIFY_DOWNLOADING, notification); + } + }; + BroadcastReceiver completeReceiver = new BroadcastReceiver() { + @Override + public void onReceive(Context context, Intent intent) { + String urlString = intent.getDataString(); + Apk apk = ACTIVE_APKS.remove(urlString); + notifyDownloadComplete(apk.packageName, intent.getDataString()); + unregisterDownloaderReceivers(urlString); + } + }; + BroadcastReceiver interruptedReceiver = new BroadcastReceiver() { + @Override + public void onReceive(Context context, Intent intent) { + String urlString = intent.getDataString(); + ACTIVE_APKS.remove(urlString); + unregisterDownloaderReceivers(urlString); + } + }; + localBroadcastManager.registerReceiver(startedReceiver, + DownloaderService.getIntentFilter(urlString, Downloader.ACTION_STARTED)); + localBroadcastManager.registerReceiver(progressReceiver, + DownloaderService.getIntentFilter(urlString, Downloader.ACTION_PROGRESS)); + localBroadcastManager.registerReceiver(completeReceiver, + DownloaderService.getIntentFilter(urlString, Downloader.ACTION_COMPLETE)); + localBroadcastManager.registerReceiver(interruptedReceiver, + DownloaderService.getIntentFilter(urlString, Downloader.ACTION_INTERRUPTED)); + receivers.put(urlString, new BroadcastReceiver[]{ + startedReceiver, progressReceiver, completeReceiver, interruptedReceiver, + }); + } + + private NotificationCompat.Builder createNotification(String urlString, @Nullable String packageName) { + return new NotificationCompat.Builder(this) + .setAutoCancel(true) + .setContentIntent(createAppDetailsIntent(0, packageName)) + .setContentTitle(getNotificationTitle(packageName)) + .addAction(R.drawable.ic_cancel_black_24dp, getString(R.string.cancel), + DownloaderService.createCancelDownloadIntent(this, 0, urlString)) + .setSmallIcon(android.R.drawable.stat_sys_download) + .setContentText(urlString) + .setProgress(100, 0, true); + } + + /** + * If downloading an apk (i.e. packageName != null) then the title will indicate + * the name of the app which the apk belongs to. Otherwise, it will be a generic "Downloading..." + * message. + */ + private String getNotificationTitle(@Nullable String packageName) { + String title; + if (packageName != null) { + App app = AppProvider.Helper.findByPackageName( + getContentResolver(), packageName, new String[]{AppProvider.DataColumns.NAME}); + title = getString(R.string.downloading_apk, app.name); + } else { + title = getString(R.string.downloading); + } + return title; + } + + private PendingIntent createAppDetailsIntent(int requestCode, String packageName) { + TaskStackBuilder stackBuilder; + if (packageName != null) { + Intent notifyIntent = new Intent(getApplicationContext(), AppDetails.class) + .putExtra(AppDetails.EXTRA_APPID, packageName); + + stackBuilder = TaskStackBuilder + .create(getApplicationContext()) + .addParentStack(AppDetails.class) + .addNextIntent(notifyIntent); + } else { + Intent notifyIntent = new Intent(getApplicationContext(), FDroid.class); + stackBuilder = TaskStackBuilder + .create(getApplicationContext()) + .addParentStack(FDroid.class) + .addNextIntent(notifyIntent); + } + + return stackBuilder.getPendingIntent(requestCode, PendingIntent.FLAG_UPDATE_CURRENT); + } + + /** + * Post a notification about a completed download. {@code packageName} must be a valid + * and currently in the app index database. + */ + private void notifyDownloadComplete(String packageName, String urlString) { + String title; + try { + PackageManager pm = getPackageManager(); + title = String.format(getString(R.string.tap_to_update_format), + pm.getApplicationLabel(pm.getApplicationInfo(packageName, 0))); + } catch (PackageManager.NameNotFoundException e) { + App app = AppProvider.Helper.findByPackageName(getContentResolver(), packageName, + new String[]{ + AppProvider.DataColumns.NAME, + }); + title = String.format(getString(R.string.tap_to_install_format), app.name); + } + + int downloadUrlId = urlString.hashCode(); + NotificationCompat.Builder builder = + new NotificationCompat.Builder(this) + .setAutoCancel(true) + .setContentTitle(title) + .setSmallIcon(android.R.drawable.stat_sys_download_done) + .setContentIntent(createAppDetailsIntent(downloadUrlId, packageName)) + .setContentText(getString(R.string.tap_to_install)); + NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + nm.notify(downloadUrlId, builder.build()); + } + /** * Install an APK, checking the cache and downloading if necessary before starting the process. * All notifications are sent as an {@link Intent} via local broadcasts to be received by diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index e5685ac49..7a5a0f37e 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -17,14 +17,11 @@ package org.fdroid.fdroid.net; -import android.app.Notification; -import android.app.NotificationManager; import android.app.PendingIntent; import android.app.Service; import android.content.Context; import android.content.Intent; import android.content.IntentFilter; -import android.content.pm.PackageManager; import android.net.Uri; import android.os.Handler; import android.os.HandlerThread; @@ -34,19 +31,11 @@ import android.os.Message; import android.os.PatternMatcher; import android.os.Process; import android.support.annotation.NonNull; -import android.support.annotation.Nullable; -import android.support.v4.app.NotificationCompat; -import android.support.v4.app.TaskStackBuilder; import android.support.v4.content.LocalBroadcastManager; import android.text.TextUtils; import android.util.Log; -import org.fdroid.fdroid.AppDetails; -import org.fdroid.fdroid.FDroid; -import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; -import org.fdroid.fdroid.data.App; -import org.fdroid.fdroid.data.AppProvider; import org.fdroid.fdroid.data.SanitizedFile; import java.io.File; @@ -85,13 +74,9 @@ import java.net.URL; public class DownloaderService extends Service { private static final String TAG = "DownloaderService"; - private static final String EXTRA_PACKAGE_NAME = "org.fdroid.fdroid.net.DownloaderService.extra.PACKAGE_NAME"; - private static final String ACTION_QUEUE = "org.fdroid.fdroid.net.DownloaderService.action.QUEUE"; private static final String ACTION_CANCEL = "org.fdroid.fdroid.net.DownloaderService.action.CANCEL"; - private static final int NOTIFY_DOWNLOADING = 0x2344; - private volatile Looper serviceLooper; private static volatile ServiceHandler serviceHandler; private static volatile Downloader downloader; @@ -153,55 +138,6 @@ public class DownloaderService extends Service { } } - private NotificationCompat.Builder createNotification(String urlString, @Nullable String packageName) { - return new NotificationCompat.Builder(this) - .setAutoCancel(true) - .setContentIntent(createAppDetailsIntent(0, packageName)) - .setContentTitle(getNotificationTitle(packageName)) - .addAction(R.drawable.ic_cancel_black_24dp, getString(R.string.cancel), - createCancelDownloadIntent(this, 0, urlString)) - .setSmallIcon(android.R.drawable.stat_sys_download) - .setContentText(urlString) - .setProgress(100, 0, true); - } - - /** - * If downloading an apk (i.e. packageName != null) then the title will indicate - * the name of the app which the apk belongs to. Otherwise, it will be a generic "Downloading..." - * message. - */ - private String getNotificationTitle(@Nullable String packageName) { - if (packageName != null) { - final App app = AppProvider.Helper.findByPackageName( - getContentResolver(), packageName, new String[]{AppProvider.DataColumns.NAME}); - if (app != null) { - return getString(R.string.downloading_apk, app.name); - } - } - return getString(R.string.downloading); - } - - private PendingIntent createAppDetailsIntent(int requestCode, String packageName) { - TaskStackBuilder stackBuilder; - if (packageName != null) { - Intent notifyIntent = new Intent(getApplicationContext(), AppDetails.class) - .putExtra(AppDetails.EXTRA_APPID, packageName); - - stackBuilder = TaskStackBuilder - .create(getApplicationContext()) - .addParentStack(AppDetails.class) - .addNextIntent(notifyIntent); - } else { - Intent notifyIntent = new Intent(getApplicationContext(), FDroid.class); - stackBuilder = TaskStackBuilder - .create(getApplicationContext()) - .addParentStack(FDroid.class) - .addNextIntent(notifyIntent); - } - - return stackBuilder.getPendingIntent(requestCode, PendingIntent.FLAG_UPDATE_CURRENT); - } - public static PendingIntent createCancelDownloadIntent(@NonNull Context context, int requestCode, @NonNull String urlString) { Intent cancelIntent = new Intent(context.getApplicationContext(), DownloaderService.class) @@ -254,12 +190,8 @@ public class DownloaderService extends Service { protected void handleIntent(Intent intent) { final Uri uri = intent.getData(); final SanitizedFile localFile = Utils.getApkDownloadPath(this, uri); - final String packageName = intent.getStringExtra(EXTRA_PACKAGE_NAME); sendBroadcast(uri, Downloader.ACTION_STARTED, localFile); - Notification notification = createNotification(intent.getDataString(), packageName).build(); - startForeground(NOTIFY_DOWNLOADING, notification); - try { downloader = DownloaderFactory.create(this, uri, localFile); downloader.setListener(new Downloader.DownloaderProgressListener() { @@ -270,17 +202,10 @@ public class DownloaderService extends Service { intent.putExtra(Downloader.EXTRA_BYTES_READ, bytesRead); intent.putExtra(Downloader.EXTRA_TOTAL_BYTES, totalBytes); localBroadcastManager.sendBroadcast(intent); - - NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - Notification notification = createNotification(uri.toString(), packageName) - .setProgress(totalBytes, bytesRead, false) - .build(); - nm.notify(NOTIFY_DOWNLOADING, notification); } }); downloader.download(); sendBroadcast(uri, Downloader.ACTION_COMPLETE, localFile); - notifyDownloadComplete(packageName, intent.getDataString()); } catch (InterruptedException e) { sendBroadcast(uri, Downloader.ACTION_INTERRUPTED, localFile); } catch (IOException e) { @@ -296,36 +221,6 @@ public class DownloaderService extends Service { downloader = null; } - /** - * Post a notification about a completed download. {@code packageName} must be a valid - * and currently in the app index database. - */ - private void notifyDownloadComplete(String packageName, String urlString) { - String title; - try { - PackageManager pm = getPackageManager(); - title = String.format(getString(R.string.tap_to_update_format), - pm.getApplicationLabel(pm.getApplicationInfo(packageName, 0))); - } catch (PackageManager.NameNotFoundException e) { - App app = AppProvider.Helper.findByPackageName(getContentResolver(), packageName, - new String[]{ - AppProvider.DataColumns.NAME, - }); - title = String.format(getString(R.string.tap_to_install_format), app.name); - } - - int downloadUrlId = urlString.hashCode(); - NotificationCompat.Builder builder = - new NotificationCompat.Builder(this) - .setAutoCancel(true) - .setContentTitle(title) - .setSmallIcon(android.R.drawable.stat_sys_download_done) - .setContentIntent(createAppDetailsIntent(downloadUrlId, packageName)) - .setContentText(getString(R.string.tap_to_install)); - NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - nm.notify(downloadUrlId, builder.build()); - } - private void sendBroadcast(Uri uri, String action, File file) { sendBroadcast(uri, action, file, null); } @@ -345,19 +240,15 @@ public class DownloaderService extends Service { *

* All notifications are sent as an {@link Intent} via local broadcasts to be received by * - * @param context this app's {@link Context} - * @param packageName The packageName of the app being downloaded - * @param urlString The URL to add to the download queue + * @param context this app's {@link Context} + * @param urlString The URL to add to the download queue * @see #cancel(Context, String) */ - public static void queue(Context context, String packageName, String urlString) { + public static void queue(Context context, String urlString) { Utils.debugLog(TAG, "Preparing " + urlString + " to go into the download queue"); Intent intent = new Intent(context, DownloaderService.class); intent.setAction(ACTION_QUEUE); intent.setData(Uri.parse(urlString)); - if (!TextUtils.isEmpty(packageName)) { - intent.putExtra(EXTRA_PACKAGE_NAME, packageName); - } context.startService(intent); } @@ -368,7 +259,7 @@ public class DownloaderService extends Service { * * @param context this app's {@link Context} * @param urlString The URL to remove from the download queue - * @see #queue(Context, String, String) + * @see #queue(Context, String) */ public static void cancel(Context context, String urlString) { Utils.debugLog(TAG, "Preparing cancellation of " + urlString + " download"); From dded004321b5ac40abba499d4dafeb2ea16d010c Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 6 May 2016 13:22:40 +0200 Subject: [PATCH 07/17] use standard URL ID int for Intents used in Notifications This keeps the IDs standard throughout the code: either urlString when it should be a String, or urlString.hashCode() when it should be an int. It also follows the naming convention in DownloaderService helper methods, e.g. getIntentFilter(). "create" to me doesn't necessarily mean also "get" Using @NonNull or @Nullable is fine when it is actually useful, like the compiler can catch errors, but it also adds a lot of noise when reading the code. For example, @NonNull here will just make people avoid thinking. Context can never be null anywhere in Android, that's a given throughout the Android API. And in this code, urlString is the unique ID used throughout the process, so if its ever null, nothing works. --- .../fdroid/fdroid/installer/InstallManagerService.java | 9 +++++---- .../java/org/fdroid/fdroid/net/DownloaderService.java | 6 ++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index 7c3a725b8..a617d9505 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -179,12 +179,13 @@ public class InstallManagerService extends Service { } private NotificationCompat.Builder createNotification(String urlString, @Nullable String packageName) { + int downloadUrlId = urlString.hashCode(); return new NotificationCompat.Builder(this) .setAutoCancel(true) - .setContentIntent(createAppDetailsIntent(0, packageName)) + .setContentIntent(getAppDetailsIntent(downloadUrlId, apk.packageName)) .setContentTitle(getNotificationTitle(packageName)) .addAction(R.drawable.ic_cancel_black_24dp, getString(R.string.cancel), - DownloaderService.createCancelDownloadIntent(this, 0, urlString)) + DownloaderService.getCancelPendingIntent(this, urlString)) .setSmallIcon(android.R.drawable.stat_sys_download) .setContentText(urlString) .setProgress(100, 0, true); @@ -207,7 +208,7 @@ public class InstallManagerService extends Service { return title; } - private PendingIntent createAppDetailsIntent(int requestCode, String packageName) { + private PendingIntent getAppDetailsIntent(int requestCode, String packageName) { TaskStackBuilder stackBuilder; if (packageName != null) { Intent notifyIntent = new Intent(getApplicationContext(), AppDetails.class) @@ -252,7 +253,7 @@ public class InstallManagerService extends Service { .setAutoCancel(true) .setContentTitle(title) .setSmallIcon(android.R.drawable.stat_sys_download_done) - .setContentIntent(createAppDetailsIntent(downloadUrlId, packageName)) + .setContentIntent(getAppDetailsIntent(downloadUrlId, packageName)) .setContentText(getString(R.string.tap_to_install)); NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); nm.notify(downloadUrlId, builder.build()); diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index 7a5a0f37e..21061f260 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -30,7 +30,6 @@ import android.os.Looper; import android.os.Message; import android.os.PatternMatcher; import android.os.Process; -import android.support.annotation.NonNull; import android.support.v4.content.LocalBroadcastManager; import android.text.TextUtils; import android.util.Log; @@ -138,13 +137,12 @@ public class DownloaderService extends Service { } } - public static PendingIntent createCancelDownloadIntent(@NonNull Context context, int - requestCode, @NonNull String urlString) { + public static PendingIntent getCancelPendingIntent(Context context, String urlString) { Intent cancelIntent = new Intent(context.getApplicationContext(), DownloaderService.class) .setData(Uri.parse(urlString)) .setAction(ACTION_CANCEL); return PendingIntent.getService(context.getApplicationContext(), - requestCode, + urlString.hashCode(), cancelIntent, PendingIntent.FLAG_CANCEL_CURRENT); } From f195c34a8bf90cc55e2f473210836c0bf7f24e5e Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Sat, 7 May 2016 00:30:05 +0200 Subject: [PATCH 08/17] make Apk the common internal data type Standardizing on Apk as the internal data type means that most of the data that is needed for the whole lifecycle of a given APK going through this process will be available regardless of the database changes. Once App instances are also included, then all of the data should be available separately from the database. This is important to support parallel operation. The index could be updated and an app could disappear while an APK of that app is being downloaded. In that case, it should not show blank notifications. Also, in AppDetail, the Apk instance is completely loaded from the db, so there should not be any nulls on the essential bits like packageName and download URL. --- .../installer/InstallManagerService.java | 82 +++++++++---------- 1 file changed, 37 insertions(+), 45 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index a617d9505..b801a8f55 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -10,13 +10,12 @@ import android.content.Intent; import android.content.pm.PackageManager; import android.net.Uri; import android.os.IBinder; -import android.support.annotation.Nullable; import android.support.v4.app.NotificationCompat; import android.support.v4.app.TaskStackBuilder; import android.support.v4.content.LocalBroadcastManager; +import android.text.TextUtils; import org.fdroid.fdroid.AppDetails; -import org.fdroid.fdroid.FDroid; import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Apk; @@ -93,7 +92,7 @@ public class InstallManagerService extends Service { String urlString = intent.getDataString(); Apk apk = ACTIVE_APKS.get(urlString); - Notification notification = createNotification(intent.getDataString(), apk.packageName).build(); + Notification notification = createNotification(intent.getDataString(), apk).build(); startForeground(NOTIFY_DOWNLOADING, notification); registerDownloaderReceivers(urlString); @@ -142,7 +141,7 @@ public class InstallManagerService extends Service { int bytesRead = intent.getIntExtra(Downloader.EXTRA_BYTES_READ, 0); int totalBytes = intent.getIntExtra(Downloader.EXTRA_TOTAL_BYTES, 0); NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - Notification notification = createNotification(urlString, apk.packageName) + Notification notification = createNotification(urlString, apk) .setProgress(totalBytes, bytesRead, false) .build(); nm.notify(NOTIFY_DOWNLOADING, notification); @@ -153,7 +152,7 @@ public class InstallManagerService extends Service { public void onReceive(Context context, Intent intent) { String urlString = intent.getDataString(); Apk apk = ACTIVE_APKS.remove(urlString); - notifyDownloadComplete(apk.packageName, intent.getDataString()); + notifyDownloadComplete(apk, urlString); unregisterDownloaderReceivers(urlString); } }; @@ -178,12 +177,12 @@ public class InstallManagerService extends Service { }); } - private NotificationCompat.Builder createNotification(String urlString, @Nullable String packageName) { + private NotificationCompat.Builder createNotification(String urlString, Apk apk) { int downloadUrlId = urlString.hashCode(); return new NotificationCompat.Builder(this) .setAutoCancel(true) - .setContentIntent(getAppDetailsIntent(downloadUrlId, apk.packageName)) - .setContentTitle(getNotificationTitle(packageName)) + .setContentIntent(getAppDetailsIntent(downloadUrlId, apk)) + .setContentTitle(getNotificationTitle(urlString, apk)) .addAction(R.drawable.ic_cancel_black_24dp, getString(R.string.cancel), DownloaderService.getCancelPendingIntent(this, urlString)) .setSmallIcon(android.R.drawable.stat_sys_download) @@ -191,60 +190,53 @@ public class InstallManagerService extends Service { .setProgress(100, 0, true); } - /** - * If downloading an apk (i.e. packageName != null) then the title will indicate - * the name of the app which the apk belongs to. Otherwise, it will be a generic "Downloading..." - * message. - */ - private String getNotificationTitle(@Nullable String packageName) { - String title; - if (packageName != null) { - App app = AppProvider.Helper.findByPackageName( - getContentResolver(), packageName, new String[]{AppProvider.DataColumns.NAME}); - title = getString(R.string.downloading_apk, app.name); + private String getAppName(Apk apk) { + App app = AppProvider.Helper.findByPackageName( + getContentResolver(), apk.packageName, new String[]{AppProvider.DataColumns.NAME}); + if (app != null && !TextUtils.isEmpty(app.name)) { + return app.name; } else { - title = getString(R.string.downloading); + return null; } - return title; } - private PendingIntent getAppDetailsIntent(int requestCode, String packageName) { - TaskStackBuilder stackBuilder; - if (packageName != null) { - Intent notifyIntent = new Intent(getApplicationContext(), AppDetails.class) - .putExtra(AppDetails.EXTRA_APPID, packageName); - - stackBuilder = TaskStackBuilder - .create(getApplicationContext()) - .addParentStack(AppDetails.class) - .addNextIntent(notifyIntent); + private String getNotificationTitle(String urlString, Apk apk) { + String name = getAppName(apk); + if (TextUtils.isEmpty(name)) { + // this is ugly, but its better than nothing as a failsafe + return getString(R.string.downloading_apk, urlString); } else { - Intent notifyIntent = new Intent(getApplicationContext(), FDroid.class); - stackBuilder = TaskStackBuilder - .create(getApplicationContext()) - .addParentStack(FDroid.class) - .addNextIntent(notifyIntent); + return getString(R.string.downloading_apk, name); } + } - return stackBuilder.getPendingIntent(requestCode, PendingIntent.FLAG_UPDATE_CURRENT); + /** + * Get a {@link PendingIntent} for a {@link Notification} to send when it + * is clicked. {@link AppDetails} handles {@code Intent}s that are missing + * or bad {@link AppDetails#EXTRA_APPID}, so it does not need to be checked + * here. + */ + private PendingIntent getAppDetailsIntent(int requestCode, Apk apk) { + Intent notifyIntent = new Intent(getApplicationContext(), AppDetails.class) + .putExtra(AppDetails.EXTRA_APPID, apk.packageName); + return TaskStackBuilder.create(getApplicationContext()) + .addParentStack(AppDetails.class) + .addNextIntent(notifyIntent) + .getPendingIntent(requestCode, PendingIntent.FLAG_UPDATE_CURRENT); } /** * Post a notification about a completed download. {@code packageName} must be a valid * and currently in the app index database. */ - private void notifyDownloadComplete(String packageName, String urlString) { + private void notifyDownloadComplete(Apk apk, String urlString) { String title; try { PackageManager pm = getPackageManager(); title = String.format(getString(R.string.tap_to_update_format), - pm.getApplicationLabel(pm.getApplicationInfo(packageName, 0))); + pm.getApplicationLabel(pm.getApplicationInfo(apk.packageName, 0))); } catch (PackageManager.NameNotFoundException e) { - App app = AppProvider.Helper.findByPackageName(getContentResolver(), packageName, - new String[]{ - AppProvider.DataColumns.NAME, - }); - title = String.format(getString(R.string.tap_to_install_format), app.name); + title = String.format(getString(R.string.tap_to_install_format), getAppName(apk)); } int downloadUrlId = urlString.hashCode(); @@ -253,7 +245,7 @@ public class InstallManagerService extends Service { .setAutoCancel(true) .setContentTitle(title) .setSmallIcon(android.R.drawable.stat_sys_download_done) - .setContentIntent(getAppDetailsIntent(downloadUrlId, packageName)) + .setContentIntent(getAppDetailsIntent(downloadUrlId, apk)) .setContentText(getString(R.string.tap_to_install)); NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); nm.notify(downloadUrlId, builder.build()); From 96c36d85c4966d65a668786fea6d6bf1cc6f465b Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Sat, 7 May 2016 00:51:17 +0200 Subject: [PATCH 09/17] keep App instances for each active APK in the install process This allows the install process to have consistent data, even if the index database changes while an APK is making its way through this process. This also provides a set of packageNames to be easily queried. --- .../installer/InstallManagerService.java | 59 +++++++++++++------ 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index b801a8f55..c6dd31608 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -20,12 +20,12 @@ import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.App; -import org.fdroid.fdroid.data.AppProvider; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; import java.io.File; import java.util.HashMap; +import java.util.Set; /** * Manages the whole process when a background update triggers an install or the user @@ -59,10 +59,19 @@ public class InstallManagerService extends Service { private static final int NOTIFY_DOWNLOADING = 0x2344; /** - * The collection of APKs that are actively going through this whole process. + * The collection of {@link Apk}s that are actively going through this whole process, + * matching the {@link App}s in {@code ACTIVE_APPS}. The key is the download URL, as + * in {@link Apk#getUrl()} or {@code urlString}. */ private static final HashMap ACTIVE_APKS = new HashMap(3); + /** + * The collection of {@link App}s that are actively going through this whole process, + * matching the {@link Apk}s in {@code ACTIVE_APKS}. The key is the + * {@code packageName} of the app. + */ + private static final HashMap ACTIVE_APPS = new HashMap(3); + /** * The array of active {@link BroadcastReceiver}s for each active APK. The key is the * download URL, as in {@link Apk#getUrl()} or {@code urlString}. @@ -152,6 +161,7 @@ public class InstallManagerService extends Service { public void onReceive(Context context, Intent intent) { String urlString = intent.getDataString(); Apk apk = ACTIVE_APKS.remove(urlString); + ACTIVE_APPS.remove(apk.packageName); notifyDownloadComplete(apk, urlString); unregisterDownloaderReceivers(urlString); } @@ -160,7 +170,8 @@ public class InstallManagerService extends Service { @Override public void onReceive(Context context, Intent intent) { String urlString = intent.getDataString(); - ACTIVE_APKS.remove(urlString); + Apk apk = ACTIVE_APKS.remove(urlString); + ACTIVE_APPS.remove(apk.packageName); unregisterDownloaderReceivers(urlString); } }; @@ -182,7 +193,7 @@ public class InstallManagerService extends Service { return new NotificationCompat.Builder(this) .setAutoCancel(true) .setContentIntent(getAppDetailsIntent(downloadUrlId, apk)) - .setContentTitle(getNotificationTitle(urlString, apk)) + .setContentTitle(getString(R.string.downloading_apk, getAppName(urlString, apk))) .addAction(R.drawable.ic_cancel_black_24dp, getString(R.string.cancel), DownloaderService.getCancelPendingIntent(this, urlString)) .setSmallIcon(android.R.drawable.stat_sys_download) @@ -190,23 +201,13 @@ public class InstallManagerService extends Service { .setProgress(100, 0, true); } - private String getAppName(Apk apk) { - App app = AppProvider.Helper.findByPackageName( - getContentResolver(), apk.packageName, new String[]{AppProvider.DataColumns.NAME}); - if (app != null && !TextUtils.isEmpty(app.name)) { - return app.name; - } else { - return null; - } - } - - private String getNotificationTitle(String urlString, Apk apk) { - String name = getAppName(apk); - if (TextUtils.isEmpty(name)) { + private String getAppName(String urlString, Apk apk) { + App app = ACTIVE_APPS.get(apk.packageName); + if (TextUtils.isEmpty(app.name)) { // this is ugly, but its better than nothing as a failsafe return getString(R.string.downloading_apk, urlString); } else { - return getString(R.string.downloading_apk, name); + return getString(R.string.downloading_apk, app.name); } } @@ -236,7 +237,7 @@ public class InstallManagerService extends Service { title = String.format(getString(R.string.tap_to_update_format), pm.getApplicationLabel(pm.getApplicationInfo(apk.packageName, 0))); } catch (PackageManager.NameNotFoundException e) { - title = String.format(getString(R.string.tap_to_install_format), getAppName(apk)); + title = String.format(getString(R.string.tap_to_install_format), getAppName(urlString, apk)); } int downloadUrlId = urlString.hashCode(); @@ -261,9 +262,29 @@ public class InstallManagerService extends Service { String urlString = apk.getUrl(); Utils.debugLog(TAG, "queue " + app.packageName + " " + apk.versionCode + " from " + urlString); ACTIVE_APKS.put(urlString, apk); + ACTIVE_APPS.put(app.packageName, app); Intent intent = new Intent(context, InstallManagerService.class); intent.setAction(ACTION_INSTALL); intent.setData(Uri.parse(urlString)); context.startService(intent); } + + /** + * Returns a {@link Set} of the {@code urlString}s that are currently active. + * {@code urlString}s are used as unique IDs throughout the + * {@code InstallManagerService} process, either as a {@code String} or as an + * {@code int} from {@link String#hashCode()}. + */ + public static Set getActiveDownloadUrls() { + return ACTIVE_APKS.keySet(); + } + + /** + * Returns a {@link Set} of the {@code packageName}s that are currently active. + * {@code packageName}s are used as unique IDs for apps throughout all of + * Android, F-Droid, and other apps stores. + */ + public static Set getActivePackageNames() { + return ACTIVE_APPS.keySet(); + } } From 7f10be18c6dd0b69e2fdbae98d09b197e60af443 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 11 May 2016 09:41:10 +0200 Subject: [PATCH 10/17] fix index update progress using simplified ProgressListener The Event class is no longer needed once there is specific ProgressListener instances for each type of progress update. The sourceUrl serves as the unique ID, like with DownloaderService and InstallManagerService. fixes #633 https://gitlab.com/fdroid/fdroidclient/issues/633 --- .../fdroid/fdroid/MultiRepoUpdaterTest.java | 4 +- .../org/fdroid/fdroid/RepoUpdaterTest.java | 1 + .../fdroid/ProgressBufferedInputStream.java | 18 ++--- .../org/fdroid/fdroid/ProgressListener.java | 77 ++----------------- .../java/org/fdroid/fdroid/RepoUpdater.java | 28 ++++--- .../java/org/fdroid/fdroid/UpdateService.java | 46 +++++------ .../main/java/org/fdroid/fdroid/Utils.java | 16 +--- 7 files changed, 59 insertions(+), 131 deletions(-) diff --git a/app/src/androidTest/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java b/app/src/androidTest/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java index ad72da4df..9fc9c9443 100644 --- a/app/src/androidTest/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java +++ b/app/src/androidTest/java/org/fdroid/fdroid/MultiRepoUpdaterTest.java @@ -30,7 +30,7 @@ import java.util.UUID; @SuppressWarnings("PMD") // TODO port this to JUnit 4 semantics public class MultiRepoUpdaterTest extends InstrumentationTestCase { - private static final String TAG = "RepoUpdaterTest"; + private static final String TAG = "MultiRepoUpdaterTest"; private static final String REPO_MAIN = "Test F-Droid repo"; private static final String REPO_ARCHIVE = "Test F-Droid repo (Archive)"; @@ -406,7 +406,7 @@ public class MultiRepoUpdaterTest extends InstrumentationTestCase { private RepoUpdater createUpdater(String name, Context context) { Repo repo = new Repo(); repo.signingCertificate = PUB_KEY; - repo.address = UUID.randomUUID().toString(); + repo.address = "https://fake.url/" + UUID.randomUUID().toString() + "/fdroid/repo"; repo.name = name; ContentValues values = new ContentValues(2); diff --git a/app/src/androidTest/java/org/fdroid/fdroid/RepoUpdaterTest.java b/app/src/androidTest/java/org/fdroid/fdroid/RepoUpdaterTest.java index c706265e9..9566bd54d 100644 --- a/app/src/androidTest/java/org/fdroid/fdroid/RepoUpdaterTest.java +++ b/app/src/androidTest/java/org/fdroid/fdroid/RepoUpdaterTest.java @@ -31,6 +31,7 @@ public class RepoUpdaterTest { context = instrumentation.getContext(); testFilesDir = TestUtils.getWriteableDir(instrumentation); Repo repo = new Repo(); + repo.address = "https://fake.url/fdroid/repo"; repo.signingCertificate = this.simpleIndexSigningCert; repoUpdater = new RepoUpdater(context, repo); } diff --git a/app/src/main/java/org/fdroid/fdroid/ProgressBufferedInputStream.java b/app/src/main/java/org/fdroid/fdroid/ProgressBufferedInputStream.java index ee5d5f0aa..31d13395a 100644 --- a/app/src/main/java/org/fdroid/fdroid/ProgressBufferedInputStream.java +++ b/app/src/main/java/org/fdroid/fdroid/ProgressBufferedInputStream.java @@ -1,17 +1,14 @@ package org.fdroid.fdroid; -import android.os.Bundle; - -import org.fdroid.fdroid.data.Repo; - import java.io.BufferedInputStream; import java.io.IOException; import java.io.InputStream; +import java.net.URL; public class ProgressBufferedInputStream extends BufferedInputStream { private final ProgressListener progressListener; - private final Bundle data; + private final URL sourceUrl; private final int totalBytes; private int currentBytes; @@ -20,12 +17,10 @@ public class ProgressBufferedInputStream extends BufferedInputStream { * Reports progress to the specified {@link ProgressListener}, with the * progress based on the {@code totalBytes}. */ - public ProgressBufferedInputStream(InputStream in, ProgressListener progressListener, Repo repo, int totalBytes) - throws IOException { + public ProgressBufferedInputStream(InputStream in, ProgressListener progressListener, URL sourceUrl, int totalBytes) { super(in); this.progressListener = progressListener; - this.data = new Bundle(1); - this.data.putString(RepoUpdater.PROGRESS_DATA_REPO_ADDRESS, repo.address); + this.sourceUrl = sourceUrl; this.totalBytes = totalBytes; } @@ -37,10 +32,7 @@ public class ProgressBufferedInputStream extends BufferedInputStream { * the digits changing because it looks pretty, < 9000 since the reads won't * line up exactly */ if (currentBytes % 333333 < 9000) { - progressListener.onProgress( - new ProgressListener.Event( - RepoUpdater.PROGRESS_TYPE_PROCESS_XML, - currentBytes, totalBytes, data)); + progressListener.onProgress(sourceUrl, currentBytes, totalBytes); } } return super.read(buffer, byteOffset, byteCount); diff --git a/app/src/main/java/org/fdroid/fdroid/ProgressListener.java b/app/src/main/java/org/fdroid/fdroid/ProgressListener.java index b2f4f252b..915b64ee6 100644 --- a/app/src/main/java/org/fdroid/fdroid/ProgressListener.java +++ b/app/src/main/java/org/fdroid/fdroid/ProgressListener.java @@ -1,76 +1,15 @@ package org.fdroid.fdroid; -import android.os.Bundle; -import android.os.Parcel; -import android.os.Parcelable; +import java.net.URL; +/** + * This is meant only to send download progress for any URL (e.g. index + * updates, APKs, etc). This also keeps this class pure Java so that classes + * that use {@code ProgressListener} can be tested on the JVM, without requiring + * an Android device or emulator. + */ public interface ProgressListener { - void onProgress(Event event); - - // I went a bit overboard with the overloaded constructors, but they all - // seemed potentially useful and unambiguous, so I just put them in there - // while I'm here. - class Event implements Parcelable { - - public static final int NO_VALUE = Integer.MIN_VALUE; - - public final String type; - public final Bundle data; - - // These two are not final, so that you can create a template Event, - // pass it into a function which performs something over time, and - // that function can initialize "total" and progressively - // update "progress" - public int progress; - public final int total; - - public Event(String type) { - this(type, NO_VALUE, NO_VALUE, null); - } - - public Event(String type, int progress, int total, Bundle data) { - this.type = type; - this.progress = progress; - this.total = total; - this.data = (data == null) ? new Bundle() : data; - } - - @Override - public int describeContents() { - return 0; - } - - @Override - public void writeToParcel(Parcel dest, int flags) { - dest.writeString(type); - dest.writeInt(progress); - dest.writeInt(total); - dest.writeBundle(data); - } - - public static final Parcelable.Creator CREATOR = new Parcelable.Creator() { - @Override - public Event createFromParcel(Parcel in) { - return new Event(in.readString(), in.readInt(), in.readInt(), in.readBundle()); - } - - @Override - public Event[] newArray(int size) { - return new Event[size]; - } - }; - - /** - * 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; - } - } + void onProgress(URL sourceUrl, int bytesRead, int totalBytes); } diff --git a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java index 4dc986e05..e720a6031 100644 --- a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java +++ b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java @@ -21,6 +21,8 @@ import org.xml.sax.XMLReader; import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.net.MalformedURLException; +import java.net.URL; import java.security.CodeSigner; import java.security.cert.Certificate; import java.security.cert.X509Certificate; @@ -48,10 +50,6 @@ public class RepoUpdater { private static final String TAG = "RepoUpdater"; - public static final String PROGRESS_TYPE_PROCESS_XML = "processingXml"; - public static final String PROGRESS_COMMITTING = "committing"; - public static final String PROGRESS_DATA_REPO_ADDRESS = "repoAddress"; - public final String indexUrl; @NonNull @@ -60,7 +58,8 @@ public class RepoUpdater { private final Repo repo; private boolean hasChanged; @Nullable - private ProgressListener progressListener; + private ProgressListener committingProgressListener; + private ProgressListener processXmlProgressListener; private String cacheTag; private X509Certificate signingCertFromJar; @@ -84,8 +83,12 @@ public class RepoUpdater { this.indexUrl = url; } - public void setProgressListener(@Nullable ProgressListener progressListener) { - this.progressListener = progressListener; + public void setProcessXmlProgressListener(ProgressListener progressListener) { + this.processXmlProgressListener = progressListener; + } + + public void setCommittingProgressListener(ProgressListener progressListener) { + this.committingProgressListener = progressListener; } public boolean hasChanged() { @@ -177,7 +180,7 @@ public class RepoUpdater { JarFile jarFile = new JarFile(downloadedFile, true); JarEntry indexEntry = (JarEntry) jarFile.getEntry("index.xml"); indexInputStream = new ProgressBufferedInputStream(jarFile.getInputStream(indexEntry), - progressListener, repo, (int) indexEntry.getSize()); + processXmlProgressListener, new URL(repo.address), (int) indexEntry.getSize()); // Process the index... final SAXParser parser = SAXParserFactory.newInstance().newSAXParser(); @@ -207,8 +210,13 @@ public class RepoUpdater { private void commitToDb() throws UpdateException { Log.i(TAG, "Repo signature verified, saving app metadata to database."); - if (progressListener != null) { - progressListener.onProgress(new ProgressListener.Event(PROGRESS_COMMITTING)); + if (committingProgressListener != null) { + try { + //TODO this should be an event, not a progress listener + committingProgressListener.onProgress(new URL(indexUrl), 0, -1); + } catch (MalformedURLException e) { + e.printStackTrace(); + } } persister.commit(repoDetailsToSave); } diff --git a/app/src/main/java/org/fdroid/fdroid/UpdateService.java b/app/src/main/java/org/fdroid/fdroid/UpdateService.java index 0cb83a178..8dc291305 100644 --- a/app/src/main/java/org/fdroid/fdroid/UpdateService.java +++ b/app/src/main/java/org/fdroid/fdroid/UpdateService.java @@ -52,10 +52,11 @@ import org.fdroid.fdroid.installer.InstallManagerService; import org.fdroid.fdroid.net.Downloader; import org.fdroid.fdroid.net.DownloaderService; +import java.net.URL; import java.util.ArrayList; import java.util.List; -public class UpdateService extends IntentService implements ProgressListener { +public class UpdateService extends IntentService { private static final String TAG = "UpdateService"; @@ -376,7 +377,8 @@ public class UpdateService extends IntentService implements ProgressListener { RepoUpdater updater = new RepoUpdater(getBaseContext(), repo); localBroadcastManager.registerReceiver(downloadProgressReceiver, DownloaderService.getIntentFilter(updater.indexUrl, Downloader.ACTION_PROGRESS)); - updater.setProgressListener(this); + updater.setProcessXmlProgressListener(processXmlProgressListener); + updater.setCommittingProgressListener(committingProgressListener); try { updater.update(); if (updater.hasChanged()) { @@ -526,25 +528,25 @@ public class UpdateService extends IntentService implements ProgressListener { notificationManager.notify(NOTIFY_ID_UPDATES_AVAILABLE, builder.build()); } - /** - * Received progress event from the RepoXMLHandler. It could be progress - * downloading from the repo, or perhaps processing the info from the repo. - */ - @Override - public void onProgress(ProgressListener.Event event) { - String message = ""; - String repoAddress = event.getData().getString(RepoUpdater.PROGRESS_DATA_REPO_ADDRESS); - String downloadedSize = Utils.getFriendlySize(event.progress); - String totalSize = Utils.getFriendlySize(event.total); - int percent = event.total > 0 ? (int) ((double) event.progress / event.total * 100) : -1; - switch (event.type) { - case RepoUpdater.PROGRESS_TYPE_PROCESS_XML: - message = getString(R.string.status_processing_xml_percent, repoAddress, downloadedSize, totalSize, percent); - break; - case RepoUpdater.PROGRESS_COMMITTING: - message = getString(R.string.status_inserting_apps); - break; + private final ProgressListener processXmlProgressListener = new ProgressListener() { + @Override + public void onProgress(URL sourceUrl, int bytesRead, int totalBytes) { + String downloadedSize = Utils.getFriendlySize(bytesRead); + String totalSize = Utils.getFriendlySize(totalBytes); + int percent = -1; + if (totalBytes > 0) { + percent = (int) ((double) bytesRead / totalBytes * 100); + } + String message = getString(R.string.status_processing_xml_percent, sourceUrl, downloadedSize, totalSize, percent); + sendStatus(getApplicationContext(), STATUS_INFO, message, percent); } - sendStatus(this, STATUS_INFO, message, percent); - } + }; + + private final ProgressListener committingProgressListener = new ProgressListener() { + @Override + public void onProgress(URL sourceUrl, int bytesRead, int totalBytes) { + String message = getString(R.string.status_inserting_apps); + sendStatus(getApplicationContext(), STATUS_INFO, message); + } + }; } diff --git a/app/src/main/java/org/fdroid/fdroid/Utils.java b/app/src/main/java/org/fdroid/fdroid/Utils.java index fb1178374..733280f52 100644 --- a/app/src/main/java/org/fdroid/fdroid/Utils.java +++ b/app/src/main/java/org/fdroid/fdroid/Utils.java @@ -115,27 +115,13 @@ public final class Utils { return "/icons-120/"; } - public static void copy(InputStream input, OutputStream output) - throws IOException { - copy(input, output, null, null); - } - - public static void copy(InputStream input, OutputStream output, - ProgressListener progressListener, - ProgressListener.Event templateProgressEvent) - throws IOException { + public static void copy(InputStream input, OutputStream output) throws IOException { byte[] buffer = new byte[BUFFER_SIZE]; - int bytesRead = 0; while (true) { int count = input.read(buffer); if (count == -1) { break; } - if (progressListener != null) { - bytesRead += count; - templateProgressEvent.progress = bytesRead; - progressListener.onProgress(templateProgressEvent); - } output.write(buffer, 0, count); } output.flush(); From 4a9ed54f42b170816fa5100a03f109bcb3fb4c13 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 11 May 2016 09:42:53 +0200 Subject: [PATCH 11/17] use simplified ProgressListener in Downloader and DownloaderService Now the simplified ProgressListener works as the generic listener again, enforcing the concept of URL as unique ID throughout the code base. --- .../java/org/fdroid/fdroid/net/Downloader.java | 16 ++++------------ .../org/fdroid/fdroid/net/DownloaderService.java | 5 +++-- .../fdroid/fdroid/net/HttpDownloaderTest.java | 9 +++++---- 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java index 7981c2571..cb68aa575 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java @@ -1,5 +1,6 @@ package org.fdroid.fdroid.net; +import org.fdroid.fdroid.ProgressListener; import org.fdroid.fdroid.Utils; import java.io.File; @@ -36,19 +37,10 @@ public abstract class Downloader { protected final URL sourceUrl; protected String cacheTag; - /** - * This is meant only to send progress to {@link DownloaderService}. This - * also keeps this class pure Java so that it can be tested on the JVM, - * without requiring an Android device or emulator. - */ - interface DownloaderProgressListener { - void sendProgress(URL sourceUrl, int bytesRead, int totalBytes); - } - /** * For sending download progress, should only be called in {@link #progressTask} */ - private volatile DownloaderProgressListener downloaderProgressListener; + private volatile ProgressListener downloaderProgressListener; protected abstract InputStream getDownloadersInputStream() throws IOException; @@ -64,7 +56,7 @@ public abstract class Downloader { return new WrappedInputStream(getDownloadersInputStream()); } - public void setListener(DownloaderProgressListener listener) { + public void setListener(ProgressListener listener) { this.downloaderProgressListener = listener; } @@ -194,7 +186,7 @@ public abstract class Downloader { @Override public void run() { if (downloaderProgressListener != null) { - downloaderProgressListener.sendProgress(sourceUrl, bytesRead, totalBytes); + downloaderProgressListener.onProgress(sourceUrl, bytesRead, totalBytes); } } }; diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index 21061f260..8b12e74a5 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -34,6 +34,7 @@ import android.support.v4.content.LocalBroadcastManager; import android.text.TextUtils; import android.util.Log; +import org.fdroid.fdroid.ProgressListener; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.SanitizedFile; @@ -192,9 +193,9 @@ public class DownloaderService extends Service { try { downloader = DownloaderFactory.create(this, uri, localFile); - downloader.setListener(new Downloader.DownloaderProgressListener() { + downloader.setListener(new ProgressListener() { @Override - public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { + public void onProgress(URL sourceUrl, int bytesRead, int totalBytes) { Intent intent = new Intent(Downloader.ACTION_PROGRESS); intent.setData(uri); intent.putExtra(Downloader.EXTRA_BYTES_READ, bytesRead); diff --git a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java index 7b56bee6b..3beedff97 100644 --- a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java @@ -1,6 +1,7 @@ package org.fdroid.fdroid.net; +import org.fdroid.fdroid.ProgressListener; import org.junit.Test; import java.io.File; @@ -48,9 +49,9 @@ public class HttpDownloaderTest { URL url = new URL(urlString); File destFile = File.createTempFile("dl-", ""); final HttpDownloader httpDownloader = new HttpDownloader(url, destFile); - httpDownloader.setListener(new Downloader.DownloaderProgressListener() { + httpDownloader.setListener(new ProgressListener() { @Override - public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { + public void onProgress(URL sourceUrl, int bytesRead, int totalBytes) { System.out.println("DownloaderProgressListener.sendProgress " + sourceUrl + " " + bytesRead + " / " + totalBytes); receivedProgress = true; } @@ -111,9 +112,9 @@ public class HttpDownloaderTest { URL url = new URL("https://f-droid.org/repo/index.jar"); File destFile = File.createTempFile("dl-", ""); final HttpDownloader httpDownloader = new HttpDownloader(url, destFile); - httpDownloader.setListener(new Downloader.DownloaderProgressListener() { + httpDownloader.setListener(new ProgressListener() { @Override - public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { + public void onProgress(URL sourceUrl, int bytesRead, int totalBytes) { System.out.println("DownloaderProgressListener.sendProgress " + bytesRead + " / " + totalBytes); receivedProgress = true; latch.countDown(); From 2080d77e6bf5df2f695c1464046231584d55a48b Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Sat, 7 May 2016 00:32:39 +0200 Subject: [PATCH 12/17] temporary notification user experience to get something workable this represents the current state of things until we can overhaul the whole notifications and update UX as outlined in #592 https://gitlab.com/fdroid/fdroidclient/issues/592 --- .../installer/InstallManagerService.java | 32 +++++++++++++++---- .../fdroid/fdroid/net/DownloaderService.java | 2 -- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index c6dd31608..aca8a12b5 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -56,7 +56,6 @@ public class InstallManagerService extends Service { public static final String TAG = "InstallManagerService"; private static final String ACTION_INSTALL = "org.fdroid.fdroid.InstallManagerService.action.INSTALL"; - private static final int NOTIFY_DOWNLOADING = 0x2344; /** * The collection of {@link Apk}s that are actively going through this whole process, @@ -78,7 +77,20 @@ public class InstallManagerService extends Service { */ private final HashMap receivers = new HashMap(3); + /** + * Get the app name based on a {@code urlString} key. The app name needs + * to be kept around for the final notification update, but {@link App} + * and {@link Apk} instances have already removed by the time that final + * notification update comes around. Once there is a proper + * {@code InstallerService} and its integrated here, this must go away, + * since the {@link App} and {@link Apk} instances will be available. + *

+ * TODO delete me once InstallerService exists + */ + private static final HashMap TEMP_HACK_APP_NAMES = new HashMap(3); + private LocalBroadcastManager localBroadcastManager; + private NotificationManager notificationManager; /** * This service does not use binding, so no need to implement this method @@ -93,6 +105,7 @@ public class InstallManagerService extends Service { super.onCreate(); Utils.debugLog(TAG, "creating Service"); localBroadcastManager = LocalBroadcastManager.getInstance(this); + notificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); } @Override @@ -102,7 +115,7 @@ public class InstallManagerService extends Service { Apk apk = ACTIVE_APKS.get(urlString); Notification notification = createNotification(intent.getDataString(), apk).build(); - startForeground(NOTIFY_DOWNLOADING, notification); + notificationManager.notify(urlString.hashCode(), notification); registerDownloaderReceivers(urlString); @@ -149,17 +162,17 @@ public class InstallManagerService extends Service { Apk apk = ACTIVE_APKS.get(urlString); int bytesRead = intent.getIntExtra(Downloader.EXTRA_BYTES_READ, 0); int totalBytes = intent.getIntExtra(Downloader.EXTRA_TOTAL_BYTES, 0); - NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); Notification notification = createNotification(urlString, apk) .setProgress(totalBytes, bytesRead, false) .build(); - nm.notify(NOTIFY_DOWNLOADING, notification); + notificationManager.notify(urlString.hashCode(), notification); } }; BroadcastReceiver completeReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { String urlString = intent.getDataString(); + // TODO these need to be removed based on whether they are fed to InstallerService or not Apk apk = ACTIVE_APKS.remove(urlString); ACTIVE_APPS.remove(apk.packageName); notifyDownloadComplete(apk, urlString); @@ -203,9 +216,13 @@ public class InstallManagerService extends Service { private String getAppName(String urlString, Apk apk) { App app = ACTIVE_APPS.get(apk.packageName); - if (TextUtils.isEmpty(app.name)) { - // this is ugly, but its better than nothing as a failsafe - return getString(R.string.downloading_apk, urlString); + if (app == null || TextUtils.isEmpty(app.name)) { + if (TEMP_HACK_APP_NAMES.containsKey(urlString)) { + return getString(R.string.downloading_apk, TEMP_HACK_APP_NAMES.get(urlString)); + } else { + // this is ugly, but its better than nothing as a failsafe + return getString(R.string.downloading_apk, urlString); + } } else { return getString(R.string.downloading_apk, app.name); } @@ -263,6 +280,7 @@ public class InstallManagerService extends Service { Utils.debugLog(TAG, "queue " + app.packageName + " " + apk.versionCode + " from " + urlString); ACTIVE_APKS.put(urlString, apk); ACTIVE_APPS.put(app.packageName, app); + TEMP_HACK_APP_NAMES.put(urlString, app.name); // TODO delete me once InstallerService exists Intent intent = new Intent(context, InstallManagerService.class); intent.setAction(ACTION_INSTALL); intent.setData(Uri.parse(urlString)); diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index 8b12e74a5..54d61bd41 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -158,7 +158,6 @@ public class DownloaderService extends Service { @Override public void onDestroy() { Utils.debugLog(TAG, "Destroying downloader service. Will move to background and stop our Looper."); - stopForeground(true); serviceLooper.quit(); //NOPMD - this is copied from IntentService, no super call needed } @@ -215,7 +214,6 @@ public class DownloaderService extends Service { if (downloader != null) { downloader.close(); } - stopForeground(true); } downloader = null; } From e75143530f84a01928b1fd4be8600d9d3a110aac Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 11 May 2016 14:32:52 +0200 Subject: [PATCH 13/17] track AppDetails visibility to improve Notification UX If AppDetails is visible, then it'll automatically launch the install process, and there is no need to put up a "Tap to install" notification. And of course, whenever I search stackoverflow, I find an answer from @commonsguy :) https://stackoverflow.com/questions/18038399/how-to-check-if-activity-is-in-foreground-or-in-visible-background/18469643#18469643 --- .../java/org/fdroid/fdroid/AppDetails.java | 11 +++++++++++ .../installer/InstallManagerService.java | 18 +++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index abcd19523..5bdb405ef 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -110,6 +110,15 @@ public class AppDetails extends AppCompatActivity { private FDroidApp fdroidApp; private ApkListAdapter adapter; + /** + * Check if {@code packageName} is currently visible to the user. + */ + public static boolean isAppVisible(String packageName) { + return packageName != null && packageName.equals(visiblePackageName); + } + + private static String visiblePackageName; + private static class ViewHolder { TextView version; TextView status; @@ -437,6 +446,7 @@ public class AppDetails extends AppCompatActivity { if (DownloaderService.isQueuedOrActive(activeDownloadUrlString)) { registerDownloaderReceivers(); } + visiblePackageName = app.packageName; } /** @@ -458,6 +468,7 @@ public class AppDetails extends AppCompatActivity { @Override protected void onPause() { super.onPause(); + visiblePackageName = null; // save the active URL for this app in case we come back PreferencesCompat.apply(getPreferences(MODE_PRIVATE) .edit() diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index aca8a12b5..c0097250d 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -175,7 +175,11 @@ public class InstallManagerService extends Service { // TODO these need to be removed based on whether they are fed to InstallerService or not Apk apk = ACTIVE_APKS.remove(urlString); ACTIVE_APPS.remove(apk.packageName); - notifyDownloadComplete(apk, urlString); + if (AppDetails.isAppVisible(apk.packageName)) { + cancelNotification(urlString); + } else { + notifyDownloadComplete(apk, urlString); + } unregisterDownloaderReceivers(urlString); } }; @@ -186,6 +190,9 @@ public class InstallManagerService extends Service { Apk apk = ACTIVE_APKS.remove(urlString); ACTIVE_APPS.remove(apk.packageName); unregisterDownloaderReceivers(urlString); + if (AppDetails.isAppVisible(apk.packageName)) { + cancelNotification(urlString); + } } }; localBroadcastManager.registerReceiver(startedReceiver, @@ -269,6 +276,15 @@ public class InstallManagerService extends Service { nm.notify(downloadUrlId, builder.build()); } + /** + * Cancel the {@link Notification} tied to {@code urlString}, which is the + * unique ID used to represent a given APK file. {@link String#hashCode()} + * converts {@code urlString} to the required {@code int}. + */ + private void cancelNotification(String urlString) { + notificationManager.cancel(urlString.hashCode()); + } + /** * Install an APK, checking the cache and downloading if necessary before starting the process. * All notifications are sent as an {@link Intent} via local broadcasts to be received by From 78c0416c84b23ac3dbb216e22012a76bdf8301cd Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 11 May 2016 15:03:25 +0200 Subject: [PATCH 14/17] clear notification for app once it has been successfully installed This logic is pretty basic for now, it'll have to be expanded a lot to support the different UX between priv and non-priv installs. For example, priv updates will be able to happen entirely in the background. Those will then require leaving a notification to tell the user that the app was updated so nothing can transparently install updates without the user knowing. When the user is an active part of each install, like the non-priv experience requires, then keeping the "app installed" notification feels like just extra noise. --- .../installer/InstallManagerService.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index c0097250d..12a20b692 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -7,6 +7,7 @@ import android.app.Service; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; +import android.content.IntentFilter; import android.content.pm.PackageManager; import android.net.Uri; import android.os.IBinder; @@ -25,6 +26,7 @@ import org.fdroid.fdroid.net.DownloaderService; import java.io.File; import java.util.HashMap; +import java.util.Map; import java.util.Set; /** @@ -106,6 +108,24 @@ public class InstallManagerService extends Service { Utils.debugLog(TAG, "creating Service"); localBroadcastManager = LocalBroadcastManager.getInstance(this); notificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + + BroadcastReceiver br = new BroadcastReceiver() { + @Override + public void onReceive(Context context, Intent intent) { + String packageName = intent.getData().getSchemeSpecificPart(); + for (Map.Entry entry : ACTIVE_APKS.entrySet()) { + if (TextUtils.equals(packageName, entry.getValue().packageName)) { + String urlString = entry.getKey(); + cancelNotification(urlString); + break; + } + } + } + }; + IntentFilter intentFilter = new IntentFilter(); + intentFilter.addAction(Intent.ACTION_PACKAGE_ADDED); + intentFilter.addDataScheme("package"); + registerReceiver(br, intentFilter); } @Override From 62295b72b414194128b13c78f81e871afb91285a Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 11 May 2016 19:53:01 +0200 Subject: [PATCH 15/17] send ACTION_INTERRUPTED when APK is canceled from queue If an APK was queued to download but had not started downloading yet, it was not able to be fully canceled because ACTION_INTERRUPTED was not sent. That meant that the UI never got updated, even though the APK was removed from the queue. #652 https://gitlab.com/fdroid/fdroidclient/issues/652 --- .../java/org/fdroid/fdroid/net/DownloaderService.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index 54d61bd41..b43a5ff53 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -121,6 +121,7 @@ public class DownloaderService extends Service { Integer whatToRemove = uriString.hashCode(); if (serviceHandler.hasMessages(whatToRemove)) { serviceHandler.removeMessages(whatToRemove); + sendBroadcast(intent.getData(), Downloader.ACTION_INTERRUPTED); } else if (isActive(uriString)) { downloader.cancelDownload(); } else { @@ -218,6 +219,10 @@ public class DownloaderService extends Service { downloader = null; } + private void sendBroadcast(Uri uri, String action) { + sendBroadcast(uri, action, null, null); + } + private void sendBroadcast(Uri uri, String action, File file) { sendBroadcast(uri, action, file, null); } @@ -225,7 +230,9 @@ public class DownloaderService extends Service { private void sendBroadcast(Uri uri, String action, File file, String errorMessage) { Intent intent = new Intent(action); intent.setData(uri); - intent.putExtra(Downloader.EXTRA_DOWNLOAD_PATH, file.getAbsolutePath()); + if (file != null) { + intent.putExtra(Downloader.EXTRA_DOWNLOAD_PATH, file.getAbsolutePath()); + } if (!TextUtils.isEmpty(errorMessage)) { intent.putExtra(Downloader.EXTRA_ERROR_MESSAGE, errorMessage); } From 81f13279fe3818966e018fada0343c13d8b2144f Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 11 May 2016 20:25:12 +0200 Subject: [PATCH 16/17] some tricks to get Cancel working on the download Notification I wrestled with this a bunch, it seems quite difficult to make the Cancel button on the notification responsive. This collection of minor changes made it more reliable, but its still kind of flaky. I think the problem might be related to the fact that it is creating a whole new Notification instance, with the accompanying Intent and PendingIntent instances, for every single download progress update. closes #652 https://gitlab.com/fdroid/fdroidclient/issues/652 --- .../main/java/org/fdroid/fdroid/net/DownloaderService.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index b43a5ff53..7e5961d95 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -142,17 +142,18 @@ public class DownloaderService extends Service { public static PendingIntent getCancelPendingIntent(Context context, String urlString) { Intent cancelIntent = new Intent(context.getApplicationContext(), DownloaderService.class) .setData(Uri.parse(urlString)) - .setAction(ACTION_CANCEL); + .setAction(ACTION_CANCEL) + .setFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_CLEAR_TASK); return PendingIntent.getService(context.getApplicationContext(), urlString.hashCode(), cancelIntent, - PendingIntent.FLAG_CANCEL_CURRENT); + PendingIntent.FLAG_UPDATE_CURRENT); } @Override public int onStartCommand(Intent intent, int flags, int startId) { - onStart(intent, startId); Utils.debugLog(TAG, "onStartCommand " + intent); + onStart(intent, startId); return START_REDELIVER_INTENT; // if killed before completion, retry Intent } From 43be8f3fd10346e88fc552a638562cfa6ef22321 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 11 May 2016 19:25:30 +0200 Subject: [PATCH 17/17] delete temp files created by DownloaderFactory#create(Context, String) --- .../main/java/org/fdroid/fdroid/CleanCacheService.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/CleanCacheService.java b/app/src/main/java/org/fdroid/fdroid/CleanCacheService.java index 463af770d..2feb9d73d 100644 --- a/app/src/main/java/org/fdroid/fdroid/CleanCacheService.java +++ b/app/src/main/java/org/fdroid/fdroid/CleanCacheService.java @@ -59,10 +59,13 @@ public class CleanCacheService extends IntentService { * Delete index files which were downloaded, but not removed (e.g. due to F-Droid being * force closed during processing of the file, before getting a chance to delete). This * may include both "index-*-downloaded" and "index-*-extracted.xml" files. - *

+ *

* Note that if the SD card is not ready, then the cache directory will probably not be * available. In this situation no files will be deleted (and thus they may still exist * after the SD card becomes available). + *

+ * This also deletes temp files that are created by + * {@link org.fdroid.fdroid.net.DownloaderFactory#create(Context, String)}, e.g. "dl-*" */ private void deleteStrayIndexFiles() { File cacheDir = getCacheDir(); @@ -79,6 +82,9 @@ public class CleanCacheService extends IntentService { if (f.getName().startsWith("index-")) { FileUtils.deleteQuietly(f); } + if (f.getName().startsWith("dl-")) { + FileUtils.deleteQuietly(f); + } } } }