-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
type1font.py fixes and test case #4522
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
Conversation
Slanting and extending had been broken
@@ -309,7 +309,7 @@ def suppress(tokens): | |||
|
|||
while True: | |||
token, value = next(tokens) | |||
if token == 'name' and value in table: | |||
if token is cls._name and value in table: |
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.
Why use is
test instead of ==
? That seems to be betting heavily on some cpython internals working as expected.
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 introduced these sentinel objects to replace string comparisons, which were hard to get right across Python versions. The idea would be to use them like None. Perhaps this needs better comments, or one of the other enum patterns.
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.
Ah, sorry 🐑 makes sense now.
Was this the stuff that was a long time broken do to token
no longer being a string?
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.
Yes, the confusion between strings and bytes was exactly the problem.
What is the license on cmr10? If we are going to bundle a font, might as well include in in the main code so it is available to users? |
I chose cmr10 because it’s already included in lib/matplotlib/mpl-data/fonts/ttf, only in a different format. I think LICENSE/LICENSE_BAKOMA is supposed to cover this, but comments in the font file itself attribute the font to the AMS and mention the SIL license. We should check what exact fonts we do distribute and what their license terms are. We don’t have full Type-1 font support, just this bare-bones parser to allow embedding TeX fonts in pdf files. The user’s TeX installation should provide all the Type-1 fonts needed, and this file is just test data. |
sounds reasonable to me. |
other than my concern about |
I started improving the tests and discovered an off-by-one error, so don't merge this quite yet. |
Add a test for changing the font name (this wasn't working before)
It looks like this broke multi-page tiffs: https://travis-ci.org/matplotlib/matplotlib/jobs/66786896#L5834 |
@@ -136,17 +135,16 @@ def _split(self, data): | |||
# but if we read a pfa file, this part is already in hex, and | |||
# I am not quite sure if even the pfb format guarantees that | |||
# it will be in binary). | |||
binary = b''.join([unichr(int(data[i:i + 2], 16)).encode('latin-1') | |||
for i in range(len1, idx, 2)]) | |||
binary = binascii.unhexlify(data[len1:idx+1]) |
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.
Something is going wrong in this line
FIX: type1font.py and test case
This started from reducing the Python 2 vs 3 special-casing in type1font as an alternative to #4472, then I added a test case using a Type-1 version of cmr10 as data and realized that the font transformations had been broken for quite a while.
Fixes #4470.