[tx][libxml2-improved] Move FDArray from fontinfo.plist to lib.plist

[kaydeearts]

Description

Fixes #1542 and #1539 Does what #1380 intended, but rewritten for the new libxml2 improvements + other considerations. 🚀

To-Dos:

• [x] ufowrite: write FDArray in lib.plist instead of fontinfo.plist
• [x] uforead: parse FDArray from lib.plist as well as fontinfo.plist (to support old & new files)
• [x] .glif files: current solution is that iFD & CID is held in a CIDMap in lib.plist. Is this what we want?
• [x] update tests to reflect the changes
• [x] Fix test failures
• [x] Remove support for FDArray in fontinfo.plist
• [x] Edit CIDMap to only have cids
• [x] Move iFD information into a group in groups.plist
• [x] Add tests for groups.plist parsing
• [x] Add tests for CIDMap in lib.plist parsing
• [ ] Fix Windows heap corruption fails

Checklist:

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

[kaydeearts]

Question (mainly for @skef ): Last year, Zachary and I had structured this so that the cid and iFD mappings would be moved from each .glif file to a CIDMap in lib.plist.

With you more recently we have discussed moving the iFD mappings to groups.plist in #1539 . I'm wondering if we should keep the CIDMap as well as have the iFD group, or move the CIDMap to groups.plist, or have only one of them?

For now, I kept the CIDMap in lib.plist in this PR as I mainly focused on refactoring my PR from last year. I can now make changes as needed since tests pass.

[skef]

The CIDmap isn't a group so it can't/shouldn't go in groups.plist.

I don't see a problem with separating the two (keeping the map in lib.plist and having the group in groups.plist). Some LGC fonts will have FD mappings but no CIDmap.

[kaydeearts]

Question about the key naming: Shall we discuss if we want keys to have "com.adobe.type" or not? Currently, the code should support keys that do and don't have it (since what we currently have in the FDK are the keys without the com.adobe.type in them, and I didn't want to break older files.)

[skef]

Was the previous code ever really in production?

[kaydeearts]

Was the previous code ever really in production?

@skef No, it only ever stayed in PR #1380 and we never ended up merging. Then the plan became to rewrite it after the libxml2 updates. However, Zachary used that branch to release Source Han Serif last year and still uses it for font development.

[punchcutter]

I already started updating to use the new way. No reason at all to keep the old way. iFD should go into groups.plist and CIDs in the CIDMap in lib.plist without all the com.adobe stuff because that's irrelevant. The top level com.adobe.postscriptCIDMap or whatever that name is (writing on my phone) has com.adobe and that should just be a dict with names mapped to CID. The only reason it had com.adobe or even "CID" was because I originally made it a dict with CID and iFD. Since we are moving iFD nothing needs to be named iFD or CID.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging fbfc499a5b147eaedd1e905cfa4319002100dda6 into 6b6487fe9de1ce0a6ed16f09f98f5ceab527468b - view on LGTM.com

new alerts:

• 1 for Lossy pointer cast

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine :gear: that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[punchcutter]

I tried the latest kd-fdarray-libplist-libxml2-2 and found two problems:

1. Running tx fails on a UFO if lib.plist has public.glyphOrder after the CID information. public.glyphOrder needs to be defined first in the lib.plist, but that doesn't make any sense. I thought the plan was to read in everything in whatever order since nothing in any of these XMLs is ordered. We can't expect keys to be in any particular order.
2. All keys with com.adobe.type.cid should be changed to com.adobe.type. That's my fault for putting cid in there in the first place, but it should be removed.

[punchcutter]

If there is a mismatch between the number of FDArrays defined in lib.plist and the number of FDSelect groups in groups.plist we should error out. Right now if we drop an FDArray from com.adobe.type.postscriptFDArray then we still get an empty one created with no warning. We want to be sure these are actually defined so we should fail like we do if a glyph isn't assigned to a dict in groups.plist.

[punchcutter]

Kind of related to my last comment running tx alone on a UFO outputs an extra empty FontDict. Running tx -t1 and outputting to a file is fine and the file doesn't have an extra dict. More of a cosmetic thing I think.

## FontDict[18]
## Private
BlueScale           0
BlueShift           0
BlueFuzz            0
StdHW               0
StdVW               0
ExpansionFactor     0

[kaydeearts]

If there is a mismatch between the number of FDArrays defined in lib.plist and the number of FDSelect groups in groups.plist we should error out. Right now if we drop an FDArray from com.adobe.type.postscriptFDArray then we still get an empty one created with no warning. We want to be sure these are actually defined so we should fail like we do if a glyph isn't assigned to a dict in groups.plist.

My latest changes now implement this. If there aren't enough FDArrays as FDSelect groups defined, it will error out. I was also going to check to see if the assigned FDArray's name matches the assigned FDSelect group, but I remembered that we had left that naming portion as optional/just for the user, so avoided doing that. Let me know if that's something we do want!

[kaydeearts]

Kind of related to my last comment running tx alone on a UFO outputs an extra empty FontDict. Running tx -t1 and outputting to a file is fine and the file doesn't have an extra dict. More of a cosmetic thing I think.

## FontDict[18]
## Private
BlueScale           0
BlueShift           0
BlueFuzz            0
StdHW               0
StdVW               0
ExpansionFactor     0

Directly out of the most recent changes, parsing a UFO with mismatched FDArrays/FDSelect groups with or without the -t1 arg will now error out.

[punchcutter]

I tried the latest and if I remove com.adobe.type.postscriptCIDMap it still reads as CID-keyed with all glyphs set to CID 0.

[skef]

I could be wrong about the UFO .plist preservation question. I suggest consulting with Josh and Zachary about that.

[punchcutter]

The one big thing that I see right now is that going tx -ufo -o test.ufo test.pfa doesn't create a groups.plist so any further tx'ing on the UFO fails

tx: (ufr) Warning: FDArraySelect not defined for cid-keyed font
tx: (ufr) glyph 'cid00000' missing FDArray index within <lib> dict

We could merge this and then address that. That second message is also unclear because lib.plist does have FDArray, but what's actually missing is groups.plist with an FDArraySelect containing that glyph.