search

go-text / typesetting

High quality text shaping in pure Go.
Other
100 stars 10 forks source link

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

Open dominikh opened 6 months ago

dominikh commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.