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

Find best matched font in TypefaceCompatApi29Impl #212

Closed
wants to merge 1 commit into from

Conversation

RikkaW
Copy link
Contributor

@RikkaW RikkaW commented Jul 24, 2021

Proposed Changes

Use best-matched font rather than "hardcoded"

The original part

final FontStyle defaultStyle = new FontStyle(
        (style & Typeface.BOLD) != 0 ? FontStyle.FONT_WEIGHT_BOLD
                : FontStyle.FONT_WEIGHT_NORMAL,
        (style & Typeface.ITALIC) != 0 ? FontStyle.FONT_SLANT_ITALIC
                : FontStyle.FONT_SLANT_UPRIGHT
);
return new Typeface.CustomFallbackBuilder(familyBuilder.build())
       .setStyle(defaultStyle)
       .build();

This will make the final Typeface ALWAYS have either 400 (FONT_WEIGHT_NORMAL) or 700 (FONT_WEIGHT_BOLD) weight. It is incorrect since fonts passed to these methods could have other font-weight. For example, pass only one font with 500 weight, the final Typeface will have 400 weight, making the fallback font displayed as 400 weight rather than 500.

This pull request copies the platform's algorithm to find the best-match font from the font family.

Testing

Test: Observed that the font weight is correct

Issues Fixed

Fixes: Fix https://issuetracker.google.com/issues/194553426

@google-cla google-cla bot added the cla: yes label Jul 24, 2021
@dlam
Copy link
Member

dlam commented Oct 6, 2021

Thanks for submitting this and also filing a bug for tracking! Sorry it took awhile to get to this, we're not "officially" taking PRs from Github for text, but spoke with the owner and he seems okay with this change 😃

@objcode
Copy link
Collaborator

objcode commented Mar 28, 2022

Thank you so much for the change 🥳 !

It looks like it should merge. I will see this CL through the rest of the way on the gerrit side.

Sorry about the delay in reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants