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

Update autoplay next recommended/playlist video setting to be "by default" #6400

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Update autoplay next recommended/playlist video setting to be "by default" #6400

merged 1 commit into from
Jan 27, 2025

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Dec 17, 2024 •
edited
Loading

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

This PR does:

  • Update behaviour of these settings
    • Autoplay Recommended Videos
    • Autoplay Playlist Videos
  • Above settings now affect the video playing session's initial values 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

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

Testing

(A1) Autoplay Recommended Videos 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

(A2)

  • Steps of (A1) but value reversed

(B1) Autoplay Playlist Videos disabled by default

  • Update auto play in settings to false
  • Open 2 videoswith 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

(B2)

  • Steps of (B1) 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
@kommunarr kommunarr mentioned this pull request Dec 17, 2024
Merged
1 task
kommunarr
kommunarr reviewed Dec 20, 2024
View reviewed changes
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

@PikachuEXE PikachuEXE mentioned this pull request Dec 24, 2024
Merged
1 task
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

@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

@PikachuEXE
Copy link
Collaborator Author

Version based on #5866 actually already done and being used by myself
Just waiting for that to be merged before I push those changes...

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jan 14, 2025
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@PikachuEXE PikachuEXE changed the title Update play next recommended video setting to be "by default" Update play next recommended/playlist video setting to be "by default" Jan 15, 2025
@PikachuEXE PikachuEXE changed the title Update play next recommended/playlist video setting to be "by default" Update autoplay next recommended/playlist video setting to be "by default" Jan 15, 2025
@PikachuEXE PikachuEXE force-pushed the feature/play-next-by-default branch from 1c90248 to 6988caf Compare January 15, 2025 00:50
@PikachuEXE PikachuEXE added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: merge conflicts / rebase needed labels Jan 15, 2025
@PikachuEXE
Copy link
Collaborator Author

PR title, body, code updated to be based on #5866

@PikachuEXE PikachuEXE requested a review from kommunarr January 15, 2025 00:52
@PikachuEXE
Copy link
Collaborator Author

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

image

@FreeTubeBot FreeTubeBot merged commit a5585e1 into FreeTubeApp:development Jan 27, 2025
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 27, 2025
SudharakaP pushed a commit to SudharakaP/FreeTube that referenced this pull request Jan 28, 2025
@PikachuEXE @SudharakaP
* Update play next recommended video setting to be "by default" (Free…
b8ea990 
…TubeApp#6400)
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jan 29, 2025
@PikachuEXE
Merge branch 'development' into custom-builds/tmp
54c3c02 
* development: (35 commits)
  Shrink mime-db even further (FreeTubeApp#6659)
  * Update play next recommended video setting to be "by default" (FreeTubeApp#6400)
  Miscellaneous performance improvements (FreeTubeApp#6658)
  Bump stylelint in the stylelint group across 1 directory (FreeTubeApp#6660)
  Bump the stylelint group across 1 directory with 4 updates (FreeTubeApp#6605)
  Fixes FreeTubeApp#5476: Adjusted z-index for tooltips to avoid overlapping with bars (FreeTubeApp#6656)
  Bump shaka-player from 4.12.8 to 4.13.0 (FreeTubeApp#6649)
  Migrate ProfileSettings, FtProfileBubble and FtProfileEdit to the composition API (FreeTubeApp#6639)
  Translated using Weblate (Arabic)
  Bump the eslint group with 4 updates (FreeTubeApp#6645)
  Bump bgutils-js from 3.1.2 to 3.1.3 (FreeTubeApp#6650)
  Translated using Weblate (Arabic)
  Translated using Weblate (Arabic)
  Bump electron from 34.0.0 to 34.0.1 (FreeTubeApp#6648)
  Bump lefthook from 1.10.9 to 1.10.10 (FreeTubeApp#6647)
  Bump the babel group with 2 updates (FreeTubeApp#6644)
  Translated using Weblate (Arabic)
  Avoid logging an error when a player cache entry does not exist (FreeTubeApp#6640)
  Move saving screenshots to the default folder to an IPC call (FreeTubeApp#6636)
  Replace rimraf dev dependency with clean script (FreeTubeApp#6638)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kommunarr kommunarr kommunarr approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@absidue absidue absidue approved these changes

@ChunkyProgrammer ChunkyProgrammer Awaiting requested review from ChunkyProgrammer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@PikachuEXE @efb4f5ff-1298-471a-8973-3d47447115dc @kommunarr @absidue @FreeTubeBot