Merge pull request #6345 from Imericxu/test-and-update-playqueue

Test and clean up PlayQueue

app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java

@@ -40,29 +40,25 @@ import io.reactivex.rxjava3.subjects.BehaviorSubject;
  */
 public abstract class PlayQueue implements Serializable {
     public static final boolean DEBUG = MainActivity.DEBUG;
-
-    private ArrayList<PlayQueueItem> backup;
-    private ArrayList<PlayQueueItem> streams;
-
     @NonNull
     private final AtomicInteger queueIndex;
-    private final ArrayList<PlayQueueItem> history;
+    private final List<PlayQueueItem> history = new ArrayList<>();
+
+    private List<PlayQueueItem> backup;
+    private List<PlayQueueItem> streams;
 
     private transient BehaviorSubject<PlayQueueEvent> eventBroadcast;
     private transient Flowable<PlayQueueEvent> broadcastReceiver;
-
+    private transient boolean disposed = false;
-    private transient boolean disposed;
 
     PlayQueue(final int index, final List<PlayQueueItem> startWith) {
-        streams = new ArrayList<>();
+        streams = new ArrayList<>(startWith);
-        streams.addAll(startWith);
+
-        history = new ArrayList<>();
         if (streams.size() > index) {
             history.add(streams.get(index));
         }
 
         queueIndex = new AtomicInteger(index);
-        disposed = false;
     }
 
     /*//////////////////////////////////////////////////////////////////////////
@@ -137,18 +133,36 @@ public abstract class PlayQueue implements Serializable {
     public synchronized void setIndex(final int index) {
         final int oldIndex = getIndex();
 
-        int newIndex = index;
+        final int newIndex;
+
         if (index < 0) {
             newIndex = 0;
+        } else if (index < streams.size()) {
+            // Regular assignment for index in bounds
+            newIndex = index;
+        } else if (streams.isEmpty()) {
+            // Out of bounds from here on
+            // Need to check if stream is empty to prevent arithmetic error and negative index
+            newIndex = 0;
+        } else if (isComplete()) {
+            // Circular indexing
+            newIndex = index % streams.size();
+        } else {
+            // Index of last element
+            newIndex = streams.size() - 1;
         }
-        if (index >= streams.size()) {
+
-            newIndex = isComplete() ? index % streams.size() : streams.size() - 1;
+        queueIndex.set(newIndex);
-        }
+
         if (oldIndex != newIndex) {
             history.add(streams.get(newIndex));
         }
 
-        queueIndex.set(newIndex);
+        /*
+        TODO: Documentation states that a SelectEvent will only be emitted if the new index is...
+        different from the old one but this is emitted regardless? Not sure what this what it does
+        exactly so I won't touch it
+         */
         broadcast(new SelectEvent(oldIndex, newIndex));
     }
 
