From 99d62381b97a7b1c587ab025b585cb1f3dcb0f56 Mon Sep 17 00:00:00 2001 From: Mauricio Colli Date: Fri, 10 Dec 2021 13:02:15 -0300 Subject: [PATCH 1/3] Fix download dialog selector layout and add some tests --- .../newpipe/util/StreamItemAdapterTest.kt | 187 ++++++++++++++++++ .../newpipe/util/StreamItemAdapter.java | 49 ++++- 2 files changed, 229 insertions(+), 7 deletions(-) create mode 100644 app/src/androidTest/java/org/schabi/newpipe/util/StreamItemAdapterTest.kt diff --git a/app/src/androidTest/java/org/schabi/newpipe/util/StreamItemAdapterTest.kt b/app/src/androidTest/java/org/schabi/newpipe/util/StreamItemAdapterTest.kt new file mode 100644 index 00000000000..0b01bfaf63b --- /dev/null +++ b/app/src/androidTest/java/org/schabi/newpipe/util/StreamItemAdapterTest.kt @@ -0,0 +1,187 @@ +package org.schabi.newpipe.util + +import android.content.Context +import android.util.SparseArray +import android.view.View +import android.view.View.GONE +import android.view.View.INVISIBLE +import android.view.View.VISIBLE +import android.widget.Spinner +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.filters.MediumTest +import androidx.test.internal.runner.junit4.statement.UiThreadStatement +import org.hamcrest.MatcherAssert.assertThat +import org.hamcrest.Matchers.`is` +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.schabi.newpipe.R +import org.schabi.newpipe.extractor.MediaFormat +import org.schabi.newpipe.extractor.stream.AudioStream +import org.schabi.newpipe.extractor.stream.Stream +import org.schabi.newpipe.extractor.stream.SubtitlesStream +import org.schabi.newpipe.extractor.stream.VideoStream + +@MediumTest +@RunWith(AndroidJUnit4::class) +class StreamItemAdapterTest { + private lateinit var context: Context + private lateinit var spinner: Spinner + + @Before + fun setUp() { + context = ApplicationProvider.getApplicationContext() + UiThreadStatement.runOnUiThread { + spinner = Spinner(context) + } + } + + @Test + fun videoStreams_noSecondaryStream() { + val adapter = StreamItemAdapter( + context, + getVideoStreams(true, true, true, true), + null + ) + + spinner.adapter = adapter + assertIconVisibility(spinner, 0, VISIBLE, VISIBLE) + assertIconVisibility(spinner, 1, VISIBLE, VISIBLE) + assertIconVisibility(spinner, 2, VISIBLE, VISIBLE) + assertIconVisibility(spinner, 3, VISIBLE, VISIBLE) + } + + @Test + fun videoStreams_hasSecondaryStream() { + val adapter = StreamItemAdapter( + context, + getVideoStreams(false, true, false, true), + getAudioStreams(false, true, false, true) + ) + + spinner.adapter = adapter + assertIconVisibility(spinner, 0, GONE, GONE) + assertIconVisibility(spinner, 1, GONE, GONE) + assertIconVisibility(spinner, 2, GONE, GONE) + assertIconVisibility(spinner, 3, GONE, GONE) + } + + @Test + fun videoStreams_Mixed() { + val adapter = StreamItemAdapter( + context, + getVideoStreams(true, true, true, true, true, false, true, true), + getAudioStreams(false, true, false, false, false, true, true, true) + ) + + spinner.adapter = adapter + assertIconVisibility(spinner, 0, VISIBLE, VISIBLE) + assertIconVisibility(spinner, 1, GONE, INVISIBLE) + assertIconVisibility(spinner, 2, VISIBLE, VISIBLE) + assertIconVisibility(spinner, 3, VISIBLE, VISIBLE) + assertIconVisibility(spinner, 4, VISIBLE, VISIBLE) + assertIconVisibility(spinner, 5, GONE, INVISIBLE) + assertIconVisibility(spinner, 6, GONE, INVISIBLE) + assertIconVisibility(spinner, 7, GONE, INVISIBLE) + } + + @Test + fun subtitleStreams_noIcon() { + val adapter = StreamItemAdapter( + context, + StreamItemAdapter.StreamSizeWrapper( + (0 until 5).map { + SubtitlesStream(MediaFormat.SRT, "pt-BR", "https://example.com", false) + }, + context + ), + null + ) + spinner.adapter = adapter + for (i in 0 until spinner.count) { + assertIconVisibility(spinner, i, GONE, GONE) + } + } + + @Test + fun audioStreams_noIcon() { + val adapter = StreamItemAdapter( + context, + StreamItemAdapter.StreamSizeWrapper( + (0 until 5).map { AudioStream("https://example.com/$it", MediaFormat.OPUS, 192) }, + context + ), + null + ) + spinner.adapter = adapter + for (i in 0 until spinner.count) { + assertIconVisibility(spinner, i, GONE, GONE) + } + } + + /** + * @return a list of video streams, in which their video only property mirrors the provided + * [videoOnly] vararg. + */ + private fun getVideoStreams(vararg videoOnly: Boolean) = + StreamItemAdapter.StreamSizeWrapper( + videoOnly.map { + VideoStream("https://example.com", MediaFormat.MPEG_4, "720p", it) + }, + context + ) + + /** + * @return a list of audio streams, containing valid and null elements mirroring the provided + * [shouldBeValid] vararg. + */ + private fun getAudioStreams(vararg shouldBeValid: Boolean) = + getSecondaryStreamsFromList( + shouldBeValid.map { + if (it) AudioStream("https://example.com", MediaFormat.OPUS, 192) + else null + } + ) + + /** + * Checks whether the item at [position] in the [spinner] has the correct icon visibility when + * it is shown in normal mode (selected) and in dropdown mode (user is choosing one of a list). + */ + private fun assertIconVisibility( + spinner: Spinner, + position: Int, + normalVisibility: Int, + dropDownVisibility: Int + ) { + spinner.setSelection(position) + spinner.adapter.getView(position, null, spinner).run { + assertThat( + "normal visibility (pos=[$position])", + findViewById(R.id.wo_sound_icon).visibility, `is`(normalVisibility) + ) + } + spinner.adapter.getDropDownView(position, null, spinner).run { + assertThat( + "drop down visibility (pos=[$position])", + findViewById(R.id.wo_sound_icon).visibility, `is`(dropDownVisibility) + ) + } + } + + /** + * Helper function that builds a secondary stream list. + */ + private fun getSecondaryStreamsFromList(streams: List) = + SparseArray?>(streams.size).apply { + streams.forEachIndexed { index, stream -> + val secondaryStreamHelper: SecondaryStreamHelper? = stream?.let { + SecondaryStreamHelper( + StreamItemAdapter.StreamSizeWrapper(streams, context), + it + ) + } + put(index, secondaryStreamHelper) + } + } +} diff --git a/app/src/main/java/org/schabi/newpipe/util/StreamItemAdapter.java b/app/src/main/java/org/schabi/newpipe/util/StreamItemAdapter.java index c89da9d459d..b66080d1b9b 100644 --- a/app/src/main/java/org/schabi/newpipe/util/StreamItemAdapter.java +++ b/app/src/main/java/org/schabi/newpipe/util/StreamItemAdapter.java @@ -42,16 +42,20 @@ public class StreamItemAdapter extends BaseA private final StreamSizeWrapper streamsWrapper; private final SparseArray> secondaryStreams; + /** + * Indicates that at least one of the primary streams is an instance of {@link VideoStream}, + * has no audio ({@link VideoStream#isVideoOnly()} returns true) and has no secondary stream + * associated with it. + */ + private final boolean hasVideoOnlyWithNoSecondaryStream; + public StreamItemAdapter(final Context context, final StreamSizeWrapper streamsWrapper, final SparseArray> secondaryStreams) { this.context = context; this.streamsWrapper = streamsWrapper; this.secondaryStreams = secondaryStreams; - } - public StreamItemAdapter(final Context context, final StreamSizeWrapper streamsWrapper, - final boolean showIconNoAudio) { - this(context, streamsWrapper, showIconNoAudio ? new SparseArray<>() : null); + this.hasVideoOnlyWithNoSecondaryStream = checkHasVideoOnlyWithNoSecondaryStream(); } public StreamItemAdapter(final Context context, final StreamSizeWrapper streamsWrapper) { @@ -115,10 +119,15 @@ private View getCustomView(final int position, final View view, final ViewGroup final VideoStream videoStream = ((VideoStream) stream); qualityString = videoStream.getResolution(); - if (secondaryStreams != null) { + if (hasVideoOnlyWithNoSecondaryStream) { if (videoStream.isVideoOnly()) { - woSoundIconVisibility = secondaryStreams.get(position) == null ? View.VISIBLE - : View.INVISIBLE; + woSoundIconVisibility = hasSecondaryStream(position) + // It has a secondary stream associated with it, so check if it's a + // dropdown view so it doesn't look out of place (missing margin) + // compared to those that don't. + ? (isDropdownItem ? View.INVISIBLE : View.GONE) + // It doesn't have a secondary stream, icon is visible no matter what. + : View.VISIBLE; } else if (isDropdownItem) { woSoundIconVisibility = View.INVISIBLE; } @@ -167,6 +176,32 @@ private View getCustomView(final int position, final View view, final ViewGroup return convertView; } + /** + * @param position which primary stream to check. + * @return whether the primary stream at position has a secondary stream associated with it. + */ + private boolean hasSecondaryStream(final int position) { + return secondaryStreams != null && secondaryStreams.get(position) != null; + } + + /** + * @return if there are any video-only streams with no secondary stream associated with them. + * @see #hasVideoOnlyWithNoSecondaryStream + */ + private boolean checkHasVideoOnlyWithNoSecondaryStream() { + for (int i = 0; i < streamsWrapper.getStreamsList().size(); i++) { + final T stream = streamsWrapper.getStreamsList().get(i); + if (stream instanceof VideoStream) { + final boolean videoOnly = ((VideoStream) stream).isVideoOnly(); + if (videoOnly && !hasSecondaryStream(i)) { + return true; + } + } + } + + return false; + } + /** * A wrapper class that includes a way of storing the stream sizes. * From 79540a8b9cdef5803fafee966272e5565aa52d61 Mon Sep 17 00:00:00 2001 From: litetex <40789489+litetex@users.noreply.github.com> Date: Sat, 2 Apr 2022 15:43:50 +0200 Subject: [PATCH 2/3] Fix tests --- .../newpipe/util/StreamItemAdapterTest.kt | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/app/src/androidTest/java/org/schabi/newpipe/util/StreamItemAdapterTest.kt b/app/src/androidTest/java/org/schabi/newpipe/util/StreamItemAdapterTest.kt index 0b01bfaf63b..a9aa40d8273 100644 --- a/app/src/androidTest/java/org/schabi/newpipe/util/StreamItemAdapterTest.kt +++ b/app/src/androidTest/java/org/schabi/newpipe/util/StreamItemAdapterTest.kt @@ -11,8 +11,7 @@ import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.filters.MediumTest import androidx.test.internal.runner.junit4.statement.UiThreadStatement -import org.hamcrest.MatcherAssert.assertThat -import org.hamcrest.Matchers.`is` +import org.junit.Assert import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -156,15 +155,17 @@ class StreamItemAdapterTest { ) { spinner.setSelection(position) spinner.adapter.getView(position, null, spinner).run { - assertThat( - "normal visibility (pos=[$position])", - findViewById(R.id.wo_sound_icon).visibility, `is`(normalVisibility) + Assert.assertEquals( + "normal visibility (pos=[$position]) is not correct", + findViewById(R.id.wo_sound_icon).visibility, + normalVisibility, ) } spinner.adapter.getDropDownView(position, null, spinner).run { - assertThat( - "drop down visibility (pos=[$position])", - findViewById(R.id.wo_sound_icon).visibility, `is`(dropDownVisibility) + Assert.assertEquals( + "drop down visibility (pos=[$position]) is not correct", + findViewById(R.id.wo_sound_icon).visibility, + dropDownVisibility ) } } From 6b1a6d264bf4eb0fa601be66649cc792abdbf38e Mon Sep 17 00:00:00 2001 From: litetex <40789489+litetex@users.noreply.github.com> Date: Sat, 2 Apr 2022 15:44:06 +0200 Subject: [PATCH 3/3] Better naming --- .../org/schabi/newpipe/util/StreamItemAdapter.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/util/StreamItemAdapter.java b/app/src/main/java/org/schabi/newpipe/util/StreamItemAdapter.java index b66080d1b9b..03342a49770 100644 --- a/app/src/main/java/org/schabi/newpipe/util/StreamItemAdapter.java +++ b/app/src/main/java/org/schabi/newpipe/util/StreamItemAdapter.java @@ -47,7 +47,7 @@ public class StreamItemAdapter extends BaseA * has no audio ({@link VideoStream#isVideoOnly()} returns true) and has no secondary stream * associated with it. */ - private final boolean hasVideoOnlyWithNoSecondaryStream; + private final boolean hasAnyVideoOnlyStreamWithNoSecondaryStream; public StreamItemAdapter(final Context context, final StreamSizeWrapper streamsWrapper, final SparseArray> secondaryStreams) { @@ -55,7 +55,8 @@ public StreamItemAdapter(final Context context, final StreamSizeWrapper strea this.streamsWrapper = streamsWrapper; this.secondaryStreams = secondaryStreams; - this.hasVideoOnlyWithNoSecondaryStream = checkHasVideoOnlyWithNoSecondaryStream(); + this.hasAnyVideoOnlyStreamWithNoSecondaryStream = + checkHasAnyVideoOnlyStreamWithNoSecondaryStream(); } public StreamItemAdapter(final Context context, final StreamSizeWrapper streamsWrapper) { @@ -119,7 +120,7 @@ private View getCustomView(final int position, final View view, final ViewGroup final VideoStream videoStream = ((VideoStream) stream); qualityString = videoStream.getResolution(); - if (hasVideoOnlyWithNoSecondaryStream) { + if (hasAnyVideoOnlyStreamWithNoSecondaryStream) { if (videoStream.isVideoOnly()) { woSoundIconVisibility = hasSecondaryStream(position) // It has a secondary stream associated with it, so check if it's a @@ -186,9 +187,9 @@ private boolean hasSecondaryStream(final int position) { /** * @return if there are any video-only streams with no secondary stream associated with them. - * @see #hasVideoOnlyWithNoSecondaryStream + * @see #hasAnyVideoOnlyStreamWithNoSecondaryStream */ - private boolean checkHasVideoOnlyWithNoSecondaryStream() { + private boolean checkHasAnyVideoOnlyStreamWithNoSecondaryStream() { for (int i = 0; i < streamsWrapper.getStreamsList().size(); i++) { final T stream = streamsWrapper.getStreamsList().get(i); if (stream instanceof VideoStream) {