From b4b6726ec8c14b0c74748aaa6ed00d6ec0ddf5cf Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 00:33:11 -0400 Subject: [PATCH 01/20] Refactor and optimize equals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove multiple casts of obj - Simply use object equals on the streams because PlayQueueItem’s equals already compares urls --- .../schabi/newpipe/player/playqueue/PlayQueue.java | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index 6131d8565..7cfc9fba1 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -500,22 +500,14 @@ 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) - || getStreams().size() != ((PlayQueue) obj).getStreams().size()) { + if (!(obj instanceof PlayQueue)) { return false; } - final PlayQueue other = (PlayQueue) obj; - for (int i = 0; i < getStreams().size(); i++) { - if (!getItem(i).getUrl().equals(other.getItem(i).getUrl())) { - return false; - } - } - - return true; + return streams.equals(other.streams); } public boolean isDisposed() { From 8b2b3f8fe2262b1e634b5a26e3d6647c005b2faf Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 17:56:10 -0400 Subject: [PATCH 02/20] Refactor ArrayList fields to List --- .../java/org/schabi/newpipe/player/playqueue/PlayQueue.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index 7cfc9fba1..bafd7fd82 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -41,12 +41,12 @@ import io.reactivex.rxjava3.subjects.BehaviorSubject; public abstract class PlayQueue implements Serializable { public static final boolean DEBUG = MainActivity.DEBUG; - private ArrayList backup; - private ArrayList streams; + private List backup; + private List streams; @NonNull private final AtomicInteger queueIndex; - private final ArrayList history; + private final List history; private transient BehaviorSubject eventBroadcast; private transient Flowable broadcastReceiver; From 61e2ab4d80b8bc336d0ebce819e03655d3690f2a Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 14:12:35 -0400 Subject: [PATCH 03/20] Refactor streams to initialize with values --- .../java/org/schabi/newpipe/player/playqueue/PlayQueue.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index bafd7fd82..857cb5e72 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -54,8 +54,7 @@ public abstract class PlayQueue implements Serializable { private transient boolean disposed; PlayQueue(final int index, final List startWith) { - streams = new ArrayList<>(); - streams.addAll(startWith); + streams = new ArrayList<>(startWith); history = new ArrayList<>(); if (streams.size() > index) { history.add(streams.get(index)); From 24b02a2dbdb4edc375216055cd82d48980d4687d Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 17:53:40 -0400 Subject: [PATCH 04/20] Test PlayQueue equals --- .../player/playqueue/PlayQueueTest.java | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java new file mode 100644 index 000000000..7ae383510 --- /dev/null +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -0,0 +1,78 @@ +package org.schabi.newpipe.player.playqueue; + +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.runners.Enclosed; +import org.junit.runner.RunWith; +import org.schabi.newpipe.extractor.stream.StreamInfoItem; +import org.schabi.newpipe.extractor.stream.StreamType; + +import java.util.Collections; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; + +@SuppressWarnings("checkstyle:HideUtilityClassConstructor") +@RunWith(Enclosed.class) +public class PlayQueueTest { + public static PlayQueue mockPlayQueue(final int index, final List 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(); + } + }; + } + + public static class EqualsTests { + private PlayQueueItem item1; + private PlayQueueItem item2; + + @Before + public void setup() { + final String url1 = "www.website1.com"; + final String url2 = "www.website2.com"; + final StreamInfoItem info1 = new StreamInfoItem( + 0, url1, "", StreamType.VIDEO_STREAM + ); + final StreamInfoItem info2 = new StreamInfoItem( + 0, url2, "", StreamType.VIDEO_STREAM + ); + item1 = new PlayQueueItem(info1); + item2 = new PlayQueueItem(info2); + } + + @Test + public void sameStreams() { + final List streams = Collections.nCopies(5, item1); + final PlayQueue queue1 = mockPlayQueue(0, streams); + final PlayQueue queue2 = mockPlayQueue(0, streams); + assertEquals(queue1, queue2); + } + + @Test + public void sameSizeDifferentItems() { + final List streams1 = Collections.nCopies(5, item1); + final List streams2 = Collections.nCopies(5, item2); + final PlayQueue queue1 = mockPlayQueue(0, streams1); + final PlayQueue queue2 = mockPlayQueue(0, streams2); + assertNotEquals(queue1, queue2); + } + + @Test + public void differentSizeStreams() { + final List streams1 = Collections.nCopies(5, item1); + final List streams2 = Collections.nCopies(6, item2); + final PlayQueue queue1 = mockPlayQueue(0, streams1); + final PlayQueue queue2 = mockPlayQueue(0, streams2); + assertNotEquals(queue1, queue2); + } + } +} From 78ff6485a22352b0c9cb1ff3bfcbef9d371eed20 Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 18:00:46 -0400 Subject: [PATCH 05/20] Add hashCode() to match equals(other) --- .../java/org/schabi/newpipe/player/playqueue/PlayQueue.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index 857cb5e72..f7d70fc34 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -509,6 +509,11 @@ public abstract class PlayQueue implements Serializable { return streams.equals(other.streams); } + @Override + public int hashCode() { + return streams.hashCode(); + } + public boolean isDisposed() { return disposed; } From 55da6326416476551a6cc3bd902603c080224d1e Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 18:04:22 -0400 Subject: [PATCH 06/20] Refactor assignments to field declaration Assignments that don't require the constructor can be moved out. --- .../org/schabi/newpipe/player/playqueue/PlayQueue.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index f7d70fc34..a5fa949de 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -46,22 +46,21 @@ public abstract class PlayQueue implements Serializable { @NonNull private final AtomicInteger queueIndex; - private final List history; + private final List history = new ArrayList<>(); private transient BehaviorSubject eventBroadcast; private transient Flowable broadcastReceiver; - private transient boolean disposed; + private transient boolean disposed = false; PlayQueue(final int index, final List startWith) { streams = new ArrayList<>(startWith); - history = new ArrayList<>(); + if (streams.size() > index) { history.add(streams.get(index)); } queueIndex = new AtomicInteger(index); - disposed = false; } /*////////////////////////////////////////////////////////////////////////// From 91819306258ff7c0bee7166ce7adac4e758a4157 Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 22:54:15 -0400 Subject: [PATCH 07/20] Test PlayQueue.setIndex(...) --- .../player/playqueue/PlayQueueTest.java | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java index 7ae383510..e050d1a82 100644 --- a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -7,11 +7,15 @@ import org.junit.runner.RunWith; 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 static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; @SuppressWarnings("checkstyle:HideUtilityClassConstructor") @RunWith(Enclosed.class) @@ -31,6 +35,73 @@ public class PlayQueueTest { }; } + public static class SetIndexTests { + private static final int SIZE = 5; + private PlayQueue nonEmptyQueue; + private PlayQueue emptyQueue; + + @Before + public void setup() { + nonEmptyQueue = spy(mockPlayQueue( + 0, Collections.nCopies(SIZE, mock(PlayQueueItem.class)) + )); + emptyQueue = spy(mockPlayQueue(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()); + } + } + public static class EqualsTests { private PlayQueueItem item1; private PlayQueueItem item2; From 784c78c89e80d62495348363f37bd642131edd9e Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 23:33:23 -0400 Subject: [PATCH 08/20] Rewrite setIndex(int) to pass unit tests Original did not cover the case of when streams is empty and documentation does not specify any input restrictions. There's an ambiguity with broadcasting an event between the documentation and the actual code (see TODO). --- .../newpipe/player/playqueue/PlayQueue.java | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index a5fa949de..46627e12e 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -135,18 +135,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)); } From 158791043e20f507fdf99d107271e96e8026a17b Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 23:41:24 -0400 Subject: [PATCH 09/20] Resolve TODO in indexOf(...) PlayQueueItem overrides equals and hashCode, so using indexOf is perfectly fine. --- .../java/org/schabi/newpipe/player/playqueue/PlayQueue.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index 46627e12e..44b33e344 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -196,8 +196,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); } From 14e8fe51d0932be8a5bbbe35632895b971a48b76 Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Fri, 21 May 2021 23:53:45 -0400 Subject: [PATCH 10/20] Refactor making a PlayQueueItem to static method --- .../player/playqueue/PlayQueueTest.java | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java index e050d1a82..b3641547f 100644 --- a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -13,6 +13,7 @@ import java.util.List; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; @@ -35,6 +36,13 @@ public class PlayQueueTest { }; } + public 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; @@ -103,22 +111,8 @@ public class PlayQueueTest { } public static class EqualsTests { - private PlayQueueItem item1; - private PlayQueueItem item2; - - @Before - public void setup() { - final String url1 = "www.website1.com"; - final String url2 = "www.website2.com"; - final StreamInfoItem info1 = new StreamInfoItem( - 0, url1, "", StreamType.VIDEO_STREAM - ); - final StreamInfoItem info2 = new StreamInfoItem( - 0, url2, "", StreamType.VIDEO_STREAM - ); - item1 = new PlayQueueItem(info1); - item2 = new PlayQueueItem(info2); - } + private final PlayQueueItem item1 = makeItemWithUrl("URL_1"); + private final PlayQueueItem item2 = makeItemWithUrl("URL_2"); @Test public void sameStreams() { From f9582a3d598c195b1621d1e927337816f9bb6a35 Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Sat, 22 May 2021 00:00:39 -0400 Subject: [PATCH 11/20] Test getItem(int) --- .../player/playqueue/PlayQueueTest.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java index b3641547f..587d54b0f 100644 --- a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -1,6 +1,7 @@ package org.schabi.newpipe.player.playqueue; import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.runners.Enclosed; import org.junit.runner.RunWith; @@ -10,6 +11,7 @@ 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.assertNotEquals; @@ -110,6 +112,34 @@ public class PlayQueueTest { } } + public static class GetItemTests { + private static List 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 = mockPlayQueue(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"); From 3a8aa90c72c6d0bc8d7a8ccf6f137f53e2e3f7bd Mon Sep 17 00:00:00 2001 From: Eric Xu Date: Sat, 22 May 2021 01:09:29 -0400 Subject: [PATCH 12/20] Rearrange fields Final fields should be arranged first --- .../org/schabi/newpipe/player/playqueue/PlayQueue.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index 44b33e344..ebd4abc44 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -40,17 +40,15 @@ import io.reactivex.rxjava3.subjects.BehaviorSubject; */ public abstract class PlayQueue implements Serializable { public static final boolean DEBUG = MainActivity.DEBUG; - - private List backup; - private List streams; - @NonNull private final AtomicInteger queueIndex; private final List history = new ArrayList<>(); + private List backup; + private List streams; + private transient BehaviorSubject eventBroadcast; private transient Flowable broadcastReceiver; - private transient boolean disposed = false; PlayQueue(final int index, final List startWith) { From 4952483013525047cb60e5b6a086097fb726c2a4 Mon Sep 17 00:00:00 2001 From: Zhiheng Xu Date: Sat, 22 May 2021 11:49:05 -0400 Subject: [PATCH 13/20] Refactor static methods to package private Stops Android Studio from "recognizing" them as tests --- .../org/schabi/newpipe/player/playqueue/PlayQueueTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java index 587d54b0f..d9b6c5f18 100644 --- a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -23,7 +23,7 @@ import static org.mockito.Mockito.spy; @SuppressWarnings("checkstyle:HideUtilityClassConstructor") @RunWith(Enclosed.class) public class PlayQueueTest { - public static PlayQueue mockPlayQueue(final int index, final List streams) { + static PlayQueue mockPlayQueue(final int index, final List streams) { // I tried using Mockito, but it didn't work for some reason return new PlayQueue(index, streams) { @Override @@ -38,7 +38,7 @@ public class PlayQueueTest { }; } - public static PlayQueueItem makeItemWithUrl(final String url) { + static PlayQueueItem makeItemWithUrl(final String url) { final StreamInfoItem infoItem = new StreamInfoItem( 0, url, "", StreamType.VIDEO_STREAM ); From 0633e93dcef31a8be2684ff747f3fe561c95e4da Mon Sep 17 00:00:00 2001 From: Zhiheng Xu Date: Sat, 22 May 2021 12:45:09 -0400 Subject: [PATCH 14/20] Add test for setIndex --- .../player/playqueue/PlayQueueTest.java | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java index d9b6c5f18..0211b5efc 100644 --- a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -8,6 +8,7 @@ import org.junit.runner.RunWith; import org.schabi.newpipe.extractor.stream.StreamInfoItem; import org.schabi.newpipe.extractor.stream.StreamType; +import java.lang.reflect.Field; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -110,6 +111,32 @@ public class PlayQueueTest { emptyQueue.setIndex(0); assertEquals(0, emptyQueue.getIndex()); } + + @SuppressWarnings("unchecked") + @Test + public void addToHistory() throws NoSuchFieldException, IllegalAccessException { + final Field field; + field = PlayQueue.class.getDeclaredField("history"); + field.setAccessible(true); + List history; + + /* + history's size is currently 1. 0 is the also the current index, so history should not + be affected. + */ + nonEmptyQueue.setIndex(0); + history = (List) Objects.requireNonNull( + field.get(nonEmptyQueue) + ); + assertEquals(1, history.size()); + + // Index 3 != 0, so the second history element should be the item at streams[3] + nonEmptyQueue.setIndex(3); + history = (List) Objects.requireNonNull( + field.get(nonEmptyQueue) + ); + assertEquals(nonEmptyQueue.getItem(3), history.get(1)); + } } public static class GetItemTests { From e29058412ecdff1d244395089c5cfb4c3f4ba86e Mon Sep 17 00:00:00 2001 From: Zhiheng Xu Date: Sat, 22 May 2021 13:28:01 -0400 Subject: [PATCH 15/20] Refactor shuffle and update documentation - Add early return for invalid sizes to shuffle - Rename variables to be more descriptive - Refactor moving list element, removing unnecessary operations - Unwrap if clause for adding to history because the condition is guaranteed by the guard clause - Inline the value 0 for the ReorderEvent - Update documentation to reflect new changes --- .../newpipe/player/playqueue/PlayQueue.java | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index ebd4abc44..1437210d9 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -422,34 +422,41 @@ public abstract class PlayQueue implements Serializable { } /** - * Shuffles the current play queue. + * Shuffles the current play queue *

- * This method first backs up the existing play queue and item being played. - * Then a newly shuffled play queue will be generated along with currently - * playing item placed at the beginning of the queue. + * This method first backs up the existing play queue and item being played. Then a newly + * shuffled play queue will be generated along with currently playing item placed at the + * beginning of the queue. This item will also be added to the history. *

*

- * Will emit a {@link ReorderEvent} in any context. + * Will emit a {@link ReorderEvent} if shuffled. *

+ * + * @implNote Does nothing if the queue is empty or has a size of 1 */ public synchronized void shuffle() { + // Can't shuffle an list that's empty or only has one element + if (size() <= 1) { + 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); - if (newIndex != -1) { - streams.add(0, streams.remove(newIndex)); - } + // Move currentItem to the head of the queue + streams.remove(currentItem); + 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)); } /** From 6da5d1585d18b580c028b4b235c8537b93d6b879 Mon Sep 17 00:00:00 2001 From: Zhiheng Xu Date: Sat, 22 May 2021 13:39:15 -0400 Subject: [PATCH 16/20] Remove redundant clearing of list --- .../main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java | 1 - 1 file changed, 1 deletion(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index 1437210d9..45f5efa03 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -476,7 +476,6 @@ public abstract class PlayQueue implements Serializable { final int originIndex = getIndex(); final PlayQueueItem current = getItem(); - streams.clear(); streams = backup; backup = null; From acbe0bfc947ea622c4cd86760f72cac35c01fd11 Mon Sep 17 00:00:00 2001 From: Zhiheng Xu Date: Sat, 22 May 2021 13:56:02 -0400 Subject: [PATCH 17/20] Change shuffle() guard to check for size <= 2 After testing the app, I realized that shuffling a queue with size 2 does nothing --- .../java/org/schabi/newpipe/player/playqueue/PlayQueue.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java index 45f5efa03..014c13339 100644 --- a/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java +++ b/app/src/main/java/org/schabi/newpipe/player/playqueue/PlayQueue.java @@ -432,11 +432,12 @@ public abstract class PlayQueue implements Serializable { * Will emit a {@link ReorderEvent} if shuffled. *

* - * @implNote Does nothing if the queue is empty or has a size of 1 + * @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() <= 1) { + if (size() <= 2) { return; } // Create a backup if it doesn't already exist From b153b5954d84118030f4402ccc5f660b0c49f2fb Mon Sep 17 00:00:00 2001 From: Zhiheng Xu Date: Sun, 23 May 2021 15:24:35 -0400 Subject: [PATCH 18/20] =?UTF-8?q?Rename=20mockPlayQueue(=E2=80=A6)=20to=20?= =?UTF-8?q?makePlayQueue(=E2=80=A6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../player/playqueue/PlayQueueTest.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java index 0211b5efc..1c0500984 100644 --- a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -24,7 +24,7 @@ import static org.mockito.Mockito.spy; @SuppressWarnings("checkstyle:HideUtilityClassConstructor") @RunWith(Enclosed.class) public class PlayQueueTest { - static PlayQueue mockPlayQueue(final int index, final List streams) { + static PlayQueue makePlayQueue(final int index, final List streams) { // I tried using Mockito, but it didn't work for some reason return new PlayQueue(index, streams) { @Override @@ -53,10 +53,10 @@ public class PlayQueueTest { @Before public void setup() { - nonEmptyQueue = spy(mockPlayQueue( + nonEmptyQueue = spy(makePlayQueue( 0, Collections.nCopies(SIZE, mock(PlayQueueItem.class)) )); - emptyQueue = spy(mockPlayQueue(0, new ArrayList<>())); + emptyQueue = spy(makePlayQueue(0, new ArrayList<>())); } @Test @@ -151,7 +151,7 @@ public class PlayQueueTest { @Before public void setup() { - queue = mockPlayQueue(0, streams); + queue = makePlayQueue(0, streams); } @Test @@ -174,8 +174,8 @@ public class PlayQueueTest { @Test public void sameStreams() { final List streams = Collections.nCopies(5, item1); - final PlayQueue queue1 = mockPlayQueue(0, streams); - final PlayQueue queue2 = mockPlayQueue(0, streams); + final PlayQueue queue1 = makePlayQueue(0, streams); + final PlayQueue queue2 = makePlayQueue(0, streams); assertEquals(queue1, queue2); } @@ -183,8 +183,8 @@ public class PlayQueueTest { public void sameSizeDifferentItems() { final List streams1 = Collections.nCopies(5, item1); final List streams2 = Collections.nCopies(5, item2); - final PlayQueue queue1 = mockPlayQueue(0, streams1); - final PlayQueue queue2 = mockPlayQueue(0, streams2); + final PlayQueue queue1 = makePlayQueue(0, streams1); + final PlayQueue queue2 = makePlayQueue(0, streams2); assertNotEquals(queue1, queue2); } @@ -192,8 +192,8 @@ public class PlayQueueTest { public void differentSizeStreams() { final List streams1 = Collections.nCopies(5, item1); final List streams2 = Collections.nCopies(6, item2); - final PlayQueue queue1 = mockPlayQueue(0, streams1); - final PlayQueue queue2 = mockPlayQueue(0, streams2); + final PlayQueue queue1 = makePlayQueue(0, streams1); + final PlayQueue queue2 = makePlayQueue(0, streams2); assertNotEquals(queue1, queue2); } } From b8c604356936771e8653e094834db0e96dfa0306 Mon Sep 17 00:00:00 2001 From: Zhiheng Xu Date: Sun, 23 May 2021 15:33:57 -0400 Subject: [PATCH 19/20] Rewrite addToHistory test without using reflection --- .../player/playqueue/PlayQueueTest.java | 36 ++++++------------- 1 file changed, 11 insertions(+), 25 deletions(-) diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java index 1c0500984..da1173e9b 100644 --- a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -8,17 +8,17 @@ import org.junit.runner.RunWith; import org.schabi.newpipe.extractor.stream.StreamInfoItem; import org.schabi.newpipe.extractor.stream.StreamType; -import java.lang.reflect.Field; 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.mock; import static org.mockito.Mockito.spy; @SuppressWarnings("checkstyle:HideUtilityClassConstructor") @@ -53,9 +53,11 @@ public class PlayQueueTest { @Before public void setup() { - nonEmptyQueue = spy(makePlayQueue( - 0, Collections.nCopies(SIZE, mock(PlayQueueItem.class)) - )); + final List 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<>())); } @@ -112,30 +114,14 @@ public class PlayQueueTest { assertEquals(0, emptyQueue.getIndex()); } - @SuppressWarnings("unchecked") @Test - public void addToHistory() throws NoSuchFieldException, IllegalAccessException { - final Field field; - field = PlayQueue.class.getDeclaredField("history"); - field.setAccessible(true); - List history; - - /* - history's size is currently 1. 0 is the also the current index, so history should not - be affected. - */ + public void addToHistory() { nonEmptyQueue.setIndex(0); - history = (List) Objects.requireNonNull( - field.get(nonEmptyQueue) - ); - assertEquals(1, history.size()); + assertFalse(nonEmptyQueue.previous()); - // Index 3 != 0, so the second history element should be the item at streams[3] nonEmptyQueue.setIndex(3); - history = (List) Objects.requireNonNull( - field.get(nonEmptyQueue) - ); - assertEquals(nonEmptyQueue.getItem(3), history.get(1)); + assertTrue(nonEmptyQueue.previous()); + assertEquals("URL_0", Objects.requireNonNull(nonEmptyQueue.getItem()).getUrl()); } } From fb8896e634c291957d3587205c9d889ca5c56167 Mon Sep 17 00:00:00 2001 From: Zhiheng Xu Date: Mon, 24 May 2021 13:03:52 -0400 Subject: [PATCH 20/20] Remove Enclosed.class runner Does not affect Gradle tests and only benefits IDE workflow --- .../org/schabi/newpipe/player/playqueue/PlayQueueTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java index da1173e9b..43e090900 100644 --- a/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java +++ b/app/src/test/java/org/schabi/newpipe/player/playqueue/PlayQueueTest.java @@ -3,8 +3,6 @@ package org.schabi.newpipe.player.playqueue; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; -import org.junit.experimental.runners.Enclosed; -import org.junit.runner.RunWith; import org.schabi.newpipe.extractor.stream.StreamInfoItem; import org.schabi.newpipe.extractor.stream.StreamType; @@ -22,7 +20,6 @@ import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.spy; @SuppressWarnings("checkstyle:HideUtilityClassConstructor") -@RunWith(Enclosed.class) public class PlayQueueTest { static PlayQueue makePlayQueue(final int index, final List streams) { // I tried using Mockito, but it didn't work for some reason