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 1 commit 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 <[email protected]>
NUnit Tests 1 files ± 0 1 suites ±0 10m 51s ⏱️ + 2m 38s Results for commit b8a2dd1. ± Comparison against base commit ac89f08. This pull request removes 2 and adds 34 tests. Note that renamed tests count towards both. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1029 +/- ##
==========================================
+ Coverage 33.02% 33.09% +0.07%
==========================================
Files 1202 1204 +2
Lines 278232 278559 +327
Branches 37169 37226 +57
==========================================
+ Hits 91880 92196 +316
+ Misses 158502 158493 -9
- Partials 27850 27870 +20
🚀 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.
| ["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.
| ["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.
| ["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.
| 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.
| 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.
|
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.
| 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.
| } | ||
| 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.
|
Correction incorporated into the follow-up summary. |
2 similar comments
|
Correction incorporated into the follow-up summary. |
|
Correction incorporated into the follow-up summary. |
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