Added subtitles plugin #2
Conversation
|
@gvamshi-metrological I would like to ask you to also add some docs describing how this plugin should be used |
* Integrated subtitles into videoPlayer * fixed ES-Lint problems * Added workFlow file * Added github actions reporter * updated log * resolved eslint errors * Updated documents * Updated documents
There was a problem hiding this comment.
Please have a look at https://github.com/Metrological/metrological-sdk/pull/2/files#r1026209737
|
Thanks for the reviews @robbertvancaem (smile) , I have addressed your review comments Please have a look. |
.github/workflows/test.yml
Outdated
| @@ -0,0 +1,59 @@ | |||
| # Heavily based on https://joelhooks.com/jest-and-github-actions | |||
There was a problem hiding this comment.
@gvamshi-metrological Could you please remove this file from your PR? Regrettably, we can't use Github actions with our Github subscription. This will not change; the repo will most likely move to a Bitbucket environment somewhere in the upcoming months. So basically; we'll never get this to work I'm afraid 😩
github-actions-reporter.js
Outdated
| @@ -0,0 +1,36 @@ | |||
| /* eslint-disable no-console */ | |||
There was a problem hiding this comment.
@gvamshi-metrological Can you please remove this file, also because we can't use Github actions 😩
src/SubtitlesParser/index.js
Outdated
| Log.info('Invalid URL') | ||
| return Promise.reject(new Error('Invalid URL')) | ||
| } | ||
| if (!((_url.protocol === 'https:' || _url.protocol === 'http:') && _url.hostname)) { |
There was a problem hiding this comment.
Why do you need to explicitly check the hostname? I think it's always there, otherwise the new URL(url) part will fail, right?
| } else { | ||
| this._captions = this.parseSubtitles(subtitleData, parseOptions) | ||
| } | ||
| if (!this._captions.length) { |
There was a problem hiding this comment.
Why do you need this check? Technically, even when subtitles are successfully parsed but yield an empty array, they are not strictly invalid. A use case could be that a subtitle file has been created for all assets but need yet to be translated (a bit of a stretch but still). I wouldn't consider it a warning
There was a problem hiding this comment.
yes, but when the subtitles are parsed and he can't see subtitles on UI, this warning will help in understanding the actual issue is not with the parser, the file itself doesn't contain any subtitles.
src/SubtitlesParser/index.js
Outdated
| throw new Error('No subtitles available') | ||
| } | ||
|
|
||
| if (Math.abs(this._previousCueTimeIndex - currentTime) < 0.5) { |
There was a problem hiding this comment.
I'm not exactly sure what this is supposed to do. I guess it's an optimisation to return the 'current' subtitles when this method is called with a timestamp that is 0.5 seconds 'close' to the current subtitle. That's a dangerous path; doing so creates some subtitle logic in this layer which doesn't belong here. It should 'just' return the correct subtitles based on the time. The performance optimisation or decision when/how often to call this method belongs in the VideoPlayer component or whoever is calling this method.
src/SubtitlesParser/index.js
Outdated
| return this._previousCue | ||
| } | ||
|
|
||
| if (this._lastIndex > this._captions.length - 1 || !this._lastIndex) { |
There was a problem hiding this comment.
I find this place a bit weird for this check, since the _lastIndex isn't set in this method (before this point). I feel that this should be wherever the variable is actually updated (so in the getActiveIndex method).
src/SubtitlesParser/index.js
Outdated
| } | ||
| const activeIndex = this.getActiveIndex(currentTime) // find active cue from the captions stored | ||
| this._previousCueTimeIndex = currentTime | ||
| if (activeIndex !== -1 && activeIndex <= this._captions.length - 1) { |
There was a problem hiding this comment.
The part where you're checking if it's within the range of the captions array is something that should be done in the getActiveIndex method, I believe
src/SubtitlesParser/index.js
Outdated
| let end = null | ||
| let payload = '' | ||
| let lines = linesArray.filter(item => item !== '' && isNaN(item)) | ||
| console.log('lines:', lines) |
There was a problem hiding this comment.
Please remove the console.log
src/SubtitlesParser/index.js
Outdated
| static parseSubtitles(plainSub, parseOptions = {}) { | ||
| if (parseOptions && 'removeSubtitleTextStyles' in parseOptions) { | ||
| this._removeSubtitleTextStyles = parseOptions.removeSubtitleTextStyles | ||
| } | ||
|
|
||
| let linesArray = plainSub | ||
| .trim() | ||
| .replace('\r\n', '\n') | ||
| .split(/[\r\n]/) | ||
| .map(line => { | ||
| return line.trim() | ||
| }) | ||
| let cues = [] | ||
| let start = null | ||
| let end = null | ||
| let payload = '' | ||
| let lines = linesArray.filter(item => item !== '' && isNaN(item)) | ||
| console.log('lines:', lines) | ||
| for (let i = 0; i < lines.length; i++) { | ||
| if (lines[i].indexOf('-->') >= 0) { | ||
| let splitted = lines[i].split(/[ \t]+-->[ \t]+/) | ||
|
|
||
| start = SubtitlesParser.parseTimeStamp(splitted[0]) | ||
| end = SubtitlesParser.parseTimeStamp(splitted[1]) | ||
| } else if (lines[i] !== '') { | ||
| if (start && end) { | ||
| if (i + 1 < lines.length && lines[i + 1].indexOf('-->') >= 0) { | ||
| let subPayload = payload ? payload + ' ' + lines[i] : lines[i] | ||
| let cue = { | ||
| start, | ||
| end, | ||
| payload: subPayload | ||
| ? this._removeSubtitleTextStyles | ||
| ? subPayload.replace(/<(.*?)>/g, '') // Remove <v- >, etc tags in subtitle text |
There was a problem hiding this comment.
I would suggest to split this method up into smaller chunks. The amount of if and else ifs makes it heard to read. I would also like to see some comments for the use cases of those if/else statements.
There was a problem hiding this comment.
Still a few remarks, most noticeably this one: https://github.com/Metrological/metrological-sdk/pull/2/files?short_path=2216872
|
Hi Robbert, I have addressed your comments, please have a look at them. |
docs/plugins/videoplayer.md
Outdated
| $videoPlayerSubtitleTextChanged(text, currentTime){ | ||
| const _subtitleText = text || ''// current subtitle to render depending on video playback | ||
| // update subtitle text to your template | ||
| this.tag('Subtitles').text = _subtitleText; |
There was a problem hiding this comment.
Shouldn't this be this.tag('Subtitles').text.text = _subtitleText;?
docs/plugins/videoplayer.md
Outdated
| const _subtitleText = VideoPlayer.currentSubtitleText || '' | ||
| this.tag('Subtitles').text = _subtitleText; |
There was a problem hiding this comment.
Why are these lines needed?
tests/subtitlesParser.spec.js
Outdated
| // it('Should not call getActiveIndex when called with in 0.5s', () => { | ||
| // jest.spyOn(SubtitlesParser, 'getActiveIndex').mockImplementation(() => 1) | ||
| // SubtitlesParser.getSubtitleByTimeIndex(4) | ||
| // SubtitlesParser.getSubtitleByTimeIndex(4.4) | ||
| // expect(SubtitlesParser.getActiveIndex).toBeCalledTimes(1) | ||
| // SubtitlesParser.getSubtitleByTimeIndex(4.6) | ||
| // expect(SubtitlesParser.getActiveIndex).toBeCalledTimes(2) | ||
| // SubtitlesParser.getActiveIndex.mockRestore() | ||
| // }) |
There was a problem hiding this comment.
Please remove commented code
|
@gvamshi-metrological I had another look and still see some minor things I'd like to get resolved before approving |
|
Hi Robbert, I have addressed your comments, Please have a look |
This plugin will fetch and parse subtitles from a given URL and provides subtitles as text depending on the 'currentTime' passed
since I don't have access to add reviewers, adding you here.
cc: @robbertvancaem, @mauro1855 , @ThamimMetrological ,@raj-natarajan