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

Feature: remember playback rate in watch session #6449

Merged
merged 3 commits into from
Dec 27, 2024
Merged

Feature: remember playback rate in watch session #6449

merged 3 commits into from
Dec 27, 2024
+23 −11

Conversation

ikizey
Copy link
Contributor

@ikizey ikizey commented Dec 24, 2024 •
edited by absidue
Loading

Pull Request Type

  • Feature Implementation

Related issue

Part of #2295

Description

Playback rate resets to default value only when leave watch view, not on every video, like it is now.

Testing

Manually tested playback rate doesn't change when you select another video, on autoplay, on next video in playlist. Resets to default, when you leave watch view.

Additional context

Closing issue created by me.

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

Hi @ikizey, thank you for working on this. I do think this idea needs more discussion before deciding on an implementation. Here are my two cents:

  • This does not needs a new setting; reasonable defaults are better than total configurability, the latter of which can lead to "settings overload".
  • Instead of permanent changes to the value, people are most likely expecting it to work like theater mode does currently, where changing the value affects it just for the session instead.
  • Someone can correct me if I'm wrong on this, but I believe that we require that all non-English translations are to be done through Weblate. If we wanted to use an automated tool for generating these strings when no manual translation existed, we would configure that through Weblate.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

I agree with @kommunarr. There is no extra setting needed here. Users sets default playback speed to 1x -> user watches video -> changes playback speed to 2x -> next video still has 2x speed -> User closes the application -> Open application -> watch video -> video uses default playback speed.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Dec 24, 2024
@PikachuEXE
Copy link
Collaborator

I would argue that it should be like #6400: exit watch page = reset to default value
Not sure what's the current behaviour (never use it

auto-merge was automatically disabled December 25, 2024 00:24

Head branch was pushed to by a user without write access

@ikizey ikizey force-pushed the feat-remember-playbackrate-setting branch from 3a2f88a to 8ecdf29 Compare December 25, 2024 00:24
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 25, 2024 00:24
@ikizey
Copy link
Contributor Author

ikizey commented Dec 25, 2024

@kommunarr @efb4f5ff-1298-471a-8973-3d47447115dc
Right now, theater mode works this way:
you open video (watch view), theater mode sets to default; you change mode; you change video; setting persists; you leave watch view to subscriptions for example, you open another video, mode resets (sets to default). That is not, exactly, what you are saying.

I updated the code to do the same thing with playback rate. (PR description updated)

kommunarr
kommunarr previously approved these changes Dec 25, 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.

Great work!

@kommunarr kommunarr added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Dec 25, 2024
@absidue
Copy link
Member

absidue commented Dec 25, 2024

I have fixed the issue linked in the pull request description to point to the correct one for you (the one you had originally linked was your own issue that was closed as a duplicate of an existing one).

@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Dec 25, 2024
@ikizey
refactor: update playback rate handling in ft-shaka-video-player and …
4c8c245 
…Watch components

- Changed default playback rate from 1.0 to 1 in ft-shaka-video-player.js
- Renamed event from 'current-playback-rate-update' to 'playback-rate-updated' in ft-shaka-video-player.js and Watch.vue
- Updated Watch.js to initialize currentPlaybackRate to null and set it based on store value in mounted lifecycle hook
auto-merge was automatically disabled December 26, 2024 06:11

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 26, 2024 06:12
@ikizey ikizey changed the title Feat remember playbackrate setting Feature remember playback rate in watch session Dec 26, 2024
@ikizey ikizey changed the title Feature remember playback rate in watch session Feature: remember playback rate in watch session Dec 26, 2024
auto-merge was automatically disabled December 26, 2024 15:34

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 26, 2024 15:34
@kommunarr kommunarr requested a review from absidue December 27, 2024 15:57
efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc approved these changes Dec 27, 2024
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

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

one step closer to #2295. Feel free to take a stab at more player controls

@FreeTubeBot FreeTubeBot merged commit 6d4c2c5 into FreeTubeApp:development Dec 27, 2024
6 checks passed
SuperAKWA pushed a commit to SuperAKWA/FreeTube that referenced this pull request Jan 24, 2025
@ikizey
Feature: remember playback rate in watch session (FreeTubeApp#6449)
07c10b3 
* feat: use state in Watch for playback rate value instead defaultPlayback store value directly

* refactor: update playback rate handling in ft-shaka-video-player and Watch components

- Changed default playback rate from 1.0 to 1 in ft-shaka-video-player.js
- Renamed event from 'current-playback-rate-update' to 'playback-rate-updated' in ft-shaka-video-player.js and Watch.vue
- Updated Watch.js to initialize currentPlaybackRate to null and set it based on store value in mounted lifecycle hook

* refactor: simplify playback rate initialization in ft-shaka-video-player and Watch components
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

@PikachuEXE PikachuEXE Awaiting requested review from PikachuEXE

@ChunkyProgrammer ChunkyProgrammer Awaiting requested review from ChunkyProgrammer

@MarmadileManteater MarmadileManteater Awaiting requested review from MarmadileManteater

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

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