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

harfbuzz: mapping from language.Language to OpenType tags seems to be case sensitive #150

Open
dominikh opened this issue Mar 27, 2024 · 8 comments

Comments

@dominikh
Copy link
Contributor

Looking at tagsFromComplexLanguage in ot_language_table.go, the input to the function seems to have the same case that the user set for SegmentProperties.Language, so for example zh-Hant-HK. However, the implementation of the function is case sensitive and doesn't normalize the case. As such, checks like if langMatches(langStr[1:], "h-hant-hk") { don't actually match.

@dominikh
Copy link
Contributor Author

This is probably user-error and the intended usage of language.Language is to use language.NewLanguage to canonicalize the string before assigning it to SegmentProperties.Language. Unfortunately, it's easy to misuse this API. Feel free to close this issue, however.

@benoitkugler
Copy link
Contributor

This is probably user-error and the intended usage of language.Language is to use language.NewLanguage to canonicalize the string before assigning it to SegmentProperties.Language. Unfortunately, it's easy to misuse this API. Feel free to close this issue, however.

Precisely.
I had hoped that using a defined type was a strong enough hint. Perhaps it could be added in the documentation of the Language type, that using the constructor is mandatory ?

@dominikh
Copy link
Contributor Author

How do you feel about something like this instead?

type Language struct {
    lang string
}

func NewLanguage(lang string) Language { ... }

func (l Language) String() string { return l.lang }

The upside is that users are forced to go through NewLanguage to create languages. The downside is that manipulating languages, e.g. via slicing, is less trivial for everyone, and more costly for users as they'd have to go through NewLanguage again. Although arguably most users shouldn't have to manipulate languages in arbitrary ways, anyway.

@benoitkugler
Copy link
Contributor

How do you feel about something like this instead?

Looks great. It would not be backward compatible though, so I'm not sure how much this change is worth doing. @andydotxyz @whereswaldon what do you think ?

@dominikh
Copy link
Contributor Author

As an aside (which I'm happy to split into its own issue if need be), the handling of language.Script is also case-sensitive, but language.ParseScript doesn't do any normalization. So shaping with the script Arab doesn't work correctly, but shaping with arab does. That's particularly confusing because script names are preferably written in title-case.

@benoitkugler
Copy link
Contributor

As an aside (which I'm happy to split into its own issue if need be), the handling of language.Script is also case-sensitive, but language.ParseScript doesn't do any normalization. So shaping with the script Arab doesn't work correctly, but shaping with arab does. That's particularly confusing because script names are preferably written in title-case.

Yes, I encountered this issue as well. I think I kept the lowercase convention because it simplifies thé interaction with fonts at some point. Not sure though, I'll take a deeper look. language.ParseScript should definitively do the appropriate normalisation.

@whereswaldon
Copy link
Member

Looks great. It would not be backward compatible though, so I'm not sure how much this change is worth doing. @andydotxyz @whereswaldon what do you think ?

I'm okay with forcing language to be constructed in the interest of eliminating mistakes. I guess it's up to @andydotxyz

@andydotxyz
Copy link
Contributor

Looks great. It would not be backward compatible though, so I'm not sure how much this change is worth doing. @andydotxyz @whereswaldon what do you think ?

I'm okay with forcing language to be constructed in the interest of eliminating mistakes. I guess it's up to @andydotxyz

Agreed. Sorry for having been out of contact last week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants