-
Notifications
You must be signed in to change notification settings - Fork 6
chapters/thumbnails support added to subtitle feature #17
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.
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
base: master
Are you sure you want to change the base?
Conversation
@jojoob can you please merge all these three commits into one. Thanks for working on this, it looks fine. |
Please resubmit pull request to JW7 branch, master branch will be deleted as per #37. |
'label' => $label); | ||
if ($label == 'chapters' || $label == 'thumbnails') { | ||
$tracks[] = array( | ||
'file' => $subtitlefileurl->out(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better as "->out(false)" to prevent the URLs being escaped which causes issues if the URL contains a query string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the URL will be embedded in the Javascript code we need the unescaped version. Otherwise jwplayer could not parse the html escaped URL.
You dropped the subtitle feature by adding the if statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my point.
By default the out method for a moodle_url has escaped set to true, so this needs to specify false to ensure we're using the unescaped version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which if statement are you referring to?
The only one there appears to check $options['subtitles'] is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misunderstood you. Maybe I should not work just from my smartphone. You are right and I will change this as soon I'm back in office.
Just a thought from a usability point of view, would it be better if these were implemented as separate thumbnails and chapters options, rather than making them part of the subtitle one? e.g. $options['thumbnails'] rather than $options['subtitles']['thumbnails']? While these are part of JWPlayer's captioning support they're not subtitles and it wouldn't take much alteration to this code to separate these out as far as I can see. |
Added support for chapters and thumbnail tracks within the subtitle feature.
jwplayer chapters and thumbnails feature:
I will rebase the commits if everything else is ready to merge.