Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Remove ttconv and implement Type-42 embedding using fontTools #20866
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
jkseppan
force-pushed
the
remove-ttconv
branch
from
447fd1b
to
a826065
Compare
jkseppan
force-pushed
the
remove-ttconv
branch
from
8c44828
to
2535508
Compare
jkseppan
force-pushed
the
remove-ttconv
branch
from
de16cf0
to
daee4e5
Compare
jkseppan
force-pushed
the
remove-ttconv
branch
2 times, most recently
from
016713a
to
7cea2d4
Compare
QuLogic
reviewed
Dec 24, 2021
| @@ -24,28 +25,59 @@ def get_glyphs_subset(fontfile, characters): | |||
| Subset a TTF font | |||
|
|
|||
| Reads the named fontfile and restricts the font to the characters. | |||
| Returns a serialization of the subset font as file-like object. | |||
| Returns a TTFont object. | |||
There was a problem hiding this comment.
Just remove the line; you have a Returns section.
| if fontfile.endswith('ttc'): | ||
| # fix this once we support loading more fonts from a collection | ||
| # https://github.com/matplotlib/matplotlib/issues/3135#issuecomment-571085541 | ||
| options.font_number = 0 |
There was a problem hiding this comment.
I think we test a ttc in CI already, so it could be used to test this line?
| The Type-42 formatted font | ||
| """ | ||
| version, breakpoints = _version_and_breakpoints(font.get('loca'), fontdata) | ||
| post, name = font['post'], font['name'] |
There was a problem hiding this comment.
Are these tables guaranteed to exist?
Comment on lines
+296
to
+301
| Read the version number of the font and determine sfnts breakpoints. | ||
| When a TrueType font file is written as a Type 42 font, it has to be |
There was a problem hiding this comment.
Suggested change
| Read the version number of the font and determine sfnts breakpoints. | |
| When a TrueType font file is written as a Type 42 font, it has to be | |
| Read the version number of the font and determine sfnts breakpoints. | |
| When a TrueType font file is written as a Type 42 font, it has to be |
Comment on lines
+314
to
+322
| tuple | ||
| ((v1, v2), breakpoints) where v1 is the major version number, | ||
| v2 is the minor version number and breakpoints is a sorted list | ||
| of offsets into fontdata; if loca is not available, just the table | ||
| boundaries |
There was a problem hiding this comment.
For multiple return, you should list the entries separately, like Parameters.
Comment on lines
+336
to
+342
| breakpoints = sorted( | ||
| set(tables.values()) | glyf_breakpoints | {len(fontdata)} | ||
| ) |
There was a problem hiding this comment.
Suggested change
| breakpoints = sorted( | |
| set(tables.values()) | glyf_breakpoints | {len(fontdata)} | |
| ) | |
| breakpoints = sorted({*tables.values(), *glyf_breakpoints, len(fontdata)}) |
Comment on lines
+384
to
+394
| s = StringIO() | ||
| go = font.getGlyphOrder() | ||
| s.write(f'/CharStrings {len(go)} dict dup begin\n') | ||
| for i, name in enumerate(go): | ||
| s.write(f'/{name} {i} def\n') | ||
| s.write('end readonly def') | ||
| return s.getvalue() |
There was a problem hiding this comment.
I don't understand why you need StringIO for this, as there is no file IO necessary.
Suggested change
| s = StringIO() | |
| go = font.getGlyphOrder() | |
| s.write(f'/CharStrings {len(go)} dict dup begin\n') | |
| for i, name in enumerate(go): | |
| s.write(f'/{name} {i} def\n') | |
| s.write('end readonly def') | |
| return s.getvalue() | |
| go = font.getGlyphOrder() | |
| s = f'/CharStrings {len(go)} dict dup begin\n' | |
| for i, name in enumerate(go): | |
| s += f'/{name} {i} def\n' | |
| s += 'end readonly def' | |
| return s |
Comment on lines
+414
to
+432
| b = BytesIO() | ||
| b.write(b'/sfnts[') | ||
| pos = 0 | ||
| while pos < len(fontdata): | ||
| i = bisect.bisect_left(breakpoints, pos + 65534) | ||
| newpos = breakpoints[i-1] | ||
| if newpos <= pos: | ||
| # have to accept a larger string | ||
| newpos = breakpoints[-1] | ||
| b.write(b'<') | ||
| b.write(binascii.hexlify(fontdata[pos:newpos])) | ||
| b.write(b'00>') # need an extra zero byte on every string | ||
| pos = newpos | ||
| b.write(b']def') | ||
| s = b.getvalue().decode('ascii') |
There was a problem hiding this comment.
Also doesn't appear to need BytesIO:
Suggested change
| b = BytesIO() | |
| b.write(b'/sfnts[') | |
| pos = 0 | |
| while pos < len(fontdata): | |
| i = bisect.bisect_left(breakpoints, pos + 65534) | |
| newpos = breakpoints[i-1] | |
| if newpos <= pos: | |
| # have to accept a larger string | |
| newpos = breakpoints[-1] | |
| b.write(b'<') | |
| b.write(binascii.hexlify(fontdata[pos:newpos])) | |
| b.write(b'00>') # need an extra zero byte on every string | |
| pos = newpos | |
| b.write(b']def') | |
| s = b.getvalue().decode('ascii') | |
| s = '/sfnts[' | |
| pos = 0 | |
| while pos < len(fontdata): | |
| i = bisect.bisect_left(breakpoints, pos + 65534) | |
| newpos = breakpoints[i-1] | |
| if newpos <= pos: | |
| # have to accept a larger string | |
| newpos = breakpoints[-1] | |
| s += f'<{fontdata[pos:newpos].hex()}00>' # Always NULL terminate. | |
| pos = newpos | |
| s += ']def' |
Comment on lines
+240
to
+261
| fontfiles = (font_manager.findfont(fp) for fp in fps) | ||
| if len(set(fontfiles)) < 6: |
There was a problem hiding this comment.
Suggested change
| fontfiles = (font_manager.findfont(fp) for fp in fps) | |
| if len(set(fontfiles)) < 6: | |
| fontfiles = {font_manager.findfont(fp) for fp in fps} | |
| if len(fontfiles) < 6: |
|
Wow, I missed that this came in. 💯 on dropping an extension module! |
|
Dropping to draft until the comments and rebase are addressed. Anyone should feel free to move back to active... |
|
Ping @jkseppan? |
oscargus
reviewed
Mar 15, 2022
|
|
||
| Returns | ||
| ------- | ||
| fontTools.ttLib.ttFont.TTFont |
There was a problem hiding this comment.
Long term, it could make sense to link to the documentation? (Although fontTools is not in the intersphinx_mapping yet.)
|
We want to try and do mpl 3.7 feature freeze around Jan 1 so I think it is unlikely we will have the bandwidth to get this rebased and reviewed by then, pushing to 3.8. |
Move the test in lib/matplotlib/tests/test_ttconv.py to lib/matplotlib/tests/test_backend_pdf.py. Actually it has not been a test of ttconv for a while, since type-3 conversion has not been using ttconv.
Split _backend_pdf_ps.get_glyphs_subset into two functions: get_glyphs_subset returns the font object (which needs to be closed after using) and font_as_file serializes this object into a BytesIO file-like object.
Fonttools cannot subset these and drops them, avoid the warning
Also don't end up in an infinite loop if there is a larger gap between breakpoints than we would like.
FT2Font throws RuntimeError for some fonts
Not the subsetted one, since in some cases fontTools.subset does not produce a good name table. Import functions from _backend_pdf_ps to help keep lines shorter.
tacaswell
force-pushed
the
remove-ttconv
branch
from
7cea2d4
to
a1b41fe
Compare
|
Do we have someone who can pick this up? |
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.
PR Summary
ttconv is now only being used for outputting fonts in Type-42 format in PostScript files, and with fontTools this turns out to be quite doable in pure Python.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).