Repository: googlefonts/glyphsLib
Pull request #525 (2019-05-11 15:45:35)

Support UFO's `public.skipExportGlyphs`, take two
The lib key is now supported alongside the old glyph level lib key, to avoid disruption in fontmake etc. until feature subsetting from `public.skipExportGlyphs` is implemented.

Todo:
- [ ] Check that this doesn't mess with the `public.skipExportGlyphs` logic used before.
- [ ] Add switch for `glyphs2ufo` and `UFOBuilder` to output either old glyph lib key or the new `public.skipExportGlyphs`, defaulting to old behavior until we get the rest of the toolchain up to scratch (automatic feature subsetting, leaving non-exported glyphs in as components to save on binary size)


Patch:
diff --git a/Lib/glyphsLib/__init__.py b/Lib/glyphsLib/__init__.py
index 9c21d754..a665e21d 100644
--- a/Lib/glyphsLib/__init__.py
+++ b/Lib/glyphsLib/__init__.py
@@ -90,6 +90,7 @@ def build_masters(
     create_background_layers=False,
     generate_GDEF=True,
     store_editor_state=True,
+    write_skipexportglyphs=False,
 ):
     """Write and return UFOs from the masters and the designspace defined in a
     .glyphs file.
@@ -124,6 +125,7 @@ def build_masters(
         minimize_glyphs_diffs=minimize_glyphs_diffs,
         generate_GDEF=generate_GDEF,
         store_editor_state=store_editor_state,
+        write_skipexportglyphs=write_skipexportglyphs,
     )
 
     # Only write full masters to disk. This assumes that layer sources are always part
diff --git a/Lib/glyphsLib/builder/__init__.py b/Lib/glyphsLib/builder/__init__.py
index 0fae7552..62010187 100644
--- a/Lib/glyphsLib/builder/__init__.py
+++ b/Lib/glyphsLib/builder/__init__.py
@@ -31,6 +31,7 @@ def to_ufos(
     minimize_glyphs_diffs=False,
     generate_GDEF=True,
     store_editor_state=True,
+    write_skipexportglyphs=False,
 ):
     """Take a GSFont object and convert it into one UFO per master.
 
@@ -53,6 +54,7 @@ def to_ufos(
         minimize_glyphs_diffs=minimize_glyphs_diffs,
         generate_GDEF=generate_GDEF,
         store_editor_state=store_editor_state,
+        write_skipexportglyphs=write_skipexportglyphs,
     )
 
     result = list(builder.masters)
@@ -71,6 +73,7 @@ def to_designspace(
     minimize_glyphs_diffs=False,
     generate_GDEF=True,
     store_editor_state=True,
+    write_skipexportglyphs=False,
 ):
     """Take a GSFont object and convert it into a Designspace Document + UFOS.
     The UFOs are available as the attribute `font` of each SourceDescriptor of
@@ -104,6 +107,7 @@ def to_designspace(
         minimize_glyphs_diffs=minimize_glyphs_diffs,
         generate_GDEF=generate_GDEF,
         store_editor_state=store_editor_state,
+        write_skipexportglyphs=write_skipexportglyphs,
     )
     return builder.designspace
 
diff --git a/Lib/glyphsLib/builder/builders.py b/Lib/glyphsLib/builder/builders.py
index ce2075ca..3342e674 100644
--- a/Lib/glyphsLib/builder/builders.py
+++ b/Lib/glyphsLib/builder/builders.py
@@ -58,6 +58,7 @@ class UFOBuilder(_LoggerMixin):
         minimize_glyphs_diffs=False,
         generate_GDEF=True,
         store_editor_state=True,
+        write_skipexportglyphs=False,
     ):
         """Create a builder that goes from Glyphs to UFO + designspace.
 
@@ -82,6 +83,10 @@ class UFOBuilder(_LoggerMixin):
                          the UFO features.
         store_editor_state -- If True, store editor state in the UFO like which
                               glyphs are open in which tabs ("DisplayStrings").
