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

Remove redundant name table records #3185

Merged
merged 7 commits into from Jul 12, 2023
Merged

Conversation

m4rc1e
Copy link
Contributor

@m4rc1e m4rc1e commented Jun 26, 2023

I've needed a way to remove redundant name table records for many years so I thought I'd just write it. I'm using Behdad's visitor pattern to collect all the name records which are being used by the fvar, STAT, CPAL etc.

You can get redundant name records when you replace a font's STAT table using the builder's buildSTAT function or when hot fixing with other scripts etc.

My plan is to make removeUnusedNameRecords a method of table__n_a_m_e. I just wanted to open this pr to get feedback before I progress too far. No worries if you don't want this pr either.

So far I've covered every table in ttLib/tables/otData, STAT, fvar and CPAL. Please let me know if I'm missing any.

Copy link
Member

@behdad behdad left a comment

Choose a reason for hiding this comment

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

Looks good!

def __init__(self):
self.seen = set()

def removeUnusedNameRecords(self, font):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove this from the visitor object, move it somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking. I've moved and converted the function so it's a static method of the name table. Imo it feels like belongs here. Happy to drop the static method and just have it as a plain func instead. I've moved it away from the Snippets dir so it can be imported into other libs and tools.

Happy to add tests if you're ok with the above. Still not too sure if I've found all the places in a font which can have a nameID though.

@behdad
Copy link
Member

behdad commented Jun 27, 2023

Thanks. We should see if we can use this, or at least compare notes with, the subsetter. That one looks very incomplete to me.

https://github.com/fonttools/fonttools/blob/main/Lib/fontTools/subset/__init__.py#L2917

@behdad
Copy link
Member

behdad commented Jun 27, 2023

@m4rc1e
[Use NameRecordVisitor in subsetter](/fonttools/fonttools/pull/3185/commits/edf8891fba54015c5d37a6397b7b95f14dfe5662)

Slick! I just wonder if we need to add a table order dependency to process name last.

@m4rc1e
Copy link
Contributor Author

m4rc1e commented Jun 28, 2023

I just wonder if we need to add a table order dependency to process name last.

I'm not too familiar with all of the subsetting code. However, if I skim, I see self._prune_pre_subset and _subset_glyphs methods call _sort_tables. Is it sufficient to just move the "name" table to last pos in _sort_tables? if not, I'll keep digging.

@behdad
Copy link
Member

behdad commented Jun 28, 2023

Thanks. Checking the subset code, I think we need to move the name-table subsetting from prune_pre_subset to prune_post_subset. That might be enough.

There's also some "name" table code in the "CPAL" subsetting method, which should be removed.

I don't remember why "name" table is in _sort_tables. May be possible to remove it now, maybe not.

@behdad
Copy link
Member

behdad commented Jun 28, 2023

There's also some "name" table code in the "CPAL" subsetting method, which should be removed.

Can you please do this as well?

Thanks. I let @anthrotype review the rest.

@behdad behdad requested a review from anthrotype June 28, 2023 16:18
@behdad behdad marked this pull request as ready for review June 28, 2023 16:19
@m4rc1e
Copy link
Contributor Author

m4rc1e commented Jun 28, 2023

There's also some "name" table code in the "CPAL" subsetting method, which should be removed.

Can you please do this as well?

Think I got it all. Thanks for your help Behdad!

Lib/fontTools/ttLib/tables/_n_a_m_e.py Outdated Show resolved Hide resolved
Lib/fontTools/ttLib/tables/_n_a_m_e.py Outdated Show resolved Hide resolved
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.

LGTM, with comments

@anthrotype
Copy link
Member

the snippet doesn't do much besides loading a TTFont, calling remoteUnusedNames method, and saving back; do we actually need it?

@m4rc1e
Copy link
Contributor Author

m4rc1e commented Jul 10, 2023

Thanks Cosimo. I've implemented your feedback and removed the snippet. The comment may be too long for your liking. Lmk and I'll rephrase.

@anthrotype anthrotype merged commit c9d965b into fonttools:main Jul 12, 2023
10 checks passed
@anthrotype anthrotype deleted the clean-nametable branch July 12, 2023 18:12
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

3 participants