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. |
johnml1135
left a comment
johnml1135
left a comment
There was a problem hiding this comment.
Second pass over the diff, focused on the areas the first review didn't reach — binary parsing edge cases, handle/cache lifetime, and the menu rebuild path. Three new issues, all inline below. None are blockers on their own, but #1 and #2 both silently discard the exact data this change exists to surface, so they're worth a decision before merge rather than after.
For completeness, several things I specifically went looking for and found correct — recording them so nobody re-treads the ground:
- The GSUB/GPOS header
FeatureListOffsetread at offset 6 is right for both v1.0 and v1.1 (v1.1'sFeatureVariationsOffsetis appended after the existing fields, so nothing shifts). - The
FeatureParamsCharacterVariantsfield offsets inReadCharacterVariantParams(labelNameId @+2, numNamedParameters @+8, firstParamNameId @+10) match the spec layout exactly. kGrLangFeature(1) cannot collide with a packed OpenType tag —IsValidOpenTypeTagrequires all four chars in 0x20–0x7e, so the smallest possible packed value is orders of magnitude above 1.ReadFeatureList'syield breakon a corrupt FeatureRecord reads like a truncation bug but is deliberate, andTruncatedFeatureList_ReturnsFeaturesBeforeTheBreakdocuments it as intended.FontFeatureCacheKey.FromHdckeys on the full LOGFONT (face, height, weight, italic, charset, pitch) rather than face name alone, so no collisions between same-named fonts and no staleness across style changes.- The Graphite and non-OpenType fallback provider selection is structurally unchanged; no regression there.
- The robustness tests assert specific values (
.Tag,.FontSuppliedLabel,.Options) with no silentAssert.Ignoreor vacuous assertions.
| 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.
Bug: character-variant options past the 31st are silently dropped.
var optionCount = Math.Min(info.Options.Count, Math.Max(0, maxValues - 1));maxValues is always kMaxValPerFeat (32) — see the call site at L742 — which is a constant inherited from the Graphite menu (// See FmtFntDlg.h for real defn.) and carries no meaning for OpenType. Meanwhile the reader happily parses up to MaxNamedParameters = 1024 options per feature.
So for any cvXX declaring more than 31 named parameters, options 32+ are unreachable in the UI, with no ellipsis, no "more…" item, and nothing in the trace to say anything was dropped. The user sees a complete-looking list that isn't.
Failure scenario: a font ships cv01 with 40 named glyph variants (decorative and some CJK families genuinely do this). The menu offers None + options 1–31. Option 37, which the font documents and which cv01=37 would render correctly, cannot be selected at all — and a stored cv01=37 from another tool round-trips through ParseFeatureString into a value the menu can't represent.
The 32 cap is a Graphite-era artifact rather than a deliberate OpenType limit. Either raise it for the OpenType provider, or — if a 40-item flyout is undesirable UI — cap deliberately and say so, but don't let it be invisible.
| 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.
Bug (or at minimum a design limitation worth confirming): same-tag records are merged across scripts with no script awareness.
if (!byTag.TryGetValue(info.Tag, out existing) || IsRicher(info, existing))
byTag[info.Tag] = info;Read flattens the entire FeatureList from both GSUB and GPOS and dedupes purely by tag, keeping whichever record scores higher in IsRicher (has a label, or has more options). Nothing tracks which script or langsys each FeatureRecord actually came from.
OpenType explicitly allows the same tag to appear as separate FeatureRecords for different scripts with different featureParams. When that happens, the "richest" record wins globally and the other script's label and option set are discarded.
Failure scenario: a font declares cv01 for latn with 5 named options and cv01 for cyrl with 3 different ones. The Latin record scores higher and wins. A user setting up a Cyrillic writing system sees the five Latin option names, picks the fourth, and we persist cv01=4 — which for Cyrillic either selects a different variant than the label promised or does nothing at all.
I want to be fair: the Read docstring documents this behaviour ("when a tag appears in more than one script/language the richest record wins"), so it's a declared simplification, not an accident. But it's worth revisiting here specifically because font features in FieldWorks are configured per writing system, and a writing system knows its script — so unlike a general-purpose text engine, we're in a position to pick the right record rather than guess. Passing the WS's script tag down to the reader and preferring that script's record (falling back to DFLT, then to richest) would be a modest change.
At a minimum, worth confirming this is an accepted limitation for 9.3 rather than something we discover from a bug report against a multi-script font.
| } | ||
| var discoveredTags = tags.ToArray(); | ||
| var discovered = OpenTypeFontFeatureInfoReader | ||
| .Read(tag => s_tableReader(hdc, MakeTableTag(tag))).ToArray(); |
There was a problem hiding this comment.
Minor: s_tableReader is read outside the lock that guards its writes.
var discovered = OpenTypeFontFeatureInfoReader
.Read(tag => s_tableReader(hdc, MakeTableTag(tag))).ToArray();This read sits between the two lock (s_cacheLock) blocks, but UseTableReaderForTests only ever mutates s_tableReader while holding that same lock. Inconsistent lock discipline on a mutable static.
Production impact is nil — nothing swaps the reader outside tests. The realistic failure is a flaky test: if fixtures ever run in parallel (or a future one uses a background thread), a GetFeatureInfos call can pick up a half-swapped reader, or run against the real GDI reader after another fixture has installed its stub, producing failures that don't reproduce in isolation.
Cheapest fix is to snapshot the field into a local inside the first lock block and close over the local. Also worth noting while we're here: s_cacheOrder makes eviction FIFO rather than LRU despite the MaxCacheEntries framing — harmless at 32 entries, just not what the name suggests.
Correction to follow-up 1/3 — I got several facts wrong thereI wrote that comment from memory without checking the sources, and then went back and verified it properly. Several claims were wrong, including one that points at the wrong edit to the exact line I asked you to change. Correcting them here rather than quietly editing, since you may have already read it.
There is no published DirectWrite default feature list. My third table column was a reconstruction presented as a citable source; Microsoft's CSS Fonts 4 does not mandate "Advisory prose with no normative force" overstated it. Plenty of the registry is advisory, but not all: Minor: in HarfBuzz, And I can't substantiate the Uniscribe caveat. I said Uniscribe applies So does the recommendation survive? Yes, but the reason was wrong, and the corrected reason is more useful. My argument was "those tags match no engine's default set." The truth is closer to the reverse: every default-on tag in the catalog is registry-endorsed, so the catalog is internally consistent and whoever seeded it was following a real source. That reframes this from "someone made mistakes" to "two legitimate sources disagree, and we picked the wrong one for this purpose." The two questions are different:
A checkbox in Font Options is answering the second question. It claims "this is currently being applied to your text." If we render through So the recommendation is unchanged — trim the default-on set to what the renderer actually applies — but the catalog should carry a comment stating that policy ("default-on reflects what our shaping engine applies, not what the OpenType registry recommends; these differ"). Without it, the next person reads the registry, sees One genuinely open item: whether |
Correction to follow-up 2/3 — one unverified claimSmaller than the other correction, but worth flagging because it was the part of that comment most likely to become someone's ticket. I suggested that the Graphite fonts we already ship are a translation corpus we could harvest — that SIL Graphite fonts carry localized feature names written by people who know both the typography and the language, and that those would beat what a general translator produces for terms like "Contextual Alternates." The capability is real; the payoff is unverified. Graphite's Feature Table does support per-language name IDs — that part checks out against the Graphite/GDL documentation. What I did not check, and stated as though I had, is whether the Graphite fonts bundled in this repo ( Treat it as a hypothesis to test in about ten minutes with fontTools before scoping any work around it, not as an established fact. If the shipped fonts turn out to be English-only, the wider SIL font catalogue (Charis, Doulos, Andika, Scheherazade) is the next place to look, since those are the fonts our users actually select. Everything else in that comment I did verify and it holds:
The |
Correction to follow-up 3/3 — I had the LibreOffice licensing backwardsIn that comment I said we could take LibreOffice's font-features dialog as "design inspiration only, not code", because "LibreOffice is MPL 2.0 / LGPLv3+ and That reasoning is wrong. MPL 2.0's Secondary Licenses clause (Exhibit B) explicitly names GPL 2.0, LGPL 2.1, AGPL 3.0 "or any later versions" as compatible, and it applies automatically unless a file carries the "Incompatible With Secondary Licenses" notice. I checked I stated a legal constraint that doesn't exist, which could have ruled out an option we actually have. If we ever do build the richer dialog, borrowing from LibreOffice's implementation is on the table — subject to the usual attribution and header requirements, and to someone who isn't me confirming it, since I was confidently wrong about this once already. "Design inspiration only" may still be the right engineering call — their dialog is built around HarfBuzz's feature enumeration and VCL widgets, so the reusable part is probably the layout and interaction model rather than the code. But that's a judgement about fit, not a licensing prohibition, and I shouldn't have presented it as the latter. The rest of that comment I did verify and it holds: |
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