-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
In Fullscreen playback, toggle play/pause with hardware space button #5331
In Fullscreen playback, toggle play/pause with hardware space button #5331
Conversation
} else { | ||
showSystemUi(); | ||
if (playerService.getView().getVisibility() != View.GONE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be != View.VISIBLE here?
what does this change intent to do here anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm checking (when fullscreen is turned off) if the player view is shown, for a sanity check. If it is, I show the viewpager again.
The whole idea of hiding it is to disable keyboard focusing to it when pressed. Also you could press "down" key on the hardware keyboard (in fullscreen view!) and scroll down to comments - which definitely looked like a bug. This is also fixed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this check after considering it. It does not seem to be really needed. @Redirion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly you could use arrow down key to reach description in fullscreen on android TV. Now it's 100% impossible which is a bad thing if you ask me
app/src/main/java/org/schabi/newpipe/player/VideoPlayerImpl.java
Outdated
Show resolved
Hide resolved
@@ -1891,8 +1891,10 @@ public void onFullscreenStateChanged(final boolean fullscreen) { | |||
|
|||
if (fullscreen) { | |||
hideSystemUiIfNeeded(); | |||
viewPager.setVisibility(View.GONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, what is this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this avoids that the comment / next video page is reachable even though the video is playing in fullscreen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was intended, wasn't it @avently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it doesn't make sense for an invisible/overlayed layout to be focusable. I don't think this was intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Stypox, thank you for letting me know.
@Redirion you just removed an ability to view comments, description, etc by long pressing on arrow down button. Now I'm unable to view anything because viewpager is hidden.
The behavior that was before is intended. Please, remove this line or find a workaround for situation you're trying to solve that doesn't ruin the intended ability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed on phones or tablets, it was introduced since it could be useful to view description or comments while watching a video, not only on android tvs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well.. I was honestly confused by this. I'm in full screen, I press down and I'm in comments.
And I can't go back to the video at all using my keyboard (i.e. using the up key). Only by swiping.
This is why I thought it's an incorrect behavior.
And for example pressing Space also switches me to Comments. But in 99% of video players space means play/pause. This is why I wanted to change this behavior.
What do you think? @Stypox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, you need to use this method to check whether NewPipe is running on a TV:
public static boolean isTv(final Context context) { |
However, I am not sure how to check whether the device has a hardware keyboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbarashkov
Take a look at how we use the arrow button:
ScreenRecord-2021-01-10-15-07-37.mp4.zip
it would be very useful to press space to pause/unpause video.
Doing it while the comments view is visible is impossible, because it gets first focus and onKeyDown event is lost
Event can not be lost. Find it and intercept. Move focus to play/pause button after that if needed. There is already working code for TVs which react on DPAD buttons without problems. Take a look on it too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well DPAD is handled in VideoPlayerImpl -> onKeyDown, I understand that.
Issue is: onKeyDown is never even being called (on any hardware keyboard button press) when the comments ViewPager is shown in fullscreen mode.
@mbarashkov the app I use on a phone doesn't support comments per review changes so it's not comfortable to use web version of github for this. Instead I continue our discussion via ordinal comments.
I'm not sure why this happens but you probably need to set correct place of focus. So whatever you do with a player should not focus on comments section. I don't get why the same code work for dpad and don't for hardware keyboard. But anyway if you want to make it happen you need find a way:) obvious |
OK. I thought this is a bug actually. Since this is intended behavior, I'll need to find another way to make it work, and submit another pull request. |
… Space button.
What is it?
Description of the changes in your PR
Due diligence