fonttools / fontbakery

🧁 A font quality assurance tool for everyone
https://fontbakery.readthedocs.io
Apache License 2.0
534 stars 99 forks source link

Clarify error message of [com.adobe.fonts/check/varfont/valid_default_instance_nameids] #4761

Closed arrowtype closed 2 weeks ago

arrowtype commented 3 weeks ago

Description

com.adobe.fonts/check/varfont/valid_default_instance_nameids checks whether there is consistency between the default instance of a variable font and the font’s typographic style name, in either NameID 2 or NameID 17.

This is important, but the current check isn’t very clear about an easy problem to fall into, so it’s hard to take action on / easy to ignore.

Example

To be specific, with an issue that recently occurred in practice on a font I helped build: it is easy in GlyphsApp to set up a variable font export wherein the Typographic Style Name (NameID 17) is set to "Regular" – even if the default location of that font is at, say, Hairline. The designer set up the font like this, which seems like an intuitive enough name, if you are making related but separate Roman and Italic variable fonts.

However, this results in an incorrectly NameID 17. This is a problem in macOS apps, for one, where this will result in the default instance being hidden from the style menu. In this case, the "Hairline" style was missing from the TextEdit style menu.

However, the existing fail/warn message only suggests renaming the default instance, which is confusing. Users are likely to see that message and think, "Why would I rename 'Hairline' to 'Regular'? My exports are set up with the intended style names."

I considered writing a totally new check to avoid this scenario, which would check to confirm that NameID 17 matches the default style. However, that would be redundant. This check does catch the problem scenario, but it simply doesn’t present the information in a way that is easy to understand and take action on.

Proposed solution

The current message is like this:

FAIL 'Hairline' instance has the same coordinates as the default instance; its subfamily name should be 'Regular'.

My proposed message adds a note:

FAIL 'Hairline' instance has the same coordinates as the default instance; its subfamily name should be 'Regular'.

Note: It is alternatively possible that Name ID 17 is incorrect, and should be set to the default instance’s subfamily name, 'Hairline', rather than 'Regular'.

This note is only added if the font actually has NameID 17, and it doesn’t match the default instance. If the font lacks NameID 17, and the default instance doesn’t match NameID 2, the user may have a 'regular' default instance with a non-standard but valid name like "Book." In that case, they would see a message like this:

FAIL 'Book' instance has the same coordinates as the default instance; its subfamily name should be 'Regular'.

Note: If the default instance really is supposed to be called 'Book', the problem may be that the font lacks NameID 17, which should probably be present and set to 'Book'.

I may be missing something, and the note I’ve added could probably be further improved. Either way, I’m happy to be given feedback!

Checklist

felipesanches commented 3 weeks ago

Screenshot from 2024-06-07 11-37-42

https://github.com/fonttools/fontbakery/actions/runs/9406838087/job/25911200967?pr=4761

arrowtype commented 2 weeks ago

Okay, I’ve edited my messages to make them fit into the Black line length limit.

Thanks for your review, Felipe! Hopefully this iteration works.

arrowtype commented 2 weeks ago

@felipesanches okay, the Black formatted check is now successful, but others are failing. I’m not quite sure why. Can you help me understand what’s wrong, if those errors are relevant / blocking the merge?