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

Update play next recommended video setting to be "by default" #6400

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Dec 17, 2024

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

This PR does:

  • Setting Play next video -> Autoplay Recommended Videos by Default (modified from Add autoplay toggle to the video player #5866
  • Setting now affects the video playing session's initial value only, where a "video playing session" is the period that user stays on the watch page (for N videos)
    • Changing the setting in settings page won't affect existing "video playing sessions" value
    • Changing the autoplay value in a "video playing session" won't affect global setting

Screenshots

image

Screen.Recording.2024-12-17.at.10.29.50.mp4

Testing

(A) Auto play disabled by default

  • Update auto play in settings to false
  • Open 2 videos without playlist, confirm both got auto play disabled
  • Update global setting to true, confirm both still got auto play disabled
  • Update global setting to false, update one/both auto play to true, confirm global setting still false
  • Pick 1 window (can test on all windows but not necessary), jump to next video, confirm local state unchanged (still true)
  • Pick 1 window, get to any non watch page, get back, confirm local state set to global state

(B)

  • Steps of (A) but value reversed

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

Existing settings which can be changed in watch page and acts default value
Screenshot 2024-12-17 at 10 33 17

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 17, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 17, 2024 02:38
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

Ooh, I like it! I have three thoughts:

  1. issue (minor, non-blocking): If we're doing it one way for recommended video autoplay, we should be doing it the same way for playlist autoplay as well, otherwise it's a bit jarring / confusing as a user to read. I get that that's done because playlist autoplay is currently not toggleable from the player page (until Add autoplay toggle to the video player #5866), but I'd recommend only merging this if we're confident we can get that other part included in the same release.
  2. nitpick: Maybe we should colocate the autoplay buttons on the left side now so the "by Default"s are by one another.
  3. nitpick: The "by Default" phrasing is reasonable to use here and consistent, but in general, I do think it makes our labels more verbose. Maybe in this or a separate PR we move all of these to its own sub-section of "Default Settings" or something like that?

@PikachuEXE PikachuEXE force-pushed the feature/play-next-by-default branch from 6202b60 to 0620083 Compare December 20, 2024 10:22
@PikachuEXE
Copy link
Collaborator Author

Removed usage of sessionStore (stupid move + I am vue newbie?
I shouldn't use sessionStore for something only last for the the duration of Watch page

(1) see commit 2
(2) Not quite sure what you mean. Autoplay Videos probably isn't included, so playlist + recommended videos?
(3) Might need a separate PR, too many things to agree on and testing (makes this PR too complicated). We can discuss here but better be done after we did (1) and maybe (2) and other main changes first

About (1) I did not make any change to pause button due to #5866
But if that PR might take longer I can change the pause button to the toggle like Up Next

@efb4f5ff-1298-471a-8973-3d47447115dc

@kommunarr i think that pika is waiting on you to respond to their latest message

@kommunarr
Copy link
Collaborator

(2) Not quite sure what you mean. Autoplay Videos probably isn't included, so playlist + recommended videos?

Yes, if we are to update the logic and names of both, all the "by Default"s should be in the same column. Although like I said in 3, I'm still not a fan of the "by Default" being there in our setting names, as I feel like it adds more wordiness than improved user comprehension

@PikachuEXE
Copy link
Collaborator Author

So just remove by Default label text change? Fine for me
Pending #5866 for merge conflict storm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants