filters/featureWriters: use '...' as placeholder for loading from UFO lib #604
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
… lib Currently the `filters` and `featureWriters` parameters override the ones defined in the UFO lib.plist, and it's impossible to extend instead of replace these, e.g. inserting some additional filter or writer either before or after. This PR allows to pass a special value `...` (the actual ``ellipsis`` singleton, not the str literal '...') in both these lists, which acts as a placeholder to signal one wants to extend the list of predefined filters/writers. E.g.: filters=[..., DecomposeTransformedComponentsFilter(pre=True)] The ellipsis is replaced by the custom filters defined in the UFO lib (if any), and the additional one is appended to the list. Only one ellipsis is allowed in the list. Fontmake will also be updated to allow passing --filter='...' or --feature-writer='...' CLI options to use this feature. Part of fixing googlefonts/fontmake#872
only for self-documentation as we don't run static typechecker. there's a lot of duck-typing in ufo2ft.. One day maybe we'll define protocols for Font, Filter, etc.
latest flake8 suddenly started complaining about unrelated stuff like: B020 Found for loop that reassigns the iterable it is iterating with each iterable value. I just fixed these even if they don't belong here, just to make CI go green. Also apparently flake8 cares about undefined names inside `# type: ` comment, I guess that's a feature. Let's see if I can fool it. https://github.com/googlefonts/ufo2ft/runs/5871883161?check_suite_focus=true#step:5:35
anthrotype
commented
Apr 7, 2022
| for st in st.statements: | ||
| for s in feaLib.statements: | ||
| if isinstance(s, ast.TableBlock) and s.name == "GDEF": | ||
| for st in s.statements: |
There was a problem hiding this comment.
this change is unrelated, it was just to appease flake8 complaining about loop variable reassigning the iterable it's iterating on
anthrotype
commented
Apr 7, 2022
| groups = defaultdict(list) | ||
| for node, color in color.items(): | ||
| for node, color in colors.items(): |
There was a problem hiding this comment.
same here, just to make flake8 happy
|
/cc @m4rc1e |
anthrotype
added a commit
to googlefonts/fontmake
that referenced
this pull request
Apr 7, 2022
Fixes #872 Depends on googlefonts/ufo2ft#604 to be merged/released first
anthrotype
added a commit
to googlefonts/fontmake
that referenced
this pull request
Apr 7, 2022
Fixes #872 Depends on googlefonts/ufo2ft#604 to be merged/released first
m4rc1e
reviewed
Apr 8, 2022
There was a problem hiding this comment.
I've just had a play with this pr and googlefonts/fontmake#874 (review) in gftools builder and it is working nicely.
Before:
INFO:ufo2ft.filters.base:Running FlattenComponentsFilter on MavenPro-Regular
INFO:ufo2ft.filters.flattenComponents:Flattened composite glyphs: 343
INFO:ufo2ft.filters.base:Running DecomposeTransformedComponentsFilter on MavenPro-Regular
After:
INFO:ufo2ft:Pre-processing glyphs
INFO:ufo2ft.filters.base:Running EraseOpenCornersFilter on MavenPro-Regular
INFO:ufo2ft.filters.base:Running FlattenComponentsFilter on MavenPro-Regular
INFO:ufo2ft.filters.flattenComponents:Flattened composite glyphs: 343
INFO:ufo2ft.filters.base:Running DecomposeTransformedComponentsFilter on MavenPro-Regular
We can see the additional filters are now being executed.
AFAIK I'm happy but I'm not really a maintainer of this repo so if someone else would like to give it the all good, feel free.
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.
Currently the
filtersandfeatureWritersparameters always override the ones defined in the UFO lib.plist, and it's impossible to extend instead of replace these, e.g. inserting some additional filter or writer either before or after.This PR allows to pass a special value
...(the actualEllipsissingleton, not the str literal '...') in both these lists, which acts as a placeholder to signal one wants to extend the list of predefined filters/writers.E.g.:
The ellipsis is replaced by the custom filters defined in the UFO lib (if any), and the additional one is appended to the list.
Only one ellipsis is allowed in the list.
Same for the featureWriters.
Fontmake will also be updated to allow passing --filter='...' or --feature-writer='...' CLI options to use this feature.
Part of fixing googlefonts/fontmake#872