Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn on questionable (dis)use of # Automatic Code #719

Merged
merged 8 commits into from Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 45 additions & 0 deletions Lib/ufo2ft/featureCompiler.py
@@ -1,9 +1,13 @@
from __future__ import annotations

import logging
import os
import re
from collections import OrderedDict
from inspect import isclass
from io import StringIO
from tempfile import NamedTemporaryFile
from typing import Any

from fontTools import mtiLib
from fontTools.feaLib.builder import addOpenTypeFeaturesFromString
Expand All @@ -21,6 +25,7 @@
isValidFeatureWriter,
loadFeatureWriters,
)
from ufo2ft.util import describe_ufo

logger = logging.getLogger(__name__)
timer = Timer(logging.getLogger("ufo2ft.timer"), level=logging.DEBUG)
Expand Down Expand Up @@ -281,6 +286,16 @@ def setupFeatures(self):
if self.featureWriters:
featureFile = parseLayoutFeatures(self.ufo, self.feaIncludeDir)

warn_about_miscased_insertion_markers(
describe_ufo(self.ufo),
featureFile,
{
feaWriter.insertFeatureMarker
for feaWriter in self.featureWriters
if feaWriter.insertFeatureMarker is not None
},
)

path = self.ufo.path
for writer in self.featureWriters:
try:
Expand Down Expand Up @@ -354,3 +369,33 @@ def buildTables(self):
table = mtiLib.build(features.splitlines(), self.ttFont)
assert table.tableTag == tag
self.ttFont[tag] = table


def warn_about_miscased_insertion_markers(
ufo_description: str, feaFile: ast.FeatureFile | ast.Block, patterns: set[str]
) -> None:
"""Warn the user about potentially mistyped feature insertion markers."""

patterns_compiled = tuple(
(re.compile(pattern), re.compile(pattern, re.IGNORECASE))
for pattern in patterns
)

def _inner(block: Any) -> None:
for statement in block.statements:
if hasattr(statement, "statements"):
_inner(statement)
elif isinstance(statement, ast.Comment):
anthrotype marked this conversation as resolved.
Show resolved Hide resolved
for pattern_case, pattern_ignore_case in patterns_compiled:
match_case = re.match(pattern_case, str(statement))
match_ignore_case = re.match(pattern_ignore_case, str(statement))
if match_ignore_case and not match_case:
logger.warning(
"%s: The insertion comment '%s' in the feature file is "
"miscased (search pattern: %s), ignoring it.",
ufo_description,
statement,
pattern_case.pattern,
)

_inner(feaFile)
3 changes: 3 additions & 0 deletions Lib/ufo2ft/featureWriters/baseFeatureWriter.py
Expand Up @@ -80,11 +80,13 @@ def setContext(self, font, feaFile, compiler=None):
instantiated from a FeatureCompiler);
- a set of features (tags) to be generated. If self.mode is "skip",
these are all the features which are _not_ already present.
- a set of all existing features (tags) in the feaFile.

Returns the context namespace instance.
"""
todo = set(self.features)
insertComments = None
existing = set()
if self.mode == "skip":
if self.insertFeatureMarker is not None:
insertComments = self.collectInsertMarkers(
Expand All @@ -104,6 +106,7 @@ def setContext(self, font, feaFile, compiler=None):
compiler=compiler,
todo=todo,
insertComments=insertComments,
existingFeatures=existing,
)

return self.context
Expand Down
23 changes: 22 additions & 1 deletion Lib/ufo2ft/featureWriters/kernFeatureWriter.py
Expand Up @@ -11,7 +11,13 @@

from ufo2ft.constants import COMMON_SCRIPT, INDIC_SCRIPTS, USE_SCRIPTS
from ufo2ft.featureWriters import BaseFeatureWriter, ast
from ufo2ft.util import DFLT_SCRIPTS, classifyGlyphs, quantize, unicodeScriptExtensions
from ufo2ft.util import (
DFLT_SCRIPTS,
classifyGlyphs,
describe_ufo,
quantize,
unicodeScriptExtensions,
)

LOGGER = logging.getLogger(__name__)

Expand Down Expand Up @@ -198,6 +204,21 @@ def setContext(self, font, feaFile, compiler=None):
ctx.gdefClasses = self.getGDEFGlyphClasses()
ctx.glyphSet = self.getOrderedGlyphSet()

# If the font contains kerning, the feaFile contains `kern` or `dist`
# feature blocks, but we have no insertion markers (or they were
# misspelt and ignored), warn the user that the kerning blocks in the
# feaFile take precedence and other kerning is dropped.
if (
font.kerning
and ctx.existingFeatures & self.features
and not ctx.insertComments
):
madig marked this conversation as resolved.
Show resolved Hide resolved
LOGGER.warning(
"%s: font has kerning, but also manually written kerning features "
"without an insertion comment. Dropping the former.",
describe_ufo(font),
)

# Remember which languages are defined for which OT tag, as all
# generated kerning needs to be registered for the script's `dflt`
# language, but also all those the designer defined manually. Otherwise,
Expand Down
15 changes: 14 additions & 1 deletion Lib/ufo2ft/util.py
Expand Up @@ -5,7 +5,7 @@
import re
from copy import deepcopy
from inspect import currentframe, getfullargspec
from typing import Mapping, Set
from typing import Any, Mapping, Set

from fontTools import subset, ttLib, unicodedata
from fontTools.designspaceLib import DesignSpaceDocument
Expand Down Expand Up @@ -612,3 +612,16 @@ def unicodeScriptExtensions(
defines "Hrkt" as an alias for both scripts.
"""
return {aliases.get(s, s) for s in unicodedata.script_extension(chr(codepoint))}


def describe_ufo(ufo: Any) -> str:
"""Returns a description of a UFO suitable for logging."""
if (
hasattr(ufo, "reader")
and hasattr(ufo.reader, "fs")
and hasattr(ufo.reader.fs, "root_path")
):
return ufo.reader.fs.root_path
elif ufo.info.familyName or ufo.info.styleName:
return f"{ufo.info.familyName} {ufo.info.styleName}"
madig marked this conversation as resolved.
Show resolved Hide resolved
madig marked this conversation as resolved.
Show resolved Hide resolved
return repr(ufo)
26 changes: 25 additions & 1 deletion tests/featureWriters/kernFeatureWriter_test.py
Expand Up @@ -7,7 +7,7 @@

from ufo2ft.constants import UNICODE_SCRIPT_ALIASES
from ufo2ft.errors import InvalidFeaturesData
from ufo2ft.featureCompiler import parseLayoutFeatures
from ufo2ft.featureCompiler import FeatureCompiler, parseLayoutFeatures
from ufo2ft.featureWriters import KernFeatureWriter, ast
from ufo2ft.util import DFLT_SCRIPTS, unicodeScriptExtensions

Expand Down Expand Up @@ -333,6 +333,30 @@ def test_insert_comment_before(self, FontClass):
"""
)

def test_comment_wrong_case_or_missing(self, FontClass, caplog):
ufo = FontClass()
for name in ("a", "b"):
ufo.newGlyph(name)
ufo.kerning.update({("a", "b"): 25.0})
ufo.features.text = dedent(
"""
feature kern {
# Automatic code
} kern;
"""
).strip()

with caplog.at_level(logging.WARNING):
compiler = FeatureCompiler(ufo, featureWriters=[KernFeatureWriter])
font = compiler.compile()

# We mis-cased the insertion marker above, so it's ignored and we end up
# with an empty `kern` block overriding the other kerning in the font
# source and therefore no `GPOS` table.
assert "miscased, ignoring it" in caplog.text
assert "Dropping the former" in caplog.text
assert "GPOS" not in font

def test_insert_comment_before_extended(self, FontClass):
ufo = FontClass()
for name in ("one", "four", "six", "seven"):
Expand Down