Skip to content

LT-22691: Add the SetVisibleWritingSystems override operation - #1097

Open
mark-sil wants to merge 1 commit into
mainfrom
LT-22691d
Open

LT-22691: Add the SetVisibleWritingSystems override operation#1097
mark-sil wants to merge 1 commit into
mainfrom
LT-22691d

Conversation

@mark-sil

@mark-sil mark-sil commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

A project view override can now store a per-field writing-system restriction. The differ captures a changed VisibleWritingSystems set as a new SetVisibleWritingSystems operation, the JSON wire format round-trips it, and the applier writes it onto the rebuilt node.

Nothing writes this operation at runtime yet. The RecordEditView mirror that records the user's Writing Systems menu choice ships separately: the mirror is the piece a native command layer would delete, while this storage layer survives either answer to the open adapter-direction question — which is why it lands alone. The review question here is narrower than the diff suggests: does the new operation round-trip losslessly, and does a bad patch file fail safely?

Where to look:

  • Round trip: DiffApply reproduces a restriction and clears one — RoundTrip_DiffThenApply_ReproducesCustomized_WritingSystemRestriction / _ClearedRestrictionComesBackNull.
  • Empty clears to null, the model's own "unrestricted"; normalized once at op bucketing in the applier.
  • Tags compare case-insensitively, matching how DetailComposer.ApplyVisibleWritingSystems resolves them — Diff_CaseOnlyWritingSystemDifference_EmitsNoOp.
  • Bad patch files fail at load, not compose: a shared ReadStringList guard now covers both string-list lanes (writingSystems and the previously unguarded childOrder) — Deserialize_NullChildOrderEntry_Throws.
  • Added nodes: a restriction on a customer-added node is not representable on an AddNode op; the differ reports override-added-ws-restriction-dropped instead of losing it silently.

