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 <noreply@anthropic.com>
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:
|
johnml1135
left a comment
johnml1135
left a comment
There was a problem hiding this comment.
Reviewed this along two axes: does it follow the repo's documented standards (AGENTS.md, .github/instructions/*), and does it faithfully implement the OpenSpec bundle the PR ships. Nice piece of work overall — the reader is carefully bounds-checked, the Graphite path is genuinely untouched, and the dlig visible / mark,mkmk,init,ccmp hidden behaviour, name-only labels and malformed-table degradation all verify correctly against the spec.
Four things I'd want addressed before merge, plus some smaller cleanups. Inline comments have the detail; summarising here:
Correctness
ReadCharacterVariantParamsdrops undecodable option names instead of holding their position, which renumbers the options after them. That's a silent wrong-glyph, not a graceful fallback — see inline.
Spec drift
- The default-on set in
OpenTypeFeatureCatalogisliga,calt,kern,clig, pluschws,cpsp,halt,rand,size. The proposal,design.mddecision 5,tasks.md1.2 and theDocs/opentype-font-features.mdthis PR ships all name only the first four, so the shipped doc is now wrong.randandhaltin particular aren't applied by default by any shaper, so they'd render pre-checked and misrepresent what the user is actually seeing — the exact defect this change sets out to fix. sizeshouldn't be a user toggle at all; it's optical-size metadata andsize=1is meaningless. It fails the spec's own hidden test ("otherwise not user-configurable") — the same rationale used to hideaalt.
Standards
- The ~120 English feature names in
OpenTypeFeatureCatalogare hardcoded in C# and reach the menu viaGetFeatureLabel→GetEnglishName.AGENTS.mdsays "Keep localization in.resx; do not hardcode translatable UI strings", andFwUtilsStrings.resxalready exists in the same assembly. Aggravating that the PR removes translatable resx entries (_aalt,_ccmp,_ss01–_ss05) at the same time. The invariant"Off"/"On"returns are fine — callers pattern-match those, and the comment explaining why is correct.
Test gaps
The two normative persistence scenarios aren't covered: an unset default-on feature must stay absent from the stored string, and unchecking it must write liga=0. The mechanism reads correct (nDefault seeds display, GenerateFeatureString skips Int32.MaxValue, ItemClickHandler writes 0) but nothing asserts it, and tasks.md 2.4 omits them too. A test pinning the default-on set would also have caught #2.
Smaller things, take or leave
OnClickregenerates the whole feature string from visible ids only, so a storedaalt=1(visible before this change, hidden after) is silently erased on the next click. Pre-existing machinery, newly reachable.ssXX/cvXXtag-shape parsing now exists twice —IsCharacterVariantTag/IsStylisticSetTagin the reader, andTryGetSetNumber(tag, 's', 's', …)in the button. Worth one shared helper.OpenTypeFeatureDefaultState.Unspecifiedvs.Offis never distinguished (onlyIsDefaultOnis consumed), andEnglishNameon aHidden(…)entry can never be displayed.private static readonly Encoding MacRomanbreaks thes_prefix its siblings use (s_layoutTables,s_entries);.editorconfighas no naming rules so nothing will flag it.CharisSilTestFontVersion 6.200inFwUtilsTests.csprojduplicatesCharisSilVersioninBuild/PackageRestore.targets, kept in sync by a comment only — a mismatch makes the reader tests silentlyAssert.Ignorerather than fail. There's precedent inTestViews.vcxproj, so low priority.- A few doc comments could shrink: the
/// Initializes a new instance of the <see cref="…"/> class.boilerplate on both new ctors adds nothing over the signature.
| { | ||
| var option = LookupName(names, firstParamNameId + i); | ||
| if (!string.IsNullOrEmpty(option)) | ||
| resolved.Add(option); |
There was a problem hiding this comment.
This drops undecodable option names instead of holding their position, which silently renumbers everything after them.
If a cvNN declares three options and option 2's name record is undecodable, resolved ends up [option1, option3], so option 3 is offered to the user as value 2 and persists cvNN=2. The shaper then applies alternate 2 — the wrong glyph, with no indication anything went wrong.
The spec is explicit that the index/value mapping is positional: "value i selects the i-th named option" and "selecting the second option SHALL persist cv25=2". Suggest keeping the slot with a numbered placeholder ("Option 2", or the existing Character Variant N style) so index ↔ value stays 1:1 and the degradation is visible rather than silent.
The robustness tests cover a missing label, but not a missing option name, which is why this path isn't caught.
| ["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.
rand as default-on looks wrong, and it's outside what the change authorised.
The proposal, design.md decision 5 and tasks.md 1.2 all specify the default-on set as liga, calt, kern, clig. This catalog also marks chws (L82), cpsp (L86), halt (L105), rand (here) and size (L166) as On, and the Docs/opentype-font-features.md shipped in this same PR still documents the four-tag set — so the doc is now inaccurate.
rand and halt in particular aren't applied by default by any shaper, so they'd show pre-checked while doing nothing, and unchecking one writes rand=0 for a feature the user never had on. That's the same class of "UI misrepresents rendering" problem LT-22638 exists to fix.
Either trim the set back to the documented four, or amend the spec and doc deliberately — but not silently. A test asserting the exact default-on set would keep the catalog, the doc and the spec from drifting again.
| ["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 should be Hidden, not a visible default-on toggle. It's optical-size metadata (the size featureParams carry a design-size range for the font), not something a shaper switches on or off — size=1 doesn't mean anything downstream.
The spec's hidden criterion is a feature "required for script shaping or … otherwise not user-configurable", which is exactly the rationale used to move aalt to hidden in this change. size fits the second half of that.
| ["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.
These ~120 English names are user-visible — they reach the Font Options menu through GetFeatureLabel → OpenTypeFeatureCatalog.GetEnglishName(tag) — so they belong in .resx.
AGENTS.md: "Keep localization in .resx; do not hardcode translatable UI strings", and .github/instructions/fieldworks-ui-review.instructions.md says the same. FwUtilsStrings.resx already exists in this assembly, so there's a home for them. The class doc acknowledges the tension ("a resx entry may override it for the UI") but the fallback still ships English strings from C#.
Worth noting the PR moves in the opposite direction at the same time: kstidOpenTypeFeature_aalt, _ccmp and _ss01–_ss05 come out of FwCoreDlgControls.resx, and design.md decision 3 said "the 21 existing (possibly translated) entries are preserved". Any of those with existing translations are lost.
(To be clear — the invariant "Off"/"On" returns in FontFeaturesButton are correct as-is, and the comment explaining that they're classification-only rather than display strings is a genuine improvement.)
| return candidateScore > existingScore; | ||
| } | ||
|
|
||
| private static byte[] SafeGet(Func<string, byte[]> tableSource, string tag) |
There was a problem hiding this comment.
SafeGet, DecodeName and CreateMacRoman all swallow everything with a bare catch, and this revision drops the file's only diagnostics (Trace.WriteLineIf(s_openTypeTraceSwitch, …)).
Degrading gracefully rather than throwing into the UI is right and the spec requires it — but with no trace output, a font that parses to nothing is indistinguishable from a font with no features, and there's nothing to go on when a user reports missing features. managed.instructions.md asks that we not swallow exceptions without logging context.
s_openTypeTraceSwitch is still there in FontFeaturesButton (L46); routing these through it (or an equivalent in FwUtils) would cost nothing at runtime and make field diagnosis possible.
| return cachedTags.ToArray(); | ||
| OpenTypeFontFeatureInfo[] cached; | ||
| if (s_featureCache.TryGetValue(cacheKey, out cached)) | ||
| return cached; |
There was a problem hiding this comment.
Minor: return cached; here and return discovered; at L1149 hand the caller the cached array directly, where the previous implementation returned .ToArray() copies. Nothing in the diff mutates the result today, but the cache is now shared mutable state across every caller of GetFeatureInfos. Returning a copy, or having the cache hold something genuinely immutable, keeps that from biting later.
Follow-up 1/3: what the default-on set should be, and whyExpanding on the inline comment about The OpenType registry is not the authority. It says things like "this feature should be on by default" in prose, inconsistently, and with no normative force. The actual authority is the shaping engine, and the engines have converged:
They agree almost exactly. Suggest we adopt the CSS Fonts L4 list as the cited authority — it's normative, published, stable, and it's what HarfBuzz implements, so it's the same answer with a URL a future reviewer can check. Better than maintaining our own opinion. The nice part: intersect that list with the tags this PR already hides as shaping-required (
Which is precisely what the proposal, Recommendation: trim to the four, add a one-line comment in One caveat worth recording in that comment. We shape through Uniscribe's OpenType path — |
Follow-up 2/3: how Graphite feature names are localized (they aren't, quite), and what that means hereContext for the inline There are three tiers of feature name, and only one of them is Crowdin's:
Tiers 1 and 2 are both English-pinned today. The language we ask for is hardcoded: public const int kUiCodePage = 0x00000409; // for now the UI language is US English
Tier 3 localizes for free, but only from { "source": "Src/**/*.resx",
"ignore": ["Src/**/*Tests/**/*", "Src/**/HelpTopicPaths.resx"] }Both It also makes the Recommendation — two parts: Now, in this PR: put the catalog names in a Separate Jira issue: wire And a nice bonus @johnml1135 raised: the Graphite fonts we already ship are themselves a translation corpus. SIL Graphite fonts carry localized feature names for languages we care about, written by people who know both the typography and the language. Those are almost certainly better than what a general translator would produce for terms like "Contextual Alternates." Harvesting them as seed translations for the tier-3 catalog strings is worth scoping into that same ticket. |
Follow-up 3/3: you can't see what's selected without opening the menuNot a defect in this PR — it's pre-existing — but this change makes it materially more noticeable, so it's worth raising while the code is warm. Current state. The control is a plain That was survivable when the menu held a handful of tags. After this PR, a font like Charis SIL 6.200 produces 40+ entries with submenus — genuinely good, and much harder to scan for "what did I turn on?" The improvement in reach makes the lack of a summary more visible, not less. Recommendation for this PR (or immediately after): summary text beside the button. Something like "3 features set", or better, the resolved list — "Capital Eng: Alt 2; Single-story a and g" — as a label next to the button and as its tooltip. It reuses the label resolution this PR already builds, needs no new dialog or menu machinery, and it's the single biggest fix for "what is currently on." Cheap enough that it doesn't need to wait for the bigger redesign. A close second, if it's easy: mark non-default entries in the menu (bold, or a leading bullet) so a user's own choices stand out from the defaults in a long list. Recommendation for a new Jira issue: a proper font-feature selection UI. The proposal's non-goals already anticipate this ("no new dialog, grouping, tooltips, or sample glyphs — tier 'b' presentation upgrade, candidate follow-up ticket"), so this is just putting a number on it. Scope worth considering: a feature list or grid grouped into Stylistic Sets / Character Variants / other, with a live preview of the selected sample text. Worth noting we already have the preview machinery — Sources of inspiration for that ticket:
Also worth a look before we design ours: Paratext 9.6, since it's the source of this PR's reader and the user bases overlap heavily. Consistency across SIL apps has value on its own, and it may already have solved some of this. |
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