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

[Text] Use CultureInfo.Name instead of ISOLanguageName for character matching #16053

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

Gillibald
Copy link
Contributor

@Gillibald Gillibald commented Jun 18, 2024

What does the pull request do?

It changes character matching to use a full bcp47 language identifier to be able to distinguish between different variants of a language family. The previous solution of specifying a two or three-letter tag wasn't enough to achieve this.

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: #12349

@Gillibald Gillibald added area-textprocessing 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 Jun 18, 2024
@avaloniaui-bot
Copy link

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

@robloo
Copy link
Contributor

robloo commented Jun 18, 2024

I know you don't always add docs to your PR descriptions for small changes, but the reasons you are doing this would ideally be documented in code.

When trying to match it seems like ISO codes are better and Name is a good fallback. I wonder if some smarter heuristics using all 3 make sense. Anyway, the more that is documented about why you made this change, based on what findings, the better.

@Gillibald
Copy link
Contributor Author

When I first introduced this part of the font manager I looked at the Skia source and it stated it is generally fine to use the ISO 639 language code so I used them but it looks like DirectWrite does not match font families properly in that case.

We can specify multiple languages so I can just add the new one as you pointed out. Thanks.

@YexuanXiao
Copy link

When I first introduced this part of the font manager I looked at the Skia source and it stated it is generally fine to use the ISO 639 language code so I used them but it looks like DirectWrite does not match font families properly in that case.

We can specify multiple languages so I can just add the new one as you pointed out. Thanks.

Thanks for fixing it. The two-letter and three-letter code are insufficient for users who utilize Chinese, as the current Chinese characters in use have at least three variants (China, China Taiwan, and Japan). They share the same Unicode code points, often represent the same meanings, but have different appearances. ISO 639 cannot distinguish between them.

@Gillibald Gillibald added this pull request to the merge queue Jun 19, 2024
Merged via the queue into AvaloniaUI:master with commit 0df64c5 Jun 19, 2024
11 checks passed
@Gillibald Gillibald deleted the fixes/characterMatching branch June 19, 2024 05:23
@robloo
Copy link
Contributor

robloo commented Jun 19, 2024

Really would have commented the main points here in source code. It's lost knowledge if you don't do that for stuff like this and someone in the years to come might change it accidentally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-textprocessing 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong default font when show CJK characters
5 participants