Postscript names for UFO sources #489

RickyDaMa · 2023-10-17T15:46:16Z

Related to: #248

Reads postscript names from the lib.plist of UFO sources, and then uses them when writing the post table.

Compiling Oswald using its Makefile yields an identical post table to that of a fontc-compiled UFO-based Oswald.

Tests not yet updated to account for the extra field on StaticMetadata, or extra parameter of StaticMetadata::new. Very much just a proof of concept to gather feedback and see if there are any changes wanted in our approach before updating tests. Naturally, tests should also be added for reading/writing these names.

For discussion:

How do we want to handle duplicate postscript entry values?
What auto-generation do we need to implement for glyph names that are not included in AGLFN? What does fontmake do?
Do we want to add a CLI flag to use design names instead of production names, similar to fontmake's --no-production-names?
Shall we implement this feature for glyphs2fontir in this PR?

Implementation notes/thoughts:

Do we want to check for equality of public.postscriptNames across all masters?
Should we read postscript names and glyph order at the same time? Or at least, without loading the plist into memory twice?
What error variant should we use for "public.postscriptNames is present, but not a dictionary"?
Does postscript_names belong in the top-level StaticMetadata, or in misc (or context, like GlyphOrder)?
Should we use a smaller type for postscript name values, i.e. GlyphName or Box<str> instead of String?

Tangent (though tests will currently fail if run): https://matklad.github.io/2022/10/24/actions-permissions.html

Store postscript names in StaticMetadata of IR Postscript names taken from default master Empty default for Glyphs.app sources (for now)

anthrotype · 2023-10-18T14:04:24Z

Thanks!

How do we want to handle duplicate postscript entry values?

duplicate as in the same key repeated more than once? does plist even allow that? Probably not as dict keys are unique. We should do whatever a normal plist parser does when encountering these, nothing more.

What auto-generation do we need to implement for glyph names that are not included in AGLFN? What does fontmake do?

we should defer to that for a follow-up PR, it's ok to first implement writing the explicit postscriptNames; ufo2ft.postProcessor can make up AGLFN-compatible names from glyphs codepoints, we could implement that later.

Do we want to add a CLI flag to use design names instead of production names, similar to fontmake's --no-production-names?

that's useful sometimes for debugging, to have human-readable names instead of uniXXXX, so I would say yes we want that. preferably matching the fontmake's CLI flag name so it's already familiar to fontamake's users.

Shall we implement this feature for glyphs2fontir in this PR?

can be done in a follow-up PR. We can file an issue so we don't lose track of this temporary asymmetry between frontends

Do we want to check for equality of public.postscriptNames across all masters?

taking the one from the designspace.lib or, if that missing, the one from the default master UFO lib.plist should suffice. It's not our job to validate these IMO.

Should we read postscript names and glyph order at the same time? Or at least, without loading the plist into memory twice?

yeah, maybe, makes sense

I have a meeting now, will come back to this later

fontir/src/ir.rs

anthrotype · 2023-10-18T14:56:01Z

What error variant should we use for "public.postscriptNames is present, but not a dictionary"?

WorkError::ParseError(lib_plist_file, "Not a dictionary".to_string()) as in

fontc/ufo2fontir/src/source.rs

Line 478 in 6381929

    
           .ok_or_else(|| WorkError::ParseError(lib_plist_file, "Not a dictionary".to_string()))

Does postscript_names belong in the top-level StaticMetadata, or in misc (or context, like GlyphOrder)?

it's fine in StaticMetadata, I think

anthrotype · 2023-10-18T15:03:50Z

taking the one from the designspace.lib or, if that missing, the one from the default master UFO lib.plist should suffice.

it looks like ufo2ft only looks for postscriptNames defined in the default master UFO lib.plist, that's the one it uses in its postProcessor when compiling a VF. I confused this with the other "skipExportGlyphs" key which ufo2ft attempts to find in the designspace.lib first. Maybe it should do that as well for postscriptNames.. Anyway matching current fontmake.py is fine for now.

Golf code around about for readability (no nested Options)

Create type alias PostscriptNames

RickyDaMa · 2023-10-19T14:01:52Z

How do we want to handle duplicate postscript entry values?

duplicate as in the same key repeated more than once?

Duplicate as in the same value repeated more than once. There's currently no handling for this

