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.

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

Refactor WebVTT building logic into WebVTT::Builder #4070

Merged
merged 7 commits into from
Oct 7, 2023

Conversation

syeopite
Copy link
Member

@syeopite syeopite commented Aug 24, 2023

#4001 (comment)

Syntax is similar to JSON.build.

string = WebVTT.build do |vtt|
  vtt.cue(Time::Span.new(seconds: 1), Time::Span.new(seconds: 2), "Line 1")
  vtt.cue(Time::Span.new(seconds: 2), Time::Span.new(seconds: 3), "Line 2")
end

string # => "WEBVTT\n\n00:00:01.000 --> 00:00:02.000\nLine 1\n\n00:00:02.000 --> 00:00:03.000\nLine 2\n\n"

@syeopite syeopite requested a review from a team as a code owner August 24, 2023 22:50
@syeopite syeopite requested review from unixfox and removed request for a team August 24, 2023 22:50
# ```
#
# Accepts an optional settings fields hash to add settings attribute to the resulting vtt file.
def self.build(setting_fields : Hash(String, String)? = nil, &)
Copy link
Member Author

Choose a reason for hiding this comment

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

The setting_fields here is mostly for backwards compatibility. I'm pretty sure — or at least I couldn't find any reference to it — that the fields Invidious adds aren't actually apart of WebVTT

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks completely non-standard. I couldn't find anything about them on MDN

Copy link
Member

@SamantazFox SamantazFox left a comment

Choose a reason for hiding this comment

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

You even wrote a spec for it <3

src/invidious/helpers/webvtt.cr Outdated Show resolved Hide resolved
src/invidious/helpers/webvtt.cr Outdated Show resolved Hide resolved
@SamantazFox SamantazFox added the need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something label Aug 28, 2023
@SamantazFox
Copy link
Member

Can you please rebase your changes?

@syeopite
Copy link
Member Author

Rebased

@SamantazFox SamantazFox added ready and removed need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Sep 27, 2023
@SamantazFox
Copy link
Member

It works like a charm :D

@SamantazFox SamantazFox merged commit 1caaf63 into iv-org:master Oct 7, 2023
7 checks passed
@SamantazFox
Copy link
Member

Thanks for your PR :D

@syeopite syeopite deleted the vtt-builder branch October 17, 2023 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants