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

fix: audio and video track changes #5890

Merged
merged 2 commits into from
Mar 25, 2019
Merged

fix: audio and video track changes #5890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
52c8cc7
catch issues with tests
brandonocasey Mar 25, 2019
7373a09
fix: Audio/Video track selection issues
brandonocasey Mar 25, 2019
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
33 changes: 16 additions & 17 deletions src/js/tracks/audio-track-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,33 +72,32 @@ class AudioTrackList extends TrackList {
return;
}

if (!this.enabledChange_) {
this.enabledChange_ = () => {
// when we are disabling other tracks (since we don't support
// more than one track at a time) we will set changing_
// to true so that we don't trigger additional change events
if (this.changing_) {
return;
}
this.changing_ = true;
disableOthers(this, track);
this.changing_ = false;
this.trigger('change');
};
}
track.enabledChange_ = () => {
// when we are disabling other tracks (since we don't support
// more than one track at a time) we will set changing_
// to true so that we don't trigger additional change events
if (this.changing_) {
return;
}
this.changing_ = true;
disableOthers(this, track);
this.changing_ = false;
this.trigger('change');
};

/**
* @listens AudioTrack#enabledchange
* @fires TrackList#change
*/
track.addEventListener('enabledchange', this.enabledChange_);
track.addEventListener('enabledchange', track.enabledChange_);
}

removeTrack(rtrack) {
super.removeTrack(rtrack);

if (rtrack.removeEventListener && this.enabledChange_) {
rtrack.removeEventListener('enabledchange', this.enabledChange_);
if (rtrack.removeEventListener && rtrack.enabledChange_) {
rtrack.removeEventListener('enabledchange', rtrack.enabledChange_);
rtrack.enabledChange_ = null;
}
}
}
Expand Down
27 changes: 13 additions & 14 deletions src/js/tracks/video-track-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,30 +87,29 @@ class VideoTrackList extends TrackList {
return;
}

if (!this.selectedChange_) {
this.selectedChange_ = () => {
if (this.changing_) {
return;
}
this.changing_ = true;
disableOthers(this, track);
this.changing_ = false;
this.trigger('change');
};
}
track.selectedChange_ = () => {
if (this.changing_) {
return;
}
this.changing_ = true;
disableOthers(this, track);
this.changing_ = false;
this.trigger('change');
};

/**
* @listens VideoTrack#selectedchange
* @fires TrackList#change
*/
track.addEventListener('selectedchange', this.selectedChange_);
track.addEventListener('selectedchange', track.selectedChange_);
}

removeTrack(rtrack) {
super.removeTrack(rtrack);

if (rtrack.removeEventListener && this.selectedChange_) {
rtrack.removeEventListener('selectedchange', this.selectedChange_);
if (rtrack.removeEventListener && rtrack.selectedChange_) {
rtrack.removeEventListener('selectedchange', rtrack.selectedChange_);
rtrack.selectedChange_ = null;
}
}
}
Expand Down
10 changes: 5 additions & 5 deletions test/unit/tracks/audio-track-list.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ QUnit.test('only one track is ever enabled', function(assert) {
assert.equal(track2.enabled, false, 'track2 is disabled');
assert.equal(track3.enabled, true, 'track3 is enabled');

track.enabled = true;
assert.equal(track.enabled, true, 'track is disabled');
assert.equal(track2.enabled, false, 'track2 is disabled');
track2.enabled = true;
assert.equal(track.enabled, false, 'track is disabled');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We did not test to make sure that a track that is not enabled and already added, would be enabled correctly. Same with video tracks

assert.equal(track2.enabled, true, 'track2 is enabled');
assert.equal(track3.enabled, false, 'track3 is disabled');

list.addTrack(track4);
assert.equal(track.enabled, true, 'track is enabled');
assert.equal(track2.enabled, false, 'track2 is disabled');
assert.equal(track.enabled, false, 'track is disabled');
assert.equal(track2.enabled, true, 'track2 is enabled');
assert.equal(track3.enabled, false, 'track3 is disabled');
assert.equal(track4.enabled, false, 'track4 is disabled');

Expand Down
10 changes: 5 additions & 5 deletions test/unit/tracks/video-track-list.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ QUnit.test('only one track is ever selected', function(assert) {
assert.equal(track2.selected, false, 'track2 is unselected');
assert.equal(track3.selected, true, 'track3 is selected');

track.selected = true;
assert.equal(track.selected, true, 'track is unselected');
assert.equal(track2.selected, false, 'track2 is unselected');
track2.selected = true;
assert.equal(track.selected, false, 'track is unselected');
assert.equal(track2.selected, true, 'track2 is selected');
assert.equal(track3.selected, false, 'track3 is unselected');

list.addTrack(track4);
assert.equal(track.selected, true, 'track is selected');
assert.equal(track2.selected, false, 'track2 is unselected');
assert.equal(track.selected, false, 'track is unselected');
assert.equal(track2.selected, true, 'track2 is selected');
assert.equal(track3.selected, false, 'track3 is unselected');
assert.equal(track4.selected, false, 'track4 is unselected');

Expand Down