Update buildcff2vf #825
Merged
Update buildcff2vf #825
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
miguelsousa
requested review from
josh-hadley
and removed request for
miguelsousa and
cjchapman
readroberts
force-pushed
the
update-buildcff2vf
branch
from
a8ecfcf
to
057dae6
Compare
cjchapman
suggested changes
Jun 27, 2019
cjchapman
previously requested changes
Jun 27, 2019
|
Also, the flake8 report has a few things from above and a few new things: |
readroberts
force-pushed
the
update-buildcff2vf
branch
from
057dae6
to
3b8b790
Compare
|
@cjchapman @miguelsousa I fixed the Codacy and flake8 issues. I see the first two builds failed, but it looks like it is an issue with the build environment rather than the code. |
…tion Fixing outline compatibility can fail because the code assumes that the first font in the master_fonts argument to do_compatibility() is the default. Fix by passing in the default font index in the font list into do_compatibilization(), so that the region font list can be formed by deleting the actual default font from the font list. Fix same issue in CompatibilityPen.getCharStrings(). Add test case. Fixes #816
|
@readroberts (cc: @miguelsousa) this branch was trying to use the |
cjchapman
force-pushed
the
update-buildcff2vf
branch
from
8deb2f1
to
e229453
Compare
|
@readroberts (cc: @miguelsousa) Correction: I've rebased this branch off the |
cjchapman
dismissed
their stale review
I'm dismissing my earlier review because the changes I requested have been made, and now I've touched this branch to fix the CI issues, so I think someone else should do the final approval of this branch.
A side effect of using the fontTools subsetter module to implement glyph subsetting with the -i option is that features are removed if they are not in the subsetter default features set. Since buildcff2vf is not intended to filter out features, I fixed this by setting the subsetter module's layout_features option field to '*', which causes it to pass through all features. Add test case. Fixes #817
When a point is a curve in one master source font and a line in another, the code will convert the line to a flat curve. However, the code in the class CompatibilityPen assumes that the point coordinates are absolute. A few months ago, I changed the parent class CFF2CharStringMergePen (in fontTools) from using absolute to relative coordinates, and forgot about this dependency. Also fixed the test cases that use the compatibilization option.
miguelsousa
force-pushed
the
update-buildcff2vf
branch
from
e229453
to
da615db
Compare
|
I've reworked the commits. Should be ready for @josh-hadley to review. |
josh-hadley
approved these changes
Jun 28, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Address issue 816: wrong set of masters referenced when fixing compatibility issues, if default master is not the first font the master font list.
Address issue 817: fix unintentional filtering of features when using the subsetting option.