Deliberately not here:

  • The RecordEditView mirror (next PR in the series; held for the adapter-direction answer on LT-22691: Fix command routing through the hidden DataTree adapter #1079).
  • Carrying the restriction on AddNode ops (deferred until the migrator gets a production caller).
  • Per-op skip for unknown op kinds — formatVersion stays 1 (see the deferred accordion).
  • Set-ops targeting patch-created node ids remain silent no-ops (pre-existing, all op kinds).

Based directly on main (originally stacked on #1096, since merged; the branch was rebased and the diff is unchanged). Verified: build.ps1 -CommentHygiene clean; 76/76 targeted FwAvaloniaTests green. Manual testing deferred to the mirror PR, when a UI write path exists.


Reading this a year from now — start here

This PR is the storage half of LT-22691's "commit 4", split so the part that survives the adapter-direction decision (this) is not held hostage by the part that decision could delete (the RecordEditView mirror). The working review record was produced in-session and lives in this description; the .review/ folder is gitignored by design, so this is the only durable copy.

Decisions, and why
  • Null and empty both mean "no restriction", and null is canonical on the node. The wire format still writes an explicit empty array for a clear-op (so the op is visible in the file), but the applier normalizes to null at op bucketing — one enforcement site, and ViewNode.VisibleWritingSystems never holds an empty list on any path. Today's composer treats both alike; the normalization exists so no future consumer ever sees the ambiguous state.
  • OrdinalIgnoreCase for tag comparison. DetailComposer.ApplyVisibleWritingSystems resolves tags case-insensitively, so an Ordinal differ would emit operations for layouts that render identically — a render-identical project would carry a persistent non-empty override file, defeating empty-patch cleanup. StableId comparisons elsewhere in the differ deliberately stay Ordinal; the two comparers are correct for different domains, which is why the two list-compare sites were not unified.
  • Validation at load, shared between lanes. The childOrder lane read the same JSON shape as the new writingSystems lane but let a null entry through to crash at compose time — where a broad catch degrades the layout to the first-slice fallback persistently. Both lanes now read through one validating helper that fails the load with InvalidDataException, which the store quarantines cleanly (logged, shipped definition used).
  • Diagnostic, not silent drop, for added-node restrictions. An AddNode op cannot carry the restriction yet; emitting override-added-ws-restriction-dropped keeps the differ's "unrepresentable edits become diagnostics" contract truthful without growing the AddNode wire shape while nothing exercises it.
Deferred, and what would unblock it
  • Carrying visibleWritingSystems on AddNode ops (so an added node's restriction survives Diff→Apply): unblocked when ViewDefinitionOverrideMigrator gets a production caller — today the drop is reported as a diagnostic and no product path produces the input.
  • Per-op graceful skip for unknown op kinds: one new-kind op in a patch file makes a pre-change build's reader throw, and the store treats the whole file as "no override" — every customization for that layout silently gone on that build. Exposure is version rollback and backup/restore only (.viewoverride.json does not travel through Send/Receive), and bumping formatVersion would be strictly worse (old readers would reject every new file). Skipping unknown ops with a diagnostic is the remedy; it changes forward-compat semantics for the whole format and deserves a deliberate design call.
  • Set-ops targeting patch-created node ids are silently ignored, and the stale-target diagnostic is suppressed because the created id lands in the seen set. Pre-existing for setVisibility/setLabel since the applier existed; only hand-authored patches can produce the input. Unblocked if the field menus ever operate on added/duplicated nodes.
Preflight review details

This branch went through an adversarial review in-session: 8 independent finder angles (line-by-line, removed-behavior, cross-file tracing, reuse, simplification, efficiency, altitude, repo conventions), 23 raw candidates deduplicated to 12, each verified by its own agent. Results: 9 CONFIRMED, 1 PLAUSIBLE, 1 REFUTED (write-path asymmetry — no producer can construct the bad input today), 1 resolved by a later fix in the same review cycle.

Fixed before this PR (each with a locking test where behavior changed):

  • Differ emit side existed but compared tags Ordinal while the composer matches OrdinalIgnoreCase — fixed, Diff_CaseOnlyWritingSystemDifference_EmitsNoOp.
  • childOrder JSON lane accepted null entries that crashed at compose time — fixed via shared ReadStringList, Deserialize_NullChildOrderEntry_Throws.
  • Added-node restrictions were dropped silently — fixed at diagnostic depth, Diff_AddedNodeWithWsRestriction_ReportsDiagnostic.
  • Empty→null normalization ran per rebuilt node — moved to op bucketing.
  • A new comment documented caller behavior (banned by the repo commenting standard) — reworded.
  • Test cleanup: duplicated ViewNode constructions unified behind WsFieldNode; a provably-dead assertion disjunct replaced with Is.Null.

Deferred with reasons (see the accordion above): AddNode restriction carriage, per-op unknown-kind skip, patch-created-id set-ops. Dropped: missing-id exception type (pre-existing for every op kind; the store catches broadly).

Validation: .\build.ps1 -CommentHygiene clean; .\test.ps1 -CommentHygiene -TestProject "Src/Common/FwAvalonia/FwAvaloniaTests" -TestFilter "FullyQualifiedName~ViewDefinitionOverride" — 76/76 passed. UTF-8 BOM state verified unchanged on all five files. Manual testing deferred to the mirror PR: nothing populates VisibleWritingSystems at runtime yet, so the read path is only reachable via hand-authored patch files; a six-scenario manual recipe (restriction, order, casing, clear, coexistence with the Field Visibility write path, corruption quarantine) is recorded for that PR.

🤖 Generated with Claude Code


This change is Reviewable

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

Comment hygiene (advisory)

No comment-style violations in the lines this branch adds since origin/main.

Base automatically changed from LT-22691c to main August 20, 2026 20:45
@mark-sil
mark-sil marked this pull request as ready for review August 20, 2026 20:47
@mark-sil
mark-sil marked this pull request as draft August 20, 2026 20:50
@mark-sil
mark-sil marked this pull request as ready for review August 20, 2026 20:58
Store a per-field writing-system restriction in the project view
override.

Note: Nothing writes this operation at runtime yet.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   9m 40s ⏱️ - 1m 20s
5 861 tests +8  5 780 ✅ +8  81 💤 ±0  0 ❌ ±0 
5 870 runs  +8  5 789 ✅ +8  81 💤 ±0  0 ❌ ±0 

Results for commit 9d6d840. ± Comparison against base commit 6f72671.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 38.35%. Comparing base (6f72671) to head (9d6d840).

Files with missing lines Patch % Lines
...nia/ViewDefinition/ViewDefinitionOverrideDiffer.cs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1097      +/-   ##
==========================================
+ Coverage   38.33%   38.35%   +0.01%     
==========================================
  Files        1507     1507              
  Lines      350580   350621      +41     
  Branches    40293    40300       +7     
==========================================
+ Hits       134410   134473      +63     
+ Misses     186941   186921      -20     
+ Partials    29229    29227       -2     
Files with missing lines Coverage Δ
...ia/ViewDefinition/ViewDefinitionOverrideApplier.cs 93.60% <100.00%> (+0.30%) ⬆️
...Definition/ViewDefinitionOverrideJsonSerializer.cs 91.66% <100.00%> (+0.75%) ⬆️
...nia/ViewDefinition/ViewDefinitionOverrideDiffer.cs 88.10% <95.23%> (+0.83%) ⬆️

... and 4 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants