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

Add subtitles font size and background configurations #1303

Merged
merged 2 commits into from
Dec 28, 2021

Conversation

siankatabg
Copy link
Contributor

@siankatabg siankatabg commented Dec 21, 2021

Changes

Added seekbar for choosing subtitles font size (18-38 range, keeping 28 by default)
Added text outlining for better legibility and an option to disable the subtitles black background.

Few screenshots and debug apk for who wants to test

Issues

Fixes: #1055
Fixes: #437

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@thornbill
Copy link
Member

A shadow is not the same as an outline for text and imho is an insufficient solution to maintain subtitle legibility.

@siankatabg
Copy link
Contributor Author

All recommendations have been implemented

@siankatabg siankatabg force-pushed the subtitle_options branch 2 times, most recently from b0679f2 to 7412e65 Compare December 22, 2021 11:55
@siankatabg
Copy link
Contributor Author

Sorry. I forgot to include a link to the source of the class that I've used for outlining the text. :)

@siankatabg
Copy link
Contributor Author

siankatabg commented Dec 22, 2021

Removed all abbreviations.

@siankatabg siankatabg force-pushed the subtitle_options branch 2 times, most recently from 0770edd to 8163f9e Compare December 22, 2021 14:43
@siankatabg siankatabg force-pushed the subtitle_options branch 4 times, most recently from 9ab8885 to 3ce8f8c Compare December 23, 2021 16:57
@siankatabg
Copy link
Contributor Author

siankatabg commented Dec 24, 2021

Ok guys, my last try to do this the way you want... I wrote the outlining class from scratch and I made it as simple as possible. I also used seekbar for the subtitles size as I didn't found a way of doing it as a list with int preference and I don't see the need of using ARGB instead of a simple boolean to enable/disable the background.

Anyway, coding random fixes and mods for the things I'm using is just a hobby to me(I'm self taught) and I just wanted to help with something that many people request from a long time (and looked simple enough to do), but it seems that my skills are not enough to make you guys happy, so fell free to help me out, or just close the request.

Happy holidays to all!

@nielsvanvelzen
Copy link
Member

I also used seekbar for the subtitles size as I didn't found a way of doing it as a list with int preference

I don't think we support lists with int right now so this solution is fine for now

and I don't see why I should use ARGB instead of a simple boolean to enable/disable the background.

It was a suggestion, it's not required or anything

Anyway, coding random fixes and mods for the things I'm using is just a hobby to me(I'm self taught) and I just wanted to help with something that many people request from a long time (and looked simple enough to do), but it seems that my skills are not enough to make you guys happy, so fell free to help me out, or just close the request.

We definitely appreciate the pull request, replying "out of my knowledge" or "not interested" to a suggestion is valid.

Happy holidays to all!

Same to you! I'll try to review this PR again when I find some time, I think it's mostly fine now :)

@siankatabg siankatabg changed the title Add subtitles font size and background switch Add subtitles font size and background configurations Dec 24, 2021
Added seekbar for choosing subtitles font size (18-38 range, keeping 28 by default)
Added text outlining for better legibility and an option to disable the subtitles black background.
mueslimak3r
mueslimak3r previously approved these changes Dec 26, 2021
@mueslimak3r mueslimak3r self-requested a review December 26, 2021 05:42
@thornbill thornbill dismissed mueslimak3r’s stale review December 26, 2021 06:07

Approval was submitted accidentally.

@nielsvanvelzen nielsvanvelzen added the release-highlight Pull request might be suitable for mentioning in the release blog post label Dec 26, 2021
@ManuLinares
Copy link

ManuLinares commented Dec 27, 2021

Could a subtitle ¿height? (bottom distance) be added as a slider also?

P.S.: The outline looks amazing, thanks for all of your work.

@siankatabg
Copy link
Contributor Author

siankatabg commented Dec 27, 2021

Could a subtitle ¿height? (bottom distance) be added as a slider also?

P.S.: The outline looks amazing, thanks for all of your work.

I also think the subtitles bottom padding(the empty space underneath them) is too big, but I don't think we need a slider for it, maybe finding a better value would do the job (now on most of the movies when 2 lines of subtitles are shown, the first one is mostly over the video, the other one under it on the black lane).

I was thinking of doing another PR about it(so we can comment it/more ideas/opinions and so on) as I've tested with different values and find the value of 20(currently 48) to be the best (imo) on most of the media I've and even when 2 lines of subtitles are shown they both are under the video in the black lane and also looks much better when the video is full screen (with no black lanes on top/bottom).

But we could discuss it here also, as it's a really small (codewise) change!?

@ManuLinares
Copy link

Could a subtitle ¿height? (bottom distance) be added as a slider also?
P.S.: The outline looks amazing, thanks for all of your work.

I also think the subtitles bottom padding(the empty space underneath them) is too big, but I don't think we need a slider for it, maybe finding a better value would do the job (now on most of the movies when 2 lines of subtitles are shown, the first one is mostly over the video, the other one under it on the black lane).

I was thinking of doing another PR about it(so we can comment it/more ideas/opinions and so on) as I've tested with different values and find the value of 20(currently 48) to be the best (imo) on most of the media I've and even when 2 lines of subtitles are shown they both are under the video in the black lane and also looks much better when the video is full screen (with no black lanes on top/bottom).

But we could discuss it here also, as it's a really small (codewise) change!?

I'm using a value of 20 bottom padding in my build. It works best and I tried with several aspect ratios.

@siankatabg
Copy link
Contributor Author

siankatabg commented Dec 27, 2021

I'm using a value of 20 bottom padding in my build. It works best and I tried with several aspect ratios.

I'm also experimenting with slight to even zero padding, but the first line to be on the same place all the time (ie when a second line should be shown it is underneath the first one which is on a "constant" place(when not more than 2 lines are shown, but that should be a really rare thing for subs usage))

I know that's not a standard solution and most probably won't be merged here, but I guess my custom builds would be using it.

Anyway, I probably should really do a new PR and we should move that convo there as that's a thing out of this PR topic right now :)

@nielsvanvelzen nielsvanvelzen enabled auto-merge (squash) December 28, 2021 18:03
@nielsvanvelzen nielsvanvelzen merged commit eadf7b9 into jellyfin:master Dec 28, 2021
@mueslimak3r
Copy link
Member

Hey @siankatabg it looks like this PR broke subtitle line wrapping. This is especially noticeable on large sizes.
Can you submit a patch for this?
20220112_014121
20220112_014143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-highlight Pull request might be suitable for mentioning in the release blog post
Projects
None yet
5 participants