Make all Font fields non-optional

[madig]

To make API more ergonomic.

Closes #159

[madig]

Random thought: should I rename Font.font_info to Font.info?

[github-actions]

🗜 Bloat check ⚖️

Comparing e79644f against d187ea4

target	old size	new size	difference
target/release/examples/load_save	1.79 MB	1.8 MB	18.26 KB (1.00%)
target/debug/examples/load_save	8.31 MB	8.46 MB	151.34 KB (1.78%)

[cmyr]

Random thought: should I rename Font.font_info to Font.info?

The original intention for norad was to match the spec as closely as possible, including in the naming of fields. Maybe let's just stick with that? It's a nice guideline or these sorts of questions.

[cmyr]

I totally agree that this is an annoying part of the API, and in some instances (like groups/kerning) I think that treating empty collections as None is reasonable, since empty collections here is meaningless.

I'm less sure about font_info; there is a distinction between no fontinfo and fontinfo that just happens to have default values (that have been chosen by norad, and have nothing to do with the spec).

So my gut feeling here is that for fontinfo we just have to accept that the API isn't great, and find other ways to work around this?

[cmyr]

What if the fontinfo intentionally has values that just happen to all be default?

[madig]

Then the fontinfo.plist is not written out AFAIK :)

[madig]

that have been chosen by norad, and have nothing to do with the spec

Hm which ones? The values should assume the default values (None or something else) as laid out in https://unifiedfontobject.org/versions/ufo3/fontinfo.plist/.

[cmyr]

oops, thought i'd responded to this!

I was confused; in runebender I assign some default ppem/ascender/descender/xHeight/capHeight values if they're missing, and I was thinking of those defaults. If every field on font_info.plist is optional, that's not a concern.

[madig]

@belluzj says:

Be careful with your PRs that change Option into just kerning; we've had that issue before that round-tripping a UFO would add or remove empty arrays because the library in question doesn't make the difference between empty array or no array at all
Just saying; I don't know what's best

Hm. I'll run some tests to see what ufoLib(2) and ufonormalizer do with empty kerning and groups files. I remember these problems and think that they are a bug in varLib...

[madig]

A quick test shows:

1. ufoLib2 and defcon start with an empty groups and kerning dict attribute. If it's empty, the corresponding plist files are not written to disk (or deleted if they existed before).
2. defcon always writes out a couple fontinfo list attributes, ufoLib2 doesn't. So, defcon always writes a fontinfo.plist with some empty attributes, ufoLib2 won't.
3. You can write default values to fontinfo in both and will get a fontinfo.plist, but if all attributes are None, no fontinfo.plist is written.
4. ufonormalizer deletes empty groups, kerning and fontinfo plist files (i.e. if they contain an empty dict element)

So I think this PR mirrors what defcon and ufoLib2 are doing.

[chrissimpkins]

How about optionally supporting #4?

[madig]

Hm? What do you mean?

[chrissimpkins]

If we currently support (1) in #187 (comment), would it be appropriate to add support for optionally removing all spec optional empty plist files at serialization in line with the ufonormalizer approach?

I suppose it is also worth considering if/how a change in file structure should happen across the full I/O roundtrip. If you read in a kerning.plist without kerning data, do you write out a UFO dir without kerning.plist included? We might want to make this an optional file structure change if that is the plan.

[madig]

would it be appropriate to add support for optionally removing all spec optional empty plist files at serialization in line with the ufonormalizer approach?

That's what should happen with this PR, IIUC? Norad always wipes the existing UFO in

norad/src/font.rs

Lines 256 to 258 in bfe3f27

	if path.exists() {
	fs::remove_dir_all(path)?;
	}

. We only write out new files if they contain something.

I suppose it is also worth considering if/how a change in file structure should happen across the full I/O roundtrip. If you read in a kerning.plist without kerning data, do you write out a UFO dir without kerning.plist included?

Yes; defcon, ufoLib2 and norad do just that by default. It makes sense to me, because it's a nicer API to start with an empty dict instead of a None, and then there is no meaningful diff between an empty plist and a non-existent one :)

[madig]

https://unifiedfontobject.org/versions/ufo3/kerning.plist/

Kerning pairs that are not defined in kerning.plist implicitly have a value of “no kerning” or zero. Therefore any kerning pair that has a value of zero, unless it is a necessary value of an exception, should not be stored in kerning.plist.

I think this implies that no kerning.plist means an empty kerning dict.

I can't find similar wording for groups but assume the same applies by analogy.

[madig]

If you're worried that several tools do several things and we should provide options, I found that all standard tools (defcon, ufoLib2, ufonormalizer) drop empty fontinfo, kerning and groups plist files. If a fontinfo is full of Nones, neither defcon nor ufoLib2 write it out. So, that's consistent with what this PR is doing.

[cmyr]

I think this is a good simplification, sorry for the delay!