LT-22702: Escape custom list range ids at the point they are written

[imnasnainaec]

Fixes LT-22702.

Companion to #1063. The two diffs do not overlap.

Quick Summary

• A custom possibility list's name is written into two XML attributes: the range/@id in
  the .lift header, and the range/@id of the same range in the .lift-ranges file.
• The exporter escaped that name once, on collection, with the element-content rules
  (MakeSafeAndNormalizedXml), rather than at each write site with the attribute rules --
  which is both the wrong rule and the wrong place.
• This branch holds the collected names unescaped and escapes at each write site, matching what
  the rest of the exporter already does for every other range and range-element id.

Two defects fall out of escaping on collection:

1. The header disagrees with its own ranges file. The header write site escapes again, so a
   list named Birds & Beasts is emitted as Birds & Beasts, while the ranges file
   kept a single escape.
2. The ranges file can be unparseable. The ranges write site does not escape at all, and
   element rules never escape ", so a list named So-called "words" closes the id attribute
   early and leaves the .lift-ranges unreadable by any conforming parser.

The change

Collect raw. Four sites stop pre-escaping, so the two name maps hold what the user typed:

• GetCustomListsAndReferencedLists
• GetCustomListsNotYetAdded
• AddPossListRefdByField
• MapCmPossibilityListGuidsToLiftRangeNames

The m_ListsGuidToRangeName map already held standard range names raw via MapGuidToRange, so
this also stops it holding a mix of escaped and unescaped values.

Escape at the write site. WritePossibilityListAsRange now puts its rangeId through
MakeSafeAndNormalizedAttribute, hoisted into a local so the normalizer runs once and both
branches are visibly the same.

Leave the two already-correct write sites alone. The header <range id href> and the
custom-field spec string need no change once their input is raw; they stop double-escaping on
their own.

Record the invariant. It is stated on the two map declarations, since "held unescaped" is
the kind of thing a later change would otherwise re-break.

Scope

Only custom and referenced possibility lists are affected. WritePossibilityListAsRange has
six callers; the other five pass RangeNames constants, where the helper is a no-op.

Not addressed here. WriteRangeRefsForListsReferencedByFields writes a header
<range id href> for every collected list, while WritePossibilityListAsRange returns before
writing anything for a list with no possibilities. So an empty custom list still yields a header
reference to a range that has no body in the ranges file. That is pre-existing and left alone;
the only effect here is that the dangling id now carries the corrected spelling.

Sweep: every other range id write already escapes and normalizes

I swept every <range id= and <range-element id= write in LiftExporter.cs to be sure this
is the whole of it.

• WritePossibilityRangeElement -- covers semantic domains, status, locations, people,
  anthropology, translation tags, production restrictions and custom lists. Escapes and
  normalizes both its range ids and its range-element ids.
• The four part-of-speech sub-range writers -- -feature-value, -slot, -infl-class,
  -stem-name. Same.
• Everything else is a literal or a RangeNames constant.

Effects worth knowing

This is not a normalization change. The collected names were already NFC-normalized on the
way in, and remain so -- MakeSafeAndNormalizedAttribute normalizes as well as escapes. No id
changes spelling as a result of this branch; only escaping changes, and only for names holding
&, <, > or ". (#1063 is the branch that changes spelling.)

FLEx's own importer is unaffected. A custom list's range falls through
ProcessRangeElement's switch to default:, which handles only the -slot, -infl-class,
-feature-value and -stem-name suffixes and otherwise does nothing but a Debug.WriteLine.
FLEx does not import custom-list ranges at all, so there is no re-import behavior to check.
This matters only to third-party consumers of the files, and to what lands in a shared repo.

The qaa-x-spec custom-field definition also changes. GetCustomFieldDefinition appends
the map value raw into ; range={0}, and GenerateCustomFieldSpecs escapes the whole spec
string once at its write site. With the map now holding raw names, a custom list name containing
one of the four characters reaches that form single-escaped where it used to be double-escaped.
This is the same defect and the same correction as the header range ref, and it brings custom
lists into line with the standard list names, which MapGuidToRange has always stored raw. It
is called out separately because it touches the header field definitions, not just ranges.

LIFT Send/Receive sees a whole-record delete plus add. range is keyed on @id by both the
Chorus element strategy and by XmlMergeService's record splitting, and unlike range-element
it has no guid to fall back on.

• For any project whose custom list name contains one of the four characters, the first export
  after upgrading rewrites that id once. Chorus models that as a delete plus an add of the whole
  <range> record, taking every <range-element> inside it along -- a large single diff.
• It is a one-time event per affected list. The new spelling is stable, unlike a normalization
  change, which an un-upgraded client would keep undoing.
• A project whose list name contains a " has been committing an unparseable ranges file until
  now.

Testing

Two regression tests in LiftExportTests, each covering one of the two defects. Both were
confirmed to fail on main before the fix.

