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.

Sign up for GitHub

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

Jump to bottom

[Text] Multiple text processing fixes #15837

Merged
merged 14 commits into from
Jun 5, 2024
Merged

[Text] Multiple text processing fixes #15837

merged 14 commits into from
Jun 5, 2024

Conversation

Gillibald
Copy link
Contributor

@Gillibald Gillibald commented May 28, 2024 •
edited
Loading

What does the pull request do?

It adds basic font table loading capabilities so we can process the font metadata ourselves without relying on Skia.

  • Adds name table processing for localized font family names
  • Adds OS/2 HorizontalHeadTable for font metrics calculation
  • Adds supported font feature list to IGlyphTypeface2
  • Fixes baseline calculation
  • Fixes TextLineImpl baseline for drawable runs

What is the current behavior?

What is the updated/expected behavior with this PR?

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

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes: #15597

@Gillibald Gillibald added backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels May 28, 2024
This was referenced May 28, 2024
Closed
Closed
@Gillibald Gillibald changed the title Add font table loading [WIP]Add font table loading May 28, 2024
@robloo
Copy link
Contributor

robloo commented May 28, 2024

I don't think the SixLabors code can be used in Avalonia:

  1. SixLabors dual licenses. It is NOT for commercial use. Avalonia however is allowed for commercial use. So if anyone uses Avalonia in a commercial setting they can't actually use SixLabors code under Apache 2.
  2. Apache is less permissive than MIT. I'm not a lawyer but I wouldn't mix and match the code per se. I wouldn't copy in Uno Platform code for a control for example. It it was an entire component/layer of Avalonia it might be easier to manage.

We need direct written permission from SixLabors to use their code here I think.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048652-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@Gillibald
Copy link
Contributor Author

I don't think the SixLabors code can be used in Avalonia:

  1. SixLabors dual licenses. It is NOT for commercial use. Avalonia however is allowed for commercial use. So if anyone uses Avalonia in a commercial setting they can't use SixLabors code under Apache 2.
  2. Apache is less permissive than MIT. I'm not a lawyer but I wouldn't mix and match the code per se. I wouldn't copy in Uno Platform code for a control for example. It it was an entire component/layer of Avalonia.

We need direct written permission from SixLabors to use their code here I think.

MIT license is compatible with the Apache 2.0 license and the code was taken from a point in time when no dual license existed. We can ask @JimBobSquarePants for permission if you want to clarify things.

@robloo
Copy link
Contributor

robloo commented May 28, 2024

@Gillibald If you copied the code from before the license change a few years ago there is no show-stopper license conflict.

Perhaps a date should be noted in the header of the source code to avoid any potential concerns for posterity. (Ported from ... at commit dated

While MIT and Apache 2.0 code is usable together the requirements are quite different. I think we need to note in documents that some portions of source code are under a different license. End apps may need to distribute a copy of the Apache 2.0 license as well. Not saying anything of the patent grants and prominant file change notices which Avalonia is now going to have to follow. These complications are why I really, really try to avoid mixing licenses in source code that is side-by-side.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048674-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@Gillibald Gillibald removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels May 29, 2024
@Gillibald Gillibald changed the title [WIP]Add font table loading Add font table loading capabilities to GlyphTypeface May 29, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048676-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@JimBobSquarePants
Copy link

I don't think the SixLabors code can be used in Avalonia:

  1. SixLabors dual licenses. It is NOT for commercial use. Avalonia however is allowed for commercial use. So if anyone uses Avalonia in a commercial setting they can't use SixLabors code under Apache 2.
  2. Apache is less permissive than MIT. I'm not a lawyer but I wouldn't mix and match the code per se. I wouldn't copy in Uno Platform code for a control for example. It it was an entire component/layer of Avalonia.

We need direct written permission from SixLabors to use their code here I think.

MIT license is compatible with the Apache 2.0 license and the code was taken from a point in time when no dual license existed. We can ask @JimBobSquarePants for permission if you want to clarify things.

Not that you need it but you have my blessing.

@Gillibald
Copy link
Contributor Author

Gillibald commented May 29, 2024 •
edited
Loading

Potentially fixes: #10658 and #15768

@robloo How do I verify this PR fixes the referenced issues?

@robloo
Copy link
Contributor

robloo commented May 29, 2024

For my reported issue I use the ComboBox in the ControlCatalog with individual fonts. Stepping through you can visually see if things are aligned or not. I can take a look later too.

Not sure how to unit test this but it probably can be done with text formatter APIs and some carefully chosen font and strings. The font would have to exist on the test PC though so not sure that's a good idea to add at this point.

@Gillibald
Copy link
Contributor Author

Screenshot 2024-05-29 152704
Calibri Word vs Avalonia

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048686-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@AngryCarrot789
Copy link

AngryCarrot789 commented May 29, 2024 •
edited
Loading

I can confirm that 11.2.999-cibuild0048686-alpha fixes the font rendering for Oxanium.
image

Consolas looks weird (in my app on the right) compared to windows notepad (left), not sure if it's just me though.
Notepad is font size 11, mine is font size 13 and the selection area is the same height as notepad, text height and width is smaller on mine despise a bigger font size
image

@Gillibald
Copy link
Contributor Author

The left side is somehow stretched vertically

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048690-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048698-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@Gillibald Gillibald changed the title Add font table loading capabilities to GlyphTypeface [WIP] Add font table loading capabilities to GlyphTypeface May 30, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048718-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@Gillibald Gillibald changed the title [WIP] Add font table loading capabilities to GlyphTypeface [Text] Multiple text processing fixes May 30, 2024
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048720-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@robloo
Copy link
Contributor

robloo commented Jun 5, 2024

I've checked this with the font ComboBox in ControlCatalog:

capturegif

There are some major improvements for sure. It functions better than the alternative implementation here as well: #15344 (comment).

I'm still a little suspicious of some fonts though. Courier New for example should likely vertically center, Consolas as well. I'm not sure this is 100% yet but don't know enough about this area to be more specific.

@Gillibald Gillibald added this pull request to the merge queue Jun 5, 2024
Merged via the queue into AvaloniaUI:master with commit 2dfd9be Jun 5, 2024
10 checks passed
@Gillibald Gillibald deleted the feature/fontTableLoading branch June 5, 2024 07:43
@kurobirds
Copy link

Hello @Gillibald, Can you re-check with FontStretch property? It not render correctly compare to version 11.0.10.

Here is my code to reproduce:

<TextBlock FontWeight="Heavy"
           FontStretch="Normal"
           FontSize="48"
           LineHeight="56"
           Text="Normal" />
<TextBlock FontWeight="Heavy"
           FontStretch="Condensed"
           FontSize="48"
           LineHeight="56"
           Text="Condensed" />
<TextBlock FontWeight="Heavy"
           FontStretch="Expanded"
           FontSize="48"
           LineHeight="56"
           Text="Expanded" />

image

@Gillibald
Copy link
Contributor Author

Gillibald commented Jun 5, 2024 •
edited
Loading

What font are you using? I don't think there is any heavy condensed or expanded font face.

@kurobirds
Copy link

I'm using default font face {compositefont:fonts:Inter#Inter, $Default#Inter, $Default}, it also happened on UniversNextPro font.

@Gillibald
Copy link
Contributor Author

Screenshot 2024-06-05 125944
<TextBlock FontWeight="Black" FontStretch="Condensed" FontFamily="/#Scan" Text="Hello World" FontSize="44"/>
Some condensed heavy font from the internet. Works just fine. I don't think any system comes with a font that covers this combination.

You request a heavy condensed font but it does not find one so the weight is simulated and the stretch is ignored.

@kurobirds
Copy link

The issue is that in version 11.0.10, it can detect my FontStretch, but in version 11.2.999-cibuild0048720-alpha, it cannot. Can you check this with the repository: AvaloniaFontStretchIssue? You can change the AvaloniaVersion in Directory.Build.props and rebuild for comparison.

@AngryCarrot789
Copy link

What version will this be officially released in? It doesn't appear to be in 11.1.1 since I still have text rendering issues on fonts like Consolas, but 11.2.999-cibuild0048720-alpha does work properly

@Gillibald
Copy link
Contributor Author

This will be part of the next release

@maxkatz6 maxkatz6 added bug and removed enhancement labels Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jp2masa jp2masa jp2masa left review comments

@maxkatz6 maxkatz6 maxkatz6 approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Font Features, Languages, and Name IDs on GlyphTypeface
8 participants
@Gillibald @robloo @avaloniaui-bot @JimBobSquarePants @AngryCarrot789 @kurobirds @maxkatz6 @jp2masa