[uforead] Refactor setFontDictKey into separate file functions

[kaydeearts]

Description

Refactored uforead.c, mainly the setFontDictKey so that it isn't one big function but rather called based on the file being parsed. This makes the CJK enhancements work easier. I had stashed these changes to keep the .glif parsing as its own PR, so this is a follow-up to that.

I have not added any new tests as this is only refactoring, and all tests added within the libxml2-related PRs are relevant to these sections of the code.

Checklist:

• I have followed the Contribution Guidelines
• I have added test code and data to prove that my code functions correctly
• I have verified that new and existing tests pass locally with my changes
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation

[skef]

Won't this leak the string passed to atol()?

[skef]

Should we add an else assert(false); before this line, to track that keyValueValid() should correspond to this list of strings? (And is that right? If keyValueValid() also has keys from other contexts, should we do else return false;?)

[kaydeearts]

I intended the bool return values mostly for the parser to know if there's any missing items (keys or values) so it wouldn't iterate to the next item assuming it was a new key. Unknown key/value pairs are parseable (simply ignored).

However, I agree with you that we should return false here for when we add in the warning messages that'll let the users know which keys weren't parsed (with the verbosity flag), so I did add the else return false; in for this and setFontInfoKey.

Now, once it returns the false from setXMLFileKey to its caller, it iterates to the cur->next, which is the keyValue (that wasn't set because it was unknown), and then it realizes it's not a key and skips it again to get to the next key/value pair. So returning false for unknown key/value still functions as expected for this addition.

keyValueValid only checks if the key value is parsable, so I wouldn't add the assert. I renamed keyValueValid to keyValueParseable to clarify its purpose.

Thanks for this catch!

[skef]

Moving forward with this code base we might want to switch to strtol() and check for errors with all of the strtoX() functions. (If you pass a char ** as the second argument they will set that to the pointer passed in if it couldn't parse a number.)

However, we don't do that now so I don't see a reason to hold up this change for that kind of improvement.

[skef]

What guarantees that the unitsPerEm key will be before the FontMatrix key in a UFO dictionary? If unitsPerEm happens to come later, won't we overwrite the matrix with the identity matrix?

Could we just initialize the matrix to the identity at some earlier point and not do that in setFontInfoUnitsPerEm()? That seems like the cleanest work-around if it's a possibility.

[kaydeearts]

@skef just fixed this so that before setting values into fontMatrix when parsing UnitsPerEm key, it checks to see if values have previously been set. It does not set values if previously set (by the FontMatrix key, for example) so that these values are not overwritten.

[skef]

Looks good