Skip to content

Allow hyphen and special characters in Feature File glyph names #5358

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.

Sign up for GitHub

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

Jump to bottom
Merged
skef merged 13 commits into from
Nov 10, 2024
Merged

Allow hyphen and special characters in Feature File glyph names #5358

skef merged 13 commits into from
Nov 10, 2024

Conversation

@iorsh
Copy link
Contributor

@iorsh iorsh commented Jan 29, 2024

This PR implements support of hyphens in glyph names, per OpenType Feature File Specification 2.f.i. Specifically, hyphen (-) and other special characters _.*+:^|~ must be supported in glyph names, and whenever hyphen is encountered, the implementation shall first attempt to find a glyph with the respective name, and only upon failure the implementation shall regard the hyphen as a range specifier.

Without hyphen support we incorrectly reject feature files for fonts with hyphens in glyph names (Iosevka being a one major example)

The entire issue of glyph names probably warrants some further discussion.

  • FontForge UI warns to use only alphanumerics, periods and underscores, but accepts everything
  • FontForge Python interface allows everything
  • Adobe Font Feature allows all the special characters in "development" mode, but not as the first character.
  • Microsoft OpenType specification allows only alphanumerics, period and underscore, anywhere in the name.

Type of change

  • Non-breaking change

@iorsh
Copy link
Contributor Author

iorsh commented Feb 25, 2024

This PR mostly affects fea_ParseGlyphClass().

The case if ( tok->type==tk_char && tok->tokbuf[0]=='-' ) is functionally unaffected, only split into two subroutines.

The case if ( tok->type==tk_name ) is first tried as a possible range, then falls back to the existing flow if it's not a range.

@skef
Copy link
Contributor

skef commented Feb 25, 2024

I know I'm supposed to be reviewing this, and I have looked at it. It seems a bit hairy and more risky than some of the other recent PRs and I want to go over it a few more times before I approve.

@iorsh
Copy link
Contributor Author

iorsh commented Feb 25, 2024

I know I'm supposed to be reviewing this, and I have looked at it. It seems a bit hairy and more risky than some of the other recent PRs and I want to go over it a few more times before I approve.

I agree with your reserved approach, this stuff is risky indeed. I'll think about some application of bulk runner to get more confidence in it.

@iorsh
Copy link
Contributor Author

iorsh commented Feb 25, 2024

Ran tests/bulk_test.sh with the following environment:

FF_PY_SCRIPT_LINE="\
f=fontforge.open(argv[1]);\
f.generateFeatureFile('Out.fea')"$'\n'"\
for l in f.gsub_lookups: f.removeLookup(l)"$'\n'"\
for l in f.gpos_lookups: f.removeLookup(l)"$'\n'"\
f.mergeFeature('Out.fea'); f.save(argv[2])"
FF_OUTPUT_FILE="Output.sfd"

Results (other than simple PASS):

./bulk_test.sh: line 56: 341824 Segmentation fault      (core dumped) $FF_BIN_SYSTEM -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
./bulk_test.sh: line 56: 341829 Segmentation fault      (core dumped) $FF_BIN_PROJECT -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
PASS: http://svn.savannah.gnu.org/viewvc/*checkout*/freefont/trunk/freefont/sfd/FreeSansOblique.sfd
./bulk_test.sh: line 56: 342269 Aborted                 (core dumped) $FF_BIN_SYSTEM -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
./bulk_test.sh: line 56: 342274 Aborted                 (core dumped) $FF_BIN_PROJECT -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
FAIL: https://github.com/marutan/riscos-free-fonts/raw/main/Fonts/Sassoon.Primary.Bold.sfd
./bulk_test.sh: line 56: 342526 Segmentation fault      (core dumped) $FF_BIN_SYSTEM -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
./bulk_test.sh: line 56: 342533 Segmentation fault      (core dumped) $FF_BIN_PROJECT -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
FAIL: https://github.com/fontforge/debugfonts/raw/master/jieub-bt-ps.sfd

Failures seem unrelated, as these are crashes on both system and project code, but I'll take a look anyway.

@iorsh
Copy link
Contributor Author

iorsh commented Mar 15, 2024 

./bulk_test.sh: line 56: 342269 Aborted                 (core dumped) $FF_BIN_SYSTEM -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
./bulk_test.sh: line 56: 342274 Aborted                 (core dumped) $FF_BIN_PROJECT -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
FAIL: https://github.com/marutan/riscos-free-fonts/raw/main/Fonts/Sassoon.Primary.Bold.sfd

This crash is unrelated, but there is a simple fix in 3c9c621 - we are releasing SplineChar object we don't own, and trying to do it multiple times because the font contains multiple duplicate kerning pairs applied to this char.

@iorsh
Copy link
Contributor Author

iorsh commented Mar 15, 2024 

./bulk_test.sh: line 56: 342526 Segmentation fault      (core dumped) $FF_BIN_SYSTEM -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
./bulk_test.sh: line 56: 342533 Segmentation fault      (core dumped) $FF_BIN_PROJECT -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
FAIL: https://github.com/fontforge/debugfonts/raw/master/jieub-bt-ps.sfd

Unrelated crash, fixed in 30389c7

@iorsh
Copy link
Contributor Author

iorsh commented Mar 15, 2024 

./bulk_test.sh: line 56: 341824 Segmentation fault      (core dumped) $FF_BIN_SYSTEM -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
./bulk_test.sh: line 56: 341829 Segmentation fault      (core dumped) $FF_BIN_PROJECT -c "$FF_PY_SCRIPT_LINE" $SFD_FILE $FF_OUTPUT_FILE > /dev/null 2>&1
PASS: http://svn.savannah.gnu.org/viewvc/*checkout*/freefont/trunk/freefont/sfd/FreeSansOblique.sfd

Unrelated crash - occurs both in master code and in the pull request.
FF crashes probably because of malformed FEA is applied back to the file, and then file is saved. I prefer not to fix the crash, as it would just occlude the underlying issue.

Stack:

libfontforge.so.4!SFD_DumpSplineFontMetadata(FILE * sfd, SplineFont * sf) (/home/iorsh/devel/fontforge/fontforge/sfd.c:2525)
libfontforge.so.4!SFD_Dump(FILE * sfd, SplineFont * sf, EncMap * map, EncMap * normal, int todir, char * dirname) (/home/iorsh/devel/fontforge/fontforge/sfd.c:2642)
libfontforge.so.4!SFDDump(FILE * sfd, SplineFont * sf, EncMap * map, EncMap * normal, int todir, char * dirname) (/home/iorsh/devel/fontforge/fontforge/sfd.c:2956)
libfontforge.so.4!SFDWrite(char * filename, SplineFont * sf, EncMap * map, EncMap * normal, int todir) (/home/iorsh/devel/fontforge/fontforge/sfd.c:3061)
libfontforge.so.4!SFDWriteBak(char * filename, SplineFont * sf, EncMap * map, EncMap * normal) (/home/iorsh/devel/fontforge/fontforge/sfd.c:3186)
libfontforge.so.4!SFDWriteBakExtended(char * locfilename, SplineFont * sf, EncMap * map, EncMap * normal, int s2d, int localRevisionsToRetain) (/home/iorsh/devel/fontforge/fontforge/sfd.c:3124)
libfontforge.so.4!PyFFFont_Save(PyFF_Font * self, PyObject * args) (/home/iorsh/devel/fontforge/fontforge/python.c:16717)

@iorsh
Copy link
Contributor Author

iorsh commented Mar 15, 2024

For me, the bulk tests are clear now.

@skef
Copy link
Contributor

skef commented Oct 6, 2024

@iorsh how thoroughly is the CID path tested? If it's been tested well I'm ready to approve this.

@iorsh
Copy link
Contributor Author

iorsh commented Oct 15, 2024

Tested CID path with the following tests/bulk_test.sh environment:

FF_PY_SCRIPT_LINE="\
f=fontforge.open(argv[1]);\
f.cidConvertByCmap('/home/iorsh/devel/cmap-resources/Adobe-Identity-0/CMap/Identity-H')"$'\n'"\
f.generateFeatureFile('Out.fea')"$'\n'"\
for l in f.gsub_lookups: f.removeLookup(l)"$'\n'"\
for l in f.gpos_lookups: f.removeLookup(l)"$'\n'"\
f.mergeFeature('Out.fea'); f.save(argv[2])"
FF_OUTPUT_FILE="Output.sfd"

with some tweaks. All tests passed except for Mothanna.sfd, which crashes on both system and project with unrelated stack.

Tweaks:

  1. Test script crashes FF nearly universally due to 27806fe; I had to apply the fix on the system to obtain meaningful comparison.
  2. Must zeroize "CreationTime" and "ModificationTime" to suppress bogus diffs.

@iorsh
Copy link
Contributor Author

iorsh commented Nov 10, 2024

@skef, I tested the CID path with the entire bulk test suite - see above. The results are ok, could you please merge?

@skef skef merged commit 42f3316 into fontforge:master Nov 10, 2024
@iorsh iorsh deleted the hyphen_names branch March 28, 2025 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skef skef skef approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iorsh @skef