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

Preserve Japanese language features when not applying embedded styling #8943

Merged
merged 2 commits into from
May 26, 2021

Conversation

dlafayet
Copy link
Contributor

@dlafayet dlafayet commented May 14, 2021

Fix bug where rubies and boutens are missing, tate-chu-yoko is rendered incorrectly when SubtitleView.setApplyEmbeddedStyles(false). This method should only affect styling elements and not remove any language features.

@icbaker

Bug is observed when using the WebViewSubtitleOutput and player sets SubtitleView.setApplyEmbeddedStyles(false).

Fix bug where rubies and boutens are missing, tate-chu-yoko is rendered
incorrectly when SubtitleView.setApplyEmbeddedStyles(false). This method
should only affect styling elements and not remove any language features.
@@ -48,5 +54,27 @@ public static float resolveTextSize(
}
}

public static void preserveJapaneseLanguageFeatures(Spannable copy, Spanned original) {
Copy link
Collaborator

@icbaker icbaker May 17, 2021

Choose a reason for hiding this comment

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

Instead of needing to encode this list of Span types in SubtitleViewUtils, what do you think about using a marker interface like LanguageFeature that would be implemented by spans like RubySpan?

(See Effective Java 3rd edition Item 41: Use marker interfaces to define types)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Will update the PR

@@ -386,7 +386,12 @@ private Cue removeEmbeddedStyling(Cue cue) {
cue.buildUpon().setTextSize(Cue.DIMEN_UNSET, Cue.TYPE_UNSET).clearWindowColor();
if (cueText != null) {
// Remove all spans, regardless of type.
strippedCue.setText(cueText.toString());
strippedCue.setText(new SpannableString(cueText.toString()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're no longer stripping all Spans, I think it's probably easier just to directly copy the Spanned to a SpannableString (including all the spans) by passing it into the constructor, and then iterate over them calling removeSpan() unless the type is recognised as a 'language feature'.

I suspect it will also reduce the number of instanceof and cast calls needed (especially if you store it in a SpannableString local before calling Cue#setText later).

It seems less complicated than being sure we definitely copied all the flags etc over.

@icbaker
Copy link
Collaborator

icbaker commented May 19, 2021

Thanks! I'll work on getting this merged.

@ojw28 ojw28 merged commit afe4217 into google:dev-v2 May 26, 2021
ojw28 added a commit that referenced this pull request Jun 10, 2021
@google google locked and limited conversation to collaborators Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants