Support variable feature syntax #2432

simoncozens · 2021-10-22T07:47:54Z

This supports the variable FEA syntax defined here.

simoncozens · 2021-10-22T07:52:14Z

I put a lot of work into this PR. Because of maintainer inactivity, the branch grew stale. I tried to resurrect it but now it doesn't work. I don't know how to fix it.

Good luck, everyone.

ctrlcctrlv · 2021-10-22T11:48:24Z

Hey @simoncozens I'm so sorry my request broke your work, you trashed your Git status pretty good and usually I'm told that I'm “really good at Git™” and in previous work it seems people always use me for Git issues but even I couldn't quite get us back where we were before perfectly (matching commit hashes and branch name) but ① the tests pass and ② the Git log is basically normal looking sort of.

Here you are then:

#!/bin/bash
cat << EOF > /tmp/patches
b880ae1787d168f656f92df07b656d2086f0d6a3
f84cacad5e1707bedbac07d29844043b5d56f299
18e84dd02df8022b8fb51e3fd92c6b2fcf01b112
7a69fb1b55903ed3be1da138d63bc181af10bf68
91cee05f17c001f61d882336756a8a14d9f10001
24c9c90fdf4e9e37f5765d372edd011d9395f5cc
c16553d6232e465b5e10c0b7c025f542f94d8364
6ca9fb02493fe35e5477b792d5f3e67de111fbc8
0c7e30740fac19555ad9714c6a5785a2cb983ede
be89af49535a52ff8d046ec5851b5dfda2229238
13a084efcb981e4462842f5a55c5453e324d755a
3b1b5fde2f4ac2519327a6e5298b99895e1f3e83
b30b8f1c8bd62374b1e52f110d9d57a58889d7fc
af66495c3c2bb1470f55d1b6702516c32ba6c077
80828cf433d2c94421b318d2a42dee1ef357378d
5f37300cbc6f81767c3724cf58ed276f3b2e2f37
ecef3eda962036ded4240d9f3fb7dc2712d72066
151fb92871d50af8036e1eebf151465b0208ea96
c4ca64c3a012d1d8295a5272c59b681310b12633
EOF

mkdir -p /tmp/simonpatches && (cd /tmp/simonpatches; while read -r line; do wget https://github.com/fonttools/fonttools/commit/$line.patch; done < /tmp/patches)

git clone https://github.com/simoncozens/fonttools/ simonのfontTools
cd !$
git checkout 96f0169d9d39972f80ebd1b245fc902fa1435722
git checkout -b 'oldSimonHead'
tac /tmp/patches | EMAIL='simon@simon-cozens.org' GIT_COMMITTER_NAME='Simon Cozens' GIT_COMMITTER_EMAIL='simon@simon-cozens.org' parallel --jobs 1 git am --committer-date-is-author-date '<' /tmp/simonpatches/{}.patch
git checkout -b variable-feature-syntax
git branch -d oldSimonHead
git log --oneline | head -n10 | xargs -I{} echo '#' {}
# d1513eb7 Allow float values in condition sets.
# 951e5ecc Set count fields when building feature variations
# 50c320f2 Put count fields in ttx
# 90bff8c5 Support building feature variations using the condition statement.
# 0b436327 feature_variations_ is a better name than conditionalFeatures_
# 5c303db1 Normalize condition sets when storing them
# 16d29ead A more generic interface, taking either GSUB or GPOS table
# 7bfba828 Oops, leftover when testing something else
# ec43cce5 Rearrange featureVars so we can do *really* raw feature builds (by lookup ID)
# 1d623bba Store the conditional features in a dictionary until we work out what to do with them

Sorry if the script looks hacky to you @simoncozens , I promise you that you completely trashed your working tree and blew away and reference to the commits outside of GitHub, literally downloading patch files from GitHub is the only way to get your work back.

simoncozens · 2021-10-22T11:56:32Z

Yeah, I rebased against master (not main...).

ctrlcctrlv · 2021-10-22T12:00:11Z

Oh—Git might some day do some spring cleaning and wipe out disconnected commits (from any repo or branch) from its online interface, or limit allowing users to get them ("security breach!!!1"), so here's a zip if you ever find you need it:

patches.zip

Remember you can rewind to 96f0169.

simoncozens · 2021-10-22T12:06:18Z

Aha! I think we got it. Thank you.

ctrlcctrlv · 2021-10-22T12:15:11Z

You're welcome...I fear I'm about to become a general purpose Git fixer ;-)

Please only call upon me if I'm partially responsible for the breakage 🙇‍♂️

behdad · 2021-10-22T14:56:41Z

@simoncozens You are defacto owner of feaLib, otlLib. You should feel free to ask @anthrotype for approval and merge as you see fit.

simoncozens · 2021-10-22T16:04:28Z

You are defacto owner of feaLib, otlLib.

The perils of working on open source software.

OK, @anthrotype, how about it? This is a backward-compatible extension which I think will help push along the new syntax proposal, so I think it's worth doing.

anthrotype

LGTM

please also run black on the modules you touched

anthrotype · 2021-10-25T13:17:56Z

Lib/fontTools/feaLib/ast.py

+        return res
+
+class VariationBlock(Block):
+    """A named feature block."""


"named feature block".. no mention of "variation" in this docstring?

anthrotype · 2021-10-25T13:21:47Z

Lib/fontTools/feaLib/ast.py

+
+        features = builder.features_
+        builder.features_ = {}
+        Block.build(self, builder)


it's not immediately clear what's going on here, why is the features_ attributes temporarily swapped with an empty dict. Maybe add a comment

anthrotype · 2021-10-25T13:45:27Z

Lib/fontTools/feaLib/variableScalar.py

+
+
+class Location(dict):
+    def __hash__(self):


hm hashable and mutable objects (dict) are dangerous => https://hynek.me/articles/hashes-and-equality/

can you do without this? e.g. by storing location as tuple of tuples in your VariableScalar.values inner dict?

Or perhaps class Location(frozenset): pass :-)

What could work is:

def Location(loc): return tuple(sorted(loc.items()))

anthrotype · 2021-10-25T13:50:33Z

the test-cython CI test failure is unrelated, please ignore (it's failing to fetch some deps for 3.10, we shall pin to python 3.9 for now, I'll do that on main)

anthrotype · 2021-10-25T13:53:27Z

thanks Simon, apologies for taking so long to review this. Do feel free to ping me if something slips my attention

This is a backward-compatible extension which I think will help push along the new syntax proposal, so I think it's worth doing.

sounds good. Ideally the spec changes would go in first, followed by implemetation, but in this case it's perhas worth doing the reverse (with a caveat that impl is experimental until spec settles and may change and break things)

anthrotype · 2021-10-25T13:54:08Z

It would also be nice to get LGTM from one of the other feaLib stakeholders, e.g. @khaledhosny, @justvanrossum
or @dscorbett

anthrotype · 2021-10-25T15:35:42Z

@simoncozens if you merge origin/main into your PR branch, it should fix the unrelated CI failures

behdad · 2021-10-25T15:46:57Z

LGTM as well.

dscorbett · 2021-10-25T16:08:46Z

Lib/fontTools/feaLib/ast.py

+    def asFea(self, res="", indent=""):
+        res += indent + f"conditionset {self.name} " + "{\n"
+        for tag, (minvalue, maxvalue) in self.conditions.items():
+            res += indent + f"   {tag} {minvalue} {maxvalue};\n"


Why indent by three spaces instead of SHIFT (four spaces)?

See adobe-type-tools/afdko#153 (comment)

… to do with them

…okup ID) See fonttools#2316

simoncozens changed the title ~~Parse variable scalars in feature files.~~ Support variable feature syntax Oct 22, 2021

simoncozens mentioned this pull request Oct 22, 2021

Support new AFDKO variable layout syntax #2228

Closed

simoncozens force-pushed the variable-feature-syntax branch from 83231ab to 0f9ef95 Compare October 22, 2021 12:06

simoncozens requested a review from anthrotype October 22, 2021 16:04

anthrotype approved these changes Oct 25, 2021

View reviewed changes

This was referenced Oct 25, 2021

[designspace] Proposal to deprecate <sub> and possibly <rule> unified-font-object/ufo-spec#194

Closed

[designspace] Proposal to add <fea> #2434

Open

dscorbett reviewed Oct 25, 2021

View reviewed changes

simoncozens added 9 commits October 28, 2021 11:41

Parse variable scalars in feature files.

6810cf5

See adobe-type-tools/afdko#153 (comment)

Build variable scalars for anchors/value records

3fd6358

Variable scalar tests

c73de1a

Support variable scalars in pos A V (...);

0c13412

Format bug

a32d7ae

Useful property

be66887

Add support for the conditionset statement

803dc38

Parse variation blocks

98215d3

Round-trip FEA correctly

46527aa

simoncozens added 13 commits October 28, 2021 11:41

Store the conditional features in a dictionary until we work out what…

32ee444

… to do with them

Rearrange featureVars so we can do *really* raw feature builds (by lo…

0cb89cc

…okup ID) See fonttools#2316

Oops, leftover when testing something else

699ffe0

A more generic interface, taking either GSUB or GPOS table

087848d

Normalize condition sets when storing them

0de9ce6

feature_variations_ is a better name than conditionalFeatures_

0031326

Support building feature variations using the "condition" statement.

c87399b

Put count fields in ttx

389b0ea

Set count fields when building feature variations

6937cc3

Allow float values in condition sets.

bd8ba64

Correct indentation

ddc7658

Address feedback

8f06c0e

Run black on all touched files

eab87ed

simoncozens force-pushed the variable-feature-syntax branch from 49c0efb to eab87ed Compare October 28, 2021 10:41

simoncozens merged commit 563730f into fonttools:main Oct 28, 2021

simoncozens deleted the variable-feature-syntax branch October 28, 2021 10:58

rsheeter mentioned this pull request Apr 25, 2023

Explore variable feature support cmyr/fea-rs#131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support variable feature syntax #2432

Support variable feature syntax #2432

simoncozens commented Oct 22, 2021

simoncozens commented Oct 22, 2021

ctrlcctrlv commented Oct 22, 2021 •

edited

simoncozens commented Oct 22, 2021

ctrlcctrlv commented Oct 22, 2021

simoncozens commented Oct 22, 2021

ctrlcctrlv commented Oct 22, 2021

behdad commented Oct 22, 2021

simoncozens commented Oct 22, 2021

anthrotype left a comment

anthrotype Oct 25, 2021

anthrotype Oct 25, 2021

anthrotype Oct 25, 2021

ctrlcctrlv Oct 25, 2021

justvanrossum Oct 25, 2021 •

edited

anthrotype commented Oct 25, 2021

anthrotype commented Oct 25, 2021

anthrotype commented Oct 25, 2021

anthrotype commented Oct 25, 2021

behdad commented Oct 25, 2021

dscorbett Oct 25, 2021

Support variable feature syntax #2432

Support variable feature syntax #2432

Conversation

simoncozens commented Oct 22, 2021

simoncozens commented Oct 22, 2021

ctrlcctrlv commented Oct 22, 2021 • edited

simoncozens commented Oct 22, 2021

ctrlcctrlv commented Oct 22, 2021

simoncozens commented Oct 22, 2021

ctrlcctrlv commented Oct 22, 2021

behdad commented Oct 22, 2021

simoncozens commented Oct 22, 2021

anthrotype left a comment

Choose a reason for hiding this comment

anthrotype Oct 25, 2021

Choose a reason for hiding this comment

anthrotype Oct 25, 2021

Choose a reason for hiding this comment

anthrotype Oct 25, 2021

Choose a reason for hiding this comment

ctrlcctrlv Oct 25, 2021

Choose a reason for hiding this comment

justvanrossum Oct 25, 2021 • edited

Choose a reason for hiding this comment

anthrotype commented Oct 25, 2021

anthrotype commented Oct 25, 2021

anthrotype commented Oct 25, 2021

anthrotype commented Oct 25, 2021

behdad commented Oct 25, 2021

dscorbett Oct 25, 2021

Choose a reason for hiding this comment

ctrlcctrlv commented Oct 22, 2021 •

edited

justvanrossum Oct 25, 2021 •

edited