From 3f9881a6f5eb66eec826ee34fab109d472a2ed91 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Apr 2016 13:26:03 +1000 Subject: [PATCH 01/11] Fix `if` condition which was always `true`. I suspect this is the expected behaviour, seeing as this service handles downloads for both Apks and repo indices. --- app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java | 2 +- 1 file changed, 1 insertion(+), 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 3d4bd7679..fb625b3c5 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -270,7 +270,7 @@ public class DownloaderService extends Service { Intent intent = new Intent(context, DownloaderService.class); intent.setAction(ACTION_QUEUE); intent.setData(Uri.parse(urlString)); - if (!TextUtils.isEmpty(EXTRA_PACKAGE_NAME)) { + if (!TextUtils.isEmpty(packageName)) { intent.putExtra(EXTRA_PACKAGE_NAME, packageName); } context.startService(intent); From 0163d6efa6013181c2e6554760e5fa6e67a6daf9 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Apr 2016 13:27:59 +1000 Subject: [PATCH 02/11] Add `PendingIntent` for notification in `DownloaderService`. Fixes #625, as devices pre 4.0 require such `PendingIntent`s to be passed to the notificaitons `setContentIntent` method. As the `DownloaderService` handles both apks and repo updates, the pending intent will either point to the relevant `AppDetails` screen for an apk download - or the main F-Droid activity if it is any other sort of download (i.e. a repo update download). --- .../fdroid/fdroid/net/DownloaderService.java | 44 ++++++++++++++++--- 1 file changed, 39 insertions(+), 5 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 fb625b3c5..df1875e86 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -19,6 +19,7 @@ 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; @@ -31,11 +32,16 @@ import android.os.Looper; 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.Preferences; import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; @@ -137,7 +143,8 @@ public class DownloaderService extends Service { } } else if (ACTION_QUEUE.equals(intent.getAction())) { if (Preferences.get().isUpdateNotificationEnabled()) { - startForeground(NOTIFY_DOWNLOADING, createNotification(intent.getDataString()).build()); + Notification notification = createNotification(intent.getDataString(), getPackageNameFromIntent(intent)).build(); + startForeground(NOTIFY_DOWNLOADING, notification); } Log.i(TAG, "Queued " + intent); Message msg = serviceHandler.obtainMessage(); @@ -152,15 +159,42 @@ public class DownloaderService extends Service { } } - private NotificationCompat.Builder createNotification(String urlString) { + @Nullable + private static String getPackageNameFromIntent(@NonNull Intent intent) { + return intent.hasExtra(EXTRA_PACKAGE_NAME) ? intent.getStringExtra(EXTRA_PACKAGE_NAME) : null; + } + + private NotificationCompat.Builder createNotification(String urlString, @Nullable String packageName) { return new NotificationCompat.Builder(this) .setAutoCancel(true) + .setContentIntent(createAppDetailsIntent(this, 0, packageName)) .setContentTitle(getString(R.string.downloading)) .setSmallIcon(android.R.drawable.stat_sys_download) .setContentText(urlString) .setProgress(100, 0, true); } + public static PendingIntent createAppDetailsIntent(@NonNull Context context, int requestCode, @Nullable String packageName) { + TaskStackBuilder stackBuilder; + if (packageName != null) { + Intent notifyIntent = new Intent(context.getApplicationContext(), AppDetails.class) + .putExtra(AppDetails.EXTRA_APPID, packageName); + + stackBuilder = TaskStackBuilder + .create(context.getApplicationContext()) + .addParentStack(AppDetails.class) + .addNextIntent(notifyIntent); + } else { + Intent notifyIntent = new Intent(context.getApplicationContext(), FDroid.class); + stackBuilder = TaskStackBuilder + .create(context.getApplicationContext()) + .addParentStack(FDroid.class) + .addNextIntent(notifyIntent); + } + + return stackBuilder.getPendingIntent(requestCode, PendingIntent.FLAG_UPDATE_CURRENT); + } + @Override public int onStartCommand(Intent intent, int flags, int startId) { onStart(intent, startId); @@ -204,6 +238,7 @@ public class DownloaderService extends Service { File downloadDir = new File(Utils.getApkCacheDir(this), uri.getHost() + "-" + uri.getPort()); downloadDir.mkdirs(); final SanitizedFile localFile = new SanitizedFile(downloadDir, uri.getLastPathSegment()); + final String packageName = getPackageNameFromIntent(intent); sendBroadcast(uri, Downloader.ACTION_STARTED, localFile); try { downloader = DownloaderFactory.create(this, uri, localFile); @@ -217,7 +252,7 @@ public class DownloaderService extends Service { localBroadcastManager.sendBroadcast(intent); NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - Notification notification = createNotification(uri.toString()) + Notification notification = createNotification(uri.toString(), packageName) .setProgress(totalBytes, bytesRead, false) .build(); nm.notify(NOTIFY_DOWNLOADING, notification); @@ -225,8 +260,7 @@ public class DownloaderService extends Service { }); downloader.download(); sendBroadcast(uri, Downloader.ACTION_COMPLETE, localFile); - DownloadCompleteService.notify(this, intent.getStringExtra(EXTRA_PACKAGE_NAME), - intent.getDataString()); + DownloadCompleteService.notify(this, packageName, intent.getDataString()); } catch (InterruptedException e) { sendBroadcast(uri, Downloader.ACTION_INTERRUPTED, localFile); } catch (IOException e) { From 8794611b2679c7612ccacf152e4681462c6f4147 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Apr 2016 13:30:33 +1000 Subject: [PATCH 03/11] DownloadCompleteService uses same notification code as DownloadService. Both classes required the same type of notification intent to be created, so they now both use the same static function to create the relevant intent. --- .../fdroid/fdroid/net/DownloadCompleteService.java | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java index 6574a76fd..e64ef3e20 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java @@ -64,22 +64,13 @@ public class DownloadCompleteService extends IntentService { title = String.format(getString(R.string.tap_to_install_format), app.name); } - Intent notifyIntent = new Intent(this, AppDetails.class); - notifyIntent.putExtra(AppDetails.EXTRA_APPID, packageName); - TaskStackBuilder stackBuilder = TaskStackBuilder - .create(this) - .addParentStack(AppDetails.class) - .addNextIntent(notifyIntent); int requestCode = Utils.getApkUrlNotificationId(intent.getDataString()); - PendingIntent pendingIntent = stackBuilder.getPendingIntent(requestCode, - PendingIntent.FLAG_UPDATE_CURRENT); - NotificationCompat.Builder builder = new NotificationCompat.Builder(this) .setAutoCancel(true) .setContentTitle(title) .setSmallIcon(android.R.drawable.stat_sys_download_done) - .setContentIntent(pendingIntent) + .setContentIntent(DownloaderService.createAppDetailsIntent(this, requestCode, packageName)) .setContentText(getString(R.string.tap_to_install)); NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); nm.notify(Utils.getApkUrlNotificationId(intent.getDataString()), builder.build()); From e56d604d6faa9fcf2f9e30ecb67cd32757aae595 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Apr 2016 13:51:16 +1000 Subject: [PATCH 04/11] Only cleanup receivers, don't reset the UI when in `onDestroy()`. When an Activity is destroyed, the next time it is created should reinitialize all of the UI stuff again. Thus, there is no point to clearing up the UI state before leaving. More importantly, this was causing a problem when navigating back and forth through activities via the downloader service notifications: ``` E java.lang.RuntimeException: Unable to destroy activity {org.fdroid.fdroid/org.fdroid.fdroid.AppDetails}: java.lang.IllegalStateException: activity is already destroyed ... E Caused by: java.lang.IllegalStateException: activity is already destroyed E at android.nfc.NfcActivityManager$NfcActivityState.(NfcActivityManager.java:126) E at android.nfc.NfcActivityManager.getActivityState(NfcActivityManager.java:176) E at android.nfc.NfcActivityManager.setNdefPushContentUri(NfcActivityManager.java:252) E at android.nfc.NfcAdapter.setBeamPushUris(NfcAdapter.java:830) E at org.fdroid.fdroid.NfcHelper.disableAndroidBeam(NfcHelper.java:68) E at org.fdroid.fdroid.AppDetails$AppDetailsHeaderFragment.updateViews(AppDetails.java:1507) E at org.fdroid.fdroid.AppDetails$AppDetailsHeaderFragment.updateViews(AppDetails.java:1490) E at org.fdroid.fdroid.AppDetails$AppDetailsHeaderFragment.removeProgress(AppDetails.java:1473) E at org.fdroid.fdroid.AppDetails.cleanUpFinishedDownload(AppDetails.java:442) E at org.fdroid.fdroid.AppDetails.onDestroy(AppDetails.java:567) E at android.app.Activity.performDestroy(Activity.java:6169) E at android.app.Instrumentation.callActivityOnDestroy(Instrumentation.java:1141) E at android.app.ActivityThread.performDestroyActivity(ActivityThread.java:3726) E ... 10 more ``` This is related to `onDestroy` calling a method which ends up calling UI related stuff that assumes the `Activity` is around to interact with. --- app/src/main/java/org/fdroid/fdroid/AppDetails.java | 2 +- 1 file changed, 1 insertion(+), 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 a295724d0..b63f1e8b5 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -564,7 +564,7 @@ public class AppDetails extends AppCompatActivity { @Override protected void onDestroy() { - cleanUpFinishedDownload(); + unregisterDownloaderReceivers(); super.onDestroy(); } From 18f3d86b68e32a79a2e8cddddcf580ba5f0443b4 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Apr 2016 14:15:50 +1000 Subject: [PATCH 05/11] Show indeterminite progess for apks queued to download. Previously, navigating back to an app which is in the queue qould indeed grey out the "Install" button and show the text "Downloading..." in that disabled button. However, it woulnd't show any sort of progress. This change shows an indeterminite progress bar with the text "Waiting to start download..." underneath. Happy to receive input on the best terminology if that is not desirable. In order to do this, I had to be more specific about when the header fragment is updated. Previously, `headerFragment.updateViews()` would get called by the `onResumeFragments()` activity method. This was redundant because the `onResume()` method of the fragment also invokes `updateViews()`. Thus, that call was removed (though we still need to obtain a reference to the fragment in `onResumeFragments()`. --- .../java/org/fdroid/fdroid/AppDetails.java | 35 ++++++++++++++++--- .../fdroid/fdroid/net/DownloaderService.java | 26 ++++++++++---- app/src/main/res/values/strings.xml | 1 + 3 files changed, 51 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index b63f1e8b5..a9288bed2 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -426,8 +426,8 @@ public class AppDetails extends AppCompatActivity { @Override protected void onResumeFragments() { super.onResumeFragments(); + headerFragment = (AppDetailsHeaderFragment)getSupportFragmentManager().findFragmentById(R.id.header); refreshApkList(); - refreshHeader(); supportInvalidateOptionsMenu(); if (DownloaderService.isQueuedOrActive(activeDownloadUrlString)) { registerDownloaderReceivers(); @@ -615,8 +615,6 @@ public class AppDetails extends AppCompatActivity { } private void refreshHeader() { - headerFragment = (AppDetailsHeaderFragment) - getSupportFragmentManager().findFragmentById(R.id.header); if (headerFragment != null) { headerFragment.updateViews(); } @@ -1416,17 +1414,44 @@ public class AppDetails extends AppCompatActivity { public void onResume() { super.onResume(); updateViews(); + restoreProgressBarOnResume(); + } + + /** + * After resuming the fragment, decide whether or not we need to show the progress bar. + * Also, put an appropriate message depending on whether or not the download is active or + * just queued. + * + * NOTE: this can't be done in the `updateViews` method as it currently stands. The reason + * is because that method gets called all the time, for all sorts of reasons. The progress + * bar is updated with actual progress values in response to async broadcasts. If we always + * tried to force the progress bar in `updateViews`, it would override the values that were + * set by the async progress broadcasts. + */ + private void restoreProgressBarOnResume() { + if (appDetails.activeDownloadUrlString != null) { + // We don't actually know what the current progress is, so this will show an indeterminate + // progress bar until the first progress/complete event we receive. + final String message = DownloaderService.isQueued(appDetails.activeDownloadUrlString) + ? getString(R.string.download_pending) + : ""; + showIndeterminateProgress(message); + } } /** * Displays empty, indeterminate progress bar and related views. */ public void startProgress() { + showIndeterminateProgress(getString(R.string.download_pending)); + updateViews(); + } + + private void showIndeterminateProgress(String message) { setProgressVisible(true); progressBar.setIndeterminate(true); - progressSize.setText(""); + progressSize.setText(message); progressPercent.setText(""); - updateViews(); } /** 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 df1875e86..152f881e4 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -328,18 +328,32 @@ public class DownloaderService extends Service { } /** - * Check if a URL is waiting in the queue for downloading or if actively - * being downloaded. This is useful for checking whether to re-register - * {@link android.content.BroadcastReceiver}s in - * {@link android.app.Activity#onResume()} + * Check if a URL is waiting in the queue for downloading or if actively being downloaded. + * This is useful for checking whether to re-register {@link android.content.BroadcastReceiver}s + * in {@link android.app.Activity#onResume()}. + * @see DownloaderService#isQueued(String) + * @see DownloaderService#isActive(String) */ public static boolean isQueuedOrActive(String urlString) { + return isQueued(urlString) || isActive(urlString); + } + + /** + * Check if a URL is waiting in the queue for downloading. + */ + public static boolean isQueued(String urlString) { if (TextUtils.isEmpty(urlString)) { return false; } Integer what = QUEUE_WHATS.get(urlString); - return (what != null && serviceHandler.hasMessages(what)) - || (downloader != null && TextUtils.equals(urlString, downloader.sourceUrl.toString())); + return (what != null && serviceHandler.hasMessages(what)); + } + + /** + * Check if a URL is actively being downloaded. + */ + public static boolean isActive(String urlString) { + return downloader != null && TextUtils.equals(urlString, downloader.sourceUrl.toString()); } /** diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index e3969b4c6..29a42b952 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -367,6 +367,7 @@ Do you want to uninstall this app? Download completed, tap to install Download unsuccessful + Waiting to start download… New: Provided by %1$s. From 7f1155816e4ac4c7d2fb1a52e111a9b72af61c25 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Apr 2016 15:23:36 +1000 Subject: [PATCH 06/11] Show name of app being downloaded in notification. If a non-apk download is taking place, revert to the current generic "Downloading..." message. --- .../fdroid/fdroid/net/DownloaderService.java | 21 ++++++++++++++++++- app/src/main/res/values/strings.xml | 1 + 2 files changed, 21 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 152f881e4..f72ec99c5 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -45,6 +45,8 @@ 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; +import org.fdroid.fdroid.data.AppProvider; import org.fdroid.fdroid.data.SanitizedFile; import java.io.File; @@ -168,12 +170,29 @@ public class DownloaderService extends Service { return new NotificationCompat.Builder(this) .setAutoCancel(true) .setContentIntent(createAppDetailsIntent(this, 0, packageName)) - .setContentTitle(getString(R.string.downloading)) + .setContentTitle(getNotificationTitle(packageName)) .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; + } + public static PendingIntent createAppDetailsIntent(@NonNull Context context, int requestCode, @Nullable String packageName) { TaskStackBuilder stackBuilder; if (packageName != null) { diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 29a42b952..20d0988a3 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -372,6 +372,7 @@ New: Provided by %1$s. Downloading… + Downloading %1$s Never Hourly From bbf7cd81a6daae218553a75ba3b984c56acb6635 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Apr 2016 15:30:04 +1000 Subject: [PATCH 07/11] Extract common code to isQueued and isActive. --- .../main/java/org/fdroid/fdroid/net/DownloaderService.java | 6 ++---- 1 file changed, 2 insertions(+), 4 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 f72ec99c5..2b1bcf260 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -134,11 +134,9 @@ public class DownloaderService extends Service { if (ACTION_CANCEL.equals(intent.getAction())) { Log.i(TAG, "Removed " + intent); Integer what = QUEUE_WHATS.remove(uriString); - if (what != null && serviceHandler.hasMessages(what)) { - // the URL is in the queue, remove it + if (isQueued(uriString)) { serviceHandler.removeMessages(what); - } else if (downloader != null && TextUtils.equals(uriString, downloader.sourceUrl.toString())) { - // the URL is being downloaded, cancel it + } else if (isActive(uriString)) { downloader.cancelDownload(); } else { Log.e(TAG, "CANCEL called on something not queued or running: " + startId + " " + intent); From b66810944fec802aa119c0e5ec8b7875930a2c22 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 22 Apr 2016 22:53:59 +1000 Subject: [PATCH 08/11] Remove download notification when last queued download cancelled. There was a race condition such that when the cancel `Intent` is received, it would queue up a message on the download thread to stop downloading. This was followed by cancelling the notification on the thread the `Intent` was received on. As a result of the message taking some time to get to the other thread, it would likely cause another progress notification to be shown after we had already removed that notification - making it show again after being cancelled. The code in the download thread progress handler, which updates the notification with progress, is not perfect. It does a check before deciding to broadcast progress and update the notification. However, the check happens at the beginning of the group of expressions. As such, there is likely a small change that `isActive` returns true, then the notification is cancelled on the other thread, and finally, the download thread updates the progress again. However this is better than it was before where the notification didn't go away. Previously, apks were not being removed from the queue once download was completed, only when removed. It didn't cause any specific issues here, but I removed downloads from the queue after completion to prevent potential problems in the future. Now the queue is a better representation of what is to be downloaded. --- .../fdroid/fdroid/net/DownloaderService.java | 37 +++++++++++++------ 1 file changed, 25 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 2b1bcf260..dd21c623d 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -133,7 +133,6 @@ public class DownloaderService extends Service { } if (ACTION_CANCEL.equals(intent.getAction())) { Log.i(TAG, "Removed " + intent); - Integer what = QUEUE_WHATS.remove(uriString); if (isQueued(uriString)) { serviceHandler.removeMessages(what); } else if (isActive(uriString)) { @@ -141,6 +140,11 @@ public class DownloaderService extends Service { } else { Log.e(TAG, "CANCEL called on something not queued or running: " + startId + " " + intent); } + + QUEUE_WHATS.remove(uriString); + if (isQueueEmpty()) { + stopForeground(true); + } } else if (ACTION_QUEUE.equals(intent.getAction())) { if (Preferences.get().isUpdateNotificationEnabled()) { Notification notification = createNotification(intent.getDataString(), getPackageNameFromIntent(intent)).build(); @@ -262,17 +266,19 @@ public class DownloaderService extends Service { downloader.setListener(new Downloader.DownloaderProgressListener() { @Override public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { - Intent intent = new Intent(Downloader.ACTION_PROGRESS); - intent.setData(uri); - intent.putExtra(Downloader.EXTRA_BYTES_READ, bytesRead); - intent.putExtra(Downloader.EXTRA_TOTAL_BYTES, totalBytes); - localBroadcastManager.sendBroadcast(intent); + if (isActive(uri.toString())) { + Intent intent = new Intent(Downloader.ACTION_PROGRESS); + intent.setData(uri); + 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); + NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + Notification notification = createNotification(uri.toString(), packageName) + .setProgress(totalBytes, bytesRead, false) + .build(); + nm.notify(NOTIFY_DOWNLOADING, notification); + } } }); downloader.download(); @@ -288,6 +294,9 @@ public class DownloaderService extends Service { if (downloader != null) { downloader.close(); } + // May have already been removed in response to a cancel intent, but that wont cause + // problems if we ask to remove it again. + QUEUE_WHATS.remove(uri.toString()); } downloader = null; } @@ -355,6 +364,10 @@ public class DownloaderService extends Service { return isQueued(urlString) || isActive(urlString); } + public static boolean isQueueEmpty() { + return QUEUE_WHATS.isEmpty(); + } + /** * Check if a URL is waiting in the queue for downloading. */ @@ -370,7 +383,7 @@ public class DownloaderService extends Service { * Check if a URL is actively being downloaded. */ public static boolean isActive(String urlString) { - return downloader != null && TextUtils.equals(urlString, downloader.sourceUrl.toString()); + return downloader != null && QUEUE_WHATS.containsKey(urlString) && TextUtils.equals(urlString, downloader.sourceUrl.toString()); } /** From a149ce54dbed8110736f660e1a8e283373030308 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 22 Apr 2016 23:13:36 +1000 Subject: [PATCH 09/11] Always stop notifications at the conclusion of a download. Without this, if the first download in the queue is downloaded, the notification will persist with details of that first download until the next download begins. With this change, the notification is remoived immediately after cancelling the download. --- .../fdroid/fdroid/net/DownloaderService.java | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 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 dd21c623d..2b951b892 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -146,10 +146,6 @@ public class DownloaderService extends Service { stopForeground(true); } } else if (ACTION_QUEUE.equals(intent.getAction())) { - if (Preferences.get().isUpdateNotificationEnabled()) { - Notification notification = createNotification(intent.getDataString(), getPackageNameFromIntent(intent)).build(); - startForeground(NOTIFY_DOWNLOADING, notification); - } Log.i(TAG, "Queued " + intent); Message msg = serviceHandler.obtainMessage(); msg.arg1 = startId; @@ -261,6 +257,12 @@ public class DownloaderService extends Service { final SanitizedFile localFile = new SanitizedFile(downloadDir, uri.getLastPathSegment()); final String packageName = getPackageNameFromIntent(intent); sendBroadcast(uri, Downloader.ACTION_STARTED, localFile); + + if (Preferences.get().isUpdateNotificationEnabled()) { + Notification notification = createNotification(intent.getDataString(), getPackageNameFromIntent(intent)).build(); + startForeground(NOTIFY_DOWNLOADING, notification); + } + try { downloader = DownloaderFactory.create(this, uri, localFile); downloader.setListener(new Downloader.DownloaderProgressListener() { @@ -273,11 +275,13 @@ public class DownloaderService extends Service { 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); + 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); + } } } }); @@ -297,6 +301,7 @@ public class DownloaderService extends Service { // May have already been removed in response to a cancel intent, but that wont cause // problems if we ask to remove it again. QUEUE_WHATS.remove(uri.toString()); + stopForeground(true); } downloader = null; } From 3ebeec0b879bbe53bb3125f5df64e9b52cf89998 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 22 Apr 2016 23:22:32 +1000 Subject: [PATCH 10/11] Cleaned up logging in downloader service. Most of the logging is purely for debugging purposes during development. As such, it has been moved to `Utils.debugLog`. Also provided more context in some of the descriptions, so devs reading the logs without the sourcecode will hopefully be able to infer more about what is happening. Left the error logging as `Log.e` as it may be more informative. --- .../fdroid/net/DownloadCompleteService.java | 4 ++-- .../fdroid/fdroid/net/DownloaderService.java | 19 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java index e64ef3e20..4cdb761d7 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java @@ -42,12 +42,12 @@ public class DownloadCompleteService extends IntentService { if (intent != null) { final String action = intent.getAction(); if (!ACTION_NOTIFY.equals(action)) { - Utils.debugLog(TAG, "intent action is not ACTION_NOTIFY"); + Utils.debugLog(TAG, "Intent action is not ACTION_NOTIFY"); return; } String packageName = intent.getStringExtra(EXTRA_PACKAGE_NAME); if (TextUtils.isEmpty(packageName)) { - Utils.debugLog(TAG, "intent is missing EXTRA_PACKAGE_NAME"); + Utils.debugLog(TAG, "Intent is missing EXTRA_PACKAGE_NAME"); return; } 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 2b951b892..c56cba2b2 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -112,7 +112,7 @@ public class DownloaderService extends Service { @Override public void onCreate() { super.onCreate(); - Log.i(TAG, "onCreate"); + Utils.debugLog(TAG, "Creating downloader service."); HandlerThread thread = new HandlerThread(TAG, Process.THREAD_PRIORITY_BACKGROUND); thread.start(); @@ -125,20 +125,20 @@ public class DownloaderService extends Service { @Override public void onStart(Intent intent, int startId) { super.onStart(intent, startId); - Log.i(TAG, "onStart " + startId + " " + intent); + Utils.debugLog(TAG, "Received Intent for downloading: " + intent + " (with a startId of " + startId + ")"); String uriString = intent.getDataString(); if (uriString == null) { Log.e(TAG, "Received Intent with no URI: " + intent); return; } if (ACTION_CANCEL.equals(intent.getAction())) { - Log.i(TAG, "Removed " + intent); + Utils.debugLog(TAG, "Cancelling download of " + uriString); if (isQueued(uriString)) { serviceHandler.removeMessages(what); } else if (isActive(uriString)) { downloader.cancelDownload(); } else { - Log.e(TAG, "CANCEL called on something not queued or running: " + startId + " " + intent); + Log.e(TAG, "ACTION_CANCEL called on something not queued or running"); } QUEUE_WHATS.remove(uriString); @@ -146,14 +146,13 @@ public class DownloaderService extends Service { stopForeground(true); } } else if (ACTION_QUEUE.equals(intent.getAction())) { - Log.i(TAG, "Queued " + intent); Message msg = serviceHandler.obtainMessage(); msg.arg1 = startId; msg.obj = intent; msg.what = what++; serviceHandler.sendMessage(msg); - Log.i(TAG, "QUEUE_WHATS.put(" + uriString + ", " + msg.what); QUEUE_WHATS.put(uriString, msg.what); + Utils.debugLog(TAG, "Queued download of " + uriString + ". Now " + QUEUE_WHATS.size() + " downloads in the queue"); } else { Log.e(TAG, "Received Intent with unknown action: " + intent); } @@ -215,13 +214,13 @@ public class DownloaderService extends Service { @Override public int onStartCommand(Intent intent, int flags, int startId) { onStart(intent, startId); - Log.i(TAG, "onStartCommand " + intent); + Utils.debugLog(TAG, "onStartCommand " + intent); return START_REDELIVER_INTENT; // if killed before completion, retry Intent } @Override public void onDestroy() { - Log.i(TAG, "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 } @@ -331,7 +330,7 @@ public class DownloaderService extends Service { * @see #cancel(Context, String) */ public static void queue(Context context, String packageName, String urlString) { - Log.i(TAG, "queue " + 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)); @@ -351,7 +350,7 @@ public class DownloaderService extends Service { * @see #queue(Context, String, String) */ public static void cancel(Context context, String urlString) { - Log.i(TAG, "cancel " + urlString); + Utils.debugLog(TAG, "Preparing cancellation of " + urlString + " download"); Intent intent = new Intent(context, DownloaderService.class); intent.setAction(ACTION_CANCEL); intent.setData(Uri.parse(urlString)); From 7e83189f5b5722ed9592ff36a96689a8d55294c6 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Fri, 22 Apr 2016 23:53:13 +1000 Subject: [PATCH 11/11] Appese checkstyle + pmd. --- app/src/main/java/org/fdroid/fdroid/AppDetails.java | 2 +- .../java/org/fdroid/fdroid/net/DownloadCompleteService.java | 3 --- .../main/java/org/fdroid/fdroid/net/DownloaderService.java | 6 +++--- 3 files changed, 4 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 a9288bed2..149065263 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -426,7 +426,7 @@ public class AppDetails extends AppCompatActivity { @Override protected void onResumeFragments() { super.onResumeFragments(); - headerFragment = (AppDetailsHeaderFragment)getSupportFragmentManager().findFragmentById(R.id.header); + headerFragment = (AppDetailsHeaderFragment) getSupportFragmentManager().findFragmentById(R.id.header); refreshApkList(); supportInvalidateOptionsMenu(); if (DownloaderService.isQueuedOrActive(activeDownloadUrlString)) { diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java index 4cdb761d7..fa5029d94 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java @@ -2,17 +2,14 @@ package org.fdroid.fdroid.net; import android.app.IntentService; import android.app.NotificationManager; -import android.app.PendingIntent; import android.content.Context; import android.content.Intent; import android.content.pm.PackageManager; import android.net.Uri; import android.os.Process; import android.support.v4.app.NotificationCompat; -import android.support.v4.app.TaskStackBuilder; import android.text.TextUtils; -import org.fdroid.fdroid.AppDetails; import org.fdroid.fdroid.R; import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.data.App; 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 c56cba2b2..2d31a6574 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -182,7 +182,7 @@ public class DownloaderService extends Service { String title; if (packageName != null) { App app = AppProvider.Helper.findByPackageName( - getContentResolver(), packageName, new String[] { AppProvider.DataColumns.NAME }); + getContentResolver(), packageName, new String[] {AppProvider.DataColumns.NAME}); title = getString(R.string.downloading_apk, app.name); } else { title = getString(R.string.downloading); @@ -365,7 +365,7 @@ public class DownloaderService extends Service { * @see DownloaderService#isActive(String) */ public static boolean isQueuedOrActive(String urlString) { - return isQueued(urlString) || isActive(urlString); + return isQueued(urlString) || isActive(urlString); } public static boolean isQueueEmpty() { @@ -380,7 +380,7 @@ public class DownloaderService extends Service { return false; } Integer what = QUEUE_WHATS.get(urlString); - return (what != null && serviceHandler.hasMessages(what)); + return what != null && serviceHandler.hasMessages(what); } /**