From 4275008227f8f3d7396e414c5a999d21fc5a6046 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 2 May 2019 12:55:49 +0100 Subject: [PATCH] [kernFeatureWriter] don't set ignoreMarks when kern pairs contains marks Basically cleaning up https://github.com/googlefonts/ufo2ft/pull/314 Also, updated expected test results; reverted unnecessary changes to _groupScriptsByTagAndDirection; removed unused addLookupReference function in ast module. --- Lib/ufo2ft/featureWriters/ast.py | 14 - .../featureWriters/kernFeatureWriter.py | 289 ++++++++---------- .../featureWriters/kernFeatureWriter_test.py | 82 ++--- 3 files changed, 144 insertions(+), 241 deletions(-) diff --git a/Lib/ufo2ft/featureWriters/ast.py b/Lib/ufo2ft/featureWriters/ast.py index 58f8fa8f0..5cf03219f 100644 --- a/Lib/ufo2ft/featureWriters/ast.py +++ b/Lib/ufo2ft/featureWriters/ast.py @@ -181,20 +181,6 @@ def addLookupReferences( ) -def addLookupReference( - feature, lookup, script=None, languages=None, exclude_dflt=False -): - """Shortcut for addLookupReferences, but for a single lookup. - """ - return addLookupReferences( - feature, - (lookup,), - script=script, - languages=languages, - exclude_dflt=exclude_dflt, - ) - - _GDEFGlyphClasses = collections.namedtuple( "_GDEFGlyphClasses", "base ligature mark component" ) diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py index 216f8e016..ed4478671 100644 --- a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py +++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py @@ -213,7 +213,9 @@ def setContext(self, font, feaFile, compiler=None): ) ctx.gdefClasses = ast.getGDEFGlyphClasses(feaFile) ctx.kerning = self.getKerningData(font, feaFile, self.getOrderedGlyphSet()) - ctx.scriptGroups = self._groupScriptsByTagAndDirection(feaFile) + + feaScripts = ast.getScriptLanguageSystems(feaFile) + ctx.scriptGroups = self._groupScriptsByTagAndDirection(feaScripts) return ctx @@ -259,8 +261,8 @@ def _write(self): statements.append(ast.Comment("")) # finally add the lookup and feature blocks - for _, lookup in sorted(lookups.items()): - statements.append(lookup) + for _, lookupGroup in sorted(lookups.items()): + statements.extend(lookupGroup) if "kern" in features: statements.append(features["kern"]) if "dist" in features: @@ -352,24 +354,20 @@ def _intersectPairs(self, attribute, glyphSets): return allKeys @staticmethod - def _groupScriptsByTagAndDirection(feaFile): + def _groupScriptsByTagAndDirection(feaScripts): # Read scripts/languages defined in feaFile's 'languagesystem' # statements and group them by the feature tag (kern or dist) # they are associated with, and the global script's horizontal # direction (DFLT is excluded) - feaScripts = ast.getScriptLanguageSystems(feaFile) scriptGroups = {} - tags = ['kern'] for scriptCode, scriptLangSys in feaScripts.items(): direction = unicodedata.script_horizontal_direction(scriptCode) if scriptCode in DIST_ENABLED_SCRIPTS: - if 'feature kern' in feaFile.asFea(): - tags.append('dist') - else: - tags = ['dist'] - for tag in tags: - scriptGroups.setdefault(tag, {}).setdefault(direction, []).extend( - scriptLangSys + tag = "dist" + else: + tag = "kern" + scriptGroups.setdefault(tag, {}).setdefault(direction, []).extend( + scriptLangSys ) return scriptGroups @@ -394,7 +392,9 @@ def _makePairPosRule(pair, rtl=False): enumerated=enumerated, ) - def _makeKerningLookup(self, name, pairs, exclude=None, rtl=False, ignoreMarks=True): + def _makeKerningLookup( + self, name, pairs, exclude=None, rtl=False, ignoreMarks=True + ): assert pairs rules = [] for pair in pairs: @@ -429,24 +429,7 @@ def _makeKerningLookups(self): else: shouldSplit = False - # If there are pairs with a mix of mark/base then the IgnoreMarks - # flag is unnecessary and should not be set - base_to_base = [] - base_to_mark = [] - for pair in self.context.kerning.pairs: - hasMarks = False - for glyph in pair.side1.glyphSet() + pair.side2.glyphSet(): - if isinstance(glyph, ast.GlyphName): - glyph = glyph.glyphSet()[0] - if self.context.gdefClasses.mark is not None: - if glyph in self.context.gdefClasses.mark: - hasMarks = True - break - if hasMarks: - base_to_mark.append(pair) - else: - base_to_base.append(pair) - + marks = self.context.gdefClasses.mark lookups = {} if shouldSplit: # make one DFLT lookup with script-agnostic characters, and two @@ -463,100 +446,89 @@ def _makeKerningLookups(self): if not pairs: return lookups - dfltKern = self._makeKerningLookup( - "kern_dflt", - pairs, - exclude=( - lambda pair: bool({"LTR", "RTL"}.intersection(pair.directions)) - or pair in base_to_mark - ), - rtl=False, - ) - if dfltKern: - lookups["DFLT"] = dfltKern - - ltrKern = self._makeKerningLookup( - "kern_ltr", - pairs, - exclude=( - lambda pair: (not pair.directions - or "RTL" in pair.directions) - or pair in base_to_mark - ), - rtl=False, - ) - if ltrKern: - lookups["LTR"] = ltrKern - - rtlKern = self._makeKerningLookup( - "kern_rtl", - pairs, - exclude=( - lambda pair: (not pair.directions - or "LTR" in pair.directions) - or pair in base_to_mark - ), - rtl=True, - ) - if rtlKern: - lookups["RTL"] = rtlKern - - if base_to_mark: - dfltKernHasMarks = self._makeKerningLookup( - "kern_dflt_marks", - pairs, - exclude=( - lambda pair: bool({"LTR", "RTL"}.intersection(pair.directions)) - and pair not in base_to_mark - ), - rtl=False, - ignoreMarks=False, - ) - if dfltKernHasMarks: - lookups["DFLTHasMarks"] = dfltKernHasMarks - - ltrKernHasMarks = self._makeKerningLookup( - "kern_ltr_marks", - pairs, - exclude=( - lambda pair: pair not in base_to_mark or (not pair.directions - or "RTL" in pair.directions) - ), - rtl=False, - ignoreMarks=False, - ) - if ltrKernHasMarks: - lookups["LTRHasMarks"] = ltrKernHasMarks - - rtlKernHasMarks = self._makeKerningLookup( - "kern_rtl_marks", - pairs, - exclude=( - lambda pair: pair not in base_to_mark or (not pair.directions - or "LTR" in pair.directions) - ), - rtl=True, - ignoreMarks=False, - ) - if rtlKernHasMarks: - lookups["RTLHasMarks"] = rtlKernHasMarks + if self.options.ignoreMarks: + # If there are pairs with a mix of mark/base then the IgnoreMarks + # flag is unnecessary and should not be set + basePairs, markPairs = self._splitBaseAndMarkPairs(pairs, marks) + self._makeSplitDirectionKernLookups(lookups, basePairs) + if markPairs: + self._makeSplitDirectionKernLookups( + lookups, markPairs, ignoreMarks=False, suffix="_marks" + ) + else: + self._makeSplitDirectionKernLookups(lookups, pairs) else: # only make a single (implicitly LTR) lookup including all base/base pairs - # and a single lookup including all base/mark pairs if necessary - lookups["LTR"] = self._makeKerningLookup( - "kern_ltr", - self.context.kerning.pairs, - exclude=(lambda pair: pair in base_to_mark), - ) - if base_to_mark: - lookups["LTRHasMarks"] = self._makeKerningLookup( - "kern_ltr_marks", - self.context.kerning.pairs, - exclude=(lambda pair: pair in base_to_base), - ignoreMarks=False, - ) + # and a single lookup including all base/mark pairs (if any) + pairs = self.context.kerning.pairs + if self.options.ignoreMarks: + basePairs, markPairs = self._splitBaseAndMarkPairs(pairs, marks) + lookups["LTR"] = [self._makeKerningLookup("kern_ltr", basePairs)] + if markPairs: + lookups["LTR"].append( + self._makeKerningLookup( + "kern_ltr_marks", + markPairs, + ignoreMarks=False, + ) + ) + else: + lookups["LTR"] = [self._makeKerningLookup("kern_ltr", pairs)] return lookups + def _splitBaseAndMarkPairs(self, pairs, marks): + basePairs, markPairs = [], [] + if marks: + for pair in pairs: + if any(glyph in marks for glyph in pair.glyphs): + markPairs.append(pair) + else: + basePairs.append(pair) + else: + basePairs[:] = pairs + return basePairs, markPairs + + def _makeSplitDirectionKernLookups( + self, lookups, pairs, ignoreMarks=True, suffix="" + ): + dfltKern = self._makeKerningLookup( + "kern_dflt" + suffix, + pairs, + exclude=( + lambda pair: {"LTR", "RTL"}.intersection(pair.directions) + ), + rtl=False, + ignoreMarks=ignoreMarks, + ) + if dfltKern: + lookups.setdefault("DFLT", []).append(dfltKern) + + ltrKern = self._makeKerningLookup( + "kern_ltr" + suffix, + pairs, + exclude=( + lambda pair: not pair.directions + or "RTL" in pair.directions + ), + rtl=False, + ignoreMarks=ignoreMarks, + ) + if ltrKern: + lookups.setdefault("LTR", []).append(ltrKern) + + rtlKern = self._makeKerningLookup( + "kern_rtl" + suffix, + pairs, + exclude=( + lambda pair: not pair.directions + or "LTR" in pair.directions + ), + rtl=True, + ignoreMarks=ignoreMarks, + ) + if rtlKern: + lookups.setdefault("RTL", []).append(rtlKern) + def _makeFeatureBlocks(self, lookups): features = {} if "kern" in self.context.todo: @@ -573,40 +545,33 @@ def _makeFeatureBlocks(self, lookups): def _registerKernLookups(self, feature, lookups): if "DFLT" in lookups: - ast.addLookupReference(feature, lookups["DFLT"]) + ast.addLookupReferences(feature, lookups["DFLT"]) + scriptGroups = self.context.scriptGroups if "dist" in self.context.todo: - distScripts = scriptGroups.get("dist") + distScripts = scriptGroups["dist"] else: distScripts = {} kernScripts = scriptGroups.get("kern", {}) ltrScripts = kernScripts.get("LTR", []) rtlScripts = kernScripts.get("RTL", []) - ltrLkp = lookups.get("LTR") - rtlLkp = lookups.get("RTL") - dfltLkpHasMarks = lookups.get("DFLTHasMarks") - ltrLkpHasMarks = lookups.get("LTRHasMarks") - rtlLkpHasMarks = lookups.get("RTLHasMarks") - ltr_lookups = [ltrLkp] - rtl_lookups = [rtlLkp] - if ltrLkpHasMarks is not None: - ltr_lookups.append(ltrLkpHasMarks) - if rtlLkpHasMarks is not None: - rtl_lookups.append(rtlLkpHasMarks) - if ltrLkp and rtlLkp: + + ltrLookups = lookups.get("LTR") + rtlLookups = lookups.get("RTL") + if ltrLookups and rtlLookups: if ltrScripts and rtlScripts: for script, langs in ltrScripts: - ast.addLookupReferences(feature, ltr_lookups, script, langs) + ast.addLookupReferences(feature, ltrLookups, script, langs) for script, langs in rtlScripts: - ast.addLookupReferences(feature, rtl_lookups, script, langs) + ast.addLookupReferences(feature, rtlLookups, script, langs) elif ltrScripts: - ast.addLookupReference(feature, rtlLkp, script="DFLT") + ast.addLookupReferences(feature, rtlLookups, script="DFLT") for script, langs in ltrScripts: - ast.addLookupReferences(feature, ltr_lookups, script, langs) + ast.addLookupReferences(feature, ltrLookups, script, langs) elif rtlScripts: - ast.addLookupReference(feature, ltrLkp, script="DFLT") + ast.addLookupReferences(feature, ltrLookups, script="DFLT") for script, langs in rtlScripts: - ast.addLookupReferences(feature, rtl_lookups, script, langs) + ast.addLookupReferences(feature, rtlLookups, script, langs) else: if not (distScripts.get("LTR") and distScripts.get("RTL")): raise ValueError( @@ -614,38 +579,28 @@ def _registerKernLookups(self, feature, lookups): "lookups; add 'languagesystems' to features for at " "least one LTR or RTL script using the kern feature" ) - elif ltrLkp: + elif ltrLookups: if not (rtlScripts or distScripts): - ast.addLookupReferences(feature, ltr_lookups) + ast.addLookupReferences(feature, ltrLookups) else: - ast.addLookupReferences(feature, ltr_lookups, script="DFLT") + ast.addLookupReferences(feature, ltrLookups, script="DFLT") for script, langs in ltrScripts: - ast.addLookupReferences(feature, ltr_lookups, script, langs) - elif rtlLkp: + ast.addLookupReferences(feature, ltrLookups, script, langs) + elif rtlLookups: if not (ltrScripts or distScripts): - ast.addLookupReferences(feature, rtl_lookups) + ast.addLookupReferences(feature, rtlLookups) else: - ast.addLookupReferences(feature, rtl_lookups, script="DFLT") + ast.addLookupReferences(feature, rtlLookups, script="DFLT") for script, langs in rtlScripts: - ast.addLookupReferences(feature, rtl_lookups, script, langs) + ast.addLookupReferences(feature, rtlLookups, script, langs) def _registerDistLookups(self, feature, lookups): - scripts = self.context.scriptGroups.get("dist") - ltrLkp = lookups.get("LTR") - if ltrLkp: + scripts = self.context.scriptGroups["dist"] + ltrLookups = lookups.get("LTR") + if ltrLookups: for script, langs in scripts.get("LTR", []): - ast.addLookupReference(feature, ltrLkp, script, langs) - - ltrLkpHasMarks = lookups.get("LTRHasMarks") - if ltrLkpHasMarks: - for script, langs in scripts.get("LTRHasMarks", []): - ast.addLookupReference(feature, ltrLkpHasMarks, script, langs) - rtlLkp = lookups.get("RTL") - if rtlLkp: + ast.addLookupReferences(feature, ltrLookups, script, langs) + rtlLookups = lookups.get("RTL") + if rtlLookups: for script, langs in scripts.get("RTL", []): - ast.addLookupReference(feature, rtlLkp, script, langs) - - rtlLkpHasMarks = lookups.get("RTLHasMarks") - if rtlLkpHasMarks: - for script, langs in scripts.get("RTLHasMarks", []): - ast.addLookupReference(feature, rtlLkpHasMarks, script, langs) + ast.addLookupReferences(feature, rtlLookups, script, langs) diff --git a/tests/featureWriters/kernFeatureWriter_test.py b/tests/featureWriters/kernFeatureWriter_test.py index 9b2188914..e0bb005a2 100644 --- a/tests/featureWriters/kernFeatureWriter_test.py +++ b/tests/featureWriters/kernFeatureWriter_test.py @@ -163,11 +163,11 @@ def test_mark_to_base_kern(self, FontClass): lookupflag IgnoreMarks; pos B C -30; } kern_ltr; - + lookup kern_ltr_marks { pos A acutecomb -55; } kern_ltr_marks; - + feature kern { lookup kern_ltr; lookup kern_ltr_marks; @@ -175,6 +175,20 @@ def test_mark_to_base_kern(self, FontClass): """ ) + feaFile = self.writeFeatures(font, ignoreMarks=False) + assert str(feaFile) == dedent( + """ + lookup kern_ltr { + pos A acutecomb -55; + pos B C -30; + } kern_ltr; + + feature kern { + lookup kern_ltr; + } kern; + """ + ) + def test_mode(self, FontClass): ufo = FontClass() for name in ("one", "four", "six", "seven"): @@ -269,8 +283,9 @@ def test__groupScriptsByTagAndDirection(self, FontClass): ) feaFile = parseLayoutFeatures(font) + scripts = ast.getScriptLanguageSystems(feaFile) scriptGroups = KernFeatureWriter._groupScriptsByTagAndDirection( - feaFile + scripts ) assert "kern" in scriptGroups @@ -287,46 +302,6 @@ def test__groupScriptsByTagAndDirection(self, FontClass): ("dev2", ["dflt"]), ] - def test__groupScriptsByTagAndDirectionDistAndKern(self, FontClass): - font = FontClass() - font.newGlyph('ka-sidd').unicode = 0x1158E - font.newGlyph('ga-sidd').unicode = 0x11590 - font.features.text = dedent( - """ - languagesystem DFLT dflt; - languagesystem sidd dflt; - - @kern1.ka = [ka-sidd]; - @kern2.ga = [ga-sidd]; - - lookup kern_ltr { - lookupflag IgnoreMarks; - enum pos @kern1.ka @kern2.ga -40; - } kern_ltr; - - feature kern { - lookup kern_ltr; - script sidd; - } kern; - - """ - ) - - feaFile = parseLayoutFeatures(font) - scriptGroups = KernFeatureWriter._groupScriptsByTagAndDirection( - feaFile - ) - - assert "kern" in scriptGroups - assert list(scriptGroups["kern"]["LTR"]) == [ - ("sidd", ["dflt"]) - ] - - assert "dist" in scriptGroups - assert list(scriptGroups["dist"]["LTR"]) == [ - ("sidd", ["dflt"]), - ] - def test_getKerningClasses(self, FontClass): font = FontClass() for i in range(65, 65 + 6): # A..F @@ -487,7 +462,7 @@ def test_kern_LTR_and_RTL(self, FontClass): ufo = makeUFO(FontClass, glyphs, groups, kerning, features) - newFeatures = self.writeFeatures(ufo) + newFeatures = self.writeFeatures(ufo, ignoreMarks=False) assert str(newFeatures) == dedent( """\ @@ -496,17 +471,14 @@ def test_kern_LTR_and_RTL(self, FontClass): @kern2.alef = [alef-ar alef-ar.isol]; lookup kern_dflt { - lookupflag IgnoreMarks; pos seven four -25; } kern_dflt; lookup kern_ltr { - lookupflag IgnoreMarks; enum pos @kern1.A V -40; } kern_ltr; lookup kern_rtl { - lookupflag IgnoreMarks; pos four-ar seven-ar -30; pos reh-ar.fina lam-ar.init <-80 0 -80 0>; pos @kern1.reh @kern2.alef <-100 0 -100 0>; @@ -606,12 +578,6 @@ def test_kern_LTR_and_RTL_with_marks(self, FontClass): pos seven four -25; } kern_dflt; - lookup kern_dflt_marks { - pos V acutecomb 70; - pos reh-ar fatha-ar 80; - pos seven four -25; - } kern_dflt_marks; - lookup kern_ltr { lookupflag IgnoreMarks; enum pos @kern1.A V -40; @@ -705,21 +671,17 @@ def test_kern_RTL_with_marks(self, FontClass): """\ @kern1.reh = [reh-ar zain-ar reh-ar.fina]; @kern2.alef = [alef-ar alef-ar.isol]; - - lookup kern_dflt_marks { - pos reh-ar fatha-ar 80; - } kern_dflt_marks; - + lookup kern_rtl { lookupflag IgnoreMarks; pos reh-ar.fina lam-ar.init <-80 0 -80 0>; pos @kern1.reh @kern2.alef <-100 0 -100 0>; } kern_rtl; - + lookup kern_rtl_marks { pos reh-ar fatha-ar <80 0 80 0>; } kern_rtl_marks; - + feature kern { lookup kern_rtl; lookup kern_rtl_marks;