jellyfin-vue
jellyfin-vue copied to clipboard
feat(subtitles): settings & custom element for external subtitles
This pull request introduces new features and improvements related to subtitle settings and displaying subtitle tracks. The main changes include:
Subtitle Settings Page:
- Added a new settings page dedicated to subtitle customization.
- Users can now select their prefered subtitle font from a list of available fonts.
- Font size and position from the bottom of the screen can be adjusted.
- Options to enable or disable subtitle backdrop and stroke.
Custom Subtitle Track Element:
- Integrated a custom track element to handle external subtitle files, supporting both VTT and ASS/SSA formats.
- Added logic to parse and sanitize subtitle files, ensuring harmful tags are removed and specific tags are replaced with styled
<span>tags for safe rendering
Cloudflare Pages deployment
| Latest commit | aa797ae |
|---|---|
| Status | ✅ Deployed! |
| Preview URL | https://f941f7f1.jf-vue.pages.dev |
| Type | 🔀 Preview |
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 passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
No data about Duplication
I think it's good to let the user choose a different font. If not purely because of personal preferences, then for better readability. Our current UI font (Figtree) may not be the easiest to read quickly (smaller x-height, wider and geometric-like letterforms, etc.), therefore allowing a font change for subtitles is a good idea. The subtitle font servers one purpose (aside from fully supporting the language the subs are in) => helping the watcher read & understand the subs really quickly so that they can look at the video for as long as possible.
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):
- The displayed fonts should be the system's ones. You can get the current installed fonts using this API: https://developer.mozilla.org/en-US/docs/Web/API/Window/queryLocalFonts
- By using the system fonts, we're sure the text can be displayed and we don't need to increase the bundle with a lot of fonts.
- That API is currently just supported by Chromium browsers. It entered draft at W3C just 2 weeks ago (7 Jun 2024), so I expect it will take a while to reach the other browsers still, but it should come at some point.
- Given this is a niche feature and Chromium is the most widely used browser, it's not critical to think of a workaround and this could be left as is. If the API is not supported the selector must be disabled.
- That API requires permission. Use https://vueuse.org/core/usePermission/#usepermission to reactively track if permission is given and disable/enable the selector with that.
- Use https://vueuse.org/core/useSupported/#usage to reactively check if the API is supported
- Given the API it's not widely supported, we need to display informational messages:
Custom fonts are currently not supported by your device
And the following buttons/links:
Learn more: When not supported, linking to the CanIUse page Allow permission: When the API is supported but permission is not yet granted.
- The font selector must be in its own component, so it could be reused for changing Books typography or even the whole UI.
- The font selector should display each font in the specific font:
- I'd say that the VSelect structure should be something like this:
<VListItem>*Current font* (Default)</VListItem>
<VDivider />
...Rest of fonts (also wrapped in VListItem)
- Current Font must be replaced by the current's font name of course. It would be great though if, instead of hardcoding it, we could query the fon'ts name from CSS and computedStyle or using Font API. That way, if we change the font later, somehow CSS stylesheet is overriden by the user (because he modified the html file, for example) or allow for font customization in appearance settings for the whole client, the real selected font is displayed.
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.
The history of this PR got mangled somehow, I'll try tomorrow to clean it up, was almost halfway through it in my last review. Updates to the branch must be made using rebases, not merge commits (but this is a FYI for now, don't do anything to fix this, I'll take care of it).
@seanmcbroom I've rebased your branch and properly fixed all the conflicts.
The way to update your branch locally is to add the main repo as a remote:
git remote add upstream https://github.com/jellyfin/jellyfin-vue
Fetch changes:
git fetch --all
And rebase (the -i is optional):
git rebase -i upstream/master
With the -i, you can squash, reword and edit commits (that's what I did to fix up the names that were wrong and didn't pass commit linting). GitLens extension (recommended by the workspace) can really help as well in those tasks by showing better how the git-rebase-todo file will perform the operations. There are plenty of tutorials online about the different rebase operations if you want to know more, but you can also ask here for an specific one.
With rebases, conflicts are fixed in a commit by commit basis, that's why there are no merge commits involved and the history of changes is fully linear.
To fix the divergence between your local branch and the remote one, the easiest thing is that, located in the master branch (the one of this PR) you switch to a new branch:
git checkout -b blah
Delete the old master:
git branch -D master
Fetch changes (this is done automatically by git and VSCode, but just in case):
git fetch --all
And change to the branch and delete the temporal one:
git checkout master && git branch -D blah
Hopefully these instructions are enough to get you back on track again.
Thanks for the advice, honestly all this git stuff is kind of confusing to me and I'm still not exactly sure what happened. I think I've fixed my branch locally though. I also have a few little fixes to the player element store that I'll push shortly. Thank you again as always.
Also, I'm wondering if we should sync these settings? The typography available in one device might not be available in others...
Don't worry about the merge conflict btw
Also, I'm wondering if we should sync these settings? The typography available in one device might not be available in others...
Don't worry about the merge conflict btw
Yeah these settings should probably be client-independent, since there are different default font faces on most operating systems.
@seanmcbroom I forgot that back in the day I added support in the synced store for syncing specific keys only. I synced all of them but the font family, does that sound good to you? As soon as you let me know about this, confirm that everything is alright with the new subtitle finding algorithm and answer this question we're shipping this! 🚀
Sorry, I wrote the review changes yesterday but didn't realize they didn't submit until now. Anyways, those should be the final changes. Once those minor bugs are worked out I think everything should be ready!
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
@seanmcbroom Okey, let's ship this! Thank you very much for all your knowledge about subtitles and patience while getting this in! ❤️❤️🚀🚀🚀