-
-
Notifications
You must be signed in to change notification settings - Fork 228
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
feat(subtitles): settings & custom element for external subtitles #2360
Conversation
Cloudflare Pages deployment
|
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.
Thank you very much for your conntribution ❤️❤️❤️!
Beforehand, please bear with me because, as I point out in one of my comments, I'm not fully knowledgeable of all the subtitle flow we need to handle in Jellyfin and I might be naive in some aspects.
Regardless of the refactoring bits, I'd like to have some input from @erikbucik as well. Although current web client provides with typography customization options, I don't think this is a good idea. While in the surface it might seem better to provide the same options as the current web client and a huge amount of customization, I don't think being feature complete === repeat the same mistakes/UX quirks of the current web client. This is my reasoning:
- Users expect consistency across an application. This means that, by default, subtitles should use the same typography that we use everywhere, right? Are we on the same page on this?
- Introducing these kind of customization options might appeal at first, because we're catering for more user tastes. But, regardless what we do, in this case it will be impossible to cater to all tastes and, at the end of the day, we're offering a closed typography selection. If I'd want another typography, I'd need to rebuild the client with the typography of my liking, figure out all this logic, add it to all this logic and profit! At this point, we're making more difficult changing it, which is something that I could also do right now (add font, add CSS, build and profit!). A counter-example of this would be the playback speed selection): it's integrated in a way that it's flexible enough to cater all the endless likings of an user without compromises, it's not complex and there are good defaults for those who don't want the extra customization (the selection menu on click).
Hopefully I explained myself right, since I'm in the middle of some exams I'm reviewing this in the quick way, sorry!
Thank you very much again for your contribution!
First of all, thank you for taking the time to review the pull request. I know you've been busy and I wish you good luck on your exams! I've already responded to a few of the discussions. I couldn't get to all of them today but I'll try to get to them done within the next few days. 👍 |
Quality Gate passedIssues Measures |
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.
A few extra comments, now I think I fully understand the scope of this much better, thank you!
Answered the non resolved conversations of my previous comments and I'm also adding some small things that I noticed in this second pass.
Re-tagging @erikbucik for his input regarding my first review comment and whether we should give typography choices or not.
Also, FYI, I might merge these days a few PRs of mine: they're part of some work I've been doing for my uni thesis (the reason why I'm so busy lately, since it needs to be delivered next week :P) and I had them locally for a while, and for my defense it's good to have them upstreamed already. Don't take it as I'm neglecting this PR please, definitely good stuff here!
Also, if you can, merge the latest changes locally and run npm ci
so you have the linter working again.
frontend/src/utils/subtitles.ts
Outdated
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.
Not sure how much does this takes for an average movie, but perhaps, in order to avoid blocking the main loop, we can move this into a WebWorker? In case you want to take a look at how it's done, check BlurhashCanvas.
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.
It's already run asynchronously, so it shouldn't be blocking the main loop anyway. What benefits does WebWorker provide besides being run asynchronously?
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.
@seanmcbroom Being async doesn't mean it doesn't block the main loop. async/await is just syntatic sugar over Promises. Promises themselves don't block the main thread while they're being resolved, the event loop can defer them in the background... But if you fire wrap in a promise a while loop that resolves at the 1 millionth iteration, you'll see how, as soon as the event loop starts processing the promise, the whole thread will be blocked because the loop itself is blocking.
asynchronous code still runs in the main thread, although in a deferred way.
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.
I see, sorry about my lack of knowledge regarding how javascript is run on a hardware level. I'll look into it and update you.
I think it's good to let the user choose a different font. If not purely because of personal preferences, then for better readability. The other customization options (position, stroke, shadow, etc.) are also nice to have. |
@seanmcbroom After further chat with Erik, we came up with the following conclusions as well (though these are more technical related):
Custom fonts are currently not supported by your device And the following buttons/links: Learn more: When not supported, linking to the CanIUse page
What do you think of these points? |
I agree with all your points and proposed changes to the font selection. I wasn't even aware of the queryLocalFonts method and the timing for it being passed into chromium is almost perfect, so I guess it was meant to be haha. |
bc4e4a7
to
0233c77
Compare
Signed-off-by: Fernando Fernández <[email protected]>
* Reduce unnecessary verbosity of the stores * Extract the current typography of the application as a CSS variable * Make font selector truly generic and also able to change the typography of the whole app There are now 3 internal values: - auto: for following app's font - system: for following system's font - default: default app's font This way subtitles can be truly configurable independently from the app. App typography follows the same schema, but without 'auto' since it's not applicable there. Signed-off-by: Fernando Fernández <[email protected]>
Signed-off-by: Fernando Fernández <[email protected]>
Signed-off-by: Fernando Fernández <[email protected]>
Signed-off-by: Fernando Fernández <[email protected]>
Signed-off-by: Fernando Fernández <[email protected]>
Signed-off-by: Fernando Fernández <[email protected]>
Quality Gate passedIssues Measures |
@seanmcbroom Okey, let's ship this! |
This pull request introduces new features and improvements related to subtitle settings and displaying subtitle tracks. The main changes include:
Subtitle Settings Page:
Custom Subtitle Track Element:
<span>
tags for safe rendering