Repository: googlefonts/glyphsLib
Pull request #538 (2019-07-26 17:40:48)

[bracket layers] support reverse, unbalanced and composite glyphs
Prompted by https://github.com/googlefonts/glyphsLib/issues/535, I decided to work on completing the bracket layers support started in https://github.com/googlefonts/glyphsLib/issues/468.

This adds support for
- "reverse" bracket layers (e.g. `]500]`) where the location represent the range's maximum, as opposed to regular bracket layers (`[500]`) where value indicates a minimum. 
- what Nikolaus called "unbalanced" bracket layers, i.e. when only some of master layers have explicilty defined bracket layers. The missing master layers are implicitly duplicated as bracket layers.
- composite glyphs with bracket layers that match the definition of their components' bracket layers (#535): the components' base glyphs need to be adjusted accordingly.

The PR is marked as WIP because next week I want to refine and add more specific tests for the new features.


Patch:
diff --git a/Lib/glyphsLib/builder/builders.py b/Lib/glyphsLib/builder/builders.py
index 93b3985d..0e288df4 100644
--- a/Lib/glyphsLib/builder/builders.py
+++ b/Lib/glyphsLib/builder/builders.py
@@ -15,8 +15,10 @@
 from __future__ import print_function, division, absolute_import, unicode_literals
 
 from collections import OrderedDict, defaultdict
+from functools import partial
 import logging
 import os
+import re
 from textwrap import dedent
 
 import defcon
@@ -29,6 +31,15 @@ from .axes import WEIGHT_AXIS_DEF, WIDTH_AXIS_DEF, find_base_style, class_to_val
 
 GLYPH_ORDER_KEY = PUBLIC_PREFIX + "glyphOrder"
 
+BRACKET_LAYER_RE = re.compile(r".*(?P<first_bracket>[\[\]])\s*(?P<value>\d+)\s*\].*")
+BRACKET_GLYPH_TEMPLATE = "{glyph_name}.{rev}BRACKET.{location}"
+REVERSE_BRACKET_LABEL = "REV_"
+BRACKET_GLYPH_RE = re.compile(
+    r"(?P<glyph_name>.+)\.(?P<rev>{})?BRACKET\.(?P<location>\d+)$".format(
+        REVERSE_BRACKET_LABEL
+    )
+)
+
 
 class _LoggerMixin(object):
 
@@ -195,10 +206,12 @@ class UFOBuilder(_LoggerMixin):
                 continue
 
             # Save processing bracket layers for when designspace() is called, as we
-            # have to extract them to free-standing glyphs.
+            # have to extract them to free-standing glyphs -- unless the parent glyph is
+            # set to non-export (in which case makes no sense to have Designspace rules
+            # referencing non existent glyphs).
             if (
-                "[" in layer.name
-                and "]" in layer.name
+                BRACKET_LAYER_RE.match(layer.name)
+                and glyph.export
                 and ".background" not in layer.name
             ):
                 self.bracket_layers.append(layer)
@@ -319,109 +332,138 @@ class UFOBuilder(_LoggerMixin):
             bracket_axis_min = bracket_axis.minimum
             bracket_axis_max = bracket_axis.maximum
 
-        # 1. bracket_layer_map: Organize all bracket layers by crossover value, so
-        #    we can go through the layers by location and copy them to free-standing
-        #    glyphs.
-        # 2. glyph_crossovers: Keep track of the crossover values of a single glyph, so
-        #    we can easily sort them into rule buckets.
-        # 3. glyph_sanity_counter: Count the number of master layers providing
-        #    bracket layers per glyph and crossover value. We currently only support
-        #    the situation where there is a bracket layer for _all_ masters, what the
-        #    Glyphs.app tutorial calls 'Changing All Masters'.
-        bracket_layer_map = defaultdict(list)
-        glyph_crossovers = defaultdict(set)
-        glyph_sanity_counter = defaultdict(list)
+        # Organize all bracket layers by glyph name and crossover value, so later we
+        # can go through the layers by location and copy them to free-standing glyphs
+        bracket_layer_map = defaultdict(partial(defaultdict, list))
         for layer in self.bracket_layers:
             glyph_name = layer.parent.name
-            n = layer.name
-            try:
-                bracket_crossover = int(n[n.index("[") + 1 : n.index("]")])
-            except ValueError:
-                raise ValueError(
-                    "Only bracket layers with one numerical (design space) location "
-                    "(meaning the first axis in the designspace file) are currently "
-                    "supported."
-                )
+
+            m = BRACKET_LAYER_RE.match(layer.name)
+            assert m
+
+            reverse = m.group("first_bracket") == "]"
+            bracket_crossover = int(m.group("value"))
+
             if not bracket_axis_min <= bracket_crossover <= bracket_axis_max:
                 raise ValueError(
                     "Glyph {glyph_name}: Bracket layer {layer_name} must be within the "
                     "design space bounds of the {bracket_axis_name} axis: minimum "
                     "{bracket_axis_minimum}, maximum {bracket_axis_maximum}.".format(
                         glyph_name=glyph_name,
-                        layer_name=n,
+                        layer_name=layer.name,
                         bracket_axis_name=bracket_axis.name,
                         bracket_axis_minimum=bracket_axis_min,
                         bracket_axis_maximum=bracket_axis_max,
                     )
                 )
-            bracket_layer_map[bracket_crossover].append(layer)
-            glyph_crossovers[glyph_name].add(bracket_crossover)
-            glyph_sanity_counter[(glyph_name, bracket_crossover)].append(
-                layer.associatedMasterId
-            )
-
-        # Check that each bracket layer is present in all master layers.
-        unbalanced_bracket_layers = []
-        n_masters = len(list(self.masters))
-        for ((glyph_name, _), master_layer_ids) in glyph_sanity_counter.items():
-            if not len(master_layer_ids) == n_masters:
-                unbalanced_bracket_layers.append(glyph_name)
-        if unbalanced_bracket_layers:
-            raise ValueError(
-                "Currently, we only support bracket layers that are present on all "
-                "masters, i.e. what the Glyphs.app tutorial calls 'Changing All "
-                "Masters'. There is a/are bracket layer(s) missing "
-                "for glyph(s) {unbalanced_glyphs}.".format(
-                    unbalanced_glyphs=unbalanced_bracket_layers
+            bracket_layer_map[glyph_name][(bracket_crossover, reverse)].append(layer)
+
+        # Sort crossovers into rule buckets, one for regular bracket layers (in which
+        # the location represents the min value) and one for 'reverse' bracket layers
+        # (in which the location is the max value).
+        max_rule_bucket = defaultdict(list)
+        min_rule_bucket = defaultdict(list)
+        for glyph_name, glyph_bracket_layers in sorted(bracket_layer_map.items()):
+            min_crossovers = set()
+            max_crossovers = set()
+            for location, reverse in glyph_bracket_layers.keys():
+                if reverse:
+                    max_crossovers.add(location)
+                else:
+                    min_crossovers.add(location)
+            # reverse and non-reverse bracket layers with overlapping ranges are
+            # tricky to implement as DS rules. They are relatively unlikely, and
+            # can usually be rewritten so that they do not overlap. For laziness/
+            # simplicity, where we simply warn that output may not be as expected.
+            invalid_locs = [
+                (mx, mn)
+                for mx, mn in zip(sorted(max_crossovers), sorted(min_crossovers))
+                if mx > mn
+            ]
+            if invalid_locs:
+                self.logger.warning(
+                    "Bracket layers for glyph '%s' have overlapping ranges: %s",
+                    glyph_name,
+                    ", ".join("]{}] > [{}]".format(*values) for values in invalid_locs),
+                )
+            for crossover_min, crossover_max in util.pairwise(
+                [bracket_axis_min] + sorted(max_crossovers)
+            ):
+                max_rule_bucket[(int(crossover_min), int(crossover_max))].append(
+                    glyph_name
                 )
-            )
-
-        # Sort crossovers into buckets.
-        rule_bucket = defaultdict(list)
-        for glyph_name, crossovers in sorted(glyph_crossovers.items()):
             for crossover_min, crossover_max in util.pairwise(
-                sorted(crossovers) + [bracket_axis_max]
+                sorted(min_crossovers) + [bracket_axis_max]
             ):
-                rule_bucket[(int(crossover_min), int(crossover_max))].append(glyph_name)
+                min_rule_bucket[(int(crossover_min), int(crossover_max))].append(
+                    glyph_name
+                )
 
         # Generate rules for the bracket layers.
-        for (axis_range_min, axis_range_max), glyph_names in sorted(
-            rule_bucket.items()
-        ):
-            rule_name = "BRACKET.{}.{}".format(axis_range_min, axis_range_max)
-            glyph_sub_suffix = ".BRACKET.{}".format(axis_range_min)
-            rule = designspaceLib.RuleDescriptor()
-            rule.name = rule_name
-            rule.conditionSets.append(
-                [
-                    {
-                        "name": bracket_axis.name,
-                        "minimum": axis_range_min,
-                        "maximum": axis_range_max,
-                    }
-                ]
-            )
-            rule.subs.extend(
-                [
-                    (glyph_name, glyph_name + glyph_sub_suffix)
-                    for glyph_name in glyph_names
-                ]
-            )
-            self._designspace.addRule(rule)
+        for reverse, rule_bucket in ((True, max_rule_bucket), (False, min_rule_bucket)):
+            for (axis_range_min, axis_range_max), glyph_names in sorted(
+                rule_bucket.items()
+            ):
+                rule = _make_designspace_rule(
+                    glyph_names,
+                    bracket_axis.name,
+                    axis_range_min,
+                    axis_range_max,
+                    reverse,
+                )
+                self._designspace.addRule(rule)
 
         # Finally, copy bracket layers to their own glyphs.
-        for location, layers in bracket_layer_map.items():
-            for layer in layers:
-                ufo_font = self._sources[
-                    layer.associatedMasterId or layer.layerId
-                ].font.layers.defaultLayer
-                ufo_glyph_name = "{glyph_name}.BRACKET.{location}".format(
-                    glyph_name=layer.parent.name, location=location
-                )
-                ufo_glyph = ufo_font.newGlyph(ufo_glyph_name)
-                self.to_ufo_glyph(ufo_glyph, layer, layer.parent)
-                ufo_glyph.unicodes = []  # Avoid cmap interference
-                ufo_glyph.lib[GLYPHLIB_PREFIX + "_originalLayerName"] = layer.name
+        self._copy_bracket_layers_to_ufo_glyphs(bracket_layer_map)
+
+    def _copy_bracket_layers_to_ufo_glyphs(self, bracket_layer_map):
+        font = self.font
+        master_ids = {m.id for m in font.masters}
+        # when a glyph master layer doesn't have an explicitly associated bracket layer
+        # for any crosspoint locations, we assume the master layer itself will be
+        # used implicitly as bracket layer for that location. See "Switching Only One
+        # Master" paragraph in "Alternating Glyph Shapes" tutorial at:
+        # https://glyphsapp.com/tutorials/alternating-glyph-shapes
+        implicit_bracket_layers = set()
+        # collect all bracket glyph names for resolving composite references
+        bracket_glyphs = set()
+
+        for glyph_name, glyph_bracket_layers in bracket_layer_map.items():
+            glyph = font.glyphs[glyph_name]
+            for (location, reverse), bracket_layers in glyph_bracket_layers.items():
+
+                for missing_master_layer_id in master_ids.difference(
+                    bl.associatedMasterId for bl in bracket_layers
+                ):
+                    master_layer = glyph.layers[missing_master_layer_id]
+                    bracket_layers.append(master_layer)
+                    implicit_bracket_layers.add(id(master_layer))
+
+                bracket_glyphs.add(_bracket_glyph_name(glyph_name, reverse, location))
+
+        for glyph_name, glyph_bracket_layers in bracket_layer_map.items():
+            for (location, reverse), layers in glyph_bracket_layers.items():
+                for layer in layers:
+                    ufo_font = self._sources[
+                        layer.associatedMasterId or layer.layerId
+                    ].font.layers.defaultLayer
+                    ufo_glyph_name = _bracket_glyph_name(glyph_name, reverse, location)
+                    ufo_glyph = ufo_font.newGlyph(ufo_glyph_name)
+                    self.to_ufo_glyph(ufo_glyph, layer, layer.parent)
+                    ufo_glyph.unicodes = []  # Avoid cmap interference
+                    # implicit bracket layers have no distinct name, they are simply
+                    # references to master layers; the empty string is a signal when
+                    # roundtripping back to Glyphs to skip the duplicate layers.
+                    ufo_glyph.lib[GLYPHLIB_PREFIX + "_originalLayerName"] = (
+                        "" if id(layer) in implicit_bracket_layers else layer.name
+                    )
+                    # swap components if base glyph contains matching bracket layers.
+                    for comp in ufo_glyph.components:
+                        bracket_comp_name = _bracket_glyph_name(
+                            comp.baseGlyph, reverse, location
+                        )
+                        if bracket_comp_name in bracket_glyphs:
+                            comp.baseGlyph = bracket_comp_name
 
     # Implementation is split into one file per feature
     from .anchors import to_ufo_propagate_font_anchors, to_ufo_glyph_anchors
@@ -456,6 +498,28 @@ class UFOBuilder(_LoggerMixin):
     )
 
 
+def _bracket_glyph_name(glyph_name, reverse, location):
+    return BRACKET_GLYPH_TEMPLATE.format(
+        glyph_name=glyph_name,
+        rev=REVERSE_BRACKET_LABEL if reverse else "",
+        location=location,
+    )
+
+
+def _make_designspace_rule(glyph_names, axis_name, range_min, range_max, reverse=False):
+    rule_name = "BRACKET.{}.{}".format(range_min, range_max)
+    rule = designspaceLib.RuleDescriptor()
+    rule.name = rule_name
+    rule.conditionSets.append(
+        [{"name": axis_name, "minimum": range_min, "maximum": range_max}]
+    )
+    location = range_max if reverse else range_min
+    for glyph_name in glyph_names:
+        sub_glyph_name = _bracket_glyph_name(glyph_name, reverse, location)
+        rule.subs.append((glyph_name, sub_glyph_name))
+    return rule
+
+
 def filter_instances_by_family(instances, family_name=None):
     """Yield instances whose 'familyName' custom parameter is
     equal to 'family_name'.
@@ -542,7 +606,7 @@ class GlyphsBuilder(_LoggerMixin):
                 source.font.lib[GLYPH_ORDER_KEY] = [
                     glyph_name
                     for glyph_name in source.font.lib[GLYPH_ORDER_KEY]
-                    if ".BRACKET." not in glyph_name
+                    if not BRACKET_GLYPH_RE.match(glyph_name)
                 ]
 
             self.to_glyphs_font_attributes(source, master, is_initial=(index == 0))
@@ -552,29 +616,36 @@ class GlyphsBuilder(_LoggerMixin):
 
             # First, move free-standing bracket glyphs back to layers to avoid dealing
             # with GSLayer transplantation.
-            for bracket_glyph in [g for g in source.font if ".BRACKET." in g.name]:
-                base_glyph, threshold = bracket_glyph.name.split(".BRACKET.")
-                try:
-                    int(threshold)
-                except ValueError:
-                    raise ValueError(
-                        "Glyph '{}' has malformed bracket layer name. Must be '<glyph "
-                        "name>.BRACKET.<crossover value>'.".format(bracket_glyph)
-                    )
+            for glyph_name in list(source.font.keys()):
+                m = BRACKET_GLYPH_RE.match(glyph_name)
+                if not m:
+                    continue
+                bracket_glyph = source.font[glyph_name]
+                base_glyph, reverse, threshold = m.groups()
                 layer_name = bracket_glyph.lib.get(
-                    GLYPHLIB_PREFIX + "_originalLayerName", "[{}]".format(threshold)
+                    GLYPHLIB_PREFIX + "_originalLayerName",
+                    "{}{}]".format("]" if reverse else "[", threshold),
                 )
-                if layer_name not in source.font.layers:
-                    ufo_layer = source.font.newLayer(layer_name)
-                else:
-                    ufo_layer = source.font.layers[layer_name]
-                bracket_glyph_new = ufo_layer.newGlyph(base_glyph)
-                bracket_glyph_new.copyDataFromGlyph(bracket_glyph)
+                # _originalLayerName is an empty string for 'implicit' bracket layers;
+                # we don't import these since they were copies of master layers.
+                if layer_name:
+                    if layer_name not in source.font.layers:
+                        ufo_layer = source.font.newLayer(layer_name)
+                    else:
+                        ufo_layer = source.font.layers[layer_name]
+                    bracket_glyph_new = ufo_layer.newGlyph(base_glyph)
+                    bracket_glyph_new.copyDataFromGlyph(bracket_glyph)
+
+                    # strip '*.BRACKET.123' suffix from the components' glyph names
+                    for comp in bracket_glyph_new.components:
+                        m = BRACKET_GLYPH_RE.match(comp.baseGlyph)
+                        if m:
+                            comp.baseGlyph = m.group("glyph_name")
 
                 # Remove all freestanding bracket layer glyphs from all layers.
                 for layer in source.font.layers:
-                    if bracket_glyph.name in layer:
-                        del layer[bracket_glyph.name]
+                    if glyph_name in layer:
+                        del layer[glyph_name]
 
             for layer in _sorted_backgrounds_last(source.font.layers):
                 self.to_glyphs_layer_lib(layer)
diff --git a/Lib/glyphsLib/builder/custom_params.py b/Lib/glyphsLib/builder/custom_params.py
index 2eeb126e..8e3cfa15 100644
--- a/Lib/glyphsLib/builder/custom_params.py
+++ b/Lib/glyphsLib/builder/custom_params.py
@@ -27,7 +27,7 @@ from .constants import (
     REVERSE_CODEPAGE_RANGES,
     PUBLIC_PREFIX,
 )
-from .features import replace_feature, replace_prefixes
+from .features import replace_feature
 
 """Set Glyphs custom parameters in UFO info or lib, where appropriate.
 
@@ -671,34 +671,6 @@ class ReplaceFeatureParamHandler(AbstractParamHandler):
 register(ReplaceFeatureParamHandler())
 
 
-class ReplacePrefixParamHandler(AbstractParamHandler):
-    def to_ufo(self, builder, glyphs, ufo):
-        repl_map = {}
-        for value in glyphs.get_custom_values("Replace Prefix"):
-            prefix_name, prefix_code = re.split(r"\s*;\s*", value, 1)
-            # if multiple 'Replace Prefix' custom params replace the same
-            # prefix, the last wins
-            repl_map[prefix_name] = prefix_code
-
-        features_text = ufo._owner.features.text
-
-        if not (repl_map and features_text):
-            return
-
-        glyph_names = set(ufo._owner.keys())
-
-        ufo._owner.features.text = replace_prefixes(
-            repl_map, features_text, glyph_names=glyph_names
-        )
-
-    def to_glyphs(self, glyphs, ufo):
-        # do the same as ReplaceFeatureParamHandler.to_glyphs
-        pass
-
-
-register(ReplacePrefixParamHandler())
-
-
 class ReencodeGlyphsParamHandler(AbstractParamHandler):
     """ The "Reencode Glyphs" custom parameter contains a list of
     'glyphname=unicodevalue' strings: e.g., ["smiley=E100", "logo=E101"].
diff --git a/Lib/glyphsLib/builder/features.py b/Lib/glyphsLib/builder/features.py
index bc719eb6..65cefb02 100644
--- a/Lib/glyphsLib/builder/features.py
+++ b/Lib/glyphsLib/builder/features.py
@@ -36,37 +36,20 @@ def autostr(automatic):
 def to_ufo_features(self):
     for master_id, source in self._sources.items():
         master = self.font.masters[master_id]
-        ufo = source.font
-
-        # Recover the original feature code if it was stored in the user data
-        original = master.userData[ORIGINAL_FEATURE_CODE_KEY]
-        if original is not None:
-            ufo.features.text = original
-        else:
-            skip_export_glyphs = self._designspace.lib.get("public.skipExportGlyphs")
-            ufo.features.text = _to_ufo_features(
-                self.font,
-                ufo,
-                generate_GDEF=self.generate_GDEF,
-                skip_export_glyphs=skip_export_glyphs,
-            )
+        _to_ufo_features(self, master, source.font)
 
 
-def _to_ufo_features(font, ufo=None, generate_GDEF=False, skip_export_glyphs=None):
-    """Convert GSFont features, including prefixes and classes, to UFO.
+def _to_ufo_features(self, master, ufo):
+    """Write an UFO's OpenType feature file."""
 
-    Optionally, build a GDEF table definiton, excluding 'skip_export_glyphs'.
-
-    Args:
-        font: GSFont
-        ufo: Optional[defcon.Font]
-        generate_GDEF: bool
-        skip_export_glyphs: Optional[List[str]]
+    # Recover the original feature code if it was stored in the user data
+    original = master.userData[ORIGINAL_FEATURE_CODE_KEY]
+    if original is not None:
+        ufo.features.text = original
+        return
 
