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

Deprecate options #2229

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
11 changes: 9 additions & 2 deletions src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class Component {
this.options_ = mergeOptions({}, this.options_);

// Updated options with supplied options
options = this.options(options);
options = this.options_ = mergeOptions(this.options_, options);

// Get ID from options or options element if one is supplied
this.id_ = options.id || (options.el && options.el.id);
Expand Down Expand Up @@ -186,6 +186,8 @@ class Component {
* @return {Object} A NEW object of this.options_ and obj merged
*/
options(obj) {
log.warn('this.options() has been deprecated and will be moved to the constructor in 6.0');
Copy link
Member

Choose a reason for hiding this comment

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

We might want to consider bringing in util-deprecate so warnings are only logged once. Not required for this PR.


if (!obj) {
return this.options_;
}
Expand Down Expand Up @@ -468,7 +470,8 @@ class Component {

if (children) {
// `this` is `parent`
let parentOptions = this.options();
let parentOptions = this.options_;

let handleAdd = (name, opts) => {
// Allow options for children to be set at the parent options
// e.g. videojs(id, { controlBar: false });
Expand All @@ -483,6 +486,10 @@ class Component {
return;
}

// We also want to pass the original player options to each component as well so they don't need to
// reach back into the player for options later.
opts.playerOptions = this.options_.playerOptions;

// Create and add the child component.
// Add a direct reference to the child by name on the parent instance.
// If two of the same component are used, different names should be supplied
Expand Down
19 changes: 14 additions & 5 deletions src/js/control-bar/playback-rate-menu/playback-rate-menu-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class PlaybackRateMenuButton extends MenuButton {
// Menu creation
createMenu() {
let menu = new Menu(this.player());
let rates = this.player().options()['playbackRates'];
let rates = this.playbackRates();

if (rates) {
for (let i = rates.length - 1; i >= 0; i--) {
Expand All @@ -64,10 +64,11 @@ class PlaybackRateMenuButton extends MenuButton {
handleClick() {
// select next rate option
let currentRate = this.player().playbackRate();
let rates = this.player().options()['playbackRates'];
let rates = this.playbackRates();

// this will select first one if the last one currently selected
let newRate = rates[0];
for (let i = 0; i <rates.length ; i++) {
for (let i = 0; i < rates.length ; i++) {
if (rates[i] > currentRate) {
newRate = rates[i];
break;
Expand All @@ -76,11 +77,19 @@ class PlaybackRateMenuButton extends MenuButton {
this.player().playbackRate(newRate);
}

playbackRates() {
return this.options_
|| this.options_['playbackRates']
|| this.options_.playerOptions
|| this.options_.playerOptions['playbackRates'];
}


playbackRateSupported() {
return this.player().tech
&& this.player().tech['featuresPlaybackRate']
&& this.player().options()['playbackRates']
&& this.player().options()['playbackRates'].length > 0
&& this.playbackRates()
&& this.playbackRates().length > 0
;
}

Expand Down
4 changes: 2 additions & 2 deletions src/js/menu/menu-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@ class MenuButton extends Button {
var menu = new Menu(this.player_);

// Add a title list item to the top
if (this.options().title) {
if (this.options_.title) {
menu.contentEl().appendChild(Dom.createEl('li', {
className: 'vjs-menu-title',
innerHTML: toTitleCase(this.options().title),
innerHTML: toTitleCase(this.options_.title),
tabIndex: -1
}));
}
Expand Down
2 changes: 1 addition & 1 deletion src/js/menu/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class Menu extends Component {
}

createEl() {
let contentElType = this.options().contentElType || 'ul';
let contentElType = this.options_.contentElType || 'ul';
this.contentEl_ = Dom.createEl(contentElType, {
className: 'vjs-menu-content'
});
Expand Down
10 changes: 7 additions & 3 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ class Player extends Component {
}, this);
Copy link
Member

Choose a reason for hiding this comment

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

Note: let's add player options to plugin options also, for the same reasons

}

// We also want to pass the original player options to each component as well so they don't need to
// reach back into the player for options later.
this.options_.playerOptions = mergeOptions(options, this.options_);

this.initChildren();

// Set isAudio based on whether or not an audio tag was used
Expand Down Expand Up @@ -1554,7 +1558,7 @@ class Player extends Component {
} else {
// We need to wrap this in a timeout to give folks a chance to add error event handlers
this.setTimeout( function() {
this.error({ code: 4, message: this.localize(this.options()['notSupportedMessage']) });
this.error({ code: 4, message: this.localize(this.options_['notSupportedMessage']) });
}, 0);

// we could not find an appropriate tech, but let's still notify the delegate that this is it
Expand Down Expand Up @@ -1917,7 +1921,7 @@ class Player extends Component {
// Clear any existing inactivity timeout to start the timer over
this.clearTimeout(inactivityTimeout);

var timeout = this.options()['inactivityTimeout'];
var timeout = this.options_['inactivityTimeout'];
if (timeout > 0) {
// In <timeout> milliseconds, if no more activity has occurred the
// user will be considered inactive
Expand Down Expand Up @@ -2117,7 +2121,7 @@ class Player extends Component {
}

toJSON() {
let options = mergeOptions(this.options());
let options = mergeOptions(this.options_);
let tracks = options.tracks;

options.tracks = [];
Expand Down
4 changes: 2 additions & 2 deletions src/js/slider/slider.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class Slider extends Component {
this.handle = this.getChild(this.options_['handleName']);

// Set a horizontal or vertical class on the slider depending on the slider type
this.vertical(!!this.options()['vertical']);
this.vertical(!!this.options_['vertical']);

this.on('mousedown', this.handleMouseDown);
this.on('touchstart', this.handleMouseDown);
Expand Down Expand Up @@ -117,7 +117,7 @@ class Slider extends Component {
let boxH = el.offsetHeight;
let handle = this.handle;

if (this.options()['vertical']) {
if (this.options_['vertical']) {
let boxY = box.top;

let pageY;
Expand Down
2 changes: 1 addition & 1 deletion src/js/tech/flash.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class Flash extends Tech {
}

createEl() {
let options = this.options();
let options = this.options_;

// Generate ID for swf object
let objId = options.techId;
Expand Down
7 changes: 4 additions & 3 deletions src/js/tech/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ class MediaLoader extends Component {

// If there are no sources when the player is initialized,
// load the first supported playback technology.
if (!player.options_['sources'] || player.options_['sources'].length === 0) {
for (let i=0, j=player.options_['techOrder']; i<j.length; i++) {

if (!options.playerOptions['sources'] || options.playerOptions['sources'].length === 0) {
for (let i=0, j=options.playerOptions['techOrder']; i<j.length; i++) {
let techName = toTitleCase(j[i]);
let tech = Component.getComponent(techName);

Expand All @@ -31,7 +32,7 @@ class MediaLoader extends Component {
// // Then load the best source.
// // A few assumptions here:
// // All playback technologies respect preload false.
player.src(player.options_['sources']);
player.src(options.playerOptions['sources']);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/js/tracks/text-track-display.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class TextTrackDisplay extends Component {

player.on('fullscreenchange', Fn.bind(this, this.updateDisplay));

let tracks = player.options_['tracks'] || [];
let tracks = this.options_.playerOptions['tracks'] || [];
for (let i = 0; i < tracks.length; i++) {
let track = tracks[i];
this.player_.addRemoteTextTrack(track);
Expand Down
7 changes: 5 additions & 2 deletions src/js/tracks/text-track-settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ class TextTrackSettings extends Component {
super(player, options);
this.hide();

// Grab `persistTextTrackSettings` from the player options if not passed in child options
this.options_.persistTextTrackSettings = options.persistTextTrackSettings || this.options_.playerOptions.persistTextTrackSettings;

Events.on(this.el().querySelector('.vjs-done-button'), 'click', Fn.bind(this, function() {
this.saveSettings();
this.hide();
Expand Down Expand Up @@ -39,7 +42,7 @@ class TextTrackSettings extends Component {
Events.on(this.el().querySelector('.vjs-edge-style select'), 'change', Fn.bind(this, this.updateDisplay));
Events.on(this.el().querySelector('.vjs-font-family select'), 'change', Fn.bind(this, this.updateDisplay));

if (player.options()['persistTextTrackSettings']) {
if (this.options_.persistTextTrackSettings) {
this.restoreSettings();
}
}
Expand Down Expand Up @@ -117,7 +120,7 @@ class TextTrackSettings extends Component {
}

saveSettings() {
if (!this.player_.options()['persistTextTrackSettings']) {
if (!this.options_.persistTextTrackSettings) {
return;
}

Expand Down
6 changes: 3 additions & 3 deletions test/unit/component.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ test('should do a deep merge of child options', function(){
}
});

var mergedOptions = comp.options();
var mergedOptions = comp.options_;
var children = mergedOptions['example'];

strictEqual(children['childOne']['foo'], 'baz', 'value three levels deep overridden');
Expand Down Expand Up @@ -132,7 +132,7 @@ test('should allows setting child options at the parent options level', function
} catch(err) {
ok(false, 'Child with `false` option was initialized');
}
equal(parent.children()[0].options()['foo'], true, 'child options set when children array is used');
equal(parent.children()[0].options_['foo'], true, 'child options set when children array is used');

// using children object
options = {
Expand All @@ -154,7 +154,7 @@ test('should allows setting child options at the parent options level', function
} catch(err) {
ok(false, 'Child with `false` option was initialized');
}
equal(parent.children()[0].options()['foo'], true, 'child options set when children object is used');
equal(parent.children()[0].options_['foo'], true, 'child options set when children object is used');
});

test('should dispose of component and children', function(){
Expand Down
4 changes: 2 additions & 2 deletions test/unit/player.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ test('should have a sensible toJSON that is equivalent to player.options', funct

const player = TestHelpers.makePlayer(playerOptions);

deepEqual(player.toJSON(), player.options(), 'simple player options toJSON produces output equivalent to player.options()');
deepEqual(player.toJSON(), player.options_, 'simple player options toJSON produces output equivalent to player.options_');

const playerOptions2 = {
tracks: [{
Expand All @@ -786,7 +786,7 @@ test('should have a sensible toJSON that is equivalent to player.options', funct

playerOptions2.tracks[0].player = player2;

const popts = player2.options();
const popts = player2.options_;
popts.tracks[0].player = undefined;

deepEqual(player2.toJSON(), popts, 'no circular references');
Expand Down