From ba27ec6b31314bbcfebf55e38a913d5b10757e83 Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 18 May 2018 14:47:37 -0700 Subject: [PATCH 01/28] refactor - DBWriter.addQueueItem() : refactor enqueue position calculation to be a unit-testable component (static inner class) --- .../antennapod/core/storage/DBWriter.java | 63 +++++++++++++++--- .../antennapod/core/feed/FeedMother.java | 2 +- .../antennapod/core/storage/DBWriterTest.java | 65 +++++++++++++++++++ 3 files changed, 120 insertions(+), 10 deletions(-) create mode 100644 core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java 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 8f0626c5c..ed086bb53 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 @@ -6,6 +6,7 @@ import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import de.danoeh.antennapod.core.event.DownloadLogEvent; import de.danoeh.antennapod.core.event.FeedListUpdateEvent; @@ -324,21 +325,21 @@ public class DBWriter { LongList markAsUnplayedIds = new LongList(); List events = new ArrayList<>(); List updatedItems = new ArrayList<>(); + ItemEnqueuePositionCalculator positionCalculator = + new ItemEnqueuePositionCalculator( + new ItemEnqueuePositionCalculator.Options() + .setEnqueueAtFront(UserPreferences.enqueueAtFront())); + for (int i = 0; i < itemIds.length; i++) { if (!itemListContains(queue, itemIds[i])) { final FeedItem item = DBReader.getFeedItem(itemIds[i]); if (item != null) { - // add item to either front ot back of queue - boolean addToFront = UserPreferences.enqueueAtFront(); - if (addToFront) { - queue.add(i, item); - events.add(QueueEvent.added(item, i)); - } else { - queue.add(item); - events.add(QueueEvent.added(item, queue.size() - 1)); - } + int insertPosition = positionCalculator.calcPosition(i, item, queue); + queue.add(insertPosition, item); + events.add(QueueEvent.added(item, insertPosition)); + item.addTag(FeedItem.TAG_QUEUE); updatedItems.add(item); queueModified = true; @@ -395,6 +396,50 @@ public class DBWriter { events.add(QueueEvent.sorted(queue)); } + @VisibleForTesting + static class ItemEnqueuePositionCalculator { + + public static class Options { + private boolean enqueueAtFront = false; + + public boolean isEnqueueAtFront() { + return enqueueAtFront; + } + + public Options setEnqueueAtFront(boolean enqueueAtFront) { + this.enqueueAtFront = enqueueAtFront; + return this; + } + } + + private final @NonNull Options options; + + public ItemEnqueuePositionCalculator(@NonNull Options options) { + this.options = options; + } + + /** + * + * @param positionAmongToAdd Typically, the callers has a list of items to be inserted to + * the queue. This parameter indicates the position (0-based) of + * the item among the one to inserted. E.g., it is needed for + * enqueue at front option. + * + * @param item the item to be inserted + * @param curQueue the queue to which the item is to be inserted + * @return the position (0-based) the item should be inserted to the named queu + */ + public int calcPosition(int positionAmongToAdd, FeedItem item, List curQueue) { + if (options.isEnqueueAtFront()) { + // 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; + } else { + return curQueue.size(); + } + } + } + /** * Removes all FeedItem objects from the queue. */ diff --git a/core/src/test/java/de/danoeh/antennapod/core/feed/FeedMother.java b/core/src/test/java/de/danoeh/antennapod/core/feed/FeedMother.java index f46797d28..991495a3f 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/feed/FeedMother.java +++ b/core/src/test/java/de/danoeh/antennapod/core/feed/FeedMother.java @@ -1,6 +1,6 @@ package de.danoeh.antennapod.core.feed; -class FeedMother { +public class FeedMother { public static final String IMAGE_URL = "http://example.com/image"; public static Feed anyFeed() { 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 new file mode 100644 index 000000000..0d494534c --- /dev/null +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java @@ -0,0 +1,65 @@ +package de.danoeh.antennapod.core.storage; + +import org.junit.Test; + +import java.util.Arrays; +import java.util.Date; +import java.util.List; + +import de.danoeh.antennapod.core.feed.FeedItem; +import de.danoeh.antennapod.core.feed.FeedMother; +import de.danoeh.antennapod.core.storage.DBWriter.ItemEnqueuePositionCalculator; +import de.danoeh.antennapod.core.storage.DBWriter.ItemEnqueuePositionCalculator.Options; + +import static org.junit.Assert.assertEquals; + +public class DBWriterTest { + + public static class ItemEnqueuePositionCalculatorTest { + + @Test + public void testEnqueueDefault() { + + ItemEnqueuePositionCalculator calculator = + new ItemEnqueuePositionCalculator(new Options()); + + List curQueue = tQueue(); + + int posActual1 = calculator.calcPosition(0, tFI(101), curQueue); + assertEquals("case default, i.e., add to the end", curQueue.size(), posActual1); + + int posActual2 = calculator.calcPosition(1, tFI(102), curQueue); + assertEquals("case default (2nd item)", curQueue.size(), posActual2); + + } + + @Test + public void testEnqueueAtFront() { + + ItemEnqueuePositionCalculator calculator = + new ItemEnqueuePositionCalculator(new Options() + .setEnqueueAtFront(true)); + + List curQueue = tQueue(); + + int posActual1 = calculator.calcPosition(0, tFI(101), curQueue); + assertEquals("case option enqueue at front", 0, posActual1); + + int posActual2 = calculator.calcPosition(1, tFI(102), curQueue); + assertEquals("case option enqueue at front (2nd item)", 1, posActual2); + + } + + private static List tQueue() { + return Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14)); + } + + private static FeedItem tFI(int id) { + FeedItem item = new FeedItem(0, "Item" + id, "ItemId" + id, "url", + new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); + return item; + } + + } + +} \ No newline at end of file From bfde3c731514f1a761710d634c39a80ad75bdf10 Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 18 May 2018 15:20:21 -0700 Subject: [PATCH 02/28] refactor - DBWriterTest: parametrize the set of tests --- .../antennapod/core/storage/DBWriterTest.java | 64 ++++++++++--------- 1 file changed, 34 insertions(+), 30 deletions(-) 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 0d494534c..d6ccbba3a 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,6 +1,10 @@ package de.danoeh.antennapod.core.storage; 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.Arrays; import java.util.Date; @@ -15,43 +19,43 @@ import static org.junit.Assert.assertEquals; public class DBWriterTest { + @RunWith(Parameterized.class) public static class ItemEnqueuePositionCalculatorTest { - @Test - public void testEnqueueDefault() { - - ItemEnqueuePositionCalculator calculator = - new ItemEnqueuePositionCalculator(new Options()); - - List curQueue = tQueue(); - - int posActual1 = calculator.calcPosition(0, tFI(101), curQueue); - assertEquals("case default, i.e., add to the end", curQueue.size(), posActual1); - - int posActual2 = calculator.calcPosition(1, tFI(102), curQueue); - assertEquals("case default (2nd item)", curQueue.size(), posActual2); + @Parameters(name = "{index}: case<{0}>, expected:{1}") + public static Iterable data() { + Options optDefault = new Options(); + Options optEnqAtFront = new Options().setEnqueueAtFront(true); + return Arrays.asList(new Object[][] { + {"case default, i.e., add to the end", QUEUE_DEFAULT.size(), optDefault , 0}, + {"case default (2nd item)", QUEUE_DEFAULT.size(), optDefault , 1}, + {"case option enqueue at front", 0, optEnqAtFront , 0}, + {"case option enqueue at front (2nd item)", 1, optEnqAtFront , 1} + }); } + private static final List QUEUE_DEFAULT = Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14)); + + @Parameter + public String message; + + @Parameter(1) + public int posExpected; + + @Parameter(2) + public Options options; + + @Parameter(3) + public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted. + + @Test - public void testEnqueueAtFront() { + public void test() { + ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); - ItemEnqueuePositionCalculator calculator = - new ItemEnqueuePositionCalculator(new Options() - .setEnqueueAtFront(true)); - - List curQueue = tQueue(); - - int posActual1 = calculator.calcPosition(0, tFI(101), curQueue); - assertEquals("case option enqueue at front", 0, posActual1); - - int posActual2 = calculator.calcPosition(1, tFI(102), curQueue); - assertEquals("case option enqueue at front (2nd item)", 1, posActual2); - - } - - private static List tQueue() { - return Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14)); + int posActual = calculator.calcPosition(posAmongAdded, tFI(101), QUEUE_DEFAULT); + assertEquals(message, posExpected , posActual); } private static FeedItem tFI(int id) { From 30f104f40b6ac9de0d0095c2723ec893215cdd15 Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 18 May 2018 20:44:39 -0700 Subject: [PATCH 03/28] #2652 (part of): The in-progress podcast at the front of the queue should remain at the front. --- .../antennapod/core/storage/DBWriter.java | 27 ++++++-- .../antennapod/core/storage/DBWriterTest.java | 66 +++++++++++++++++-- 2 files changed, 82 insertions(+), 11 deletions(-) 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 ed086bb53..872ed973d 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 @@ -328,7 +328,9 @@ public class DBWriter { ItemEnqueuePositionCalculator positionCalculator = new ItemEnqueuePositionCalculator( new ItemEnqueuePositionCalculator.Options() - .setEnqueueAtFront(UserPreferences.enqueueAtFront())); + .setEnqueueAtFront(UserPreferences.enqueueAtFront()) + .setKeepInProgressAtFront(true) // TODO: to expose with preference + ); for (int i = 0; i < itemIds.length; i++) { if (!itemListContains(queue, itemIds[i])) { @@ -401,6 +403,7 @@ public class DBWriter { public static class Options { private boolean enqueueAtFront = false; + private boolean keepInProgressAtFront = false; public boolean isEnqueueAtFront() { return enqueueAtFront; @@ -410,6 +413,15 @@ public class DBWriter { this.enqueueAtFront = enqueueAtFront; return this; } + + public boolean isKeepInProgressAtFront() { + return keepInProgressAtFront; + } + + public Options setKeepInProgressAtFront(boolean keepInProgressAtFront) { + this.keepInProgressAtFront = keepInProgressAtFront; + return this; + } } private final @NonNull Options options; @@ -431,9 +443,16 @@ public class DBWriter { */ public int calcPosition(int positionAmongToAdd, FeedItem item, List curQueue) { if (options.isEnqueueAtFront()) { - // 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; + if (options.isKeepInProgressAtFront() && + 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 + } 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; + } } else { return curQueue.size(); } 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 d6ccbba3a..c1983ec4f 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 @@ -11,6 +11,7 @@ import java.util.Date; import java.util.List; import de.danoeh.antennapod.core.feed.FeedItem; +import de.danoeh.antennapod.core.feed.FeedMedia; import de.danoeh.antennapod.core.feed.FeedMother; import de.danoeh.antennapod.core.storage.DBWriter.ItemEnqueuePositionCalculator; import de.danoeh.antennapod.core.storage.DBWriter.ItemEnqueuePositionCalculator.Options; @@ -26,17 +27,50 @@ public class DBWriterTest { public static Iterable data() { Options optDefault = new Options(); Options optEnqAtFront = new Options().setEnqueueAtFront(true); + Options optKeepInProgressAtFront = + new Options().setEnqueueAtFront(true).setKeepInProgressAtFront(true); + // edge case: keep in progress without enabling enqueue at front is meaningless + Options optKeepInProgressAtFrontWithNoEnqueueAtFront = + new Options().setKeepInProgressAtFront(true); + return Arrays.asList(new Object[][] { - {"case default, i.e., add to the end", QUEUE_DEFAULT.size(), optDefault , 0}, - {"case default (2nd item)", QUEUE_DEFAULT.size(), optDefault , 1}, - {"case option enqueue at front", 0, optEnqAtFront , 0}, - {"case option enqueue at front (2nd item)", 1, optEnqAtFront , 1} + {"case default, i.e., add to the end", + QUEUE_DEFAULT.size(), optDefault , 0, QUEUE_DEFAULT}, + {"case default (2nd item)", + QUEUE_DEFAULT.size(), optDefault , 1, QUEUE_DEFAULT}, + {"case option enqueue at front", + 0, optEnqAtFront , 0, QUEUE_DEFAULT}, + {"case option enqueue at front (2nd item)", + 1, optEnqAtFront , 1, QUEUE_DEFAULT}, + {"case option keep in progress at front", + 1, optKeepInProgressAtFront , 0, QUEUE_FRONT_IN_PROGRESS}, + {"case option keep in progress at front (2nd item)", + 2, optKeepInProgressAtFront , 1, QUEUE_FRONT_IN_PROGRESS}, + {"case option keep in progress at front, front item not in progress", + 0, optKeepInProgressAtFront , 0, QUEUE_DEFAULT}, + {"case option keep in progress at front, front item no media at all", + 0, optKeepInProgressAtFront , 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception + {"case option keep in progress at front, but enqueue at front is disabled", + QUEUE_FRONT_IN_PROGRESS.size(), optKeepInProgressAtFrontWithNoEnqueueAtFront , 0, QUEUE_FRONT_IN_PROGRESS}, + {"case empty queue, option default", + 0, optDefault, 0, QUEUE_EMPTY}, + {"case empty queue, option enqueue at front", + 0, optEnqAtFront, 0, QUEUE_EMPTY}, + {"case empty queue, option keep in progress at front", + 0, optKeepInProgressAtFront, 0, QUEUE_EMPTY}, + }); } + private static final List QUEUE_EMPTY = Arrays.asList(); + private static final List QUEUE_DEFAULT = Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14)); + private static final List QUEUE_FRONT_IN_PROGRESS = Arrays.asList(tFI(11, 60000), tFI(12), tFI(13)); + + private static final List QUEUE_FRONT_NO_MEDIA = Arrays.asList(tFINoMedia(11), tFI(12), tFI(13)); + @Parameter public String message; @@ -49,21 +83,39 @@ public class DBWriterTest { @Parameter(3) public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted. + @Parameter(4) + public List curQueue; + @Test public void test() { ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); - int posActual = calculator.calcPosition(posAmongAdded, tFI(101), QUEUE_DEFAULT); + int posActual = calculator.calcPosition(posAmongAdded, tFI(101), curQueue); assertEquals(message, posExpected , posActual); } private static FeedItem tFI(int id) { + return tFI(id, -1); + } + + private static FeedItem tFI(int id, int position) { + FeedItem item = tFINoMedia(id); + FeedMedia media = new FeedMedia(item, "download_url", 1234567, "audio/mpeg"); + item.setMedia(media); + + if (position >= 0) { + media.setPosition(position); + } + + return item; + } + + private static FeedItem tFINoMedia(int id) { FeedItem item = new FeedItem(0, "Item" + id, "ItemId" + id, "url", new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); return item; } - } -} \ No newline at end of file +} From 17e61335db73dd36473693dfe941587039302180 Mon Sep 17 00:00:00 2001 From: orionlee Date: Sat, 19 May 2018 12:44:35 -0700 Subject: [PATCH 04/28] #2652 (part of): Expose keep in-progress at front as a preference (in Playback > Queue section) --- .../preferences/PlaybackPreferencesFragment.java | 14 ++++++++++++++ app/src/main/res/xml/preferences_playback.xml | 6 ++++++ .../core/preferences/UserPreferences.java | 13 ++++++++++++- .../danoeh/antennapod/core/storage/DBWriter.java | 2 +- core/src/main/res/values/strings.xml | 2 ++ 5 files changed, 35 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/de/danoeh/antennapod/fragment/preferences/PlaybackPreferencesFragment.java b/app/src/main/java/de/danoeh/antennapod/fragment/preferences/PlaybackPreferencesFragment.java index 1795dfc29..64ac1b8ed 100644 --- a/app/src/main/java/de/danoeh/antennapod/fragment/preferences/PlaybackPreferencesFragment.java +++ b/app/src/main/java/de/danoeh/antennapod/fragment/preferences/PlaybackPreferencesFragment.java @@ -63,6 +63,20 @@ public class PlaybackPreferencesFragment extends PreferenceFragmentCompat { behaviour.setEntries(R.array.video_background_behavior_options_without_pip); behaviour.setEntryValues(R.array.video_background_behavior_values_without_pip); } + + findPreference(UserPreferences.PREF_QUEUE_ADD_TO_FRONT).setOnPreferenceChangeListener( + (preference, newValue) -> { + if (newValue instanceof Boolean) { + boolean enableKeepInProgressAtFront = ((Boolean) newValue); + checkKeepInProgressAtFrontItemVisibility(enableKeepInProgressAtFront); + } + return true; + }); + checkKeepInProgressAtFrontItemVisibility(UserPreferences.enqueueAtFront()); + } + + private void checkKeepInProgressAtFrontItemVisibility(boolean enabled) { + findPreference(UserPreferences.PREF_QUEUE_KEEP_IN_PROGESS_AT_FRONT).setEnabled(enabled); } private void buildSmartMarkAsPlayedPreference() { diff --git a/app/src/main/res/xml/preferences_playback.xml b/app/src/main/res/xml/preferences_playback.xml index 2334e1b1c..b743bdbaf 100644 --- a/app/src/main/res/xml/preferences_playback.xml +++ b/app/src/main/res/xml/preferences_playback.xml @@ -96,6 +96,12 @@ android:key="prefQueueAddToFront" android:summary="@string/pref_queueAddToFront_sum" android:title="@string/pref_queueAddToFront_title"/> + Android versions before 4.1 do not support expanded notifications. Add new episodes to the front of the queue. Enqueue at Front + Keep In-progress Episode at Front + If the episode at front is in-progress, i.e., you have listened to part of it, keep it at the front of the queue. Disabled Image Cache Size Size of the disk cache for images. From 0973efa9436455288acdae870d43ee7bd177f56b Mon Sep 17 00:00:00 2001 From: orionlee Date: Wed, 23 May 2018 14:22:13 -0700 Subject: [PATCH 05/28] refactor test - break ItemEnqueuePositionCalculatorTest to be more modular to prepare for testing more complex enqueue options. --- .../antennapod/core/storage/DBWriterTest.java | 154 ++++++++++-------- 1 file changed, 89 insertions(+), 65 deletions(-) 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 c1983ec4f..6d4dc98fd 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 @@ -7,6 +7,7 @@ import org.junit.runners.Parameterized.Parameter; import org.junit.runners.Parameterized.Parameters; import java.util.Arrays; +import java.util.Collections; import java.util.Date; import java.util.List; @@ -20,86 +21,108 @@ import static org.junit.Assert.assertEquals; public class DBWriterTest { - @RunWith(Parameterized.class) public static class ItemEnqueuePositionCalculatorTest { - @Parameters(name = "{index}: case<{0}>, expected:{1}") - public static Iterable data() { - Options optDefault = new Options(); - Options optEnqAtFront = new Options().setEnqueueAtFront(true); - Options optKeepInProgressAtFront = - new Options().setEnqueueAtFront(true).setKeepInProgressAtFront(true); - // edge case: keep in progress without enabling enqueue at front is meaningless - Options optKeepInProgressAtFrontWithNoEnqueueAtFront = - new Options().setKeepInProgressAtFront(true); + @RunWith(Parameterized.class) + public static class IEPCBasicTest { + @Parameters(name = "{index}: case<{0}>, expected:{1}") + public static Iterable data() { + Options optDefault = new Options(); + Options optEnqAtFront = new Options().setEnqueueAtFront(true); + + return Arrays.asList(new Object[][] { + {"case default, i.e., add to the end", + QUEUE_DEFAULT.size(), optDefault , 0, QUEUE_DEFAULT}, + {"case default (2nd item)", + QUEUE_DEFAULT.size(), optDefault , 1, QUEUE_DEFAULT}, + {"case option enqueue at front", + 0, optEnqAtFront , 0, QUEUE_DEFAULT}, + {"case option enqueue at front (2nd item)", + 1, optEnqAtFront , 1, QUEUE_DEFAULT}, + {"case empty queue, option default", + 0, optDefault, 0, QUEUE_EMPTY}, + {"case empty queue, option enqueue at front", + 0, optEnqAtFront, 0, QUEUE_EMPTY}, + }); + } + + @Parameter + public String message; + + @Parameter(1) + public int posExpected; + + @Parameter(2) + public Options options; + + @Parameter(3) + public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted. + + @Parameter(4) + public List curQueue; - return Arrays.asList(new Object[][] { - {"case default, i.e., add to the end", - QUEUE_DEFAULT.size(), optDefault , 0, QUEUE_DEFAULT}, - {"case default (2nd item)", - QUEUE_DEFAULT.size(), optDefault , 1, QUEUE_DEFAULT}, - {"case option enqueue at front", - 0, optEnqAtFront , 0, QUEUE_DEFAULT}, - {"case option enqueue at front (2nd item)", - 1, optEnqAtFront , 1, QUEUE_DEFAULT}, - {"case option keep in progress at front", - 1, optKeepInProgressAtFront , 0, QUEUE_FRONT_IN_PROGRESS}, - {"case option keep in progress at front (2nd item)", - 2, optKeepInProgressAtFront , 1, QUEUE_FRONT_IN_PROGRESS}, - {"case option keep in progress at front, front item not in progress", - 0, optKeepInProgressAtFront , 0, QUEUE_DEFAULT}, - {"case option keep in progress at front, front item no media at all", - 0, optKeepInProgressAtFront , 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception - {"case option keep in progress at front, but enqueue at front is disabled", - QUEUE_FRONT_IN_PROGRESS.size(), optKeepInProgressAtFrontWithNoEnqueueAtFront , 0, QUEUE_FRONT_IN_PROGRESS}, - {"case empty queue, option default", - 0, optDefault, 0, QUEUE_EMPTY}, - {"case empty queue, option enqueue at front", - 0, optEnqAtFront, 0, QUEUE_EMPTY}, - {"case empty queue, option keep in progress at front", - 0, optKeepInProgressAtFront, 0, QUEUE_EMPTY}, + public static final int TFI_TO_ADD_ID = 101; + + /** + * Add a FeedItem with ID {@link #TFI_TO_ADD_ID} with the setup + */ + @Test + public void test() { + ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); + + int posActual = calculator.calcPosition(posAmongAdded, tFI(TFI_TO_ADD_ID), curQueue); + assertEquals(message, posExpected , posActual); + } - }); } - private static final List QUEUE_EMPTY = Arrays.asList(); + @RunWith(Parameterized.class) + public static class IEPCKeepInProgressAtFrontTest extends IEPCBasicTest { + @Parameters(name = "{index}: case<{0}>, expected:{1}") + public static Iterable data() { + Options optKeepInProgressAtFront = + new Options().setEnqueueAtFront(true).setKeepInProgressAtFront(true); + // edge case: keep in progress without enabling enqueue at front is meaningless + Options optKeepInProgressAtFrontWithNoEnqueueAtFront = + new Options().setKeepInProgressAtFront(true); - private static final List QUEUE_DEFAULT = Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14)); + return Arrays.asList(new Object[][]{ + {"case option keep in progress at front", + 1, optKeepInProgressAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, + {"case option keep in progress at front (2nd item)", + 2, optKeepInProgressAtFront, 1, QUEUE_FRONT_IN_PROGRESS}, + {"case option keep in progress at front, front item not in progress", + 0, optKeepInProgressAtFront, 0, QUEUE_DEFAULT}, + {"case option keep in progress at front, front item no media at all", + 0, optKeepInProgressAtFront, 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception + {"case option keep in progress at front, but enqueue at front is disabled", + QUEUE_FRONT_IN_PROGRESS.size(), optKeepInProgressAtFrontWithNoEnqueueAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, + {"case empty queue, option keep in progress at front", + 0, optKeepInProgressAtFront, 0, QUEUE_EMPTY}, + }); + } - private static final List QUEUE_FRONT_IN_PROGRESS = Arrays.asList(tFI(11, 60000), tFI(12), tFI(13)); + private static final List QUEUE_FRONT_IN_PROGRESS = Arrays.asList(tFI(11, 60000), tFI(12), tFI(13)); - private static final List QUEUE_FRONT_NO_MEDIA = Arrays.asList(tFINoMedia(11), tFI(12), tFI(13)); + private static final List QUEUE_FRONT_NO_MEDIA = Arrays.asList(tFINoMedia(11), tFI(12), tFI(13)); - @Parameter - public String message; - - @Parameter(1) - public int posExpected; - - @Parameter(2) - public Options options; - - @Parameter(3) - public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted. - - @Parameter(4) - public List curQueue; - - - @Test - public void test() { - ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); - - int posActual = calculator.calcPosition(posAmongAdded, tFI(101), curQueue); - assertEquals(message, posExpected , posActual); } - private static FeedItem tFI(int id) { + // Common helpers: + // - common queue (of items) for tests + // - construct FeedItems for tests + + static final List QUEUE_EMPTY = Collections.unmodifiableList(Arrays.asList()); + + static final List QUEUE_DEFAULT = Collections.unmodifiableList(Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14))); + + + static FeedItem tFI(int id) { return tFI(id, -1); } - private static FeedItem tFI(int id, int position) { + static FeedItem tFI(int id, int position) { FeedItem item = tFINoMedia(id); FeedMedia media = new FeedMedia(item, "download_url", 1234567, "audio/mpeg"); item.setMedia(media); @@ -111,11 +134,12 @@ public class DBWriterTest { return item; } - private static FeedItem tFINoMedia(int id) { + static FeedItem tFINoMedia(int id) { FeedItem item = new FeedItem(0, "Item" + id, "ItemId" + id, "url", new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); return item; } + } } From 97905e5ed4afc273e6e4165682aa820e50ac05da Mon Sep 17 00:00:00 2001 From: orionlee Date: Wed, 23 May 2018 15:37:03 -0700 Subject: [PATCH 06/28] #2448: make podcast episode enqueue position respect download start order --- .../handler/MediaDownloadedHandler.java | 13 +- .../antennapod/core/storage/DBTasks.java | 4 + .../antennapod/core/storage/DBWriter.java | 40 ++++- .../core/storage/DownloadRequester.java | 2 +- ...dFileDownloadStatusRequesterInterface.java | 13 ++ .../antennapod/core/storage/DBWriterTest.java | 142 +++++++++++++++++- 6 files changed, 193 insertions(+), 21 deletions(-) create mode 100644 core/src/main/java/de/danoeh/antennapod/core/storage/FeedFileDownloadStatusRequesterInterface.java 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 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 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 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 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 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 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; } - } } From fb824b541d4c8c63d351f968e961f2696c5610d2 Mon Sep 17 00:00:00 2001 From: orionlee Date: Wed, 23 May 2018 16:56:46 -0700 Subject: [PATCH 07/28] Test cases readability: change expected format from position to the actual queue (list of IDs), to make the test case more readable. --- .../antennapod/core/storage/DBWriterTest.java | 123 +++++++++++++----- 1 file changed, 91 insertions(+), 32 deletions(-) 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 7753f55dc..afdfc323d 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 @@ -15,6 +15,7 @@ import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import de.danoeh.antennapod.core.feed.FeedFile; import de.danoeh.antennapod.core.feed.FeedItem; @@ -38,17 +39,23 @@ public class DBWriterTest { return Arrays.asList(new Object[][]{ {"case default, i.e., add to the end", - QUEUE_DEFAULT.size(), optDefault, 0, QUEUE_DEFAULT}, + concat(QUEUE_DEFAULT_IDS, TFI_ID), + optDefault, 0, QUEUE_DEFAULT}, {"case default (2nd item)", - QUEUE_DEFAULT.size(), optDefault, 1, QUEUE_DEFAULT}, + concat(QUEUE_DEFAULT_IDS, TFI_ID), + optDefault, 1, QUEUE_DEFAULT}, {"case option enqueue at front", - 0, optEnqAtFront, 0, QUEUE_DEFAULT}, + concat(TFI_ID, QUEUE_DEFAULT_IDS), + optEnqAtFront, 0, QUEUE_DEFAULT}, {"case option enqueue at front (2nd item)", - 1, optEnqAtFront, 1, QUEUE_DEFAULT}, + list(11L, TFI_ID, 12L, 13L, 14L), + optEnqAtFront, 1, QUEUE_DEFAULT}, {"case empty queue, option default", - 0, optDefault, 0, QUEUE_EMPTY}, + list(TFI_ID), + optDefault, 0, QUEUE_EMPTY}, {"case empty queue, option enqueue at front", - 0, optEnqAtFront, 0, QUEUE_EMPTY}, + list(TFI_ID), + optEnqAtFront, 0, QUEUE_EMPTY}, }); } @@ -56,7 +63,7 @@ public class DBWriterTest { public String message; @Parameter(1) - public int posExpected; + public List idsExpected; @Parameter(2) public Options options; @@ -68,17 +75,22 @@ public class DBWriterTest { public List curQueue; - public static final int TFI_TO_ADD_ID = 101; + public static final long TFI_ID = 101; /** - * Add a FeedItem with ID {@link #TFI_TO_ADD_ID} with the setup + * Add a FeedItem with ID {@link #TFI_ID} with the setup */ @Test public void test() { ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); - int posActual = calculator.calcPosition(posAmongAdded, tFI(TFI_TO_ADD_ID), curQueue); - assertEquals(message, posExpected, posActual); + // shallow copy to which the test will add items + List queue = new ArrayList<>(curQueue); + + FeedItem tFI = tFI(TFI_ID); + int posActual = calculator.calcPosition(posAmongAdded, tFI, queue); + queue.add(posActual, tFI); + assertEquals(message, idsExpected, toIDs(queue)); } } @@ -95,23 +107,31 @@ public class DBWriterTest { return Arrays.asList(new Object[][]{ {"case option keep in progress at front", - 1, optKeepInProgressAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, + list(11L, TFI_ID, 12L, 13L), + optKeepInProgressAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, {"case option keep in progress at front (2nd item)", - 2, optKeepInProgressAtFront, 1, QUEUE_FRONT_IN_PROGRESS}, + list(11L, 12L, TFI_ID, 13L), + optKeepInProgressAtFront, 1, QUEUE_FRONT_IN_PROGRESS}, {"case option keep in progress at front, front item not in progress", - 0, optKeepInProgressAtFront, 0, QUEUE_DEFAULT}, + concat(TFI_ID, QUEUE_DEFAULT_IDS), + optKeepInProgressAtFront, 0, QUEUE_DEFAULT}, {"case option keep in progress at front, front item no media at all", - 0, optKeepInProgressAtFront, 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception + concat(TFI_ID, QUEUE_FRONT_NO_MEDIA_IDS), + optKeepInProgressAtFront, 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception {"case option keep in progress at front, but enqueue at front is disabled", - QUEUE_FRONT_IN_PROGRESS.size(), optKeepInProgressAtFrontWithNoEnqueueAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, + concat(QUEUE_FRONT_IN_PROGRESS_IDS, TFI_ID), + optKeepInProgressAtFrontWithNoEnqueueAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, {"case empty queue, option keep in progress at front", - 0, optKeepInProgressAtFront, 0, QUEUE_EMPTY}, + list(TFI_ID), + optKeepInProgressAtFront, 0, QUEUE_EMPTY}, }); } private static final List QUEUE_FRONT_IN_PROGRESS = Arrays.asList(tFI(11, 60000), tFI(12), tFI(13)); + private static final List QUEUE_FRONT_IN_PROGRESS_IDS = toIDs(QUEUE_FRONT_IN_PROGRESS); private static final List QUEUE_FRONT_NO_MEDIA = Arrays.asList(tFINoMedia(11), tFI(12), tFI(13)); + private static final List QUEUE_FRONT_NO_MEDIA_IDS = toIDs(QUEUE_FRONT_NO_MEDIA); } @@ -123,14 +143,20 @@ public class DBWriterTest { Options optDefault = new Options(); Options optEnqAtFront = new Options().setEnqueueAtFront(true); + // Attempts to make test more readable by showing the expected list of ids + // (rather than the expected positions) 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, + concat(QUEUE_DEFAULT_IDS, 101L), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L)), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L)), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L, 202L)), optDefault, QUEUE_DEFAULT}, {"download order test, enqueue at front", - 0, 1, - 2, 3, + concat(101L, QUEUE_DEFAULT_IDS), + concat(list(101L, 102L), QUEUE_DEFAULT_IDS), + concat(list(101L, 102L, 201L), QUEUE_DEFAULT_IDS), + concat(list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS), optEnqAtFront, QUEUE_DEFAULT}, }); } @@ -139,17 +165,17 @@ public class DBWriterTest { public String message; @Parameter(1) - public int pos101Expected; + public List idsExpectedAfter101; @Parameter(2) - public int pos102Expected; + public List idsExpectedAfter102; // 2XX are for testing bulk insertion cases @Parameter(3) - public int pos201Expected; + public List idsExpectedAfter201; @Parameter(4) - public int pos202Expected; + public List idsExpectedAfter202; @Parameter(5) public Options options; @@ -179,26 +205,28 @@ public class DBWriterTest { int pos101Actual = calculator.calcPosition(0, tFI101, queue); queue.add(pos101Actual, tFI101); assertEquals(message + " (1st download)", - pos101Expected, pos101Actual); + idsExpectedAfter101, toIDs(queue)); // 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); + idsExpectedAfter102, toIDs(queue)); // 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); + assertEquals(message + " (bulk insertion, 1st item)", + idsExpectedAfter201, toIDs(queue)); 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); + assertEquals(message + " (bulk insertion, 2nd item)", + idsExpectedAfter202, toIDs(queue)); // TODO: simulate download failure cases. } @@ -242,13 +270,14 @@ public class DBWriterTest { static final List QUEUE_EMPTY = Collections.unmodifiableList(Arrays.asList()); static final List QUEUE_DEFAULT = Collections.unmodifiableList(Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14))); + static final List QUEUE_DEFAULT_IDS = QUEUE_DEFAULT.stream().map(fi -> fi.getId()).collect(Collectors.toList()); - static FeedItem tFI(int id) { + static FeedItem tFI(long id) { return tFI(id, -1); } - static FeedItem tFI(int id, int position) { + static FeedItem tFI(long id, int position) { FeedItem item = tFINoMedia(id); FeedMedia media = new FeedMedia(item, "download_url", 1234567, "audio/mpeg"); media.setId(item.getId()); @@ -261,11 +290,41 @@ public class DBWriterTest { return item; } - static FeedItem tFINoMedia(int id) { + static FeedItem tFINoMedia(long id) { FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url", new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); return item; } + + // Collections helpers + + static List concat(T item, List list) { + List res = new ArrayList<>(list); + res.add(0, item); + return res; + } + + static List concat(List list, T item) { + List res = new ArrayList<>(list); + res.add(item); + return res; + } + + static List concat(List list1, List list2) { + List res = new ArrayList<>(list1); + res.addAll(list2); + return res; + } + + public static List list(T... a) { + return Arrays.asList(a); + } + + + static List toIDs(List items) { + return items.stream().map(i->i.getId()).collect(Collectors.toList()); + } + } } From ce5aa26878481a5c07bd611d186ddd735b504d5e Mon Sep 17 00:00:00 2001 From: orionlee Date: Wed, 23 May 2018 22:13:01 -0700 Subject: [PATCH 08/28] refactoring test - factor out common operations of calc position, add to queue and verify result into common helper. --- .../antennapod/core/storage/DBWriterTest.java | 47 ++++++++++--------- 1 file changed, 26 insertions(+), 21 deletions(-) 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 afdfc323d..5603f8778 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 @@ -86,11 +86,10 @@ public class DBWriterTest { // shallow copy to which the test will add items List queue = new ArrayList<>(curQueue); - FeedItem tFI = tFI(TFI_ID); - int posActual = calculator.calcPosition(posAmongAdded, tFI, queue); - queue.add(posActual, tFI); - assertEquals(message, idsExpected, toIDs(queue)); + doAddToQueueAndAssertResult(message, + calculator, posAmongAdded, tFI, queue, + idsExpected); } } @@ -201,32 +200,27 @@ public class DBWriterTest { // 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)", - idsExpectedAfter101, toIDs(queue)); + doAddToQueueAndAssertResult(message + " (1st download)", + calculator, 0, tFI101, queue, + idsExpectedAfter101); // 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)", - idsExpectedAfter102, toIDs(queue)); + doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)", + calculator, 0, tFI102, queue, + idsExpectedAfter102); // 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)", - idsExpectedAfter201, toIDs(queue)); + doAddToQueueAndAssertResult(message + " (bulk insertion, 1st item)", + calculator, 0, tFI201, queue, + idsExpectedAfter201); FeedItem tFI202 = tFI_isDownloading(202, mockDownloadRequester); - int pos202Actual = calculator.calcPosition(1, tFI202, queue); - queue.add(pos202Actual, tFI202); - assertEquals(message + " (bulk insertion, 2nd item)", - idsExpectedAfter202, toIDs(queue)); + doAddToQueueAndAssertResult(message + " (bulk insertion, 2nd item)", + calculator, 0, tFI202, queue, + idsExpectedAfter202); // TODO: simulate download failure cases. } @@ -267,6 +261,17 @@ public class DBWriterTest { // - common queue (of items) for tests // - construct FeedItems for tests + static void doAddToQueueAndAssertResult(String message, + ItemEnqueuePositionCalculator calculator, + int positionAmongAdd, + FeedItem itemToAdd, + List queue, + List idsExpected) { + int posActual = calculator.calcPosition(positionAmongAdd, itemToAdd, queue); + queue.add(posActual, itemToAdd); + assertEquals(message, idsExpected, toIDs(queue)); + } + static final List QUEUE_EMPTY = Collections.unmodifiableList(Arrays.asList()); static final List QUEUE_DEFAULT = Collections.unmodifiableList(Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14))); From 820b0b07939f9997266ea14677639add652e4ea7 Mon Sep 17 00:00:00 2001 From: orionlee Date: Mon, 28 May 2018 13:34:07 -0700 Subject: [PATCH 09/28] test case bug fix: Bulk download 2nd item position should be 1 --- .../java/de/danoeh/antennapod/core/storage/DBWriterTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5603f8778..f0f557f2f 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 @@ -219,7 +219,7 @@ public class DBWriterTest { FeedItem tFI202 = tFI_isDownloading(202, mockDownloadRequester); doAddToQueueAndAssertResult(message + " (bulk insertion, 2nd item)", - calculator, 0, tFI202, queue, + calculator, 1, tFI202, queue, idsExpectedAfter202); // TODO: simulate download failure cases. From fb7fb05b5e3d61975d300369271dba385490838a Mon Sep 17 00:00:00 2001 From: orionlee Date: Mon, 28 May 2018 13:35:34 -0700 Subject: [PATCH 10/28] test case tweak: preserve download order test, fix test case name (remove the incomplete expected from test case name) --- .../java/de/danoeh/antennapod/core/storage/DBWriterTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f0f557f2f..42860c903 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 @@ -137,7 +137,7 @@ public class DBWriterTest { @RunWith(Parameterized.class) public static class ItemEnqueuePositionCalculatorPreserveDownloadOrderTest { - @Parameters(name = "{index}: case<{0}>, expected:{1}") + @Parameters(name = "{index}: case<{0}>") public static Iterable data() { Options optDefault = new Options(); Options optEnqAtFront = new Options().setEnqueueAtFront(true); From 2d1ee52014aa6b171733261a698129f5de3f4036 Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 4 Oct 2019 12:39:56 -0700 Subject: [PATCH 11/28] fix imports post androidX migration --- .../core/storage/FeedFileDownloadStatusRequesterInterface.java | 2 +- .../java/de/danoeh/antennapod/core/storage/DBWriterTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 index c33b5c137..b331fc4d7 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/FeedFileDownloadStatusRequesterInterface.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/FeedFileDownloadStatusRequesterInterface.java @@ -1,6 +1,6 @@ package de.danoeh.antennapod.core.storage; -import android.support.annotation.NonNull; +import androidx.annotation.NonNull; import de.danoeh.antennapod.core.feed.FeedFile; 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 42860c903..46822de81 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,6 +1,6 @@ package de.danoeh.antennapod.core.storage; -import android.support.annotation.NonNull; +import androidx.annotation.NonNull; import org.junit.Test; import org.junit.runner.RunWith; From cd3d20d61338180bfa585fbf8fcc2b13261df709 Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 4 Oct 2019 13:06:29 -0700 Subject: [PATCH 12/28] refactor - move ItemEnqueuePositionCalculator to top-level per review. --- .../antennapod/core/storage/DBWriter.java | 95 ----- .../ItemEnqueuePositionCalculator.java | 109 ++++++ .../antennapod/core/storage/DBWriterTest.java | 335 ------------------ .../ItemEnqueuePositionCalculatorTest.java | 330 +++++++++++++++++ 4 files changed, 439 insertions(+), 430 deletions(-) create mode 100644 core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java delete mode 100644 core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java create mode 100644 core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java 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 1ba58f3d3..57db6123c 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 @@ -398,101 +398,6 @@ public class DBWriter { events.add(QueueEvent.sorted(queue)); } - @VisibleForTesting - static class ItemEnqueuePositionCalculator { - - public static class Options { - private boolean enqueueAtFront = false; - private boolean keepInProgressAtFront = false; - - public boolean isEnqueueAtFront() { - return enqueueAtFront; - } - - public Options setEnqueueAtFront(boolean enqueueAtFront) { - this.enqueueAtFront = enqueueAtFront; - return this; - } - - public boolean isKeepInProgressAtFront() { - return keepInProgressAtFront; - } - - public Options setKeepInProgressAtFront(boolean keepInProgressAtFront) { - this.keepInProgressAtFront = keepInProgressAtFront; - return this; - } - } - - 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; - } - - /** - * - * @param positionAmongToAdd Typically, the callers has a list of items to be inserted to - * the queue. This parameter indicates the position (0-based) of - * the item among the one to inserted. E.g., it is needed for - * enqueue at front option. - * - * @param item the item to be inserted - * @param curQueue the queue to which the item is to be inserted - * @return the position (0-based) the item should be inserted to the named queu - */ - public int calcPosition(int positionAmongToAdd, FeedItem item, List curQueue) { - if (options.isEnqueueAtFront()) { - if (options.isKeepInProgressAtFront() && - curQueue.size() > 0 && - curQueue.get(0).getMedia() != null && - curQueue.get(0).getMedia().isInProgress()) { - // 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 getPositionOf1stNonDownloadingItem(positionAmongToAdd, curQueue); - } - } else { - return curQueue.size(); - } - } - - private int getPositionOf1stNonDownloadingItem(int startPosition, List 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 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; - } - } - } - /** * Removes all FeedItem objects from the queue. */ diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java new file mode 100644 index 000000000..6e7843836 --- /dev/null +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java @@ -0,0 +1,109 @@ +package de.danoeh.antennapod.core.storage; + +import android.content.Context; + +import androidx.annotation.NonNull; +import androidx.annotation.VisibleForTesting; + +import java.util.List; + +import de.danoeh.antennapod.core.feed.FeedItem; + +/** + * @see DBWriter#addQueueItem(Context, boolean, long...) it uses the class to determine + * the positions of the {@link FeedItem} in the queue. + */ +class ItemEnqueuePositionCalculator { + + public static class Options { + private boolean enqueueAtFront = false; + private boolean keepInProgressAtFront = false; + + public boolean isEnqueueAtFront() { + return enqueueAtFront; + } + + public Options setEnqueueAtFront(boolean enqueueAtFront) { + this.enqueueAtFront = enqueueAtFront; + return this; + } + + public boolean isKeepInProgressAtFront() { + return keepInProgressAtFront; + } + + public Options setKeepInProgressAtFront(boolean keepInProgressAtFront) { + this.keepInProgressAtFront = keepInProgressAtFront; + return this; + } + } + + 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; + } + + /** + * + * @param positionAmongToAdd Typically, the callers has a list of items to be inserted to + * the queue. This parameter indicates the position (0-based) of + * the item among the one to inserted. E.g., it is needed for + * enqueue at front option. + * + * @param item the item to be inserted + * @param curQueue the queue to which the item is to be inserted + * @return the position (0-based) the item should be inserted to the named queu + */ + public int calcPosition(int positionAmongToAdd, FeedItem item, List curQueue) { + if (options.isEnqueueAtFront()) { + if (options.isKeepInProgressAtFront() && + curQueue.size() > 0 && + curQueue.get(0).getMedia() != null && + curQueue.get(0).getMedia().isInProgress()) { + // 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 getPositionOf1stNonDownloadingItem(positionAmongToAdd, curQueue); + } + } else { + return curQueue.size(); + } + } + + private int getPositionOf1stNonDownloadingItem(int startPosition, List 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 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/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java deleted file mode 100644 index 46822de81..000000000 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/DBWriterTest.java +++ /dev/null @@ -1,335 +0,0 @@ -package de.danoeh.antennapod.core.storage; - -import androidx.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 java.util.stream.Collectors; - -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; -import de.danoeh.antennapod.core.storage.DBWriter.ItemEnqueuePositionCalculator; -import de.danoeh.antennapod.core.storage.DBWriter.ItemEnqueuePositionCalculator.Options; - -import static org.junit.Assert.assertEquals; - -public class DBWriterTest { - - public static class ItemEnqueuePositionCalculatorTest { - - @RunWith(Parameterized.class) - public static class IEPCBasicTest { - @Parameters(name = "{index}: case<{0}>, expected:{1}") - public static Iterable data() { - Options optDefault = new Options(); - Options optEnqAtFront = new Options().setEnqueueAtFront(true); - - return Arrays.asList(new Object[][]{ - {"case default, i.e., add to the end", - concat(QUEUE_DEFAULT_IDS, TFI_ID), - optDefault, 0, QUEUE_DEFAULT}, - {"case default (2nd item)", - concat(QUEUE_DEFAULT_IDS, TFI_ID), - optDefault, 1, QUEUE_DEFAULT}, - {"case option enqueue at front", - concat(TFI_ID, QUEUE_DEFAULT_IDS), - optEnqAtFront, 0, QUEUE_DEFAULT}, - {"case option enqueue at front (2nd item)", - list(11L, TFI_ID, 12L, 13L, 14L), - optEnqAtFront, 1, QUEUE_DEFAULT}, - {"case empty queue, option default", - list(TFI_ID), - optDefault, 0, QUEUE_EMPTY}, - {"case empty queue, option enqueue at front", - list(TFI_ID), - optEnqAtFront, 0, QUEUE_EMPTY}, - }); - } - - @Parameter - public String message; - - @Parameter(1) - public List idsExpected; - - @Parameter(2) - public Options options; - - @Parameter(3) - public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted. - - @Parameter(4) - public List curQueue; - - - public static final long TFI_ID = 101; - - /** - * Add a FeedItem with ID {@link #TFI_ID} with the setup - */ - @Test - public void test() { - ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); - - // shallow copy to which the test will add items - List queue = new ArrayList<>(curQueue); - FeedItem tFI = tFI(TFI_ID); - doAddToQueueAndAssertResult(message, - calculator, posAmongAdded, tFI, queue, - idsExpected); - } - - } - - @RunWith(Parameterized.class) - public static class IEPCKeepInProgressAtFrontTest extends IEPCBasicTest { - @Parameters(name = "{index}: case<{0}>, expected:{1}") - public static Iterable data() { - Options optKeepInProgressAtFront = - new Options().setEnqueueAtFront(true).setKeepInProgressAtFront(true); - // edge case: keep in progress without enabling enqueue at front is meaningless - Options optKeepInProgressAtFrontWithNoEnqueueAtFront = - new Options().setKeepInProgressAtFront(true); - - return Arrays.asList(new Object[][]{ - {"case option keep in progress at front", - list(11L, TFI_ID, 12L, 13L), - optKeepInProgressAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, - {"case option keep in progress at front (2nd item)", - list(11L, 12L, TFI_ID, 13L), - optKeepInProgressAtFront, 1, QUEUE_FRONT_IN_PROGRESS}, - {"case option keep in progress at front, front item not in progress", - concat(TFI_ID, QUEUE_DEFAULT_IDS), - optKeepInProgressAtFront, 0, QUEUE_DEFAULT}, - {"case option keep in progress at front, front item no media at all", - concat(TFI_ID, QUEUE_FRONT_NO_MEDIA_IDS), - optKeepInProgressAtFront, 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception - {"case option keep in progress at front, but enqueue at front is disabled", - concat(QUEUE_FRONT_IN_PROGRESS_IDS, TFI_ID), - optKeepInProgressAtFrontWithNoEnqueueAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, - {"case empty queue, option keep in progress at front", - list(TFI_ID), - optKeepInProgressAtFront, 0, QUEUE_EMPTY}, - }); - } - - private static final List QUEUE_FRONT_IN_PROGRESS = Arrays.asList(tFI(11, 60000), tFI(12), tFI(13)); - private static final List QUEUE_FRONT_IN_PROGRESS_IDS = toIDs(QUEUE_FRONT_IN_PROGRESS); - - private static final List QUEUE_FRONT_NO_MEDIA = Arrays.asList(tFINoMedia(11), tFI(12), tFI(13)); - private static final List QUEUE_FRONT_NO_MEDIA_IDS = toIDs(QUEUE_FRONT_NO_MEDIA); - - } - - @RunWith(Parameterized.class) - public static class ItemEnqueuePositionCalculatorPreserveDownloadOrderTest { - - @Parameters(name = "{index}: case<{0}>") - public static Iterable data() { - Options optDefault = new Options(); - Options optEnqAtFront = new Options().setEnqueueAtFront(true); - - // Attempts to make test more readable by showing the expected list of ids - // (rather than the expected positions) - return Arrays.asList(new Object[][] { - {"download order test, enqueue default", - concat(QUEUE_DEFAULT_IDS, 101L), - concat(QUEUE_DEFAULT_IDS, list(101L, 102L)), - concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L)), - concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L, 202L)), - optDefault, QUEUE_DEFAULT}, - {"download order test, enqueue at front", - concat(101L, QUEUE_DEFAULT_IDS), - concat(list(101L, 102L), QUEUE_DEFAULT_IDS), - concat(list(101L, 102L, 201L), QUEUE_DEFAULT_IDS), - concat(list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS), - optEnqAtFront, QUEUE_DEFAULT}, - }); - } - - @Parameter - public String message; - - @Parameter(1) - public List idsExpectedAfter101; - - @Parameter(2) - public List idsExpectedAfter102; - - // 2XX are for testing bulk insertion cases - @Parameter(3) - public List idsExpectedAfter201; - - @Parameter(4) - public List idsExpectedAfter202; - - @Parameter(5) - public Options options; - - @Parameter(6) - public List 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 queue = new ArrayList<>(queueInitial); - - - // Test body - - // User clicks download on feed item 101 - FeedItem tFI101 = tFI_isDownloading(101, mockDownloadRequester); - doAddToQueueAndAssertResult(message + " (1st download)", - calculator, 0, tFI101, queue, - idsExpectedAfter101); - - // Then user clicks download on feed item 102 - FeedItem tFI102 = tFI_isDownloading(102, mockDownloadRequester); - doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)", - calculator, 0, tFI102, queue, - idsExpectedAfter102); - - // Items 201 and 202 are added as part of a single DBWriter.addQueueItem() calls - - FeedItem tFI201 = tFI_isDownloading(201, mockDownloadRequester); - doAddToQueueAndAssertResult(message + " (bulk insertion, 1st item)", - calculator, 0, tFI201, queue, - idsExpectedAfter201); - - FeedItem tFI202 = tFI_isDownloading(202, mockDownloadRequester); - doAddToQueueAndAssertResult(message + " (bulk insertion, 2nd item)", - calculator, 1, tFI202, queue, - idsExpectedAfter202); - - // 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 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 - - static void doAddToQueueAndAssertResult(String message, - ItemEnqueuePositionCalculator calculator, - int positionAmongAdd, - FeedItem itemToAdd, - List queue, - List idsExpected) { - int posActual = calculator.calcPosition(positionAmongAdd, itemToAdd, queue); - queue.add(posActual, itemToAdd); - assertEquals(message, idsExpected, toIDs(queue)); - } - - static final List QUEUE_EMPTY = Collections.unmodifiableList(Arrays.asList()); - - static final List QUEUE_DEFAULT = Collections.unmodifiableList(Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14))); - static final List QUEUE_DEFAULT_IDS = QUEUE_DEFAULT.stream().map(fi -> fi.getId()).collect(Collectors.toList()); - - - static FeedItem tFI(long id) { - return tFI(id, -1); - } - - static FeedItem tFI(long 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) { - media.setPosition(position); - } - - return item; - } - - static FeedItem tFINoMedia(long id) { - FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url", - new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); - return item; - } - - // Collections helpers - - static List concat(T item, List list) { - List res = new ArrayList<>(list); - res.add(0, item); - return res; - } - - static List concat(List list, T item) { - List res = new ArrayList<>(list); - res.add(item); - return res; - } - - static List concat(List list1, List list2) { - List res = new ArrayList<>(list1); - res.addAll(list2); - return res; - } - - public static List list(T... a) { - return Arrays.asList(a); - } - - - static List toIDs(List items) { - return items.stream().map(i->i.getId()).collect(Collectors.toList()); - } - - } - -} diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java new file mode 100644 index 000000000..1331df67d --- /dev/null +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -0,0 +1,330 @@ +package de.danoeh.antennapod.core.storage; + +import androidx.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 java.util.stream.Collectors; + +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; +import de.danoeh.antennapod.core.storage.ItemEnqueuePositionCalculator.Options; + +import static org.junit.Assert.assertEquals; + +public class ItemEnqueuePositionCalculatorTest { + + @RunWith(Parameterized.class) + public static class IEPCBasicTest { + @Parameters(name = "{index}: case<{0}>, expected:{1}") + public static Iterable data() { + Options optDefault = new Options(); + Options optEnqAtFront = new Options().setEnqueueAtFront(true); + + return Arrays.asList(new Object[][]{ + {"case default, i.e., add to the end", + concat(QUEUE_DEFAULT_IDS, TFI_ID), + optDefault, 0, QUEUE_DEFAULT}, + {"case default (2nd item)", + concat(QUEUE_DEFAULT_IDS, TFI_ID), + optDefault, 1, QUEUE_DEFAULT}, + {"case option enqueue at front", + concat(TFI_ID, QUEUE_DEFAULT_IDS), + optEnqAtFront, 0, QUEUE_DEFAULT}, + {"case option enqueue at front (2nd item)", + list(11L, TFI_ID, 12L, 13L, 14L), + optEnqAtFront, 1, QUEUE_DEFAULT}, + {"case empty queue, option default", + list(TFI_ID), + optDefault, 0, QUEUE_EMPTY}, + {"case empty queue, option enqueue at front", + list(TFI_ID), + optEnqAtFront, 0, QUEUE_EMPTY}, + }); + } + + @Parameter + public String message; + + @Parameter(1) + public List idsExpected; + + @Parameter(2) + public Options options; + + @Parameter(3) + public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted. + + @Parameter(4) + public List curQueue; + + + public static final long TFI_ID = 101; + + /** + * Add a FeedItem with ID {@link #TFI_ID} with the setup + */ + @Test + public void test() { + ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); + + // shallow copy to which the test will add items + List queue = new ArrayList<>(curQueue); + FeedItem tFI = tFI(TFI_ID); + doAddToQueueAndAssertResult(message, + calculator, posAmongAdded, tFI, queue, + idsExpected); + } + + } + + @RunWith(Parameterized.class) + public static class IEPCKeepInProgressAtFrontTest extends IEPCBasicTest { + @Parameters(name = "{index}: case<{0}>, expected:{1}") + public static Iterable data() { + Options optKeepInProgressAtFront = + new Options().setEnqueueAtFront(true).setKeepInProgressAtFront(true); + // edge case: keep in progress without enabling enqueue at front is meaningless + Options optKeepInProgressAtFrontWithNoEnqueueAtFront = + new Options().setKeepInProgressAtFront(true); + + return Arrays.asList(new Object[][]{ + {"case option keep in progress at front", + list(11L, TFI_ID, 12L, 13L), + optKeepInProgressAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, + {"case option keep in progress at front (2nd item)", + list(11L, 12L, TFI_ID, 13L), + optKeepInProgressAtFront, 1, QUEUE_FRONT_IN_PROGRESS}, + {"case option keep in progress at front, front item not in progress", + concat(TFI_ID, QUEUE_DEFAULT_IDS), + optKeepInProgressAtFront, 0, QUEUE_DEFAULT}, + {"case option keep in progress at front, front item no media at all", + concat(TFI_ID, QUEUE_FRONT_NO_MEDIA_IDS), + optKeepInProgressAtFront, 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception + {"case option keep in progress at front, but enqueue at front is disabled", + concat(QUEUE_FRONT_IN_PROGRESS_IDS, TFI_ID), + optKeepInProgressAtFrontWithNoEnqueueAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, + {"case empty queue, option keep in progress at front", + list(TFI_ID), + optKeepInProgressAtFront, 0, QUEUE_EMPTY}, + }); + } + + private static final List QUEUE_FRONT_IN_PROGRESS = Arrays.asList(tFI(11, 60000), tFI(12), tFI(13)); + private static final List QUEUE_FRONT_IN_PROGRESS_IDS = toIDs(QUEUE_FRONT_IN_PROGRESS); + + private static final List QUEUE_FRONT_NO_MEDIA = Arrays.asList(tFINoMedia(11), tFI(12), tFI(13)); + private static final List QUEUE_FRONT_NO_MEDIA_IDS = toIDs(QUEUE_FRONT_NO_MEDIA); + + } + + @RunWith(Parameterized.class) + public static class ItemEnqueuePositionCalculatorPreserveDownloadOrderTest { + + @Parameters(name = "{index}: case<{0}>") + public static Iterable data() { + Options optDefault = new Options(); + Options optEnqAtFront = new Options().setEnqueueAtFront(true); + + // Attempts to make test more readable by showing the expected list of ids + // (rather than the expected positions) + return Arrays.asList(new Object[][] { + {"download order test, enqueue default", + concat(QUEUE_DEFAULT_IDS, 101L), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L)), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L)), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L, 202L)), + optDefault, QUEUE_DEFAULT}, + {"download order test, enqueue at front", + concat(101L, QUEUE_DEFAULT_IDS), + concat(list(101L, 102L), QUEUE_DEFAULT_IDS), + concat(list(101L, 102L, 201L), QUEUE_DEFAULT_IDS), + concat(list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS), + optEnqAtFront, QUEUE_DEFAULT}, + }); + } + + @Parameter + public String message; + + @Parameter(1) + public List idsExpectedAfter101; + + @Parameter(2) + public List idsExpectedAfter102; + + // 2XX are for testing bulk insertion cases + @Parameter(3) + public List idsExpectedAfter201; + + @Parameter(4) + public List idsExpectedAfter202; + + @Parameter(5) + public Options options; + + @Parameter(6) + public List 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 queue = new ArrayList<>(queueInitial); + + + // Test body + + // User clicks download on feed item 101 + FeedItem tFI101 = tFI_isDownloading(101, mockDownloadRequester); + doAddToQueueAndAssertResult(message + " (1st download)", + calculator, 0, tFI101, queue, + idsExpectedAfter101); + + // Then user clicks download on feed item 102 + FeedItem tFI102 = tFI_isDownloading(102, mockDownloadRequester); + doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)", + calculator, 0, tFI102, queue, + idsExpectedAfter102); + + // Items 201 and 202 are added as part of a single DBWriter.addQueueItem() calls + + FeedItem tFI201 = tFI_isDownloading(201, mockDownloadRequester); + doAddToQueueAndAssertResult(message + " (bulk insertion, 1st item)", + calculator, 0, tFI201, queue, + idsExpectedAfter201); + + FeedItem tFI202 = tFI_isDownloading(202, mockDownloadRequester); + doAddToQueueAndAssertResult(message + " (bulk insertion, 2nd item)", + calculator, 1, tFI202, queue, + idsExpectedAfter202); + + // 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 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 + + static void doAddToQueueAndAssertResult(String message, + ItemEnqueuePositionCalculator calculator, + int positionAmongAdd, + FeedItem itemToAdd, + List queue, + List idsExpected) { + int posActual = calculator.calcPosition(positionAmongAdd, itemToAdd, queue); + queue.add(posActual, itemToAdd); + assertEquals(message, idsExpected, toIDs(queue)); + } + + static final List QUEUE_EMPTY = Collections.unmodifiableList(Arrays.asList()); + + static final List QUEUE_DEFAULT = Collections.unmodifiableList(Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14))); + static final List QUEUE_DEFAULT_IDS = QUEUE_DEFAULT.stream().map(fi -> fi.getId()).collect(Collectors.toList()); + + + static FeedItem tFI(long id) { + return tFI(id, -1); + } + + static FeedItem tFI(long 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) { + media.setPosition(position); + } + + return item; + } + + static FeedItem tFINoMedia(long id) { + FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url", + new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); + return item; + } + + // Collections helpers + + static List concat(T item, List list) { + List res = new ArrayList<>(list); + res.add(0, item); + return res; + } + + static List concat(List list, T item) { + List res = new ArrayList<>(list); + res.add(item); + return res; + } + + static List concat(List list1, List list2) { + List res = new ArrayList<>(list1); + res.addAll(list2); + return res; + } + + static List list(T... a) { + return Arrays.asList(a); + } + + + static List toIDs(List items) { + return items.stream().map(i->i.getId()).collect(Collectors.toList()); + } + +} From 2f82a5d46421a602044b6fcd728e4394a2ad77ea Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 4 Oct 2019 13:13:46 -0700 Subject: [PATCH 13/28] refactor - rename FeedFileDownloadStatusRequesterInterface to a more generic DownloadStateProvider. --- .../danoeh/antennapod/core/storage/DownloadRequester.java | 7 ++++--- ...sRequesterInterface.java => DownloadStateProvider.java} | 5 ++++- .../core/storage/ItemEnqueuePositionCalculator.java | 7 ++----- .../core/storage/ItemEnqueuePositionCalculatorTest.java | 4 ++-- 4 files changed, 12 insertions(+), 11 deletions(-) rename core/src/main/java/de/danoeh/antennapod/core/storage/{FeedFileDownloadStatusRequesterInterface.java => DownloadStateProvider.java} (69%) 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 8d4a197ad..c61abc168 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 @@ -3,12 +3,13 @@ package de.danoeh.antennapod.core.storage; import android.content.Context; import android.content.Intent; import android.os.Bundle; -import androidx.annotation.NonNull; -import androidx.core.content.ContextCompat; import android.text.TextUtils; import android.util.Log; import android.webkit.URLUtil; +import androidx.annotation.NonNull; +import androidx.core.content.ContextCompat; + import org.apache.commons.io.FilenameUtils; import java.io.File; @@ -31,7 +32,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 implements FeedFileDownloadStatusRequesterInterface { +public class DownloadRequester implements DownloadStateProvider { 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/DownloadStateProvider.java similarity index 69% rename from core/src/main/java/de/danoeh/antennapod/core/storage/FeedFileDownloadStatusRequesterInterface.java rename to core/src/main/java/de/danoeh/antennapod/core/storage/DownloadStateProvider.java index b331fc4d7..ece40353f 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/FeedFileDownloadStatusRequesterInterface.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/DownloadStateProvider.java @@ -4,7 +4,10 @@ import androidx.annotation.NonNull; import de.danoeh.antennapod.core.feed.FeedFile; -public interface FeedFileDownloadStatusRequesterInterface { +/** + * Allow callers to query the states of downloads, but not affect them. + */ +public interface DownloadStateProvider { /** * @return {@code true} if the named feedfile is in the downloads list */ diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java index 6e7843836..e69a1adf3 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java @@ -41,11 +41,8 @@ class ItemEnqueuePositionCalculator { private final @NonNull Options options; - /** - * The logic needs to use {@link DownloadRequester#isDownloadingFile(FeedFile)} method only - */ @VisibleForTesting - FeedFileDownloadStatusRequesterInterface requester = DownloadRequester.getInstance(); + DownloadStateProvider downloadStateProvider = DownloadRequester.getInstance(); public ItemEnqueuePositionCalculator(@NonNull Options options) { this.options = options; @@ -100,7 +97,7 @@ class ItemEnqueuePositionCalculator { if (curItem != null && curItem.getMedia() != null && - requester.isDownloadingFile(curItem.getMedia())) { + downloadStateProvider.isDownloadingFile(curItem.getMedia())) { return true; } else { return false; diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index 1331df67d..7b5296e8e 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -186,7 +186,7 @@ public class ItemEnqueuePositionCalculatorTest { // ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); MockDownloadRequester mockDownloadRequester = new MockDownloadRequester(); - calculator.requester = mockDownloadRequester; + calculator.downloadStateProvider = mockDownloadRequester; // Setup initial data // A shallow copy, as the test code will manipulate the queue @@ -236,7 +236,7 @@ public class ItemEnqueuePositionCalculatorTest { return item; } - private static class MockDownloadRequester implements FeedFileDownloadStatusRequesterInterface { + private static class MockDownloadRequester implements DownloadStateProvider { private Map downloadingByIds = new HashMap<>(); From fb6fa010f8b8ba8aca4bb0eec173183bf1163f66 Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 4 Oct 2019 14:22:23 -0700 Subject: [PATCH 14/28] Enqueue tweaks - replace custom stub DownloadStateProvider with mockito mocks in test --- .../ItemEnqueuePositionCalculatorTest.java | 41 ++++++------------- 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index 7b5296e8e..424240fa8 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -1,7 +1,5 @@ package de.danoeh.antennapod.core.storage; -import androidx.annotation.NonNull; - import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -12,18 +10,18 @@ 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 java.util.stream.Collectors; -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; import de.danoeh.antennapod.core.storage.ItemEnqueuePositionCalculator.Options; import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.stub; public class ItemEnqueuePositionCalculatorTest { @@ -185,8 +183,9 @@ public class ItemEnqueuePositionCalculatorTest { // Setup class under test // ItemEnqueuePositionCalculator calculator = new ItemEnqueuePositionCalculator(options); - MockDownloadRequester mockDownloadRequester = new MockDownloadRequester(); - calculator.downloadStateProvider = mockDownloadRequester; + DownloadStateProvider stubDownloadStateProvider = mock(DownloadStateProvider.class); + stub(stubDownloadStateProvider.isDownloadingFile(any(FeedMedia.class))).toReturn(false); + calculator.downloadStateProvider = stubDownloadStateProvider; // Setup initial data // A shallow copy, as the test code will manipulate the queue @@ -196,25 +195,25 @@ public class ItemEnqueuePositionCalculatorTest { // Test body // User clicks download on feed item 101 - FeedItem tFI101 = tFI_isDownloading(101, mockDownloadRequester); + FeedItem tFI101 = tFI_isDownloading(101, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (1st download)", calculator, 0, tFI101, queue, idsExpectedAfter101); // Then user clicks download on feed item 102 - FeedItem tFI102 = tFI_isDownloading(102, mockDownloadRequester); + FeedItem tFI102 = tFI_isDownloading(102, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)", calculator, 0, tFI102, queue, idsExpectedAfter102); // Items 201 and 202 are added as part of a single DBWriter.addQueueItem() calls - FeedItem tFI201 = tFI_isDownloading(201, mockDownloadRequester); + FeedItem tFI201 = tFI_isDownloading(201, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (bulk insertion, 1st item)", calculator, 0, tFI201, queue, idsExpectedAfter201); - FeedItem tFI202 = tFI_isDownloading(202, mockDownloadRequester); + FeedItem tFI202 = tFI_isDownloading(202, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (bulk insertion, 2nd item)", calculator, 1, tFI202, queue, idsExpectedAfter202); @@ -223,7 +222,7 @@ public class ItemEnqueuePositionCalculatorTest { } - private static FeedItem tFI_isDownloading(int id, MockDownloadRequester requester) { + private static FeedItem tFI_isDownloading(int id, DownloadStateProvider stubDownloadStateProvider) { FeedItem item = tFI(id); FeedMedia media = new FeedMedia(item, "http://download.url.net/" + id @@ -231,26 +230,10 @@ public class ItemEnqueuePositionCalculatorTest { media.setId(item.getId()); item.setMedia(media); - requester.mockDownloadingFile(media, true); + stub(stubDownloadStateProvider.isDownloadingFile(media)).toReturn(true); return item; } - - private static class MockDownloadRequester implements DownloadStateProvider { - - private Map 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); - } - } } From 418d4fa4d4b2e983888ce92bc54cecae82c534fe Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 11 Oct 2019 13:17:33 -0700 Subject: [PATCH 15/28] bugfix respect download order - obey user settings "Enqueue Downloaded" --- .../test/antennapod/storage/DBTasksTest.java | 99 +++++++++++++++++-- .../core/preferences/UserPreferences.java | 22 ++++- .../antennapod/core/storage/DBTasks.java | 29 +++++- 3 files changed, 140 insertions(+), 10 deletions(-) diff --git a/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java b/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java index 55ea16f13..82462d35f 100644 --- a/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java +++ b/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java @@ -2,26 +2,31 @@ package de.test.antennapod.storage; import android.content.Context; +import androidx.test.InstrumentationRegistry; +import androidx.test.filters.SmallTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Date; import java.util.List; +import java.util.stream.Collectors; -import androidx.test.InstrumentationRegistry; -import androidx.test.filters.LargeTest; -import androidx.test.filters.SmallTest; import de.danoeh.antennapod.core.feed.Feed; import de.danoeh.antennapod.core.feed.FeedItem; import de.danoeh.antennapod.core.feed.FeedMedia; import de.danoeh.antennapod.core.preferences.UserPreferences; 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.storage.PodDBAdapter; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; import static java.util.Collections.singletonList; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -179,4 +184,86 @@ public class DBTasksTest { lastDate = item.getPubDate(); } } + + @Test + public void testAddQueueItemsInDownload_EnqueueEnabled() throws Exception { + // Setup test data / environment + UserPreferences.setEnqueueDownloadedEpisodes(true); + UserPreferences.setEnqueueAtFront(false); + + List fis1 = createSavedFeed("Feed 1", 2).getItems(); + List fis2 = createSavedFeed("Feed 2", 3).getItems(); + + DBWriter.addQueueItem(context, fis1.get(0), fis2.get(0)).get(); + // the first item fis1.get(0) is already in the queue + FeedItem[] itemsToDownload = new FeedItem[]{ fis1.get(0), fis1.get(1), fis2.get(2), fis2.get(1) }; + + // Expectations: + List expectedEnqueued = Arrays.asList(fis1.get(1), fis2.get(2), fis2.get(1)); + List expectedQueue = new ArrayList<>(); + expectedQueue.addAll(DBReader.getQueue()); + expectedQueue.addAll(expectedEnqueued); + + // Run actual test and assert results + List actualEnqueued = + DBTasks.enqueueFeedItemsToDownload(context, itemsToDownload); + + assertEqualsByIds("Only items not in the queue are enqueued", expectedEnqueued, actualEnqueued); + assertEqualsByIds("Queue has new items appended", expectedQueue, DBReader.getQueue()); + } + + @Test + public void testAddQueueItemsInDownload_EnqueueDisabled() throws Exception { + // Setup test data / environment + UserPreferences.setEnqueueDownloadedEpisodes(false); + + List fis1 = createSavedFeed("Feed 1", 2).getItems(); + List fis2 = createSavedFeed("Feed 2", 3).getItems(); + + DBWriter.addQueueItem(context, fis1.get(0), fis2.get(0)).get(); + FeedItem[] itemsToDownload = new FeedItem[]{ fis1.get(0), fis1.get(1), fis2.get(2), fis2.get(1) }; + + // Expectations: + List expectedEnqueued = Collections.emptyList(); + List expectedQueue = DBReader.getQueue(); + + // Run actual test and assert results + List actualEnqueued = + DBTasks.enqueueFeedItemsToDownload(context, itemsToDownload); + + assertEqualsByIds("No item is enqueued", expectedEnqueued, actualEnqueued); + assertEqualsByIds("Queue is unchanged", expectedQueue, DBReader.getQueue()); + } + + private void assertEqualsByIds(String msg, List expected, List actual) { + // assert only the IDs, so that any differences are easily to spot. + List expectedIds = toIds(expected); + List actualIds = toIds(actual); + assertEquals(msg, expectedIds, actualIds); + } + + private static List toIds(List items) { + return items.stream().map(FeedItem::getId).collect(Collectors.toList()); + } + + private Feed createSavedFeed(String title, int numFeedItems) { + final Feed feed = new Feed("url", null, title); + + if (numFeedItems > 0) { + List items = new ArrayList<>(numFeedItems); + for (int i = 1; i <= numFeedItems; i++) { + FeedItem item = new FeedItem(0, "item " + i + " of " + title, "id", "link", + new Date(), FeedItem.UNPLAYED, feed); + items.add(item); + } + feed.setItems(items); + } + + PodDBAdapter adapter = PodDBAdapter.getInstance(); + adapter.open(); + adapter.setCompleteFeed(feed); + adapter.close(); + return feed; + } + } 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 edd5e61d1..16561779e 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 @@ -4,12 +4,14 @@ import android.content.Context; import android.content.SharedPreferences; import android.content.res.Configuration; import android.preference.PreferenceManager; -import androidx.annotation.IntRange; -import androidx.annotation.NonNull; -import androidx.core.app.NotificationCompat; import android.text.TextUtils; import android.util.Log; +import androidx.annotation.IntRange; +import androidx.annotation.NonNull; +import androidx.annotation.VisibleForTesting; +import androidx.core.app.NotificationCompat; + import org.json.JSONArray; import org.json.JSONException; @@ -285,10 +287,24 @@ public class UserPreferences { return prefs.getBoolean(PREF_ENQUEUE_DOWNLOADED, true); } + @VisibleForTesting + public static void setEnqueueDownloadedEpisodes(boolean enqueueDownloadedEpisodes) { + prefs.edit() + .putBoolean(PREF_ENQUEUE_DOWNLOADED, enqueueDownloadedEpisodes) + .apply(); + } + public static boolean enqueueAtFront() { return prefs.getBoolean(PREF_QUEUE_ADD_TO_FRONT, false); } + @VisibleForTesting + public static void setEnqueueAtFront(boolean enqueueAtFront) { + prefs.edit() + .putBoolean(PREF_QUEUE_ADD_TO_FRONT, enqueueAtFront) + .apply(); + } + /** * * @return {@code true} if in enqueuing items/podcast episodes, when the existing front item is 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 def4d84b5..7dc53f8b3 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 @@ -7,6 +7,8 @@ import android.os.Looper; import android.text.TextUtils; import android.util.Log; +import androidx.annotation.VisibleForTesting; + import java.util.ArrayList; import java.util.Collections; import java.util.Date; @@ -26,6 +28,7 @@ import de.danoeh.antennapod.core.feed.Feed; import de.danoeh.antennapod.core.feed.FeedItem; import de.danoeh.antennapod.core.feed.FeedMedia; import de.danoeh.antennapod.core.feed.FeedPreferences; +import de.danoeh.antennapod.core.preferences.UserPreferences; import de.danoeh.antennapod.core.service.GpodnetSyncService; import de.danoeh.antennapod.core.service.download.DownloadStatus; import de.danoeh.antennapod.core.service.playback.PlaybackService; @@ -331,7 +334,12 @@ public final class DBTasks { } // #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); + try { + enqueueFeedItemsToDownload(context, items); + } catch (Throwable t) { + throw new DownloadRequestException("Unexpected exception during enqueue before downloads", t); + } + // Then, download them for (FeedItem item : items) { if (item.getMedia() != null @@ -358,6 +366,25 @@ public final class DBTasks { } } + @VisibleForTesting + public static List enqueueFeedItemsToDownload(final Context context, + FeedItem... items) + throws InterruptedException, ExecutionException { + List itemsToEnqueue = new ArrayList<>(); + if (UserPreferences.enqueueDownloadedEpisodes()) { + LongList queueIDList = DBReader.getQueueIDList(); + for (FeedItem item : items) { + if (!queueIDList.contains(item.getId())) { + itemsToEnqueue.add(item); + } + } + DBWriter.addQueueItem(context, + itemsToEnqueue.toArray(new FeedItem[0])) + .get(); + } + return itemsToEnqueue; + } + /** * Looks for undownloaded episodes in the queue or list of unread items and request a download if * 1. Network is available From 69c00224724875866da6963745712befaca91c9b Mon Sep 17 00:00:00 2001 From: orionlee Date: Fri, 4 Oct 2019 14:06:35 -0700 Subject: [PATCH 16/28] code style fixes - naming, indentation, etc. --- app/src/main/res/xml/preferences_playback.xml | 10 +++++----- .../storage/ItemEnqueuePositionCalculator.java | 16 ++++++++-------- .../ItemEnqueuePositionCalculatorTest.java | 4 ++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/src/main/res/xml/preferences_playback.xml b/app/src/main/res/xml/preferences_playback.xml index b743bdbaf..44ecf32ec 100644 --- a/app/src/main/res/xml/preferences_playback.xml +++ b/app/src/main/res/xml/preferences_playback.xml @@ -97,11 +97,11 @@ android:summary="@string/pref_queueAddToFront_sum" android:title="@string/pref_queueAddToFront_title"/> + android:defaultValue="false" + android:enabled="false" + android:key="prefQueueKeepInProgressAtFront" + android:summary="@string/pref_queueKeepInProgressAtFront_sum" + android:title="@string/pref_queueKeepInProgressAtFront_title"/> curQueue) { + private int getPositionOfFirstNonDownloadingItem(int startPosition, List curQueue) { final int curQueueSize = curQueue.size(); for (int i = startPosition; i < curQueueSize; i++) { if (!isItemAtPositionDownloading(i, curQueue)) { @@ -95,9 +95,9 @@ class ItemEnqueuePositionCalculator { curItem = null; } - if (curItem != null && - curItem.getMedia() != null && - downloadStateProvider.isDownloadingFile(curItem.getMedia())) { + if (curItem != null + && curItem.getMedia() != null + && downloadStateProvider.isDownloadingFile(curItem.getMedia())) { return true; } else { return false; diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index 424240fa8..c076ec892 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -26,7 +26,7 @@ import static org.mockito.Mockito.stub; public class ItemEnqueuePositionCalculatorTest { @RunWith(Parameterized.class) - public static class IEPCBasicTest { + public static class BasicTest { @Parameters(name = "{index}: case<{0}>, expected:{1}") public static Iterable data() { Options optDefault = new Options(); @@ -90,7 +90,7 @@ public class ItemEnqueuePositionCalculatorTest { } @RunWith(Parameterized.class) - public static class IEPCKeepInProgressAtFrontTest extends IEPCBasicTest { + public static class KeepInProgressAtFrontTest extends BasicTest { @Parameters(name = "{index}: case<{0}>, expected:{1}") public static Iterable data() { Options optKeepInProgressAtFront = From d24669d4c1eae8f32d59c85ea2b748b16851a5e3 Mon Sep 17 00:00:00 2001 From: orionlee Date: Sun, 27 Oct 2019 10:17:34 -0700 Subject: [PATCH 17/28] refactor extract common FeedItem List to IDs method --- .../java/de/test/antennapod/storage/DBTasksTest.java | 10 +++------- .../de/danoeh/antennapod/core/util/FeedItemUtil.java | 12 ++++++++++++ .../storage/ItemEnqueuePositionCalculatorTest.java | 11 ++++------- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java b/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java index 82462d35f..0cb5270ef 100644 --- a/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java +++ b/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java @@ -14,7 +14,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.Date; import java.util.List; -import java.util.stream.Collectors; import de.danoeh.antennapod.core.feed.Feed; import de.danoeh.antennapod.core.feed.FeedItem; @@ -25,6 +24,7 @@ import de.danoeh.antennapod.core.storage.DBTasks; import de.danoeh.antennapod.core.storage.DBWriter; import de.danoeh.antennapod.core.storage.PodDBAdapter; +import static de.danoeh.antennapod.core.util.FeedItemUtil.getIdList; import static java.util.Collections.singletonList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -237,15 +237,11 @@ public class DBTasksTest { private void assertEqualsByIds(String msg, List expected, List actual) { // assert only the IDs, so that any differences are easily to spot. - List expectedIds = toIds(expected); - List actualIds = toIds(actual); + List expectedIds = getIdList(expected); + List actualIds = getIdList(actual); assertEquals(msg, expectedIds, actualIds); } - private static List toIds(List items) { - return items.stream().map(FeedItem::getId).collect(Collectors.toList()); - } - private Feed createSavedFeed(String title, int numFeedItems) { final Feed feed = new Feed("url", null, title); diff --git a/core/src/main/java/de/danoeh/antennapod/core/util/FeedItemUtil.java b/core/src/main/java/de/danoeh/antennapod/core/util/FeedItemUtil.java index 8d77f0f24..5ae8dbcc7 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/util/FeedItemUtil.java +++ b/core/src/main/java/de/danoeh/antennapod/core/util/FeedItemUtil.java @@ -1,7 +1,10 @@ package de.danoeh.antennapod.core.util; +import androidx.annotation.NonNull; + import org.apache.commons.lang3.StringUtils; +import java.util.ArrayList; import java.util.List; import de.danoeh.antennapod.core.feed.FeedItem; @@ -40,6 +43,15 @@ public class FeedItemUtil { return result; } + @NonNull + public static List getIdList(List items) { + List result = new ArrayList<>(); + for (FeedItem item : items) { + result.add(item.getId()); + } + return result; + } + /** * Get the link for the feed item for the purpose of Share. It fallbacks to * use the feed's link if the named feed item has no link. diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index c076ec892..4b8140083 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -18,6 +18,7 @@ import de.danoeh.antennapod.core.feed.FeedMedia; import de.danoeh.antennapod.core.feed.FeedMother; import de.danoeh.antennapod.core.storage.ItemEnqueuePositionCalculator.Options; +import static de.danoeh.antennapod.core.util.FeedItemUtil.getIdList; import static org.junit.Assert.assertEquals; import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; @@ -122,10 +123,10 @@ public class ItemEnqueuePositionCalculatorTest { } private static final List QUEUE_FRONT_IN_PROGRESS = Arrays.asList(tFI(11, 60000), tFI(12), tFI(13)); - private static final List QUEUE_FRONT_IN_PROGRESS_IDS = toIDs(QUEUE_FRONT_IN_PROGRESS); + private static final List QUEUE_FRONT_IN_PROGRESS_IDS = getIdList(QUEUE_FRONT_IN_PROGRESS); private static final List QUEUE_FRONT_NO_MEDIA = Arrays.asList(tFINoMedia(11), tFI(12), tFI(13)); - private static final List QUEUE_FRONT_NO_MEDIA_IDS = toIDs(QUEUE_FRONT_NO_MEDIA); + private static final List QUEUE_FRONT_NO_MEDIA_IDS = getIdList(QUEUE_FRONT_NO_MEDIA); } @@ -249,7 +250,7 @@ public class ItemEnqueuePositionCalculatorTest { List idsExpected) { int posActual = calculator.calcPosition(positionAmongAdd, itemToAdd, queue); queue.add(posActual, itemToAdd); - assertEquals(message, idsExpected, toIDs(queue)); + assertEquals(message, idsExpected, getIdList(queue)); } static final List QUEUE_EMPTY = Collections.unmodifiableList(Arrays.asList()); @@ -306,8 +307,4 @@ public class ItemEnqueuePositionCalculatorTest { } - static List toIDs(List items) { - return items.stream().map(i->i.getId()).collect(Collectors.toList()); - } - } From 406f1cceb838be0dc468cdaa4c09224bebf27124 Mon Sep 17 00:00:00 2001 From: orionlee Date: Sun, 27 Oct 2019 10:22:41 -0700 Subject: [PATCH 18/28] refactor move generic Collection helpers to CollectionTestUtil --- .../ItemEnqueuePositionCalculatorTest.java | 66 ++++++------------- .../core/util/CollectionTestUtil.java | 30 +++++++++ 2 files changed, 51 insertions(+), 45 deletions(-) create mode 100644 core/src/test/java/de/danoeh/antennapod/core/util/CollectionTestUtil.java diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index 4b8140083..a72400adf 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -17,6 +17,7 @@ import de.danoeh.antennapod.core.feed.FeedItem; import de.danoeh.antennapod.core.feed.FeedMedia; import de.danoeh.antennapod.core.feed.FeedMother; import de.danoeh.antennapod.core.storage.ItemEnqueuePositionCalculator.Options; +import de.danoeh.antennapod.core.util.CollectionTestUtil; import static de.danoeh.antennapod.core.util.FeedItemUtil.getIdList; import static org.junit.Assert.assertEquals; @@ -35,22 +36,22 @@ public class ItemEnqueuePositionCalculatorTest { return Arrays.asList(new Object[][]{ {"case default, i.e., add to the end", - concat(QUEUE_DEFAULT_IDS, TFI_ID), + CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, TFI_ID), optDefault, 0, QUEUE_DEFAULT}, {"case default (2nd item)", - concat(QUEUE_DEFAULT_IDS, TFI_ID), + CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, TFI_ID), optDefault, 1, QUEUE_DEFAULT}, {"case option enqueue at front", - concat(TFI_ID, QUEUE_DEFAULT_IDS), + CollectionTestUtil.concat(TFI_ID, QUEUE_DEFAULT_IDS), optEnqAtFront, 0, QUEUE_DEFAULT}, {"case option enqueue at front (2nd item)", - list(11L, TFI_ID, 12L, 13L, 14L), + CollectionTestUtil.list(11L, TFI_ID, 12L, 13L, 14L), optEnqAtFront, 1, QUEUE_DEFAULT}, {"case empty queue, option default", - list(TFI_ID), + CollectionTestUtil.list(TFI_ID), optDefault, 0, QUEUE_EMPTY}, {"case empty queue, option enqueue at front", - list(TFI_ID), + CollectionTestUtil.list(TFI_ID), optEnqAtFront, 0, QUEUE_EMPTY}, }); } @@ -102,22 +103,22 @@ public class ItemEnqueuePositionCalculatorTest { return Arrays.asList(new Object[][]{ {"case option keep in progress at front", - list(11L, TFI_ID, 12L, 13L), + CollectionTestUtil.list(11L, TFI_ID, 12L, 13L), optKeepInProgressAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, {"case option keep in progress at front (2nd item)", - list(11L, 12L, TFI_ID, 13L), + CollectionTestUtil.list(11L, 12L, TFI_ID, 13L), optKeepInProgressAtFront, 1, QUEUE_FRONT_IN_PROGRESS}, {"case option keep in progress at front, front item not in progress", - concat(TFI_ID, QUEUE_DEFAULT_IDS), + CollectionTestUtil.concat(TFI_ID, QUEUE_DEFAULT_IDS), optKeepInProgressAtFront, 0, QUEUE_DEFAULT}, {"case option keep in progress at front, front item no media at all", - concat(TFI_ID, QUEUE_FRONT_NO_MEDIA_IDS), + CollectionTestUtil.concat(TFI_ID, QUEUE_FRONT_NO_MEDIA_IDS), optKeepInProgressAtFront, 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception {"case option keep in progress at front, but enqueue at front is disabled", - concat(QUEUE_FRONT_IN_PROGRESS_IDS, TFI_ID), + CollectionTestUtil.concat(QUEUE_FRONT_IN_PROGRESS_IDS, TFI_ID), optKeepInProgressAtFrontWithNoEnqueueAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, {"case empty queue, option keep in progress at front", - list(TFI_ID), + CollectionTestUtil.list(TFI_ID), optKeepInProgressAtFront, 0, QUEUE_EMPTY}, }); } @@ -142,16 +143,16 @@ public class ItemEnqueuePositionCalculatorTest { // (rather than the expected positions) return Arrays.asList(new Object[][] { {"download order test, enqueue default", - concat(QUEUE_DEFAULT_IDS, 101L), - concat(QUEUE_DEFAULT_IDS, list(101L, 102L)), - concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L)), - concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L, 202L)), + CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, 101L), + CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, CollectionTestUtil.list(101L, 102L)), + CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, CollectionTestUtil.list(101L, 102L, 201L)), + CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, CollectionTestUtil.list(101L, 102L, 201L, 202L)), optDefault, QUEUE_DEFAULT}, {"download order test, enqueue at front", - concat(101L, QUEUE_DEFAULT_IDS), - concat(list(101L, 102L), QUEUE_DEFAULT_IDS), - concat(list(101L, 102L, 201L), QUEUE_DEFAULT_IDS), - concat(list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS), + CollectionTestUtil.concat(101L, QUEUE_DEFAULT_IDS), + CollectionTestUtil.concat(CollectionTestUtil.list(101L, 102L), QUEUE_DEFAULT_IDS), + CollectionTestUtil.concat(CollectionTestUtil.list(101L, 102L, 201L), QUEUE_DEFAULT_IDS), + CollectionTestUtil.concat(CollectionTestUtil.list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS), optEnqAtFront, QUEUE_DEFAULT}, }); } @@ -282,29 +283,4 @@ public class ItemEnqueuePositionCalculatorTest { return item; } - // Collections helpers - - static List concat(T item, List list) { - List res = new ArrayList<>(list); - res.add(0, item); - return res; - } - - static List concat(List list, T item) { - List res = new ArrayList<>(list); - res.add(item); - return res; - } - - static List concat(List list1, List list2) { - List res = new ArrayList<>(list1); - res.addAll(list2); - return res; - } - - static List list(T... a) { - return Arrays.asList(a); - } - - } diff --git a/core/src/test/java/de/danoeh/antennapod/core/util/CollectionTestUtil.java b/core/src/test/java/de/danoeh/antennapod/core/util/CollectionTestUtil.java new file mode 100644 index 000000000..21f1ef5d4 --- /dev/null +++ b/core/src/test/java/de/danoeh/antennapod/core/util/CollectionTestUtil.java @@ -0,0 +1,30 @@ +package de.danoeh.antennapod.core.util; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +public class CollectionTestUtil { + + public static List concat(T item, List list) { + List res = new ArrayList<>(list); + res.add(0, item); + return res; + } + + public static List concat(List list, T item) { + List res = new ArrayList<>(list); + res.add(item); + return res; + } + + public static List concat(List list1, List list2) { + List res = new ArrayList<>(list1); + res.addAll(list2); + return res; + } + + public static List list(T... a) { + return Arrays.asList(a); + } +} From 52521ecddb7d7e66e5e132def7341d451c1dbee2 Mon Sep 17 00:00:00 2001 From: orionlee Date: Mon, 28 Oct 2019 12:40:56 -0700 Subject: [PATCH 19/28] #2652 the UI of a new setting enqueue location - replaced existing enqueue at front - the option after current episode will replace Keep In-Progress in Queue that was in the PR (30f104f4). --- .../test/antennapod/ui/PreferencesTest.java | 13 ++--- .../PlaybackPreferencesFragment.java | 52 +++++++++++++++---- app/src/main/res/xml/preferences_playback.xml | 19 +++---- .../core/preferences/UserPreferences.java | 26 ++++++++-- core/src/main/res/values/arrays.xml | 13 +++++ core/src/main/res/values/strings.xml | 9 ++-- 6 files changed, 93 insertions(+), 39 deletions(-) diff --git a/app/src/androidTest/java/de/test/antennapod/ui/PreferencesTest.java b/app/src/androidTest/java/de/test/antennapod/ui/PreferencesTest.java index 65bc7d745..6f541e5cf 100644 --- a/app/src/androidTest/java/de/test/antennapod/ui/PreferencesTest.java +++ b/app/src/androidTest/java/de/test/antennapod/ui/PreferencesTest.java @@ -4,13 +4,13 @@ import android.content.Intent; import android.content.SharedPreferences; import android.content.res.Resources; import android.preference.PreferenceManager; -import androidx.test.filters.LargeTest; +import androidx.test.filters.LargeTest; import androidx.test.rule.ActivityTestRule; + import com.robotium.solo.Solo; import com.robotium.solo.Timeout; -import de.test.antennapod.EspressoTestUtils; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -28,6 +28,7 @@ import de.danoeh.antennapod.core.storage.EpisodeCleanupAlgorithm; import de.danoeh.antennapod.fragment.EpisodesFragment; import de.danoeh.antennapod.fragment.QueueFragment; import de.danoeh.antennapod.fragment.SubscriptionFragment; +import de.test.antennapod.EspressoTestUtils; import static androidx.test.InstrumentationRegistry.getInstrumentation; import static androidx.test.espresso.Espresso.onView; @@ -126,13 +127,9 @@ public class PreferencesTest { } @Test - public void testEnqueueAtFront() { + public void testEnqueueLocation() { clickPreference(R.string.playback_pref); - final boolean enqueueAtFront = UserPreferences.enqueueAtFront(); - clickPreference(R.string.pref_queueAddToFront_title); - assertTrue(solo.waitForCondition(() -> enqueueAtFront != UserPreferences.enqueueAtFront(), Timeout.getLargeTimeout())); - clickPreference(R.string.pref_queueAddToFront_title); - assertTrue(solo.waitForCondition(() -> enqueueAtFront == UserPreferences.enqueueAtFront(), Timeout.getLargeTimeout())); + // TODO-2652: implement the test } @Test diff --git a/app/src/main/java/de/danoeh/antennapod/fragment/preferences/PlaybackPreferencesFragment.java b/app/src/main/java/de/danoeh/antennapod/fragment/preferences/PlaybackPreferencesFragment.java index 64ac1b8ed..5d9af14bd 100644 --- a/app/src/main/java/de/danoeh/antennapod/fragment/preferences/PlaybackPreferencesFragment.java +++ b/app/src/main/java/de/danoeh/antennapod/fragment/preferences/PlaybackPreferencesFragment.java @@ -4,8 +4,15 @@ import android.app.Activity; import android.content.res.Resources; import android.os.Build; import android.os.Bundle; + +import androidx.annotation.NonNull; +import androidx.collection.ArrayMap; import androidx.preference.ListPreference; +import androidx.preference.Preference; import androidx.preference.PreferenceFragmentCompat; + +import java.util.Map; + import de.danoeh.antennapod.R; import de.danoeh.antennapod.activity.MediaplayerActivity; import de.danoeh.antennapod.activity.PreferenceActivity; @@ -64,19 +71,42 @@ public class PlaybackPreferencesFragment extends PreferenceFragmentCompat { behaviour.setEntryValues(R.array.video_background_behavior_values_without_pip); } - findPreference(UserPreferences.PREF_QUEUE_ADD_TO_FRONT).setOnPreferenceChangeListener( - (preference, newValue) -> { - if (newValue instanceof Boolean) { - boolean enableKeepInProgressAtFront = ((Boolean) newValue); - checkKeepInProgressAtFrontItemVisibility(enableKeepInProgressAtFront); - } - return true; - }); - checkKeepInProgressAtFrontItemVisibility(UserPreferences.enqueueAtFront()); + buildEnqueueLocationPreference(); } - private void checkKeepInProgressAtFrontItemVisibility(boolean enabled) { - findPreference(UserPreferences.PREF_QUEUE_KEEP_IN_PROGESS_AT_FRONT).setEnabled(enabled); + private void buildEnqueueLocationPreference() { + final Resources res = requireActivity().getResources(); + final Map options = new ArrayMap<>(); + { + String[] keys = res.getStringArray(R.array.enqueue_location_values); + String[] values = res.getStringArray(R.array.enqueue_location_options); + for (int i = 0; i < keys.length; i++) { + options.put(keys[i], values[i]); + } + } + + ListPreference pref = requirePreference(UserPreferences.PREF_ENQUEUE_LOCATION); + pref.setSummary(res.getString(R.string.pref_enqueue_location_sum, options.get(pref.getValue()))); + + pref.setOnPreferenceChangeListener((preference, newValue) -> { + if (!(newValue instanceof String)) { + return false; + } + String newValStr = (String)newValue; + pref.setSummary(res.getString(R.string.pref_enqueue_location_sum, options.get(newValStr))); + return true; + }); + } + + @NonNull + private T requirePreference(@NonNull CharSequence key) { + // Possibly put it to a common method in abstract base class + T result = findPreference(key); + if (result == null) { + throw new IllegalArgumentException("Preference with key '" + key + "' is not found"); + + } + return result; } private void buildSmartMarkAsPlayedPreference() { diff --git a/app/src/main/res/xml/preferences_playback.xml b/app/src/main/res/xml/preferences_playback.xml index 44ecf32ec..254c907c5 100644 --- a/app/src/main/res/xml/preferences_playback.xml +++ b/app/src/main/res/xml/preferences_playback.xml @@ -90,18 +90,13 @@ android:key="prefEnqueueDownloaded" android:summary="@string/pref_enqueue_downloaded_summary" android:title="@string/pref_enqueue_downloaded_title" /> - - + @string/episode_cleanup_never + + @string/enqueue_location_back + @string/enqueue_location_front + @string/enqueue_location_after_current + + + + + BACK + FRONT + AFTER_CURRENTLY_PLAYING + + -1 0 diff --git a/core/src/main/res/values/strings.xml b/core/src/main/res/values/strings.xml index cf6bc620d..19fc3a81c 100644 --- a/core/src/main/res/values/strings.xml +++ b/core/src/main/res/values/strings.xml @@ -454,10 +454,11 @@ Show Download Report If downloads fail, generate a report that shows the details of the failure. Android versions before 4.1 do not support expanded notifications. - Add new episodes to the front of the queue. - Enqueue at Front - Keep In-progress Episode at Front - If the episode at front is in-progress, i.e., you have listened to part of it, keep it at the front of the queue. + Enqueue Location + Add episodes to: %1$s. + back of the queue + front of the queue + after current episode Disabled Image Cache Size Size of the disk cache for images. From bddd2bfa2e02577112203044b3653d028cd55cd5 Mon Sep 17 00:00:00 2001 From: orionlee Date: Mon, 28 Oct 2019 14:26:10 -0700 Subject: [PATCH 20/28] enqueue location: use the new 3-value settings --- .../test/antennapod/storage/DBTasksTest.java | 2 +- .../test/antennapod/ui/PreferencesTest.java | 14 +- .../preferences/PreferenceUpgrader.java | 8 + .../core/preferences/UserPreferences.java | 25 +-- .../antennapod/core/storage/DBWriter.java | 11 +- .../ItemEnqueuePositionCalculator.java | 80 ++++----- core/src/main/res/values/strings.xml | 8 +- .../ItemEnqueuePositionCalculatorTest.java | 168 ++++++++++-------- 8 files changed, 162 insertions(+), 154 deletions(-) diff --git a/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java b/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java index 0cb5270ef..cce4e5111 100644 --- a/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java +++ b/app/src/androidTest/java/de/test/antennapod/storage/DBTasksTest.java @@ -189,7 +189,7 @@ public class DBTasksTest { public void testAddQueueItemsInDownload_EnqueueEnabled() throws Exception { // Setup test data / environment UserPreferences.setEnqueueDownloadedEpisodes(true); - UserPreferences.setEnqueueAtFront(false); + UserPreferences.setEnqueueLocation(UserPreferences.EnqueueLocation.BACK); List fis1 = createSavedFeed("Feed 1", 2).getItems(); List fis2 = createSavedFeed("Feed 2", 3).getItems(); diff --git a/app/src/androidTest/java/de/test/antennapod/ui/PreferencesTest.java b/app/src/androidTest/java/de/test/antennapod/ui/PreferencesTest.java index 6f541e5cf..f22e4b426 100644 --- a/app/src/androidTest/java/de/test/antennapod/ui/PreferencesTest.java +++ b/app/src/androidTest/java/de/test/antennapod/ui/PreferencesTest.java @@ -5,6 +5,7 @@ import android.content.SharedPreferences; import android.content.res.Resources; import android.preference.PreferenceManager; +import androidx.annotation.StringRes; import androidx.test.filters.LargeTest; import androidx.test.rule.ActivityTestRule; @@ -21,6 +22,7 @@ import java.util.concurrent.TimeUnit; import de.danoeh.antennapod.R; import de.danoeh.antennapod.activity.PreferenceActivity; import de.danoeh.antennapod.core.preferences.UserPreferences; +import de.danoeh.antennapod.core.preferences.UserPreferences.EnqueueLocation; import de.danoeh.antennapod.core.storage.APCleanupAlgorithm; import de.danoeh.antennapod.core.storage.APNullCleanupAlgorithm; import de.danoeh.antennapod.core.storage.APQueueCleanupAlgorithm; @@ -129,7 +131,17 @@ public class PreferencesTest { @Test public void testEnqueueLocation() { clickPreference(R.string.playback_pref); - // TODO-2652: implement the test + doTestEnqueueLocation(R.string.enqueue_location_after_current, EnqueueLocation.AFTER_CURRENTLY_PLAYING); + doTestEnqueueLocation(R.string.enqueue_location_front, EnqueueLocation.FRONT); + doTestEnqueueLocation(R.string.enqueue_location_back, EnqueueLocation.BACK); + } + + private void doTestEnqueueLocation(@StringRes int optionResId, EnqueueLocation expected) { + clickPreference(R.string.pref_enqueue_location_title); + onView(withText(optionResId)).perform(click()); + assertTrue(solo.waitForCondition( + () -> expected == UserPreferences.getEnqueueLocation(), + Timeout.getLargeTimeout())); } @Test diff --git a/app/src/main/java/de/danoeh/antennapod/preferences/PreferenceUpgrader.java b/app/src/main/java/de/danoeh/antennapod/preferences/PreferenceUpgrader.java index 767f71bb6..455038960 100644 --- a/app/src/main/java/de/danoeh/antennapod/preferences/PreferenceUpgrader.java +++ b/app/src/main/java/de/danoeh/antennapod/preferences/PreferenceUpgrader.java @@ -7,6 +7,7 @@ import android.preference.PreferenceManager; import de.danoeh.antennapod.BuildConfig; import de.danoeh.antennapod.R; import de.danoeh.antennapod.core.preferences.UserPreferences; +import de.danoeh.antennapod.core.preferences.UserPreferences.EnqueueLocation; import de.danoeh.antennapod.core.util.download.AutoUpdateManager; import de.danoeh.antennapod.core.util.gui.NotificationUtils; @@ -75,6 +76,13 @@ public class PreferenceUpgrader { } UserPreferences.setQueueLocked(false); + + if (!prefs.contains(UserPreferences.PREF_ENQUEUE_LOCATION)) { + final String keyOldPrefEnqueueFront = "prefQueueAddToFront"; + boolean enqueueAtFront = prefs.getBoolean(keyOldPrefEnqueueFront, false); + EnqueueLocation enqueueLocation = enqueueAtFront ? EnqueueLocation.FRONT : EnqueueLocation.BACK; + UserPreferences.setEnqueueLocation(enqueueLocation); + } } } } 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 796848aaf..8e7cc1bec 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 @@ -63,9 +63,6 @@ public class UserPreferences { public static final String PREF_BACK_BUTTON_BEHAVIOR = "prefBackButtonBehavior"; private static final String PREF_BACK_BUTTON_GO_TO_PAGE = "prefBackButtonGoToPage"; - // Queue - public static final String PREF_QUEUE_ADD_TO_FRONT = "prefQueueAddToFront"; - public static final String PREF_QUEUE_KEEP_IN_PROGESS_AT_FRONT = "prefQueueKeepInProgressAtFront"; public static final String PREF_QUEUE_KEEP_SORTED = "prefQueueKeepSorted"; public static final String PREF_QUEUE_KEEP_SORTED_ORDER = "prefQueueKeepSortedOrder"; @@ -299,6 +296,7 @@ public class UserPreferences { BACK, FRONT, AFTER_CURRENTLY_PLAYING; } + @NonNull public static EnqueueLocation getEnqueueLocation() { String valStr = prefs.getString(PREF_ENQUEUE_LOCATION, EnqueueLocation.BACK.name()); try { @@ -310,29 +308,12 @@ public class UserPreferences { } } - // TODO-2652: migrate settings - - public static boolean enqueueAtFront() { // TODO-2652: migrate to the new settings - return prefs.getBoolean(PREF_QUEUE_ADD_TO_FRONT, false); - } - - @VisibleForTesting - public static void setEnqueueAtFront(boolean enqueueAtFront) { // TODO-2652: migrate to the new settings + public static void setEnqueueLocation(@NonNull EnqueueLocation location) { prefs.edit() - .putBoolean(PREF_QUEUE_ADD_TO_FRONT, enqueueAtFront) + .putString(PREF_ENQUEUE_LOCATION, location.name()) .apply(); } - /** - * - * @return {@code true} if in enqueuing items/podcast episodes, when the existing front item is - * in-progress, i.e., the user has played part of it, such item remains at the front of the - * queue; {@code false} otherwise. - */ - public static boolean keepInProgressAtFront() { // TODO-2652: migrate to the new settings - return prefs.getBoolean(PREF_QUEUE_KEEP_IN_PROGESS_AT_FRONT, false); - } - public static boolean isPauseOnHeadsetDisconnect() { return prefs.getBoolean(PREF_PAUSE_ON_HEADSET_DISCONNECT, true); } 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 57db6123c..d6571f60a 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 @@ -46,6 +46,7 @@ import de.danoeh.antennapod.core.util.IntentUtils; import de.danoeh.antennapod.core.util.LongList; import de.danoeh.antennapod.core.util.Permutor; import de.danoeh.antennapod.core.util.SortOrder; +import de.danoeh.antennapod.core.util.playback.Playable; /** * Provides methods for writing data to AntennaPod's database. @@ -326,19 +327,15 @@ public class DBWriter { List events = new ArrayList<>(); List updatedItems = new ArrayList<>(); ItemEnqueuePositionCalculator positionCalculator = - new ItemEnqueuePositionCalculator( - new ItemEnqueuePositionCalculator.Options() - .setEnqueueAtFront(UserPreferences.enqueueAtFront()) - .setKeepInProgressAtFront(UserPreferences.keepInProgressAtFront()) - ); - + new ItemEnqueuePositionCalculator(UserPreferences.getEnqueueLocation()); + Playable currentlyPlaying = Playable.PlayableUtils.createInstanceFromPreferences(context); for (int i = 0; i < itemIds.length; i++) { if (!itemListContains(queue, itemIds[i])) { final FeedItem item = DBReader.getFeedItem(itemIds[i]); if (item != null) { - int insertPosition = positionCalculator.calcPosition(i, item, queue); + int insertPosition = positionCalculator.calcPosition(i, item, queue, currentlyPlaying); queue.add(insertPosition, item); events.add(QueueEvent.added(item, insertPosition)); diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java index c23c371e3..f87a60203 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java @@ -3,11 +3,15 @@ package de.danoeh.antennapod.core.storage; import android.content.Context; import androidx.annotation.NonNull; +import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; import java.util.List; import de.danoeh.antennapod.core.feed.FeedItem; +import de.danoeh.antennapod.core.feed.FeedMedia; +import de.danoeh.antennapod.core.preferences.UserPreferences.EnqueueLocation; +import de.danoeh.antennapod.core.util.playback.Playable; /** * @see DBWriter#addQueueItem(Context, boolean, long...) it uses the class to determine @@ -15,65 +19,41 @@ import de.danoeh.antennapod.core.feed.FeedItem; */ class ItemEnqueuePositionCalculator { - public static class Options { - private boolean enqueueAtFront = false; - private boolean keepInProgressAtFront = false; - - public boolean isEnqueueAtFront() { - return enqueueAtFront; - } - - public Options setEnqueueAtFront(boolean enqueueAtFront) { - this.enqueueAtFront = enqueueAtFront; - return this; - } - - public boolean isKeepInProgressAtFront() { - return keepInProgressAtFront; - } - - public Options setKeepInProgressAtFront(boolean keepInProgressAtFront) { - this.keepInProgressAtFront = keepInProgressAtFront; - return this; - } - } - @NonNull - private final Options options; + private final EnqueueLocation enqueueLocation; @VisibleForTesting DownloadStateProvider downloadStateProvider = DownloadRequester.getInstance(); - public ItemEnqueuePositionCalculator(@NonNull Options options) { - this.options = options; + public ItemEnqueuePositionCalculator(@NonNull EnqueueLocation enqueueLocation) { + this.enqueueLocation = enqueueLocation; } /** - * * @param positionAmongToAdd Typically, the callers has a list of items to be inserted to * the queue. This parameter indicates the position (0-based) of * the item among the one to inserted. E.g., it is needed for * enqueue at front option. - * - * @param item the item to be inserted - * @param curQueue the queue to which the item is to be inserted - * @return the position (0-based) the item should be inserted to the named queu + * @param item the item to be inserted + * @param curQueue the queue to which the item is to be inserted + * @param currentPlaying the currently playing media + * @return the position (0-based) the item should be inserted to the named queue */ - public int calcPosition(int positionAmongToAdd, FeedItem item, List curQueue) { - if (options.isEnqueueAtFront()) { - if (options.isKeepInProgressAtFront() && - curQueue.size() > 0 && - curQueue.get(0).getMedia() != null && - curQueue.get(0).getMedia().isInProgress()) { - // leave the front in progress item at the front - return getPositionOfFirstNonDownloadingItem(positionAmongToAdd + 1, curQueue); - } else { // typical case + public int calcPosition(int positionAmongToAdd, @NonNull FeedItem item, + @NonNull List curQueue, @Nullable Playable currentPlaying) { + switch (enqueueLocation) { + case BACK: + return curQueue.size(); + case FRONT: // 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 getPositionOfFirstNonDownloadingItem(positionAmongToAdd, curQueue); - } - } else { - return curQueue.size(); + case AFTER_CURRENTLY_PLAYING: + int currentlyPlayingPosition = getCurrentlyPlayingPosition(curQueue, currentPlaying); + return getPositionOfFirstNonDownloadingItem( + currentlyPlayingPosition + (1 + positionAmongToAdd), curQueue); + default: + throw new AssertionError("calcPosition() : unrecognized enqueueLocation option: " + enqueueLocation); } } @@ -103,4 +83,18 @@ class ItemEnqueuePositionCalculator { return false; } } + + private static int getCurrentlyPlayingPosition(@NonNull List curQueue, + @Nullable Playable currentPlaying) { + if (!(currentPlaying instanceof FeedMedia)) { + return -1; + } + final long curPlayingItemId = ((FeedMedia) currentPlaying).getItem().getId(); + for (int i = 0; i < curQueue.size(); i++) { + if (curPlayingItemId == curQueue.get(i).getId()) { + return i; + } + } + return -1; + } } diff --git a/core/src/main/res/values/strings.xml b/core/src/main/res/values/strings.xml index 19fc3a81c..b8187a404 100644 --- a/core/src/main/res/values/strings.xml +++ b/core/src/main/res/values/strings.xml @@ -455,10 +455,10 @@ If downloads fail, generate a report that shows the details of the failure. Android versions before 4.1 do not support expanded notifications. Enqueue Location - Add episodes to: %1$s. - back of the queue - front of the queue - after current episode + Add episodes to: %1$s + Back + Front + After current episode Disabled Image Cache Size Size of the disk cache for images. diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index a72400adf..324f3812f 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -16,9 +16,16 @@ import java.util.stream.Collectors; import de.danoeh.antennapod.core.feed.FeedItem; import de.danoeh.antennapod.core.feed.FeedMedia; import de.danoeh.antennapod.core.feed.FeedMother; -import de.danoeh.antennapod.core.storage.ItemEnqueuePositionCalculator.Options; -import de.danoeh.antennapod.core.util.CollectionTestUtil; +import de.danoeh.antennapod.core.feed.MediaType; +import de.danoeh.antennapod.core.preferences.UserPreferences.EnqueueLocation; +import de.danoeh.antennapod.core.util.playback.ExternalMedia; +import de.danoeh.antennapod.core.util.playback.Playable; +import static de.danoeh.antennapod.core.preferences.UserPreferences.EnqueueLocation.AFTER_CURRENTLY_PLAYING; +import static de.danoeh.antennapod.core.preferences.UserPreferences.EnqueueLocation.BACK; +import static de.danoeh.antennapod.core.preferences.UserPreferences.EnqueueLocation.FRONT; +import static de.danoeh.antennapod.core.util.CollectionTestUtil.concat; +import static de.danoeh.antennapod.core.util.CollectionTestUtil.list; import static de.danoeh.antennapod.core.util.FeedItemUtil.getIdList; import static org.junit.Assert.assertEquals; import static org.mockito.Mockito.any; @@ -31,28 +38,25 @@ public class ItemEnqueuePositionCalculatorTest { public static class BasicTest { @Parameters(name = "{index}: case<{0}>, expected:{1}") public static Iterable data() { - Options optDefault = new Options(); - Options optEnqAtFront = new Options().setEnqueueAtFront(true); - return Arrays.asList(new Object[][]{ {"case default, i.e., add to the end", - CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, TFI_ID), - optDefault, 0, QUEUE_DEFAULT}, + concat(QUEUE_DEFAULT_IDS, TFI_ID), + BACK, 0, QUEUE_DEFAULT}, {"case default (2nd item)", - CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, TFI_ID), - optDefault, 1, QUEUE_DEFAULT}, + concat(QUEUE_DEFAULT_IDS, TFI_ID), + BACK, 1, QUEUE_DEFAULT}, {"case option enqueue at front", - CollectionTestUtil.concat(TFI_ID, QUEUE_DEFAULT_IDS), - optEnqAtFront, 0, QUEUE_DEFAULT}, + concat(TFI_ID, QUEUE_DEFAULT_IDS), + FRONT, 0, QUEUE_DEFAULT}, {"case option enqueue at front (2nd item)", - CollectionTestUtil.list(11L, TFI_ID, 12L, 13L, 14L), - optEnqAtFront, 1, QUEUE_DEFAULT}, + list(11L, TFI_ID, 12L, 13L, 14L), + FRONT, 1, QUEUE_DEFAULT}, {"case empty queue, option default", - CollectionTestUtil.list(TFI_ID), - optDefault, 0, QUEUE_EMPTY}, + list(TFI_ID), + BACK, 0, QUEUE_EMPTY}, {"case empty queue, option enqueue at front", - CollectionTestUtil.list(TFI_ID), - optEnqAtFront, 0, QUEUE_EMPTY}, + list(TFI_ID), + FRONT, 0, QUEUE_EMPTY}, }); } @@ -63,7 +67,7 @@ public class ItemEnqueuePositionCalculatorTest { public List idsExpected; @Parameter(2) - public Options options; + public EnqueueLocation options; @Parameter(3) public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted. @@ -71,7 +75,6 @@ public class ItemEnqueuePositionCalculatorTest { @Parameter(4) public List curQueue; - public static final long TFI_ID = 101; /** @@ -85,49 +88,62 @@ public class ItemEnqueuePositionCalculatorTest { List queue = new ArrayList<>(curQueue); FeedItem tFI = tFI(TFI_ID); doAddToQueueAndAssertResult(message, - calculator, posAmongAdded, tFI, queue, + calculator, posAmongAdded, tFI, queue, getCurrentlyPlaying(), idsExpected); } + Playable getCurrentlyPlaying() { return null; } } @RunWith(Parameterized.class) - public static class KeepInProgressAtFrontTest extends BasicTest { + public static class AfterCurrentlyPlayingTest extends BasicTest { @Parameters(name = "{index}: case<{0}>, expected:{1}") public static Iterable data() { - Options optKeepInProgressAtFront = - new Options().setEnqueueAtFront(true).setKeepInProgressAtFront(true); - // edge case: keep in progress without enabling enqueue at front is meaningless - Options optKeepInProgressAtFrontWithNoEnqueueAtFront = - new Options().setKeepInProgressAtFront(true); - return Arrays.asList(new Object[][]{ - {"case option keep in progress at front", - CollectionTestUtil.list(11L, TFI_ID, 12L, 13L), - optKeepInProgressAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, - {"case option keep in progress at front (2nd item)", - CollectionTestUtil.list(11L, 12L, TFI_ID, 13L), - optKeepInProgressAtFront, 1, QUEUE_FRONT_IN_PROGRESS}, - {"case option keep in progress at front, front item not in progress", - CollectionTestUtil.concat(TFI_ID, QUEUE_DEFAULT_IDS), - optKeepInProgressAtFront, 0, QUEUE_DEFAULT}, - {"case option keep in progress at front, front item no media at all", - CollectionTestUtil.concat(TFI_ID, QUEUE_FRONT_NO_MEDIA_IDS), - optKeepInProgressAtFront, 0, QUEUE_FRONT_NO_MEDIA}, // No media should not cause any exception - {"case option keep in progress at front, but enqueue at front is disabled", - CollectionTestUtil.concat(QUEUE_FRONT_IN_PROGRESS_IDS, TFI_ID), - optKeepInProgressAtFrontWithNoEnqueueAtFront, 0, QUEUE_FRONT_IN_PROGRESS}, - {"case empty queue, option keep in progress at front", - CollectionTestUtil.list(TFI_ID), - optKeepInProgressAtFront, 0, QUEUE_EMPTY}, + {"case option after currently playing", + list(11L, TFI_ID, 12L, 13L, 14L), + AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, 11L}, + {"case option after currently playing (2nd item)", + list(11L, 12L, TFI_ID, 13L, 14L), + AFTER_CURRENTLY_PLAYING, 1, QUEUE_DEFAULT, 11L}, + {"case option after currently playing, currently playing in the middle of the queue", + list(11L, 12L, 13L, TFI_ID, 14L), + AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, 13L}, + {"case option after currently playing, currently playing is not in queue", + concat(TFI_ID, QUEUE_DEFAULT_IDS), + AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, 99L}, + {"case option after currently playing, no currentlyPlaying is null", + concat(TFI_ID, QUEUE_DEFAULT_IDS), + AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NULL}, + {"case option after currently playing, currentlyPlaying is externalMedia", + concat(TFI_ID, QUEUE_DEFAULT_IDS), + AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA}, + {"case empty queue, option after currently playing", + list(TFI_ID), + AFTER_CURRENTLY_PLAYING, 0, QUEUE_EMPTY, ID_CURRENTLY_PLAYING_NULL}, }); } - private static final List QUEUE_FRONT_IN_PROGRESS = Arrays.asList(tFI(11, 60000), tFI(12), tFI(13)); - private static final List QUEUE_FRONT_IN_PROGRESS_IDS = getIdList(QUEUE_FRONT_IN_PROGRESS); + @Parameter(5) + public long idCurrentlyPlaying = -1; - private static final List QUEUE_FRONT_NO_MEDIA = Arrays.asList(tFINoMedia(11), tFI(12), tFI(13)); - private static final List QUEUE_FRONT_NO_MEDIA_IDS = getIdList(QUEUE_FRONT_NO_MEDIA); + @Override + Playable getCurrentlyPlaying() { + if (ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA == idCurrentlyPlaying) { + return externalMedia(); + } + if (ID_CURRENTLY_PLAYING_NULL == idCurrentlyPlaying) { + return null; + } + return tFI(idCurrentlyPlaying).getMedia(); + } + + private static Playable externalMedia() { + return new ExternalMedia("http://example.com/episode.mp3", MediaType.AUDIO); + } + + private static final long ID_CURRENTLY_PLAYING_NULL = -1L; + private static final long ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA = -9999L; } @@ -136,24 +152,21 @@ public class ItemEnqueuePositionCalculatorTest { @Parameters(name = "{index}: case<{0}>") public static Iterable data() { - Options optDefault = new Options(); - Options optEnqAtFront = new Options().setEnqueueAtFront(true); - // Attempts to make test more readable by showing the expected list of ids // (rather than the expected positions) return Arrays.asList(new Object[][] { {"download order test, enqueue default", - CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, 101L), - CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, CollectionTestUtil.list(101L, 102L)), - CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, CollectionTestUtil.list(101L, 102L, 201L)), - CollectionTestUtil.concat(QUEUE_DEFAULT_IDS, CollectionTestUtil.list(101L, 102L, 201L, 202L)), - optDefault, QUEUE_DEFAULT}, + concat(QUEUE_DEFAULT_IDS, 101L), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L)), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L)), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L, 202L)), + BACK, QUEUE_DEFAULT}, {"download order test, enqueue at front", - CollectionTestUtil.concat(101L, QUEUE_DEFAULT_IDS), - CollectionTestUtil.concat(CollectionTestUtil.list(101L, 102L), QUEUE_DEFAULT_IDS), - CollectionTestUtil.concat(CollectionTestUtil.list(101L, 102L, 201L), QUEUE_DEFAULT_IDS), - CollectionTestUtil.concat(CollectionTestUtil.list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS), - optEnqAtFront, QUEUE_DEFAULT}, + concat(101L, QUEUE_DEFAULT_IDS), + concat(list(101L, 102L), QUEUE_DEFAULT_IDS), + concat(list(101L, 102L, 201L), QUEUE_DEFAULT_IDS), + concat(list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS), + FRONT, QUEUE_DEFAULT}, }); } @@ -174,7 +187,7 @@ public class ItemEnqueuePositionCalculatorTest { public List idsExpectedAfter202; @Parameter(5) - public Options options; + public EnqueueLocation options; @Parameter(6) public List queueInitial; @@ -217,7 +230,7 @@ public class ItemEnqueuePositionCalculatorTest { FeedItem tFI202 = tFI_isDownloading(202, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (bulk insertion, 2nd item)", - calculator, 1, tFI202, queue, + calculator, 1, tFI202, queue, null, idsExpectedAfter202); // TODO: simulate download failure cases. @@ -249,31 +262,34 @@ public class ItemEnqueuePositionCalculatorTest { FeedItem itemToAdd, List queue, List idsExpected) { - int posActual = calculator.calcPosition(positionAmongAdd, itemToAdd, queue); + doAddToQueueAndAssertResult(message, calculator, positionAmongAdd, itemToAdd, queue, null, idsExpected); + } + + static void doAddToQueueAndAssertResult(String message, + ItemEnqueuePositionCalculator calculator, + int positionAmongAdd, + FeedItem itemToAdd, + List queue, + Playable currentlyPlaying, + List idsExpected) { + int posActual = calculator.calcPosition(positionAmongAdd, itemToAdd, queue, currentlyPlaying); queue.add(posActual, itemToAdd); assertEquals(message, idsExpected, getIdList(queue)); } static final List QUEUE_EMPTY = Collections.unmodifiableList(Arrays.asList()); - static final List QUEUE_DEFAULT = Collections.unmodifiableList(Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14))); - static final List QUEUE_DEFAULT_IDS = QUEUE_DEFAULT.stream().map(fi -> fi.getId()).collect(Collectors.toList()); + static final List QUEUE_DEFAULT = + Collections.unmodifiableList(Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14))); + static final List QUEUE_DEFAULT_IDS = + QUEUE_DEFAULT.stream().map(fi -> fi.getId()).collect(Collectors.toList()); static FeedItem tFI(long id) { - return tFI(id, -1); - } - - static FeedItem tFI(long 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) { - media.setPosition(position); - } - return item; } From e233398753d9b2a131d51d6ddf44c06d9e42a05e Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 29 Oct 2019 10:47:57 -0700 Subject: [PATCH 21/28] code style fixes: naming, indentation. --- app/src/main/res/xml/preferences_playback.xml | 12 ++++---- .../ItemEnqueuePositionCalculatorTest.java | 30 ++++++++----------- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/app/src/main/res/xml/preferences_playback.xml b/app/src/main/res/xml/preferences_playback.xml index 254c907c5..b01105376 100644 --- a/app/src/main/res/xml/preferences_playback.xml +++ b/app/src/main/res/xml/preferences_playback.xml @@ -91,12 +91,12 @@ android:summary="@string/pref_enqueue_downloaded_summary" android:title="@string/pref_enqueue_downloaded_title" /> + android:defaultValue="BACK" + android:entries="@array/enqueue_location_options" + android:entryValues="@array/enqueue_location_values" + android:key="prefEnqueueLocation" + android:title="@string/pref_enqueue_location_title" + app:useStockLayout="true"/> queue = new ArrayList<>(curQueue); - FeedItem tFI = tFI(TFI_ID); + FeedItem tFI = createFeedItem(TFI_ID); doAddToQueueAndAssertResult(message, calculator, posAmongAdded, tFI, queue, getCurrentlyPlaying(), idsExpected); @@ -135,7 +135,7 @@ public class ItemEnqueuePositionCalculatorTest { if (ID_CURRENTLY_PLAYING_NULL == idCurrentlyPlaying) { return null; } - return tFI(idCurrentlyPlaying).getMedia(); + return createFeedItem(idCurrentlyPlaying).getMedia(); } private static Playable externalMedia() { @@ -210,25 +210,25 @@ public class ItemEnqueuePositionCalculatorTest { // Test body // User clicks download on feed item 101 - FeedItem tFI101 = tFI_isDownloading(101, stubDownloadStateProvider); + FeedItem tFI101 = setAsDownloading(101, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (1st download)", calculator, 0, tFI101, queue, idsExpectedAfter101); // Then user clicks download on feed item 102 - FeedItem tFI102 = tFI_isDownloading(102, stubDownloadStateProvider); + FeedItem tFI102 = setAsDownloading(102, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)", calculator, 0, tFI102, queue, idsExpectedAfter102); // Items 201 and 202 are added as part of a single DBWriter.addQueueItem() calls - FeedItem tFI201 = tFI_isDownloading(201, stubDownloadStateProvider); + FeedItem tFI201 = setAsDownloading(201, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (bulk insertion, 1st item)", calculator, 0, tFI201, queue, idsExpectedAfter201); - FeedItem tFI202 = tFI_isDownloading(202, stubDownloadStateProvider); + FeedItem tFI202 = setAsDownloading(202, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (bulk insertion, 2nd item)", calculator, 1, tFI202, queue, null, idsExpectedAfter202); @@ -237,8 +237,8 @@ public class ItemEnqueuePositionCalculatorTest { } - private static FeedItem tFI_isDownloading(int id, DownloadStateProvider stubDownloadStateProvider) { - FeedItem item = tFI(id); + private static FeedItem setAsDownloading(int id, DownloadStateProvider stubDownloadStateProvider) { + FeedItem item = createFeedItem(id); FeedMedia media = new FeedMedia(item, "http://download.url.net/" + id , 100000 + id, "audio/mp3"); @@ -280,23 +280,19 @@ public class ItemEnqueuePositionCalculatorTest { static final List QUEUE_EMPTY = Collections.unmodifiableList(Arrays.asList()); static final List QUEUE_DEFAULT = - Collections.unmodifiableList(Arrays.asList(tFI(11), tFI(12), tFI(13), tFI(14))); + Collections.unmodifiableList(Arrays.asList( + createFeedItem(11), createFeedItem(12), createFeedItem(13), createFeedItem(14))); static final List QUEUE_DEFAULT_IDS = QUEUE_DEFAULT.stream().map(fi -> fi.getId()).collect(Collectors.toList()); - static FeedItem tFI(long id) { - FeedItem item = tFINoMedia(id); + static FeedItem createFeedItem(long id) { + FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url", + new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); FeedMedia media = new FeedMedia(item, "download_url", 1234567, "audio/mpeg"); media.setId(item.getId()); item.setMedia(media); return item; } - static FeedItem tFINoMedia(long id) { - FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url", - new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); - return item; - } - } From 52f6a121f1bc0a1561eef3541dea5226b1c15edf Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 29 Oct 2019 15:59:22 -0700 Subject: [PATCH 22/28] AFTER_CURRENTLY_PLAYING enqueue location option - test boundary condition handling --- .../antennapod/storage/AutoDownloadTest.java | 168 ++++++++++++++++++ build.gradle | 2 +- .../core/preferences/UserPreferences.java | 8 + 3 files changed, 177 insertions(+), 1 deletion(-) create mode 100644 app/src/androidTest/java/de/test/antennapod/storage/AutoDownloadTest.java diff --git a/app/src/androidTest/java/de/test/antennapod/storage/AutoDownloadTest.java b/app/src/androidTest/java/de/test/antennapod/storage/AutoDownloadTest.java new file mode 100644 index 000000000..c6beb2d42 --- /dev/null +++ b/app/src/androidTest/java/de/test/antennapod/storage/AutoDownloadTest.java @@ -0,0 +1,168 @@ +package de.test.antennapod.storage; + +import android.content.Context; +import android.content.Intent; + +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; +import androidx.test.core.app.ApplicationProvider; + +import org.awaitility.Awaitility; +import org.awaitility.core.ConditionTimeoutException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.util.List; + +import de.danoeh.antennapod.core.ClientConfig; +import de.danoeh.antennapod.core.DBTasksCallbacks; +import de.danoeh.antennapod.core.feed.FeedItem; +import de.danoeh.antennapod.core.feed.FeedMedia; +import de.danoeh.antennapod.core.preferences.UserPreferences; +import de.danoeh.antennapod.core.service.playback.PlaybackService; +import de.danoeh.antennapod.core.storage.AutomaticDownloadAlgorithm; +import de.danoeh.antennapod.core.storage.DBReader; +import de.danoeh.antennapod.core.storage.DBTasks; +import de.danoeh.antennapod.core.storage.EpisodeCleanupAlgorithm; +import de.danoeh.antennapod.core.storage.PodDBAdapter; +import de.danoeh.antennapod.core.util.playback.Playable; +import de.test.antennapod.ui.UITestUtils; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +public class AutoDownloadTest { + + private Context context; + private UITestUtils stubFeedsServer; + + private DBTasksCallbacks dbTasksCallbacksOrig; + + @Before + public void setUp() throws Exception { + context = ApplicationProvider.getApplicationContext(); + + stubFeedsServer = new UITestUtils(context);; + stubFeedsServer.setup(); + + dbTasksCallbacksOrig = ClientConfig.dbTasksCallbacks; + + // create new database + PodDBAdapter.init(context); + PodDBAdapter.deleteDatabase(); + PodDBAdapter adapter = PodDBAdapter.getInstance(); + adapter.open(); + adapter.close(); + } + + @After + public void tearDown() throws Exception { + stubFeedsServer.tearDown(); + + context.sendBroadcast(new Intent(PlaybackService.ACTION_SHUTDOWN_PLAYBACK_SERVICE)); + Awaitility.await().until(() -> !PlaybackService.isRunning); + + ClientConfig.dbTasksCallbacks = dbTasksCallbacksOrig; + } + + /** + * A cross-functional test, ensuring playback's behavior works with Auto Download in boundary condition. + * + * Scenario: + * - For setting enqueue location AFTER_CURRENTLY_PLAYING + * - when playback of an episode is complete and the app advances to the next episode (continuous playback on) + * - when automatic download kicks in, + * - ensure the next episode is the current playing one, needed for AFTER_CURRENTLY_PLAYING enqueue location. + */ + @Test + public void downloadsEnqueuedToAfterCurrent_CurrentAdvancedToNextOnPlaybackComplete() throws Exception { + UserPreferences.setFollowQueue(true); // continuous playback + + // Setup: feeds and queue + // downloads 3 of them, leave some in new state (auto-downloadable) + stubFeedsServer.addLocalFeedData(false); + List queue = DBReader.getQueue(); + assertTrue(queue.size() > 1); + FeedItem item0 = queue.get(0); + FeedItem item1 = queue.get(1); + + // Setup: enable automatic download + // it is not needed, as the actual automatic download is stubbed. + StubDownloadAlgorithm stubDownloadAlgorithm = new StubDownloadAlgorithm(); + useDownloadAlgorithm(stubDownloadAlgorithm); + + // Actual test + // Play the first one in the queue + playEpisode(item0); + + try { + // when playback is complete, advances to the next one, and auto download kicks in, + // ensure that currently playing has been advanced to the next one by this point. + Awaitility.await("advanced to the next episode") + .atMost(6000, MILLISECONDS) // the test mp3 media is 3-second long. twice should be enough + .until(() -> item1.equals(stubDownloadAlgorithm.getCurrentlyPlayingAtDownload())); + } catch (ConditionTimeoutException cte) { + FeedItem actual = stubDownloadAlgorithm.getCurrentlyPlayingAtDownload(); + fail("when auto download is triggered, the next episode should be playing: (" + + item1.getId() + ", " + item1.getTitle() + ") . " + + "Actual playing: (" + + (actual == null ? "" : actual.getId() + ", " + actual.getTitle()) + ")" + ); + } + + } + + private void playEpisode(@NonNull FeedItem item) { + FeedMedia media = item.getMedia(); + DBTasks.playMedia(context, media, false, true, true); + Awaitility.await("episode is playing") + .atMost(1000, MILLISECONDS) + .until(() -> item.equals(getCurrentlyPlaying())); + } + + private FeedItem getCurrentlyPlaying() { + Playable playable = Playable.PlayableUtils.createInstanceFromPreferences(context); + if (playable == null) { + return null; + } + return ((FeedMedia)playable).getItem(); + } + + private void useDownloadAlgorithm(final AutomaticDownloadAlgorithm downloadAlgorithm) { + DBTasksCallbacks dbTasksCallbacksStub = new DBTasksCallbacks() { + @Override + public AutomaticDownloadAlgorithm getAutomaticDownloadAlgorithm() { + return downloadAlgorithm; + } + + @Override + public EpisodeCleanupAlgorithm getEpisodeCacheCleanupAlgorithm() { + return dbTasksCallbacksOrig.getEpisodeCacheCleanupAlgorithm(); + } + }; + ClientConfig.dbTasksCallbacks = dbTasksCallbacksStub; + } + + private class StubDownloadAlgorithm implements AutomaticDownloadAlgorithm { + @Nullable + private FeedItem currentlyPlaying; + + @Override + public Runnable autoDownloadUndownloadedItems(Context context) { + return () -> { + if (currentlyPlaying == null) { + currentlyPlaying = getCurrentlyPlaying(); + } else { + throw new AssertionError("Stub automatic download should be invoked once and only once"); + } + }; + } + + @Nullable + FeedItem getCurrentlyPlayingAtDownload() { + return currentlyPlaying; + } + } +} diff --git a/build.gradle b/build.gradle index 8ac4eda94..73eab1704 100644 --- a/build.gradle +++ b/build.gradle @@ -46,7 +46,7 @@ project.ext { workManagerVersion = "1.0.1" espressoVersion = "3.2.0" - awaitilityVersion = "3.1.2" + awaitilityVersion = "3.1.6" commonsioVersion = "2.5" commonslangVersion = "3.6" commonstextVersion = "1.3" 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 8e7cc1bec..ff2a2e1ca 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 @@ -339,6 +339,14 @@ public class UserPreferences { return prefs.getBoolean(PREF_FOLLOW_QUEUE, true); } + /** + * Set to true to enable Continuous Playback + */ + @VisibleForTesting + public static void setFollowQueue(boolean value) { + prefs.edit().putBoolean(UserPreferences.PREF_FOLLOW_QUEUE, value).apply(); + } + public static boolean shouldSkipKeepEpisode() { return prefs.getBoolean(PREF_SKIP_KEEPS_EPISODE, true); } public static boolean shouldFavoriteKeepEpisode() { From f8fbc8e649723df1c0c6b13e0f0ec1123c5f81bc Mon Sep 17 00:00:00 2001 From: orionlee Date: Thu, 31 Oct 2019 12:55:51 -0700 Subject: [PATCH 23/28] test fix: ensure test is not dependent on UserPreferences's enqueueLocation --- .../test/antennapod/storage/DBWriterTest.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/app/src/androidTest/java/de/test/antennapod/storage/DBWriterTest.java b/app/src/androidTest/java/de/test/antennapod/storage/DBWriterTest.java index 42f4413d3..3bc650da0 100644 --- a/app/src/androidTest/java/de/test/antennapod/storage/DBWriterTest.java +++ b/app/src/androidTest/java/de/test/antennapod/storage/DBWriterTest.java @@ -4,12 +4,15 @@ import android.content.Context; import android.content.SharedPreferences; import android.database.Cursor; import android.preference.PreferenceManager; -import androidx.test.InstrumentationRegistry; -import androidx.test.filters.LargeTest; -import androidx.test.filters.MediumTest; import android.util.Log; +import androidx.test.InstrumentationRegistry; +import androidx.test.filters.MediumTest; + import org.awaitility.Awaitility; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; import java.io.File; import java.io.IOException; @@ -29,9 +32,7 @@ import de.danoeh.antennapod.core.storage.DBReader; import de.danoeh.antennapod.core.storage.DBWriter; import de.danoeh.antennapod.core.storage.PodDBAdapter; import de.danoeh.antennapod.core.util.Consumer; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; +import de.danoeh.antennapod.core.util.FeedItemUtil; import static androidx.test.InstrumentationRegistry.getInstrumentation; import static org.junit.Assert.assertEquals; @@ -477,6 +478,7 @@ public class DBWriterTest { private Feed queueTestSetupMultipleItems(final int NUM_ITEMS) throws InterruptedException, ExecutionException, TimeoutException { final Context context = getInstrumentation().getTargetContext(); + UserPreferences.setEnqueueLocation(UserPreferences.EnqueueLocation.BACK); Feed feed = new Feed("url", null, "title"); feed.setItems(new ArrayList<>()); for (int i = 0; i < NUM_ITEMS; i++) { @@ -573,12 +575,16 @@ public class DBWriterTest { Cursor cursor = adapter.getQueueIDCursor(); assertTrue(cursor.moveToFirst()); assertTrue(cursor.getCount() == NUM_ITEMS); + List expectedIds = FeedItemUtil.getIdList(feed.getItems()); + List actualIds = new ArrayList<>(); for (int i = 0; i < NUM_ITEMS; i++) { assertTrue(cursor.moveToPosition(i)); - assertTrue(cursor.getLong(0) == feed.getItems().get(i).getId()); + actualIds.add(cursor.getLong(0)); } cursor.close(); adapter.close(); + assertEquals("Bulk add to queue: result order should be the same as the order given", + expectedIds, actualIds); } @Test From b80973bc303df5d51c4a677e2d65084eb3b938e8 Mon Sep 17 00:00:00 2001 From: orionlee Date: Thu, 31 Oct 2019 12:59:49 -0700 Subject: [PATCH 24/28] refactor - make enqueue position logic more readable per review. --- .../antennapod/core/storage/DBWriter.java | 9 +- .../ItemEnqueuePositionCalculator.java | 15 +-- .../ItemEnqueuePositionCalculatorTest.java | 125 ++++++++---------- 3 files changed, 63 insertions(+), 86 deletions(-) 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 d6571f60a..aff88ed80 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 @@ -329,13 +329,13 @@ public class DBWriter { ItemEnqueuePositionCalculator positionCalculator = new ItemEnqueuePositionCalculator(UserPreferences.getEnqueueLocation()); Playable currentlyPlaying = Playable.PlayableUtils.createInstanceFromPreferences(context); - for (int i = 0; i < itemIds.length; i++) { - if (!itemListContains(queue, itemIds[i])) { - final FeedItem item = DBReader.getFeedItem(itemIds[i]); + int insertPosition = positionCalculator.calcPosition(queue, currentlyPlaying); + for (long itemId : itemIds) { + if (!itemListContains(queue, itemId)) { + final FeedItem item = DBReader.getFeedItem(itemId); if (item != null) { - int insertPosition = positionCalculator.calcPosition(i, item, queue, currentlyPlaying); queue.add(insertPosition, item); events.add(QueueEvent.added(item, insertPosition)); @@ -345,6 +345,7 @@ public class DBWriter { if (item.isNew()) { markAsUnplayedIds.add(item.getId()); } + insertPosition++; } } } diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java index f87a60203..700c15eb6 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java @@ -30,28 +30,23 @@ class ItemEnqueuePositionCalculator { } /** - * @param positionAmongToAdd Typically, the callers has a list of items to be inserted to - * the queue. This parameter indicates the position (0-based) of - * the item among the one to inserted. E.g., it is needed for - * enqueue at front option. - * @param item the item to be inserted + * Determine the position (0-based) that the item(s) should be inserted to the named queue. + * * @param curQueue the queue to which the item is to be inserted * @param currentPlaying the currently playing media - * @return the position (0-based) the item should be inserted to the named queue */ - public int calcPosition(int positionAmongToAdd, @NonNull FeedItem item, - @NonNull List curQueue, @Nullable Playable currentPlaying) { + public int calcPosition(@NonNull List curQueue, @Nullable Playable currentPlaying) { switch (enqueueLocation) { case BACK: return curQueue.size(); case FRONT: // 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 getPositionOfFirstNonDownloadingItem(positionAmongToAdd, curQueue); + return getPositionOfFirstNonDownloadingItem(0, curQueue); case AFTER_CURRENTLY_PLAYING: int currentlyPlayingPosition = getCurrentlyPlayingPosition(curQueue, currentPlaying); return getPositionOfFirstNonDownloadingItem( - currentlyPlayingPosition + (1 + positionAmongToAdd), curQueue); + currentlyPlayingPosition + 1, curQueue); default: throw new AssertionError("calcPosition() : unrecognized enqueueLocation option: " + enqueueLocation); } diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index e8f64524a..68eb434a4 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -41,22 +41,16 @@ public class ItemEnqueuePositionCalculatorTest { return Arrays.asList(new Object[][]{ {"case default, i.e., add to the end", concat(QUEUE_DEFAULT_IDS, TFI_ID), - BACK, 0, QUEUE_DEFAULT}, - {"case default (2nd item)", - concat(QUEUE_DEFAULT_IDS, TFI_ID), - BACK, 1, QUEUE_DEFAULT}, + BACK, QUEUE_DEFAULT}, {"case option enqueue at front", concat(TFI_ID, QUEUE_DEFAULT_IDS), - FRONT, 0, QUEUE_DEFAULT}, - {"case option enqueue at front (2nd item)", - list(11L, TFI_ID, 12L, 13L, 14L), - FRONT, 1, QUEUE_DEFAULT}, + FRONT, QUEUE_DEFAULT}, {"case empty queue, option default", list(TFI_ID), - BACK, 0, QUEUE_EMPTY}, + BACK, QUEUE_EMPTY}, {"case empty queue, option enqueue at front", list(TFI_ID), - FRONT, 0, QUEUE_EMPTY}, + FRONT, QUEUE_EMPTY}, }); } @@ -70,9 +64,6 @@ public class ItemEnqueuePositionCalculatorTest { public EnqueueLocation options; @Parameter(3) - public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted. - - @Parameter(4) public List curQueue; public static final long TFI_ID = 101; @@ -88,7 +79,7 @@ public class ItemEnqueuePositionCalculatorTest { List queue = new ArrayList<>(curQueue); FeedItem tFI = createFeedItem(TFI_ID); doAddToQueueAndAssertResult(message, - calculator, posAmongAdded, tFI, queue, getCurrentlyPlaying(), + calculator, tFI, queue, getCurrentlyPlaying(), idsExpected); } @@ -102,40 +93,31 @@ public class ItemEnqueuePositionCalculatorTest { return Arrays.asList(new Object[][]{ {"case option after currently playing", list(11L, TFI_ID, 12L, 13L, 14L), - AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, 11L}, - {"case option after currently playing (2nd item)", - list(11L, 12L, TFI_ID, 13L, 14L), - AFTER_CURRENTLY_PLAYING, 1, QUEUE_DEFAULT, 11L}, + AFTER_CURRENTLY_PLAYING, QUEUE_DEFAULT, 11L}, {"case option after currently playing, currently playing in the middle of the queue", list(11L, 12L, 13L, TFI_ID, 14L), - AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, 13L}, + AFTER_CURRENTLY_PLAYING, QUEUE_DEFAULT, 13L}, {"case option after currently playing, currently playing is not in queue", concat(TFI_ID, QUEUE_DEFAULT_IDS), - AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, 99L}, + AFTER_CURRENTLY_PLAYING, QUEUE_DEFAULT, 99L}, {"case option after currently playing, no currentlyPlaying is null", concat(TFI_ID, QUEUE_DEFAULT_IDS), - AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NULL}, + AFTER_CURRENTLY_PLAYING, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NULL}, {"case option after currently playing, currentlyPlaying is externalMedia", concat(TFI_ID, QUEUE_DEFAULT_IDS), - AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA}, + AFTER_CURRENTLY_PLAYING, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA}, {"case empty queue, option after currently playing", list(TFI_ID), - AFTER_CURRENTLY_PLAYING, 0, QUEUE_EMPTY, ID_CURRENTLY_PLAYING_NULL}, + AFTER_CURRENTLY_PLAYING, QUEUE_EMPTY, ID_CURRENTLY_PLAYING_NULL}, }); } - @Parameter(5) - public long idCurrentlyPlaying = -1; + @Parameter(4) + public long idCurrentlyPlaying; @Override Playable getCurrentlyPlaying() { - if (ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA == idCurrentlyPlaying) { - return externalMedia(); - } - if (ID_CURRENTLY_PLAYING_NULL == idCurrentlyPlaying) { - return null; - } - return createFeedItem(idCurrentlyPlaying).getMedia(); + return ItemEnqueuePositionCalculatorTest.getCurrentlyPlaying(idCurrentlyPlaying); } private static Playable externalMedia() { @@ -150,6 +132,11 @@ public class ItemEnqueuePositionCalculatorTest { @RunWith(Parameterized.class) public static class ItemEnqueuePositionCalculatorPreserveDownloadOrderTest { + /** + * The test covers the use case that when user initiates multiple downloads in succession, + * resulting in multiple addQueueItem() calls in succession. + * the items in the queue will be in the same order as the the order user taps to download + */ @Parameters(name = "{index}: case<{0}>") public static Iterable data() { // Attempts to make test more readable by showing the expected list of ids @@ -158,15 +145,15 @@ public class ItemEnqueuePositionCalculatorTest { {"download order test, enqueue default", concat(QUEUE_DEFAULT_IDS, 101L), concat(QUEUE_DEFAULT_IDS, list(101L, 102L)), - concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L)), - concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L, 202L)), - BACK, QUEUE_DEFAULT}, - {"download order test, enqueue at front", + BACK, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NULL}, + {"download order test, enqueue at front (currently playing has no effect)", concat(101L, QUEUE_DEFAULT_IDS), concat(list(101L, 102L), QUEUE_DEFAULT_IDS), - concat(list(101L, 102L, 201L), QUEUE_DEFAULT_IDS), - concat(list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS), - FRONT, QUEUE_DEFAULT}, + FRONT, QUEUE_DEFAULT, 11L}, // 11 is at the front, currently playing + {"download order test, enqueue after currently playing", + list(11L, 101L, 12L, 13L, 14L), + list(11L, 101L, 102L, 12L, 13L, 14L), + AFTER_CURRENTLY_PLAYING, QUEUE_DEFAULT, 11L} // 11 is at the front, currently playing }); } @@ -179,19 +166,15 @@ public class ItemEnqueuePositionCalculatorTest { @Parameter(2) public List idsExpectedAfter102; - // 2XX are for testing bulk insertion cases @Parameter(3) - public List idsExpectedAfter201; - - @Parameter(4) - public List idsExpectedAfter202; - - @Parameter(5) public EnqueueLocation options; - @Parameter(6) + @Parameter(4) public List queueInitial; + @Parameter(5) + public long idCurrentlyPlaying; + @Test public void testQueueOrderWhenDownloading2Items() { @@ -209,30 +192,20 @@ public class ItemEnqueuePositionCalculatorTest { // Test body + Playable currentlyPlaying = getCurrentlyPlaying(idCurrentlyPlaying); + // User clicks download on feed item 101 FeedItem tFI101 = setAsDownloading(101, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (1st download)", - calculator, 0, tFI101, queue, + calculator, tFI101, queue, currentlyPlaying, idsExpectedAfter101); // Then user clicks download on feed item 102 FeedItem tFI102 = setAsDownloading(102, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)", - calculator, 0, tFI102, queue, + calculator, tFI102, queue, currentlyPlaying, idsExpectedAfter102); - // Items 201 and 202 are added as part of a single DBWriter.addQueueItem() calls - - FeedItem tFI201 = setAsDownloading(201, stubDownloadStateProvider); - doAddToQueueAndAssertResult(message + " (bulk insertion, 1st item)", - calculator, 0, tFI201, queue, - idsExpectedAfter201); - - FeedItem tFI202 = setAsDownloading(202, stubDownloadStateProvider); - doAddToQueueAndAssertResult(message + " (bulk insertion, 2nd item)", - calculator, 1, tFI202, queue, null, - idsExpectedAfter202); - // TODO: simulate download failure cases. } @@ -258,21 +231,11 @@ public class ItemEnqueuePositionCalculatorTest { static void doAddToQueueAndAssertResult(String message, ItemEnqueuePositionCalculator calculator, - int positionAmongAdd, - FeedItem itemToAdd, - List queue, - List idsExpected) { - doAddToQueueAndAssertResult(message, calculator, positionAmongAdd, itemToAdd, queue, null, idsExpected); - } - - static void doAddToQueueAndAssertResult(String message, - ItemEnqueuePositionCalculator calculator, - int positionAmongAdd, FeedItem itemToAdd, List queue, Playable currentlyPlaying, List idsExpected) { - int posActual = calculator.calcPosition(positionAmongAdd, itemToAdd, queue, currentlyPlaying); + int posActual = calculator.calcPosition(queue, currentlyPlaying); queue.add(posActual, itemToAdd); assertEquals(message, idsExpected, getIdList(queue)); } @@ -286,6 +249,24 @@ public class ItemEnqueuePositionCalculatorTest { QUEUE_DEFAULT.stream().map(fi -> fi.getId()).collect(Collectors.toList()); + static Playable getCurrentlyPlaying(long idCurrentlyPlaying) { + if (ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA == idCurrentlyPlaying) { + return externalMedia(); + } + if (ID_CURRENTLY_PLAYING_NULL == idCurrentlyPlaying) { + return null; + } + return createFeedItem(idCurrentlyPlaying).getMedia(); + } + + static Playable externalMedia() { + return new ExternalMedia("http://example.com/episode.mp3", MediaType.AUDIO); + } + + static final long ID_CURRENTLY_PLAYING_NULL = -1L; + static final long ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA = -9999L; + + static FeedItem createFeedItem(long id) { FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url", new Date(), FeedItem.PLAYED, FeedMother.anyFeed()); From 6e019f72de951acb21cec433a4da6cb64a103082 Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 5 Nov 2019 10:14:07 -0800 Subject: [PATCH 25/28] code style / comment tweak per review --- .../core/storage/ItemEnqueuePositionCalculator.java | 6 ++++-- .../core/storage/ItemEnqueuePositionCalculatorTest.java | 8 -------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java index 700c15eb6..4b28d36b5 100644 --- a/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java +++ b/core/src/main/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculator.java @@ -40,8 +40,10 @@ class ItemEnqueuePositionCalculator { case BACK: return curQueue.size(); case FRONT: - // 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 not necessarily 0, so that when a list of items are downloaded and enqueued + // in succession of calls (e.g., users manually tapping download one by one), + // the items enqueued are kept the same order. + // Simply returning 0 will reverse the order. return getPositionOfFirstNonDownloadingItem(0, curQueue); case AFTER_CURRENTLY_PLAYING: int currentlyPlayingPosition = getCurrentlyPlayingPosition(curQueue, currentPlaying); diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index 68eb434a4..40ffd26b4 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -189,17 +189,13 @@ public class ItemEnqueuePositionCalculatorTest { // A shallow copy, as the test code will manipulate the queue List queue = new ArrayList<>(queueInitial); - // Test body - Playable currentlyPlaying = getCurrentlyPlaying(idCurrentlyPlaying); - // User clicks download on feed item 101 FeedItem tFI101 = setAsDownloading(101, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (1st download)", calculator, tFI101, queue, currentlyPlaying, idsExpectedAfter101); - // Then user clicks download on feed item 102 FeedItem tFI102 = setAsDownloading(102, stubDownloadStateProvider); doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)", @@ -225,10 +221,6 @@ public class ItemEnqueuePositionCalculatorTest { } - // Common helpers: - // - common queue (of items) for tests - // - construct FeedItems for tests - static void doAddToQueueAndAssertResult(String message, ItemEnqueuePositionCalculator calculator, FeedItem itemToAdd, From 9d6db7b9fc2cbdd66f43dcb61aea9c350a6bcf9f Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 5 Nov 2019 10:38:31 -0800 Subject: [PATCH 26/28] enqueue respect download order: add test case for download failures. --- .../ItemEnqueuePositionCalculatorTest.java | 38 +++++++++++++++---- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java index 40ffd26b4..17b88bdd2 100644 --- a/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java +++ b/core/src/test/java/de/danoeh/antennapod/core/storage/ItemEnqueuePositionCalculatorTest.java @@ -145,14 +145,20 @@ public class ItemEnqueuePositionCalculatorTest { {"download order test, enqueue default", concat(QUEUE_DEFAULT_IDS, 101L), concat(QUEUE_DEFAULT_IDS, list(101L, 102L)), + concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 103L)), BACK, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NULL}, {"download order test, enqueue at front (currently playing has no effect)", concat(101L, QUEUE_DEFAULT_IDS), concat(list(101L, 102L), QUEUE_DEFAULT_IDS), + concat(list(101L, 103L, 102L), QUEUE_DEFAULT_IDS), + // ^ 103 is put ahead of 102, after 102 failed. + // It is a limitation as the logic can't tell 102 download has failed + // (as opposed to simply being enqueued) FRONT, QUEUE_DEFAULT, 11L}, // 11 is at the front, currently playing {"download order test, enqueue after currently playing", list(11L, 101L, 12L, 13L, 14L), list(11L, 101L, 102L, 12L, 13L, 14L), + list(11L, 101L, 103L, 102L, 12L, 13L, 14L), AFTER_CURRENTLY_PLAYING, QUEUE_DEFAULT, 11L} // 11 is at the front, currently playing }); } @@ -167,12 +173,15 @@ public class ItemEnqueuePositionCalculatorTest { public List idsExpectedAfter102; @Parameter(3) - public EnqueueLocation options; + public List idsExpectedAfter103; @Parameter(4) - public List queueInitial; + public EnqueueLocation options; @Parameter(5) + public List queueInitial; + + @Parameter(6) public long idCurrentlyPlaying; @Test @@ -192,32 +201,45 @@ public class ItemEnqueuePositionCalculatorTest { // Test body Playable currentlyPlaying = getCurrentlyPlaying(idCurrentlyPlaying); // User clicks download on feed item 101 - FeedItem tFI101 = setAsDownloading(101, stubDownloadStateProvider); + FeedItem tFI101 = setAsDownloading(101, stubDownloadStateProvider, true); doAddToQueueAndAssertResult(message + " (1st download)", calculator, tFI101, queue, currentlyPlaying, idsExpectedAfter101); // Then user clicks download on feed item 102 - FeedItem tFI102 = setAsDownloading(102, stubDownloadStateProvider); + FeedItem tFI102 = setAsDownloading(102, stubDownloadStateProvider, true); doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)", calculator, tFI102, queue, currentlyPlaying, idsExpectedAfter102); + // simulate download failure case for 102 + setAsDownloading(tFI102, stubDownloadStateProvider, false); + // Then user clicks download on feed item 103 + FeedItem tFI103 = setAsDownloading(103, stubDownloadStateProvider, true); + doAddToQueueAndAssertResult(message + + " (3rd download, with 2nd download failed; " + + "it should be behind 1st download (unless enqueueLocation is BACK)", + calculator, tFI103, queue, currentlyPlaying, + idsExpectedAfter103); - // TODO: simulate download failure cases. } - private static FeedItem setAsDownloading(int id, DownloadStateProvider stubDownloadStateProvider) { + private static FeedItem setAsDownloading(int id, DownloadStateProvider stubDownloadStateProvider, + boolean isDownloading) { FeedItem item = createFeedItem(id); FeedMedia media = new FeedMedia(item, "http://download.url.net/" + id , 100000 + id, "audio/mp3"); media.setId(item.getId()); item.setMedia(media); + return setAsDownloading(item, stubDownloadStateProvider, isDownloading); + } - stub(stubDownloadStateProvider.isDownloadingFile(media)).toReturn(true); - + private static FeedItem setAsDownloading(FeedItem item, DownloadStateProvider stubDownloadStateProvider, + boolean isDownloading) { + stub(stubDownloadStateProvider.isDownloadingFile(item.getMedia())).toReturn(isDownloading); return item; } + } From 9e8904bbcaac2224d332e36d79e728f5ba3a095b Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 5 Nov 2019 11:34:36 -0800 Subject: [PATCH 27/28] code style fixes --- .../java/de/test/antennapod/storage/DBWriterTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/androidTest/java/de/test/antennapod/storage/DBWriterTest.java b/app/src/androidTest/java/de/test/antennapod/storage/DBWriterTest.java index 3bc650da0..1e13bd5c3 100644 --- a/app/src/androidTest/java/de/test/antennapod/storage/DBWriterTest.java +++ b/app/src/androidTest/java/de/test/antennapod/storage/DBWriterTest.java @@ -476,12 +476,12 @@ public class DBWriterTest { assertFalse(OLD_DATE == media.getPlaybackCompletionDate().getTime()); } - private Feed queueTestSetupMultipleItems(final int NUM_ITEMS) throws InterruptedException, ExecutionException, TimeoutException { + private Feed queueTestSetupMultipleItems(final int numItems) throws InterruptedException, ExecutionException, TimeoutException { final Context context = getInstrumentation().getTargetContext(); UserPreferences.setEnqueueLocation(UserPreferences.EnqueueLocation.BACK); Feed feed = new Feed("url", null, "title"); feed.setItems(new ArrayList<>()); - for (int i = 0; i < NUM_ITEMS; i++) { + for (int i = 0; i < numItems; i++) { FeedItem item = new FeedItem(0, "title " + i, "id " + i, "link " + i, new Date(), FeedItem.PLAYED, feed); feed.getItems().add(item); } From 89d76702c033eb81e92699cda3f058c2fc3c7d8c Mon Sep 17 00:00:00 2001 From: orionlee Date: Tue, 5 Nov 2019 11:39:36 -0800 Subject: [PATCH 28/28] code style - reduce nested ifs --- .../antennapod/core/storage/DBWriter.java | 91 +++++++++---------- 1 file changed, 44 insertions(+), 47 deletions(-) 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 aff88ed80..23d14fe87 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 @@ -6,12 +6,7 @@ import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.Nullable; -import androidx.annotation.VisibleForTesting; -import de.danoeh.antennapod.core.event.DownloadLogEvent; -import de.danoeh.antennapod.core.event.FeedListUpdateEvent; -import de.danoeh.antennapod.core.event.PlaybackHistoryEvent; -import de.danoeh.antennapod.core.event.UnreadItemsUpdateEvent; import org.greenrobot.eventbus.EventBus; import java.io.File; @@ -26,10 +21,14 @@ import java.util.concurrent.Future; import de.danoeh.antennapod.core.ClientConfig; import de.danoeh.antennapod.core.R; +import de.danoeh.antennapod.core.event.DownloadLogEvent; import de.danoeh.antennapod.core.event.FavoritesEvent; import de.danoeh.antennapod.core.event.FeedItemEvent; +import de.danoeh.antennapod.core.event.FeedListUpdateEvent; import de.danoeh.antennapod.core.event.MessageEvent; +import de.danoeh.antennapod.core.event.PlaybackHistoryEvent; import de.danoeh.antennapod.core.event.QueueEvent; +import de.danoeh.antennapod.core.event.UnreadItemsUpdateEvent; import de.danoeh.antennapod.core.feed.Feed; import de.danoeh.antennapod.core.feed.FeedEvent; import de.danoeh.antennapod.core.feed.FeedItem; @@ -316,55 +315,53 @@ public class DBWriter { public static Future addQueueItem(final Context context, final boolean performAutoDownload, final long... itemIds) { return dbExec.submit(() -> { - if (itemIds.length > 0) { - final PodDBAdapter adapter = PodDBAdapter.getInstance(); - adapter.open(); - final List queue = DBReader.getQueue(adapter); + if (itemIds.length < 1) { + return; + } - if (queue != null) { - boolean queueModified = false; - LongList markAsUnplayedIds = new LongList(); - List events = new ArrayList<>(); - List updatedItems = new ArrayList<>(); - ItemEnqueuePositionCalculator positionCalculator = - new ItemEnqueuePositionCalculator(UserPreferences.getEnqueueLocation()); - Playable currentlyPlaying = Playable.PlayableUtils.createInstanceFromPreferences(context); - int insertPosition = positionCalculator.calcPosition(queue, currentlyPlaying); - for (long itemId : itemIds) { - if (!itemListContains(queue, itemId)) { - final FeedItem item = DBReader.getFeedItem(itemId); + final PodDBAdapter adapter = PodDBAdapter.getInstance(); + adapter.open(); + final List queue = DBReader.getQueue(adapter); + boolean queueModified = false; + LongList markAsUnplayedIds = new LongList(); + List events = new ArrayList<>(); + List updatedItems = new ArrayList<>(); + ItemEnqueuePositionCalculator positionCalculator = + new ItemEnqueuePositionCalculator(UserPreferences.getEnqueueLocation()); + Playable currentlyPlaying = Playable.PlayableUtils.createInstanceFromPreferences(context); + int insertPosition = positionCalculator.calcPosition(queue, currentlyPlaying); + for (long itemId : itemIds) { + if (!itemListContains(queue, itemId)) { + final FeedItem item = DBReader.getFeedItem(itemId); + if (item != null) { + queue.add(insertPosition, item); + events.add(QueueEvent.added(item, insertPosition)); - if (item != null) { - queue.add(insertPosition, item); - events.add(QueueEvent.added(item, insertPosition)); - - item.addTag(FeedItem.TAG_QUEUE); - updatedItems.add(item); - queueModified = true; - if (item.isNew()) { - markAsUnplayedIds.add(item.getId()); - } - insertPosition++; - } - } - } - if (queueModified) { - applySortOrder(queue, events); - adapter.setQueue(queue); - for (QueueEvent event : events) { - EventBus.getDefault().post(event); - } - EventBus.getDefault().post(FeedItemEvent.updated(updatedItems)); - if (markAsUnplayedIds.size() > 0) { - DBWriter.markItemPlayed(FeedItem.UNPLAYED, markAsUnplayedIds.toArray()); + item.addTag(FeedItem.TAG_QUEUE); + updatedItems.add(item); + queueModified = true; + if (item.isNew()) { + markAsUnplayedIds.add(item.getId()); } + insertPosition++; } } - adapter.close(); - if (performAutoDownload) { - DBTasks.autodownloadUndownloadedItems(context); + } + if (queueModified) { + applySortOrder(queue, events); + adapter.setQueue(queue); + for (QueueEvent event : events) { + EventBus.getDefault().post(event); } + EventBus.getDefault().post(FeedItemEvent.updated(updatedItems)); + if (markAsUnplayedIds.size() > 0) { + DBWriter.markItemPlayed(FeedItem.UNPLAYED, markAsUnplayedIds.toArray()); + } + } + adapter.close(); + if (performAutoDownload) { + DBTasks.autodownloadUndownloadedItems(context); } }); }