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

feat: Font manifest #17118

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

Youssef1313
Copy link
Member

GitHub Issue (If applicable): closes #

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@github-actions github-actions bot added the area/skia ✏️ Categorizes an issue or PR as relevant to Skia label Jun 13, 2024
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@github-actions github-actions bot added platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform area/automation Categorizes an issue or PR as relevant to project automation labels Jun 13, 2024
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@carldebilly
Copy link
Member

Is that manifest a standard thing or something we're creating?

@NVLudwig
Copy link

@Youssef1313 This is a very unexpected addition.
Thank you very much.
Support for Line Height across platforms would be a game changer :)

Comment on lines +332 to +348
public FontStretch FontStretch
{
get => (FontStretch)this.GetValue(FontStretchProperty);
set => this.SetValue(FontStretchProperty, value);
}

public static DependencyProperty FontStretchProperty { get; } =
DependencyProperty.Register(
nameof(FontStretch),
typeof(FontStretch),
typeof(ContentPresenter),
new FrameworkPropertyMetadata(
FontStretch.Normal,
FrameworkPropertyMetadataOptions.Inherits,
(s, e) => ((ContentPresenter)s)?.OnFontStretchChanged((FontStretch)e.OldValue, (FontStretch)e.NewValue)
)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use generated properties

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeromelaban For readability or perf?

My concern with generated DPs is that they cache the value when it changes. So, it's faster, but increases the object size. So my personal view to it is "speed vs memory"

Comment on lines +719 to +729
public static DependencyProperty FontStretchProperty { get; } =
DependencyProperty.Register(
nameof(FontStretch),
typeof(FontStretch),
typeof(Control),
new FrameworkPropertyMetadata(
FontStretch.Normal,
FrameworkPropertyMetadataOptions.Inherits,
(s, e) => ((Control)s)?.OnFontStretchChanged((FontStretch)e.OldValue, (FontStretch)e.NewValue)
)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated properties

Comment on lines +196 to +206
public static DependencyProperty FontStretchProperty { get; } =
DependencyProperty.Register(
nameof(FontStretch),
typeof(FontStretch),
typeof(TextBlock),
new FrameworkPropertyMetadata(
defaultValue: FontStretch.Normal,
options: FrameworkPropertyMetadataOptions.Inherits,
propertyChangedCallback: (s, e) => ((TextBlock)s).OnFontStretchChanged()
)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated properties

src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.skia.cs Outdated Show resolved Hide resolved
{
// While font family itself didn't change, OnFontFamilyChanged will invalidate whatever
// needed for the rendering to happen correct on the next frame.
_fontInfo.FontUpdated += OnFontFamilyChanged;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

src/Uno.UI/UI/Xaml/Documents/Run.skia.cs Outdated Show resolved Hide resolved
Comment on lines +120 to +130
public static DependencyProperty FontStretchProperty { get; } =
DependencyProperty.Register(
nameof(FontStretch),
typeof(FontStretch),
typeof(TextElement),
new FrameworkPropertyMetadata(
defaultValue: FontStretch.Normal,
options: FrameworkPropertyMetadataOptions.Inherits,
propertyChangedCallback: (s, e) => ((TextElement)s).OnFontStretchChanged()
)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated properties

}
catch
{
// manifest file is not found or cannot be read. Ignore it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an existing internal API to determine the presence of a file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeromelaban What is it? I couldn't find it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeromelaban That's in Uno.UI.Toolkit 😕

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@Youssef1313
Copy link
Member Author

Is that manifest a standard thing or something we're creating?

@carldebilly It's something we're creating/designing ourselves.

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-17118/index.html

@github-actions github-actions bot added platform/macos 🍏 Categorizes an issue or PR as relevant to the macOS platform platform/ios 🍎 Categorizes an issue or PR as relevant to the iOS platform labels Jun 18, 2024
@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

uri = new Uri(FontManifestHelpers.GetFamilyNameFromManifest(manifestStream, weight, style, stretch));
}
}
catch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not hide the original error

}
}

var file = await StorageFile.GetFileFromApplicationUriAsync(uri);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's log in debug that we're fetching a font


// SKTypeface.FromFile may return null if the file is not found (SkiaSharp is not yet nullable attributed)
skTypeFace = SKTypeface.FromFile(filePath);
if (Uri.TryCreate(name, UriKind.Absolute, out var uri) && uri.Scheme == "ms-appx")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have IsLocalResource

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

1 similar comment
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-17118/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-17118/index.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/automation Categorizes an issue or PR as relevant to project automation area/skia ✏️ Categorizes an issue or PR as relevant to Skia platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform platform/ios 🍎 Categorizes an issue or PR as relevant to the iOS platform platform/macos 🍏 Categorizes an issue or PR as relevant to the macOS platform platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants