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

uvsDict uses list of tuples instead of dict #496

Merged
merged 2 commits into from
Apr 15, 2021

Conversation

moyogo
Copy link
Collaborator

@moyogo moyogo commented Apr 15, 2021

FontTools cmap uses list of tuples instead of a dict for the UVS mapping.

@anthrotype
Copy link
Member

public.unicodeVariationSequences is a dict of dict.. but I suppose that doesn't matter

lgtm

@anthrotype
Copy link
Member

anthrotype commented Apr 15, 2021

wait, why have the tests in #495 not failed?

@moyogo
Copy link
Collaborator Author

moyogo commented Apr 15, 2021

wait, why have the tests in #495 not failed?

Not sure. But I'm hitting another issue when compiling:
https://github.com/fonttools/fonttools/blob/main/Lib/fontTools/ttLib/tables/O_S_2f_2.py#L210-L216

	def updateFirstAndLastCharIndex(self, ttFont):
		if 'cmap' not in ttFont:
			return
		codes = set()
		for table in getattr(ttFont['cmap'], 'tables', []):
			if table.isUnicode():
				codes.update(table.cmap.keys())

The cmap subtable format 14 doesn't have a cmap attribute but isUnicode() returns True so this raises and AttributeError.

@anthrotype
Copy link
Member

Not sure.

probably we're only testing values after built, but before actually compiling the table (in TTFont.save())

The cmap subtable format 14 doesn't have a cmap attribute but isUnicode() returns True so this raises and AttributeError.

oh, then it might need special handling. Could you PR that in fonttools?

@moyogo
Copy link
Collaborator Author

moyogo commented Apr 15, 2021

OK. The UVS test has been updated to crash if cmap.compile() fails. TIL the spec wants them sorted.

The keys are also hex strings instead of ints, so now that works properly.

For the cmap attribute, fontTools just adds a self.cmap = {} for format 14, so ufo2ft does the same.

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.

thanks

@moyogo moyogo merged commit fe565c8 into googlefonts:main Apr 15, 2021
@moyogo moyogo deleted the uvs-lib-key branch April 16, 2021 11:18
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