-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updates to CID-keyed UFO handling #1628
Conversation
For reference we previously had an error |
I didn't run tests or add any new yet. Just want to get this out there first so we can look at it. |
Related to the absfont_dump changes I made I noticed that dumping a CID-keyed UFO doesn't report CIDCount because it's not set in uforead until after we have read the glyphs in Update: we should be setting it in |
With the last commit we get output like this from a CID-keyed UFO that also has real glyph names. By default we write UFOs with glyph names like
|
I keep forgetting to put |
I made a few changes to writing the lib.plist. The CIDMap entry keys need to be ordered alphabetically or we fail in the uforead bsearch when checking glyphs. But we also want the |
…warning because the dup glif is discarded earlier when calling findGLIFRecByName(h, glyphName). Previously, this was searching by fileName rather than glyphName.
5c064ea
to
a526a83
Compare
…vious memory. More work needed for this in the future.
ef164a5
to
17e6879
Compare
…e have different inputs and different parsing functions that aren't all consistent yet. Better fixes in the future.
FPRINTF_S(h->fp, "## glyph[tag] {cid,iFD"); | ||
else | ||
/* UFO can store names even when CID-keyed */ | ||
if (top->sup.srcFontType == 7) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there, or could there be, some constant defined to use instead of 7
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duh, I have no idea why I put 7. The enum is abfSrcFontTypeUFOCID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with the enum string instead of 7.
else | ||
sprintf(gname, "cid%hu", info->cid); | ||
if (info->gname.ptr != NULL) { | ||
strcpy(gname, info->gname.ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using strncpy seems particularly wise here given the fixed buffer size and the potentially unverified length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -340,6 +340,86 @@ int ufwBegFont(ufwCtx h, long flags, char *glyphLayerDir) { | |||
return ufwSuccess; | |||
} | |||
|
|||
static void orderNameKeyedGlyphs(ufwCtx h) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we're using these n^2 sorts for stability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think all this sorting is lovely, but cffwrite and t1write have ordering by CID or Name so I half copied what's done in those so that we can correctly sort here. The reason it came up was because we were writing the CIDMap in CID order which then made uforead fail to read it correctly, but more importantly if I open and save the UFO in typical UFO Python tools then the lib.plist was in a completely different. The whole XML dict is sorted by key including CIDMap and we weren't doing that in ufowrite so that's what all this does. Order by name so that we can write the CIDMap correctly and then order by CID so that public.glyphOrder is in the correct CID order. It's kind of ugly. We should really be reading all of this into a libxml2 structure and work from that, but this all works well enough so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking more about the efficiency than presence of the sorting, but I guess the worst case is 2^32 operations, which isn't a deal breaker.
I believe we use a canned quicksort elsewhere, which is why I asked about stability (quicksort isn't stable), but it seems like it's doubtful we'd have duplicate keys anyway (in which case stability isn't relevant).
I dunno, I guess we can do this for the time being and revisit later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
42a9390
to
0d7a5ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skef had already approved & I'm adding my 👍 !
Added
getGlyphByCID
to uforead@kaydeearts this is on top of your kd-fix-glyphname-search branch. We need the same CID functionality as in t1read and cffread so this is one of those things. We should also check the
-decid
flag and make sure we can do that with UFO as well.