unified-font-object / ufoNormalizer

A tool that will normalize the XML and other data inside of a UFO.
Other
51 stars 19 forks source link

Be more permissive for horizontal & vertical guidelines #50

Closed belluzj closed 5 years ago

belluzj commented 5 years ago

This is both a contribution and a question: we recently came upon horizontal and vertical guidelines being stored in UFO by some version of Robofont 3 (not the latest build) as:

  <guideline x="120" y="0"/>
  <guideline x="860" y="0"/>
  <guideline x="0" y="2920"/>

Currently, ufonormalizer considers those invalid and removes them, but they make sense as vertical or horizontal and the spec doesn't explicitly forbid them (that's how I read it).

This patch accepts those guidelines and normalizes them by removing the "0" coordinate. But is it a good idea? Or is it more a matter of clarifying slightly the spec, and a fluke in Robofont? (I couldn't reproduce the weird guidelines with the latest version).

@moyogo

anthrotype commented 5 years ago

Do you know if this still happens in Robofont 3, or if it was fixed?

It's true that the spec only says this (note the use of negation)

If x or y are not specified, angle must not be specified

and then later

If y and angle are omitted, the element represents a vertical guideline. If x and angle are omitted, the element represents a horizontal guideline.

The inline comment in the code here says

if x and y are specified, angle must be specified

but that is not necessarily implied from the first sentence from the spec quoted above.

I'm leaning towards accepting this patch. however, if @typesupply wrote this validation code in the ufonormalizer, it was the (implicit) meaning he wanted to convey as the spec author.

In that case, it might be worth making it more explicit in the wording of the spec.

typesupply commented 5 years ago

The normalizer code is what I intended with the spec, but I agree that the spec is not super clear. I recommend that this patch be approved and a clarifying change to the spec. In this normalizer code, a note that says, "# the spec was ambiguous about y=0 or x=0, so don't raise an error here" (or something like that) would be a good breadcrumb for later reference.

belluzj commented 5 years ago

I added the note # the spec was ambiguous about y=0 or x=0, so don't raise an error here

anthrotype commented 5 years ago

unsurprisingly, there is code very similar to the one in ufonormalizer in the fontTools.ufoLib.validators module (from standalone ufoLib):

https://github.com/fonttools/fonttools/blob/2f2ccdc2c691f3ca49715a2c9f458871262226dc/Lib/fontTools/ufoLib/validators.py#L544-L553

I guess that Robofont build had turned off ufoLib validation, and ended up writing an invalid guideline.

belluzj commented 5 years ago

Honestly I wouldn't draw any conclusions about Robofont from this, as I don't know which version was used.

typemytype commented 5 years ago

in RF you can turn on/off validators by with the default key: ufoLibWriteValidate and ufoLibReadValidate (both True as default)