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

Fix broken subtitles line wrapping #1346

Merged
merged 3 commits into from
Jan 16, 2022

Conversation

siankatabg
Copy link
Contributor

@siankatabg siankatabg commented Jan 13, 2022

Changes

Apparently I did a mistake using the ReplacementSpan for outlining the text as it doesn't support line wrapping, so lets remove it and do it with a custom TextView :)

Issues

#1303 broke subtitle line wrapping

Apparently I did a mistake using the ReplacementSpan for outlining the text as it doesn't support line wrapping, so lets do it with a custom TextView :)
@mueslimak3r
Copy link
Member

mueslimak3r commented Jan 14, 2022

I haven't dug into the details of your implementation so I won't comment on that yet.

But conceptually I'm a fan of having a custom textview for outlined text. This could be used to add outlines to other UI elements that need it, like the title/clock row in the playback overlay. Currently that can be difficult to read depending on what's on screen in a video.

@siankatabg
Copy link
Contributor Author

That's exactly what I thought while doing it. Also easy to add extras as outlining colour and so on if needed at some point.

@nielsvanvelzen nielsvanvelzen changed the title Fix broken subtitles line wrapping after my last PR Fix broken subtitles line wrapping Jan 14, 2022
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

I've not tested this or the other PR but my preference goes to a custom view like this one does. I've done a quick initial review on the code.

@siankatabg
Copy link
Contributor Author

siankatabg commented Jan 16, 2022

From linter warning:

This TypedArray should be recycled after use with #recycle()

Sorry, I did it on a hurry yesterday and didn't even checked the warnings (the reason I didn't marked anything as resolved), I just came to review what I did and is left to be done, but you was first.
Anyway, your patience and the code quality you're trying to keep in this app is amazing, thank you!

@mueslimak3r
Copy link
Member

mueslimak3r commented Jan 16, 2022

I tested it with some videos/subtitles that clipped before and it works well!
I didn't spot any errors or find any new leaks (I did discover a leak but I checked that it was there before these recent subtitles-related changes).

@nielsvanvelzen nielsvanvelzen merged commit 72ae1d3 into jellyfin:master Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants