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

Bug in volume option #1737

Open
aubreyz opened this issue Mar 20, 2020 · 5 comments
Open

Bug in volume option #1737

aubreyz opened this issue Mar 20, 2020 · 5 comments

Comments

@aubreyz
Copy link

aubreyz commented Mar 20, 2020

Expected behaviour

Volume option should set initial volume, e.g
volume: 0.9,

Actual behaviour

Volume is not responsive at all to this option, either in video or audio, e.g in

Steps to reproduce

e.g in

const player = Plyr.setup('.js-playerv', {
volume: 1.0,
tooltips: { controls: true },
captions: { defaultActive: true }
});

However a direct change via API does work, e.g

player.volume = 0.75;

In fact pointed out previously in a somewhat obscure fashion, but not sure why nobody has noticed this before. #848

Environment

  • Browser:
  • Version:
  • Operating System:
  • Version:

Console errors (if any)

Link to where the bug is happening

@sampotts
Copy link
Owner

I think this is caused by the local storage that stores the user preferences. I'll find a way to force the update if it's passed via the API.

@liesahead
Copy link
Contributor

+1, same with captions: { active: true } on init

@mum-never-proud
Copy link

mum-never-proud commented Mar 27, 2020

problem for volume

this.volume = null;

I don't think we need a reset we can go with user config

plyr/src/js/plyr.js

Lines 538 to 545 in fcd8208

if (!is.number(volume)) {
volume = this.storage.get('volume');
}
// Use config if all else fails
if (!is.number(volume)) {
({ volume } = this.config);
}

when build is called in ui.js we are resetting the volume to null which calls the setter method volume, the first preference is given to local storage

problem for captions

plyr/src/js/captions.js

Lines 95 to 98 in fcd8208

let active = this.storage.get('captions');
if (!is.boolean(active)) {
({ active } = this.config.captions);
}

we are giving pref to local storage first rather than config which in my opinion is wrong

I can raise a pr in some time if no one is working on it

any reason we are resetting the values instead of reading from the config?

plyr/src/js/ui.js

Lines 65 to 77 in fcd8208

this.volume = null;
// Reset mute state
this.muted = null;
// Reset loop state
this.loop = null;
// Reset quality setting
this.quality = null;
// Reset speed
this.speed = null;

cc: @sampotts

@sampotts
Copy link
Owner

If we always read from config, what is the point of storing the user settings? There's definitely some improvement to be made but ignoring the user settings isn't the answer. If the user sets volume low, then you change source and it shoots up to 100%, it's not a great experience.

@liesahead
Copy link
Contributor

For us the initial captions and volume options do not work even when we explicitly disable user options storage.

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

No branches or pull requests

4 participants