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.

Depends on #1097 (the storage layer this writes into); stacked on LT-22691d and draft until it merges. This PR 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 via #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. 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.

[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.

[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.