jellyfin-vue icon indicating copy to clipboard operation
jellyfin-vue copied to clipboard

feat(subtitles): settings & custom element for external subtitles

Open seanmcbroom opened this issue 1 year ago • 6 comments

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

seanmcbroom avatar Jun 05 '24 01:06 seanmcbroom

Cloudflare Pages deployment

Latest commit aa797ae
Status ✅ Deployed!
Preview URL https://f941f7f1.jf-vue.pages.dev
Type 🔀 Preview

View build logs

jellyfin-bot avatar Jun 05 '24 01:06 jellyfin-bot

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. 👍

seanmcbroom avatar Jun 09 '24 02:06 seanmcbroom

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Jun 09 '24 18:06 sonarqubecloud[bot]

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.

erikbucik avatar Jun 19 '24 20:06 erikbucik

@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:

image

  • 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?

ferferga avatar Jun 19 '24 23:06 ferferga

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.

seanmcbroom avatar Jun 21 '24 03:06 seanmcbroom

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).

ferferga avatar Aug 13 '24 22:08 ferferga

@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.

ferferga avatar Aug 14 '24 19:08 ferferga

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.

seanmcbroom avatar Aug 14 '24 22:08 seanmcbroom

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

ferferga avatar Sep 08 '24 21:09 ferferga

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 avatar Sep 09 '24 08:09 seanmcbroom

@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! 🚀

ferferga avatar Sep 10 '24 06:09 ferferga

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!

seanmcbroom avatar Sep 10 '24 17:09 seanmcbroom

@seanmcbroom Okey, let's ship this! Thank you very much for all your knowledge about subtitles and patience while getting this in! ❤️❤️🚀🚀🚀

ferferga avatar Sep 12 '24 01:09 ferferga