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

Overhaul name checks #3194

Closed
wants to merge 40 commits into from
Closed

Overhaul name checks #3194

wants to merge 40 commits into from

Conversation

m4rc1e
Copy link
Collaborator

@m4rc1e m4rc1e commented Mar 10, 2021

This PR will tidy up our existing name table checks. It also fixes the typographic subfamily name check for VFs with multiple axes.

I've had to move away from using the filenames to determine the family name and subfamily name. Instead, I've used the name table itself. This means I will need to implement a check to ensure that the filenames are related to the name table.

#3191

Still wip, needs much more work.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Mar 10, 2021

@felipesanches before you crucify me, I'll use enums and error codes :-)

@chrissimpkins
Copy link
Member

chrissimpkins commented Mar 10, 2021

IIUC we ignore all name records defined outside of 3, 1, 1033 correct?

Do non-1033 langID have any role beyond locale-specific menu / example text display strings?

@m4rc1e m4rc1e force-pushed the new-names branch 2 times, most recently from e18f75c to 98901f5 Compare March 19, 2021 12:12
@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Mar 23, 2021

@felipesanches Just an fyi. I'm dog fooding this whilst working on an NDA project so my progress is happening slower than expected. I plan on getting this finished in the next fortnight or so. Due to changing the source of truth, we will need to rigorously test this pr.

@felipesanches
Copy link
Collaborator

Thanks for putting some more effort on this, @m4rc1e :-)

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Jul 26, 2021

Thanks and apologies for the delay on this. We should maybe have a hangout once I think it's ready since I'm making a tonne of changes.

@felipesanches
Copy link
Collaborator

@m4rc1e, please let me know when this is ready for a code-review. Then if I get confused about some aspects of it during review, we can possibly have a video call for clarification. Cheers!

@felipesanches
Copy link
Collaborator

In general, though, I prefer to have the clarification in text comments here so that we keep a public record of the rationales behind all our project decisions.

import logging


__all__ = ["GFNames"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be __all__ = ["GFNameData"] here.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Jul 27, 2021

@felipesanches SGTM.

I took a step back last night and thought it may be more user friendly if we consolidate all the name checks together. Currently, every name record is an individual check which produces massive reports. Imo, it is far more elegant to produce tables which show all the records together. This approach makes the reports much easier to understand since users can see all the info on one page. Since we also use Rich, here's a mock up of a potential friendlier future.

Screenshot 2021-07-27 at 12 42 24

@RosaWagner @vv-monsalve @felipesanches thoughts on this idea?

@RosaWagner
Copy link
Contributor

That is beautiful, I love it !!!!

  • style bits could be more explanatory like: is bold, is regular, is italic. use_typo_metric:true, embedding:installable.
  • We could have 21 and 22 so it can be useful for other project than GF stuff
  • It could be presented the same for copyright and designer info
  • actually for all font table related stuff because it is awesome

@felipesanches
Copy link
Collaborator

That's pretty good, @m4rc1e !
My original intention when splitting checks into individual fields was to make it easier for the users to grasp because I thought there were too many criteria for the name table to be presented all at once. Maybe I was mistaken. I'm glad you're experimenting with a different approach and it seems to be wise to present it in tabular form. Thanks!

@felipesanches
Copy link
Collaborator

Here's a small improvement suggestion:

mockup_name_table_report
Use a dark green color for the not-set entries that are good. And say "Not set." in both current and expected columns.

@vv-monsalve
Copy link
Collaborator

vv-monsalve commented Jul 29, 2021

More structured information, lovely!

So happy not seeing the hexadecimal for the bits enabled in the style bits details, and agree on the explanations could be informative.

Name ID 4 would be relevant and perhaps including ID 25 (while it is needed for Adobe issues).

@vv-monsalve
Copy link
Collaborator

Could be this a good opportunity to review the status of this check?

@felipesanches
Copy link
Collaborator

It seems that this is now outdated, as we've got a new attempt at #3800

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.

5 participants