mono / SkiaSharp

SkiaSharp is a cross-platform 2D graphics API for .NET platforms based on Google's Skia Graphics Library. It provides a comprehensive 2D API that can be used across mobile, server and desktop models to render images.
MIT License
4.55k stars 543 forks source link

[BUG?] SKTypeface.FromFamilyName should not return null... or could? #1058

Open mattleibow opened 4 years ago

mattleibow commented 4 years ago

Description

Not sure what is going with SKTypeface.FromFamilyName. The code docs say it should NEVER return null, but does and should in some cases? Those are conflicting statements right there.

I see this previous issue where I say that there is no guarantee: https://github.com/mono/SkiaSharp/issues/643

But, there is in this docs:

    /** Creates a new reference to the typeface that most closely matches the
        requested familyName and fontStyle. This method allows extended font
        face specifiers as in the SkFontStyle type. Will never return null.

https://github.com/mono/skia/blob/xamarin-mobile-bindings/include/core/SkTypeface.h#L95-L104

And then we go straight into this:

            // On Android, we must return nullptr when we can't find the requested
            // named typeface so that the system/app can provide their own recovery
            // mechanism. On other platforms we'd provide a typeface from the
            // default family instead.

https://github.com/mono/skia/blob/xamarin-mobile-bindings/src/ports/SkFontMgr_android.cpp#L475-L484

Fear!

Never mind the fontconfig variants which basically just return nullptr: https://github.com/google/skia/blob/chrome/m68/src/ports/SkFontMgr_fontconfig.cpp#L894

Who knows at this point!

mattleibow commented 4 years ago

This PR (https://github.com/toptensoftware/RichTextKit/pull/6) in RichTextKit fixes it, but I am not sure if we should fix this in SkiaSharp. Isn't a null match meaning that there is no match.

mattleibow commented 4 years ago

I am thinking about this and I am starting to agree with #643 and just say it may return null if no match was found.

Is there a point in SkiaSharp doing the extra work on the overloads? Is a ?? SKTypeface.Default fine to add? Not even sure that is useful to do by default as when you ask for a match and then just gives you a random one? That is totally unexpected. I would even go so far as to say that we should return null if none was found and then have the user decide what is actually wanted. But changing that in the core skia is going to be work, and may break other things that are using this "feature".

Gillibald commented 4 years ago

I need a way to know that a font wasn't found. If I can always compare with the default it is fine to return the default. I doubt that always works.

mattleibow commented 4 years ago

That's the thing... Deep inside the logic, it may/may not decide to use default.

It might be better to use the SKFontManager and use GetFontStyles: https://docs.microsoft.com/en-us/dotnet/api/skiasharp.skfontmanager.getfontstyles

You can then also find the nearest bold/italic combo that tickles your fancy. For example, there my be a black and not a bold or something.

(but Matthew starts to wonder if their promise of never null is also a lie here)

mattleibow commented 4 years ago

I think to be consistent, the SKTypeface should never return null, but SKFontManager is to be used to see if there was a match. I'll do some tests.

Gillibald commented 4 years ago

FromFamilyName already does the fallback for you. https://docs.microsoft.com/en-us/dotnet/api/skiasharp.sktypeface.fromfamilyname?view=skiasharp-1.68.1#SkiaSharp_SKTypeface_FromFamilyName_System_String_SkiaSharp_SKFontStyleWeight_SkiaSharp_SKFontStyleWidth_SkiaSharp_SKFontStyleSlant_

I prefer getting a null if the request can't be fulfilled so I can deside how to proceed.

mattleibow commented 4 years ago

That's the thing, on Android and Linux, it doesn't. It may return null.

Gillibald commented 4 years ago

Currently I do this https://github.com/AvaloniaUI/Avalonia/blob/master/src/Skia/Avalonia.Skia/FontManagerImpl.cs#L97 this works as long as the default never changes.

mattleibow commented 4 years ago

If I do this on Android:

var tf = SKTypeface.FromFamilyName("System");

I get a null, on macOS I get the default. But, it is not equal to SKTypeface.Default...

mattleibow commented 4 years ago

I tried:

var fm = SKFontManager.Default.GetFontStyles("System");
var fm2 = SKFontManager.Default.MatchFamily("System", SKFontStyle.Bold);

var tf = SKTypeface.FromFamilyName("System");

on macOS, none are null, but fm is an empty set. fm2 is the default.

on Android, fm is also an empty set, but fm2 and tf are null

mattleibow commented 4 years ago

Looking at the implementation of the matching, it pretty much always creates a new reference to the font. So we can't even use the == Default because it creates a new object each time.

Gillibald commented 4 years ago

Is the UniqueId also different?

mattleibow commented 4 years ago

I haven't exposed that, but most probably as the only reference to that field is just a blind new, incrementing value:

https://github.com/mono/skia/blob/xamarin-mobile-bindings/src/core/SkTypeface.cpp#L21

Gillibald commented 4 years ago

Why on earth did they do that? So I have to cache Typefaces on my own? 🤔

mattleibow commented 4 years ago

Looking at Android some more, and I see that if I match Helvetica - it matches, but sans-serif is the font name...

mattleibow commented 4 years ago

Looks like the most consistent is the SKFontManager.Default.GetFontStyles(<family>) and that returns a list of supported styles. If there is no font, then there are no styles.

You can then use SKFontManager.Default.MatchFamily(<family>, <style>) to find the specific typeface.

Gillibald commented 4 years ago

It depends on the actual font manager. For platforms that are using FontConfig aliases are sometimes used to resolve some font.

Gillibald commented 4 years ago

Good to know this. Will change my code to use the SKFontManager to resolve fonts.