Reviewing the code, PANOSE_expected() seems wrong for LATIN_DECORATIVE

drj11 commented 2 months ago

Observed behaviour

The PANOSE_expected() function in https://github.com/fonttools/fontbakery/blob/main/Lib/fontbakery/checks/opentype/name.py is used to dispense advice in the related PANOSE warning when a font appears to be monospaced but does not have its PANOSE info set correctly (and also various other metadata).

But looking at this function, we see:

    if family_type in [
        PANOSE_Family_Type.LATIN_HAND_WRITTEN,
        PANOSE_Family_Type.LATIN_SYMBOL,
    ]:
        return f"Please set PANOSE Spacing to {PANOSE_Spacing.MONOSPACED} (monospaced)"

    if family_type == PANOSE_Family_Type.LATIN_SYMBOL:
        return (
            f"PANOSE Family Type is set to 4 (latin symbol)."
            f" Please set it instead to {PANOSE_Family_Type.LATIN_TEXT} (latin text),"
            f" {PANOSE_Family_Type.LATIN_HAND_WRITTEN} (latin hand written)"
            f" or {PANOSE_Family_Type.LATIN_SYMBOL} (latin symbol)"
        )

The problem with this code is that the second if will never run. The case if family_type == PANOSE_Family_Type.LATIN_SYMBOL has already been handled by the previous if which flows into a return.

Expected behaviour

I'm guessing, mostly from the presence of PANOSE Family Type is set to 4 that the condition in the second if should be against PANOSE_Family_Type.LATIN_DECORATIVE (note DECORATIVE, not SYMBOL), which does indeed have numeric value 4. (see https://monotype.github.io/panose/pan4.htm)

If this is correct and the change is made, the recommendation in the string also needs fixing: latin symbol -> latin decorative.

Although, as someone who is preparing a font that is likely to be both monospaced and decorative, i'm skeptical that the check that uses this is doing anything useful in this case: it would probably be okay to let LATIN_DECORATIVE fonts to be seems_monospaced without having a monospaced indicator in its PANOSE (in so far as i think PANOSE is useful at all).

So maybe we could delete that entire arm that checks the LATIN_DECORATIVE case (incorrectly).

Resources and steps needed to reproduce

I don't have any fonts or tests or other supporting materials for this yet.

felipesanches commented 2 months ago

I'm booking a videocall with @tphinney later this week to discuss this check, and also to address issues #2857 and #2831.

For now, I've removed the unreachable code and also made the check not emit any advice when uncertain. This will likely improve with an additional PR after my upcoming conversation with Thomas.

drj11 commented 2 months ago

While we're on this topic, can we remove the bad-numberOfHMetrics check: that numberOfHMetrics == 3 which pretty much only makes sense for Courier circa 1995 (given zerowidth marks) https://github.com/fonttools/fonttools/issues/3014

fonttools / fontbakery