search

googlefonts / fontations

Reading and writing font files
Apache License 2.0
396 stars 26 forks source link

[write] Use Vec instead of BTreeSet for name records #1256

Closed cmyr closed 2 days ago

cmyr commented 2 days ago

It is an invariant that entries in the name_records array are sorted, and to enforce this at the type level the initial design used a BTreeSet for these records.

This didn't fully help, though, because it was still possible to have a duplicate entry in the array (the same platform/encoding/language/nameId, mapped to different strings) since the string itself is included in the Eq method of NameRecord.

This patch tries to be less cute: it is now the responsibility of the caller to make sure that their data is right, and we will verify this at validation time.

Ultimately this probably deserves a builder (there's one in fea-rs that should maybe get moved over here?)

cc @simoncozens; I recall you not enjoying the ergonomics of the BTreeSet approach, so here you go 💁

rsheeter commented 2 days ago

since the string itself is included in the Eq method of NameRecord

I appreciate this being a problem but ... wouldn't using a map model this better? - (plat/enc/lang/nameid) => value

cmyr commented 2 days ago

This is in generated code in write-fonts, where the types we're using are very close to what is in the spec. If we want to do something with a map, that would go in a NameBuilder or similar.