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

Fallback code for openTypeOS2UnicodeRanges and #254

Merged
merged 2 commits into from
May 23, 2018
Merged

Fallback code for openTypeOS2UnicodeRanges and #254

merged 2 commits into from
May 23, 2018

Conversation

khaledhosny
Copy link
Collaborator

I don’t have the exact details, but fonts that don’t set Unicode range and code pages fail on Windows. With this code fonts built with fontmake without setting openTypeOS2UnicodeRanges and openTypeOS2UnicodeRanges work on Windows.

@anthrotype
Copy link
Member

Thanks Khaled! I'll review this tomorrow.

Just one thing. You know that the fonttools' O_S_2f_2 module also some similar code for the unicode ranges (not the codepage ranges)
https://github.com/fonttools/fonttools/blob/master/Lib/fontTools/ttLib/tables/O_S_2f_2.py#L251-L516

might be worth merging with your own work to avoid duplication.

Setting some Unicode range seems to be required by some applications on
Windows. I don’t have the exact details, but fonts that don’t set
Unicode range and code pages fail on Windows. People who don’t like the
fallaback, can always set openTypeOS2UnicodeRanges explicitly.

This should have been in fontInfoData.py, but there is no way to access
the ttFont there.
@khaledhosny
Copy link
Collaborator Author

Dropped my Unicode ranges implementation and used fontTools’, they seem to be equivalent.

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Khaled, this looks good to me.
So you basically chose a few "key" character for each codepage range and when they are present, set the bit accordingly. How did you come up with them?
It would be nice if these were documented.

def _calcCodepageRanges(self):
codepageRanges = set()

unicodes = [i for i in self.unicodeToGlyphNameMapping.keys() if i is not None]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the keys in that mapping are never None, you don't need to check if i is not None. If we do checks like that elsewhere, we need to change it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I copied that from somewhere else in the same file.

hasCP866 = 0
hasCP869 = 0
for code in unicodes:
if code == 0x00DE and hasAscii:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all this magic constant hex digits, would be nice to have comments showing what characters they refer to.
maybe you could use byteord('Þ') (defined in fontTools.misc.py23) so it's clear what these characters are.

if hasCP708 and hasLineart:
codepageRanges.add(61) # Arabic; ASMO 708

return list(codepageRanges)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can stay a set. the intListToNum only checks if some integer is contained in that sequence

@anthrotype
Copy link
Member

I think this would also need some accompanying test cases, especially since there're so many if branches.

@khaledhosny
Copy link
Collaborator Author

The code pages code is basically a translation of the code from FontForge (as mentioned in the commit message), I don’t what is the rationale behind it, but it seems to work. Alternatively, we can just do the same as Unicode ranges and if any character of the code page is present set the corresponding bit, but I don’t know if this will work.

@anthrotype
Copy link
Member

a translation of the code from FontForge (as mentioned in the commit message)

ok, I missed that.

Direct translation of FontForge implementation, hopefully it is time
tested…
@anthrotype
Copy link
Member

I see you wrote the unicode literals in the code directly instead of their hex codes, that's good. However, I don't understand how come the tests on Python 2.7 did not fail with SyntaxError: Non-ASCII character ..., given that you didn't add the # -*- coding: utf-8 -*- declaration at the top... 🤔

@anthrotype anthrotype merged commit 4c2a4a1 into googlefonts:master May 23, 2018
@anthrotype
Copy link
Member

gonna fix it myself, thanks

@khaledhosny khaledhosny deleted the calc-uniranges-and-codepages branch May 23, 2018 19:09
@khaledhosny
Copy link
Collaborator Author

Thanks Cosimo!

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.

2 participants