Repository navigation
Fix RTL punctuation marks issues with mixed BiDi subtitles - #2553
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
icbaker
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've added some comments. I was also wondering if you'd be able to provide an example stream or subtitle file that demonstrates the problem you're fixing here? So I can run the demo app without this patch, see the problem, then run it again and see it fixed.
I would also be interested to see what happens with span styling when this code is triggered - because atm I'm worried it might all be lost.
|
Hi, thanks for the comments. This is a standard SRT file that can be used to reproduce this issue (just rename to srt if needed): Here are some examples: These lines are displayed as: should be displayed as: These lines are displayed as: Should be displayed as: You can notice that the punctuation marks are displayed to the right of the sentences and should be displayed to the left (as in all RTL languages). Hope you are good with Hebrew :-) |
|
Thank you for the test file and screenshots, that's really helpful. I took your SRT file and adapted it to WebVTT so I could add some styling (borrowed from one of our test files), and confirmed that this PR currently strips styling info. Using this file (just rename to Compare before your change (styling visible, punctuation in the wrong place): With after (styling lost, punctuation in the correct place): Please take a look and see if you can tweak this PR so it puts the punctuation in the correct place while preserving (or restoring) the span styling. |
|
I was able to reproduce the styling issues with the VTT file on my side, I think the best option here will be to manipulate the cues and then restore the original spans with the indices fixed. I am working on this. |
|
Hi, is there anything I can do to help you merge this PR ? |
|
Thanks for pinging, I was waiting for the tests and didn't realise they'd already been added (I think just 'resolving' a conversation on GitHub maybe doesn't trigger an email - so I didn't know to look back here) - I'll take another look. |
Spans are preserved when wrapping RTL subtitles
- Added support for non Spanned cues
- Added Unit Tests
|
I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks! |
f09e6a5 to
679cec6
Compare
Also add a release note and some nullness fixes








This PR fixes the punctuation marks issues with subtitles text containing a mix of RTL and LTR languages.
google/ExoPlayer#11214
jellyfin/jellyfin-androidtv#4296