Simple G3 RTL Kerning Support

Lib/glyphsLib/builder/constants.py

@@ -53,6 +53,7 @@
 UFO_ORIGINAL_KERNING_GROUPS_KEY = GLYPHLIB_PREFIX + "originalKerningGroups"
 UFO_GROUPS_NOT_IN_FEATURE_KEY = GLYPHLIB_PREFIX + "groupsNotInFeature"
 UFO_KERN_GROUP_PATTERN = re.compile("^public\\.kern([12])\\.(.*)$")
+SORT_G3_RTL_KERNING_SUFFIX = ".G3RTLKERNINGCONVERSION"
 
 LOCKED_GUIDE_NAME_SUFFIX = " [locked]"
 

Lib/glyphsLib/builder/groups.py

@@ -22,6 +22,7 @@
     UFO_GROUPS_NOT_IN_FEATURE_KEY,
     UFO_KERN_GROUP_PATTERN,
     BRACKET_GLYPH_RE,
+    SORT_G3_RTL_KERNING_SUFFIX,
 )
 
 
@@ -71,14 +72,17 @@ def to_ufo_groups(self):
                     recovered.add((glyph_name, int(side)))
 
     # Read modified grouping values
+    sides = [1, 2]
     for glyph in self.font.glyphs.values():
-        for side in 1, 2:
+        for i, side in enumerate(sides):
+            other_side = sides[len(sides) - 1 - i]
             if (glyph.name, side) not in recovered:
                 attr = _glyph_kerning_attr(glyph, side)
-                group = getattr(glyph, attr)
-                if group:
-                    group = f"public.kern{side}.{group}"
-                    groups[group].append(glyph.name)
+                glyph_group = getattr(glyph, attr)
+                if glyph_group:
+                    _add_glyph_to_ufo_groups(
+                        glyph, groups, glyph_group, side, other_side
+                    )
 
     # Update all UFOs with the same info
     for source in self._sources.values():
@@ -87,13 +91,40 @@ def to_ufo_groups(self):
             source.font.groups[name] = glyphs[:]
 
 
+def _add_glyph_to_ufo_groups(glyph, groups, glyph_group, side, other_side):
+    if not glyph_group.endswith(SORT_G3_RTL_KERNING_SUFFIX):
+        # Traditional group
+        group = f"public.kern{side}.{glyph_group}"
+        if glyph.name not in groups[group]:
+            groups[group].append(glyph.name)
+        # Additional RTL group
+        # This will add lots of unused groups which we'll prune later
+        # but is necessary to implement G3 RTL kerning. See kerning.py
+        group = f"public.kern{other_side}.{glyph_group}{SORT_G3_RTL_KERNING_SUFFIX}"
+        if glyph.name not in groups[group]:
+            groups[group].append(glyph.name)
+    else:
+        # Additional RTL group
+        # This will add lots of unused groups which we'll prune later
+        # but is necessary to implement G3 RTL kerning. See kerning.py
+        group = f"public.kern{side}.{glyph_group}"
+        if glyph.name not in groups[group]:
+            groups[group].append(glyph.name)
+
+        # Traditional group
+        glyph_group = glyph_group.replace(SORT_G3_RTL_KERNING_SUFFIX, "")
+        group = f"public.kern{other_side}.{glyph_group}"
+        if glyph.name not in groups[group]:
+            groups[group].append(glyph.name)
+
+
 def to_glyphs_groups(self):
     # Build the GSClasses from the groups of the first UFO.
     groups = []
     for source in self._sources.values():
         for name, glyphs in source.font.groups.items():
             # Filter out all BRACKET glyphs first, as they are created at
-            # to_designspace time to inherit glyph kerning to their bracket
+            # to_designspace time to inherit to_ufo_groups
             # variants. They need to be removed because Glpyhs.app handles that
             # on its own.
             glyphs = [name for name in glyphs if not BRACKET_GLYPH_RE.match(name)]

Lib/glyphsLib/builder/kerning.py

@@ -15,39 +15,123 @@
 
 import re
 
-from .constants import BRACKET_GLYPH_RE, UFO_KERN_GROUP_PATTERN
+from .constants import (
+    BRACKET_GLYPH_RE,
+    UFO_KERN_GROUP_PATTERN,
+    SORT_G3_RTL_KERNING_SUFFIX,
+)
 
 
 def to_ufo_kerning(self):
+    used_groups = set()
     for master in self.font.masters:
+        ufo = self._sources[master.id].font
         kerning_source = master.metricsSource  # Maybe be a linked master
         if kerning_source is None:
             kerning_source = master
-        if kerning_source.id in self.font.kerning:
-            kerning = self.font.kerning[kerning_source.id]
-            _to_ufo_kerning(self, self._sources[master.id].font, kerning)
+        if kerning_source.id in self.font.kerningLTR:
+            kerning = self.font.kerningLTR[kerning_source.id]
+            used_groups.update(_to_ufo_kerning(self, ufo, kerning))
+        if kerning_source.id in self.font.kerningRTL:
+            kerning = self.font.kerningRTL[kerning_source.id]
+            used_groups.update(_to_ufo_kerning(self, ufo, kerning, "RTL"))
 
+    # Pruning as consequence of PR #838
+    for master in self.font.masters:
+        ufo = self._sources[master.id].font
+
+        # Prune kerning groups that are not used in kerning rules
+        # (added but unused .RTL groups, see groups.py)
+        for group in list(ufo.groups.keys()):
+            # Group exists in font but not in kerning rules
+            if group.startswith("public.kern") and group not in used_groups:
+                del ufo.groups[group]
+
+        # Remove .RTL from groups
+        for group in list(ufo.groups.keys()):
+            if group.startswith("public.kern") and group.endswith(
+                SORT_G3_RTL_KERNING_SUFFIX
+            ):
+                ufo.groups[group.replace(SORT_G3_RTL_KERNING_SUFFIX, "")] = ufo.groups[
+                    group
+                ]
+                del ufo.groups[group]
+
+        # Remove .RTL from kerning
+        for first, second in list(ufo.kerning.keys()):
+            if (
+                first.startswith("public.kern")
+                and first.endswith(SORT_G3_RTL_KERNING_SUFFIX)
+                and second.startswith("public.kern")
+                and second.endswith(SORT_G3_RTL_KERNING_SUFFIX)
+            ):
+                ufo.kerning[
+                    first.replace(SORT_G3_RTL_KERNING_SUFFIX, ""),
+                    second.replace(SORT_G3_RTL_KERNING_SUFFIX, ""),
+                ] = ufo.kerning[first, second]
+                del ufo.kerning[first, second]
+            elif first.startswith("public.kern") and first.endswith(
+                SORT_G3_RTL_KERNING_SUFFIX
+            ):
+                ufo.kerning[
+                    first.replace(SORT_G3_RTL_KERNING_SUFFIX, ""), second
+                ] = ufo.kerning[first, second]
+                del ufo.kerning[first, second]
+            elif second.startswith("public.kern") and second.endswith(
+                SORT_G3_RTL_KERNING_SUFFIX
+            ):
+                ufo.kerning[
+                    first, second.replace(SORT_G3_RTL_KERNING_SUFFIX, "")
+                ] = ufo.kerning[first, second]
+                del ufo.kerning[first, second]
 
-def _to_ufo_kerning(self, ufo, kerning_data):
+
+def _to_ufo_kerning(self, ufo, kerning_data, direction="LTR"):
     """Add .glyphs kerning to an UFO."""
 
