LayerSet + other layer-related tweaks #106

cmyr · 2021-04-15T19:01:15Z

This adds a LayerSet type that enforces various constraints on
layers.

Among other things, this means that:

Layer names are now also stored as Arc
There is no more LayerInfo type
there is now always a default layer
the default layer cannot be removed
new layers can be added

This also introduces a few other breaking api changes; for instance
I'm removing Font::get_default_layer(&self) -> Option<&Layer>
in favour of Font::default_layer(&self) -> &Layer.

In addition it moves the "compute a file name from a user provided name"
logic into a new util module.

This was motivated by my experiments with python bindings,
but I think this is generally useful independent of that work
and so I'm PRing it separately.

supersedes and closes Fold LayerInfo into Layer #100
closes Add Ufo::new_layer(name) method #84 (with an asterisk, this method is on the public LayerSet type)
closes Ensure default layer is always present #89

@madig I caught a few issues here thanks to your tests! 🎉

This takes the previous default_file_name_for_glyph_name function out of the glyph module and moves it to a new `util` module; it then adapts it to also work for layer names.

src/ufo.rs

src/upconversion.rs

src/layer.rs

src/ufo.rs

madig · 2021-04-15T21:45:38Z

The idea of putting the default layer permanently into [0] is interesting. I'd love to fuzz the API to see if there is a weird corner case that unhinges it! Maybe needs a debug_assert in default_layer(_mut) etc. to ensure it is never dethroned?

madig · 2021-04-15T21:47:07Z

Also, the extra effort in normalizer to fish out the layers to delete is... unfortunate.

src/layer.rs

cmyr · 2021-04-16T01:45:44Z

Thanks for the feedback, I agree with everything.

I also agree it's somewhat a shame that the normalizer example is more verbose now, but I don't really know what an alternative API would be that lets us maintain our guarantees. 🤔

github-actions · 2021-04-16T01:50:14Z

🗜 Bloat check ⚖️

Comparing 56fde0a against 06c42a2

target	old size	new size	difference
target/release/examples/open_ufo	1.77 MB	1.76 MB	-14.33 KB (-0.79%)
target/debug/examples/open_ufo	7.15 MB	7.18 MB	31.83 KB (0.43%)

madig · 2021-04-16T09:40:38Z

We could have a retain method that silently skips the default layer 🤔

cmyr · 2021-04-16T14:20:02Z

We could have a retain method that silently skips the default layer 🤔

Let's see what some more real-world use cases are, and then we can try and come up with an API that addresses them?

This adds a LayerSet type that enforces various constraints on layers. Among other things, this means that: - Layer names are now also stored as Arc<str> - There is no more LayerInfo type - there is now always a default layer - the default layer cannot be removed - new layers can be added This also introduces a few other breaking api changes; for instance I'm removing `Font::get_default_layer(&self) -> Option<&Layer>` in favour of `Font::default_layer(&self) -> &Layer`. This was motivated by my experiments with python bindings, but I think this is generally useful independent of that work and so I'm PRing it separately.

github-actions · 2021-04-16T22:30:24Z

🗜 Bloat check ⚖️

Comparing ddb3757 against 06c42a2

target	old size	new size	difference
target/release/examples/open_ufo	1.77 MB	1.76 MB	-14.33 KB (-0.79%)
target/debug/examples/open_ufo	7.15 MB	7.18 MB	31.5 KB (0.43%)

Generalize computing of paths from user strings

90ed488

This takes the previous default_file_name_for_glyph_name function out of the glyph module and moves it to a new `util` module; it then adapts it to also work for layer names.

This comment has been minimized.

Sign in to view

cmyr force-pushed the layerset branch from 85f884e to 62ea9d5 Compare April 15, 2021 19:17

This comment has been minimized.

Sign in to view