LT-22638: Show OpenType feature names and multi-valued variants - #1029
LT-22638: Show OpenType feature names and multi-valued variants#1029jasonleenaylor wants to merge 2 commits into
Conversation
- Read the font-supplied names and named options from GSUB featureParams and the name table so features are comprehensible and character variants are selectable, as with Graphite. - Add OpenTypeFontFeatureInfoReader adapted from Paratext's OpenTypeFeatures.Ttf. - Add OpenTypeFeatureCatalog registered-feature hidden and default-on classification plus English names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NUnit Tests 1 files ± 0 1 suites ±0 12m 12s ⏱️ + 3m 59s Results for commit 7a4be53. ± Comparison against base commit ac89f08. This pull request removes 8 and adds 1726 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1029 +/- ##
==========================================
+ Coverage 33.02% 40.91% +7.89%
==========================================
Files 1202 1558 +356
Lines 278232 366731 +88499
Branches 37169 40406 +3237
==========================================
+ Hits 91880 150053 +58173
- Misses 158502 187435 +28933
- Partials 27850 29243 +1393
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Four merge issues; details are inline:
- Preserve
cvNNoption positions when labels cannot decode. - Align default-on features with verified renderer behavior, spec, and docs; hide
size. - Move user-visible catalog names to
.resx. - Add default-on persistence tests: unset stays absent; unchecked writes
liga=0.
Next: address or explicitly defer each inline finding.
| { | ||
| var option = LookupName(names, firstParamNameId + i); | ||
| if (!string.IsNullOrEmpty(option)) | ||
| resolved.Add(option); |
There was a problem hiding this comment.
Preserve undecodable cvNN option slots. Dropping one renumbers later values, so selecting "option 2" can persist the wrong glyph. Use a numbered placeholder and add a missing-option-name test.
There was a problem hiding this comment.
Fixed. Slots keep their position now and a name that won't decode stays null for the UI to number, since that index is the value we persist and the doc comment on Options already promised it.
I surveyed 192 font files here to see how this behaves in practice: Windows fonts, the per-user store, DistFiles and Downloads. 477 cvNN features declare named options across 15 fonts and not one of them has a slot the reader can't resolve, so nobody is hitting this today. Worth fixing anyway since a user can install any font, but it isn't a live corruption.
The same survey changed what I did about the all-fail case. 420 of those 477 features declare exactly one option, so if that single name fails to decode a "None / Option 1" dropdown would be worse than the checkbox we show now. I kept the fallback to a binary toggle when none of the names decode, and only preserve positions when at least one does.
Two tests: a middle slot missing keeps {"First", null, "Third"}, and no slot decoding leaves the feature binary.
| ["psts"] = Hidden("Post Base Substitutions"), | ||
| ["pwid"] = Visible("Proportional Width"), | ||
| ["qwid"] = Visible("Quarter Widths", OpenTypeFeatureDefaultState.Off), | ||
| ["rand"] = Visible("Randomize", OpenTypeFeatureDefaultState.On), |
There was a problem hiding this comment.
The catalog's default-on set exceeds the four tags specified by the proposal, design, tasks, and shipped docs. Align it with verified Uniscribe defaults or update the spec/docs deliberately; confirm kern, then pin the exact set in a test.
There was a problem hiding this comment.
rand, chws, cpsp and halt are off now, size is hidden per your other comment, and DefaultOnFeatures_AreOnlyTheDocumentedFour pins the set. CommonlyDefaultOnFeatures_AreDefaultOn checks one tag per TestCase, so it could only ever catch a tag going missing, never a fifth being added, which is how these got in.
Chasing this down turned up something more interesting than the five tags, and it changed my mind twice.
IsDefaultOn is the one place this provider isn't equivalent to the Graphite one. GraphiteFontFeatureProvider.GetFeatureValues gets defaultValue straight out of the font, and the same font table drives the Graphite shaper, so the checkbox and the renderer can't disagree. OpenType has nowhere to put that: a GSUB/GPOS FeatureRecord has no default-value field, and only cvNN/ssNN carry featureParams, which hold labels and not defaults. So the OpenType provider fills the same out-parameter from a static table, and the two providers share a signature that hides the difference. Under Graphite a wrong default is impossible; here it's an unverified claim about Uniscribe.
That's the actual defect. The five tags were a symptom.
I went to the OpenType registry expecting it to be the equivalent of the Graphite font table, and it isn't. It recommends cpsp and rand be on by default, and chws and halt for horizontal CJK layout in applications that don't implement CLREQ/JLREQ/KLREQ, which we don't. So on the registry's own wording the nine-tag set was closer than the four-tag set. But the registry is telling an application to apply a feature, which here means writing tag=1, and marking a tag default-on does the opposite: the string stays silent and the feature is never requested. Marking cpsp default-on doesn't honour that recommendation, it prevents it.
Marking a tag on only sets the initial checkbox for a feature the writing system doesn't mention. Marked on, cpsp, rand, chws and halt showed as enabled while the text was unaffected, and a user's first click wrote tag=0, so the gesture meant to enable them disabled them. Marked off, the first click writes tag=1, a no-op if Uniscribe applies the feature anyway, and tag=0 is one click further.
chws and halt couldn't both have been on anyway. Each is mutually exclusive with the other and with every horizontal glyph-width feature, and the catalog has no per-script state to choose between them.
Which brings me back to your wording. "Align it with verified Uniscribe defaults" is the only phrasing that closes this loop, and it's a stronger point than I first read it as. kern is in the same position as the other three, just better-supported by shaping convention: UniscribeSegment.cpp:792 parses the feature string and :835 switches to ShapePlaceRunWithOpenType with an explicit record list once anything is set, so whether kern survives that switch is measurable and unmeasured. ShapeText_OpenTypeFeatureToggleChangesShapingData would answer it but skips itself, because Times New Roman gave no deterministic liga delta. That's LT-22774.
Docs/opentype-font-features.md now carries all of this, and the pinning test points at it, so the next person who wants to add cpsp back reads the reason first.
| ["salt"] = Visible("Stylistic Alternatives", OpenTypeFeatureDefaultState.Off), | ||
| ["sinf"] = Visible("Scientific Inferiors", OpenTypeFeatureDefaultState.Off), | ||
| ["smcp"] = Visible("Lowercase to Small Capitals", OpenTypeFeatureDefaultState.Off), | ||
| ["size"] = Visible("Optical size", OpenTypeFeatureDefaultState.On), |
There was a problem hiding this comment.
size is optical-sizing metadata, not a meaningful size=1 toggle. Hide it under the spec's "otherwise not user-configurable" rule.
There was a problem hiding this comment.
Hidden. The spec is more definite than "not meaningful": the size Feature table must not reference any lookups, and its featureParamsOffset points at five uint16 values. There are no lookups for size=1 to trigger, so the toggle could never have done anything. Zero of the 192 fonts I surveyed declare size at all, so nobody has met the dead checkbox either.
One correction on the citation. spec.md:41 reads "required for script shaping AND not user-configurable", and size isn't shaping-required, so that scenario doesn't reach it. The clause that does is the requirement above it at :34, which scopes discovery to user-configurable tags.
Worth flagging so it doesn't look like a contradiction later: the registry does say "This feature should be active by default" for size. That's telling the application to honour the optical-size data when the user picks a point size, not to show a ticked checkbox. Hiding it and honouring it are different questions, and the second one isn't in scope here.
| ["abvf"] = Hidden("Above Base Forms"), | ||
| ["abvm"] = Hidden("Above Base Mark"), | ||
| ["abvs"] = Hidden("Above Base Substitutions"), | ||
| ["afrc"] = Visible("Vertical Fractions", OpenTypeFeatureDefaultState.Off), |
There was a problem hiding this comment.
Move these user-visible English names to FwUtilsStrings.resx; C# fallbacks bypass Crowdin and violate AGENTS.md. Preserve or deliberately replace the translated keys removed from FwCoreDlgControls.resx.
There was a problem hiding this comment.
Fixed, 57 keys added, so the resx now has one for each of the 71 visible features instead of 14. You're right on the rule and AGENTS.md:15 states it outright: keep localization in .resx, don't hardcode translatable UI strings.
I put them in FwCoreDlgControls.resx rather than FwUtilsStrings.resx. Three reasons. The resx-first lookup already exists at FontFeaturesButton.cs:1010, so the keys are consumed where they land with no new plumbing. The 14 that were already there are in that file, so splitting would leave feature labels in two places. And these are UI labels, which sit oddly in a utility assembly. crowdin.json:20 globs Src/**/*.resx, so either destination is a Crowdin source with no config change. Say the word if you'd still rather have them in FwUtils and I'll move them.
The 54 hidden names stay in C#. They're never shown, so they aren't translatable UI strings.
One thing I'm not claiming: nothing yet stops the next added feature from having no key and quietly falling back to the C# name. All 71 resolve today because I generated the keys from the catalog, and ResxOpenTypeFeatureLabels_MapToVisibleFeatures guards the other direction, that a key maps to a visible feature. The missing direction, that a visible feature has a key, is in LT-22774.
On the nine removed keys, all deliberate:
kstidOpenTypeFeatureValueOffand_ValueOnhad to go, and localizing them would have been a bug.:763-781lowercases those strings and matches them against "on" and "off" to decide whether a feature is binary. A translated "Ein"/"Aus" fails that match, and the checkbox becomes a submenu._aaltand_ccmphad to go too. They're hidden now, andResxOpenTypeFeatureLabels_MapToVisibleFeaturesfails if a key maps to a hidden feature._ss01through_ss05are replaced bykstidOpenTypeFeatureStylisticSet, "Stylistic Set {0}", so they're still translatable as one format string instead of five literals.
| return candidateScore > existingScore; | ||
| } | ||
|
|
||
| private static byte[] SafeGet(Func<string, byte[]> tableSource, string tag) |
There was a problem hiding this comment.
Keep graceful degradation, but trace parse failures. Bare catches make malformed fonts indistinguishable from fonts without features and remove the prior diagnostic path; route failures through the existing trace switch or an equivalent FwUtils source.
There was a problem hiding this comment.
Fixed, all three, and they route through FontFeatureSettings.DiagnosticsSwitch, the equivalent FwUtils source you pointed at. It's internal and the reader is in the same assembly, so nothing new was needed.
The three are the table read at :119, name-record decoding at :267, and the Mac Roman encoding fallback at :336. Split by severity rather than all warnings: the table read and the encoding fallback are warnings, name-record decoding is info. That one runs per name record, and the comment at :261 says non-decodable platforms are expected and fall back by design, so logging each as a warning would bury the two that mean something.
The Mac Roman one is worth a warning on its own account. If codepage 10000 is unavailable we silently fall back to ASCII, which mangles any non-ASCII Mac name rather than failing.
Degradation is unchanged. ThrowingTableSource_IsSwallowed and UndecodablePlatform_FallsBackToNoLabel both still pass, so discovery still degrades instead of throwing; it just says so now.
Expected volume, from the 192-font survey: no font produced a name slot the reader couldn't resolve, so these should be silent in practice and a message should mean something real rather than being background noise.
The truncation case in FontFeaturesButton traces too, but through FontFeatures.OpenType rather than the FwUtils switch, since DiagnosticsSwitch is internal to FwUtils and that code is in FwCoreDlgControls. That one belongs with your kMaxValPerFeat comment.
| return cachedTags.ToArray(); | ||
| OpenTypeFontFeatureInfo[] cached; | ||
| if (s_featureCache.TryGetValue(cacheKey, out cached)) | ||
| return cached; |
There was a problem hiding this comment.
Return a copy or immutable collection. Returning the cached array exposes shared mutable state across callers, unlike the previous .ToArray() behavior.
There was a problem hiding this comment.
Fixed, and you were right about the comparison. I went and looked at GetFeatureTags on main before this PR: it did return cachedTags.ToArray() on the hit path and return discoveredTags.ToArray() on the miss path. So the copy wasn't missing, it was removed, which makes this a regression of an existing safeguard rather than hardening that was never there. That changed how I treated it.
The cache stores a ReadOnlyCollection now and both paths hand back that same wrapper. Two reasons for wrapping once at insert instead of copying on the way out: no allocation on the hit path, which is where this is called from menu building, and a caller who casts to IList<T> gets a NotSupportedException rather than a silent success. IReadOnlyList<T> on its own was only a compile-time promise, so a cast back to the array type could write through to the cache and change what every later caller saw for that font.
One change beyond what you asked for. The miss path used to return its own discovered even when another thread had won the insert race, so two callers could hold different equal instances. It now returns whatever is in the cache, so they share one. Happy to take that back out if you'd rather keep the diff literal.
|
Three follow-ups:
Next: decide which items belong in this PR versus Jira. |
|
Consolidated into the follow-up summary. |
1 similar comment
|
Consolidated into the follow-up summary. |
There was a problem hiding this comment.
Three follow-up findings; details are inline:
- OpenType character variants after option 31 are unreachable.
- Same-tag records can expose one script's labels and values in another writing system.
s_tableReaderis read outside the lock guarding test-time replacement.
Next: fix or explicitly accept each limitation.
| if (info != null && info.Options.Count > 0) | ||
| { | ||
| // value 0 = "None", value i = the i-th named character-variant option. | ||
| var optionCount = Math.Min(info.Options.Count, Math.Max(0, maxValues - 1)); |
There was a problem hiding this comment.
OpenType character variants after option 31 are silently unreachable because this reuses Graphite's kMaxValPerFeat. Raise the OpenType limit or expose truncation explicitly; add a test for more than 31 named options.
There was a problem hiding this comment.
Taking your second option, exposing it rather than raising it, and I'll explain why since you offered both.
I surveyed 192 font files: Windows fonts, the per-user store, DistFiles and Downloads. 477 cvNN features declare named options across 15 fonts, and they break down as 420 with one option, 40 with two, 16 with three, and one with four. The maximum anywhere is four, in ScheherazadeNew cv82. Nothing is above 31, and nothing is close.
The spec ties that number to something physical: numNamedParameters "should not exceed the number of alternate glyphs in lookups associated with the feature". So 32 named options means a font shipping 32 named alternate glyphs of a single character. That isn't a font anyone has made, and a test for it would have to hand-build the table, which is why I skipped the test rather than writing one that can only pass against a fixture. Your call if you'd rather have it anyway.
Two things I did instead. GetFeatureValues now traces a warning at :1042 when it truncates, naming the tag and both counts, so a font that ever hits this says so before a user notices. And the comment on the constant pointed at FmtFntDlg.h, which isn't in the tree, and nothing in any .h or .cpp references kMaxValPerFeat either. The ceiling is a vestigial managed constant, not a platform limit, so the comment now says what the number is and where it came from rather than sending the next reader to a file that doesn't exist. Your "reuses Graphite's" reading was exactly right, and it's the same pattern as the default-on discussion: the OpenType path inherited a Graphite constant that had no OpenType justification.
Same class, worse failure mode, also unreachable: the reader's own MaxNamedParameters = 1024 at OpenTypeFontFeatureInfoReader.cs:59. Above that it returns no options at all, so a variant selector silently becomes a binary toggle. Given the observed maximum is four I've left it, but it's the more damaging of the two if anything ever gets there.
| foreach (var info in ReadFeatureList(table, names)) | ||
| { | ||
| OpenTypeFontFeatureInfo existing; | ||
| if (!byTag.TryGetValue(info.Tag, out existing) || IsRicher(info, existing)) |
There was a problem hiding this comment.
Same-tag feature records are merged across scripts by "richest" metadata, so a Cyrillic writing system can receive Latin labels and values. Prefer the writing system's script, then DFLT; otherwise document this as an accepted limitation.
There was a problem hiding this comment.
Taking your second option, with the measurement written down rather than just a note.
Your description is right, and understated: ReadFeatureList at :129 walks the flat feature list and never touches ScriptList, so the reader has no concept of script at all. It isn't preferring the wrong script, it doesn't know scripts exist. A font that labelled the same tag differently per script would show one script's strings to the other, exactly as you say.
What decided me against building it: across 192 fonts, 176 cv/ss tags appear more than once in a flat feature list, so IsRicher at :106 runs constantly, and zero of those duplicates differ in derived label or options. Every one is identical, so it's always choosing between equal records and the choice never matters.
Your scenario is also the normal case here rather than a corner, which is what made me check it properly. CharisSIL and DoulosSIL both cover DFLT, cyrl and latn; GentiumPlus adds grek; ScheherazadeNew covers arab and latn. So a Cyrillic writing system using a font that also covers Latin is the ordinary situation, and it's inert because these fonts define each feature once and register it under every script they cover.
The real fix means passing the writing system's script into Read and keeping per-script records instead of deduplicating by tag, which changes the signature and its only caller. That's a fair amount of work for a difference no font in the corpus exhibits, so the remarks on Read now say what the limitation is, why it's accepted, and what would break it, and the docs carry the same. Whoever meets the first font that labels a tag per script will find the reason rather than the behaviour.
| } | ||
| var discoveredTags = tags.ToArray(); | ||
| var discovered = OpenTypeFontFeatureInfoReader | ||
| .Read(tag => s_tableReader(hdc, MakeTableTag(tag))).ToArray(); |
There was a problem hiding this comment.
Snapshot s_tableReader inside s_cacheLock before invoking it. Tests write this mutable static under the lock but production reads it outside, allowing parallel fixtures to observe the wrong reader.
There was a problem hiding this comment.
Done, and there's a second hole behind it that the snapshot alone doesn't close.
Snapshotting the reader inside the lock stops a caller invoking the wrong one, which is what you described. It doesn't stop this: a discovery misses the cache and leaves the lock, the reader is swapped and the cache cleared, then the discovery finishes and seeds the freshly cleared cache with old-reader data. Every later caller then gets results from a reader that is no longer installed. The snapshot doesn't help because that discovery legitimately captured the old reader; the fault is that its result outlives the swap.
So the reader and a generation counter are read together at :1166, the counter is bumped on both the swap and the restore in UseTableReaderForTests, and at :1186 a result whose generation has moved is handed back to the caller that asked for it but not written to the cache.
Being straight about reachability: this isn't reachable today. Nothing in Src carries [Parallelizable] or LevelOfParallelism, so despite Test.runsettings setting NumberOfTestWorkers=0, NUnit runs fixtures sequentially within an assembly, and the MaxCpuCount parallelism is across assemblies, which are separate processes with separate statics. In production the field is never reassigned at all, it's set once to ReadTable.
I fixed it anyway where I deferred your option-31 and cross-script items, and the difference is cost rather than severity. This is four lines in one private nested class with no signature change. Those two needed a font nobody ships and a signature change respectively. It also matters a bit more now: the ReadOnlyCollection change makes this method the only place the cache gets populated.
|
Correction incorporated into the follow-up summary. |
2 similar comments
|
Correction incorporated into the follow-up summary. |
|
Correction incorporated into the follow-up summary. |
An undecodable character-variant option name was dropped from the option list, which renumbered every option after it. That index is the value persisted to the writing system, so choosing one variant could apply a different glyph. Slots now keep their position and the UI numbers any the font did not name; a font that names none of them still falls back to a binary toggle. The default-on set held nine tags where the proposal, design and docs name four. Dropped rand, chws, cpsp and halt, and hid size, which references no lookups at all, so size=1 could never have done anything. A test now pins the exact set instead of asserting one tag at a time, which is why five extra tags survived. IsDefaultOn is the one place this provider is not equivalent to the Graphite one. GraphiteFontFeatureProvider reads defaultValue out of the font, and the same font table drives the Graphite shaper, so the checkbox and the renderer cannot disagree. OpenType records no per-feature default anywhere, so this provider asserts one from a static table. Marking a tag on only sets the initial checkbox for a feature the writing system does not mention: wrongly on, it displays as enabled while the text is unaffected and the user's first click writes tag=0, so the gesture meant to enable it disables it; wrongly off, the first click writes tag=1, a no-op if the renderer applies the feature anyway. The OpenType registry does suggest cpsp, rand, chws and halt be on by default, but that instructs an application to apply a feature, which marking a tag default-on prevents by leaving the string silent. Docs/opentype-font-features.md records the reasoning; LT-22774 measures what the renderer actually applies. Moved the 57 remaining visible feature names into FwCoreDlgControls.resx, taking it from 14 keys to 71, so no user-visible name is English-only in C#. AGENTS.md requires this and crowdin.json already globs Src/**/*.resx. Swallowed parse failures now report through the font-feature trace switches, so a malformed font is distinguishable from one with no features. Option counts above the menu ceiling report too rather than truncating in silence; no font we have measured declares more than four. The feature cache handed out its own array behind an IReadOnlyList, which a caller could cast back and write through, so it now stores a ReadOnlyCollection. The table reader and a generation counter are read together under the cache lock, so a discovery that began under one reader cannot seed the cache after the reader changed. Documented the script-blind deduplication as an accepted limitation with the reason: across 192 installed fonts, 176 cv/ss tags appear more than once and every duplicate carries identical metadata.
Comment hygiene (advisory)57 comment-style violation(s) in the lines this branch adds since
32 more not listed here -- see this check's log. Fix them per |
|
Round two. All nine inline comments answered, seven with code changes and two documented as accepted limitations. Details are in the threads; this is the method and the parts that don't belong in any one of them. Fixed: cvNN option positions, the default-on set plus a test pinning it, Font survey. Several of these came down to what fonts actually do, so I measured rather than reasoned. 192 font files: Windows fonts, the per-user font store, Verification. LT-22774 covers what's deferred: confirming the default-on set against measured renderer behavior, and a test that every visible feature resolves a resx label. Fix version FW 9.3, related to LT-22638. The blocker there is that
On your three follow-ups: defaults are aligned with the docs and pinned, with the measurement in LT-22774. Names are in resx, and I don't know whether the bundled fonts carry reusable translations, so treat that as still open. The UI follow-up for a selection summary and grouped live preview isn't filed. Say the word and I'll file it, or take it yourself if you have the shape in mind. Nothing is resolved from my side. Items 7 and 8 are deferrals and 2 is partly one, so they're yours to close or push back on. |
3 participants
Summary
Fixes LT-22638. Makes OpenType font
features usable in Font Options: features show human-readable names from the
font (or a registered-feature catalog), and character variants with multiple
named alternates are selectable instead of a single On/Off toggle — parity with
the existing Graphite experience, with no change to the dropdown menu, so both
the Writing System and Styles font dialogs inherit it.
What changed
OpenTypeFontFeatureInfoReader(FwUtils) — parses GSUB/GPOS featurelists,
featureParams, and thenametable via a table-source delegate (GDIGetFontDatain the app, font-file bytes in tests). Bounds-checked; malformedfonts degrade to tag-only records. Adapted from Paratext's
OpenTypeFeatures.Ttf.OpenTypeFeatureCatalog(FwUtils) — classifies registered features(hidden / default-on) and supplies English names, seeded from Paratext and
audited against the OpenType registry (
dligvisible,aalthidden,kerndefault-on).
FontFeaturesButton— character variantswith named options become "None + option" submenus stored as
cvNN=k; labelsresolve font-supplied → resx → catalog → numbered fallback; default-on features
initialize enabled; hidden features filtered.
aalt,ccmp) and entries orphaned by the rewrite (ss01–ss05,ValueOff/ValueOn).Docs/opentype-font-features.mdupdated.Storage, rendering, and CSS/Word export are untouched — the renderer-neutral
tag=valuestring already carried multi-values.Testing
cv43"Capital Eng" + 3options,
ss01"Single-story a and g", GPOSmark/mkmk) and ScheherazadeNew (
cv70"Damma").priority, hidden filtering,
cv43=2round-trip, resx↔catalog consistency.catalog/info 100%, reader 86%.
🤖 Generated with Claude Code
This change is