diff options
Diffstat (limited to 'core/src')
6 files changed, 193 insertions, 21 deletions
diff --git a/core/src/main/java/de/danoeh/antennapod/core/service/download/handler/MediaDownloadedHandler.java b/core/src/main/java/de/danoeh/antennapod/core/service/download/handler/MediaDownloadedHandler.java index cf5a84eea..7465b5b38 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/service/download/handler/MediaDownloadedHandler.java +++ b/core/src/main/java/de/danoeh/antennapod/core/service/download/handler/MediaDownloadedHandler.java @@ -3,22 +3,22 @@ package de.danoeh.antennapod.core.service.download.handler; import android.content.Context; import android.media.MediaMetadataRetriever; import android.util.Log; + import androidx.annotation.NonNull; + +import java.util.concurrent.ExecutionException; + import de.danoeh.antennapod.core.feed.FeedItem; import de.danoeh.antennapod.core.feed.FeedMedia; import de.danoeh.antennapod.core.gpoddernet.model.GpodnetEpisodeAction; import de.danoeh.antennapod.core.preferences.GpodnetPreferences; -import de.danoeh.antennapod.core.preferences.UserPreferences; import de.danoeh.antennapod.core.service.download.DownloadRequest; import de.danoeh.antennapod.core.service.download.DownloadStatus; import de.danoeh.antennapod.core.storage.DBReader; -import de.danoeh.antennapod.core.storage.DBTasks; import de.danoeh.antennapod.core.storage.DBWriter; import de.danoeh.antennapod.core.util.ChapterUtils; import de.danoeh.antennapod.core.util.DownloadError; -import java.util.concurrent.ExecutionException; - /** * Handles a completed media download. */ @@ -82,11 +82,6 @@ public class MediaDownloadedHandler implements Runnable { // to ensure subscribers will get the updated FeedMedia as well DBWriter.setFeedItem(item).get(); } - - if (item != null && UserPreferences.enqueueDownloadedEpisodes() - && !DBTasks.isInQueue(context, item.getId())) { - DBWriter.addQueueItem(context, item).get(); - } } catch (InterruptedException e) { Log.e(TAG, "MediaHandlerThread was interrupted"); } catch (ExecutionException e) { diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/DBTasks.java b/core/src/main/java/de/danoeh/antennapod/core/storage/DBTasks.java index 9d37a5f2a..def4d84b5 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/DBTasks.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/DBTasks.java @@ -329,6 +329,10 @@ public final class DBTasks { }.start(); } + // #2448: First, add to-download items to the queue before actual download + // so that the resulting queue order is the same as when download is clicked + DBWriter.addQueueItem(context, items); + // Then, download them for (FeedItem item : items) { if (item.getMedia() != null && !requester.isDownloadingFile(item.getMedia()) diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/DBWriter.java b/core/src/main/java/de/danoeh/antennapod/core/storage/DBWriter.java index 097f07b27..1ba58f3d3 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/DBWriter.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/DBWriter.java @@ -330,7 +330,7 @@ public class DBWriter { new ItemEnqueuePositionCalculator.Options() .setEnqueueAtFront(UserPreferences.enqueueAtFront()) .setKeepInProgressAtFront(UserPreferences.keepInProgressAtFront()) - ); + ); for (int i = 0; i < itemIds.length; i++) { if (!itemListContains(queue, itemIds[i])) { @@ -426,6 +426,12 @@ public class DBWriter { private final @NonNull Options options; + /** + * The logic needs to use {@link DownloadRequester#isDownloadingFile(FeedFile)} method only + */ + @VisibleForTesting + FeedFileDownloadStatusRequesterInterface requester = DownloadRequester.getInstance(); + public ItemEnqueuePositionCalculator(@NonNull Options options) { this.options = options; } @@ -447,16 +453,44 @@ public class DBWriter { curQueue.size() > 0 && curQueue.get(0).getMedia() != null && curQueue.get(0).getMedia().isInProgress()) { - return positionAmongToAdd + 1; // leave the front in progress item at the front + // leave the front in progress item at the front + return getPositionOf1stNonDownloadingItem(positionAmongToAdd + 1, curQueue); } else { // typical case // return NOT 0, so that when a list of items are inserted, the items inserted // keep the same order. Returning 0 will reverse the order - return positionAmongToAdd; + return getPositionOf1stNonDownloadingItem(positionAmongToAdd, curQueue); } } else { return curQueue.size(); } } + + private int getPositionOf1stNonDownloadingItem(int startPosition, List<FeedItem> curQueue) { + final int curQueueSize = curQueue.size(); + for (int i = startPosition; i < curQueueSize; i++) { + if (!isItemAtPositionDownloading(i, curQueue)) { + return i; + } // else continue to search; + } + return curQueueSize; + } + + private boolean isItemAtPositionDownloading(int position, List<FeedItem> curQueue) { + FeedItem curItem; + try { + curItem = curQueue.get(position); + } catch (IndexOutOfBoundsException e) { + curItem = null; + } + + if (curItem != null && + curItem.getMedia() != null && + requester.isDownloadingFile(curItem.getMedia())) { + return true; + } else { + return false; + } + } } /** diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/DownloadRequester.java b/core/src/main/java/de/danoeh/antennapod/core/storage/DownloadRequester.java index 71f6845c5..8d4a197ad 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/DownloadRequester.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/DownloadRequester.java @@ -31,7 +31,7 @@ import de.danoeh.antennapod.core.util.URLChecker; * Sends download requests to the DownloadService. This class should always be used for starting downloads, * otherwise they won't work correctly. */ -public class DownloadRequester { +public class DownloadRequester implements FeedFileDownloadStatusRequesterInterface { private static final String TAG = "DownloadRequester"; private static final String FEED_DOWNLOADPATH = "cache/"; diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/FeedFileDownloadStatusRequesterInterface.java b/core/src/main/java/de/danoeh/antennapod/core/storage/FeedFileDownloadStatusRequesterInterface.java new file mode 100644 index 000000000..c33b5c137 --- /dev/null +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/FeedFileDownloadStatusRequesterInterface.java @@ -0,0 +1,13 @@ +package de.danoeh.antennapod.core.storage; + +import android.support.annotation.NonNull; + +import de.danoeh.antennapod.core.feed.FeedFile; + +public interface FeedFileDownloadStatusRequesterInterface { + /** + * @return {@code true} if the named feedfile is in the downloads list + */ + boolean isDownloadingFile(@NonNull FeedFile item); + +} diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java index 6d4dc98fd..7753f55dc 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java @@ -1,16 +1,22 @@ package de.danoeh.antennapod.core.storage; +import android.support.annotation.NonNull; + import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameter; import org.junit.runners.Parameterized.Parameters; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Date; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import de.danoeh.antennapod.core.feed.FeedFile; import de.danoeh.antennapod.core.feed.FeedItem; import de.danoeh.antennapod.core.feed.FeedMedia; import de.danoeh.antennapod.core.feed.FeedMother; @@ -30,15 +36,15 @@ public class DBWriterTest { Options optDefault = new Options(); Options optEnqAtFront = new Options().setEnqueueAtFront(true); - return Arrays.asList(new Object[][] { + return Arrays.asList(new Object[][]{ {"case default, i.e., add to the end", - QUEUE_DEFAULT.size(), optDefault , 0, QUEUE_DEFAULT}, + QUEUE_DEFAULT.size(), optDefault, 0, QUEUE_DEFAULT}, {"case default (2nd item)", - QUEUE_DEFAULT.size(), optDefault , 1, QUEUE_DEFAULT}, + QUEUE_DEFAULT.size(), optDefault, 1, QUEUE_DEFAULT}, {"case option enqueue at front", - 0, optEnqAtFront , 0, QUEUE_DEFAULT}, + 0, optEnqAtFront, 0, QUEUE_DEFAULT}, {"case option enqueue at front (2nd item)", - 1, optEnqAtFront , 1, QUEUE_DEFAULT}, + 1, optEnqAtFront, 1, QUEUE_DEFAULT}, {"case empty queue, option default", 0, optDefault, 0, QUEUE_EMPTY}, {"case empty queue, option enqueue at front", @@ -72,7 +78,7 @@ public class DBWriterTest { ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); int posActual = calculator.calcPosition(posAmongAdded, tFI(TFI_TO_ADD_ID), curQueue); - assertEquals(message, posExpected , posActual); + assertEquals(message, posExpected, posActual); } } @@ -109,6 +115,126 @@ public class DBWriterTest { } + @RunWith(Parameterized.class) + public static class ItemEnqueuePositionCalculatorPreserveDownloadOrderTest { + + @Parameters(name = "{index}: case<{0}>, expected:{1}") + public static Iterable<Object[]> data() { + Options optDefault = new Options(); + Options optEnqAtFront = new Options().setEnqueueAtFront(true); + + return Arrays.asList(new Object[][] { + {"download order test, enqueue default", + QUEUE_DEFAULT.size(), QUEUE_DEFAULT.size() + 1, + QUEUE_DEFAULT.size() + 2, QUEUE_DEFAULT.size() + 3, + optDefault, QUEUE_DEFAULT}, + {"download order test, enqueue at front", + 0, 1, + 2, 3, + optEnqAtFront, QUEUE_DEFAULT}, + }); + } + + @Parameter + public String message; + + @Parameter(1) + public int pos101Expected; + + @Parameter(2) + public int pos102Expected; + + // 2XX are for testing bulk insertion cases + @Parameter(3) + public int pos201Expected; + + @Parameter(4) + public int pos202Expected; + + @Parameter(5) + public Options options; + + @Parameter(6) + public List<FeedItem> queueInitial; + + @Test + public void testQueueOrderWhenDownloading2Items() { + + // Setup class under test + // + ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); + MockDownloadRequester mockDownloadRequester = new MockDownloadRequester(); + calculator.requester = mockDownloadRequester; + + // Setup initial data + // A shallow copy, as the test code will manipulate the queue + List<FeedItem> queue = new ArrayList<>(queueInitial); + + + // Test body + + // User clicks download on feed item 101 + FeedItem tFI101 = tFI_isDownloading(101, mockDownloadRequester); + + int pos101Actual = calculator.calcPosition(0, tFI101, queue); + queue.add(pos101Actual, tFI101); + assertEquals(message + " (1st download)", + pos101Expected, pos101Actual); + + // Then user clicks download on feed item 102 + FeedItem tFI102 = tFI_isDownloading(102, mockDownloadRequester); + int pos102Actual = calculator.calcPosition(0, tFI102, queue); + queue.add(pos102Actual, tFI102); + assertEquals(message + " (2nd download, it should preserve order of download)", + pos102Expected, pos102Actual); + + // Items 201 and 202 are added as part of a single DBWriter.addQueueItem() calls + + FeedItem tFI201 = tFI_isDownloading(201, mockDownloadRequester); + int pos201Actual = calculator.calcPosition(0, tFI201, queue); + queue.add(pos201Actual, tFI201); + assertEquals(message + " (bulk insertion, 1st item)", pos201Expected, pos201Actual); + + FeedItem tFI202 = tFI_isDownloading(202, mockDownloadRequester); + int pos202Actual = calculator.calcPosition(1, tFI202, queue); + queue.add(pos202Actual, tFI202); + assertEquals(message + " (bulk insertion, 2nd item)", pos202Expected, pos202Actual); + + // TODO: simulate download failure cases. + } + + + private static FeedItem tFI_isDownloading(int id, MockDownloadRequester requester) { + FeedItem item = tFI(id); + FeedMedia media = + new FeedMedia(item, "http://download.url.net/" + id + , 100000 + id, "audio/mp3"); + media.setId(item.getId()); + item.setMedia(media); + + requester.mockDownloadingFile(media, true); + + return item; + } + + private static class MockDownloadRequester implements FeedFileDownloadStatusRequesterInterface { + + private Map<Long, Boolean> downloadingByIds = new HashMap<>(); + + @Override + public synchronized boolean isDownloadingFile(@NonNull FeedFile item) { + return downloadingByIds.getOrDefault(item.getId(), false); + } + + // All other parent methods should not be called + + public void mockDownloadingFile(FeedFile item, boolean isDownloading) { + downloadingByIds.put(item.getId(), isDownloading); + } + } + } + + // Common helpers: // - common queue (of items) for tests // - construct FeedItems for tests @@ -125,6 +251,7 @@ public class DBWriterTest { static FeedItem tFI(int id, int position) { FeedItem item = tFINoMedia(id); FeedMedia media = new FeedMedia(item, "download_url", 1234567, "audio/mpeg"); + media.setId(item.getId()); item.setMedia(media); if (position >= 0) { @@ -135,11 +262,10 @@ public class DBWriterTest { } static FeedItem tFINoMedia(int id) { - FeedItem item = new FeedItem(0, "Item" + id, "ItemId" + id, "url", + FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url", new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); return item; } - } } |