+        write_skipexportglyphs -- If True, write the export status of a glyph
+                                         into the UFOs' and Designspace's lib instead
+                                         of the glyph level lib key
+                                         "com.schriftgestaltung.Glyphs.Export".
         """
         self.font = font
         self.ufo_module = ufo_module
@@ -93,6 +98,7 @@ class UFOBuilder(_LoggerMixin):
         self.generate_GDEF = generate_GDEF
         self.store_editor_state = store_editor_state
         self.bracket_layers = []
+        self.write_skipexportglyphs = write_skipexportglyphs
 
         # The set of (SourceDescriptor + UFO)s that will be built,
         # indexed by master ID, the same order as masters in the source GSFont.
@@ -208,6 +214,18 @@ class UFOBuilder(_LoggerMixin):
             for layer in ufo.layers:
                 self.to_ufo_layer_lib(layer)
 
+        if self.write_skipexportglyphs:
+            # Sanitize skip list and write it to both Designspace- and UFO-level lib
+            # keys. The latter is unnecessary when using e.g. the ufo2ft.compile*FromDS`
+            # functions, but the data may take a different path. Writing it everywhere
+            # can save on surprises/logic in other software.
+            skip_export_glyphs = self._designspace.lib.get("public.skipExportGlyphs")
+            if skip_export_glyphs is not None:
+                skip_export_glyphs = sorted(set(skip_export_glyphs))
+                self._designspace.lib["public.skipExportGlyphs"] = skip_export_glyphs
+                for source in self._sources.values():
+                    source.font.lib["public.skipExportGlyphs"] = skip_export_glyphs
+
         self.to_ufo_features()  # This depends on the glyphOrder key
         self.to_ufo_groups()
         self.to_ufo_kerning()
@@ -494,6 +512,13 @@ class GlyphsBuilder(_LoggerMixin):
         else:
             raise RuntimeError("Please provide a designspace or at least one UFO.")
 
+        if "public.skipExportGlyphs" in self.designspace.lib:
+            self.skip_export_glyphs = set(
+                self.designspace.lib["public.skipExportGlyphs"]
+            )
+        else:
+            self.skip_export_glyphs = set()
+
         self._font = None
         """The GSFont that will be built."""
 
@@ -669,6 +694,25 @@ class GlyphsBuilder(_LoggerMixin):
             source.path = ufo.path
             source.location = ufo_to_location[ufo]
             designspace.addSource(source)
+
+        # UFO-level skip list lib keys are usually ignored, except when we don't have a
+        # Designspace file to start from. If they exist in the UFOs, promote them to a
+        # Designspace-level lib key. However, to avoid accidents, expect the list to
+        # exist in none or be the same in all UFOs.
+        if any("public.skipExportGlyphs" in ufo.lib for ufo in ufos):
+            skip_export_glyphs = {
+                frozenset(ufo.lib.get("public.skipExportGlyphs", [])) for ufo in ufos
+            }
+            if len(skip_export_glyphs) == 1:
+                designspace.lib["public.skipExportGlyphs"] = sorted(
+                    next(iter(skip_export_glyphs))
+                )
+            else:
+                raise ValueError(
+                    "The `public.skipExportGlyphs` list of all UFOs must either not "
+                    "exist or be the same in every UFO."
+                )
+
         return designspace
 
     # Implementation is split into one file per feature
diff --git a/Lib/glyphsLib/builder/features.py b/Lib/glyphsLib/builder/features.py
index deaee55d..65cefb02 100644
--- a/Lib/glyphsLib/builder/features.py
+++ b/Lib/glyphsLib/builder/features.py
@@ -96,7 +96,9 @@ def _to_ufo_features(self, master, ufo):
                 "The features already contain a `table GDEF {...}` statement. "
                 "Either delete it or set generate_GDEF to False."
             )
-        gdef_str = _build_gdef(ufo)
+        gdef_str = _build_gdef(
+            ufo, self._designspace.lib.get("public.skipExportGlyphs")
+        )
 
     # make sure feature text is a unicode string, for defcon
     full_text = (
@@ -105,7 +107,7 @@ def _to_ufo_features(self, master, ufo):
     ufo.features.text = full_text if full_text.strip() else ""
 
 
-def _build_gdef(ufo):
+def _build_gdef(ufo, skipExportGlyphs=None):
     """Build a GDEF table statement (GlyphClassDef and LigatureCaretByPos).
 
     Building GlyphClassDef requires anchor propagation or user care to work as
@@ -132,6 +134,12 @@ def _build_gdef(ufo):
     subCategory_key = GLYPHLIB_PREFIX + "subCategory"
 
     for glyph in ufo:
+        # Do not generate any entries for non-export glyphs, as looking them up on
+        # compilation will fail.
+        if skipExportGlyphs is not None:
+            if glyph.name in skipExportGlyphs:
+                continue
+
         has_attaching_anchor = False
         for anchor in glyph.anchors:
             name = anchor.name
diff --git a/Lib/glyphsLib/builder/glyph.py b/Lib/glyphsLib/builder/glyph.py
index 3bbd0e80..b47205e6 100644
--- a/Lib/glyphsLib/builder/glyph.py
+++ b/Lib/glyphsLib/builder/glyph.py
@@ -65,7 +65,12 @@ def to_ufo_glyph(self, ufo_glyph, layer, glyph):
 
     export = glyph.export
     if not export:
-        ufo_glyph.lib[GLYPHLIB_PREFIX + "Export"] = export
+        if self.write_skipexportglyphs:
+            if "public.skipExportGlyphs" not in self._designspace.lib:
+                self._designspace.lib["public.skipExportGlyphs"] = []
+            self._designspace.lib["public.skipExportGlyphs"].append(glyph.name)
+        else:
+            ufo_glyph.lib[GLYPHLIB_PREFIX + "Export"] = export
 
     # FIXME: (jany) next line should be an API of GSGlyph?
     glyphinfo = glyphsLib.glyphdata.get_glyph(ufo_glyph.name)
@@ -147,9 +152,7 @@ def to_glyphs_glyph(self, ufo_glyph, ufo_layer, master):
 
     if ufo_glyph.unicodes:
         glyph.unicodes = ["{:04X}".format(c) for c in ufo_glyph.unicodes]
-    note = ufo_glyph.note
-    if note is not None:
-        glyph.note = note
+    glyph.note = ufo_glyph.note or ""
     if GLYPHLIB_PREFIX + "lastChange" in ufo_glyph.lib:
         last_change = ufo_glyph.lib[GLYPHLIB_PREFIX + "lastChange"]
         # We cannot be strict about the dateformat because it's not an official
@@ -157,8 +160,15 @@ def to_glyphs_glyph(self, ufo_glyph, ufo_layer, master):
         glyph.lastChange = from_loose_ufo_time(last_change)
     if ufo_glyph.markColor:
         glyph.color = _to_glyphs_color(ufo_glyph.markColor)
+
+    # The export flag can be stored in the glyph's lib key (for upgrading legacy
+    # sources) or the Designspace-level public.skipExportGlyphs lib key (canonical
+    # place to store the information). The UFO level lib key is ignored.
     if GLYPHLIB_PREFIX + "Export" in ufo_glyph.lib:
         glyph.export = ufo_glyph.lib[GLYPHLIB_PREFIX + "Export"]
+    if ufo_glyph.name in self.skip_export_glyphs:
+        glyph.export = False
+
     ps_names_key = PUBLIC_PREFIX + "postscriptNames"
     if (
         ps_names_key in ufo_glyph.font.lib
diff --git a/Lib/glyphsLib/cli.py b/Lib/glyphsLib/cli.py
index 09690000..8fcc7ebf 100644
--- a/Lib/glyphsLib/cli.py
+++ b/Lib/glyphsLib/cli.py
@@ -114,6 +114,15 @@ def main(args=None):
             "in which tab (DisplayStrings)."
         ),
     )
+    group.add_argument(
+        "--write-public-skip-export-glyphs",
+        action="store_true",
+        help=(
+            "Store the glyph export flag in the `public.skipExportGlyphs` list "
+            "instead of the glyph-level 'com.schriftgestaltung.Glyphs.Export' lib "
+            "key."
+        ),
+    )
 
     parser_ufo2glyphs = subparsers.add_parser("ufo2glyphs", help=ufo2glyphs.__doc__)
     parser_ufo2glyphs.set_defaults(func=ufo2glyphs)
@@ -188,6 +197,7 @@ def glyphs2ufo(options):
         create_background_layers=options.create_background_layers,
         generate_GDEF=options.generate_GDEF,
         store_editor_state=not options.no_store_editor_state,
+        write_skipexportglyphs=options.write_public_skip_export_glyphs,
     )
 
 
diff --git a/tox.ini b/tox.ini
index d780c9dc..e839b49e 100644
--- a/tox.ini
+++ b/tox.ini
@@ -43,5 +43,5 @@ commands =
 select = B,C,E,F,W,T4,B9
 ignore = E203,E266,E501,W503
 max-line-length = 80
-max-complexity = 18
+max-complexity = 19
 exclude = .git,__pycache__,build,dist,.eggs,.tox


Test patch:
diff --git a/tests/builder/builder_test.py b/tests/builder/builder_test.py
index 425df4fe..8fa0b159 100644
--- a/tests/builder/builder_test.py
+++ b/tests/builder/builder_test.py
@@ -836,17 +836,171 @@ class ToUfosTest(unittest.TestCase):
     def test_glyph_lib_Export(self):
         font = generate_minimal_font()
         glyph = add_glyph(font, "a")
-
         self.assertEqual(glyph.export, True)
 
         ufo = to_ufos(font)[0]
+        ds = to_designspace(font)
 
         self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo["a"].lib)
+        self.assertNotIn("public.skipExportGlyphs", ufo.lib)
+        self.assertNotIn("public.skipExportGlyphs", ds.lib)
 
-        glyph.export = False
-        ufo = to_ufos(font)[0]
+        font2 = to_glyphs(ds)
+        self.assertEqual(font2.glyphs["a"].export, True)
+
+        font2.glyphs["a"].export = False
+
+        # Test write_skipexportglyphs=True
+        ufo = to_ufos(font2, write_skipexportglyphs=True)[0]
+        ds = to_designspace(font2, write_skipexportglyphs=True)
+
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo["a"].lib)
+        self.assertEqual(ufo.lib["public.skipExportGlyphs"], ["a"])
+        self.assertEqual(ds.lib["public.skipExportGlyphs"], ["a"])
+
+        font3 = to_glyphs(ds)
+        self.assertEqual(font3.glyphs["a"].export, False)
+
+        # Test write_skipexportglyphs=False
+        ufo = to_ufos(font2, write_skipexportglyphs=False)[0]
+        ds = to_designspace(font2, write_skipexportglyphs=False)
+
+        self.assertFalse(ufo["a"].lib[GLYPHLIB_PREFIX + "Export"])
+        self.assertNotIn("public.skipExportGlyphs", ufo.lib)
+        self.assertNotIn("public.skipExportGlyphs", ds.lib)
+
+        font3 = to_glyphs(ds)
+        self.assertEqual(font3.glyphs["a"].export, False)
+
+    def test_glyph_lib_Export_mixed_to_public_skipExportGlyphs(self):
+        font = generate_minimal_font()
+        add_glyph(font, "a")
+        add_glyph(font, "b")
+        add_glyph(font, "c")
+        add_glyph(font, "d")
+        ds = to_designspace(font, write_skipexportglyphs=True)
+        ufo = ds.sources[0].font
+
+        ufo["a"].lib[GLYPHLIB_PREFIX + "Export"] = False
+        ufo.lib["public.skipExportGlyphs"] = ["b"]
+        ds.lib["public.skipExportGlyphs"] = ["c"]
+
+        font2 = to_glyphs(ds)
+
+        ds2 = to_designspace(font2, write_skipexportglyphs=True)
+        ufo2 = ds2.sources[0].font
+
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo2["a"].lib)
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo2["b"].lib)
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo2["c"].lib)
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo2["d"].lib)
+        self.assertEqual(ufo2.lib["public.skipExportGlyphs"], ["a", "c"])
+        self.assertEqual(ds2.lib["public.skipExportGlyphs"], ["a", "c"])
+
+        font3 = to_glyphs(ds2)
+        self.assertEqual(font3.glyphs["a"].export, False)
+        self.assertEqual(font3.glyphs["b"].export, True)
+        self.assertEqual(font3.glyphs["c"].export, False)
+        self.assertEqual(font3.glyphs["d"].export, True)
+
+        ufos3 = to_ufos(font3, write_skipexportglyphs=True)
+        ufo3 = ufos3[0]
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo3["a"].lib)
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo3["b"].lib)
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo3["c"].lib)
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo3["d"].lib)
+        self.assertEqual(ufo3.lib["public.skipExportGlyphs"], ["a", "c"])
+
+    def test_glyph_lib_Export_mixed_to_lib_key(self):
+        font = generate_minimal_font()
+        add_glyph(font, "a")
+        add_glyph(font, "b")
+        add_glyph(font, "c")
+        add_glyph(font, "d")
+        ds = to_designspace(font, write_skipexportglyphs=False)
+        ufo = ds.sources[0].font
+
+        ufo["a"].lib[GLYPHLIB_PREFIX + "Export"] = False
+        ufo.lib["public.skipExportGlyphs"] = ["b"]
+        ds.lib["public.skipExportGlyphs"] = ["c"]
+
+        font2 = to_glyphs(ds)
+
+        ds2 = to_designspace(font2, write_skipexportglyphs=False)
+        ufo2 = ds2.sources[0].font
+
+        self.assertFalse(ufo2["a"].lib[GLYPHLIB_PREFIX + "Export"])
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo2["b"].lib)
+        self.assertFalse(ufo2["c"].lib[GLYPHLIB_PREFIX + "Export"])
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo2["d"].lib)
+        self.assertNotIn("public.skipExportGlyphs", ufo2.lib)
+        self.assertNotIn("public.skipExportGlyphs", ds2.lib)
+
+        font3 = to_glyphs(ds2)
+        self.assertEqual(font3.glyphs["a"].export, False)
+        self.assertEqual(font3.glyphs["b"].export, True)
+        self.assertEqual(font3.glyphs["c"].export, False)
+        self.assertEqual(font3.glyphs["d"].export, True)
+
+        ufos3 = to_ufos(font3, write_skipexportglyphs=False)
+        ufo3 = ufos3[0]
+        self.assertFalse(ufo3["a"].lib[GLYPHLIB_PREFIX + "Export"])
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo3["b"].lib)
+        self.assertFalse(ufo3["c"].lib[GLYPHLIB_PREFIX + "Export"])
+        self.assertNotIn(GLYPHLIB_PREFIX + "Export", ufo3["d"].lib)
+        self.assertNotIn("public.skipExportGlyphs", ufo3.lib)
+
+        font4 = to_glyphs(ufos3)
+        self.assertEqual(font4.glyphs["a"].export, False)
+        self.assertEqual(font4.glyphs["b"].export, True)
+        self.assertEqual(font4.glyphs["c"].export, False)
+        self.assertEqual(font4.glyphs["d"].export, True)
+
+    def test_glyph_lib_Export_GDEF(self):
+        font = generate_minimal_font()
+        add_glyph(font, "a")
+        add_glyph(font, "d")
+        add_anchor(font, "d", "top", 100, 100)
+
+        ds = to_designspace(font, write_skipexportglyphs=True)
+        ufo = ds.sources[0].font
+        self.assertIn(
+            "GlyphClassDef[d]", ufo.features.text.replace("\n", "").replace(" ", "")
+        )
+
+        font.glyphs["d"].export = False
+        ds2 = to_designspace(font, write_skipexportglyphs=True)
+        ufo2 = ds2.sources[0].font
+        self.assertEqual(ufo2.features.text, "")
+
+    def test_glyph_lib_Export_fake_designspace(self):
+        font = generate_minimal_font()
+        master = GSFontMaster()
+        master.ascender = 0
+        master.capHeight = 0
+        master.descender = 0
+        master.id = "id"
+        master.xHeight = 0
+        font.masters.append(master)
+        add_glyph(font, "a")
+        add_glyph(font, "b")
+        ds = to_designspace(font, write_skipexportglyphs=True)
+
+        ufos = [source.font for source in ds.sources]
+
+        font2 = to_glyphs(ufos)
+        ds2 = to_designspace(font2, write_skipexportglyphs=True)
+        self.assertNotIn("public.skipExportGlyphs", ds2.lib)
+
+        ufos[0].lib["public.skipExportGlyphs"] = ["a"]
+
+        with self.assertRaises(ValueError):
+            to_glyphs(ufos)
 
-        self.assertEqual(ufo["a"].lib[GLYPHLIB_PREFIX + "Export"], False)
+        ufos[1].lib["public.skipExportGlyphs"] = ["a"]
+        font3 = to_glyphs(ufos)
+        self.assertEqual(font3.glyphs["a"].export, False)
+        self.assertEqual(font3.glyphs["b"].export, True)
 
     def test_glyph_lib_metricsKeys(self):
         font = generate_minimal_font()