Fix LT-22351: OpenType/Graphite font features from style defaults now reach Dictionary Preview#1005
Draft
johnml1135 wants to merge 4 commits into
Draft
Fix LT-22351: OpenType/Graphite font features from style defaults now reach Dictionary Preview#1005johnml1135 wants to merge 4 commits into
johnml1135 wants to merge 4 commits into
Conversation
liblcm's BaseStyleInfo.ProcessStyleRules previously dropped a style's default ktptFontVariations (font features) when loading from persisted Rules (sillsdev/liblcm#388). FieldWorks compensated for this in the Styles dialog only, via StyleInfo.LoadDefaultFontFeatures, which manually rescanned style.Rules for ktptFontVariations after calling the base constructor. The Dictionary Preview CSS path (CssGenerator.GenerateCssStyleFromLcmStyleSheet -> AddFontInfoCss) never went through StyleInfo -- it reads plain BaseStyleInfo objects straight out of LcmStyleSheet -- so it had no such adapter and silently lost default font features, meaning font-feature-settings never reached the generated preview CSS. Now that liblcm loads default font features itself, remove the FieldWorks-side adapter and let StyleInfo(IStStyle) rely entirely on BaseStyleInfo.ProcessStyleRules, as the LT-22324 design doc intended once the round-trip test passed through the authoritative liblcm path. Add a CssGenerator regression test that builds a style the way LcmStyleSheet does in production (a real IStStyle with persisted Rules, wrapped in BaseStyleInfo -- not a TestStyle double with an in-memory ExplicitValue) and asserts that default font features reach the generated CSS end-to-end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Note: CI on this PR is expected to be red until sillsdev/liblcm#388 is released and Build/SilVersions.props is bumped. The committed SilVersions.props still pins upstream 11.0.0-beta0173, which lacks the liblcm fix, so both the pre-existing gate test (SaveToDB_DefaultFontFeatures_RoundTripsThroughRules) and the new CssGenerator regression test in this PR will fail against that pin in CI. This is the expected pre-release state, not a broken branch -- see the DO NOT MERGE note above. |
NUnit Tests 1 files ±0 1 suites ±0 10m 24s ⏱️ + 3m 34s For more details on these failures, see this check. Results for commit 5bc3727. ± Comparison against base commit f505729. ♻️ This comment has been updated with latest results. |
Review follow-ups on the LT-22351 branch: - Widen SafelyAddStyleToSheetAndTable to accept BaseStyleInfo (both collections it touches already store BaseStyleInfo) and add a matching SafelyRemoveStyleFromSheetAndTable helper. - The persisted-Rules preview test now uses those helpers and removes its style in a finally block: m_styleSheet is per-fixture but the base class undoes all data changes per test, so leaving a BaseStyleInfo wrapping a real IStStyle behind would strand a stale entry with a dead RealStyle that can crash later tests order-dependently. - Add GenerateCssForConfiguration_NormalStyleOwnFontFeatures_ BeatWritingSystemDefaultFontFeatures: now that liblcm loads a style's own default ktptFontVariations from persisted Rules (sillsdev/liblcm#388), a Normal style WITH its own features must win over the writing system's DefaultFontFeatures fallback in AddFontInfoCss; the existing sibling test only covered Normal WITHOUT its own features. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Word export path (WordStylesGenerator.AddFontInfoWordStyles) has the same precedence logic as the CSS path but had no coverage through the authoritative liblcm load path: the existing typography test used a TestStyle double with an in-memory ExplicitValue (bypassing BaseStyleInfo.ProcessStyleRules) and the existing Normal-style test only covered the writing-system DefaultFontFeatures fallback branch. Add two tests mirroring the CSS-side coverage: - GenerateCharacterStyleFromLcmStyleSheet_DefaultFontFeaturesFromPersisted StyleRules_AddsWordTypographyProperties: a real IStStyle whose persisted Rules carry ktptFontVariations, wrapped in a plain BaseStyleInfo the way LcmStyleSheet does in production, produces the expected w14 typography properties (sillsdev/liblcm#388). - GenerateCharacterStyleFromLcmStyleSheet_NormalStyleOwnFontFeatures_Beat WritingSystemDefaultFontFeatures: a Normal style with its own persisted features wins over the WS DefaultFontFeatures fallback (font family still falls back to the WS default font). Both tests clean up in finally blocks so no stale entry wrapping an undone IStStyle is left in the fixture-lifetime stylesheet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
design.md Decision 11, proposal.md, and research.md still described StyleInfo.LoadDefaultFontFeatures as an existing, load-bearing compatibility adapter. Update those passages to record that the adapter was removed under LT-22351 once liblcm's BaseStyleInfo.ProcessStyleRules gained the ktptFontVariations case (sillsdev/liblcm#388) and the Decision 11 gating condition (SaveToDB_DefaultFontFeatures_RoundTripsThroughRules passing through the authoritative path) was satisfied. Surgical wording updates only; no restructuring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
liblcm's
BaseStyleInfo.ProcessStyleRulesdropped a style's defaultktptFontVariations(OpenType/Graphite font features) when loading from persistedRules(sillsdev/liblcm#388). FieldWorks'StyleInfohad a compensating adapter, so Data Entry was unaffected — but Dictionary Preview CSS generation (CssGenerator.GenerateCssStyleFromLcmStyleSheet→AddFontInfoCss) reads plainBaseStyleInfostraight out ofLcmStyleSheet, bypassing the adapter, sofont-feature-settingsnever reached the generated preview CSS.Depends on liblcm #388 — do not merge until a liblcm release containing it is published and
Build/SilVersions.propsis bumped to that version (currently pinned to11.0.0-beta0173, which predates the fix).What changed
StyleInfo.LoadDefaultFontFeaturescompatibility adapter (Src/FwCoreDlgs/FwCoreDlgControls/StyleInfo.cs) now that liblcm loads default font features itself.BaseStyleInfo/IStStylepath (not theTestStyledouble) for: Dictionary Preview CSS, CSS precedence (a style's own features beat the writing system's fallback), and the equivalent Word-export path.add-opentype-font-featuresopenspec docs, which still described the removed adapter as load-bearing.Why CI is red right now
This is expected, not a regression — every failure here traces back to the same cause:
SaveToDB_DefaultFontFeatures_RoundTripsThroughRules, 2×CssGenerator*, 2×LcmWordGenerator*)11.0.0-beta0173— before #388's fix — so features are still silently dropped in this run. They pass locally against a repacked liblcm build with the fix.These tests will go green on their own once liblcm #388 merges, a release publishes to nuget.org, and
Build/SilVersions.propsis bumped to reference it — no FieldWorks-side code change needed.JIRA
LT-22324 partially implements → LT-22351
🤖 Generated with Claude Code
This change is