-
Notifications
You must be signed in to change notification settings - Fork 103
[family_and_style_max_length] Modernize check #5020
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
base: main
Are you sure you want to change the base?
Conversation
Some of your questions are answered here: google/fonts#9185 I did research into the family name length problem and documented my findings there. |
@arrowtype, I'll review this soon. I just did not get to it yet. In the mean-time, I'm curious to hear from @simoncozens about his views on these changes, as it will likely be something to be updated on Fontspector as well, since that's gradually turning into our preferred QA tool. side-note: I think that Fontbakery's python codebase can still be useful as this prototyping stage both for new checks as well as for updating checking criteria. But users are strongly encouraged to try Fontspector, as it is more at the leading-edge of QA tooling. |
Thanks, @felipesanches! All good, and thanks for the quick comment. I think I’ve done what I can, so far. There is still a test failing, but it’s for a file that isn’t related to the changes made in this PR (so far as I can tell). The failing test is from
|
Also, that sounds like a good plan, prototyping checks here, then moving them into Fontspector. That system is blazing fast, so it will be great to use on larger projects, especially. Also, having that step be almost instant will help projects of any size. |
55d09ca
to
dba6e3e
Compare
dba6e3e
to
0dae195
Compare
Made a small refactor of the check based on a review from a colleague. Also merged the CHANGELOG, and made a couple of typos, so I had to revise that commit and force push. |
Thanks for approving these changes, @RamsesAupart! I don’t have authorization to push to this branch... is there anything needed from me, before we merge this? |
I almost hesitated to add anything else here, as I worry it could further delay this update. However, I’ve run into a small issue in my updated check, and it was easily fixed: The earlier state of this check would falsely fail on a variable + STAT name like “Longish Family Name Regular Italic” (34 characters), but MS Word doesn’t actually exhibit its name length issue in this style, because it elides STAT names “Regular” and “Italic” (even if they aren’t marked as elidable in the STAT table). So, my latest commit doesn’t count those STAT name particles towards the length limit. I’ve tested to verify that it does still appropriately fail on other long STAT names, and everything still seems to be working well. |
Reading through the issue again – especially this comment — and constantly hitting warnings for long postscript names, I think maybe this PR really should remove the NameID 6 check. I also want to double-check that nameID 4 doesn’t come into play. I don’t think it does, but I don’t see a record of this being tested. UPDATE: I’ve tested it here. The static checks in this PR are still solid. So, planning to update this once more. |
Okay, I’ve removed a note about nameID 21 (WWS family name) in variable fonts, after additional testing. |
Description
Relates to issue #2179, specifically later comments and testing, like #2179 (comment)
Changes:
FONT_FAMILY_NAME
) rather than NameID 4 (FULL_FONT_NAME
)Consider removing test of NameID 6... I ended up leaving this, but noting in its warning message that the issue is probably only with pre-Mac OS X systems.Checklist
CHANGELOG.md
Process notes
Questions to answer
Does a long nameID 6 really "cause issues with PostScript printers, especially on Mac platforms"? What is the basis for this check? Neither of the listed issues seem to say anything about this.This could be a later adjustment.Question for @felipesanches
I’ve left the prior check for VF family name + fvar entry, but I’m not quite sure whether it’s actually useful. This may depend on a question: does FontBakery “allow” VFs that don’t have STAT tables, or is there a common check for that? It looks like there is:
fontbakery/Lib/fontbakery/checks/required_tables.py
Lines 87 to 90 in 49dcd6a
So, the
required_tables
check fails if a VF doen’t include a STAT table, and this check is part of the Universal profile. And, it does look like the Universal profile is included in almost all of the other profiles, either directly or via googlefonts/adobefonts profiles. Does that mean it’s fairly safe to assume that we shouldn’t bother with checking VFs that lack a STAT table, or is it better to keep the old fvar condition, anyway?Process screenshots
Static fonts
MS Word, Windows 11:
Variable fonts
Works:
Proxima Nova Thai Loop Var
Blck
(Roman
is flagged as elidable, and therefore, not counted), 31 total charactersThin
, 31 total charactersFails:
Proxima Nova Thai Loop Var
Extrabold
(Roman
is flagged as elidable, and therefore, not counted), 36 total charactersLight
, 32 total characters