refactor - make enqueue position logic more readable per review.
This commit is contained in:
parent
f8fbc8e649
commit
b80973bc30
|
@ -329,13 +329,13 @@ public class DBWriter {
|
||||||
ItemEnqueuePositionCalculator positionCalculator =
|
ItemEnqueuePositionCalculator positionCalculator =
|
||||||
new ItemEnqueuePositionCalculator(UserPreferences.getEnqueueLocation());
|
new ItemEnqueuePositionCalculator(UserPreferences.getEnqueueLocation());
|
||||||
Playable currentlyPlaying = Playable.PlayableUtils.createInstanceFromPreferences(context);
|
Playable currentlyPlaying = Playable.PlayableUtils.createInstanceFromPreferences(context);
|
||||||
for (int i = 0; i < itemIds.length; i++) {
|
int insertPosition = positionCalculator.calcPosition(queue, currentlyPlaying);
|
||||||
if (!itemListContains(queue, itemIds[i])) {
|
for (long itemId : itemIds) {
|
||||||
final FeedItem item = DBReader.getFeedItem(itemIds[i]);
|
if (!itemListContains(queue, itemId)) {
|
||||||
|
final FeedItem item = DBReader.getFeedItem(itemId);
|
||||||
|
|
||||||
|
|
||||||
if (item != null) {
|
if (item != null) {
|
||||||
int insertPosition = positionCalculator.calcPosition(i, item, queue, currentlyPlaying);
|
|
||||||
queue.add(insertPosition, item);
|
queue.add(insertPosition, item);
|
||||||
events.add(QueueEvent.added(item, insertPosition));
|
events.add(QueueEvent.added(item, insertPosition));
|
||||||
|
|
||||||
|
@ -345,6 +345,7 @@ public class DBWriter {
|
||||||
if (item.isNew()) {
|
if (item.isNew()) {
|
||||||
markAsUnplayedIds.add(item.getId());
|
markAsUnplayedIds.add(item.getId());
|
||||||
}
|
}
|
||||||
|
insertPosition++;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -30,28 +30,23 @@ class ItemEnqueuePositionCalculator {
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @param positionAmongToAdd Typically, the callers has a list of items to be inserted to
|
* Determine the position (0-based) that the item(s) should be inserted to the named queue.
|
||||||
* 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
|
* @param curQueue the queue to which the item is to be inserted
|
||||||
* @param currentPlaying the currently playing media
|
* @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,
|
public int calcPosition(@NonNull List<FeedItem> curQueue, @Nullable Playable currentPlaying) {
|
||||||
@NonNull List<FeedItem> curQueue, @Nullable Playable currentPlaying) {
|
|
||||||
switch (enqueueLocation) {
|
switch (enqueueLocation) {
|
||||||
case BACK:
|
case BACK:
|
||||||
return curQueue.size();
|
return curQueue.size();
|
||||||
case FRONT:
|
case FRONT:
|
||||||
// return NOT 0, so that when a list of items are inserted, the items inserted
|
// 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
|
// keep the same order. Returning 0 will reverse the order
|
||||||
return getPositionOfFirstNonDownloadingItem(positionAmongToAdd, curQueue);
|
return getPositionOfFirstNonDownloadingItem(0, curQueue);
|
||||||
case AFTER_CURRENTLY_PLAYING:
|
case AFTER_CURRENTLY_PLAYING:
|
||||||
int currentlyPlayingPosition = getCurrentlyPlayingPosition(curQueue, currentPlaying);
|
int currentlyPlayingPosition = getCurrentlyPlayingPosition(curQueue, currentPlaying);
|
||||||
return getPositionOfFirstNonDownloadingItem(
|
return getPositionOfFirstNonDownloadingItem(
|
||||||
currentlyPlayingPosition + (1 + positionAmongToAdd), curQueue);
|
currentlyPlayingPosition + 1, curQueue);
|
||||||
default:
|
default:
|
||||||
throw new AssertionError("calcPosition() : unrecognized enqueueLocation option: " + enqueueLocation);
|
throw new AssertionError("calcPosition() : unrecognized enqueueLocation option: " + enqueueLocation);
|
||||||
}
|
}
|
||||||
|
|
|
@ -41,22 +41,16 @@ public class ItemEnqueuePositionCalculatorTest {
|
||||||
return Arrays.asList(new Object[][]{
|
return Arrays.asList(new Object[][]{
|
||||||
{"case default, i.e., add to the end",
|
{"case default, i.e., add to the end",
|
||||||
concat(QUEUE_DEFAULT_IDS, TFI_ID),
|
concat(QUEUE_DEFAULT_IDS, TFI_ID),
|
||||||
BACK, 0, QUEUE_DEFAULT},
|
BACK, QUEUE_DEFAULT},
|
||||||
{"case default (2nd item)",
|
|
||||||
concat(QUEUE_DEFAULT_IDS, TFI_ID),
|
|
||||||
BACK, 1, QUEUE_DEFAULT},
|
|
||||||
{"case option enqueue at front",
|
{"case option enqueue at front",
|
||||||
concat(TFI_ID, QUEUE_DEFAULT_IDS),
|
concat(TFI_ID, QUEUE_DEFAULT_IDS),
|
||||||
FRONT, 0, QUEUE_DEFAULT},
|
FRONT, QUEUE_DEFAULT},
|
||||||
{"case option enqueue at front (2nd item)",
|
|
||||||
list(11L, TFI_ID, 12L, 13L, 14L),
|
|
||||||
FRONT, 1, QUEUE_DEFAULT},
|
|
||||||
{"case empty queue, option default",
|
{"case empty queue, option default",
|
||||||
list(TFI_ID),
|
list(TFI_ID),
|
||||||
BACK, 0, QUEUE_EMPTY},
|
BACK, QUEUE_EMPTY},
|
||||||
{"case empty queue, option enqueue at front",
|
{"case empty queue, option enqueue at front",
|
||||||
list(TFI_ID),
|
list(TFI_ID),
|
||||||
FRONT, 0, QUEUE_EMPTY},
|
FRONT, QUEUE_EMPTY},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -70,9 +64,6 @@ public class ItemEnqueuePositionCalculatorTest {
|
||||||
public EnqueueLocation options;
|
public EnqueueLocation options;
|
||||||
|
|
||||||
@Parameter(3)
|
@Parameter(3)
|
||||||
public int posAmongAdded; // the position of feed item to be inserted among the list to be inserted.
|
|
||||||
|
|
||||||
@Parameter(4)
|
|
||||||
public List<FeedItem> curQueue;
|
public List<FeedItem> curQueue;
|
||||||
|
|
||||||
public static final long TFI_ID = 101;
|
public static final long TFI_ID = 101;
|
||||||
|
@ -88,7 +79,7 @@ public class ItemEnqueuePositionCalculatorTest {
|
||||||
List<FeedItem> queue = new ArrayList<>(curQueue);
|
List<FeedItem> queue = new ArrayList<>(curQueue);
|
||||||
FeedItem tFI = createFeedItem(TFI_ID);
|
FeedItem tFI = createFeedItem(TFI_ID);
|
||||||
doAddToQueueAndAssertResult(message,
|
doAddToQueueAndAssertResult(message,
|
||||||
calculator, posAmongAdded, tFI, queue, getCurrentlyPlaying(),
|
calculator, tFI, queue, getCurrentlyPlaying(),
|
||||||
idsExpected);
|
idsExpected);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -102,40 +93,31 @@ public class ItemEnqueuePositionCalculatorTest {
|
||||||
return Arrays.asList(new Object[][]{
|
return Arrays.asList(new Object[][]{
|
||||||
{"case option after currently playing",
|
{"case option after currently playing",
|
||||||
list(11L, TFI_ID, 12L, 13L, 14L),
|
list(11L, TFI_ID, 12L, 13L, 14L),
|
||||||
AFTER_CURRENTLY_PLAYING, 0, QUEUE_DEFAULT, 11L},
|
AFTER_CURRENTLY_PLAYING, 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",
|
{"case option after currently playing, currently playing in the middle of the queue",
|
||||||
list(11L, 12L, 13L, TFI_ID, 14L),
|
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",
|
{"case option after currently playing, currently playing is not in queue",
|
||||||
concat(TFI_ID, QUEUE_DEFAULT_IDS),
|
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",
|
{"case option after currently playing, no currentlyPlaying is null",
|
||||||
concat(TFI_ID, QUEUE_DEFAULT_IDS),
|
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",
|
{"case option after currently playing, currentlyPlaying is externalMedia",
|
||||||
concat(TFI_ID, QUEUE_DEFAULT_IDS),
|
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",
|
{"case empty queue, option after currently playing",
|
||||||
list(TFI_ID),
|
list(TFI_ID),
|
||||||
AFTER_CURRENTLY_PLAYING, 0, QUEUE_EMPTY, ID_CURRENTLY_PLAYING_NULL},
|
AFTER_CURRENTLY_PLAYING, QUEUE_EMPTY, ID_CURRENTLY_PLAYING_NULL},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
@Parameter(5)
|
@Parameter(4)
|
||||||
public long idCurrentlyPlaying = -1;
|
public long idCurrentlyPlaying;
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
Playable getCurrentlyPlaying() {
|
Playable getCurrentlyPlaying() {
|
||||||
if (ID_CURRENTLY_PLAYING_NOT_FEEDMEDIA == idCurrentlyPlaying) {
|
return ItemEnqueuePositionCalculatorTest.getCurrentlyPlaying(idCurrentlyPlaying);
|
||||||
return externalMedia();
|
|
||||||
}
|
|
||||||
if (ID_CURRENTLY_PLAYING_NULL == idCurrentlyPlaying) {
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
return createFeedItem(idCurrentlyPlaying).getMedia();
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private static Playable externalMedia() {
|
private static Playable externalMedia() {
|
||||||
|
@ -150,6 +132,11 @@ public class ItemEnqueuePositionCalculatorTest {
|
||||||
@RunWith(Parameterized.class)
|
@RunWith(Parameterized.class)
|
||||||
public static class ItemEnqueuePositionCalculatorPreserveDownloadOrderTest {
|
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}>")
|
@Parameters(name = "{index}: case<{0}>")
|
||||||
public static Iterable<Object[]> data() {
|
public static Iterable<Object[]> data() {
|
||||||
// Attempts to make test more readable by showing the expected list of ids
|
// 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",
|
{"download order test, enqueue default",
|
||||||
concat(QUEUE_DEFAULT_IDS, 101L),
|
concat(QUEUE_DEFAULT_IDS, 101L),
|
||||||
concat(QUEUE_DEFAULT_IDS, list(101L, 102L)),
|
concat(QUEUE_DEFAULT_IDS, list(101L, 102L)),
|
||||||
concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L)),
|
BACK, QUEUE_DEFAULT, ID_CURRENTLY_PLAYING_NULL},
|
||||||
concat(QUEUE_DEFAULT_IDS, list(101L, 102L, 201L, 202L)),
|
{"download order test, enqueue at front (currently playing has no effect)",
|
||||||
BACK, QUEUE_DEFAULT},
|
|
||||||
{"download order test, enqueue at front",
|
|
||||||
concat(101L, QUEUE_DEFAULT_IDS),
|
concat(101L, QUEUE_DEFAULT_IDS),
|
||||||
concat(list(101L, 102L), QUEUE_DEFAULT_IDS),
|
concat(list(101L, 102L), QUEUE_DEFAULT_IDS),
|
||||||
concat(list(101L, 102L, 201L), QUEUE_DEFAULT_IDS),
|
FRONT, QUEUE_DEFAULT, 11L}, // 11 is at the front, currently playing
|
||||||
concat(list(101L, 102L, 201L, 202L), QUEUE_DEFAULT_IDS),
|
{"download order test, enqueue after currently playing",
|
||||||
FRONT, QUEUE_DEFAULT},
|
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)
|
@Parameter(2)
|
||||||
public List<Long> idsExpectedAfter102;
|
public List<Long> idsExpectedAfter102;
|
||||||
|
|
||||||
// 2XX are for testing bulk insertion cases
|
|
||||||
@Parameter(3)
|
@Parameter(3)
|
||||||
public List<Long> idsExpectedAfter201;
|
|
||||||
|
|
||||||
@Parameter(4)
|
|
||||||
public List<Long> idsExpectedAfter202;
|
|
||||||
|
|
||||||
@Parameter(5)
|
|
||||||
public EnqueueLocation options;
|
public EnqueueLocation options;
|
||||||
|
|
||||||
@Parameter(6)
|
@Parameter(4)
|
||||||
public List<FeedItem> queueInitial;
|
public List<FeedItem> queueInitial;
|
||||||
|
|
||||||
|
@Parameter(5)
|
||||||
|
public long idCurrentlyPlaying;
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void testQueueOrderWhenDownloading2Items() {
|
public void testQueueOrderWhenDownloading2Items() {
|
||||||
|
|
||||||
|
@ -209,30 +192,20 @@ public class ItemEnqueuePositionCalculatorTest {
|
||||||
|
|
||||||
// Test body
|
// Test body
|
||||||
|
|
||||||
|
Playable currentlyPlaying = getCurrentlyPlaying(idCurrentlyPlaying);
|
||||||
|
|
||||||
// User clicks download on feed item 101
|
// User clicks download on feed item 101
|
||||||
FeedItem tFI101 = setAsDownloading(101, stubDownloadStateProvider);
|
FeedItem tFI101 = setAsDownloading(101, stubDownloadStateProvider);
|
||||||
doAddToQueueAndAssertResult(message + " (1st download)",
|
doAddToQueueAndAssertResult(message + " (1st download)",
|
||||||
calculator, 0, tFI101, queue,
|
calculator, tFI101, queue, currentlyPlaying,
|
||||||
idsExpectedAfter101);
|
idsExpectedAfter101);
|
||||||
|
|
||||||
// Then user clicks download on feed item 102
|
// Then user clicks download on feed item 102
|
||||||
FeedItem tFI102 = setAsDownloading(102, stubDownloadStateProvider);
|
FeedItem tFI102 = setAsDownloading(102, stubDownloadStateProvider);
|
||||||
doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)",
|
doAddToQueueAndAssertResult(message + " (2nd download, it should preserve order of download)",
|
||||||
calculator, 0, tFI102, queue,
|
calculator, tFI102, queue, currentlyPlaying,
|
||||||
idsExpectedAfter102);
|
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.
|
// TODO: simulate download failure cases.
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -258,21 +231,11 @@ public class ItemEnqueuePositionCalculatorTest {
|
||||||
|
|
||||||
static void doAddToQueueAndAssertResult(String message,
|
static void doAddToQueueAndAssertResult(String message,
|
||||||
ItemEnqueuePositionCalculator calculator,
|
ItemEnqueuePositionCalculator calculator,
|
||||||
int positionAmongAdd,
|
|
||||||
FeedItem itemToAdd,
|
|
||||||
List<FeedItem> queue,
|
|
||||||
List<Long> idsExpected) {
|
|
||||||
doAddToQueueAndAssertResult(message, calculator, positionAmongAdd, itemToAdd, queue, null, idsExpected);
|
|
||||||
}
|
|
||||||
|
|
||||||
static void doAddToQueueAndAssertResult(String message,
|
|
||||||
ItemEnqueuePositionCalculator calculator,
|
|
||||||
int positionAmongAdd,
|
|
||||||
FeedItem itemToAdd,
|
FeedItem itemToAdd,
|
||||||
List<FeedItem> queue,
|
List<FeedItem> queue,
|
||||||
Playable currentlyPlaying,
|
Playable currentlyPlaying,
|
||||||
List<Long> idsExpected) {
|
List<Long> idsExpected) {
|
||||||
int posActual = calculator.calcPosition(positionAmongAdd, itemToAdd, queue, currentlyPlaying);
|
int posActual = calculator.calcPosition(queue, currentlyPlaying);
|
||||||
queue.add(posActual, itemToAdd);
|
queue.add(posActual, itemToAdd);
|
||||||
assertEquals(message, idsExpected, getIdList(queue));
|
assertEquals(message, idsExpected, getIdList(queue));
|
||||||
}
|
}
|
||||||
|
@ -286,6 +249,24 @@ public class ItemEnqueuePositionCalculatorTest {
|
||||||
QUEUE_DEFAULT.stream().map(fi -> fi.getId()).collect(Collectors.toList());
|
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) {
|
static FeedItem createFeedItem(long id) {
|
||||||
FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url",
|
FeedItem item = new FeedItem(id, "Item" + id, "ItemId" + id, "url",
|
||||||
new Date(), FeedItem.PLAYED, FeedMother.anyFeed());
|
new Date(), FeedItem.PLAYED, FeedMother.anyFeed());
|
||||||
|
|
Loading…
Reference in New Issue