Repository: googlefonts/glyphsLib
Pull request #825 (2022-10-17 20:58:27)

Implements part of #754 via getter/setters for proxying the file format 3 attributes
This implements writing `postscriptBlueValues`, `postscriptOtherBlues`, `postscriptStemSnapV` and `postscriptStemSnapH` for glyphs > ufo.

I added [./tests/glyphs3_test.py](tests) and other tests are passing as well.

I'm not sure how elegant or clumsy it is to have those `GSFontMaster._...` underscore internals and accessing them via the getter/setter proxies, but it seemed the most practical way synthesize them from the `GSFont` attributes required for the offsets.

There are [this](https://github.com/kontur/glyphsLib/blob/fc2b551ddd02da4df0f0b2290fc526a554108847/Lib/glyphsLib/classes.py#L1765), [this](https://github.com/kontur/glyphsLib/blob/fc2b551ddd02da4df0f0b2290fc526a554108847/Lib/glyphsLib/classes.py#L1799) & [this](https://github.com/kontur/glyphsLib/blob/fc2b551ddd02da4df0f0b2290fc526a554108847/Lib/glyphsLib/classes.py#L1819) rather blunt stop gaps in the getters which mainly function to return nothing for cases where the masters are constructed via code, e.g. in the tests (about 50 or so fail otherwise). In a way this could mask a real issue as well, but not sure how realistic it is to rewrite the tests, or add some kind of "defaulting" structures to keep the indexes for constructed objects in sync between GSFont and GSFontMaster.

Should I squash my commits into one? The first commit attempts were a bit hit and miss...

kontur: Any chance to get this reviewed? :)
kontur: Rebased on `main` again, but there appear to be a lot of unrelated tests failing. What's the deal with those, are those residue from merged in WIP and the respective features will still fix those, or is there an effort to just tackle them as separate task?
anthrotype: > there appear to be a lot of unrelated tests failing.

the CI checks seem to be passing now, not sure what you're referring to: https://github.com/googlefonts/glyphsLib/actions/runs/3627890015

I'll take another review pass
anthrotype: the regression tests failing is expected when adding new features from a PR branch, but once that's merged to main, they fix themselves (the regression test CI job compares output from PR branch with that from main branch).

One needs to manually check that the changes are as expected:
https://github.com/googlefonts/glyphsLib/actions/runs/3627890014/jobs/6119377961
anthrotype: the CI also runs tox the same way as you do so that's unexpected.. let me try reproduce locally from a copy of your PR branch
anthrotype: the regression tests failures seem all benign, I can see that postscriptBlueValues are now being compiled which is what this PR is about
kontur: Ah, perfect, thanks for the `-r` hint, not very familiar with tox. Tests run locally without issues as well 🙇 
kontur: @anthrotype yep, those type checks are better now, thanks for pointing that out.

regarding @schriftgestalt 's suggestion: I agree in general, but this would probably need a more orchestrated effort overall, no? it shouldn't just prioritise v3 handling for one attribute. I can rework this for the `alignmentZones` if that will otherwise prevent the PR from being merged. personally I just need to be able to work with v3 files, from releases preferably.
anthrotype: does glyphs.app python API still provide getters like `alignmentZones` returning a list of [GSAlignmentZone](https://docu.glyphsapp.com/#GSAlignmentZone) objects for v3 as well as v2 fonts? If so, we could keep the old v2 API and file an issue to expose the new v3 API alongside it.
simoncozens: I said the other day to Cosimo that we should just support the G3 model and have a separate bit of code to “upgrade” G2 files on load. Trying to manipulate both systems is making the code atrocious, and G3 is much cleaner. But this would be a major overhaul.
kontur: Rebased and refactored the `alignmentZones` getter code filtering that determined what Metric is or is not an alignmentZone.

Patch:
diff --git a/Lib/glyphsLib/classes.py b/Lib/glyphsLib/classes.py
index 362d644f..4abc1473 100755
--- a/Lib/glyphsLib/classes.py
+++ b/Lib/glyphsLib/classes.py
@@ -1445,8 +1445,15 @@ class GSAlignmentZone(GSBase):
         )
 
     def __lt__(self, other):
+        if not isinstance(other, GSAlignmentZone):
+            return NotImplemented
         return (self.position, self.size) < (other.position, other.size)
 
+    def __eq__(self, other):
+        if not isinstance(other, GSAlignmentZone):
+            return NotImplemented
+        return (self.position, self.size) == (other.position, other.size)
+
     def plistValue(self, format_version=2):
         return '"{{{}, {}}}"'.format(
             floatToString5(self.position), floatToString5(self.size)
@@ -1579,25 +1586,30 @@ class GSFontMaster(GSBase):
     _axis_defaults = (100, 100)
 
     def _parse_alignmentZones_dict(self, parser, text):
+        """
+        For glyphs file format 2 this parses the aligmentZone parameter
+        directly. For file format 3 alignmentZones will be infered from
+        metricValues.
+        """
         _zones = parser._parse(text, str)
-        self.alignmentZones = [GSAlignmentZone().read(x) for x in _zones]
+        self._alignmentZones = [GSAlignmentZone().read(x) for x in _zones]
 
     def __init__(self):
         self._customParameters = []
         self._name = None
         self._userData = None
-        self.alignmentZones = []
+        self._alignmentZones = None
+        self._horizontalStems = None
+        self._verticalStems = None
         self.axes = list(self._axis_defaults)
         self.metrics = []
         self.customName = ""
         self.font = None
         self.guides = []
-        self.horizontalStems = 0
         self.iconName = ""
         self.id = str(uuid.uuid4()).upper()
         self.numbers = []
         self.stems = []
-        self.verticalStems = 0
         self.visible = False
         self.weight = self._defaultsForName["weight"]
         self.width = self._defaultsForName["width"]
@@ -1747,6 +1759,87 @@ class GSFontMaster(GSBase):
             self.metrics.append(GSMetricValue(position=0))
         self.metrics[metricIndex] = GSMetricValue(position=value)
 
+    @property
+    def alignmentZones(self):
+        # If there are values that are parsed from file format 2 or which are
+        # explicitly set return those
+        if self._alignmentZones is not None:
+            return self._alignmentZones
+
+        if len(self.metrics) == 0:
+            return []
+
+        zones = []
+        for index, fontMetric in enumerate(self.font.metrics):
+            # Ignore the "italic angle" "metric", it is not an alignmentZone
+            if fontMetric.type == "italic angle":
+                continue
+            metric = self.metrics[index]
+            # Ignore metric without overshoot, it is not an alignmentZone
+            if metric.overshoot == 0:
+                continue
+            zone = GSAlignmentZone(pos=metric.position, size=metric.overshoot)
+            zones.append(zone)
+        return zones
+
+    @alignmentZones.setter
+    def alignmentZones(self, entries):
+        if not isinstance(entries, tuple) and not isinstance(entries, list):
+            raise TypeError(
+                "alignmentZones expected as list, got %s (%s)"
+                % (entries, type(entries))
+            )
+        zones = []
+        for zone in entries:
+            if not isinstance(zone, tuple) and not isinstance(zone, GSAlignmentZone):
+                raise TypeError(
+                    "alignmentZones values expected as tuples of (pos, size) "
+                    "or GSAligmentZone, got: %s (%s)" % (zone, type(zone))
+                )
+            if zone not in zones:
+                zones.append(zone)
+        self._alignmentZones = zones
+
+    @property
+    def horizontalStems(self):
+        if self._horizontalStems is not None:
+            return self._horizontalStems
+
+        if not hasattr(self.font, "stems"):
+            return []
+
+        horizontalStems = []
+        for index, font_stem in enumerate(self.font.stems):
+            if not font_stem.horizontal:
+                continue
+            horizontalStems.append(self.stems[index])
+        return horizontalStems
+
+    @horizontalStems.setter
+    def horizontalStems(self, value):
+        assert type(value) == list
+        self._horizontalStems = value
+
+    @property
+    def verticalStems(self):
+        if self._verticalStems is not None:
+            return self._verticalStems
+
+        if not hasattr(self.font, "stems"):
+            return []
+
+        verticalStems = []
+        for index, font_stem in enumerate(self.font.stems):
+            if font_stem.horizontal:
+                continue
+            verticalStems.append(self.stems[index])
+        return verticalStems
+
+    @verticalStems.setter
+    def verticalStems(self, value):
+        assert type(value) == list
+        self._verticalStems = value
+
     @property
     def ascender(self):
         return self._get_metric("ascender")


Test patch:
diff --git a/tests/glyphs3_test.py b/tests/glyphs3_test.py
index 872df46e..0abc79fe 100644
--- a/tests/glyphs3_test.py
+++ b/tests/glyphs3_test.py
@@ -1,5 +1,6 @@
 import glyphsLib
-from glyphsLib.classes import GSFont, GSFontMaster
+import pytest
+from glyphsLib.classes import GSFont, GSFontMaster, GSAlignmentZone
 
 
 def test_metrics():
@@ -22,3 +23,65 @@ def test_glyphspackage_load(datadir):
     font1.DisplayStrings = ""  # glyphspackages, rather sensibly, don't store user state
     font2 = glyphsLib.load(str(datadir.join("GlyphsUnitTestSans3.glyphspackage")))
     assert glyphsLib.dumps(font1) == glyphsLib.dumps(font2)
+
+
+def test_glyphs3_alignment_zones(datadir):
+    font = glyphsLib.load(str(datadir.join("GlyphsUnitTestSans3.glyphs")))
+    master = font.masters[0]
+
+    assert master.alignmentZones != []
+
+    # As per classes_tests::GSAlignmentZoneFromFileTest
+    zones = [(800, 10), (700, 10), (470, 10), (0, -10), (-200, -10)]
+    for i, (pos, size) in enumerate(zones):
+        assert master.alignmentZones[i].position == pos
+        assert master.alignmentZones[i].size == size
+
+    assert len(master.alignmentZones) == 5
+
+    # alignmentZones can be set as tuples or instances
+    master.alignmentZones = [GSAlignmentZone(800, 10), GSAlignmentZone(0, -10)]
+
+    assert master.alignmentZones[-2].position == 800
+    assert master.alignmentZones[-2].size == 10
+
+    assert master.alignmentZones[-1].position == 0
+    assert master.alignmentZones[-1].size == -10
+
+    assert len(master.alignmentZones) == 2
+
+    # Duplicates should get added only once
+    master.alignmentZones = [(0, -10), (0, -10)]
+    assert len(master.alignmentZones) == 1
+
+    # Let it be emptyable
+    master.alignmentZones = []
+    assert len(master.alignmentZones) == 0
+
+    # Non-list values not allowed
+    with pytest.raises(TypeError):
+        master.alignmentZones = (800, 10)
+
+    with pytest.raises(TypeError):
+        master.alignmentZones = 800
+
+    with pytest.raises(TypeError):
+        master.alignmentZones = "800"
+
+    # Only tuples and GSAlignmentZone allowed inside
+    with pytest.raises(TypeError):
+        master.alignmentZones = [False, True]
+
+    with pytest.raises(TypeError):
+        master.alignmentZones = [[0, -10], [800, 10]]
+
+    with pytest.raises(TypeError):
+        master.alignmentZones = ["", ""]
+
+
+def test_glyphs3_stems(datadir):
+    font = glyphsLib.load(str(datadir.join("GlyphsUnitTestSans3.glyphs")))
+    master = font.masters[0]
+
+    assert master.verticalStems == [17, 19]
+    assert master.horizontalStems == [16, 16, 18]