Added a play next button to the long press menu #6872

z3r0r4 · 2021-08-07T17:30:40Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Adds a play next button in the long press menu of a video, as asked for in #4136.

add getPlayerNextIntent in NavigationHelper
handle not Append Only Intent in Player class
add nextOn*Player functions in NavigationHelper
create new StreamDialogEntry: next
add new entry to the BaseListFragment
add Strings for names

Before/After Screenshots/Screen Record

Before	After

Fixes the following issue(s)

Fixes Feature Request: Add "Play next" button #4136
Fixes add „pre-queue“ option #6026

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

I read the contribution guidelines.

Critique / Todo

remove my comments
reduce code duplication
add translation strings

…ew long press dialog entry, new dialog functions, new strings

app/src/main/res/values/strings.xml

to make long press entry more descriptive Co-authored-by: opusforlife2 <53176348+opusforlife2@users.noreply.github.com>

Stypox

Thank you for adding this requested feature :-)

Sorry for the many things I asked you to do, feel free to ask for help or delegate something to me if you can't figure it out ;-)

app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java

app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

app/src/main/java/org/schabi/newpipe/player/Player.java

app/src/main/res/values/strings.xml

Co-authored-by: Stypox <stypox@pm.me>

z3r0r4 · 2021-08-09T17:52:35Z

Thank you for adding this requested features :-)

Sorry for the many things I asked you to do, feel free to ask for help or delegate something to me if you can't figure it out ;-)

My pleasure!
Thank you for the thorough review. Some of the requested changes should have been quite obvious optimizations when the goal is more than a mvp.
As long as it isnt urgent i'll get it done by myself (hopefully this week).
I might be back for some clarification on how to integrate your code though.

SameenAhnaf · 2021-08-10T13:42:07Z

A few bugs found.

Play queue become inaccessible once 'Play next' is selected. User needs to click on Background or selected player button. Also, some videos get multiple times although selected just once.
https://drive.google.com/file/d/1BJnia6hJJasVy_LY-90n9eK4UQH7Czp2/view?usp=drivesdk
There are multiple bugs here. Watch the video for more details.
https://drive.google.com/file/d/1BHzXe_Wf1YdcACAjEmAo5RmmLComv4ni/view?usp=drivesdk

z3r0r4 · 2021-08-10T16:10:59Z

Interesting and major findings (kind of weird as well) I'll look into it after i've implemented the requested changes. Thank you very much for testing.

MD77MD · 2021-08-11T14:50:41Z

just s suggestion... could we call it "enqueue next"... also more consistent.

this is what VLC uses if I remember correctly.

Stypox · 2021-08-13T09:46:58Z

@MD77MD mmmh yeah, I agree with you

opusforlife2 · 2021-08-14T02:54:55Z

Hmmm. If there is only the current video in the queue, Enqueue and Enqueue Next will have the same function. Should the button check for >1 items in the queue before showing?

Co-authored-by: Stypox <stypox@pm.me>

…d RESUME_PLAYBACK/ deprecate enqueueNextOn*Player and enqueueOn*Player methods add getPlayerIntent, getPlayerEnqueueIntent and getPlayerEnqueueNextIntent without selectOnAppend and RESUME_PLAYBACK/ deprecate those with add section comments

removed redundant methods

…nqueueOn*Player methods

z3r0r4 · 2021-08-15T16:01:40Z

A few bugs found.

1. Play queue become inaccessible once 'Play next' is selected. User needs to click on Background or selected player button. Also, some videos get multiple times although selected just once.
   https://drive.google.com/file/d/1BJnia6hJJasVy_LY-90n9eK4UQH7Czp2/view?usp=drivesdk

2. There are multiple bugs here. Watch the video for more details.
   https://drive.google.com/file/d/1BHzXe_Wf1YdcACAjEmAo5RmmLComv4ni/view?usp=drivesdk

I cant seem to replicate these anymore, would it be possible for you to check again @SameenAhnaf ?

z3r0r4 · 2021-08-15T16:03:18Z

I do have to note a new bug tho:
The play next option is inaccessible when selecting videos inside of a playlist.

I think it might not be referenced in the code which generates the dialog inside of playlist, dont know yet where to check though

z3r0r4 · 2021-08-15T16:16:00Z

I do have to note a new bug tho:
The play next option is inaccessible when selecting videos inside of a playlist.

I think it might not be referenced in the code which generates the dialog inside of playlist, dont know yet where to check though

