robotools / fontParts

The replacement for RoboFab
MIT License
135 stars 44 forks source link

Component compatibility: misleading warning message when component order switched #349

Open madig opened 6 years ago

madig commented 6 years ago
<?xml version="1.0" encoding="UTF-8"?>
<glyph name="uni1FF2" format="2">
    <unicode hex="1FF2"/>
    <advance width="838"/>
    <outline>
        <component base="omega"/>
        <component base="uni1FEF" xOffset="237"/>
        <component base="uni1FBE" xOffset="283"/>
    </outline>
</glyph>

vs.

<?xml version="1.0" encoding="UTF-8"?>
<glyph name="uni1FF2" format="2">
    <unicode hex="1FF2"/>
    <advance width="842"/>
    <outline>
        <component base="omega"/>
        <component base="uni1FBE" xOffset="285"/>
        <component base="uni1FEF" xOffset="246"/>
    </outline>
</glyph>

results in

In [25]: ur[gn].isCompatible(ub[gn])
Out[25]:
(True, [Warning] Glyph: "uni1FF2" + "uni1FF2"
 [Warning] Glyph: "uni1FF2" contains component ('uni1FEF', 1) not in "uni1FF2"
 [Warning] Glyph: "uni1FF2" contains component ('uni1FBE', 2) not in "uni1FF2"
 [Warning] Glyph: "uni1FF2" contains component ('uni1FBE', 1) not in "uni1FF2"
 [Warning] Glyph: "uni1FF2" contains component ('uni1FEF', 2) not in "uni1FF2")

This should read more like

[Warning] Glyph: "uni1FF2" vs. "uni1FF2"
[Warning] Glyph: component index 1: left is uni1FEF, right is uni1FBE
[Warning] Glyph: component index 2: left is uni1FBE, right is uni1FEF
benkiel commented 6 years ago

@madig Agree. It should catch both differences in names and index, right now it's a bit too dumb. Any chance you could make a PR for this change?

madig commented 6 years ago

Looking into it, have to understand https://github.com/robofab-developers/fontParts/blob/master/Lib/fontParts/base/compatibility.py first. Might take a while with my other priorities.

I'd actually like to change the reporting mechanism a bit at some point. I'm writing a Fontbakery check for UFO sources and find it limiting that I can compare only two fonts at a time and then have to scrape the relevant results out like this (can't rely on report.fatal because it doesn't consider anchor and component diffs fatal):

[...]
        for glyph_name in sorted(all_glyphs):
            if not all(glyph_name in font for font in fonts):
                _compatible = False
                yield WARN, f"{family}, glyph {glyph_name} not in all fonts of family."

            glyphs_to_check = [font[glyph_name] for font in fonts if glyph_name in font]
            compat_reports = [
                glyph.isCompatible(glyphs_to_check[(i + 1) % len(glyphs_to_check)])[1]
                for i, glyph in enumerate(glyphs_to_check)
            ]
            incompatibilities = []
            if any(r.contours or r.contourCountDifference for r in compat_reports):
                incompatibilities.append("contours")
            if any(
                r.componentCountDifference
                or r.componentsMissingFromGlyph1
                or r.componentsMissingFromGlyph2
                for r in compat_reports
            ):
                incompatibilities.append("components")
            if any(
                r.anchorCountDifference
                or r.anchorsMissingFromGlyph1
                or r.anchorsMissingFromGlyph2
                for r in compat_reports
            ):
                incompatibilities.append("anchors")

            if incompatibilities:
                _compatible = False
                incompatibility_str = ", ".join(incompatibilities)
                yield WARN, f"{family}, glyph {glyph_name} isn't compatible across the family due to: {incompatibility_str}."
[...]
[...]
 * WARN: Ubuntu Mono (Italics), glyph w isn't compatible across the family due to: contours.
 * WARN: Ubuntu Mono (Italics), glyph x isn't compatible across the family due to: contours.
 * WARN: Ubuntu Mono (Italics), glyph xi isn't compatible across the family due to: contours.
[...]

Have to come up with something good first, though.