1. LiftExportRanges_CustomListRangeIdWithQuoteIsEscapedForAnAttribute -- a custom list named
   So-called "words", asserting the ranges document parses and the id reads back as the name
   stored. Without the fix this fails with
   XmlException: 'words' is an unexpected token. Expecting white space.
2. LiftExportRanges_CustomListRangeIdWithAmpersandIsEscapedExactlyOnce -- a custom list named
   Birds & Beasts, asserting the ranges id reads back as the name stored and that the .lift
   header names the range the same way. Without the fix the header and the ranges file disagree.

Both drive the real exporter over a real custom list and custom field, through a shared
ExportWithCustomList helper that runs the .lift pass and the ranges pass on one exporter --
the ranges pass writes only the lists the .lift pass collected.

.\test.ps1 -SkipNative -TestProject LexTextControlsTests -TestFilter "FullyQualifiedName~LiftExportTests":
7 passed, 0 failed, including the pre-existing LiftExport (which asserts the header range
ids for both a referenced and an unreferenced custom list) and
LiftExport_MultiParagraphWithAmpersandExports. The full suite and the native tests have not
been run locally; leaving this as a draft for CI.

Test fixture resets

The fixture also now resets its five shared custom-field collections in [SetUp], alongside the
maps it already reset there. NUnit shares one fixture instance across a class, so each of them
outlived the cache it indexes and grew by several entries per test, while VerifyExport and
VerifyExportRanges index them positionally.

Why each of the five needed resetting

m_customListsGuids had to be reset for the new tests to pass: guids are minted per object, so
[0] and [1] named lists in a cache that had already been disposed.

The four flid lists (m_customFieldEntryIds, m_customFieldSenseIds,
m_customFieldAllomorphsIds, m_customFieldExampleSentencesIds) were still passing, but only
because AddCustomField allocates from clid * 1000 + 500 upward per metadata cache, so a fresh
cache plus an unchanged [SetUp] order reassigns the same flids and the stale entries happened
to hold the values the current cache would assign. Resetting them too retires that unwritten
invariant, which a reordered or added custom field would otherwise break into a confusing
failure.

CI-ready checklist

• Commit messages follow .github/commit-guidelines.md.
• No whitespace warnings locally (git log --check and git diff --check are clean).
• Builds locally, and the touched test fixture passes.
• Jira ticket filed: LT-22702.

---

This change is

[github-actions]

NUnit Tests

    1 files   -     1      1 suites   - 1   11m 45s ⏱️ - 2m 46s
5 762 tests +    1  5 681 ✅ +   39  81 💤 ± 0  0 ❌  - 38 
5 771 runs   - 5 769  5 690 ✅  - 5 614  81 💤  - 81  0 ❌  - 74 

Results for commit c4b3bf2. ± Comparison against base commit 7f93348.

This pull request removes 6 and adds 7 tests. Note that renamed tests count towards both.

SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ NotifyDataAccessSemanticsChanged_DefersUntilVisible
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ NotifyDataAccessSemanticsChanged_Reconstructs_WhenRootBoxDoesNotNeedReconstruct
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ RefreshDisplay_Reconstructs_WhenRootBoxNeedsReconstruct
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ RefreshDisplay_SkipsReconstruct_WhenRootBoxDoesNotNeedReconstruct
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ SetRootBoxDataAccessAndRefresh_Reconstructs_WhenSwapChangesDisplaySemantics
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ SetRootBoxDataAccess_DoesNotReconstruct_WhenSwapIsCheap

LexTextControlsTests.LiftExportTests ‑ LiftExportRanges_CustomListRangeIdWithAmpersandIsEscapedExactlyOnce
LexTextControlsTests.LiftExportTests ‑ LiftExportRanges_CustomListRangeIdWithQuoteIsEscapedForAnAttribute
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ NotifyDataAccessSemanticsChanged_DefersUntilVisible
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ NotifyDataAccessSemanticsChanged_Reconstructs
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ RefreshDisplay_AlwaysReconstructs
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ SetRootBoxDataAccessAndRefresh_Reconstructs_WhenSwapChangesDisplaySemantics
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ SetRootBoxDataAccess_DoesNotReconstruct_WhenSwapIsCheap

♻️ This comment has been updated with latest results.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.04%. Comparing base (7f93348) to head (c4b3bf2).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1064      +/-   ##
==========================================
+ Coverage   37.91%   38.04%   +0.13%     
==========================================
  Files        1499     1499              
  Lines      350117   350109       -8     
  Branches    40233    40231       -2     
==========================================
+ Hits       132747   133215     +468     
+ Misses     188043   187610     -433     
+ Partials    29327    29284      -43     

Files with missing lines	Coverage Δ
Src/LexText/LexTextControls/LiftExporter.cs	64.30% <100.00%> (+0.01%)	⬆️

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

[papeh]

should this comment go at the beginning of this method?

[papeh]

I'm not sure we need this comment. The field name is self-documenting, and the usage is documented by example.