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 to avoid duplicate chapters by building a new list each time #3354

Closed
wants to merge 9 commits into from
Closed

Fix to avoid duplicate chapters by building a new list each time #3354

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8a9944a
Fix to avoid duplicate chapters by building a new list each time
vdeshpande Jun 1, 2016
be8d2e8
Adding in the desired workflow changes
vdeshpande Jun 2, 2016
8740f0c
Adding in changes to make sure tests pass and put forth extra work-fl…
vdeshpande Jun 8, 2016
d575e93
Adding in further changes to the workflow logic based on comments
vdeshpande Jun 8, 2016
32f7bbd
Adding singlequote to string chapters instead of double quotes
vdeshpande Jun 8, 2016
426c0a8
More code changes to the fix
vdeshpande Jun 9, 2016
a70e2c4
Removing extra whitespaces as pointed out
vdeshpande Jun 9, 2016
399d85e
Fixing one sneaky whitespace found
vdeshpande Jun 9, 2016
842df6a
Removing not needed carriage rerurns
vdeshpande Jun 9, 2016
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: 0 additions & 2 deletions src/js/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,6 @@ class Component {
*/
dispose() {
this.trigger({ type: 'dispose', bubbles: false });

// Dispose all children.
if (this.children_) {
for (let i = this.children_.length - 1; i >= 0; i--) {
Copy link
Member

Choose a reason for hiding this comment

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

why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment back

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be showing up as changed in the diff.

if (this.children_[i].dispose) {
Expand Down
32 changes: 25 additions & 7 deletions src/js/control-bar/text-track-controls/chapters-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,11 @@ class ChaptersButton extends TextTrackButton {
createMenu() {
let tracks = this.player_.textTracks() || [];
let chaptersTrack;
let items = this.items = [];
let items = this.items || [];

for (let i = 0, length = tracks.length; i < length; i++) {
for (let i = tracks.length - 1; i >= 0; i--) {

// We will always choose the last track as our chaptersTrack
let track = tracks[i];

if (track['kind'] === this.kind_) {
Expand All @@ -97,6 +99,13 @@ class ChaptersButton extends TextTrackButton {
});
menu.children_.unshift(title);
Dom.insertElFirst(title, menu.contentEl());
this.addChild(menu);
Copy link
Member

Choose a reason for hiding this comment

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

should this line just not be here?

} else {
// We will empty out the menu children each time because we want a
// fresh new menu child list each time
items.forEach(item => menu.removeChild(item));
// Empty out the ChaptersButton menu items because we no longer need them
items = [];
}

if (chaptersTrack && chaptersTrack.cues == null) {
Expand Down Expand Up @@ -124,17 +133,26 @@ class ChaptersButton extends TextTrackButton {

menu.addChild(mi);
}

this.addChild(menu);
}

if (this.items.length > 0) {
if (items.length > 0) {
this.show();
}

// Assigning the value of items back to this.items for next iteration
this.items = items;
return menu;
}

// Overriding the update button because duplicate chapters get created each time when
// we try to put the chapter title into the DOM

update() {
if(this.children_.length <= 1) {
this.firstCall = true;
super.update();
Copy link
Member

Choose a reason for hiding this comment

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

This can still be called if a non-chapters track is added. We should make sure that it only does anything when chapters are added.
The update method is an event handler.

} else if(this.children_.length > 1) {
this.children_.pop();
}
}
}

ChaptersButton.prototype.kind_ = 'chapters';
Expand Down
5 changes: 5 additions & 0 deletions src/js/menu/menu-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ class MenuButton extends ClickableComponent {
if (this.menu) {
this.removeChild(this.menu);
}
// We need to remove the menu because we are already adding one before.
// This can create duplicate menu items causing tests to fail
if(this.menu === undefined && this.firstCall) {
Copy link
Member

Choose a reason for hiding this comment

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

this duplicated the code from lines 41-34.

this.removeChild(menu);
}

this.menu = menu;
this.addChild(menu);
Expand Down
25 changes: 14 additions & 11 deletions src/js/utils/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,17 +171,20 @@ const elIdAttr = 'vdata' + (new Date()).getTime();
* @function getElData
*/
export function getElData(el) {
let id = el[elIdAttr];

if (!id) {
id = el[elIdAttr] = Guid.newGUID();
}

if (!elData[id]) {
elData[id] = {};
}

return elData[id];
//if(el) {
Copy link
Member

Choose a reason for hiding this comment

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

work-in-progress checked in.


Copy link
Member

Choose a reason for hiding this comment

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

extra whitespace.

let id = el[elIdAttr];
Copy link
Member

Choose a reason for hiding this comment

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

The indent is off. It shouldn't be showing up as changed in the diff.


if (!id) {
id = el[elIdAttr] = Guid.newGUID();
}

if (!elData[id]) {
elData[id] = {};
}

return elData[id];
//}
}

/**
Expand Down