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

Fix enqueuing behavior #8262

Closed
wants to merge 1 commit into from
Closed

Conversation

dtcxzyw
Copy link
Contributor

@dtcxzyw dtcxzyw commented Apr 20, 2022

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

  • Show Enqueue menu item even when the play queue is empty.
  • First enqueued video/audio will remain paused by setting playWhenReady to false.
  • Buffering the paused video/audio (Tested on my device, but I can't tell from the visual feedback that it's working).

Before/After Screenshots/Screen Record

  • Before:
before-mute.mp4
  • After:
after-mute.mp4

Fixes the following issue(s)

APK testing

Debug APK

Due diligence

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Stypox
Copy link
Member

Stypox commented May 2, 2022

Thank you for the contribution! Unfortunately I think we don't want this behavior:

  • Enqueue means "Add to the queue", so it doesn't make sense as an action if there is no queue.
  • Which player should be opened? Background, popup or main? There is no single answer.
  • Why start paused? This would look like a random action happening for the user who isn't aware, and anyway we don't want new random behaviors without a natural meaning.
  • Also, there could be issues when the player is still starting and receives another enqueue intent: it might not be happy and react with a crash.

@opusforlife2
Copy link
Collaborator

Enqueue means "Add to the queue", so it doesn't make sense as an action if there is no queue.

@Stypox This is how music apps behave. Try Vanilla Music, for example.

Why start paused? This would look like a random action happening for the user who isn't aware, and anyway we don't want new random behaviors without a natural meaning.

See the linked #2611. I would argue this is the natural meaning.

Also, there could be issues when the player is still starting and receives another enqueue intent: it might not be happy and react with a crash.

Can that not be guarded against programmatically? And is it not already possible to cause this in the current release? Enqueuing something while the player is still starting?

Which player should be opened? Background, popup or main? There is no single answer.

I think the enqueuing should be done in the background player. If you're not actively opening the popup or main player using the relevant buttons, and are only building a queue from scratch, you likely don't want to switch to video watching just yet.

@Stypox
Copy link
Member

Stypox commented May 2, 2022

@opusforlife2 then instead of overloading the Enqueue button, I would create a new button named "Start paused in the background" that is only shown when there is no player active/starting.

@opusforlife2
Copy link
Collaborator

@Stypox It will have the same purpose, but sure. Will it then be a swap between Enqueue and Start paused based on whether a player instance is already open?

@Stypox
Copy link
Member

Stypox commented May 2, 2022

Will it then be a swap between Enqueue and Start paused based on whether a player instance is already open?

Yes, always either one or the other (or none, while the player is starting), in the same place

@dtcxzyw
Copy link
Contributor Author

dtcxzyw commented May 3, 2022

Creating a new button might be more user-friendly.
I try to handle the case that the player is still starting when enqueuing.
Is there anything else that needs to be modified? @Stypox

@Stypox
Copy link
Member

Stypox commented May 5, 2022

No, I think adding a new button "Start paused in the background" to the long-press menu would be enough. Make sure to only show that button if the player is not open (if the player is open but the play queue is null, nothing should be shown; if the player is open and the play queue is not null, then the "Enqueue" should be shown)

@opusforlife2
Copy link
Collaborator

I try to handle the case that the player is still starting when enqueuing.

Are you saying this cannot be done, @Stypox? Or that it should not be done in this PR?

@Stypox
Copy link
Member

Stypox commented May 6, 2022

Both ;-)

I mean, everything is doable, but enqueueing things while the player is starting sounds complicated, thus I'd suggest to do it after the player and the ways it can be started have been refactored

@opusforlife2
Copy link
Collaborator

@dtcxzyw Then if you're interested in handling the player-still-starting case, keep an eye on #8170 and similar future PRs. Is that okay, @Stypox?

@opusforlife2
Copy link
Collaborator

This PR wasn't being blocked by #8170 except for a particular case. Are you interested in going further with this, @dtcxzyw?

@opusforlife2 opusforlife2 added the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Aug 23, 2022
@Stypox
Copy link
Member

Stypox commented Nov 22, 2022

Closing since there has not been an answer for a while, feel free to reopen ;-)

@Stypox Stypox closed this Nov 22, 2022
@opusforlife2 opusforlife2 removed the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show Enqueue menu item even when no current video playing Queueing logic when queue is empty (bug?)
3 participants