@@ -180,8 +194,6 @@ public abstract class PlayQueue implements Serializable {
      * @return the index of the given item
      */
     public int indexOf(@NonNull final PlayQueueItem item) {
-        // referential equality, can't think of a better way to do this
-        // todo: better than this
         return streams.indexOf(item);
     }
 
@@ -410,34 +422,42 @@ public abstract class PlayQueue implements Serializable {
     }
 
     /**
-     * Shuffles the current play queue.
+     * Shuffles the current play queue
      * <p>
-     * This method first backs up the existing play queue and item being played.
+     * This method first backs up the existing play queue and item being played. Then a newly
-     * Then a newly shuffled play queue will be generated along with currently
+     * shuffled play queue will be generated along with currently playing item placed at the
-     * playing item placed at the beginning of the queue.
+     * beginning of the queue. This item will also be added to the history.
      * </p>
      * <p>
-     * Will emit a {@link ReorderEvent} in any context.
+     * Will emit a {@link ReorderEvent} if shuffled.
      * </p>
+     *
+     * @implNote Does nothing if the queue has a size <= 2 (the currently playing video must stay on
+     * top, so shuffling a size-2 list does nothing)
      */
     public synchronized void shuffle() {
+        // Can't shuffle an list that's empty or only has one element
+        if (size() <= 2) {
+            return;
+        }
+        // Create a backup if it doesn't already exist
         if (backup == null) {
             backup = new ArrayList<>(streams);
         }
-        final int originIndex = getIndex();
+
-        final PlayQueueItem current = getItem();
+        final int originalIndex = getIndex();
+        final PlayQueueItem currentItem = getItem();
+
         Collections.shuffle(streams);
 
-        final int newIndex = streams.indexOf(current);
+        // Move currentItem to the head of the queue
-        if (newIndex != -1) {
+        streams.remove(currentItem);
-            streams.add(0, streams.remove(newIndex));
+        streams.add(0, currentItem);
-        }
         queueIndex.set(0);
-        if (streams.size() > 0) {
-            history.add(streams.get(0));
-        }
 
-        broadcast(new ReorderEvent(originIndex, queueIndex.get()));
+        history.add(currentItem);
+
+        broadcast(new ReorderEvent(originalIndex, 0));
     }
 
     /**
@@ -457,7 +477,6 @@ public abstract class PlayQueue implements Serializable {
         final int originIndex = getIndex();
         final PlayQueueItem current = getItem();
 
-        streams.clear();
         streams = backup;
         backup = null;
 
@@ -500,22 +519,19 @@ public abstract class PlayQueue implements Serializable {
      * we don't have to do anything with new queue.
      * This method also gives a chance to track history of items in a queue in
      * VideoDetailFragment without duplicating items from two identical queues
-     * */
+     */
     @Override
     public boolean equals(@Nullable final Object obj) {
-        if (!(obj instanceof PlayQueue)
+        if (!(obj instanceof PlayQueue)) {
-                || getStreams().size() != ((PlayQueue) obj).getStreams().size()) {
             return false;
         }
-
         final PlayQueue other = (PlayQueue) obj;
-        for (int i = 0; i < getStreams().size(); i++) {
+        return streams.equals(other.streams);
-            if (!getItem(i).getUrl().equals(other.getItem(i).getUrl())) {
+    }
-                return false;
-            }
-        }
 
-        return true;
+    @Override
+    public int hashCode() {
+        return streams.hashCode();
     }
 
     public boolean isDisposed() {

app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java

@@ -0,0 +1,183 @@
+package org.schabi.newpipe.player.playqueue;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.schabi.newpipe.extractor.stream.StreamInfoItem;
+import org.schabi.newpipe.extractor.stream.StreamType;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.spy;
+
+@SuppressWarnings("checkstyle:HideUtilityClassConstructor")
+public class PlayQueueTest {
+    static PlayQueue makePlayQueue(final int index, final List<PlayQueueItem> streams) {
+        // I tried using Mockito, but it didn't work for some reason
+        return new PlayQueue(index, streams) {
+            @Override
+            public boolean isComplete() {
+                throw new UnsupportedOperationException();
+            }
+
+            @Override
+            public void fetch() {
+                throw new UnsupportedOperationException();
+            }
+        };
+    }
+
+    static PlayQueueItem makeItemWithUrl(final String url) {
+        final StreamInfoItem infoItem = new StreamInfoItem(
+                0, url, "", StreamType.VIDEO_STREAM
+        );
+        return new PlayQueueItem(infoItem);
+    }
+
+    public static class SetIndexTests {
+        private static final int SIZE = 5;
+        private PlayQueue nonEmptyQueue;
+        private PlayQueue emptyQueue;
+
+        @Before
+        public void setup() {
+            final List<PlayQueueItem> streams = new ArrayList<>(5);
+            for (int i = 0; i < 5; ++i) {
+                streams.add(makeItemWithUrl("URL_" + i));
+            }
+            nonEmptyQueue = spy(makePlayQueue(0, streams));
+            emptyQueue = spy(makePlayQueue(0, new ArrayList<>()));
+        }
+
+        @Test
+        public void negative() {
+            nonEmptyQueue.setIndex(-5);
+            assertEquals(0, nonEmptyQueue.getIndex());
+
+            emptyQueue.setIndex(-5);
+            assertEquals(0, nonEmptyQueue.getIndex());
+        }
+
+        @Test
+        public void inBounds() {
+            nonEmptyQueue.setIndex(2);
+            assertEquals(2, nonEmptyQueue.getIndex());
+
+            // emptyQueue not tested because 0 isn't technically inBounds
+        }
+
+        @Test
+        public void outOfBoundIsComplete() {
+            doReturn(true).when(nonEmptyQueue).isComplete();
+            nonEmptyQueue.setIndex(7);
+            assertEquals(2, nonEmptyQueue.getIndex());
+
+            doReturn(true).when(emptyQueue).isComplete();
+            emptyQueue.setIndex(2);
+            assertEquals(0, emptyQueue.getIndex());
+        }
+
+        @Test
+        public void outOfBoundsNotComplete() {
+            doReturn(false).when(nonEmptyQueue).isComplete();
+            nonEmptyQueue.setIndex(7);
+            assertEquals(SIZE - 1, nonEmptyQueue.getIndex());
+
+            doReturn(false).when(emptyQueue).isComplete();
+            emptyQueue.setIndex(2);
+            assertEquals(0, emptyQueue.getIndex());
+        }
+
+        @Test
+        public void indexZero() {
+            nonEmptyQueue.setIndex(0);
+            assertEquals(0, nonEmptyQueue.getIndex());
+
+            doReturn(true).when(emptyQueue).isComplete();
+            emptyQueue.setIndex(0);
+            assertEquals(0, emptyQueue.getIndex());
+
+            doReturn(false).when(emptyQueue).isComplete();
+            emptyQueue.setIndex(0);
+            assertEquals(0, emptyQueue.getIndex());
+        }
+
+        @Test
+        public void addToHistory() {
+            nonEmptyQueue.setIndex(0);
+            assertFalse(nonEmptyQueue.previous());
+
+            nonEmptyQueue.setIndex(3);
+            assertTrue(nonEmptyQueue.previous());
+            assertEquals("URL_0", Objects.requireNonNull(nonEmptyQueue.getItem()).getUrl());
+        }
+    }
+
+    public static class GetItemTests {
+        private static List<PlayQueueItem> streams;
+        private PlayQueue queue;
+
+        @BeforeClass
+        public static void init() {
+            streams = new ArrayList<>(Collections.nCopies(5, makeItemWithUrl("OTHER_URL")));
+            streams.set(3, makeItemWithUrl("TARGET_URL"));
+        }
+
+        @Before
+        public void setup() {
+            queue = makePlayQueue(0, streams);
+        }
+
+        @Test
+        public void inBounds() {
+            assertEquals("TARGET_URL", Objects.requireNonNull(queue.getItem(3)).getUrl());
+            assertEquals("OTHER_URL", Objects.requireNonNull(queue.getItem(1)).getUrl());
+        }
+
+        @Test
+        public void outOfBounds() {
+            assertNull(queue.getItem(-1));
+            assertNull(queue.getItem(5));
+        }
+    }
+
+    public static class EqualsTests {
+        private final PlayQueueItem item1 = makeItemWithUrl("URL_1");
+        private final PlayQueueItem item2 = makeItemWithUrl("URL_2");
+
+        @Test
+        public void sameStreams() {
+            final List<PlayQueueItem> streams = Collections.nCopies(5, item1);
+            final PlayQueue queue1 = makePlayQueue(0, streams);
+            final PlayQueue queue2 = makePlayQueue(0, streams);
+            assertEquals(queue1, queue2);
+        }
+
+        @Test
+        public void sameSizeDifferentItems() {
+            final List<PlayQueueItem> streams1 = Collections.nCopies(5, item1);
+            final List<PlayQueueItem> streams2 = Collections.nCopies(5, item2);
+            final PlayQueue queue1 = makePlayQueue(0, streams1);
+            final PlayQueue queue2 = makePlayQueue(0, streams2);
+            assertNotEquals(queue1, queue2);
+        }
+
+        @Test
+        public void differentSizeStreams() {
+            final List<PlayQueueItem> streams1 = Collections.nCopies(5, item1);
+            final List<PlayQueueItem> streams2 = Collections.nCopies(6, item2);
+            final PlayQueue queue1 = makePlayQueue(0, streams1);
+            final PlayQueue queue2 = makePlayQueue(0, streams2);
+            assertNotEquals(queue1, queue2);
+        }
+    }
+}