Merge CFF rebased #2447
Merge CFF rebased #2447
Conversation
|
I’m not getting duplicate |
|
LGTM. Thanks Khaled. |
khaledhosny
force-pushed
the
merge-cff-rebased
branch
3 times, most recently
from
b9e0b7f
to
fa0b30f
Compare
Lib/fontTools/merge.py
Outdated
| # Rename CFF CharStrings to match the new glyphOrder. | ||
| # Using cffTable before reloading the fonts, because reasons. |
There was a problem hiding this comment.
Using cffTable before reloading the fonts, because reasons.
a bit cryptic, perhaps intentionally so. But as far as I can tell, you are actually cffTable after reloading, so I'm even more confused by the comment.
There was a problem hiding this comment.
ah I now see. You are using the cffTable from the original fonts, before they have been reloaded. Ok, then why?
There was a problem hiding this comment.
Yes. I don’t fully understand what is going on, but if we don’t do this then duplicated glyphs gets messed up (they seem to be using the outlines from the first font) in the output font. This seems to be related to the font reloading step, if we stop doing this then all this becomes moot. My testing does not show any other difference with or without reloading.
|
@khaledhosny are you planning to add some minimal tests as well? |
I want to add tests, but merge module does not have any integration tests and I’m not sure unit tests will be very helpful here. If I can figure an easy way to add some integration tests, I’ll do that. |
khaledhosny
force-pushed
the
merge-cff-rebased
branch
from
fa0b30f
to
f7b462e
Compare
This reverts commit 6deeb70.
This way we can use it from both subset and merge modules.
The font is opened later, so use that.
I don’t know why this code was there, but my tests make no difference with or without it.
khaledhosny
force-pushed
the
merge-cff-rebased
branch
from
f7b462e
to
753a841
Compare
khaledhosny
force-pushed
the
merge-cff-rebased
branch
from
dc6db90
to
d7c3a96
Compare
We are not doing anything about subroutines (and there shouldn’t be any since the tables are desubroutinized), so this code is just making it look noisy.
khaledhosny
force-pushed
the
merge-cff-rebased
branch
from
f9095b1
to
1fa579f
Compare
|
Thanks Jeremie and Khaled! |
Rebased #1319, but had to remove
.notdefnon-duplication since it conflicts with the change made in #1729 and needs bigger change to do properly.TODO: