Resolves style issues in CustomTypefaceSpan

[c-b-h]

When two or more CustomTypefaceSpan are applied on a same character
sequence, previous TextPaint's style was not respected.

Example:

Strong Emphasis strong emphasis and emphasis combined

Before the inner text will only have the font applied without respecting
that the outer text is strong.

Related bug report on Google:

[noties]

Hello @c-b-h ,

thank you for bringing this up 🙌

There is one thing I think we can improve. We can skip the final Typeface styledTypeface = Typeface.create(typeface, want); operation if old typeface is null or oldStyle is NORMAL. In that case paint.setTypeface(typeface); can be used as before

[noties]

Also, I want to mention a case for discussion: when bundling a custom typeface. In most cases we are given multiple typeface files, for example:

• MyFont-bold.ttf
• MyFont-italic.ttf
• MyFont-boldItalic.ttf

So I'm not sure what will happen when using them with CustomTypefaceSpan. It seems that display would be broken as Typeface.create won't be able to locate proper file with typeface definition

[c-b-h]

Hello @c-b-h ,

thank you for bringing this up 🙌

There is one thing I think we can improve. We can skip the final Typeface styledTypeface = Typeface.create(typeface, want); operation if old typeface is null or oldStyle is NORMAL. In that case paint.setTypeface(typeface); can be used as before

The reason I didn't skip if same or null is because how create looks internally:

public static Typeface create(Typeface family, @Style int style) {
    if ((style & ~STYLE_MASK) != 0) {
        style = NORMAL;
    }
    if (family == null) {
        family = sDefaultTypeface;
    }
    // Return early if we're asked for the same face/style
    if (family.mStyle == style) {
        return family;
    }

family here is the NonNull passed into CustomTypefaceSpan and as you can see, if family.mStyle matches old style, the same instance is returned. Generally I prefer code readability to over-engineering (and redundant checking) so if you don't mind, let's keep it as is, if not, I change it to the style (no pun intended 😄) you prefer.

[c-b-h]

Also, I want to mention a case for discussion: when bundling a custom typeface. In most cases we are given multiple typeface files, for example:

• MyFont-bold.ttf
• MyFont-italic.ttf
• MyFont-boldItalic.ttf

So I'm not sure what will happen when using them with CustomTypefaceSpan. It seems that display would be broken as Typeface.create won't be able to locate proper file with typeface definition

First of all, there is no Node for emphasis within strong or strong within emphisis only StrongEmphasis and Emphasis distinctly.
My change only affects the situation where a family is defined such as:

<font-family xmlns:android="http://schemas.android.com/apk/res/android">
    <!-- regular -->
    <font
        android:font="@font/dr_publik_regular"
        android:fontStyle="normal"
        android:fontWeight="400" />

    <!-- italic -->
    <font
        android:font="@font/dr_publik_italic"
        android:fontStyle="italic"
        android:fontWeight="400" />

    <!-- bold -->
    <font
        android:font="@font/dr_publik_bold"
        android:fontStyle="normal"
        android:fontWeight="700" />

    <!-- bold italic -->
    <font
        android:font="@font/dr_publik_bold_italic"
        android:fontStyle="italic"
        android:fontWeight="700" />
</font-family>

If the Typeface is created out of this resource, then the Typeface will effectively be a font family with style association. Explicitly setting the family would always result in REGULAR while it now respects the Paint's current style and merges the new style with the old. Conversely if you would explicitly set any of the individual child fonts, the child font will always be interpreted as is, no matter style as there is no style associated to the child, only the child within a family. Does my reasoning make sense?

[noties]

The reason I didn't skip if same or null is because how create looks internally:

Unfortunately, internal code cannot be viewed as the contract - different API levels can have different implementations. Moreover, different devices can have different implementations of internal methods (vendor specific code). So, let's be on a safer side, especially when it is in our control and the change required is small.

Thanks for clearing this up for XML-defined fonts. I was referring to fonts bundled with the app in the assets folder and created via code. In this case typeface information won't be preserved, as Typeface.create has no idea of other styles of the font available. So, the name CustomTypefaceSpan would even be wrong. For example, you create the span with:

CustomTypefaceSpan(MyFont.italic())

and then have a system regular font used instead because some text appeared inside other typeface. I see the value in having styles merged, but it seems that it must be done by a new span that makes it explicit and obvious that a font must be creatable by the Typeface.create method. Does CustomTypefaceStyleSpan sound good?

[c-b-h]

Unfortunately, internal code cannot be viewed as the contract - different API levels can have different implementations. Moreover, different devices can have different implementations of internal methods (vendor specific code). So, let's be on a safer side, especially when it is in our control and the change required is small.

I'll make the appropriate changes in the follow-up commit.

The source of android.text.style.StyleSpan clearly shows that styles must be merged independent of current typeface. Spans have history within Paint, Canvas and other that need to be respected when the same elements are being manipulated.

Philosophy aside, if you want us to retain the old contract as much as possible, I suggest adding a boolean mergeStyles that could optionally be passed on to CustomTypefaceSpan.create (by the way, why does this class both have a factory-create and a public constructor?). Only when provided and true will the styles be merged. What say you?

[noties]

Hello @c-b-h ,

I like the mergeStyles solution as it doesn't break current implementation 👍

by the way, why does this class both have a factory-create and a public constructor?

It should not really, the constructor must've been at least package-private