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

[dominikh]

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]

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]

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]

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]

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]

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]

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]

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]

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.