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

init options bug #1761

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

init options bug #1761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/js/captions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ const captions = {
// When passive, don't override user preferences
if (!passive) {
this.captions.active = active;
this.storage.set({ captions: active });
this.storage.set({ captions: this.captions });
}

// Force language if the call isn't passive and there is no matching language to toggle to
Expand Down
2 changes: 1 addition & 1 deletion src/js/listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ class Listeners {
controls.updateSetting.call(player, 'speed');

// Save to storage
player.storage.set({ speed: player.speed });
player.storage.set({ speed: player.config.speed });
});

// Quality change
Expand Down
38 changes: 21 additions & 17 deletions src/js/plyr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { createElement, hasClass, removeElement, replaceElement, toggleClass, wr
import { off, on, once, triggerEvent, unbindListeners } from './utils/events';
import is from './utils/is';
import loadSprite from './utils/load-sprite';
import normalizeOptions from './utils/normalize-options';
import { clamp } from './utils/numbers';
import { cloneDeep, extend } from './utils/objects';
import { silencePromise } from './utils/promise';
Expand All @@ -37,7 +38,11 @@ import { parseUrl } from './utils/urls';

// Plyr instance
class Plyr {
constructor(target, options) {
constructor(target, options = {}) {
const { storage } = defaults;

this.storage = new Storage(storage.enabled, storage.key);

this.timers = {};

// State
Expand Down Expand Up @@ -65,18 +70,16 @@ class Plyr {
// Set config
this.config = extend(
{},
defaults,
Plyr.defaults,
options || {},
(() => {
try {
return JSON.parse(this.media.getAttribute('data-plyr-config'));
} catch (e) {
return {};
}
})(),
defaults,
this.storage.get() || {},
options,
this.dataPlayerConfig
);

this.config.speed = normalizeOptions(this.config.speed, defaults.speed);
this.config.quality = normalizeOptions(this.config.quality, defaults.quality);

// Elements cache
this.elements = {
container: null,
Expand Down Expand Up @@ -257,9 +260,6 @@ class Plyr {
// Create listeners
this.listeners = new Listeners(this);

// Setup local storage for user settings
this.storage = new Storage(this);

// Store reference
this.media.plyr = this;

Expand Down Expand Up @@ -403,6 +403,14 @@ class Plyr {
return Boolean(this.media.ended);
}

get dataPlayerConfig() {
try {
return JSON.parse(this.media.getAttribute('data-plyr-config'));
} catch (e) {
return {};
}
}

Comment on lines +406 to +413
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need this as a getter?

Copy link
Author

Choose a reason for hiding this comment

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

my bad we don't need a getter, but the current implementation is unreadable as well, it confuses the editor that the code below this line is not reachable

I guess we can have a method say parseJSON(jsonString) to handle this

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, it could just be a util really.

/**
* Toggle playback based on current status
* @param {Boolean} input
Expand Down Expand Up @@ -653,10 +661,6 @@ class Plyr {
speed = input;
}

if (!is.number(speed)) {
speed = this.storage.get('speed');
}

if (!is.number(speed)) {
speed = this.config.speed.selected;
}
Expand Down
6 changes: 3 additions & 3 deletions src/js/storage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import is from './utils/is';
import { extend } from './utils/objects';

class Storage {
constructor(player) {
this.enabled = player.config.storage.enabled;
this.key = player.config.storage.key;
constructor(enabled, key) {
this.enabled = enabled;
this.key = key;
}

// Check for actual support (see if we can use it)
Expand Down
20 changes: 6 additions & 14 deletions src/js/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,12 @@ const ui = {
captions.setup.call(this);
}

// Reset volume
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;
// set volume, mute, loop, quality, speed
this.volume = this.config.volume;
this.muted = this.config.muted;
this.loop = this.config.loop;
this.quality = this.config.quality;
this.speed = this.config.speed;

// Reset volume display
controls.updateVolume.call(this);
Expand Down
18 changes: 18 additions & 0 deletions src/js/utils/normalize-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
export default function(options, defaults) {
Copy link
Owner

Choose a reason for hiding this comment

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

Is this not just merging the two objects? In which case we can just use the extend helper in the object utils?

Copy link
Author

Choose a reason for hiding this comment

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

yep, it is! We have two options with multiple properties quality and speed

the current implementation will break if the initialization is like

const player = new Plyr('#player', {
    speed: {
        selected: 2
    }
});

it will break here since the user has not provided the options

plyr/src/js/controls.js

Line 1057 in 39558cb

this.options.speed = this.options.speed.filter(o => o >= this.minimumSpeed && o <= this.maximumSpeed);

the above code just makes sure that incorrect options don't break the code, it wasn't breaking previously since we were overriding user's config

if (!options) {
return defaults;
}

const normalizedOptions = {};

Object.keys(defaults)
.forEach(key => {
if (Object.prototype.hasOwnProperty.call(options, key)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why not use Object.keys(options).includes(key) ?

Copy link
Author

Choose a reason for hiding this comment

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

hasOwnProperty is faster with fewer properties say 100, I don't think there's gonna be more than 100 keys here (as of now)

Copy link
Owner

Choose a reason for hiding this comment

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

Readability > a probably very minor speed increase. You'll notice the rest of the code we use that style so let's stick with that. It's the sort of thing browsers will optimise anyway.

normalizedOptions[key] = options[key];
} else {
normalizedOptions[key] = defaults[key];
}
});

return normalizedOptions;
}
5 changes: 1 addition & 4 deletions src/js/utils/objects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,7 @@ export function extend(target = {}, ...sources) {

Object.keys(source).forEach(key => {
if (is.object(source[key])) {
if (!Object.keys(target).includes(key)) {
Object.assign(target, { [key]: {} });
}

Comment on lines -31 to -34
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason behind this change? You could potentially overwrite an existing value rather than merge.

Copy link
Author

Choose a reason for hiding this comment

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

I felt like extending an object should extend it unconditionally, rather than checking for the existence of keys in loop

Copy link
Owner

Choose a reason for hiding this comment

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

Did you test it? From looking at it, happens with your change will be that if the target already includes key, you're going to wipe it to be an empty object.

i.e.

const obj1 = { a: { b: 2, c: 3 }};
const obj2 = { a: { d: 4, e: 5 }};
const obj3 = extend(obj1, obj2);

You would expect obj3 to be { b: 2, c: 3, d: 4, e: 5 } but with your change it'll be { d: 4, e: 5 } because it wipes out the previous value.

Object.assign(target, { [key]: {} });
extend(target[key], source[key]);
} else {
Object.assign(target, { [key]: source[key] });
Expand Down