LT-22691: Copy writing-system menu choices into the view override

[mark-sil]

In the new UI, changing which writing systems a multi-writing-system field shows — unchecking a toggle, re-checking one, or using the Configure dialog — now updates the Avalonia detail view immediately and persists per project. This completes the field-menu family started with Field Visibility and Move Field, and fixes the long-standing bug where unchecking a writing system left the row visible.

Builds on #1097 (the storage layer this writes into), which has merged; this branch is rebased onto main and the diff is only the new work. It also delivers two commitments from #1097's review discussion: the ordering canonicalization and the UI write path with auto-refresh.

Three commits, in reading order:

1. Registry refactor — the interceptor's HelpId switch becomes an OverrideCommandRegistry. Behavior-identical; the commit message carries the rationale (natively-handled commands become enumerable data, which a later per-menu-group completion check can query — a switch cannot).
2. The selection copy — writing-system items dispatch to the hidden adapter slice as before; the resulting selection is then copied into the project view override.
3. Bridge leaf contract — the interceptor receives the leaf's already-computed display properties (no second Display* round trip), and every leaf, default or retargeted, carries no execute action when disabled.

Where to look:

• The timing asymmetry is the load-bearing subtlety: toggles read the selection property (written before OnClick returns; the slice reacts later), Configure reads the slice (updated synchronously by the modal dialog). Reading the slice after a toggle stores the pre-click set — that was the bug. Locked by WritingSystemToggle_TwoVernaculars_StoresTheReducedThenRestoredSet, red under the old code.
• Canonicalization: the stored set is filtered to the slice's options in option order via StringSliceUtils.GetVisibleWritingSystems, so uncheck+recheck never reorders rows and junk tokens never restrict.
• Cancel writes nothing: the selection is snapshotted before dispatch; an unchanged Configure result is not copied.
• Guards fail safe with a log trail: unreadable adapter slice, a slice that isn't the clicked row's, and empty sets all decline to write.
• The bridge's disabled-no-execute rule applies to every tool's menus, not just the Lexicon.

Deliberately not here:

• "Show all right now" is visibly inert in the Avalonia view — deliberate: it is a transient reveal in legacy, and persisting it would pin the full set. The native transient reveal is the recorded follow-up.
• Reversal-entries rows (the plugin row neither raises this menu nor reads the restriction yet; the copy log-and-bails there).
• Mutation-command conversion and adapter end-state (the open LT-22691: Fix command routing through the hidden DataTree adapter #1079 direction question).

Verified: build.ps1 -CommentHygiene clean; 23/23 targeted menu fixtures; 213/213 Detail-fixture sweep after the bridge change. Manual testing is complete — the 9-scenario Sena 3 plan, including the Configure dialog lanes that cannot run headlessly.

---

Reading this a year from now — start here

This is the write path ("4b") for the SetVisibleWritingSystems override operation whose storage layer landed as #1097 ("4a"). The split existed because a native command layer — one possible answer to the #1079 adapter-direction question — would delete this copy code while the storage survives. This PR was developed stacked on #1097's branch and rebased onto main once that merged. The working review record lives in this description; .review/ is gitignored by design.

Decisions, and why

• A registry instead of the switch, honestly weighed. The switch would have worked (the id-less toggles only need one if). The registry is the deliberate "middle path" from the adapter end-state analysis: natively-handled commands become enumerable data, so a later per-menu-group completion check can ask what is covered and skip building the hidden adapter for a fully covered menu. If the direction ruling ends up plain per-command peeling forever, the cost is one 45-line class.
• "Copy", not "mirror." This codebase already uses "mirrors legacy X" to mean reimplements the same algorithm (a parity claim). The operation here is one-directional, after-the-fact state copying — so the vocabulary is copy (the action) and stored (what tests assert), and "mirror" was retired from these additions to avoid two senses of one word in one file.
• Copy-after-dispatch, not replacement. The hidden adapter slice owns the picker, the Configure dialog, and the legacy layout-inventory record; the Avalonia view composes from its own override store. Copying the outcome keeps both sides working without reimplementing either — and it is precisely the code a native command layer would later delete, which is why it is isolated to one registry entry and three private members.
• Show-all stays visibly inert (chosen over graying it out or reinterpreting it as a persistent clear). Legacy's command is a temporary reveal that reverts when the slice loses currency; every cheap way to make the button "do something" persistent was rejected as data corruption or dishonest UI. Inert-and-visible makes the gap obvious until the native transient reveal is built.
• The disabled-no-execute rule lives in the bridge, for all leaves. Real UI already blocked disabled clicks; the rule exists for programmatic invokers (tests, future keyboard/automation), and enforcing it once in the bridge deleted a ws-specific guard and made Execute != null a uniform invariant.

Deferred, and what would unblock it

• Show-all transient reveal (the desired end state): intercept the click into host-level transient state, recompose, and have DetailComposer.ApplyVisibleWritingSystems consult it — it must be composer-level because the restriction can come from the shipped layout, not just the override. The open design decision is expiry (legacy expires on slice-currency loss; Avalonia analogs are field focus loss or record navigation). Roughly 100–150 lines with tests.
• Reversal-entries rows: three missing pieces — the plugin row raising the standard menu, the row consuming VisibleWritingSystems, and the copy gaining a slice-agnostic options source (a small shared interface). Belongs to reversal-slice Avalonia parity; today the copy logs and declines on that slice type.
• Adapter end-state: whether the registry grows group-completion checks and native mutation commands is the LT-22691: Fix command routing through the hidden DataTree adapter #1079 direction question; nothing here commits either way.

Preflight review details

The branch went through the 8-angle adversarial review (line-by-line, removed-behavior, cross-file tracing, reuse, simplification, efficiency, altitude, conventions): 34 raw candidates deduplicated to 10 findings, each verified against quoted source. All ten are dispositioned; the significant ones and their fixes:

• Show-all was being persisted despite being a transient reveal in legacy (OnDataTreeWritingSystemsShowAll never persists; SetCurrentState(false) reverts) — excluded from the copy.
• Configure-Cancel wrote an override pinning the current set — before/after snapshot comparison; no change, no write.
• Raw property order and tokens were stored verbatim while legacy renders in options order — canonicalized through StringSliceUtils.GetVisibleWritingSystems; exact-order test assertions.
• A stale adapter slice could write the wrong row's set — object-identity guard with a log entry.
• A null override-target location silently disabled the copy — the ws entry now registers before locating.
• The reversal slice drives the same menu property — both lanes require a MultiStringSlice and log-and-bail otherwise.
• Bridge-level issues (double GetDisplayProperties per intercepted item; disabled leaves carrying execute) — the leaf-contract commit.
• Plus an unreachable guard removed, a silent-bail log added, and comment-standard fixes.

Validation: build.ps1 -CommentHygiene clean at every step; 23/23 targeted menu fixtures; 213/213 Detail sweep after the bridge change. The author reviewed every change line-by-line, approving names individually ("copy"/"stored" vocabulary, WritingSystemItem, CopyWritingSystemSelectionToOverride). Manual testing: the 9-scenario Sena 3 plan has been executed by the author — it is the only coverage for the Configure dialog lanes, which cannot run headlessly (modal).

🤖 Generated with Claude Code

---

This change is 

[thejambi]

You may want other eyes on this but !

@thejambi reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on mark-sil).

[johnml1135]

Reviewing this together with #1097, since they land as one change. The general reply is on #1097; the specific defects are here, where the code they concern actually lives.

Three inline comments below. Two more defects have no line in this diff to anchor on:

