Skip to content

Commit

Permalink
feat: Change addRemoteTextTrack's manualCleanup option default value …
Browse files Browse the repository at this point in the history
…to false (#7588)
  • Loading branch information
alex-barstow authored and misteroneill committed May 19, 2022
1 parent cd93783 commit 15d8f00
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 40 deletions.
9 changes: 3 additions & 6 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -4521,23 +4521,20 @@ class Player extends Component {

/**
* Create a remote {@link TextTrack} and an {@link HTMLTrackElement}.
* When manualCleanup is set to false, the track will be automatically removed
* on source changes.
*
* @param {Object} options
* Options to pass to {@link HTMLTrackElement} during creation. See
* {@link HTMLTrackElement} for object properties that you should use.
*
* @param {boolean} [manualCleanup=true] if set to false, the TextTrack will be
* removed on a source change
* @param {boolean} [manualCleanup=false] if set to true, the TextTrack will not be removed
* from the TextTrackList and HtmlTrackElementList
* after a source change
*
* @return {HtmlTrackElement}
* the HTMLTrackElement that was created and added
* to the HtmlTrackElementList and the remote
* TextTrackList
*
* @deprecated The default value of the "manualCleanup" parameter will default
* to "false" in upcoming versions of Video.js
*/
addRemoteTextTrack(options, manualCleanup) {
if (this.tech_) {
Expand Down
8 changes: 4 additions & 4 deletions src/js/tech/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -906,12 +906,12 @@ class Html5 extends Tech {
*
* @param {Object} options The object should contain values for
* kind, language, label, and src (location of the WebVTT file)
* @param {boolean} [manualCleanup=true] if set to false, the TextTrack will be
* automatically removed from the video element whenever the source changes
* @param {boolean} [manualCleanup=false] if set to true, the TextTrack
* will not be removed from the TextTrackList and HtmlTrackElementList
* after a source change
* @return {HTMLTrackElement} An Html Track Element.
* This can be an emulated {@link HTMLTrackElement} or a native one.
* @deprecated The default value of the "manualCleanup" parameter will default
* to "false" in upcoming versions of Video.js
*
*/
addRemoteTextTrack(options, manualCleanup) {
const htmlTrackElement = super.addRemoteTextTrack(options, manualCleanup);
Expand Down
13 changes: 4 additions & 9 deletions src/js/tech/tech.js
Original file line number Diff line number Diff line change
Expand Up @@ -758,32 +758,27 @@ class Tech extends Component {
* @param {Object} options
* See {@link Tech#createRemoteTextTrack} for more detailed properties.
*
* @param {boolean} [manualCleanup=true]
* @param {boolean} [manualCleanup=false]
* - When false: the TextTrack will be automatically removed from the video
* element whenever the source changes
* - When True: The TextTrack will have to be cleaned up manually
*
* @return {HTMLTrackElement}
* An Html Track Element.
*
* @deprecated The default functionality for this function will be equivalent
* to "manualCleanup=false" in the future. The manualCleanup parameter will
* also be removed.
*/
addRemoteTextTrack(options = {}, manualCleanup) {
const htmlTrackElement = this.createRemoteTextTrack(options);

if (manualCleanup !== true && manualCleanup !== false) {
// deprecation warning
log.warn('Calling addRemoteTextTrack without explicitly setting the "manualCleanup" parameter to `true` is deprecated and default to `false` in future version of video.js');
manualCleanup = true;
if (typeof manualCleanup !== 'boolean') {
manualCleanup = false;
}

// store HTMLTrackElement and TextTrack to remote list
this.remoteTextTrackEls().addTrackElement_(htmlTrackElement);
this.remoteTextTracks().addTrack(htmlTrackElement.track);

if (manualCleanup !== true) {
if (manualCleanup === false) {
// create the TextTrackList if it doesn't exist
this.ready(() => this.autoRemoteTextTracks_.addTrack(htmlTrackElement.track));
}
Expand Down
3 changes: 0 additions & 3 deletions test/unit/player.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2061,7 +2061,6 @@ QUnit.test('player#reset removes the poster', function(assert) {
});

QUnit.test('player#reset removes remote text tracks', function(assert) {
sinon.stub(log, 'warn');
const player = TestHelpers.makePlayer();

this.clock.tick(1);
Expand All @@ -2076,8 +2075,6 @@ QUnit.test('player#reset removes remote text tracks', function(assert) {
assert.strictEqual(player.remoteTextTracks().length, 1, 'there is one RTT');
player.reset();
assert.strictEqual(player.remoteTextTracks().length, 0, 'there are zero RTTs');
assert.strictEqual(log.warn.callCount, 1, 'one warning about for manualCleanup');
log.warn.restore();
});

QUnit.test('player#reset progress bar', function(assert) {
Expand Down
22 changes: 4 additions & 18 deletions test/unit/tech/tech.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,14 +219,8 @@ QUnit.test('dispose() should clear all tracks that are added after creation', fu
assert.equal(tech.textTracks().length, 0, 'should have zero video tracks after dispose');
});

QUnit.test('switching sources should clear all remote tracks that are added with manualCleanup = false', function(assert) {

QUnit.test('switching sources should clear all remote tracks that are added with the default manualCleanup = false', function(assert) {
const oldLogWarn = log.warn;
let warning;

log.warn = function(wrning) {
warning = wrning;
};

// Define a new tech class
const MyTech = extend(Tech);
Expand Down Expand Up @@ -254,18 +248,10 @@ QUnit.test('switching sources should clear all remote tracks that are added with
// set the initial source
tech.setSource({src: 'foo.mp4', type: 'mp4'});

// default value for manualCleanup is true
tech.addRemoteTextTrack({});
this.clock.tick(1);

assert.equal(
warning,
'Calling addRemoteTextTrack without explicitly setting the "manualCleanup" parameter to `true` is deprecated and default to `false` in future version of video.js',
'we log a warning when `addRemoteTextTrack` is called without a manualCleanup argument'
);

// should not be automatically cleaned up when source changes
tech.addRemoteTextTrack({}, true);
// should be automatically cleaned up when source changes
tech.addRemoteTextTrack({}, false);
tech.addRemoteTextTrack({});
this.clock.tick(1);

assert.equal(tech.textTracks().length, 2, 'should have two text tracks at the start');
Expand Down

0 comments on commit 15d8f00

Please sign in to comment.