b8ebda7 fixes this hopefully

Co-authored-by: Stypox <stypox@pm.me>

Stypox

Sorry for keeping asking for changes, I hope this is the last one. Just to be sure, please make sure that the if (player is open) before enqueueing is removed everywhere, and that the play next action is available in every long press menu of each fragment. Thank you :-)

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

…er is open. Dont generally default to background play.

z3r0r4 · 2021-09-13T14:03:21Z

I've checked the fragments mentioned in this comment and I could not find a dialog which doesnt include a play next action.

app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java

Stypox

Thank you!

z3r0r4 · 2021-09-18T12:37:16Z

my pleasure.
Thank you for the helpful feedback!

TobiGr · 2021-10-15T20:18:23Z

app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java

@@ -82,7 +84,6 @@ private NavigationHelper() {
                intent.putExtra(Player.PLAY_QUEUE_KEY, cacheKey);
            }
        }
-        intent.putExtra(Player.RESUME_PLAYBACK, resumePlayback);


NewPipe stopped resuming playback for all streams. This PR seems to be the cause of this.

Can you please explain, why the resumePlayback parameter has been removed from all methods? It was not true everywhere before. What was the reason to make it true all the time?

Also, why has this line been removed? Shouldn't it be

intent.putExtra(Player.RESUME_PLAYBACK, true);

if resumePlayback was always true?

CC @z3r0r4 @Stypox

Does it work if you readd that line?

From #7268:

Pressing the "background" button while a video is playing does not resume the stream, but still starts at the beginning.

Yes, resumePlayback should always be true like you said, I don't know why I didn't notice it was removed altoghether. But now I was asking whether by readding the line you commented on (and replacing resumePlayback with true) fixes the issue.

I'll try to find out how to fix it now

This was caused by #6872

Stypox

I analyzed things a little bit, here is what I saw:

the only places where resumePlayback was false for enqueue actions were the ones I pointed out in the . review comments
enqueueuing can currently happen in two ways
- when no video is already playing. In this case having resumePlayback=true makes sure the video is resumed, otherwise it isn't. Before this PR the only situation when it can happen that a video is enqueued when none were already playing and resumePlayback is false, is when the user long-presses the background or popup buttons in the video detail fragments (the first two . review comments; the third one can never be triggered when no video is already playing since the enqueue option would not be shown in the stream dialog menu in that case).
- when something is already playing. In this case the resumePlayback value is simply ignored and the stream will start from the beginning when its turn will come. So having resumePlayback=false in the stream dialog menu (the last . review comment) does not make a difference to having it set to true.
- => I think we can safely set resumePlayback=false for every enqueue action, since it only really makes a difference when there is no player running and long-pressing the control buttons in video detail fragment, playlist & channel. This way a user could start a video with resumePlayback=true by normal-pressing the background/popup button, and with resumePlayback=false by long-pressing the background/popup button. And in my opinion it makes sense not to resume playback when enqueueing.
this PR accidentally sets resumePlayback=true for every single action (being it enqueue or not). This is an error, mostly caused by not noticing that:
- there were places where the enqueue action had resumePlayback=false
- the playOnMainPlayer, playOnPopupPlayer and playOnBackgroundPlayer functions also used getPlayerIntent. After removing the resumePlayback parameter from getPlayerIntent Android Studio didn't give any error on those functions, since there was another overload of getPlayerIntent with parameters (Context, Class, PlayQueue, boolean), but the last parameter was playWhenReady instead of resumePlayback!

I will soon create a PR reinstantiating resumePlayback as it was before, except setting it to false in all enqueue actions as I explained above. Is this ok @TobiGr @Redirion ?

Stypox · 2021-10-20T08:08:55Z

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