ViewDefinitionOverrideJsonSerializer.cs:29 (on #1097) -- forward compatibility. Adding a name to the op-kind map makes a file containing the new kind unreadable to any build predating it, and because the failure is whole-file a version rollback silently drops the user's Field Visibility and Move Field customisations too, not just writing systems. That blast radius is wider than the operation being added. Per-op skip-with-diagnostic is the behaviour I would want. Happy for it to be a separate PR, but I would like it agreed before this lands rather than carried as a documented deferral, because every future op kind inherits whatever we settle on.

DetailComposer.ApplyVisibleWritingSystems -- unavailable ids fail in the opposite direction to legacy. Unchanged by either PR, so no anchor anywhere. It ends return result.Count > 0 ? result : systems;, so a stored set naming only writing systems that have since been removed from the project shows all of them; legacy's GetVisibleWritingSystems returns empty and shows none. Delete a vernacular ws and the two UIs disagree immediately. Fix it or file it -- "fails safely" is fair, but it fails differently on each side.

Minor, no anchor: ViewDefinitionOverrideDiffer.cs:286 (on #1097) reports a dropped AddNode writing-system restriction as a Warning. Good that it is not silent, but is a Warning the right terminal state? A user who adds a field and restricts its writing systems gets a log line plus a field showing all of them, which reads as a bug rather than a documented limitation. Fine to leave if that path is not reachable from the UI yet -- worth a line in the description if so.

[johnml1135]

Defect: every writing-system change writes both stores, and the copy then shadows the original. This is the contingency on my approval of #1097.

OnClick dispatches to the adapter slice, which persists the selection to .fwlayout via ReplacePartWithNewAttribute (MultiStringSlice.cs:306). CopyWritingSystemSelectionToOverride then writes the same selection into the json override. Both files now carry it -- and ViewDefinitionOverrideApplier.cs:169 (on #1097) prefers the json unconditionally:

var writingSystems = _setWritingSystems.TryGetValue(node.StableId, out var w)
    ? w : node.VisibleWritingSystems;

XmlLayoutImporter.cs:362 has already populated node.VisibleWritingSystems from the .fwlayout partRef, its own comment noting that the partRef is "where the legacy editor persists the user's choice". So the json copy permanently shadows the value it was copied from: redundant on write, authoritative on read.

The user-visible consequence is on the WinForms side. Set writing systems there, .fwlayout is updated, Avalonia reads it -- and then renders an older json value instead, with no recency check and no diagnostic.

Either resolution works for me:

1. Explicit precedence. While both files can carry this attribute, make the rule deliberate rather than incidental -- last-write-wins, or partRef-wins, or at minimum a diagnostic when the two disagree.
2. Land Use project .fwlayout files for Avalonia persistence #1111 first. It removes the second store, which makes the question moot rather than answered.

What I do not think survives a bug report is "json always" shipping as it stands. I am not asking you to pick between one store and two inside this PR -- only to close this specific hole one way or the other.

[mark-sil]

I traced the persistence path before answering, and the load side doesn't connect the way this assumes. The Avalonia view never reads the file legacy writes to:

• ReplacePartWithNewAttribute → Inventory.PersistOverrideElement writes into LcmFileHelper.GetConfigSettingsDir(m_projectPath) — the per-project ConfigurationSettings folder (Inventory.cs:503).
• DetailComposer.LoadSources reads only FwDirectoryFinder.GetCodeSubDirectory(@"Language Explorer\Configuration\Parts") — the installed shipped directory (DetailComposer.cs:3226). Nothing loads the project folder into the composer.
• visibleWritingSystems appears in zero files under DistFiles/Language Explorer/Configuration/, so node.VisibleWritingSystems is null for every field in the product today.

The XmlLayoutImporter.cs:362 comment you quoted describes partRef-vs-content precedence within a layout file; it does not put the legacy value into the composed model.

So there is no value for the JSON to shadow, and the stated consequence — "set writing systems in WinForms, Avalonia renders an older JSON value instead" — cannot occur, because Avalonia was not showing the WinForms value before this PR either. That gap is real, predates both PRs, and is unchanged by them. This PR changes the opposite direction: before it, a writing-system change made in the new UI was invisible there. That is the bug it fixes.

Whether the same intent should live in two stores is a separate, legitimate question — and it is the same sparse-patch-vs-whole-copy question as point 1 of your #1097 review, which is with Jason as a #964 foundation decision. #1111 is one answer to it. It gets settled there, not as a merge condition on a leaf PR, because whatever we choose governs every op kind.

[johnml1135]

Defect: one identifier, three comparers. Reusing legacy's own filter here is exactly right, and it is also what exposes the inconsistency.

StringSliceUtils.GetVisibleWritingSystems builds new HashSet<string>(wsIds) with the default comparer, so this canonicalisation is case-sensitive. Line 734 below uses StringComparer.Ordinal for the Cancel check, consistent with it. But:

• ViewDefinitionOverrideDiffer.WritingSystemsEqual (LT-22691: Add the SetVisibleWritingSystems override operation #1097, line 338) compares with OrdinalIgnoreCase
• DetailComposer.ApplyVisibleWritingSystems builds both its lookup and its dedupe set with OrdinalIgnoreCase

So the same identifier is written case-sensitively, diffed case-insensitively, and read case-insensitively. In practice ids come from ws.Id so it rarely bites, but a hand-edited or migrated FR is dropped by this line and honoured by the composer -- and the differ would consider it equal to fr while this code does not.

Pick one comparer and use it on all three paths. Ordinal matches legacy, which is the side we are calling authoritative, so that is my preference -- but ignore-case applied consistently would also be defensible. The split is the defect.

[mark-sil]

The observation is correct — I verified all four sites. It is also not something this PR introduced:

• StringSliceUtils.GetVisibleWritingSystems has been case-sensitive since long before this work, and WinForms depends on it.
• DetailComposer.ApplyVisibleWritingSystems has been OrdinalIgnoreCase since LT-22625: Add the WinForms to Avalonia conversion foundation #964.
• The differ is OrdinalIgnoreCase deliberately: our LT-22691: Add the SetVisibleWritingSystems override operation #1097 review found that Ordinal disagreed with the composer, so a case-only difference (fr-FR vs fr-fr) emitted an operation and left a persistent override file for a layout that renders identically. "Ordinal everywhere" reintroduces that unless the composer changes too.
• The line you commented on reuses legacy's own filter, which was itself a review request. Making it ignore case alone would make it disagree with the helper it reuses.

Unifying the comparer means changing shared code WinForms depends on and foundation code from #964, and it needs verification against existing projects. Please open an issue for it naming the three sites, and it can be scheduled on its own evidence.

[johnml1135]

Not a defect -- keep this, whatever happens to the storage format.

This matcher overload is the important part of the file, and the reason deserves to be recorded where someone will find it: ListPropertyChoice does not override HelpId (it returns the empty string, Choice.cs:538), so the per-writing-system toggles can never be matched by command id. Matching on ParentProperty is the right fix.

The hardcoded HelpId switch this replaces carries the same blind spot everywhere it is used, so this is a general improvement rather than a writing-system detail. If this work gets reshaped onto a different store, this is the piece to carry across.

[mark-sil]

Agreed.

[github-actions]

NUnit Tests

    1 files  ±0      1 suites  ±0   11m 4s ⏱️ +30s
5 882 tests +2  5 801 ✅ +2  81 💤 ±0  0 ❌ ±0 
5 891 runs  +2  5 810 ✅ +2  81 💤 ±0  0 ❌ ±0 

Results for commit e810cd2. ± Comparison against base commit b2bcba4.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 70.87379% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.51%. Comparing base (b2bcba4) to head (e810cd2).

Files with missing lines	Patch %	Lines
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs	62.96%	20 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1108      +/-   ##
==========================================
+ Coverage   38.44%   38.51%   +0.07%     
==========================================
  Files        1507     1508       +1     
  Lines      350698   350776      +78     
  Branches    40314    40329      +15     
==========================================
+ Hits       134818   135095     +277     
+ Misses     186648   186476     -172     
+ Partials    29232    29205      -27     

Files with missing lines	Coverage Δ
...xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs	100.00% <100.00%> (ø)
Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs	78.87% <100.00%> (+8.72%)	⬆️
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs	55.29% <62.96%> (+9.55%)	⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mark-sil]

Forward compatibility / op-kind map. This is a property of the wire format as it arrived in #964, and it applies identically to all seven op kinds — this PR neither introduced it nor made it worse. I'm not treating it as a condition on this work. If you want per-op skip-with-diagnostic, please open the issue or the PR for it; it's a format-wide change and it should be evaluated as one.

ApplyVisibleWritingSystems unavailable-id direction. As you say, it is unchanged by either PR — it is #964 foundation code, and neither PR touches that method. Please log it so the fail-open vs fail-closed decision gets made deliberately, with the legacy comparison recorded.

AddNode drop reported as a Warning. Not reachable from the UI: ViewDefinitionOverrideMigrator has no production caller, so nothing today can produce an AddNode operation carrying a writing-system restriction. The diagnostic exists so the limitation is recorded if that path is ever wired.

One process request. Several comments in this review concern code neither PR changes. Reviewing a PR against problems it did not introduce expands the change beyond what was proposed and delays work that is ready. Please raise pre-existing issues as issues, and keep PR review comments to items that need action in the PR under review.

[johnml1135]

Approved provisionally - please open up JIRA issues as needed for follow up.

[johnml1135]

Wait - let me look this over one more time.

[johnml1135]

Approving. Verified the rebuttals against source: the composer loads parts only from the shipped directory (DetailComposer.cs:3218), so the json copy shadows nothing, and the toggle-vs-slice timing holds because the property-changed broadcast is queued (Mediator.cs:757). The remaining points belong in Jira, not in this PR. Recommended issues:

1. Unify the writing-system id comparer across the three sites. StringSliceUtils.GetVisibleWritingSystems filters case-sensitively, while DetailComposer.ApplyVisibleWritingSystems and ViewDefinitionOverrideDiffer.WritingSystemsEqual use OrdinalIgnoreCase, so one id is written, diffed, and read under different rules. Pick one comparer and apply it on all three paths, keeping the differ and composer in agreement so a case-only difference does not emit a persistent override file. Verify against existing projects since WinForms depends on the legacy helper.

2. Decide the unavailable-id direction in DetailComposer.ApplyVisibleWritingSystems. When a stored set names only writing systems no longer in the project, the composer falls back to showing all of them, while legacy's GetVisibleWritingSystems shows none. Record the legacy comparison and make the fail-open vs fail-closed choice deliberately. Either answer is defensible; the two UIs disagreeing is the defect.

3. Per-op forward compatibility in ViewDefinitionOverrideJsonSerializer. Adding a name to the op-kind map makes a file containing the new kind unreadable to any build predating it, and the failure is whole-file, so a rollback drops Field Visibility and Move Field customizations as well. Evaluate per-op skip-with-diagnostic as a format-wide change covering all op kinds. Every future op kind inherits whatever is settled here.