googlefonts / glyphsLib

A bridge from Glyphs source files (.glyphs) to UFOs
Apache License 2.0
182 stars 51 forks source link

Are .minimal and .minimize_glyphs_diffs really different? #805

Open simoncozens opened 2 years ago

simoncozens commented 2 years ago

The idea behind setting minimal=True on the builder was to only build the things we need to compile a font rather than to preserve everything. But I think the idea of minimize_glyphs_diffs was to preserve everything. So it feels like these two options are precise opposites: either you want to keep all Glyphs metadata in your UFO, or you just want to export the things you need for building.

Presumably it would not make sense to have both of these options set to True. Shall we get rid of one of them?

anthrotype commented 2 years ago

yeah they kind of are mutually opposite, doesn't make sense to have them both True, they are also confusingly named very similarly. minimal=False should really mean minimize_glyphs_diffs=True, as in "maximal glyphsLib junk"

simoncozens commented 2 years ago

OK. I suggest we keep both kwargs options for compatibility, and call the object property allow_roundtripping. (This is because we more often want to do more things when we want to support roundtripping, so we want a positive test; ie lots of if self.allow_roundtripping: ... instead of if not self.minimal: ...)

I'm still mid-refactor, so I'll add it there.

anthrotype commented 2 years ago

sounds good, thanks. Maybe set the old params to None, and issue a deprecation warning if not None, suggesting to use the new param. And we must ensure the default behavior (no parameter passed explicitly) stays the same as now

schriftgestalt commented 2 months ago

I find the "minimal" flag quite confusing. Does it mean minimal changes when round-tripping or only write the bare minimum needed (for what?)? Adding another flag doesn't help much with the confusion.

We have two distinct use cases for glyphsLib: 1) in fontmake to generate a binary font from a .glyphs file 2) converting between .glyphs and .designspace with the intend to convert in back to the original format because you like to use the other format in a tool that only supports one of them.

So I would propose a flag like "roundTrip". It set, it would try to facilitate the second use case as much as possible. If not set, it would only write out stuff needed for fontMake > .otf. In case of #1026, it would skip those disabled features altogether.

anthrotype commented 2 months ago

yeah that's exactly what Simon proposed above. the problem is also making sure the old options don't suddenly stop working but we give some deprecation warnings and time for clients to update

schriftgestalt commented 2 months ago

as far as I understand, "minimal" is supposed to be the opposite of "roundTrip". So that should fall back safely enough?

anthrotype commented 2 months ago

minimal=True means "only export what is required for building an OpenType font with fontmake".

What Simon suggested above sounds sensible to me:

I suggest we keep both kwargs options for compatibility, and call the object property allow_roundtripping.

schriftgestalt commented 2 months ago

Keep the command line option, of course. But rename it in the code. It seems to me, it is not used correctly in all places because of the ambiguity.