Fix to avoid duplicate chapters by building a new list each time #3354
Conversation
| let track = tracks[i]; | ||
|
|
||
| // We will always choose the last track as our chaptersTrack | ||
| let track = tracks[length - 1]; |
There was a problem hiding this comment.
wouldn't this always choose the last track? Don't we want the last chapters track?
There was a problem hiding this comment.
Also, the track chosen here and in createItems is now different.
There was a problem hiding this comment.
Actually, does this even need createItems since it's all happening in there? We should move the item creation login from this method into createItems.
There was a problem hiding this comment.
I think for the last chapters track, due to the break statement....it does not always choose the last track.
Let's say we have multiple chapters list and if one the in-between chapters kind matches ; it will choose a track from in-between of the list and break out from the loop.
I wanted to always make sure that we are taking the last track no matter how big the list is.
Also track chosen in createItems would be different, but we are popping out the items in createMenu while always maintaining a chapters list, so that would not be an issue I think.
Well we actually do not need createItems if we decide to take this approach.
There was a problem hiding this comment.
but length never changes, so, it'll always choose track[track.length - 1]
There was a problem hiding this comment.
I think moving things there is good because it'll be consistent with the other menu buttons.
There was a problem hiding this comment.
Ok, will try to see if I can move things around. :)
Regarding choosing the last track, earlier it was like this
for loop {
let track = tracks[i]
(if track matches kind as chapters) {
chaptersTrack = track;
break;
}
}
Now let's say i = 1 and kind matches with chapters,
but we want the 2nd track( i = 2) to be the correct and most recent
chapters Track that has to be added to chapters list
This will cause the chaptersTrack to always take value of the first track, and break out from the loop.
If this happens, the last most recent track added never gets added to the list as chaptersTrack will always take value of the first previous track. Would this be not the case ?
| @@ -97,6 +99,16 @@ class ChaptersButton extends TextTrackButton { | |||
| }); | |||
| menu.children_.unshift(title); | |||
| Dom.insertElFirst(title, menu.contentEl()); | |||
| this.addChild(menu); | |||
| } | |||
| else { | |||
There was a problem hiding this comment.
I think this should be on the previous line (i.e. } else {).
| @@ -105,8 +105,6 @@ class Component { | |||
| */ | |||
| dispose() { | |||
| this.trigger({ type: 'dispose', bubbles: false }); | |||
|
|
|||
| // Dispose all children. | |||
There was a problem hiding this comment.
Added the comment back
There was a problem hiding this comment.
This shouldn't be showing up as changed in the diff.
| elData[id] = {}; | ||
| } | ||
|
|
||
| let id = el[elIdAttr]; |
There was a problem hiding this comment.
The indent is off. It shouldn't be showing up as changed in the diff.
| @@ -105,7 +105,7 @@ class Component { | |||
| */ | |||
| dispose() { | |||
| this.trigger({ type: 'dispose', bubbles: false }); | |||
|
|
|||
gkatsev
added
patch
|
LGTM |
1 similar comment
|
LGTM |
Description
Duplicate chapters list gets added to the existing chapters item list whenever we try adding a chapter using
addRemoteTextTrack()This PR is to make sure that duplicate chapters do not get added to that list.
Specific Changes proposed
Changes proposed within this PR are as follows:
This approach makes it easy to avoid duplicate chapters and only provide a new chapter item list whenever the user tries to create one.
Requirements Checklist