From d7643acd52d9dcc0b5b0b7ae02efbd4275a80a9e Mon Sep 17 00:00:00 2001 From: olly Date: Mon, 13 Jan 2020 12:51:34 +0000 Subject: [PATCH] Deprecate DownloadService state change methods As discovered whilst investigating #6798, there are cases where these methods are not correctly. They were added as convenience methods that could be overridden by concrete DownloadService implementations, but since they don't work properly it's preferable to require application code to listen to their DownloadManager directly instead. Notes: - The original proposal to fix #6798 stored the state change events in memory until they could be delivered. This approach is not ideal because the events still end up being delivered later than they should be. We also want to fix the root cause in a different way that does not require doing this. - This change does not fix #6798. It's a preparatory step. Issue: #6798 PiperOrigin-RevId: 289418555 --- .../exoplayer2/demo/DemoDownloadService.java | 69 +++++++++++++------ .../exoplayer2/offline/DownloadService.java | 17 +++-- 2 files changed, 58 insertions(+), 28 deletions(-) diff --git a/demos/main/src/main/java/com/google/android/exoplayer2/demo/DemoDownloadService.java b/demos/main/src/main/java/com/google/android/exoplayer2/demo/DemoDownloadService.java index c3909dfe46..be2863d4eb 100644 --- a/demos/main/src/main/java/com/google/android/exoplayer2/demo/DemoDownloadService.java +++ b/demos/main/src/main/java/com/google/android/exoplayer2/demo/DemoDownloadService.java @@ -16,6 +16,7 @@ package com.google.android.exoplayer2.demo; import android.app.Notification; +import android.content.Context; import com.google.android.exoplayer2.offline.Download; import com.google.android.exoplayer2.offline.DownloadManager; import com.google.android.exoplayer2.offline.DownloadService; @@ -32,8 +33,6 @@ public class DemoDownloadService extends DownloadService { private static final int JOB_ID = 1; private static final int FOREGROUND_NOTIFICATION_ID = 1; - private static int nextNotificationId = FOREGROUND_NOTIFICATION_ID + 1; - private DownloadNotificationHelper notificationHelper; public DemoDownloadService() { @@ -43,7 +42,6 @@ public class DemoDownloadService extends DownloadService { CHANNEL_ID, R.string.exo_download_notification_channel_name, /* channelDescriptionResourceId= */ 0); - nextNotificationId = FOREGROUND_NOTIFICATION_ID + 1; } @Override @@ -54,7 +52,13 @@ public class DemoDownloadService extends DownloadService { @Override protected DownloadManager getDownloadManager() { - return ((DemoApplication) getApplication()).getDownloadManager(); + DownloadManager downloadManager = ((DemoApplication) getApplication()).getDownloadManager(); + // This will only happen once, because getDownloadManager is guaranteed to be called only once + // in the life cycle of the process. + downloadManager.addListener( + new TerminalStateNotificationHelper( + this, notificationHelper, FOREGROUND_NOTIFICATION_ID + 1)); + return downloadManager; } @Override @@ -68,24 +72,45 @@ public class DemoDownloadService extends DownloadService { R.drawable.ic_download, /* contentIntent= */ null, /* message= */ null, downloads); } - @Override - protected void onDownloadChanged(Download download) { - Notification notification; - if (download.state == Download.STATE_COMPLETED) { - notification = - notificationHelper.buildDownloadCompletedNotification( - R.drawable.ic_download_done, - /* contentIntent= */ null, - Util.fromUtf8Bytes(download.request.data)); - } else if (download.state == Download.STATE_FAILED) { - notification = - notificationHelper.buildDownloadFailedNotification( - R.drawable.ic_download_done, - /* contentIntent= */ null, - Util.fromUtf8Bytes(download.request.data)); - } else { - return; + /** + * Creates and displays notifications for downloads when they complete or fail. + * + *

This helper will outlive the lifespan of a single instance of {@link DemoDownloadService}. + * It is static to avoid leaking the first {@link DemoDownloadService} instance. + */ + private static final class TerminalStateNotificationHelper implements DownloadManager.Listener { + + private final Context context; + private final DownloadNotificationHelper notificationHelper; + + private int nextNotificationId; + + public TerminalStateNotificationHelper( + Context context, DownloadNotificationHelper notificationHelper, int firstNotificationId) { + this.context = context.getApplicationContext(); + this.notificationHelper = notificationHelper; + nextNotificationId = firstNotificationId; + } + + @Override + public void onDownloadChanged(DownloadManager manager, Download download) { + Notification notification; + if (download.state == Download.STATE_COMPLETED) { + notification = + notificationHelper.buildDownloadCompletedNotification( + R.drawable.ic_download_done, + /* contentIntent= */ null, + Util.fromUtf8Bytes(download.request.data)); + } else if (download.state == Download.STATE_FAILED) { + notification = + notificationHelper.buildDownloadFailedNotification( + R.drawable.ic_download_done, + /* contentIntent= */ null, + Util.fromUtf8Bytes(download.request.data)); + } else { + return; + } + NotificationUtil.setNotification(context, nextNotificationId++, notification); } - NotificationUtil.setNotification(this, nextNotificationId++, notification); } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java index db10517b67..1260497e1c 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java @@ -731,23 +731,27 @@ public abstract class DownloadService extends Service { } /** - * Called when the state of a download changes. The default implementation is a no-op. - * - * @param download The new state of the download. + * @deprecated Some state change events may not be delivered to this method. Instead, use {@link + * DownloadManager#addListener(DownloadManager.Listener)} to register a listener directly to + * the {@link DownloadManager} that you return through {@link #getDownloadManager()}. */ + @Deprecated protected void onDownloadChanged(Download download) { // Do nothing. } /** - * Called when a download is removed. The default implementation is a no-op. - * - * @param download The last state of the download before it was removed. + * @deprecated Some download removal events may not be delivered to this method. Instead, use + * {@link DownloadManager#addListener(DownloadManager.Listener)} to register a listener + * directly to the {@link DownloadManager} that you return through {@link + * #getDownloadManager()}. */ + @Deprecated protected void onDownloadRemoved(Download download) { // Do nothing. } + @SuppressWarnings("deprecation") private void notifyDownloadChanged(Download download) { onDownloadChanged(download); if (foregroundNotificationUpdater != null) { @@ -761,6 +765,7 @@ public abstract class DownloadService extends Service { } } + @SuppressWarnings("deprecation") private void notifyDownloadRemoved(Download download) { onDownloadRemoved(download); if (foregroundNotificationUpdater != null) {