Skip to content
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

type1font.py fixes and test case #4522

Merged
merged 10 commits into from Jul 3, 2015
Merged

Conversation

jkseppan
Copy link
Member

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.

@@ -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:
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

@tacaswell
Copy link
Member

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?

@jkseppan
Copy link
Member Author

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.

@tacaswell
Copy link
Member

sounds reasonable to me.

@tacaswell tacaswell added this to the next point release milestone Jun 14, 2015
@tacaswell
Copy link
Member

other than my concern about is rather than == this looks good to me.

@jkseppan
Copy link
Member Author

I started improving the tests and discovered an off-by-one error, so don't merge this quite yet.

@tacaswell
Copy link
Member

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])
Copy link
Member

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

tacaswell added a commit that referenced this pull request Jul 3, 2015
FIX: type1font.py and test case
@tacaswell tacaswell merged commit ae5e903 into matplotlib:master Jul 3, 2015
@jkseppan jkseppan deleted the type1font branch July 6, 2015 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants