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.

Sign up for GitHub

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

Jump to bottom

#5263: Allow the user to pause while a video is buffering #5929

Merged
merged 5 commits into from
Mar 29, 2021
Merged

#5263: Allow the user to pause while a video is buffering #5929

merged 5 commits into from
Mar 29, 2021

Conversation

Douile
Copy link
Contributor

@Douile Douile commented Mar 27, 2021 •
edited by Stypox
Loading

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

  • Allow the user to pause/unpause while a video is buffering
    • Check whether the playWhenReady is true instead of isPlaying: isPlaying returns false if the video is buffering

Fixes the following issue(s)

APK testing

On the website the APK can be found by going to the "Checks" tab below the title and then on "artifacts" on the right.

Due diligence

@opusforlife2
Copy link
Collaborator

Yay! Much needed usability fix.

@AudricV AudricV added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Mar 27, 2021
Stypox
Stypox requested changes Mar 28, 2021
View reviewed changes
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.

Thank you!
Almost there ;-)

app/src/main/java/org/schabi/newpipe/player/Player.java Outdated Show resolved Hide resolved
@Douile
Copy link
Contributor Author

Douile commented Mar 28, 2021

These changes seem to make
https://github.com/Douile/NewPipe/blob/963a7609f1681ec4942a2a3e560db82f280fc0e3/app/src/main/java/org/schabi/newpipe/player/Player.java#L709-L713

redundant, should I replace this snippit with playPause? I'm not sure why getPlayWhenReady was being used here but not in playPause

@Stypox
Copy link
Member

Stypox commented Mar 28, 2021

@Douile nice catch. Replace every instance of if (getPlayWhenReady()) play(); else pause(); with playPause(), thanks

@Douile
Copy link
Contributor Author

Douile commented Mar 29, 2021

I think that's the only one.

@Stypox Stypox merged commit 84de865 into TeamNewPipe:dev Mar 29, 2021
@Stypox
Copy link
Member

Stypox commented Mar 29, 2021

@Douile thank you :-D

This was referenced Apr 11, 2021
Closed
Merged
@triallax triallax mentioned this pull request Apr 21, 2021
Closed
3 tasks
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 23, 2021
@Douile @tossj
Allow the user to pause while a video is buffering (TeamNewPipe/NewPi…
0ee898a 
…pe#5929)

Fix pause while buffering
Use getPlayWhenReady wrapper everywhere playWhenReady is checked
Remove duplicate `playPause()` code
@Redirion Redirion mentioned this pull request Apr 28, 2021
Closed
4 tasks
@litetex
Copy link
Member

litetex commented May 10, 2021

ℹ This PR very likely caused #6179, see also #6179 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stypox Stypox Stypox approved these changes

Assignees
No one assigned
Labels
bug Issue is related to a bug player Issues related to any player (main, popup and background)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't pause video while buffering
5 participants
@Douile @opusforlife2 @Stypox @litetex @AudricV