[fontscan] Better font resolution #178
Conversation
…stitutions table from upstream fontconfig
…c family match strategy
benoitkugler
requested review from
whereswaldon and
andydotxyz
as code owners
|
Thanks for doing this. I have a concern about another breaking change and I don't think this does have to be breaking. There are two font map implementations and one doesn't use the script, so it seems like a big change on the API level - can we not leave it only on the maps that support it, and make it optional usage and avoid the breaking change? |
I think that would require to type assert if the fontmap implements the extended interface (with [SetScript]). Perhaps not a big deal. On the other hand, is there major pain point with the breaking change ? Assuming it is properly tagged. |
|
Breaking changes cause people pain, and I feel they should only be done with good reason. Any "go get -u" will break compilation unless every project using this API releases at exactly the same time. In this example it does not seem to be necessary so it would be good to avoid. |
Fair enough. I had overlooked the fact that |
|
69e1db3 uses an optional interface to preserve backward compatibility. |
fontscan/match.go
Outdated
| func (fm fontSet) selectByFamilies(crible familyCrible, footprintBuffer *scoredFootprints) []int { | ||
| footprintBuffer.reset(fm) | ||
| // select the fonts in the fontSet matching [crible], returning their (sorted) indices. | ||
| // [buffer1] and [buffer2] are used to reduce allocations. |
There was a problem hiding this comment.
Nit: Outdated parameter names
There was a problem hiding this comment.
I think this was on a previous version of the changes, and has been fixed.
fontscan/fontmap.go
Outdated
| // | ||
| // 1 - Only fonts matching exacly one of the [Query.Families] are considered; the list | ||
| // is prunned to keep the best match with [Query.Aspect] | ||
| // 2 - Fallback fonts are considered, that is fonts with similar families and fonts | ||
| // supporting the current script; the list is also prunned according to [Query.Aspect] | ||
| // 3 - All fonts matching the current script (set by [FontMap.SetScript]) are tried, | ||
| // supporting the current script; the list is also prunned according to [Query.Aspect]4 |
There was a problem hiding this comment.
Nit: Trailing 4 is probably a typo.
There was a problem hiding this comment.
Thanks, fixed.
Hi,
This PR aims at providing a better font resolution, especially in the fallback case (where the family asked by the user do not support the text runes).
The main changes (inspired by fontconfig) are :
This is a breaking change, since the
shaping.Fontmapinterface now requires to be script aware (via the added methodSetScript). The two defaults implementations are updated accordingly.As a side note, the language table is updated (meaning the internal IDs have changed), so that the cache version is bumped.
Thank you for your feedback !