Keep Glyphs custom parameter does not work

[punchcutter]

After doing some testing on files that use the Keep Glyphs custom parameter (Noto Sans Devanagari, Noto Sans Bengali, Noto Sans Kannada, Noto Sans Malayalam) it was clear that nothing at all was happening. I see a couple problems so far.

font_project.py

276    def save_otfs(self,
277                  ufos,
278                  ttf=False,
279                  is_instance=False,
280                  interpolatable=False,
281                  use_afdko=False,
282                  autohint=None,
283                  subset=None,
   ...
406        for font, ufo in zip(fonts, ufos):
407            if subset is None:
408                export_key = GLYPHS_PREFIX + 'Glyphs.Export'
409                subset = ((GLYPHS_PREFIX + 'Keep Glyphs') in ufo.lib or
410                          any(glyph.lib.get(export_key, True) is False
411                              for glyph in ufo))
412            if subset:
413                self.subset_otf_from_ufo(otf_path, ufo)
   ...
446        keep_glyphs_list = ufo.lib.get(GLYPHS_PREFIX + 'Keep Glyphs')

subset is initialized to None so the first time through the for loop it is None, but after that it gets set to either True or False and any subsequent iteration doesn't necessarily get to the subset code. Each ufo needs to be checked for the presence of Keep Glyphs because it could be different for each one. In Noto Sans Bengali all glyphs are set to export so any(glyph.lib.get(export_key, True) is False for glyph in ufo) is False and so is (GLYPHS_PREFIX + 'Keep Glyphs') in ufo.lib because GLYPHS_PREFIX + 'Keep Glyphs' is never set in the ufo in the first place.

I adjusted things locally so that everything works, but when it does work we run into another big problem: googlefonts/glyphsLib#205
If Keep Glyphs works and removes the encoded glyphs from the UI instances (because those are replaced with UI specific glyphs) then those codepoints need to be assigned to the UI specific glyphs. Otherwise they just show up as .notdef. That's way worse than the current behavior which is to completely miss the Keep Glyphs which leaves the non-UI glyphs in the UI instances.

[anthrotype]

@punchcutter thanks for debugging this. The first is definitely a bug. It should reset the subset variable to None and evaluate the custom parameters of each ufo independently. I'll fix that.
I'll also work on adding support for Reencode Glyphs in glyphsLib. I think the best place for this is in the apply_instance_data function that fontmake calls after mutatormath has generated the UFO instances. The custom parameters should already be dumped in the designspace document's lib element. We just need to apply them.

[anthrotype]

Ouch.. I just found another issue which is related to this.

In glyphsLib 2.2.1 (used by fontmake 1.4), the "Keep Glyphs" instance custom parameter was (used to be) exported with the following UFO lib key:

"com.schriftgestaltung.Keep Glyphs"

That's how fontmake itself expects it to be when it goes to subset_otf_from_ufo.

However, in the latest glyphsLib 2.3.0, the same "Keep Glyphs" custom parameter is now being exported using this mouthful:

"com.schriftgestaltung.customParameter.InstanceDescriptorAsGSInstance.Keep Glyphs"

Therefore fontmake cannot see it...

The glyphsLib.builder.custom_params was completely rewritten by @belluzj to support going in both directions (glyphs -> UFOs, and viceversa):
googlefonts/glyphsLib@v2.2.1...master#diff-b0b07578ca1cbb8c00dd528d6b044934

I personally don't like that long name (let alone the space between the words, but that's not the point).
The problem is, prefixing it with com.schriftgestaltung.*, suggests that this comes from Glyphs.app the app itself, whereas it really is a private key which is understood and meant to be consumed only by glyphsLib, ie. the python library. So, perhaps something like com.github.googlei18n.glyphsLib.* would be more appropriate in this and possibly other cases, I don't know.
It's one thing to use com.schriftgestaltung.* when Glyphs.app's built-in UFO exporter uses that prefix for the same kind of data. But if glyphsLib is storing new stuff in the UFO lib for its own use, it should not pretend that those come from Glyphs.app.

Btw, it's not just fontmake that needs to handle the new lib key prefix scheme from glyphsLib 2.3, but also ufo2ft, eg. here:
https://github.com/googlei18n/ufo2ft/blob/dbb107c8093bb8824c7e26fa69d240d3accdb548/Lib/ufo2ft/constants.py#L8

[anthrotype]

Anyway, for now I'll change fontmake to check both the old short name, and the new long one.
I hope one day we get the chance to clean up all this technical debt..

[moyogo]

I think glyphsLib should only do Glyphs.app to master UFOs+designspace and not Glyphs.app to master UFOs. That way instance specific parameters can be stored in the designspace instance lib, and be repricated when instance UFOs are generated. We could then get rid of com.schriftgestaltung.customParameter.InstanceDescriptorAsGSInstance.* keys and only have com.schriftgestaltung.* keys.

[belluzj]

"com.schriftgestaltung.customParameter.InstanceDescriptorAsGSInstance.Keep Glyphs"

Yes, this is a stupidly long name. I'm going to explain how that came to be, so that a better solution can be found:

• for font & master custom parameters, I added "GSFont" or "GSFontMaster" inside the name, because both font and master custom parameters end up in the same UFO lib. When we go in the other direction, we can know from the name where the custom parameter should go.

  • this is happening here as well for instance custom parameters ("InstanceDescriptorAsGSInstance"), but it is unnecessary because those custom parameters are not meant to "go back" to glyphs, since they are added to a UFO that has been freshly interpolated by fontmake/mutatorMath and that will be discarded when we get a TTF, and also because if this customParameter from the GSInstance is in conflict with one that came e.g. from the GSFont, then the instance value should overwrite (or be appended?) to the value from the GSFont.
  • even in the case of GSFont vs GSFontMaster, we could do:
    • GSFont <-> designspace lib
    • GSFontMaster <-> UFO lib
    • GSInstance <-> designspace instance lib
      and thus there is a one-to-one mapping and no need for that part of the key name.

  This part is added here:
  https://github.com/googlei18n/glyphsLib/blob/b35f4abe16df4d49f687833dbf841b9d7b0ec281/Lib/glyphsLib/builder/custom_params.py#L77-L79

• the key starts by "com.schriftgestaltung.customParameter" because I wanted to be sure of what is a custom parameter and what is not. A key like "com.github.googlei18n.glyphsLib.custom" would be fine as well. The key prefix is here:
  https://github.com/googlei18n/glyphsLib/blob/b35f4abe16df4d49f687833dbf841b9d7b0ec281/Lib/glyphsLib/builder/custom_params.py#L66