From dc6221fb8250edfd937ff39edffa141c8205fb5b Mon Sep 17 00:00:00 2001 From: orionlee Date: Sat, 26 Oct 2019 20:03:41 -0700 Subject: [PATCH] respect download order - dequeue upon cancelling download --- .../service/download/DownloadServiceTest.java | 70 ++++++++++++++++++- .../core/preferences/UserPreferences.java | 5 ++ .../service/download/DownloadRequest.java | 18 +++++ .../service/download/DownloadService.java | 36 ++++++++-- 4 files changed, 122 insertions(+), 7 deletions(-) diff --git a/app/src/androidTest/java/de/test/antennapod/service/download/DownloadServiceTest.java b/app/src/androidTest/java/de/test/antennapod/service/download/DownloadServiceTest.java index 93723bc78..d0e14d70a 100644 --- a/app/src/androidTest/java/de/test/antennapod/service/download/DownloadServiceTest.java +++ b/app/src/androidTest/java/de/test/antennapod/service/download/DownloadServiceTest.java @@ -5,7 +5,6 @@ import androidx.annotation.Nullable; import androidx.test.InstrumentationRegistry; import androidx.test.runner.AndroidJUnit4; -import de.danoeh.antennapod.core.service.download.DownloaderFactory; import org.awaitility.Awaitility; import org.awaitility.core.ConditionTimeoutException; import org.junit.After; @@ -27,6 +26,7 @@ import de.danoeh.antennapod.core.service.download.DownloadRequest; import de.danoeh.antennapod.core.service.download.DownloadService; import de.danoeh.antennapod.core.service.download.DownloadStatus; import de.danoeh.antennapod.core.service.download.Downloader; +import de.danoeh.antennapod.core.service.download.DownloaderFactory; import de.danoeh.antennapod.core.service.download.StubDownloader; import de.danoeh.antennapod.core.storage.DBReader; import de.danoeh.antennapod.core.storage.DBWriter; @@ -59,7 +59,9 @@ public class DownloadServiceTest { } private Feed setUpTestFeeds() throws Exception { - Feed feed = new Feed("url", null, "Test Feed title 1"); + // To avoid complication in case of test failures, leaving behind orphaned + // media files: add a timestamp so that each test run will have its own directory for media files. + Feed feed = new Feed("url", null, "Test Feed title 1 " + System.currentTimeMillis()); List items = new ArrayList<>(); feed.setItems(items); FeedItem item1 = new FeedItem(0, "Item 1-1", "Item 1-1", "url", new Date(), FeedItem.NEW, feed); @@ -123,6 +125,70 @@ public class DownloadServiceTest { }); } + @Test + public void testCancelDownload_UndoEnqueue_Normal() throws Exception { + doTestCancelDownload_UndoEnqueue(false); + } + + @Test + public void testCancelDownload_UndoEnqueue_AlreadyInQueue() throws Exception { + doTestCancelDownload_UndoEnqueue(true); + } + + private void doTestCancelDownload_UndoEnqueue(boolean itemAlreadyInQueue) throws Exception { + // let download takes longer to ensure the test can cancel the download in time + DownloadService.setDownloaderFactory(new StubDownloaderFactory(150, downloadStatus -> { + downloadStatus.setSuccessful(); + })); + UserPreferences.setEnqueueDownloadedEpisodes(true); + UserPreferences.setEnableAutodownload(false); + + final long item1Id = testMedia11.getItem().getId(); + if (itemAlreadyInQueue) { + // simulate item already in queue condition + DBWriter.addQueueItem(InstrumentationRegistry.getTargetContext(), false, item1Id).get(); + assertTrue(DBReader.getQueueIDList().contains(item1Id)); + } else { + assertFalse(DBReader.getQueueIDList().contains(item1Id)); + } + + withFeedItemEventListener(feedItemEventListener -> { + try { + DownloadRequester.getInstance().downloadMedia(false, InstrumentationRegistry.getTargetContext(), + testMedia11.getItem()); + if (itemAlreadyInQueue) { + Awaitility.await("download service receives the request - " + + "no event is expected before cancel is issued") + .atLeast(100, TimeUnit.MILLISECONDS) + .until(() -> true); + } else { + Awaitility.await("item enqueue event") + .atMost(1000, TimeUnit.MILLISECONDS) + .until(() -> feedItemEventListener.getEvents().size() >= 1); + } + DownloadRequester.getInstance().cancelDownload(InstrumentationRegistry.getTargetContext(), + testMedia11); + final int totalNumEventsExpected = itemAlreadyInQueue ? 1 : 3; + Awaitility.await("item dequeue event + download termination event") + .atMost(1000, TimeUnit.MILLISECONDS) + .until(() ->feedItemEventListener.getEvents().size() >= totalNumEventsExpected); + assertFalse("The download should have been canceled", + DBReader.getFeedMedia(testMedia11.getId()).isDownloaded()); + if (itemAlreadyInQueue) { + assertTrue("The FeedItem should still be in the queue after the download is cancelled." + + " It's there before download.", + DBReader.getQueueIDList().contains(item1Id)); + } else { + assertFalse("The FeedItem should not be in the queue after the download is cancelled.", + DBReader.getQueueIDList().contains(item1Id)); + } + } catch (ConditionTimeoutException cte) { + fail("The expected FeedItemEvent (for media download complete) has not been posted. " + + cte.getMessage()); + } + }); + } + private static class StubDownloaderFactory implements DownloaderFactory { private final long downloadTime; diff --git a/core/src/main/java/de/danoeh/antennapod/core/preferences/UserPreferences.java b/core/src/main/java/de/danoeh/antennapod/core/preferences/UserPreferences.java index a3d3b56e0..750f79711 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/preferences/UserPreferences.java +++ b/core/src/main/java/de/danoeh/antennapod/core/preferences/UserPreferences.java @@ -544,6 +544,11 @@ public class UserPreferences { return prefs.getBoolean(PREF_ENABLE_AUTODL, false); } + @VisibleForTesting + public static void setEnableAutodownload(boolean enabled) { + prefs.edit().putBoolean(PREF_ENABLE_AUTODL, enabled).apply(); + } + public static boolean isEnableAutodownloadOnBattery() { return prefs.getBoolean(PREF_ENABLE_AUTODL_ON_BATTERY, true); } diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadRequest.java b/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadRequest.java index aa77c85b6..2dd46cf96 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadRequest.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadRequest.java @@ -28,6 +28,7 @@ public class DownloadRequest implements Parcelable { private long soFar; private long size; private int statusMsg; + private boolean mediaEnqueued; public DownloadRequest(@NonNull String destination, @NonNull String source, @@ -47,6 +48,7 @@ public class DownloadRequest implements Parcelable { this.username = username; this.password = password; this.deleteOnFailure = deleteOnFailure; + this.mediaEnqueued = false; this.arguments = (arguments != null) ? arguments : new Bundle(); } @@ -78,6 +80,7 @@ public class DownloadRequest implements Parcelable { deleteOnFailure = (in.readByte() > 0); username = nullIfEmpty(in.readString()); password = nullIfEmpty(in.readString()); + mediaEnqueued = (in.readByte() > 0); arguments = in.readBundle(); } @@ -102,6 +105,7 @@ public class DownloadRequest implements Parcelable { // see: https://stackoverflow.com/a/22926342 dest.writeString(nonNullString(username)); dest.writeString(nonNullString(password)); + dest.writeByte((mediaEnqueued) ? (byte) 1 : 0); dest.writeBundle(arguments); } @@ -148,6 +152,7 @@ public class DownloadRequest implements Parcelable { if (title != null ? !title.equals(that.title) : that.title != null) return false; if (username != null ? !username.equals(that.username) : that.username != null) return false; + if (mediaEnqueued != that.mediaEnqueued) return false; return true; } @@ -167,6 +172,7 @@ public class DownloadRequest implements Parcelable { result = 31 * result + (int) (soFar ^ (soFar >>> 32)); result = 31 * result + (int) (size ^ (size >>> 32)); result = 31 * result + statusMsg; + result = 31 * result + (mediaEnqueued ? 1 : 0); return result; } @@ -248,6 +254,18 @@ public class DownloadRequest implements Parcelable { return deleteOnFailure; } + public boolean isMediaEnqueued() { + return mediaEnqueued; + } + + /** + * Set to true if the media is enqueued because of this download. + * The state is helpful if the download is cancelled, and undoing the enqueue is needed. + */ + public void setMediaEnqueued(boolean mediaEnqueued) { + this.mediaEnqueued = mediaEnqueued; + } + public Bundle getArguments() { return arguments; } diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadService.java b/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadService.java index d9e898a88..cf0502c92 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadService.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/download/DownloadService.java @@ -371,10 +371,20 @@ public class DownloadService extends Service { Downloader d = getDownloader(url); if (d != null) { d.cancel(); - DownloadRequester.getInstance().removeDownload(d.getDownloadRequest()); + DownloadRequest request = d.getDownloadRequest(); + DownloadRequester.getInstance().removeDownload(request); - FeedItem item = getFeedItemFromId(d.getDownloadRequest().getFeedfileId()); + FeedItem item = getFeedItemFromId(request.getFeedfileId()); if (item != null) { + // undo enqueue upon cancel + if (request.isMediaEnqueued()) { + Log.v(TAG, "Undoing enqueue upon cancelling download"); + try { + DBWriter.removeQueueItem(getApplicationContext(), false, item).get(); + } catch (Throwable t) { + Log.e(TAG, "Unexpected exception during undoing enqueue upon cancel", t); + } + } EventBus.getDefault().post(FeedItemEvent.updated(item)); } } else { @@ -418,10 +428,9 @@ public class DownloadService extends Service { Log.e(TAG, "Unexpected exception during enqueue before downloads. Abort download", e); return; } - // TODO-2448: use itemsEnqueued to handle cancel download for (DownloadRequest request : requests) { - onDownloadQueued(request); + onDownloadQueued(request, itemsEnqueued); } } @@ -443,7 +452,8 @@ public class DownloadService extends Service { return DBTasks.enqueueFeedItemsToDownload(getApplicationContext(), feedItems); } - private void onDownloadQueued(@NonNull DownloadRequest request) { + private void onDownloadQueued(@NonNull DownloadRequest request, + @NonNull List itemsEnqueued) { writeFileUrl(request); Downloader downloader = downloaderFactory.create(request); @@ -453,6 +463,9 @@ public class DownloadService extends Service { if (request.getFeedfileType() == Feed.FEEDFILETYPE_FEED) { downloads.add(0, downloader); } else { + if (isEnqueued(request, itemsEnqueued)) { + request.setMediaEnqueued(true); + } downloads.add(downloader); } downloadExecutor.submit(downloader); @@ -463,6 +476,19 @@ public class DownloadService extends Service { queryDownloads(); } + private static boolean isEnqueued(@NonNull DownloadRequest request, + @NonNull List itemsEnqueued) { + if (request.getFeedfileType() == FeedMedia.FEEDFILETYPE_FEEDMEDIA) { + final long mediaId = request.getFeedfileId(); + for (FeedItem item : itemsEnqueued) { + if (item.getMedia() != null && item.getMedia().getId() == mediaId) { + return true; + } + } + } + return false; + } + @VisibleForTesting public static DownloaderFactory getDownloaderFactory() { return downloaderFactory;