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

Non-horizontal drags to the seek bar activate vertical dragging instead of seeking #4121

Closed
nyanpasu64 opened this issue Aug 12, 2020 · 22 comments · Fixed by #4155
Closed

Non-horizontal drags to the seek bar activate vertical dragging instead of seeking #4121

nyanpasu64 opened this issue Aug 12, 2020 · 22 comments · Fixed by #4155
Labels
bug Issue is related to a bug player Issues related to any player (main, popup and background)

Comments

@nyanpasu64
Copy link
Contributor

Version

Steps to reproduce the bug

  • Install recent dev build with unified player.
  • Open a video in the main player.
  • Tap the video so the seek bar appears.
  • Press the seek bar either on or off the current position (doesn't matter), optionally hold your finger still, then drag the seek bar diagonally or vertically.

Expected behavior

If you press the seek bar, then all drags will be interpreted as seeks, and not vertical sliding.

Actual behaviour

If your first movement (including after you hold still) is more vertical (either up or down) than horizontal, then the finger press is interpreted as a vertical scroll instead of seeking.

If your initial press is above a horizontal line (slightly below the seek bar), a vertical drag moves the video down. Below that line (but still in the video and not the title), it drags the description/comments section up.

In the official YouTube app (or possibly an older version), the seek bar appears between the video and the description, and dragging it vertically is still considered a seek, not an attempt to collapse the video.

Screenshots/Screen recordings

https://youtu.be/c7M55-zRahQ

@nyanpasu64 nyanpasu64 added the bug Issue is related to a bug label Aug 12, 2020
@TobiGr TobiGr added the player Issues related to any player (main, popup and background) label Aug 12, 2020
@avently
Copy link
Contributor

avently commented Aug 15, 2020

Vertical dragging triggers vertical dragging and ... it's wrong? ¯\_(ツ)_/¯

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Aug 15, 2020

In 0.19.8, in both portrait and landscape modes, the moment you touch the seekbar anywhere, the thumb (why is it called that?) jumps to under your finger, and a timer comes on screen which tells you the seek position.

In the unified player, touching the seekbar does nothing. It is the swipe action after touching which activates the seekbar and repositions the thumb. Until then, the app doesn't know if you're going to swipe horizontally or vertically, so it doesn't show the same behaviour as the current release.

I think the point here is usability. There is now a non-zero likelihood (versus zero in the current release) that a person will trigger the wrong gesture and drag the video when they actually want to seek.

While testing this, I discovered that in landscape mode, the unified player's seekbar and rotation button (but not the playing time and video length, strangely) are another swipe zone in addition to the one at the top. Why did you make this design choice?

@avently
Copy link
Contributor

avently commented Aug 15, 2020

Why did you make this design choice?

I didn't do anything special. It just works like this because the whole player view located inside AppBarLayout which responds to a swipe. So when you swipe inside player in landscape all touches intercepted by volume and brightness gestures but when you touching a button this interception is not working and the swipe gesture intercepted by top-level viewGroup which is AppBarLayout.

@opusforlife2
Copy link
Collaborator

Got it, got it. So just like the volume and brightness gesture areas, is it possible to 'protect/reserve' the entire bottom area such that it isn't intercepted by AppBarLayout? This would solve this bug in portrait mode as well as the funky jumpy behaviour the video shows when swiping in landscape, as I discovered above.

@avently
Copy link
Contributor

avently commented Aug 15, 2020

is it possible to 'protect/reserve' the entire bottom area such that it isn't intercepted by AppBarLayout?

Yes, it's possible

Do this:
enter , R.id.bottomControls
here: https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/player/event/CustomBottomSheetBehavior.java#L28

enter

                        // Makes bottom part of the player draggable in portrait when
                        // playbackControlRoot is hidden
                        if (element == R.id.bottomControls
                                && child.findViewById(R.id.playbackControlRoot)
                                .getVisibility() != View.VISIBLE) {
                            return super.onInterceptTouchEvent(parent, child, event);
                        }

before this line: https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/player/event/CustomBottomSheetBehavior.java#L54

enter

    final View seekBar = child.findViewById(R.id.playbackSeekBar);
        if (seekBar != null) {
            final boolean visible = seekBar.getGlobalVisibleRect(globalRect);
            if (visible && globalRect.contains((int) ev.getRawX(), (int) ev.getRawY())) {
                allowScroll = false;
                return false;
            }
        }

before this line: https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/com/google/android/material/appbar/FlingBehavior.java#L72

Your problem and the problem from this issue will be fixed. If you confirm that the solution works, I'll make PR someday. Or you can do it yourself, just copy-paste the code and ensure that it works without problems.

@opusforlife2
Copy link
Collaborator

🤭 Since you're basically spoon-feeding me the code, I could try it out and even make a PR, but the moment the devs review it and ask me to make changes, I'll be like a deer caught in the headlights.

@Stypox Is this something you have time for? If not, I could try making a PR, but I expect it to be a messy affair.

@avently
Copy link
Contributor

avently commented Aug 16, 2020

@opusforlife2 it's not difficult for me to make a PR when code is ready. I just don't have a time on testing. I did some testing and the code works ok. Just need a confirmation that it works in every situation

@opusforlife2
Copy link
Collaborator

Could you send me the apk real quick? I'll test it and get right back to you.

@avently
Copy link
Contributor

avently commented Aug 16, 2020

@opusforlife2

app-release.zip

@nyanpasu64
Copy link
Contributor Author

While testing this, I discovered that in landscape mode, the unified player's seekbar and rotation button (but not the playing time and video length, strangely) are another swipe zone in addition to the one at the top. Why did you make this design choice?

I tested the apk download. The playing time and video length still allow you to drag the issue up and scroll down to the description and comments, but do not allow you to drag the page down to collapse the video player. Not a big deal, but an inconsistency nonetheless.

Other than that, this functionality seems better than the current version of the unified UI. Thanks!

@avently
Copy link
Contributor

avently commented Aug 16, 2020

The playing time and video length still allow you to drag the issue up and scroll down to the description

scroll down to the description and comments

scroll down to the description

Intentional because of this. So now you can do it in landscape when you have visible fullscreen button (swipe from down to up on the button)

@nyanpasu64
Copy link
Contributor Author

Nitpick: In the build you posted:

  • When not in full screen, both dragging the timestamps and full-screen button allow scrolling down.
  • In full screen, only dragging the full-screen button lets you scroll down while maintaining full-screen. Dragging the timestamps does nothing, and dragging the seek bar seeks instead.

I didn't know you could scroll down in full-screen before reading your comment.

@avently
Copy link
Contributor

avently commented Aug 16, 2020

Nitpick

i do not accept nit-picking. I have my life not for this.

@opusforlife2
Copy link
Collaborator

@avently

So now you can do it in landscape when you have visible fullscreen button (swipe from down to up on the button)

This is AWESOME. I just tried it out. It looks amazing, and pretty useful for those who want to view comments/suggested videos while the video is playing.

Anyway, first of all, the main bug this issue was opened for is confirmed FIXED. Vertical swipes from the seekbar no longer activate dragging, but seek to that position instead. So no more of this:

There is now a non-zero likelihood (versus zero in the current release) that a person will trigger the wrong gesture and drag the video when they actually want to seek.

Please add fixes linkage to this issue when you open the PR.

Now, apart from that,

While testing this, I discovered that in landscape mode, the unified player's seekbar and rotation button (but not the playing time and video length, strangely) are another swipe zone in addition to the one at the top.

FIXED. What I forgot to mention here yesterday was that I discovered that swiping up on the seekbar would cause the video to "jump" down to accommodate the status bar appearing. I did mention it in passing, but not the full thing:

as well as the funky jumpy behaviour the video shows when swiping in landscape, as I discovered above.

It looked glitchy as hell. Experimenting with that was what led me to discover that the bottom area was a swipe zone. This is now fixed. No glitchy jumps. We have probably avoided a bunch of new bug reports complaining about this.

^ I'll open this as a separate issue so that you can add fixes linkage to it in your PR whenever you open it.

In 0.19.8, in both portrait and landscape modes, the moment you touch the seekbar anywhere, the thumb (why is it called that?) jumps to under your finger, and a timer comes on screen which tells you the seek position.

It still takes a swipe to activate the seekbar instead of just a touch. Is the current release's behaviour impossible to implement now or did you decide not to for a design reason?

@nyanpasu64 Open a separate issue. Maybe someone decides to take a shot at it. Personally, I don't mind it since it's only a cosmetic issue, and no functionality is impacted.

@justanidea
Copy link
Contributor

@opusforlife2 would you have time to provide a video with this "scroll up gesture" ? I cant reproduce so i wonder if i understand well what you are all 3 talking anout :)
This would be pretty kind from you