taking the one from the designspace.lib or, if that missing, the one from the default master UFO lib.plist should suffice

Yeah this was how we were going to initially set out to implement it before we also realised it's not looked for in the DS in the Python stack. Maybe something to revise down the line, but probably out of scope for this PR

I've pushed a couple of the basic suggested changes, and shall now look into adding the CLI flag and removing the double load-from-disk

anthrotype · 2023-10-19T14:08:12Z

Duplicate as in the same value repeated more than once. There's currently no handling for this

I don't think the python toolchain checks for this, does it? In any case, feel free to add or not add the additional check. To me it feels more like the job of a linter

Hoolean · 2023-10-19T14:31:31Z

Hello all, and many thanks for the speedy review Cosimo :)

I don't think the python toolchain checks for this, does it? In any case, feel free to add or not add the additional check. To me it feels more like the job of a linter

IIRC in ufo2ft we do some sanitisation of names and add a suffix to ensure uniqueness, although I cannot see a uniqueness requirement in the post table spec; I am not sure if this was added for platform support or just to cooperate with ttLib's use of names as unique IDs. In most real-world instances where I have seen duplicate post names in sources it has been unintentional, however, and so I am torn.

anthrotype · 2023-10-19T14:44:24Z

Thanks Harry. Indeed, looks like that dedup logic was deliberately added in ufo2ft to address some fonttools quirk:
googlefonts/ufo2ft#274

while the post spec may not require the names to be unique, it kind of defeats the purpose of having multiple glyphs mapped to the same postscript name if you use the name to identify a specific glyph or reverse-cmap, which is what AGLFN is primarily about. If cmap maps from unicode codepoint to glyph (ids), it's impossible for a given codepoint to map to more than one glyph at a time.

I think it's rare that sources have duplicate postscript glyph names, and if they do it's usually by mistake, I don't see a use case for it besides wanting to make a deliberately broken font for testing purposes. But sure let's add the dedup logic

anthrotype · 2023-10-19T14:46:55Z

sure let's add the dedup logic

also feel free to kick the can down the road and not do it in this PR (can file an issue)

anthrotype · 2023-10-19T14:51:57Z

anyway, when decompiling post, fonttools seems to be able to deduplicate glyph names as needed:

https://github.com/fonttools/fonttools/blob/6d531fed449d299c31e4cf69957b3fdd81cb982d/Lib/fontTools/ttLib/tables/_p_o_s_t.py#L117-L122

since we are not using fonttools to compile the binary font, we don't have this problem, so it's only about whether or not we think a valid post should contain unique names. Given the post spec does not enforce this requirement, I believe we should not either.

anthrotype · 2023-10-19T14:54:51Z

it's impossible for a given codepoint to map to more than one glyph at a time.

while that's true, if you also think about alternate glyphs substituted for via GSUB, then indirectly it is possible to say that both glyph "A" and glyph "A.ss01" are mapped to 0x0041, and in fact AGL spec says one should drop the suffix after the period when mapping from glyph name to character string: https://github.com/adobe-type-tools/agl-specification#2-the-mapping

yeah I think I have convinced myself that we do not need to do any checks for duplicates here

anthrotype · 2023-10-19T14:57:47Z

I don't see a use case

now I do see it. If you don't care about the nice debug glyph names and being able to distinguish glyph "A" from "A.ss01" (e.g. when decompiling the font in a font editor) but only care about AGL compliance and reverse-cmapping of names to characters for old PDF distillers, then it may be beneficial to actually reuse postscript names for alternate glyphs that ultimately are mapped to the same unicode codepoint(s) such that the post table is as compact as needed.

RickyDaMa · 2023-10-19T15:26:13Z

I've added a duplicate value check. I agree with Harry that this is often a mistake, so I'm erring on the side of being (hopefully) more helpful. Given that it's not strictly spec to have to have unique values, it's just a warning

I've also added the --no-production-names flag. I think I've followed the existing modus operandi there, feel free to point out if I haven't

Shall look at deduplicating the plist loads tomorrow, then onto tests!

ufo2fontir/src/source.rs

Loses some resolution in a very rare error (public.postscriptNames not being a dictionary)

Fix glyph_order signature in other test

Double negatives are the curse of programmers everywhere Add debug printing of bitflags in arg_default_matches_flags_default for ease of debugging Add Default::default() to params of StaticMetadata::new where needed

RickyDaMa · 2023-10-20T16:21:19Z

Okay, I think the PR is in a good state now. I've addressed the suggestions, and implemented tests. Let me know if there's anything further you'd like to see :)

Once merged, I'll open any appropriate issues to track the deficiencies in the current implementation: not supporting glyphs sources, not matching fontmake's name generation, etc.

fontc/src/args.rs

rsheeter · 2023-10-20T22:23:39Z

fontc/src/args.rs

        let arg_default = Args::parse_from(vec!["program", "--source", "dont.care"]).flags();
        let flags_default = Flags::default();
+        debug!("flags_default: {:#032b}", flags_default.bits());
+        debug!("args_default:  {:#032b}", arg_default.bits());


Instead of firing up the logger and writing to debug make the assert do what you want, either by altering the left/right values or by adding a custom panic string to show the {:#032b} strings

Sure thing. Only downside is it means I can't align the values when they're printed

ufo2fontir/src/source.rs

rsheeter

LGTM subject to nits being fixed

Note corresponding fontmake flag for --no-postscript-names Move debug logs into assertion Change duplicate_values_check name Make filter expr one line Only check for postscript duplicate values when warn logs are enabled Don't duplicate code in tests (add Flags param to build_static_metadata)

RickyDaMa · 2023-10-23T11:19:33Z

All comments addressed, let me know if there's anything else!

ufo2fontir/src/source.rs

Hoolean · 2023-10-23T12:06:22Z

fontir/src/serde.rs

@@ -70,6 +72,7 @@ impl From<StaticMetadataSerdeRepr> for StaticMetadata {
            from.axes,
            from.named_instances,
            from.glyph_locations.into_iter().collect(),
+            from.postscript_names,
        )


Hello again! A wee question, tangential to this PR: we noticed that misc is serialised in StaticMetadata→StaticMetadataSerdeRepr but not deserialised in StaticMetadataSerdeRepr→StaticMetadata; do we populate it elsewhere, or could this be a bug? 🐛

looks like a bug to me

Shall I sort this as part of this PR, or file this as a bug?

technically it's a separate issue, might be worth tackling it separately

Raised as #499

After PR #489, fontc should use the same glyph postscript names as fontmake does, so we no longer need to pass the --no-production-names flag to compare the two sets of fonts

RickyDaMa added 2 commits October 17, 2023 16:00

Load postscript names from lib.plist for UFO sources

78eb7b2

Store postscript names in StaticMetadata of IR Postscript names taken from default master Empty default for Glyphs.app sources (for now)

Use postscript names when writing post table

aa77e0e

anthrotype reviewed Oct 18, 2023

View reviewed changes

fontir/src/ir.rs Outdated Show resolved Hide resolved

RickyDaMa added 2 commits October 19, 2023 14:45

Error handling for public.postscriptNames not being a dictionary

e10de9d

Golf code around about for readability (no nested Options)

Change postscript name values to GlyphName

c6cfe63

Create type alias PostscriptNames

Warn about duplicate public.postscriptNames values

04221e6

Add arg/flag to disable using production names

4efae34

RickyDaMa commented Oct 19, 2023

View reviewed changes

ufo2fontir/src/source.rs Outdated Show resolved Hide resolved

RickyDaMa added 6 commits October 20, 2023 12:16

Only load lib.plist once

2972157

Loses some resolution in a very rare error (public.postscriptNames not being a dictionary)

Only print one warning for any re-used production names

4bf4733

Add happy path test

938d906

Fix glyph_order signature in other test

Edge case tests for postscript_names

5e96484

Fix --no-production-names default & test compilation

fb35536

Double negatives are the curse of programmers everywhere Add debug printing of bitflags in arg_default_matches_flags_default for ease of debugging Add Default::default() to params of StaticMetadata::new where needed

Test postscript name loading within StaticMetadataWork

cf3e2b3

RickyDaMa marked this pull request as ready for review October 20, 2023 16:08

Remove println!

c2cdeca

RickyDaMa requested a review from anthrotype October 20, 2023 16:21

RickyDaMa requested a review from rsheeter October 20, 2023 16:21

rsheeter reviewed Oct 20, 2023

View reviewed changes

fontc/src/args.rs Show resolved Hide resolved

rsheeter reviewed Oct 20, 2023

View reviewed changes

ufo2fontir/src/source.rs Outdated Show resolved Hide resolved

rsheeter reviewed Oct 20, 2023

View reviewed changes

ufo2fontir/src/source.rs Outdated Show resolved Hide resolved

rsheeter reviewed Oct 20, 2023

View reviewed changes

ufo2fontir/src/source.rs Outdated Show resolved Hide resolved

rsheeter reviewed Oct 20, 2023

View reviewed changes

ufo2fontir/src/source.rs Outdated Show resolved Hide resolved

rsheeter approved these changes Oct 20, 2023

View reviewed changes

RickyDaMa requested a review from rsheeter October 23, 2023 11:18

anthrotype reviewed Oct 23, 2023

View reviewed changes

ufo2fontir/src/source.rs Outdated Show resolved Hide resolved

anthrotype reviewed Oct 23, 2023

View reviewed changes

ufo2fontir/src/source.rs Show resolved Hide resolved

Hoolean reviewed Oct 23, 2023

View reviewed changes

RickyDaMa added 2 commits October 23, 2023 13:09

Rename flags_without_emit_ir -> default_test_flags

2600315

Remove unwanted assertion

82ad1bd

anthrotype approved these changes Oct 23, 2023

View reviewed changes

anthrotype added this pull request to the merge queue Oct 23, 2023

Merged via the queue into googlefonts:main with commit 4f35733 Oct 23, 2023
8 checks passed

anthrotype mentioned this pull request Oct 23, 2023

[ttx_diff.py] remove fontmake's --no-production-names flag #498

Merged

RickyDaMa deleted the postscript-names branch October 23, 2023 14:55

This was referenced Oct 23, 2023

Match fontmake glyph names #248

Open

fontir::MiscMetadata isn't deserialized as part of StaticMetadata #499

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Postscript names for UFO sources #489

Postscript names for UFO sources #489

RickyDaMa commented Oct 17, 2023 •

edited

Loading

anthrotype commented Oct 18, 2023

anthrotype commented Oct 18, 2023

anthrotype commented Oct 18, 2023

RickyDaMa commented Oct 19, 2023 •

edited

Loading

anthrotype commented Oct 19, 2023

Hoolean commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

RickyDaMa commented Oct 19, 2023

RickyDaMa commented Oct 20, 2023 •

edited

Loading

rsheeter Oct 20, 2023

RickyDaMa Oct 23, 2023

rsheeter left a comment

RickyDaMa commented Oct 23, 2023

Hoolean Oct 23, 2023

anthrotype Oct 23, 2023

RickyDaMa Oct 23, 2023

anthrotype Oct 23, 2023

RickyDaMa Oct 23, 2023

Postscript names for UFO sources #489

Postscript names for UFO sources #489

Conversation

RickyDaMa commented Oct 17, 2023 • edited Loading

anthrotype commented Oct 18, 2023

anthrotype commented Oct 18, 2023

anthrotype commented Oct 18, 2023

RickyDaMa commented Oct 19, 2023 • edited Loading

anthrotype commented Oct 19, 2023

Hoolean commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

anthrotype commented Oct 19, 2023

RickyDaMa commented Oct 19, 2023

RickyDaMa commented Oct 20, 2023 • edited Loading

rsheeter Oct 20, 2023

Choose a reason for hiding this comment

RickyDaMa Oct 23, 2023

Choose a reason for hiding this comment

rsheeter left a comment

Choose a reason for hiding this comment

RickyDaMa commented Oct 23, 2023

Hoolean Oct 23, 2023

Choose a reason for hiding this comment

anthrotype Oct 23, 2023

Choose a reason for hiding this comment

RickyDaMa Oct 23, 2023

Choose a reason for hiding this comment

anthrotype Oct 23, 2023

Choose a reason for hiding this comment

RickyDaMa Oct 23, 2023

Choose a reason for hiding this comment

RickyDaMa commented Oct 17, 2023 •

edited

Loading

RickyDaMa commented Oct 19, 2023 •

edited

Loading

RickyDaMa commented Oct 20, 2023 •

edited

Loading