WIP: Implement de- and serialization for all fontinfo attributes

[madig]

According to serde-rs/serde#1301 and serde-rs/serde#1444, easily de/serializing things with custom functions inside containers like Option is not possible, so you have to handle everything around it. Since fontinfo is quite expansive with more custom stuff (and de/serialization) to come, maybe it should move to its own module mid-term.

[madig]

Todo:

• Move to font_info.rs file.
• Implement validation for all fields
• Implement serialization?

[cmyr]

This looks good. I think it's probably worth moving to a separate file as part of this PR? But I think that validation and serialization make sense to to separately.

[madig]

I think I have to properly organize de/serializer code now. Also, need to parse guidelines into existing glyph::Guideline.

[cmyr]

Okay, just ping me when you think this is ready!

[madig]

So, now all possible fields actually get deserialized in the test. What's needed next is custom types (e.g. OS2Panose, OS2FamilyClass, the WOFF dir attributes that should be an enum, etc.) plus custom de/serializers to perform validation. The glyph::Guideline type should be reused for fontinfo and might need more extensive custom de/serialization. Not sure what to do about standard types that need validation, like the WOFF list attributes where the spec demands they contain at least one item.

[madig]

Do Identifiers need their own ::new and stuff? The UFO spec has an example algorithm.

[madig]

Not sure I need all the Visitors? Maybe doing the work in just deserialize would be enough.

[cmyr]

Identifiers are annoying, I'm not sure how best to handle them. Probably yes, we should have a newtype and store them a s a string, but we should validate them on creation.

[madig]

Same for e.g. openTypeOS2VendorID:

Four character identifier for the creator of the font. Corresponds to the OpenType OS/2 table achVendID field.

OS/2 spec, tag spec:

Array of four uint8s (length = 32 bits) used to identify a table, design-variation axis, script, language system, feature, or baseline
[...]
A Tag value is a uint8 array. Each byte within the array must have a value in the range 0x20 to 0x7E. This corresponds to the range of values of Unicode Basic Latin characters in UTF-8 encoding, which is the same as the printable ASCII characters. As a result, a Tag value can be re-interpreted as a four-character sequence, which is conventionally how they are referred to. Formally, however, the value is a byte array. When re-interpreted as characters, the Tag value is case sensitive. It must have one to four non-space characters, padded with trailing spaces (byte value 0x20). A space character cannot be followed by a non-space character.

So maybe both Tag and Identifier should be newtypes with a ::new and/or ::from_string or something.

This reminds me that my Identifier verification is wrong.

[madig]

Hrm. There's some small stuff that needs validation, like various postscript* list attributes have a length limit. I think it is much less work to implement a is_valid function on FontInfo that is run before serialization and after deserialization?

[cmyr]

Yea, I think a validation function is totally reasonable.

[madig]

Went through the spec again at http://unifiedfontobject.org/versions/ufo3/fontinfo.plist/ and found some type discrepancies. Also changed some Todo comments, the spec does not mandate checks for some attributes. The spec is also somewhat lenient with types, declaring lots of stuff Integer when the OT spec specifies int16, etc. I am thinking about introducing some type aliases like Integer = i32, NonNegativeInteger = u32, just to be very spec-spirit compliant? http://unifiedfontobject.org/versions/ufo3/conventions/ does not define limits.

[cmyr]

I'm happy either way with the aliases.

[madig]

I was just informed that the UFO spec is going to grow a minor version soon, 3.1, so that the glyph element can grow a verticalOrigin attribute in advance element.

[anthrotype]

unified-font-object/ufo-spec#78 and unified-font-object/ufo-spec#95

[anthrotype]

the UFO spec is going to grow a minor version soon, 3.1

also the GLIF <glyph> element will have a corresponding additional formatMinor attribute.

[madig]

note to self:

• Complete validation. Crude parsing of openTypeHeadCreated: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=362260516a1c3a981e1d72199c13800d
• Test FontInfo serialization?
• Error enums. Would they benefit any process other than loading? if Runebender ever generates invalid data, it probably can't recover easily?
• Document more. See how it looks in cargo doc.

[cmyr]

Okay some general thoughts but overall this looks good, thanks so much for taking it on!

I think it's worth merging this soon as a checkpoint, and then we can build out better error handling etc from there.

[cmyr]

for all these parse errors we could do something like, x.parse::<u8>().map_err(|_| Error::FontInfoError)?

[cmyr]

oh, one other suggestion here: run cargo clippy on this patch, and see what suggestions it comes up with.

[madig]

That was a lot of lints. Fixed them all.

[cmyr]

Okay, I haven't gone over all the latest changes closely, but I trust between you and clippy they make sense. I think this is a good time to get this merged, and then we can build from here.

I've added you as a collaborator to this repo; feel free to press the big green merge button when you think it's ready!

[madig]

Edit: duh, needed to accept the invite.