feat: add support for converting Netflix-style ruby text to HTML#829
feat: add support for converting Netflix-style ruby text to HTML#829killergerbah merged 8 commits intokillergerbah:mainfrom
Conversation
|
Currently unable to push a fix due to github git operations being down... Will push when back up. |
There was a problem hiding this comment.
Thanks for doing this, this is super useful and a lot of people have requested this feature.
My main comment is that I think the setting and how it maps to behavior can be simplified:
- Netflix-only because no other streaming service supplies word readings this way. See
PageSettings. - If you turn it on, Netflix should render readings as ruby. If you turn it off, then it should do nothing.
- I think the name of the setting could be clearer. Maybe something like "detect and display ruby." "Convert ruby text" sounds like you already have the ruby, and you need to convert it.
| import { WebVTT } from 'vtt.js'; | ||
| import { XMLParser } from 'fast-xml-parser'; | ||
| import { SubtitleHtml, SubtitleTextImage } from '@project/common'; | ||
| import { parseRubyText } from '../util'; |
There was a problem hiding this comment.
Looks like this file didn't need to be changed
| private _buildTextHtml(text: string, track?: number) { | ||
| let processedText = parseRubyText(text, this.convertRubyText); | ||
|
|
||
| if (this.subtitleHtml === SubtitleHtml.remove) { |
There was a problem hiding this comment.
I'd like to avoid having to mix settings like this to achieve very specific behaviors. We already have a regex filter that would do the same thing as convert ruby text + remove html.
Instead, I think convert ruby text should just be a Netflix-specific setting that displays the ruby when enabled and does nothing when disabled. Let me know what you think.
|
Thanks for the feedback.
That is true. Netflix is the only platform that actually provides readings in this format. The reason I didn’t want to scope the setting strictly under "Netflix" is that a lot of people (myself included) download subtitles from sites like jimaku that are in Netflix format, then watch them elsewhere (Plex, Crunchyroll, local files, etc.). In that workflow the playback source isn’t Netflix, but the subtitles still contain Netflix-style ruby. Because of that, I’m a bit worried that putting this under Netflix-specific settings would make it harder to discover for those cases.
Yup, totally agree.
Somehow I completely missed there was already a regex filter. That is a much cleaner solution. |
|
Ah yeah you're right. I agree it shouldn't be a Netflix specific setting then. Can just keep it at the top level. |
…emove html+ruby specific behavior
|
Just pushed the following changes:
|
|
I've made those changes The only issue is there's some unexpected behavior when you change the HTML remove/render settings with subtitles already loaded. The correct behavior occurs when you reload the subtitle though. I guess this is because once subtitle-reader.ts processes the subtitle, it doesn't reprocess when the user changes the Render HTML or Display Ruby setting. Examples: [
[
Obviously what I did with the useMemo is a bit hacky, but it did fix this weird toggle behavior. I'm happy to do whatever you think is best though. |
killergerbah
left a comment
There was a problem hiding this comment.
For what it's worth I don't think the useMemo in App.tsx is a hack. The extension side might still need some help too but it's an existing bug.
This adds optional parsing of netflix style ruby text: https://partnerhelp.netflixstudios.com/hc/en-us/articles/215767517-Japanese-Timed-Text-Style-Guide
I encounter this a lot, I'm sure others do too. But the netflix style ruby text can look really messy when we don't parse it. Here's a visual example of the new behavior:
Convert Ruby: OFF (original behavior)
Convert Ruby: ON + Render HTML: Render
Convert Ruby: ON + Render HTML: Remove (This effectively removes the ruby)