Replace Shell sans-serif-medium magic string on Android #13544
Conversation
jfversluis
added
t/bug
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
-
What device were you running this on? I know I've seen this exception before, but I just tested on a few emulators and I'm not seeing the issue happen.
-
I noticed that on simulators (API 23 and API 30) that the font looked different with your change vs before your change. So, I'm still just trying to understand and track why that is.
Here's a pic.
Left is this PR and right is without this PR
|
I'd have to check in the actual device/emulators I've seen this on. I think I have an answer to the "why" though. I replaced it with SansSerif and not medium (because it's not in there) and the magic string had medium. So somehow we need to replicate that better then? Or maybe try to pin down on which devices this is happening exactly and maybe make it so that it applies the current way to devices where it works and the "new" way to devices where it gives the exception? |
| @@ -511,7 +511,6 @@ internal static DataTemplate CreateDefaultFlyoutItemCell(string textBinding, str | |||
|
|
|||
| defaultLabelClass.Setters.Add(new Setter { Property = Label.FontSizeProperty, Value = 14 }); | |||
| defaultLabelClass.Setters.Add(new Setter { Property = Label.TextColorProperty, Value = textColor }); | |||
| defaultLabelClass.Setters.Add(new Setter { Property = Label.FontFamilyProperty, Value = "sans-serif-medium" }); | |||
There was a problem hiding this comment.
I think this is the only one that throws from the magic string because the way we load magic strings on Label is by consulting the assets first and then it falls back to the Typeface call :-/
I wonder if there's a way we can change it to not throw so many times or know we need to not consult the assets.
I think the main place this exception comes from is the label. The code inside the
This is really a problem for any font that a user uses that's not part of our assets but will load as part of the OS fonts. Ideally, we could fix this for all magic strings. Maybe there's a way we can just fix this for the default |
| text.Typeface = services.GetRequiredService<IFontManager>() | ||
| .GetTypeface(Font.OfSize("sans-serif-medium", 0.0)); | ||
|
|
||
| text.SetTypeface(Typeface.SansSerif, TypefaceStyle.Normal); |
There was a problem hiding this comment.
Can we just delete this entire method? It's marked obsolete.
And from my tests it's never called.
|
Converting to draft while we talk more about this one |
|
Hi @jfversluis @PureWeen Any news here? |
|
@Rabosa616 not really unfortunately. We found that, despite the exception, this code actually does do something. So this PR isn't the right fix and I still need to get back to this to determine what is the right approach here. |
|
Hi @jfversluis |
Same here. Confirming. |
|
From what I can tell this is fixed by #15759, this isn't needed anymore |
Description of Change
In 3 places which had to do with Shell and/or TabbedPage we had a reference to
sans-serif-mediumas a string. This was causing exceptions, from what I can tell, on all Android devices and throwing underwater exceptions that the font could not be loaded.I've changed these references to the
TypeFace.SansSerifwhich is built-in and assume that it resolves to the same. From a visual test I don't see any differences.Thinking about it, there will probably be no differences, since the font couldn't be loaded earlier, it would not have shown as it was intended in the first place.
Unfortunately, I couldn't really find any definitive source on if this was removed on Android at some point or what the cause is that we are seeing this now.
Preferably I'd like @PureWeen to have a look being the one most likely to have implemented this
Issues Fixed
Fixes #13239