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

Fixed wordwrapping of subtitles. #422

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JontomXire
Copy link

I made this change some time ago but I don't think I ever submitted it. I have been using it for years and it works fine except where subtitles are so long that when divided across the maximum number of rows they still go off the edge of the screen. But are still very readable.

@torarin
Copy link

torarin commented Feb 1, 2016

As discussed previously (#88), I don't understand why you are changing the preparation code. I don't think the preparation function should change the formatting of a subtitle, merely prepare it for rendering. It would make more sense for wrapping to be done some time prior to calling prepare, so that prepare is given already wrapped text lines for rendering.

@JontomXire
Copy link
Author

I am very far from an expert on the OMXPlayer code. As far as I remember I put it here for efficiency. This function is already looping through the lines and calculating line widths from character widths, so it made sense (to me) to do the word wrap here.

Frankly I am not that bothered if you don't take this commit. Since I first started using OMXPlayer I have learnt a lot more about Git and am now quite happy to maintain my own fork including any updates that you are unwilling to take. However this change would benefit others, so I offer it here.

If you can find a better way to implement the same change, then feel free to do so.

@Ruffio
Copy link

Ruffio commented May 4, 2016

@popcornmix how about this one?

@Ruffio
Copy link

Ruffio commented Jan 7, 2017

@popcornmix Can this be merged?

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