Skip to content

Make text property XML reader and writer agree - #410

Open
johnml1135 wants to merge 6 commits into
sillsdev:masterfrom
johnml1135:fix/text-props-roundtrip
Open

Make text property XML reader and writer agree#410
johnml1135 wants to merge 6 commits into
sillsdev:masterfrom
johnml1135:fix/text-props-roundtrip

Conversation

@johnml1135

@johnml1135 johnml1135 commented Nov 5, 2026 •
edited by ddaspit
Loading

Copy link
Copy Markdown
Contributor

Fixes #409

A custom bullet chosen in Format > Styles > Bullets (LT-16341) was lost every time the project was reopened. It is now saved; it now also reaches the CSS and Word exports.

The rest stops TsPropsSerializer (StyleRules) and TsStringSerializer (runs) writing XML their own reader rejects, which shows as "corrupted record" and closes FieldWorks.

  • Reader: it accepts every form a released writer emitted, so projects already broken by these attributes open again.
  • Writer: it adds nothing older readers newly reject. It stops writing what no reader keeps, and it no longer throws on a value it holds. Malformed binary values are left out. Characters XML can't hold, including unpaired surrogates in string properties, are stripped.

Compatibility. An independent sweep wrote every property × variation × value with this branch and read it back with master's reader. The only new rejection was a negative relative line height, and that is now written unsigned, as master did. No FLEx workflow persists the eight Views layout attributes (set only at display time); I recommend keeping them written, which matches master byte-for-byte.

Tests. TextPropsRoundTripTests makes every FwTextPropType constant classified. Each one round-trips through both serializers, and any value the writer emits must load. 19 *_Fail tests that pinned rejection of -1 are now *_Negative. Green on net462 and net8.0.

Reader: newly accepted
  • MarginTop, plus directionDepth, hyphenate, maxLines, cellBorderWidth, cellSpacing, cellPadding, setRowDefaults and relLineHeight
  • negatives, including offset on a run (lowered text)
  • numeric enumeration values and non-negative numeric units
  • prop_N in BulNumFontInfo (N ≤ 0xFFFF)
  • tabDef, tabList, charStyle, paraStyle and editable are ignored without Debug.Fail
Writer changes
  • marginTop; bulCusTxt
  • no longer writes tabDef, tabList, charStyle, paraStyle (no producer) or editable (display-only; reading it back would make text read-only); ktptTableRule is still not written
  • saves line height int.MinValue and bullet font info with font name and features, which used to throw
  • leaves out truncated bullet, ws-style, object data or tags (with a Debug.WriteLine) instead of throwing
  • numbers use the invariant culture

🤖 Generated with Claude Code

This change is Reviewable

johnml1135 and others added 2 commits November 5, 2026 14:24
@johnml1135 @claude
Make text property XML reader and writer agree
c7a0876 
TsPropsSerializer (StyleRules) and TsStringSerializer (runs) wrote
properties and values their own reader rejects, so the record failed
to load as "corrupted record"; others were written but dropped.

Reader: accept MarginTop, directionDepth, hyphenate, maxLines,
cellBorderWidth, cellSpacing, cellPadding, setRowDefaults,
relLineHeight, negative numbers (incl. offset on a run), numeric
enumeration and unit values, and prop_N in BulNumFontInfo; ignore
tabDef, tabList, charStyle, paraStyle and editable quietly.

Writer: write marginTop and the custom bullet (bulCusTxt, previously
lost on save); stop writing tabDef, tabList, charStyle, paraStyle and
editable, which were never read back; never throw on a value it holds
(lineHeight int.MinValue, bullet font info with font features,
malformed bullet/ws-style/object data/tags, XML-invalid characters);
write numbers with the invariant culture.

Add TextPropsRoundTripTests: every FwTextPropType constant must be
classified, and round-trips through both serializers; any value the
writer emits must load.

Fixes sillsdev#409

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135 @claude
Address review of text property XML round trip
cddda37 
- Write a relative line height unsigned again, as master did, so
  older readers never meet a new negative (P3-1).
- Strip unpaired surrogates before ICU normalization and in the
  invalid-XML regex, through one helper shared with run text (P3-2).
- Skip prop_N above 0xFFFF in BulNumFontInfo instead of renumbering
  it (P3-3); reject negative numeric units (P3-4).
- Debug.WriteLine wherever the writer drops or strips a malformed
  value (P3-5); fix an indentation slip (P3-6).
- Tests: escape U+FFFF literals, add lone surrogates and ws/baseWs to
  the sweep, take NotPersisted kinds from the table (P3-7, P3-8).
- Split the CHANGELOG entry into Fixed and Changed (P3-9).
- AGENTS.md: note DisableGitVersionTask for detached-HEAD builds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Nov 5, 2026
Open
Open
Open
Open
johnml1135 and others added 2 commits November 5, 2026 17:38
@johnml1135 @claude
Warn instead of asserting on picture object data
484f405 
The writer itself writes type="picture" for picture object data, and
the reader ignores it. The Debug.Assert in both readers fired on that
writer output, which in a Debug build opens a dialog on net462 and
hangs the test host (AnyStrValue_Loads(6)). Use Trace.TraceWarning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135 @claude
Strip string property values independently of run text
6fae643 
GetXmlSafeString now removes unpaired surrogates and invalid XML
characters itself instead of delegating to TsStringSerializer's
GetXmlSafeText. Run text must keep its length (offsets index into it),
so its handling may change to replacement; property values are not
indexed and can simply drop what XML cannot hold. Keeping the two
helpers separate lets each evolve without changing the other.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
johnml1135 added a commit to johnml1135/liblcm that referenced this pull request Nov 5, 2026
@johnml1135 @claude
Replace lone surrogates before stripping invalid XML characters; cove…
25ba970 
…r custom String fields

- GetXmlSafeText replaces lone surrogates first, so it keeps the length
  whether or not StripInvalidXmlChars also removes lone surrogates (as
  sillsdev#410's regex does), and removing a control character cannot join a lone
  high and low surrogate into a false pair. Test pins the order.
- CmObject.SetCustomPropertyForSDA runs NormalizeNfd on custom String
  fields, like model String properties. Test added.
- Tests use \u escapes for every non-ASCII character.
- CHANGELOG: say that callers relying on the exception now get U+FFFD.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@johnml1135 johnml1135 mentioned this pull request Nov 6, 2026
Merged
johnml1135 and others added 2 commits November 5, 2026 21:12
@johnml1135 @claude
Do not throw on a text property string that is empty or strips to empty
9b646b8 
liblcm stores an empty string property as null. The new GetXmlSafeString
dereferenced it, so saving such a property threw, and the custom bullet
(now written) added more ways to reach it. Guard null and empty there,
and in the ktptTags writer. A Field name that is only characters XML
cannot hold is now treated as no name instead of writing an empty one
that does not load.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135 @claude
Use a \u escape in the empty-value tests; correct the GetXmlSafeStrin…
935e5a1 
…g comment

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135 johnml1135 mentioned this pull request Nov 7, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Text property XML: writer emits properties and values its own reader rejects (record fails to load)

1 participant

@johnml1135