-    warning_msg = "Non-existent glyph class %s found in kerning rules."
-
-    for left, pairs in kerning_data.items():
-        match = re.match(r"@MMK_L_(.+)", left)
-        left_is_class = bool(match)
-        if left_is_class:
-            left = "public.kern1.%s" % match.group(1)
-            if left not in ufo.groups:
-                self.logger.warning(warning_msg % left)
-        for right, kerning_val in pairs.items():
-            match = re.match(r"@MMK_R_(.+)", right)
-            right_is_class = bool(match)
-            if right_is_class:
-                right = "public.kern2.%s" % match.group(1)
-                if right not in ufo.groups:
-                    self.logger.warning(warning_msg % right)
-            ufo.kerning[left, right] = kerning_val
+    class_missing_msg = "Non-existent glyph class %s found in kerning rules."
+    overwriting_kerning_msg = "Overwriting kerning value for %s."
+
+    used_groups = set()
+
+    for first, pairs in kerning_data.items():
+        first, is_class = _ufo_class_name(first, direction, 1)
+        if is_class:
+            used_groups.add(first)
+        if is_class and first not in ufo.groups:
+            self.logger.warning(class_missing_msg, first)
+
+        for second, kerning_val in pairs.items():
+            second, is_class = _ufo_class_name(second, direction, 2)
+            if is_class:
+                used_groups.add(second)
+            if is_class and second not in ufo.groups:
+                self.logger.warning(class_missing_msg, second)
+
+            if (first, second) in ufo.kerning:
+                self.logger.warning(overwriting_kerning_msg, first, second)
+            ufo.kerning[first, second] = kerning_val
+
+    return used_groups
+
+
+def _ufo_class_name(name, direction, order):
+    """Return the UFO class name for a .glyphs class name."""
+    if order == 1:
+        side = "L" if direction == "LTR" else "R"
+    else:
+        assert order == 2
+        side = "R" if direction == "LTR" else "L"
+
+    match = re.match(rf"@MMK_{side}_(.+)", name)
+
+    name_is_class = bool(match)
+    if name_is_class:
+        name = f"public.kern{order}.{match.group(1)}"
+        if direction == "RTL":
+            name += SORT_G3_RTL_KERNING_SUFFIX
+
+    return name, name_is_class
 
 
 def to_glyphs_kerning(self):

README.rst

@@ -145,6 +145,23 @@ In practice there are always a few diffs on things that don't really make a
 difference, like optional things being added/removed or whitespace changes or
 things getting reordered...
 
+Kerning interaction between Glyphs 3 and UFO
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Glyphs 3 introduced the attribute ``.kerningRTL`` for the storage of RTL kerning pairs
+which breaks with the UFO spec of storing kerning as logical first/second pairs
+regardless of writing direction.
+As of `PR #838 <https://github.com/googlefonts/glyphsLib/pull/838>`__ glyphsLib
+reverts this separate Glyphs 3-style RTL kerning back to Glyphs 2/UFO-style kerning
+upon conversion of a Glyphs object to a UFO object, *but it does not convert the kerning
+back to Glyphs 3-style when converting a UFO object to a Glyphs object!* 
+
+This means that if you convert a UFO to a Glyphs file and subsequently open that file
+in Glyphs 3, the RTL kerning will initially not be visible in the UI, but be hidden
+in the LTR kerning. This is identical to opening a Glyphs 2 file with RTL kerning
+in Glyphs 3. It is in the responsibility of Glyphs 3 and the user to convert the kerning
+back to Glyphs 3's separate RTL kerning.
+
 Make a release
 ^^^^^^^^^^^^^^
 

tests/builder/to_glyphs_test.py

@@ -163,8 +163,22 @@ def test_groups(ufo_module):
 
     (ufo,) = to_ufos(font)
 
-    # Check that nothing has changed
-    assert dict(ufo.groups) == groups_dict
+    # Prior to PR #838, we were checking for d1 and d2 to be equal.
+    # However, with PR #838, we are now pruning kerning groups that are not
+    # used in the kerning table.
+    # The groups in the assertion below got pruned.
+    d1 = dict(ufo.groups)
+    d2 = groups_dict
+
+    assert set(d2) ^ set(d1) == {
+        "public.kern1.notInFont",
+        "public.kern1.hebrewLikeT",
+        "public.kern1.halfInFont",
+        "public.kern2.hebrewLikeO",
+        "public.kern2.oe",
+        "public.kern1.empty",
+        "public.kern1.T",
+    }
 
     # Check that changing the `left/rightKerningGroup` fields in Glyphs
     # updates the UFO kerning groups
@@ -178,7 +192,20 @@ def test_groups(ufo_module):
 
     (ufo,) = to_ufos(font)
 
-    assert dict(ufo.groups) == groups_dict
+    # Checking for known pruned groups (PR #838)
+    d1 = dict(ufo.groups)
+    d2 = groups_dict
+
+    assert set(d2) ^ set(d1) == {
+        "public.kern1.halfInFont",
+        "public.kern1.empty",
+        "public.kern1.newNameT",
+        "public.kern2.oe",
+        "public.kern2.hebrewLikeO",
+        "public.kern1.onItsOwnO",
+        "public.kern1.hebrewLikeT",
+        "public.kern1.notInFont",
+    }
 
 
 def test_guidelines(ufo_module):