Simple G3 RTL Kerning Support

[yanone]

This is based on the latest discussion in #778, namely @simoncozens’ last comment.

I needed to make this into a separate branch for my own sanity.

[yanone]

There are a lot of duplicate .RTL groups now that ruin the tests. Let's focus on the core functionality first and then I'll think of ways to remove the unnecessary groups.

[anthrotype]

ways to remove the unnecessary groups.

i guess, when you're done with the kerning, you can pass again on the groups and remove those that aren't actually used anywhere in the kerning dict

[yanone]

About round-tripping

The way I see it, it's not easily possible, for the same reasons we were stuck at earlier: lack of directional data for single glyph kerning.

Theoretically it should happen here:

def to_glyphs_kerning(self):
    """Add UFO kerning to GSFont."""
    for master_id, source in self._sources.items():
        for (left, right), value in source.font.kerning.items():
            if BRACKET_GLYPH_RE.match(left) or BRACKET_GLYPH_RE.match(right):
                # Skip all bracket glyph entries, as they are duplicates of their
                # parents'.
                continue
            left_match = UFO_KERN_GROUP_PATTERN.match(left)
            right_match = UFO_KERN_GROUP_PATTERN.match(right)
            if left_match:
                left = "@MMK_L_{}".format(left_match.group(2))
            if right_match:
                right = "@MMK_R_{}".format(right_match.group(2))
            print(left.endswith(".RTL"), right.endswith(".RTL"), left, right)
            self.font.setKerningForPair(master_id, left, right, value)

The inserted print statement shows the problem (False False in lowest line):

True False @MMK_L_leftAlef.RTL he-hb
True True @MMK_L_leftBet.RTL @MMK_R_rightAlef.RTL
True True @MMK_L_reh-ar.RTL @MMK_R_hah-ar.init.swsh.RTL
False True he-hb @MMK_R_rightAlef.RTL
False False he-hb he-hb

Any suggestions welcome.

About the checks

I need help with the checks.

tests/builder/to_glyphs_test.py:167: AssertionError

The pruning currently doesn't check for empty groups or whether they make any sense at all. It only checks for whether or not groups are part of kerning rules, so my assumption is that no kerning has been defined for these test-cases. Above from where the error is thrown, several groups are added that are nonsensical, like empty groups or groups named "notInFont". So these get thrown out. This check definitely clashes with kerning group pruning in several of those nonsensical groups. My added kerning group pruning is explicit in checking for groups to be pruned to start with public.kern so that arbitrary groups will be preserved.

Here are some examples of nonsensical groups in that check:

    ufo.groups["public.kern1.notInFont"] = ["L"]
    ...
    # Empty groups as well (found in the wild)
    ufo.groups["public.kern1.empty"] = []

One suggestion to deal with this is to, instead of comparing dictionaries to be identical, to use a dictionary comparison to compare the difference of the two dictionaries to be identical to a known set of pruned groups.
I would use deepdiff (to my knowledge the only way to check for differences in dictionaries regardless of the dictionary order, as in the diff output will always be ordered in the same way). This adds an extra dependency package. Suggestions welcome.

This will output: {'dictionary_item_removed': [root['public.kern1.T'], root['public.kern2.oe'], root['public.kern1.notInFont'], root['public.kern1.halfInFont'], root['public.kern1.empty'], root['public.kern1.hebrewLikeT'], root['public.kern2.hebrewLikeO']]} which kind of confirms my idea and confirms that only kerning groups are pruned.

tests/builder/builder_test.py:2268: AssertionError

Using the same technique, the dictionary difference is: {'dictionary_item_removed': [root['public.kern2.foo'], root['public.kern1.foo']]}

A few lines further down in that check, kerning is actually checked to be empty: assert ds.sources[1].font.kerning == {}, which confirms why the groups aren't there.

I propose to alter both checks to make sense under the idea of pruning unused kerning groups. I'm only insecure about using deepdiff. Maybe you know of a better way to compare without added dependencies. I know that sets can do stuff like that, but I'm not sure of the data order.

[yanone]

Windows checks are passing now. Only the regression check remains

[anthrotype]

we want groups.plist be the same for all master UFOs, as fontmake.instantiator will just use the groups.plist of the default master when creating instance UFOs.
In Glyphs.app the kerning groups are at the glyph level and as such apply to all masters, so when converting to designspace all the UFO sources (1 for each GSMaster) are expected to have the same groups.plist.
Please take the union of all the used groups across all masters when deciding which groups to keep and ensure they are the same across all UFO masters, and fix up any test failures that may arise accordingly.
As regards the other ufoLib warning about glyph being in more than one group, I think for the time being the only solution, or rather, workaround is to add a logging filter to fontmake that specifically mutes that particular logging warning when building from .glyphs sources. Ignore that one in this PR.

[yanone]

Okay guys, I think this is it.

Don't ask me how I ended up here, but I'm now basically undoing all of G3 RTL kerning back to pristine G2-style kerning by also pruning .RTL from group names and kerning definition itself.

As a consequence, the checks that I had altered in builder_test.py are now also back to their original state entirely, while the changes to the other check in to_glyphs_test.py with the nonsensically added groups is staying in place because those groups actually had no kerning to back them up.

The code to assign kerning groups in groups.py has gotten twice as complex because groups were coming in with .RTL already appended (possibly from the round-tripping, I'm not sure), so then they'll get sorted across as previously, except the other way around.

Round-tripping now means the same as before today's edits, except that we're only looking at pristine G2-style kerning now, indistinguishable from how Glyphs 2 would have saved it. For illustration, see how I removed the .RTL in the G3 kerning check:

    assert (
        first_derivative_ufos[0].kerning[
            ("public.kern1.reh-ar", "public.kern2.hah-ar.init.swsh")
        ]
        == -50
    )

So really all we've done here is undo Georg's work :)

[yanone]

I updated the README about the kerning conversion. Please have a look. There's nothing more to change from my side.

[yanone]

Happy New Year.
Can we please revisit this issue? We're slowly approaching one year now since I discovered the problem (and longer since Georg quietly changed his file data). That's one lost year for open-source, Glyphs-based Arabic typography.

[yanone]

Can we please revisit this?

I've randomly checked the differences of the regression tests and have found that these are all kerning groups for which no kerning is defined, so they are all legitimately missing from the UFOs as generated by this PR.

I therefore strongly believe that this PR is good and see no reason why this is stalled.

[anthrotype]

I'm inclined to close this in favour of #865