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.

Sign up for GitHub

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

Jump to bottom

Allow UTF-8 input for name entries #1133

Merged
merged 5 commits into from
May 20, 2020
Merged

Allow UTF-8 input for name entries #1133

merged 5 commits into from
May 20, 2020

Conversation

khaledhosny
Copy link
Collaborator

Fixes #165.

UTF-8 comments seem to work fine, I’m not aware for other places where UTF-8 input is rejected.

@khaledhosny khaledhosny mentioned this pull request May 17, 2020
Closed
@khaledhosny
Copy link
Collaborator Author

Several failures seem to be caused by fonttools/fonttools#1964 which is unrelated to this PR, and CircleCI linux-python3 is failing to lint setup.py which is also unrelated to this PR.

The most unfortunate PR I ever made, catching two unrelated CI failures 😄.

@khaledhosny
[ANTLR] Update generated file
31baee9 
Keep the spelling written by ANTLR, no need to spell-check it.
@khaledhosny
[makeotf] Allow UTF-8 input for Windows names
d8e3f84 
Allow direct UTF-8 input for Windows name, Mac name continue to require
escaped 8-bit character codes.
josh-hadley
josh-hadley requested changes May 19, 2020
View reviewed changes
Copy link
Collaborator

@josh-hadley josh-hadley left a comment

Choose a reason for hiding this comment

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

@khaledhosny can you please bump MAKEOTF_VERSION and HOT_VERSION?

@josh-hadley josh-hadley requested a review from punchcutter May 19, 2020 23:18
@josh-hadley
Copy link
Collaborator

@khaledhosny can you please bump MAKEOTF_VERSION and HOT_VERSION?

Also __version__ attribute (version & date) in makeotf.py; even though the Python code didn't change, just so we reflect a change in the overall makeotf toolchain.

@khaledhosny
[makeotf*] update versions
69b85b7 
- update HOT_VERSION to 0x10072 ("1.0.114")
- update makeotfexe version to "2.5.65599"
- update makeotf.py version to "2.7.11 May 20 2020"
punchcutter
punchcutter approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@punchcutter punchcutter left a comment

Choose a reason for hiding this comment

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

LGTM

punchcutter
punchcutter previously approved these changes May 20, 2020
View reviewed changes
@josh-hadley
Copy link
Collaborator

Something very strange is going on with dependencies. The GHA Test suites, which explicitly pip install -r requirements.txt, work fine. But GHA Coverage and other CIs fail with bizarre failures on code that hasn't been touched in this PR. I experienced something like this locally earlier. Need to investigate, something in the packaging/setup scheme must have changed.

@punchcutter
Copy link
Contributor

Tests are failing because fontTools changed SFNTReader and removed a required import of SimpleNamespace.

@anthrotype
Copy link
Member

It should be fixed in fonttools 4.10.2, apologies for inconvenience

@josh-hadley
Copy link
Collaborator

@khaledhosny this is indeed an unfortunate PR 😸 Looks like the fontTools 4.10.2 update has another change that causes one of our tests to fail still (unrelated to your code). I am going to get that fixed so this PR is clean (tests passing) and get this merged ASAP before something else happens!

@anthrotype
Copy link
Member

Looks like the fontTools 4.10.2 update has another change that causes one of our tests to fail still

As for the above mentioned change with findMultilingualName this time it should be a feature rather than a bug.

@anthrotype
Copy link
Member

For the records, see fonttools/fonttools#1921 (comment)

@josh-hadley
Copy link
Collaborator

As for the above mentioned change with findMultilingualName this time it should be a feature rather than a bug.

Understood. That's what I got from Just's log message, and is why I'm updating our test code (actually it looks like we have the same test case or at least the same name strings, because the changes to the fontTools test case are exactly the failures we have currently).

@josh-hadley
[tests] update GSUBVar.ttx
542395a 
fontTools 4.10.1 (.2) changed/fixed fonttools/fonttools#1921 (comment) so need to update our test case to match that behavior.
@josh-hadley josh-hadley self-requested a review May 20, 2020 16:27
josh-hadley
josh-hadley previously approved these changes May 20, 2020
View reviewed changes
@josh-hadley
[requirements] update requirements.txt
29c72a0 
Bump fontTools to 4.10.2 to ensure that version gets installed in the various install scenarios.
@josh-hadley josh-hadley merged commit 7498a54 into develop May 20, 2020
@josh-hadley josh-hadley deleted the name-utf8 branch May 20, 2020 19:15
@josh-hadley
Copy link
Collaborator

Thanks for your work on this @khaledhosny and sorry it was such a pain to get finished! Smoother ride next time 🤞

@khaledhosny
Copy link
Collaborator Author

Thanks @josh-hadley. It wasn’t that painful, it was actually a bit funny 😄

@anthrotype anthrotype mentioned this pull request Jun 18, 2020
Closed
@frankrolf frankrolf mentioned this pull request Dec 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josh-hadley josh-hadley josh-hadley approved these changes

@punchcutter punchcutter punchcutter left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[spec] Specify Feature File encoding as UTF-8
4 participants
@khaledhosny @josh-hadley @punchcutter @anthrotype