@opusforlife2
Copy link
Collaborator

@justanidea Lock system rotation. Open a video. Notice the fullscreen button on the right side of the seekbar. Tap to go fullscreen. Now swipe upwards on this same button.

@avently
Copy link
Contributor

avently commented Aug 17, 2020

Is the current release's behaviour impossible to implement now or did you decide not to for a design reason?

I just didn't try to change the behavior. Single tap is working, swipe is working, no problem for me.

@opusforlife2
Copy link
Collaborator

Not a tap. Touch and hold there.

@justanidea
Copy link
Contributor

@opusforlife2 Oh thx. System rotation locking was missing👍🏻

@MD77MD
Copy link

MD77MD commented Aug 19, 2020

i think this is great... it would solve the constant rotation between landscape and portrait mode. although I wish it would work regardless of rotation states... I mean if swiping from bottom to top would work always the same like swiping from top to bottom... video info could have at the bottom the same gesture zone size as the top gesture zone

anyway I think this feature should be rotation states free😉

@MD77MD
Copy link

MD77MD commented Aug 19, 2020

another cool thing would be if the video would keep playing in the background the same as when pressing playlists bottom in full screen mode... what do you guys think of this?

@opusforlife2
Copy link
Collaborator

@MD77MD New issue. New issue. Or, if you want to discuss only, then come to Newpipe's IRC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants