Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selectable behavior of Back button #6895

Closed
wants to merge 2 commits into from
Closed

Conversation

avently
Copy link
Contributor

@avently avently commented Aug 10, 2021

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

Made an option to change behavior of back button on video fragment. It's something that should be included into initial unified player PR. Now NewPipe users can choose what to do when back button pressed. There are six choices:

  • open previous video from history (like now, stays default choice)
  • open previous video only in case it was playing from the same playlist as current one
  • hide to mini player regardless of what was in the history
  • hide into the mini player if the stream was played, close otherwise
  • close the player fragment completely but only if nothing is playing or only video is playing. Just hides the player to mini player in case audio or popup are playing
  • close even in fullscreen.

Also I noticed that for some reason equals check is not working correctly currently. So I reimplemented it based on urls compare of inner streams (see the first commit).

Fixes the following issue(s)

Fixes #4479

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

@SameenAhnaf
Copy link
Collaborator

SameenAhnaf commented Aug 11, 2021

Thanks for the amazing PR. It's working flawlessly. If I am not wrong, #4569 and #5590 should also be linked with the PR.

Could you please add an option like "Direct exit from player" so that users can directly exit video from full screen? I know, there's a gesture for that. But it's quite tricky on TV.

@opusforlife2
Copy link
Collaborator

@avently Is this intended to close #4569? If option 4 is meant for that, then I'll address it on that basis (I found some problems). If not, then no issues.

@avently
Copy link
Contributor Author

avently commented Aug 11, 2021

@SameenAhnaf
I'll add

@opusforlife2

@avently Is this intended to close #4569? If option 4 is meant for that, then I'll address it on that basis (I found some problems). If not, then no issues.

Address it, if it is needed for you

@triallax triallax added the feature request Issue is related to a feature in the app label Aug 11, 2021
@opusforlife2
Copy link
Collaborator

Okay, so for option 4, we ideally want the Back button to close the player directly only if backstack size is 0, as stated in #4569. Right now, this PR closes using Back button even if backstack size is non-zero. This happens even if a video is playing.

@avently
Copy link
Contributor Author

avently commented Aug 26, 2021

I added two new options:

@ghost ghost mentioned this pull request Aug 27, 2021
4 tasks
@opusforlife2
Copy link
Collaborator

I added two new options: hide and close

@avently Bug: Back button closes the player even if the backstack size is non-zero.

  • Open a video, but don't play it.
  • Open a related video, but don't play that either.
  • Tap the Back button.
  • Player closed, losing the entire backstack.

What it should do in this case is rewind through the stack, just like the current release does.

To be clear, closing should only happen if both conditions are satisfied: size of backstack == 0 && player is open == false.

@avently
Copy link
Contributor Author

avently commented Aug 28, 2021

It was intentional. So what's the point of your idea then? It acts as the default behavior with exception to non-played first item in backstack?

Ok, will make like this.

@SameenAhnaf
Copy link
Collaborator

I think, choosing "Close the player even in full screen" should put the app back in portrait on smartphone after clicking 'Back'. Could you please do something about it?

@avently
Copy link
Contributor Author

avently commented Aug 28, 2021

Check again, made these two changes you both asked

@SameenAhnaf
Copy link
Collaborator

SameenAhnaf commented Aug 29, 2021

On my device, 'Close the main player even in full screen' is chosen and auto-rotation is disabled. 2 seconds of lag for closing mini player and changing rotation can be seen when I am trying to exit the video from full screen.

Such a lag is not found when getting back into main player both through 'Minimize' and 'Back' buttons. So, this lag is unexpected.

Could you please fix it?

@opusforlife2
Copy link
Collaborator

@avently Now the opposite happens. I open a video but don't tap the thumbnail. I tap Back. The page goes into the mini player instead of closing. Since I opened only one video, it's the only one in the backstack, it should have closed directly.

It acts as the default behavior with exception to non-played first item in backstack?

Yes, that's the idea.

@avently
Copy link
Contributor Author

avently commented Aug 30, 2021

Such a lag is not found when getting back into main player both through 'Minimize' and 'Back' buttons. So, this lag is unexpected.

"The lag" is equal to 500ms, not two seconds. Player closes first, after that rotation happens. Without 500ms delay rotation mechanism skipping player closing. So nothing special here. If you see the 2 sec delay is because of slow rotation on debug apk. On release apk it will be much faster.

@opusforlife2 could not reproduce. I have disabled autoplay and after I open a video without clicking on thumbnail and then click back button, player closes. Make sure you selected Hide into the mini player if the stream was played, close otherwise

@opusforlife2
Copy link
Collaborator

Make sure you selected Hide into the mini player if the stream was played, close otherwise

Ah, indeed. I am a dum-dum. After deleting and installing the latest APK and importing my database, I forgot to set the option again. After setting it to this, the app works exactly as expected.

Thank you, @avently!

@opusforlife2
Copy link
Collaborator

If I am not wrong, #4569 and #5590 should also be linked with the PR.

Indeed. Done. 👍

@avently
Copy link
Contributor Author

avently commented Sep 4, 2021

Guys, let me know when you'll be ready to merge this. I'll fix merge conflicts then

@opusforlife2
Copy link
Collaborator

@avently Could you add the two new options into the PR description? Thanks.

@avently
Copy link
Contributor Author

avently commented Sep 26, 2021

@opusforlife2 done

@skyGtm
Copy link

skyGtm commented Sep 30, 2021

This PR also addresses the issue I had.

Issue: I am using NP v0.21.10
When autoplay is disabled

  1. play some video
  2. scroll down the comments
  3. Press back button
    Video pauses and comments jumps to the top , rather than minimizing the player while still background playing the video.

But this PR fixes this. Thankyou. 🎉 🎉

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks neat! I'd add more comments though, e.g. above every if (behavior == ...), explaining what the behavior does and why the code works the way it is. Thank you for this much appreciated change and sorry for not reviewing earlier :-)
Note: I have not tested anything yet

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
if (behavior == PlayerHelper.BackBehavior.BACK_BEHAVIOR_CLOSE_FULLSCREEN) {
if (!isPlayerAvailable() || player.videoPlayerSelected()) {
bottomSheetBehavior.setState(BottomSheetBehavior.STATE_HIDDEN);
requireView().postDelayed(this::rotateScreenOnPhoneIfInLandscape, 500);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks suspicious, how did you calculate the 500ms delay to make sure the rotation happens correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, forgot to made something about that. This delay was just brute-forced. If it will not work for someone I'll try to find a better way.

@ghost
Copy link

ghost commented Jan 30, 2022

Hey the default back button works great on touch screen but can the default behavior be changed on TVs to close the player fragment? Thanks for the work @avently

@newhinton
Copy link

@avently Maybe you can remove some of the options to slim it down and easier review? There was a suggestion of what to keep and what not, and i guess most users would be fine if we can get rid of the backstack.

Maybe that would speed up things?

On the other hand, maybe the maintainers should just decide what do keep, since to me it seems the implementation is already thoroughly done and we just need a decision.

@Stypox
Copy link
Member

Stypox commented Aug 5, 2022

Yes, we just need a decision

@avently
Copy link
Contributor Author

avently commented Aug 5, 2022

@newhinton

Maybe you can remove some of the options to slim it down and easier review?

The problem is that there is no decision what to remove and what to keep.

@Stypox
Copy link
Member

Stypox commented Aug 6, 2022

Sorry @avently, we forgot about it once again. Our final decision is the following. Do you have some time to finally finish off this PR? Thanks ;-)

The solution

The solution we agreed upon is to force #4569 (see below) on phones/tablets and force BACK_BEHAVIOR_CLOSE on Android TVs. Yeah, you read it right, there will be no "selectable behavior", but rather the current behavior will be improved. Here is why we think this would make more people happy without impacting too much on people who want different behaviors:

  • People on Android TVs will get a behavior consistent with the platform they are on, since the backstack is almost unusable there, so I think they would be happy with only having BACK_BEHAVIOR_CLOSE.
  • Some people may want the video detail fragment to be minimized and not closed when they press back after they just opened a (single) video. Back button behaviour with no backstack #4569 would not suit them, but a workaround would be to minimize the player by swiping it down, instead of pressing back. Some people will still be unhappy, though maybe not many, as most would just have to get used to the change.
  • Some people may want the video detail fragment to always be minimized on back presses. I am not sure who really wants this, because at this point the backstack is useless, and, again, minimizing the video detail fragment by swiping down would yield the same result.
  • Some people may want the video detail fragment to always be closed on back presses (pre 0.19 behavior): Back button behaviour with no backstack #4569 would partially suit them, at least when there is only one video in the backstack.

Behavior on phones/tablets (#4569)

When a back press event occurs in the video detail fragment:

  • IF video player open
    • IF fullscreen
      • -> exit from fullscreen
    • ELSE IF there is a previous video in the backstack
      • -> play previous video from backstack
    • ELSE (i.e. not fullscreen and no previous video in backstack)
      • -> stop playing the video and show the thumbnail on the video detail fragment
  • ELSE IF background/popup player open
    • IF there is a previous video in the backstack
      • -> open previous video from backstack, without playing it
    • ELSE (i.e. no previous video in backstack)
      • -> minimize the video detail fragment into the bottom sheet
  • ELSE (i.e. no player open)
    • IF there is a previous video in the backstack
      • -> open previous video from backstack, without playing it
    • ELSE (i.e. no previous video in backstack)
      • -> completely close the video detail fragment

Behavior on Android TVs (BACK_BEHAVIOR_CLOSE)

When a back press event occurs in the video detail fragment:

  • IF video player open
    • IF fullscreen
      • -> exit from fullscreen
    • ELSE (i.e. not fullscreen)
      • -> stop playing the video
  • ELSE (i.e. no player open, or background/popup player open)
    • -> completely close the video detail fragment

@newhinton
Copy link

Could you explain how such a complicated decision-tree is an improvement?

And what part exactly changes from the current behaviour?

My usecase for this pr is to actually get rid of the backstack, because 99% of the time my backstack is completely the opposite of what i want, and while i cant prove it i feel that i am not alone with that sentiment

@Stypox
Copy link
Member

Stypox commented Aug 6, 2022

Could you explain how such a complicated decision-tree is an improvement?

It looks complicated, but it isn't really. It is just two layers deep: the first layer depends on which player is currently open, while the second layer depends on the backstack and has similar behaviors across all three player situations.

According to what @opusforlife2 said a while ago:

I tried to remember more about the discussion I had with Assassin. The reason we proposed #4569 was because a majority of complaints we were seeing at the time (I think it was multiple places, including, e.g. Reddit) were about breaking a simple navigation flow. Users who were just browsing the app without opening any video were forced to deal with "this stupid bottom bar thingy" each and every time they tried to go back. #4569 would make it so that unless there is a queue or active playback, the minimised player wouldn't bother the user anymore.


And what part exactly changes from the current behaviour?

The fact that the video detail fragment is completely closed (and its backstack thrown away) if you press back when there is only one video in the backstack and it is not currently playing. @opusforlife2 correct me if I'm wrong ;-)


My usecase for this pr is to actually get rid of the backstack, because 99% of the time my backstack is completely the opposite of what i want, and while i cant prove it i feel that i am not alone with that sentiment

Yes, you are not alone. The backstack is almost useless to me, too. #4569 would suit you (and me) since it would prevent the backstack from building up. Say you play a video by tapping on it from e.g. trending: if you then want to stop playing that video and watch another one, you can just press back one time to exit fullscreen, one other time to stop the video, and one last time to completely close the video detail fragment. Next time you open a video you will be able to do the same thing without having the previous video in the backstack.

@newhinton
Copy link

Yes, you are not alone. The backstack is almost useless to me, too. #4569 would suit you (and me) since it would prevent the backstack from building up. Say you play a video by tapping on it from e.g. trending: if you then want to stop playing that video and watch another one, you can just press back one time to exit fullscreen, one other time to stop the video, and one last time to completely close the video detail fragment. Next time you open a video you will be able to do the same thing without having the previous video in the backstack.

And if minimize the video A, and start a new one B, i would have a backstack of [A, B], right? Afaik, that should be the current behaviour. This is not my preferred solution but i guess its a good compromise. (I sometimes browse and switch while watching, or stop a video and come back waaaay later to open a different one, both will create a backstack that to me, logically, is "wrong")

@opusforlife2
Copy link
Collaborator

you can just press back one time to exit fullscreen, one other time to stop the video, and one last time to completely close the video detail fragment

No, that's 2 presses, not 3. If you tap Back to exit full screen, the video automatically pauses. Then it's just one more tap to close the backstack.

@avently
Copy link
Contributor Author

avently commented Aug 7, 2022

@Stypox

ELSE (i.e. not fullscreen and no previous video in backstack)
-> stop playing the video and show the thumbnail on the video detail fragment

Why do we need this step? Why not to make this instead?

ELSE (i.e. not fullscreen and no previous video in backstack)
-> minimize the video detail fragment into the bottom sheet

?

From my point of view it's a useless action and looks like a problem with the app as from the user's perspective. While hiiding to bottom sheet will show that it's THE LAST video in backstack, no more back pressing can happen andat the same time, the video is still active (no interrupt).

play previous video from backstack

Does this action includes playing previous videos from playlistQueueItem?

Consider the following backstack:

- one video
- another video
- playlist queue item
-- first video in playlist
-- second video in playlist    <<<<--------  WE ARE HERE

Once user presses back button, what video should be played: first video in playlist or another video?

@avently
Copy link
Contributor Author

avently commented Aug 7, 2022

@Stypox

I reading your post again and again and still can't understand why can't we just integrate selectable behavior in the settings?
Having different default options for TV and for phone/tablet is a good thing and can easily be implemented on this PR's basis.
If for the user these options will be good, he will not even go to settings. If not good, the user will just try one-by-one every option until he finds what he like more (i'm talking about problem you mention earlier that it's hard to distinguish one option from another by reading it's name in the settings).

By integrating this PR you don't need a separate option for disabling/enabling backstack. It's just already included here for those who needs it.

We can remove one or two options from the list. For example, I can remove:

  • BACK_BEHAVIOR_PREV_PLAYLIST.

It is useless. If the user likes the idea of backstack he will like the idea to play non-playlist too.

I can remove BACK_BEHAVIOR_CLOSE_FULLSCREEN too but it's useful for TV. Good to have the ability to start playing something in the feed and close it right after you want it without double pressing back button.

@opusforlife2
Copy link
Collaborator

Why not to make this instead?

@avently The point is to get rid of the bottom sheet ASAP when using Back button navigation. If the user wants to retain the bottom sheet for the last video in the backstack, they can always swipe down instead.

Does this action includes playing previous videos from playlistQueueItem?

What is the - playlist queue item in your sample backstack? What separates it from the 1st and 2nd playlist videos? I thought the backstack would have been

- 1st video
- 2nd video
- 1st playlist video
- 2nd playlist video

If for the user these options will be good, he will not even go to settings. If not good, the user will just try one-by-one every option until he finds what he like more

If after implementing the suggested behaviour, we still see user complaints about wanting to do navigation differently, then we can think about changing/adding navigation options in the future. For now, we're aiming to minimise disruption by going for the most needed navigation flows.

@avently
Copy link
Contributor Author

avently commented Aug 8, 2022

What is the - playlist queue item in your sample backstack?

There is a technical difference in the implementation. It's not important in the context of discussion since the option with playlist will be removed anyway for the reason I said above.

If after implementing the suggested behaviour, we still see user complaints about wanting to do navigation differently, then we can think about changing/adding navigation options in the future

I'm complain about wanting to do navigation differently right now. Your ideas leave me with nothing. As well as all people I did this PR for. What do you think about this?

@opusforlife2
Copy link
Collaborator

I'm complain about wanting to do navigation differently right now.

Isn't the current implementation already designed as per your idea of the ideal navigation flow? After all, you wrote the code for it... 😕

@avently
Copy link
Contributor Author

avently commented Aug 8, 2022

@opusforlife2 i'm talking about idea you and @Stypox are discussing. Current flow in release version is brilliant for me. '

@opusforlife2
Copy link
Collaborator

Okay, so the only difference between the current implementation and our suggestion is that on the very last backstack item, under very specific conditions, tapping Back should do something different than swiping down.

@Stypox
Copy link
Member

Stypox commented Aug 8, 2022

can't we just integrate selectable behavior in the settings?

I have already said this multiple times. The back behaviour is a standard thing, not something that should be customizable. Also, it is really difficult to explain what happens exactly with a specific option, leading to maintenance burden as opus explained.

Once user presses back button, what video should be played: first video in playlist or another video?

another video, since moving through the playlist should be done with the prev/next buttons in the player

@avently
Copy link
Contributor Author

avently commented Aug 8, 2022

@Stypox

...
ELSE (i.e. not fullscreen and no previous video in backstack)
-> stop playing the video and show the thumbnail on the video detail fragment
...
ELSE (i.e. no previous video in backstack)
-> minimize the video detail fragment into the bottom sheet
...
ELSE (i.e. no previous video in backstack)
-> completely close the video detail fragment

Also, it is really difficult to explain what happens exactly with a specific option, leading to maintenance burden as opus explained

Hm, I think that it's really difficult to explain what exactly happens here .^. even without any options. But current behaviour PREV_ANY) is pretty consistent in this situation: it always minimize the player into mini player in all three cases above. So whenever you do some movement you 100% sure what happens next. Same for other options in this PR.
More options in this specific PR != more troubles for devs. Because it's so easy to follow in code.

The thought I noticed by talking with all here is that the backstack behaviour is not as easy as we want to. I mean, really, we can't have one behaviour that feels good to everyone or at least to many. And we can make it comfortable for them by giving more options.

As from the maintaining complexity... Did you notice that this PR have only one file conflict after one year? And it's even in non-important file (not in fragment)! No complexity, no problems. As from the user's perspective, more options is what people find useful in NewPipe in comparison to official Youtube. It's how things work here

@opusforlife2
Copy link
Collaborator

More options in this specific PR != more troubles for devs. Because it's so easy to follow in code.

Did you notice that this PR have only one file conflict after one year?

#6895 (comment)

@newhinton
Copy link

newhinton commented Aug 8, 2022

@opusforlife2 @avently What about this compromise:

We keep the current behavior. We dont change anything, we dont introduce a different behavior directly to the back button.

Instead, we provide a simple toggle switch somewhere in the settings, that disables the backstack. (In technical terms: limit the size to 1).

From a maintenance standpoint: It adds next to nothing
From a behavioural standpoint: It only adds a little complexity, but not (really) to the back button.
It also should be more easy to communicate that than a full list of items.

Would that help/solve the issue? I am not exactly sure anymore what the desired goal is :D

@opusforlife2
Copy link
Collaborator

@newhinton Well, you'll have to go back and read the issues that were created regarding navigation after the unified player was released. We are intending for the backstack/bottom sheet to be disabled for Android TV, where it is very difficult to use, but we want to keep it for handheld devices, where the backstack is very useful.

@newhinton
Copy link

Yes and i understand that quite a few users are disagreeing with the usefulness of the backstack.

Wouldn't my suggestion fix the original issue (#4479) without introducing a problematic amount of options?

@opusforlife2
Copy link
Collaborator

We're not intending to introduce any options. The navigation flow should be uniform for all TV users, and separately for all phone/tablet users.

@newhinton
Copy link

So, this pr is completely useless because there wont be a change to this anyway? To be honest, i do think introducing 3 or 5 different behaviours is over the top and untenable, but the backstack does annoy quite a few users as can be seen by the interest those issues&pr's gather. Even @Stypox agreed that for a part of the userbase the backstack is exactly the wrong behaviour. Why not expand on that in a reasonable manner?

@opusforlife2
Copy link
Collaborator

We're trying to do exactly that.

@avently
Copy link
Contributor Author

avently commented Aug 13, 2022

Looks like we will not come to any decision here that will fit our needs. You don't want selectable behavior so this PR will not help anyway.
Closing it after one year.

@avently avently closed this Aug 13, 2022
@newhinton
Copy link

newhinton commented Aug 13, 2022

This is very sad to see. I hope the maintainers find a way to integrate parts of this PR into the app. There is no reason to change behaviour that does not work properly once in a while, even breaking with long established behaviour.

Edit: I have reopened #7273 so that we maybe can help people out that absolutely do not need the backstack.

@fevers
Copy link

fevers commented Sep 24, 2023

I think the choice not to add a settings screen, and instead to force users into your preferred workflow, will always be a mistaken and anti-user one.

@burgrr

This comment was marked as spam.

@burgrr

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app
Projects
None yet