-
Notifications
You must be signed in to change notification settings - Fork 827
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
Setting loudness to default volume in new videos #3203
Setting loudness to default volume in new videos #3203
Conversation
video.js keeps track of the volume and muted settings on the video player separately, so I think the better approach would be to store the muted and volume properties separately in the session and set both on the player when it gets created. That way it will properly keep the volume and muted status across videos. |
Head branch was pushed to by a user without write access
Do we actually need the default volume? I feel like it might be confusing or unexpected behaviour from a users perspective if the player sometimes uses the volume from the last video and other times uses the default volume. diff --git a/src/renderer/components/ft-video-player/ft-video-player.js b/src/renderer/components/ft-video-player/ft-video-player.js
index 332ceb13..bdaa74cc 100644
--- a/src/renderer/components/ft-video-player/ft-video-player.js
+++ b/src/renderer/components/ft-video-player/ft-video-player.js
@@ -89,6 +89,7 @@ export default defineComponent({
id: '',
powerSaveBlocker: null,
volume: 1,
+ muted: false,
player: null,
useDash: false,
useHls: false,
@@ -315,11 +316,19 @@ export default defineComponent({
},
mounted: function () {
const volume = sessionStorage.getItem('volume')
+ const muted = sessionStorage.getItem('muted')
if (volume !== null) {
this.volume = volume
}
+ if (muted !== null) {
+ // sessionStorage only returns strings
+ // any string that isn't empty is considered truthy by the javascript runtime
+ // so we need to explicitly check for a string value of true, as the string "false" would be considered true as well
+ this.muted = muted === 'true'
+ }
+
this.dataSetup.playbackRates = this.playbackRates
this.createFullWindowButton()
@@ -400,6 +409,7 @@ export default defineComponent({
})
this.player.volume(this.volume)
+ this.player.muted(this.muted)
this.player.playbackRate(this.defaultPlayback)
this.player.textTrackSettings.setValues(this.defaultCaptionSettings)
// Remove big play button
@@ -712,10 +722,11 @@ export default defineComponent({
},
updateVolume: function (_event) {
- // 0 means muted
// https://docs.videojs.com/html5#volume
- const volume = this.player.muted() ? 0 : this.player.volume()
+ const volume = this.player.volume()
+ const muted = this.player.muted()
sessionStorage.setItem('volume', volume)
+ sessionStorage.setItem('muted', muted)
},
mouseScrollVolume: function (event) {
|
Actually, I was going for the fix as suggested in bug report. But yes, now I think this approach would be more suitable. from user's perspective. What should I do now? (Sorry I am a bit new to contributing on github in general), should I make these changes and commit them? or have you done that already in which case I reckon I should close this PR? |
You should make these changes and commit them 🙂 |
Head branch was pushed to by a user without write access
Pull request was closed
cb7be98
to
6c8088a
Compare
Sorry I made a mistake and ran some wrong commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default volume set to 70%
A) Not muted
- View a video, set volume to ~10%
- View another video, volume remains at ~10% (OK)
B) Muted
- View a video, set volume to ~10%, set as muted
- View another video, volume remains muted (OK)
- Unmute, volume jumps to 100% instead of default volume 70% (Not OK)
How are you muting it? If we directly click on mute icon or press 'm' to mute/unmute then volume should be set at previously muted volume however if we drag the volume slider to 0 then volume in sessionStorage is set to 0 and if you unmute now, it will set volume to 100% |
I only mute it by pressing the mute icon (not even 'm') |
…lume slider to zero. Volume and muted are kept track of seperately and both are set on player with respect to how it was set for previous video
Head branch was pushed to by a user without write access
@toby63 Problem is there can be a use case of listening to a list of videos known to need larger volume value |
@toby63 navigating from an unmuted video to another unmuted video, should keep the volume of the previous video, that's how it behaved before this pull request and that behaviour should really be kept, changing that now is going to annoy more people than it helps. |
Head branch was pushed to by a user without write access
Just in theory (see also below): Could this not be solved by identifying list videos?
Ok, I see that this is a situation where a design decision has to be made. |
Something like this iv-org/invidious#3600 could potentially be added but imo it's a different feature request and should be discussed in a different issue |
Are there any issues with commit that need be fixed now? |
Im a bit confused by reading all the comments. Could u explain to me what the expected behavior is with a few example scenarios |
Assume default volume set to 80% in user settings.
|
Uumm is there a reason for not closing #3125 |
* development: (106 commits) Translated using Weblate (Swedish) Translated using Weblate (Swedish) Translated using Weblate (Swedish) Translated using Weblate (Portuguese (Brazil)) Translated using Weblate (Swedish) Translated using Weblate (Slovak) Translated using Weblate (Galician) Setting loudness to default volume in new videos (FreeTubeApp#3203) Translated using Weblate (Bulgarian) Translated using Weblate (Russian) Skip the exists check for the databases, as stat does it anyway (FreeTubeApp#3272) Use smaller Vue esm runtime build (FreeTubeApp#3271) Translated using Weblate (Italian) Bump eslint-config-prettier from 8.6.0 to 8.7.0 (FreeTubeApp#3269) Bump lefthook from 1.3.1 to 1.3.3 (FreeTubeApp#3268) Bump rimraf from 4.1.2 to 4.3.0 (FreeTubeApp#3264) Bump video.js from 7.21.2 to 7.21.3 (FreeTubeApp#3265) Bump eslint-plugin-unicorn from 45.0.2 to 46.0.0 (FreeTubeApp#3266) Translated using Weblate (Georgian) Translated using Weblate (Lithuanian) ...
Title
Pull Request Type
Related issue
#3125
Partially fixes above (Pika: updated to NOT close the issue when this is merged)
With latest commit, the issue should be fixed completely and not partially. (Edit: predystopic-dev)
Description
When user mutes a video and opens new video, it will open with volume set to default volume set by user in settings. However, if video was not muted, and new video is opened, then volume will remain at it's original value.
Screenshots
n/a
Testing
Code is self-explanatory and it didn't affect anything else.
Desktop
Additional context
Instead of starting the new video muted, and settings it to default volume on unmuting, I made volume be set directly to default volume for new videos if previous video was muted by user. If someone can point me in the right direction, I can attempt to fix this issue as suggested in #3125 but I was unable to find the function responsible for settings the value back to 100% on unmuting, hence tried to partially fix the issue.