Get rid of DownloadState.STATE_REMOVED

It's a transient state whose only use is when passing a removed
download to onDownloadStateChanged. It's a bit strange to have it
as a proper state, since we end up asserting that it's an invalid
value in other places.

This change adds an explicit onDownloadRemoved, which allows
removal of the transient STATE_REMOVED, related assertions, and
uncertainty when dealing with an @State variable whether it's
necessary to handle it being STATE_REMOVED.

PiperOrigin-RevId: 242444128
This commit is contained in:
olly 2019-04-08 13:10:13 +01:00 committed by Oliver Woodman
parent 925dd5aa12
commit 0daaba443c
8 changed files with 107 additions and 48 deletions

View file

@ -124,13 +124,18 @@ public class DownloadTracker implements DownloadManager.Listener {
@Override
public void onDownloadStateChanged(DownloadManager downloadManager, DownloadState downloadState) {
boolean downloaded = isDownloaded(downloadState.uri);
if (downloadState.state == DownloadState.STATE_REMOVED) {
downloadStates.remove(downloadState.uri);
} else {
downloadStates.put(downloadState.uri, downloadState);
boolean downloadAdded = downloadStates.put(downloadState.uri, downloadState) == null;
if (downloadAdded) {
for (Listener listener : listeners) {
listener.onDownloadsChanged();
}
}
if (downloaded != isDownloaded(downloadState.uri)) {
}
@Override
public void onDownloadRemoved(DownloadManager downloadManager, DownloadState downloadState) {
boolean downloadRemoved = downloadStates.remove(downloadState.uri) != null;
if (downloadRemoved) {
for (Listener listener : listeners) {
listener.onDownloadsChanged();
}

View file

@ -192,7 +192,6 @@ public final class DefaultDownloadIndex implements DownloadIndex {
*/
public void putDownloadState(DownloadState downloadState) throws DatabaseIOException {
ensureInitialized();
Assertions.checkState(downloadState.state != DownloadState.STATE_REMOVED);
try {
ContentValues values = new ContentValues();
values.put(COLUMN_ID, downloadState.id);

View file

@ -23,7 +23,6 @@ import static com.google.android.exoplayer2.offline.DownloadState.STATE_COMPLETE
import static com.google.android.exoplayer2.offline.DownloadState.STATE_DOWNLOADING;
import static com.google.android.exoplayer2.offline.DownloadState.STATE_FAILED;
import static com.google.android.exoplayer2.offline.DownloadState.STATE_QUEUED;
import static com.google.android.exoplayer2.offline.DownloadState.STATE_REMOVED;
import static com.google.android.exoplayer2.offline.DownloadState.STATE_REMOVING;
import static com.google.android.exoplayer2.offline.DownloadState.STATE_RESTARTING;
import static com.google.android.exoplayer2.offline.DownloadState.STATE_STOPPED;
@ -81,6 +80,14 @@ public final class DownloadManager {
default void onDownloadStateChanged(
DownloadManager downloadManager, DownloadState downloadState) {}
/**
* Called when a download is removed.
*
* @param downloadManager The reporting instance.
* @param downloadState The last state of the download before it was removed.
*/
default void onDownloadRemoved(DownloadManager downloadManager, DownloadState downloadState) {}
/**
* Called when there is no active download left.
*
@ -114,6 +121,7 @@ public final class DownloadManager {
private static final int MSG_INITIALIZED = 0;
private static final int MSG_PROCESSED = 1;
private static final int MSG_DOWNLOAD_STATE_CHANGED = 2;
private static final int MSG_DOWNLOAD_REMOVED = 3;
// Messages posted to the background handler.
private static final int MSG_INITIALIZE = 0;
@ -470,6 +478,10 @@ public final class DownloadManager {
DownloadState state = (DownloadState) message.obj;
onDownloadStateChanged(state);
break;
case MSG_DOWNLOAD_REMOVED:
state = (DownloadState) message.obj;
onDownloadRemoved(state);
break;
case MSG_PROCESSED:
int processedMessageCount = message.arg1;
int activeDownloadCount = message.arg2;
@ -495,7 +507,7 @@ public final class DownloadManager {
}
private void onDownloadStateChanged(DownloadState downloadState) {
if (isFinished(downloadState.state)) {
if (downloadState.state == STATE_COMPLETED || downloadState.state == STATE_FAILED) {
downloadStates.remove(downloadState.id);
} else {
downloadStates.put(downloadState.id, downloadState);
@ -505,6 +517,13 @@ public final class DownloadManager {
}
}
private void onDownloadRemoved(DownloadState downloadState) {
downloadStates.remove(downloadState.id);
for (Listener listener : listeners) {
listener.onDownloadRemoved(this, downloadState);
}
}
private void onMessageProcessed(int processedMessageCount, int activeDownloadCount) {
this.pendingMessages -= processedMessageCount;
this.activeDownloadCount = activeDownloadCount;
@ -615,15 +634,30 @@ public final class DownloadManager {
}
}
private void onDownloadStateChange(Download download, DownloadState downloadState) {
private void onDownloadStateChangedInternal(Download download, DownloadState downloadState) {
logd("Download state is changed", download);
updateDownloadIndex(downloadState);
if (isFinished(download.state)) {
try {
downloadIndex.putDownloadState(downloadState);
} catch (DatabaseIOException e) {
Log.e(TAG, "Failed to update index", e);
}
if (download.state == STATE_COMPLETED || download.state == STATE_FAILED) {
downloads.remove(download);
}
mainHandler.obtainMessage(MSG_DOWNLOAD_STATE_CHANGED, downloadState).sendToTarget();
}
private void onDownloadRemovedInternal(Download download, DownloadState downloadState) {
logd("Download is removed", download);
try {
downloadIndex.removeDownloadState(downloadState.id);
} catch (DatabaseIOException e) {
Log.e(TAG, "Failed to remove from index", e);
}
downloads.remove(download);
mainHandler.obtainMessage(MSG_DOWNLOAD_REMOVED, downloadState).sendToTarget();
}
private void setNotMetRequirementsInternal(
@Requirements.RequirementFlags int notMetRequirements) {
if (this.notMetRequirements == notMetRequirements) {
@ -687,18 +721,6 @@ public final class DownloadManager {
download.initialize();
}
private void updateDownloadIndex(DownloadState downloadState) {
try {
if (downloadState.state == DownloadState.STATE_REMOVED) {
downloadIndex.removeDownloadState(downloadState.id);
} else {
downloadIndex.putDownloadState(downloadState);
}
} catch (DatabaseIOException e) {
Log.e(TAG, "updateDownloadIndex failed", e);
}
}
private static void logd(String message) {
if (DEBUG) {
Log.d(TAG, message);
@ -779,10 +801,6 @@ public final class DownloadManager {
}
}
private static boolean isFinished(@DownloadState.State int state) {
return state == STATE_FAILED || state == STATE_COMPLETED || state == STATE_REMOVED;
}
private static final class Download {
private final DownloadManager downloadManager;
@ -867,7 +885,6 @@ public final class DownloadManager {
}
public DownloadAction getAction() {
Assertions.checkState(state != STATE_REMOVED);
return DownloadAction.createDownloadAction(
downloadState.type,
downloadState.uri,
@ -897,7 +914,7 @@ public final class DownloadManager {
}
}
if (oldDownloadState == downloadState) {
downloadManager.onDownloadStateChange(this, getUpdatedDownloadState());
downloadManager.onDownloadStateChangedInternal(this, getUpdatedDownloadState());
}
}
@ -913,7 +930,7 @@ public final class DownloadManager {
setState(STATE_STOPPED);
}
if (state == initialState) {
downloadManager.onDownloadStateChange(this, getUpdatedDownloadState());
downloadManager.onDownloadStateChangedInternal(this, getUpdatedDownloadState());
}
}
@ -935,7 +952,7 @@ public final class DownloadManager {
private void setState(@DownloadState.State int newState) {
if (state != newState) {
state = newState;
downloadManager.onDownloadStateChange(this, getUpdatedDownloadState());
downloadManager.onDownloadStateChangedInternal(this, getUpdatedDownloadState());
}
}
@ -945,10 +962,10 @@ public final class DownloadManager {
}
if (isCanceled) {
downloadManager.startDownloadThread(this);
} else if (state == STATE_REMOVING) {
downloadManager.onDownloadRemovedInternal(this, getUpdatedDownloadState());
} else if (state == STATE_RESTARTING) {
initialize(STATE_QUEUED);
} else if (state == STATE_REMOVING) {
setState(STATE_REMOVED);
} else { // STATE_DOWNLOADING
if (error != null) {
Log.e(TAG, "Download failed: " + downloadState.id, error);

View file

@ -509,6 +509,15 @@ public abstract class DownloadService extends Service {
// Do nothing.
}
/**
* Called when a download is removed. The default implementation is a no-op.
*
* @param downloadState The last state of the download before it was removed.
*/
protected void onDownloadRemoved(DownloadState downloadState) {
// Do nothing.
}
private void notifyDownloadStateChange(DownloadState downloadState) {
onDownloadStateChanged(downloadState);
if (foregroundNotificationUpdater != null) {
@ -522,6 +531,13 @@ public abstract class DownloadService extends Service {
}
}
private void notifyDownloadRemoved(DownloadState downloadState) {
onDownloadRemoved(downloadState);
if (foregroundNotificationUpdater != null) {
foregroundNotificationUpdater.update();
}
}
private void stop() {
if (foregroundNotificationUpdater != null) {
foregroundNotificationUpdater.stopPeriodicUpdates();
@ -642,6 +658,13 @@ public abstract class DownloadService extends Service {
}
}
@Override
public void onDownloadRemoved(DownloadManager downloadManager, DownloadState downloadState) {
if (downloadService != null) {
downloadService.notifyDownloadRemoved(downloadState);
}
}
@Override
public final void onIdle(DownloadManager downloadManager) {
if (downloadService != null) {

View file

@ -34,8 +34,8 @@ public final class DownloadState {
/**
* Download states. One of {@link #STATE_QUEUED}, {@link #STATE_STOPPED}, {@link
* #STATE_DOWNLOADING}, {@link #STATE_COMPLETED}, {@link #STATE_FAILED}, {@link #STATE_REMOVING},
* {@link #STATE_REMOVED} or {@link #STATE_RESTARTING}.
* #STATE_DOWNLOADING}, {@link #STATE_COMPLETED}, {@link #STATE_FAILED}, {@link #STATE_REMOVING}
* or {@link #STATE_RESTARTING}.
*/
@Documented
@Retention(RetentionPolicy.SOURCE)
@ -46,10 +46,10 @@ public final class DownloadState {
STATE_COMPLETED,
STATE_FAILED,
STATE_REMOVING,
STATE_REMOVED,
STATE_RESTARTING
})
public @interface State {}
// Important: These constants are persisted into DownloadIndex. Do not change them.
/** The download is waiting to be started. */
public static final int STATE_QUEUED = 0;
/** The download is stopped. */
@ -62,8 +62,6 @@ public final class DownloadState {
public static final int STATE_FAILED = 4;
/** The download is being removed. */
public static final int STATE_REMOVING = 5;
/** The download is removed. */
public static final int STATE_REMOVED = 6;
/** The download will restart after all downloaded data is removed. */
public static final int STATE_RESTARTING = 7;
@ -97,8 +95,6 @@ public final class DownloadState {
return "FAILED";
case STATE_REMOVING:
return "REMOVING";
case STATE_REMOVED:
return "REMOVED";
case STATE_RESTARTING:
return "RESTARTING";
default:

View file

@ -565,10 +565,6 @@ public class DownloadManagerTest {
return assertState(DownloadState.STATE_REMOVING);
}
private TaskWrapper assertRemoved() {
return assertState(DownloadState.STATE_REMOVED);
}
private TaskWrapper assertFailed() {
return assertState(DownloadState.STATE_FAILED);
}
@ -586,6 +582,11 @@ public class DownloadManagerTest {
return this;
}
private TaskWrapper assertRemoved() {
downloadManagerListener.assertRemoved(taskId, ASSERT_TRUE_TIMEOUT);
return this;
}
@Override
public boolean equals(Object o) {
if (this == o) {

View file

@ -65,8 +65,7 @@ public final class DownloadNotificationHelper {
boolean haveRemoveTasks = false;
for (DownloadState downloadState : downloadStates) {
if (downloadState.state == DownloadState.STATE_REMOVING
|| downloadState.state == DownloadState.STATE_RESTARTING
|| downloadState.state == DownloadState.STATE_REMOVED) {
|| downloadState.state == DownloadState.STATE_RESTARTING) {
haveRemoveTasks = true;
continue;
}

View file

@ -34,6 +34,7 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen
private static final int TIMEOUT = 1000;
private static final int INITIALIZATION_TIMEOUT = 10000;
private static final int STATE_REMOVED = -1;
private final DownloadManager downloadManager;
private final DummyMainThread dummyMainThread;
@ -79,6 +80,11 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen
getStateQueue(downloadState.id).add(downloadState.state);
}
@Override
public void onDownloadRemoved(DownloadManager downloadManager, DownloadState downloadState) {
getStateQueue(downloadState.id).add(STATE_REMOVED);
}
@Override
public synchronized void onIdle(DownloadManager downloadManager) {
if (downloadFinishedCondition != null) {
@ -120,7 +126,15 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen
}
}
public void assertRemoved(String taskId, int timeoutMs) {
assertStateInternal(taskId, STATE_REMOVED, timeoutMs);
}
public void assertState(String taskId, @State int expectedState, int timeoutMs) {
assertStateInternal(taskId, expectedState, timeoutMs);
}
private void assertStateInternal(String taskId, int expectedState, int timeoutMs) {
ArrayList<Integer> receivedStates = new ArrayList<>();
while (true) {
Integer state = null;
@ -140,7 +154,12 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen
if (i > 0) {
sb.append(',');
}
sb.append(DownloadState.getStateString(receivedStates.get(i)));
int receivedState = receivedStates.get(i);
String receivedStateString =
receivedState == STATE_REMOVED
? "REMOVED"
: DownloadState.getStateString(receivedState);
sb.append(receivedStateString);
}
fail(
String.format(