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
This commit is contained in:
olly 2020-01-13 12:51:34 +00:00 committed by Oliver Woodman
parent 15f974a277
commit d7643acd52
2 changed files with 58 additions and 28 deletions

View file

@ -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.
*
* <p>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);
}
}

View file

@ -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) {