-    Returns: str
-    """
     prefixes = []
-    for prefix in font.featurePrefixes:
+    for prefix in self.font.featurePrefixes:
         strings = []
         if prefix.name != ANONYMOUS_FEATURE_PREFIX_NAME:
             strings.append("# Prefix: %s\n" % prefix.name)
@@ -77,7 +60,7 @@ def _to_ufo_features(font, ufo=None, generate_GDEF=False, skip_export_glyphs=Non
     prefix_str = "\n\n".join(prefixes)
 
     class_defs = []
-    for class_ in font.classes:
+    for class_ in self.font.classes:
         prefix = "@" if not class_.name.startswith("@") else ""
         name = prefix + class_.name
         class_defs.append(
@@ -86,7 +69,7 @@ def _to_ufo_features(font, ufo=None, generate_GDEF=False, skip_export_glyphs=Non
     class_str = "\n\n".join(class_defs)
 
     feature_defs = []
-    for feature in font.features:
+    for feature in self.font.features:
         code = feature.code
         lines = ["feature %s {" % feature.name]
         if feature.notes:
@@ -107,20 +90,21 @@ def _to_ufo_features(font, ufo=None, generate_GDEF=False, skip_export_glyphs=Non
     # results, we would need anchor propagation or user intervention. Glyphs.app
     # only generates it on generating binaries.
     gdef_str = None
-    if generate_GDEF:
-        assert ufo is not None
+    if self.generate_GDEF:
         if re.search(r"^\s*table\s+GDEF\s+{", prefix_str, flags=re.MULTILINE):
             raise ValueError(
                 "The features already contain a `table GDEF {...}` statement. "
                 "Either delete it or set generate_GDEF to False."
             )
-        gdef_str = _build_gdef(ufo, skip_export_glyphs)
+        gdef_str = _build_gdef(
+            ufo, self._designspace.lib.get("public.skipExportGlyphs")
+        )
 
     # make sure feature text is a unicode string, for defcon
     full_text = (
         "\n\n".join(filter(None, [class_str, prefix_str, fea_str, gdef_str])) + "\n"
     )
-    return full_text if full_text.strip() else ""
+    ufo.features.text = full_text if full_text.strip() else ""
 
 
 def _build_gdef(ufo, skipExportGlyphs=None):
@@ -216,32 +200,6 @@ def replace_feature(tag, repl, features):
     )
 
 
-def replace_prefixes(repl_map, features_text, glyph_names=None):
-    """Replace all '# Prefix: NAME' sections in features.
-
-    Args:
-        repl_map: Dict[str, str]: dictionary keyed by prefix name containing
-            feature code snippets to be replaced.
-        features_text: str: feature text to be parsed.
-        glyph_names: Optional[Sequence[str]]: list of valid glyph names, used
-            by feaLib Parser to distinguish glyph name tokens containing '-' from
-            glyph ranges such as 'a-z'.
-
-    Returns:
-        str: new feature text with replaced prefix paragraphs.
-    """
-    from glyphsLib.classes import GSFont
-
-    temp_font = GSFont()
-    _to_glyphs_features(temp_font, features_text, glyph_names=glyph_names)
-
-    for prefix in temp_font.featurePrefixes:
-        if prefix.name in repl_map:
-            prefix.code = repl_map[prefix.name]
-
-    return _to_ufo_features(temp_font)
-
-
 def to_glyphs_features(self):
     if not self.designspace.sources:
         # Needs at least one UFO
@@ -270,26 +228,9 @@ def to_glyphs_features(self):
     ufo = self.designspace.sources[0].font
     if ufo.features.text is None:
         return
-    _to_glyphs_features(
-        self.font,
-        ufo.features.text,
-        glyph_names=ufo.keys(),
-        glyphs_module=self.glyphs_module,
-    )
-
-
-def _to_glyphs_features(font, features_text, glyph_names=None, glyphs_module=None):
-    """Import features text in GSFont, split into prefixes, features and classes.
-
-    Args:
-        font: GSFont
-        feature_text: str
-        glyph_names: Optional[Sequence[str]]
-        glyphs_module: Optional[Any]
-    """
-    document = FeaDocument(features_text, glyph_names)
-    processor = FeatureFileProcessor(document, glyphs_module)
-    processor.to_glyphs(font)
+    document = FeaDocument(ufo.features.text, ufo.keys())
+    processor = FeatureFileProcessor(document, self.glyphs_module)
+    processor.to_glyphs(self.font)
 
 
 def _features_are_different_across_ufos(self):
@@ -333,12 +274,9 @@ def _to_glyphs_features_basic(self):
 class FeaDocument(object):
     """Parse the string of a fea code into statements."""
 
-    def __init__(self, text, glyph_set=None):
+    def __init__(self, text, glyph_set):
         feature_file = StringIO(text)
-        glyph_names = glyph_set if glyph_set is not None else ()
-        parser_ = parser.Parser(
-            feature_file, glyphNames=glyph_names, followIncludes=False
-        )
+        parser_ = parser.Parser(feature_file, glyph_set, followIncludes=False)
         self._doc = parser_.parse()
         self.statements = self._doc.statements
         self._lines = text.splitlines(True)  # keepends=True
@@ -458,10 +396,8 @@ class PeekableIterator(object):
 class FeatureFileProcessor(object):
     """Put fea statements into the correct fields of a GSFont."""
 
-    def __init__(self, doc, glyphs_module=None):
+    def __init__(self, doc, glyphs_module):
         self.doc = doc
-        if glyphs_module is None:
-            from glyphsLib import classes as glyphs_module
         self.glyphs_module = glyphs_module
         self.statements = PeekableIterator(doc.statements)
         self._font = None


Test patch:
diff --git a/tests/builder/custom_params_test.py b/tests/builder/custom_params_test.py
index cf229329..42b3c036 100644
--- a/tests/builder/custom_params_test.py
+++ b/tests/builder/custom_params_test.py
@@ -292,74 +292,6 @@ class SetCustomParamsTest(unittest.TestCase):
 
         self.assertEqual(self.ufo.features.text, original)
 
-    def test_replace_prefix(self):
-        self.ufo.features.text = dedent(
-            """\
-            # Prefix: AAA
-            include(../aaa.fea);
-
-            # Prefix: FOO
-            # foo
-
-            # Prefix: ZZZ
-            include(../zzz.fea);
-
-            # Prefix: BAR
-            # bar
-
-            feature liga {
-            sub f i by f_i;
-            } liga;
-
-            table GDEF {
-            GlyphClassDef
-                [f i], # Base
-                [f_i], # Liga
-                , # Mark
-                ;
-            } GDEF;
-            """
-        )
-
-        self.master.customParameters.append(
-            GSCustomParameter("Replace Prefix", "FOO; include(../foo.fea);")
-        )
-        self.master.customParameters.append(
-            GSCustomParameter("Replace Prefix", "BAR; include(../bar.fea);")
-        )
-        self.set_custom_params()
-
-        self.assertEqual(
-            self.ufo.features.text,
-            dedent(
-                """\
-                # Prefix: AAA
-                include(../aaa.fea);
-
-                # Prefix: FOO
-                include(../foo.fea);
-
-                # Prefix: ZZZ
-                include(../zzz.fea);
-
-                # Prefix: BAR
-                include(../bar.fea);
-
-                table GDEF {
-                GlyphClassDef
-                    [f i], # Base
-                    [f_i], # Liga
-                    , # Mark
-                    ;
-                } GDEF;
-
-                feature liga {
-                sub f i by f_i;
-                } liga;
-                """
-            ),
-        )
-
     def test_useProductionNames(self):
         for value in (True, False):
             self.master.customParameters["Don't use Production Names"] = value
diff --git a/tests/builder/designspace_gen_test.py b/tests/builder/designspace_gen_test.py
index df14092a..5e523258 100644
--- a/tests/builder/designspace_gen_test.py
+++ b/tests/builder/designspace_gen_test.py
@@ -18,6 +18,7 @@ from __future__ import print_function, division, absolute_import, unicode_litera
 import os
 from xmldiff import main, formatting
 
+import itertools
 import pytest
 import defcon
 
@@ -304,16 +305,105 @@ def test_designspace_generation_bracket_roundtrip_no_layername(datadir):
 
 
 def test_designspace_generation_bracket_unbalanced_brackets(datadir):
-    with open(str(datadir.join("BracketTestFont.glyphs"))) as f:
+    with open(str(datadir.join("BracketTestFont2.glyphs"))) as f:
+        font = glyphsLib.load(f)
+
+    layer_names = {l.name for l in font.glyphs["C"].layers}
+    assert layer_names == {"Regular", "Bold", "Bold [600]"}
+
+    designspace = to_designspace(font)
+
+    for source in designspace.sources:
+        assert "C.BRACKET.600" in source.font
+
+    font_rt = to_glyphs(designspace)
+
+    assert "C" in font_rt.glyphs
+
+    assert {l.name for l in font_rt.glyphs["C"].layers} == layer_names
+    assert "C.BRACKET.600" not in font_rt.glyphs
+
+
+def test_designspace_generation_bracket_composite_glyph(datadir):
+    with open(str(datadir.join("BracketTestFont2.glyphs"))) as f:
+        font = glyphsLib.load(f)
+
+    g = font.glyphs["B"]
+    for layer in g.layers:
+        assert layer.components[0].name == "A"
+
+    designspace = to_designspace(font)
+
+    for source in designspace.sources:
+        ufo = source.font
+        assert "B.BRACKET.600" in ufo
+        assert ufo["B"].components[0].baseGlyph == "A"
+        assert ufo["B.BRACKET.600"].components[0].baseGlyph == "A.BRACKET.600"
+
+    font_rt = to_glyphs(designspace)
+
+    assert "B" in font_rt.glyphs
+
+    g2 = font_rt.glyphs["B"]
+    for layer in g2.layers:
+        assert layer.components[0].name == "A"
+
+    assert "B.BRACKET.600" not in font_rt.glyphs
+
+
+def test_designspace_generation_reverse_bracket_roundtrip(datadir):
+    with open(str(datadir.join("BracketTestFont2.glyphs"))) as f:
         font = glyphsLib.load(f)
 
-    # Delete the "Other [600]" layer to unbalance bracket layers.
-    del font.glyphs["x"].layers["C5C3CA59-C2D0-46F6-B5D3-86541DE36ACB"]
-    with pytest.raises(ValueError, match=r"bracket layer\(s\) missing"):
-        to_designspace(font)
+    g = font.glyphs["D"]
 
-    # Delete the other [600] layers to rebalance.
-    del font.glyphs["x"].layers["E729A72D-C6FF-4DDD-ADA1-BB5B6FD7E3DD"]
-    del font.glyphs["x"].layers["F5778F4C-2B04-4030-9D7D-09E3C951C089"]
-    del font.glyphs["x"].layers["24328DA8-2CE1-4D0A-9C91-214ED36F6393"]
-    assert to_designspace(font)
+    assert {"Regular ]600]", "Bold ]600]"}.intersection(l.name for l in g.layers)
+
+    designspace = to_designspace(font)
+
+    assert designspace.rules[1].name == "BRACKET.400.600"
+    assert designspace.rules[1].conditionSets == [
+        [dict(name="Weight", minimum=400, maximum=600)]
+    ]
+    assert designspace.rules[1].subs == [("D", "D.REV_BRACKET.600")]
+
+    for source in designspace.sources:
+        ufo = source.font
+        assert "D.REV_BRACKET.600" in ufo
+
+    font_rt = to_glyphs(designspace)
+
+    assert "D" in font_rt.glyphs
+
+    g2 = font_rt.glyphs["D"]
+    assert {"Regular ]600]", "Bold ]600]"}.intersection(l.name for l in g2.layers)
+
+    assert "D.REV_BRACKET.600" not in font_rt.glyphs
+
+
+def test_designspace_generation_bracket_no_export_glyph(datadir):
+    with open(str(datadir.join("BracketTestFont2.glyphs"))) as f:
+        font = glyphsLib.load(f)
+
+    font.glyphs["E"].export = False
+
+    designspace = to_designspace(font, write_skipexportglyphs=True)
+
+    assert "E" in designspace.lib.get("public.skipExportGlyphs")
+
+    for source in designspace.sources:
+        assert "E.REV_BRACKET.570" not in source.font
+        assert "E.BRACKET.630" not in source.font
+
+    for rule in designspace.rules:
+        assert "E" not in {g for g in itertools.chain(*rule.subs)}
+
+    font_rt = to_glyphs(designspace)
+
+    assert "E" in font_rt.glyphs
+    assert {l.name for l in font_rt.glyphs["E"].layers} == {
+        "Regular",
+        "Regular [630]",
+        "Bold",
+        "Bold ]570]",
+    }
diff --git a/tests/data/BracketTestFont2.glyphs b/tests/data/BracketTestFont2.glyphs
new file mode 100644
index 00000000..66cd3bb8
--- /dev/null
+++ b/tests/data/BracketTestFont2.glyphs
@@ -0,0 +1,499 @@
+{
+.appVersion = "1230";
+DisplayStrings = (
+ABCDEF
+);
+customParameters = (
+{
+name = Axes;
+value = (
+{
+Name = Weight;
+Tag = wght;
+},
+{
+Name = Width;
+Tag = wdth;
+}
+);
+}
+);
+date = "2019-07-25 10:20:06 +0000";
+familyName = BracketTestFont2;
+fontMaster = (
+{
+ascender = 800;
+capHeight = 700;
+descender = -200;
+id = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+weightValue = 400;
+xHeight = 500;
+},
+{
+ascender = 800;
+capHeight = 700;
+descender = -200;
+id = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+weight = Bold;
+weightValue = 700;
+xHeight = 500;
+}
+);
+glyphs = (
+{
+glyphname = A;
+lastChange = "2019-07-29 12:07:03 +0000";
+layers = (
+{
+layerId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+paths = (
+{
+closed = 1;
+nodes = (
+"50 159 LINE",
+"202 159 LINE",
+"202 311 LINE",
+"50 311 LINE"
+);
+}
+);
+width = 252;
+},
+{
+layerId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+paths = (
+{
+closed = 1;
+nodes = (
+"50 77 LINE",
+"366 77 LINE",
+"366 393 LINE",
+"50 393 LINE"
+);
+}
+);
+width = 416;
+},
+{
+associatedMasterId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+layerId = "EC5C925B-6633-4542-A6D0-25A705C07D3A";
+name = "Regular [600]";
+paths = (
+{
+closed = 1;
+nodes = (
+"204 145 OFFCURVE",
+"248 189 OFFCURVE",
+"248 244 CURVE SMOOTH",
+"248 299 OFFCURVE",
+"204 343 OFFCURVE",
+"149 343 CURVE SMOOTH",
+"94 343 OFFCURVE",
+"50 299 OFFCURVE",
+"50 244 CURVE SMOOTH",
+"50 189 OFFCURVE",
+"94 145 OFFCURVE",
+"149 145 CURVE SMOOTH"
+);
+}
+);
+width = 298;
+},
+{
+associatedMasterId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+layerId = "544838C0-0251-432C-BCBD-A505EB52F9E4";
+name = "Bold [600]";
+paths = (
+{
+closed = 1;
+nodes = (
+"366 46 OFFCURVE",
+"457 137 OFFCURVE",
+"457 250 CURVE SMOOTH",
+"457 362 OFFCURVE",
+"366 453 OFFCURVE",
+"254 453 CURVE SMOOTH",
+"141 453 OFFCURVE",
+"50 362 OFFCURVE",
+"50 250 CURVE SMOOTH",
+"50 137 OFFCURVE",
+"141 46 OFFCURVE",
+"254 46 CURVE SMOOTH"
+);
+}
+);
+width = 507;
+}
+);
+unicode = 0041;
+},
+{
+glyphname = B;
+lastChange = "2019-07-29 12:37:08 +0000";
+layers = (
+{
+components = (
+{
+name = A;
+}
+);
+layerId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+width = 252;
+},
+{
+components = (
+{
+name = A;
+}
+);
+layerId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+width = 416;
+},
+{
+associatedMasterId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+components = (
+{
+name = A;
+}
+);
+layerId = "D2130EB1-1AC6-4D35-ACCA-0E1EDD0F7195";
+name = "Regular [600]";
+width = 298;
+},
+{
+associatedMasterId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+components = (
+{
+name = A;
+}
+);
+layerId = "82DCA10A-9CCD-47A7-962F-54D1CAFB7990";
+name = "Bold [600]";
+width = 507;
+}
+);
+unicode = 0042;
+},
+{
+glyphname = C;
+lastChange = "2019-07-25 14:16:37 +0000";
+layers = (
+{
+layerId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+paths = (
+{
+closed = 1;
+nodes = (
+"134 0 LINE",
+"445 0 LINE",
+"445 616 LINE",
+"134 616 LINE"
+);
+}
+);
+width = 600;
+},
+{
+layerId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+paths = (
+{
+closed = 1;
+nodes = (
+"-131 108 LINE",
+"727 108 LINE",
+"727 351 LINE",
+"-131 351 LINE"
+);
+}
+);
+width = 600;
+},
+{
+associatedMasterId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+layerId = "B27C8969-8000-48F7-9D84-8DC46817C710";
+name = "Bold [600]";
+paths = (
+{
+closed = 1;
+nodes = (
+"294 -60 LINE",
+"364 -60 LINE",
+"812 441 LINE",
+"-178 441 LINE"
+);
+}
+);
+width = 600;
+}
+);
+unicode = 0043;
+},
+{
+glyphname = D;
+lastChange = "2019-07-25 15:50:01 +0000";
+layers = (
+{
+layerId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+paths = (
+{
+closed = 1;
+nodes = (
+"134 0 LINE",
+"445 0 LINE",
+"445 616 LINE",
+"134 616 LINE"
+);
+}
+);
+width = 600;
+},
+{
+layerId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+paths = (
+{
+closed = 1;
+nodes = (
+"-131 108 LINE",
+"727 108 LINE",
+"727 351 LINE",
+"-131 351 LINE"
+);
+}
+);
+width = 600;
+},
+{
+associatedMasterId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+layerId = "3B8EF315-03C2-48B0-99CD-68A39D05BC39";
+name = "Bold ]600]";
+paths = (
+{
+closed = 1;
+nodes = (
+"294 -60 LINE",
+"364 -60 LINE",
+"812 441 LINE",
+"-178 441 LINE"
+);
+}
+);
+width = 600;
+},
+{
+associatedMasterId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+layerId = "01DB0260-6B98-47ED-85C3-BA953963A053";
+name = "Regular ]600]";
+paths = (
+{
+closed = 1;
+nodes = (
+"134 0 LINE",
+"445 0 LINE",
+"346 616 LINE",
+"303 616 LINE"
+);
+}
+);
+width = 600;
+}
+);
+unicode = 0044;
+},
+{
+glyphname = E;
+lastChange = "2019-07-25 17:42:44 +0000";
+layers = (
+{
+layerId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+paths = (
+{
+closed = 1;
+nodes = (
+"560 -105 LINE",
+"605 -64 LINE",
+"71 529 LINE",
+"39 500 LINE"
+);
+}
+);
+width = 600;
+},
+{
+layerId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+paths = (
+{
+closed = 1;
+nodes = (
+"294 -60 LINE",
+"364 -60 LINE",
+"812 441 LINE",
+"-178 441 LINE"
+);
+}
+);
+width = 600;
+},
+{
+associatedMasterId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+layerId = "3B8EF315-03C2-48B0-99CD-68A39D05BC39";
+name = "Bold ]570]";
+paths = (
+{
+closed = 1;
+nodes = (
+"-131 108 LINE",
+"727 108 LINE",
+"727 351 LINE",
+"-131 351 LINE"
+);
+}
+);
+width = 600;
+},
+{
+associatedMasterId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+layerId = "01DB0260-6B98-47ED-85C3-BA953963A053";
+name = "Regular [630]";
+paths = (
+{
+closed = 1;
+nodes = (
+"134 0 LINE",
+"445 0 LINE",
+"445 616 LINE",
+"134 616 LINE"
+);
+}
+);
+width = 600;
+}
+);
+unicode = 0045;
+},
+{
+glyphname = F;
+lastChange = "2019-07-26 09:56:33 +0000";
+layers = (
+{
+layerId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+paths = (
+{
+closed = 1;
+nodes = (
+"134 0 LINE",
+"445 0 LINE",
+"445 616 LINE",
+"134 616 LINE"
+);
+}
+);
+width = 600;
+},
+{
+layerId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+paths = (
+{
+closed = 1;
+nodes = (
+"-131 108 LINE",
+"727 108 LINE",
+"727 351 LINE",
+"-131 351 LINE"
+);
+}
+);
+width = 600;
+},
+{
+associatedMasterId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+layerId = "3B8EF315-03C2-48B0-99CD-68A39D05BC39";
+name = "Bold [630]";
+paths = (
+{
+closed = 1;
+nodes = (
+"294 -60 LINE",
+"364 -60 LINE",
+"812 441 LINE",
+"-178 441 LINE"
+);
+}
+);
+width = 600;
+},
+{
+associatedMasterId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+layerId = "01DB0260-6B98-47ED-85C3-BA953963A053";
+name = "Regular ]630]";
+paths = (
+{
+closed = 1;
+nodes = (
+"560 -105 LINE",
+"605 -64 LINE",
+"71 529 LINE",
+"39 500 LINE"
+);
+}
+);
+width = 600;
+}
+);
+unicode = 0046;
+},
+{
+glyphname = space;
+lastChange = "2019-07-25 10:21:14 +0000";
+layers = (
+{
+layerId = "AC80BBED-896D-4DAA-A852-C79F9201ACE8";
+width = 600;
+},
+{
+layerId = "48798C20-C9A0-4BB9-9A3E-BDB30805D9C9";
+width = 600;
+}
+);
+unicode = 0020;
+}
+);
+instances = (
+{
+interpolationWeight = 400;
+instanceInterpolations = {
+"AC80BBED-896D-4DAA-A852-C79F9201ACE8" = 1;
+};
+name = Regular;
+},
+{
+interpolationWeight = 600;
+instanceInterpolations = {
+"48798C20-C9A0-4BB9-9A3E-BDB30805D9C9" = 0.66667;
+"AC80BBED-896D-4DAA-A852-C79F9201ACE8" = 0.33333;
+};
+name = SemiBold;
+weightClass = SemiBold;
+},
+{
+interpolationWeight = 700;
+instanceInterpolations = {
+"48798C20-C9A0-4BB9-9A3E-BDB30805D9C9" = 1;
+};
+isBold = 1;
+name = Bold;
+weightClass = Bold;
+},
+{
+interpolationWeight = 500;
+instanceInterpolations = {
+"48798C20-C9A0-4BB9-9A3E-BDB30805D9C9" = 0.33333;
+"AC80BBED-896D-4DAA-A852-C79F9201ACE8" = 0.66667;
+};
+name = Medium;
+weightClass = Medium;
+}
+);
+unitsPerEm = 1000;
+versionMajor = 1;
+versionMinor = 0;
+}