From 57474e2dab44f568243019d96d6edd4b2f6e6439 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 013c59f904595899a404dae672a16245174d3eda 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 4cd1f201f536e52fbf183f281ec2267ebdc72e40 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 882b235a781fd10228e03c55d212cebe310baf4d 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 441c68ead25ac33e1381c4fa09a729748df2e2e7 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 8efe2859b843580a31ca3f16331bf792e528b643 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 8d0f2d371dacb98bd17add0c26d20c6dd6c47b6f 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 775fbc9a75f57ef66a4659778a0017753f34bcab 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 e8eeac673570e3bb4e8dd202be3085a7a359ab69 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 77f694033666bebcfd41ad153e8e540983678b62 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 363bbf5fd3d3fa0ceb060f4d0552db9f062fcff7 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 92a67bb8cb65cbfca24883de822b350e9748a8cf 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 08949ee3475994cb32881651aec78614fa69e992 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 bf8e8798d9e12f46ceeb3fa5bd029bb8ad0f9236 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 e1a6347c4eef0bc374109b3efcb57562f6a8440b 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 5ab6e84044d5d3ada8c9c3e9f8f754a7b712c51e 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 2e161a1f45d8da2bedd2b924da1c90cc68678714 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 e518c0dc148a041116e80bd867b1002d209c37b4 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 40f66977c77bb743c276f1baad4849944bb769b6 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 c0f47195a25ec3561bbe915196ce7097d684d4bb 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