@@ -1096,8 +1097,8 @@ private void openPopupPlayer(final boolean append) {
        toggleFullscreenIfInFullscreenMode();

        final PlayQueue queue = setupPlayQueueForIntent(append);
-        if (append) {
-            NavigationHelper.enqueueOnPopupPlayer(activity, queue, false);


Stypox · 2021-10-20T08:09:10Z

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

@@ -1155,7 +1156,7 @@ private void openNormalBackgroundPlayer(final boolean append) {

        final PlayQueue queue = setupPlayQueueForIntent(append);
        if (append) {
-            NavigationHelper.enqueueOnBackgroundPlayer(activity, queue, false);


Stypox · 2021-10-20T08:09:29Z

app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

-        if (type == AUDIO) {
-            NavigationHelper.enqueueOnBackgroundPlayer(fragment.getContext(),
-                    new SinglePlayQueue(item), false);
-        } else if (type == POPUP) {
-            NavigationHelper.enqueueOnPopupPlayer(fragment.getContext(),
-                    new SinglePlayQueue(item), false);
-        } else /* type == VIDEO */ {
-            NavigationHelper.enqueueOnVideoPlayer(fragment.getContext(),
-                    new SinglePlayQueue(item), false);
-        }


TobiGr · 2021-10-20T09:30:41Z

@Stypox That depends on how long you need to create the PR. If you can do it today, please do it. Otherwise I suggest to revert this PR and re-do it. We need to publish a fix quickly.
(If you create a PR, please target release/0.21.13)

Stypox · 2021-10-20T10:15:13Z

I don't think I'll make it in time for today; tomorrow I will have time for sure though

Edit: maybe I found some time

z3r0r4 · 2021-10-25T15:01:38Z

It seems to be solved, still here's the specific commit where I deprecated the needed function and my comment on it for future reference.
I think I should have been more clear with my objection / notice.
bea56dd (deprecated here, removed in the next ones)
#6872 (comment) (minor comment hinting to the problem)

z3r0r4 added 2 commits August 7, 2021 19:03

added mvp play next button in long press menu; new intent handling, n…

13f9f34

…ew long press dialog entry, new dialog functions, new strings

changed line length for checkstyle pass

d86560c

z3r0r4 changed the title ~~added mvp play next button in long press menu~~ Added a play next button to the long press menu Aug 7, 2021

z3r0r4 commented Aug 7, 2021

View reviewed changes

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved

cleaned comments, moved strings

7966d55

opusforlife2 reviewed Aug 8, 2021

View reviewed changes

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved

AudricV added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) labels Aug 8, 2021

Update app/src/main/res/values/strings.xml

5649ef5

to make long press entry more descriptive Co-authored-by: opusforlife2 <53176348+opusforlife2@users.noreply.github.com>

Stypox requested changes Aug 9, 2021

View reviewed changes

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved

Update app/src/main/res/values/strings.xml

a345886

Co-authored-by: Stypox <stypox@pm.me>

z3r0r4 and others added 6 commits August 15, 2021 16:20

replace redundant nextOnVideoPlayer methods

340b497

Co-authored-by: Stypox <stypox@pm.me>

Merge remote-tracking branch 'origin/playNextButton' into playNextButton

4286b98

removed deprecated methods

fbda029

removed redundant methods

removed deprecated methods

09920ac

removed redundant methods

replaced APPEND_ONLY, removed SELECT_ON_APPEND / replaced remaining e…

9bab471

…nqueueOn*Player methods

z3r0r4 force-pushed the playNextButton branch from f7f9851 to 9bab471 Compare August 15, 2021 15:48

now works with playlists

b8ebda7

z3r0r4 and others added 2 commits September 12, 2021 21:16

correcting logic

6153b41

Co-authored-by: Stypox <stypox@pm.me>

remove 2 year old unused string, formating

dde9fed

Co-authored-by: Stypox <stypox@pm.me>

Stypox requested changes Sep 12, 2021

View reviewed changes

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java Outdated Show resolved Hide resolved

z3r0r4 added 2 commits September 13, 2021 15:37

correct enqueue (next) conditionals, default to background if no play…

f324c39

…er is open. Dont generally default to background play.

remove player open checks from button long press enqueue actions

1865cb7

triallax reviewed Sep 13, 2021

View reviewed changes

app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java Outdated Show resolved Hide resolved

improve log msg

3dad84d

z3r0r4 mentioned this pull request Sep 13, 2021

[refactor] Merge Queue Control Dialogs and Buttons #7103

Open

3 tasks

z3r0r4 requested a review from Stypox September 16, 2021 19:58

Stypox added 2 commits September 18, 2021 11:15

Rename next to enqueue_next

cf0427e

Refactor kotlin

7e37d85

Stypox approved these changes Sep 18, 2021

View reviewed changes

Stypox merged commit 70354eb into TeamNewPipe:dev Sep 18, 2021

This was referenced Oct 2, 2021

Release/0.21.11 #7192

Merged

Release 0.21.11 #7193

Closed

SameenAhnaf mentioned this pull request Oct 7, 2021

Add move to next feature in play queue #2413

Closed

1 task

TobiGr mentioned this pull request Oct 15, 2021

Resume Playback is not working #7236

Closed

TobiGr reviewed Oct 15, 2021

View reviewed changes

TobiGr added a commit that referenced this pull request Oct 17, 2021

[Player] Fix resuming playback

5fcc3b4

This was caused by #6872

TobiGr mentioned this pull request Oct 17, 2021

Release/0.21.13 #7268

Merged

5 tasks

Stypox reviewed Oct 20, 2021

View reviewed changes

Stypox added a commit that referenced this pull request Oct 20, 2021

Revert part of #6872 and fix playback resuming

6461c2f

Stypox mentioned this pull request Oct 20, 2021

Revert part of #6872 and fix playback resuming #7286

Merged

5 tasks

TobiGr pushed a commit that referenced this pull request Oct 20, 2021

Revert part of #6872 and fix playback resuming

ac071b3

TobiGr mentioned this pull request Oct 21, 2021

Release/0.21.13 #7291

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a play next button to the long press menu #6872

Added a play next button to the long press menu #6872

z3r0r4 commented Aug 7, 2021 •

edited

Loading

Stypox left a comment •

edited

Loading

z3r0r4 commented Aug 9, 2021

SameenAhnaf commented Aug 10, 2021

z3r0r4 commented Aug 10, 2021 via email •

edited by AudricV

Loading

MD77MD commented Aug 11, 2021

Stypox commented Aug 13, 2021

opusforlife2 commented Aug 14, 2021

z3r0r4 commented Aug 15, 2021

z3r0r4 commented Aug 15, 2021 •

edited

Loading

z3r0r4 commented Aug 15, 2021

Stypox left a comment

z3r0r4 commented Sep 13, 2021

Stypox left a comment

z3r0r4 commented Sep 18, 2021

TobiGr Oct 15, 2021 •

edited

Loading

Stypox Oct 19, 2021

TobiGr Oct 19, 2021

Stypox Oct 20, 2021 •

edited

Loading

Stypox Oct 20, 2021

Stypox left a comment •

edited

Loading

Stypox Oct 20, 2021

Stypox Oct 20, 2021

Stypox Oct 20, 2021

TobiGr commented Oct 20, 2021

Stypox commented Oct 20, 2021 •

edited

Loading

z3r0r4 commented Oct 25, 2021

Added a play next button to the long press menu #6872

Added a play next button to the long press menu #6872

Conversation

z3r0r4 commented Aug 7, 2021 • edited Loading

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

Fixes the following issue(s)

APK testing

Due diligence

Critique / Todo

Stypox left a comment • edited Loading

Choose a reason for hiding this comment

z3r0r4 commented Aug 9, 2021

SameenAhnaf commented Aug 10, 2021

z3r0r4 commented Aug 10, 2021 via email • edited by AudricV Loading

MD77MD commented Aug 11, 2021

Stypox commented Aug 13, 2021

opusforlife2 commented Aug 14, 2021

z3r0r4 commented Aug 15, 2021

z3r0r4 commented Aug 15, 2021 • edited Loading

z3r0r4 commented Aug 15, 2021

Stypox left a comment

Choose a reason for hiding this comment

z3r0r4 commented Sep 13, 2021

Stypox left a comment

Choose a reason for hiding this comment

z3r0r4 commented Sep 18, 2021

TobiGr Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Stypox Oct 19, 2021

Choose a reason for hiding this comment

TobiGr Oct 19, 2021

Choose a reason for hiding this comment

Stypox Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Stypox Oct 20, 2021

Choose a reason for hiding this comment

Stypox left a comment • edited Loading

Choose a reason for hiding this comment

Stypox Oct 20, 2021

Choose a reason for hiding this comment

Stypox Oct 20, 2021

Choose a reason for hiding this comment

Stypox Oct 20, 2021

Choose a reason for hiding this comment

TobiGr commented Oct 20, 2021

Stypox commented Oct 20, 2021 • edited Loading

z3r0r4 commented Oct 25, 2021

z3r0r4 commented Aug 7, 2021 •

edited

Loading

Stypox left a comment •

edited

Loading

z3r0r4 commented Aug 10, 2021 via email •

edited by AudricV

Loading

z3r0r4 commented Aug 15, 2021 •

edited

Loading

TobiGr Oct 15, 2021 •

edited

Loading

Stypox Oct 20, 2021 •

edited

Loading

Stypox left a comment •

edited

Loading

Stypox commented Oct 20, 2021 •

edited

Loading