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

Support subtitle font selection #498

Merged
merged 1 commit into from
Jul 29, 2022
Merged

Conversation

PangMo5
Copy link
Member

@PangMo5 PangMo5 commented Jul 23, 2022

  • Add subtitle font selection in Settings (Default value: .systemFont)

@PangMo5 PangMo5 requested a review from LePips July 23, 2022 18:37
@PangMo5 PangMo5 self-assigned this Jul 23, 2022
@PangMo5 PangMo5 added the enhancement New feature or request label Jul 23, 2022
@PangMo5 PangMo5 changed the title Support select subtitle font Support subtitle font selection Jul 23, 2022
Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

It's quite fun to play around with the fonts! However, I get a crash from opengl every other time playback is started. Probably from the Selector(("setTextRendererFont:")) as this is currently a workaround for VLCKit. I'm not comfortable shipping this because of these crashes.

I would also like to find out if this directly solves the issue. The author of the issue isn't necessarily correct that it's a "missing font" issue, it just looks like the font doesn't support that character. I know that some other fonts might have these missing characters but we would need to test.

Swiftfin/Views/Representable/FontPicker.swift Outdated Show resolved Hide resolved
Shared/Coordinators/FontPickerCoordinator.swift Outdated Show resolved Hide resolved
Shared/Coordinators/SettingsCoordinator.swift Outdated Show resolved Hide resolved
@PangMo5
Copy link
Member Author

PangMo5 commented Jul 25, 2022

I didn't experience the related crash.
Which font did you use to load the word?
Anyway, I think it can definitely lead to problems. Did you see the error on a device other than the simulator? If you saw it on a real device, we could move it to the experimental settings.

@LePips
Copy link
Member

LePips commented Jul 25, 2022

Sorry, I haven't been able to test on a physical device because I've been out of town so I'll be able to do that later. I do have that exact episode so I might be able to load up the exact word. The crashing occurred on any font.

Swiftfin/Views/SettingsView/SettingsView.swift Outdated Show resolved Hide resolved
Swiftfin/Views/FontPicker.swift Outdated Show resolved Hide resolved
@LePips
Copy link
Member

LePips commented Jul 29, 2022

Okay, testing on a real device hasn't crashed on changing fonts. All good for normal use.

@LePips
Copy link
Member

LePips commented Jul 29, 2022

Sadly, I don't think this necessarily fixes the original issue. I've tested that same media on different fonts and still get the box character or another character in that space. Due to that, I think this is a VLCKit issue so we can close the issue. Nonetheless, this is still a really cool feature.

@PangMo5
Copy link
Member Author

PangMo5 commented Jul 29, 2022

Sadly, I don't think this necessarily fixes the original issue. I've tested that same media on different fonts and still get the box character or another character in that space. Due to that, I think this is a VLCKit issue so we can close the issue. Nonetheless, this is still a really cool feature.

I see :d

@LePips LePips merged commit 48a03d8 into jellyfin:main Jul 29, 2022
@PangMo5 PangMo5 deleted the PangMo5/font branch July 29, 2022 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants