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

FontManager rework #3234

Merged
merged 8 commits into from Dec 6, 2019
Merged

FontManager rework #3234

merged 8 commits into from Dec 6, 2019

Conversation

@Gillibald
Copy link
Contributor

Gillibald commented Nov 9, 2019

What does the pull request do?

It changes FontManager to be responsible for caching loaded typefaces. Constructing a typeface can a very costly operation so reuse as much as we can is required.

This also changes how the default FontFamily is defined. Now there is a special instance that indicates that the default should be used.

To get the actual default's name a user has to ask the font manager for the name.

This also removes issues with the initialization order of the platform in combination with the font manager.

What is the current behavior?

Currently, there are multiple places where resources related to the font face are cached.

What is the updated/expected behavior with this PR?

The font manager is now responsible for caching font-related resources.

How was the solution implemented (if it's not obvious)?

Typefaces are not constructed by a FontKey that represents a unique combination of (FontFamily, FontWeight, FontStyle) that way we can avoid creating multiple instances.

Checklist

Breaking changes

Fixed issues

Fixes #3224

@Gillibald Gillibald changed the title FontManager rework [WIP] FontManager rework Nov 9, 2019
@Gillibald Gillibald changed the title [WIP] FontManager rework FontManager rework Nov 16, 2019
src/Avalonia.Visuals/Media/FontFamily.cs Outdated Show resolved Hide resolved
src/Avalonia.Visuals/Media/FontFamily.cs Outdated Show resolved Hide resolved
@Gillibald Gillibald requested a review from AvaloniaUI/core Nov 26, 2019
@Gillibald Gillibald mentioned this pull request Nov 29, 2019
0 of 3 tasks complete
@donandren

This comment has been minimized.

Copy link
Member

donandren commented Nov 30, 2019

@Gillibald #3308 is not fixed for direct2d, although proper typeface is resolved( like in the direct2d fotnmanagerimpl unit test) rendering of formattedtext is using system default font.
this is what is missing for diect2d -> https://github.com/AvaloniaUI/Avalonia/pull/3309/files#diff-5ce38b060142020056c7d38b141ce908R25-R27
need to pass the properly resolved font name to the DWrite.TextFormat
on skia side everything is fine in regard for #3308

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Nov 30, 2019

The GlyphTypefaceImpl should have the correct font. Will change the formatted text impl to use that.

…perly with the application lifetime
@Gillibald Gillibald force-pushed the Gillibald:reworkedFontManager branch from 1138448 to 1b0221e Dec 4, 2019
@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Dec 4, 2019

@donandren Is the Direct2D1 backend now working for you with font fallbacks?

@donandren

This comment has been minimized.

Copy link
Member

donandren commented Dec 4, 2019

@donandren Is the Direct2D1 backend now working for you with font fallbacks?

not working properly, repro steps at #3308

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Dec 4, 2019

@donandren Should be fixed now.

@grokys
grokys approved these changes Dec 6, 2019
Copy link
Member

grokys left a comment

Looks good to me, apart from one minor nit; however I must say I don't know a whole lot about fonts so i could have missed something. Mainly got some questions.

src/Avalonia.Visuals/Media/FontManager.cs Outdated Show resolved Hide resolved
IEnumerator IEnumerable.GetEnumerator()
{
return GetEnumerator();
}

public struct Enumerator : IEnumerator, IEnumerator<string>

This comment has been minimized.

Copy link
@kekekeks

kekekeks Dec 6, 2019

Member

It might be better to extract this enumerator to some utility namespace. ImmutableReadOnlyListStructEnumerator<T> or something.

This comment has been minimized.

Copy link
@Gillibald

Gillibald Dec 6, 2019

Author Contributor

@kekekeks I did this change. Please let me know if that's what you wanted.

if (culture == null)
{
culture = CultureInfo.CurrentUICulture;
}

if (s_languageTagBuffer == null)

This comment has been minimized.

Copy link
@kekekeks

kekekeks Dec 6, 2019

Member

@grokys we probably need a different naming convention for [ThreadStatic] fields.

This comment has been minimized.

Copy link
@grokys

grokys Dec 6, 2019

Member

Perhaps yeah, do you know what anyone else uses?

This comment has been minimized.

Copy link
@kekekeks

kekekeks Dec 6, 2019

Member

s_tlsFieldName might be an option.

This comment has been minimized.

Copy link
@Gillibald

Gillibald Dec 6, 2019

Author Contributor

CoreFX uses t_fieldName

@kekekeks

This comment has been minimized.

Copy link
Member

kekekeks commented Dec 6, 2019

Why exactly are we changing the way of FontManager instantiation? It used to be registered during render platform initialization, but now we are creating it lazily.

@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Dec 6, 2019

I guess we can just register the FontManager instance during initialization but then we can't enforce a platform implementation. The render platform has to implement CreateFontManagerImpl. We can change the way FontManager.Current is implemented.

Gillibald added 3 commits Dec 6, 2019
@Gillibald

This comment has been minimized.

Copy link
Contributor Author

Gillibald commented Dec 6, 2019

Merging this now. We can change the FontManager.Current implementation later if you still have concerns.

@Gillibald Gillibald merged commit c4708e2 into AvaloniaUI:master Dec 6, 2019
2 of 3 checks passed
2 of 3 checks passed
Avalonia (Pull Requests) Avalonia (Pull Requests) failed
Details
AvaloniaUI.Avalonia #20191206.5 succeeded
Details
WIP Ready for review
Details
@Gillibald Gillibald deleted the Gillibald:reworkedFontManager branch Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.