Skip to content

LT-22691: Compose singular-selector custom fields as string rows - #1109

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

LT-22691: Compose singular-selector custom fields as string rows#1109
mark-sil wants to merge 1 commit into
mainfrom
LT-22691f

Conversation

@mark-sil

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

Copy link
Copy Markdown
Contributor

Quick Summary

In the Avalonia detail view, a custom text field stored as
MultiUnicode/MultiString with a singular writing-system selector
(kwsAnal, kwsVern, or unset) now composes as a plain single-ws string
row, matching the legacy StringSlice. Before this, its label and
in-string menus carried the mnuDataTree-MultiStringSlice group — a
Writing Systems submenu and Show All — that legacy never shows for these
rows. (LT-22691 follow-up.)

The first question this diff raises: how does that field shape exist at
all, when the Custom Fields dialog forces singular-ws text fields to type
String? LIFT import takes both the type and the WsSelector verbatim from
the file header, and older projects can carry the combination natively —
legacy MakeAutoCustomSlice has an explicit arm for it. So the review
question is the fix site, not reachability.

Where to look:

  • DetailComposer.MakeCustomFieldNode — the fix mirrors legacy's slice
    choice (singular selector → string editor). Layout-authored
    multistring parts are untouched by design: legacy always builds a
    MultiStringSlice for those regardless of ws spec, so plurality only
    discriminates on the custom-field path.
  • Compose_CustomMultiStringWithSingularWsSelector_ComposesAPlainStringRow
    — fabricates the MultiString+kwsAnal trigger in the shared custom-field
    fixture and asserts both directions (singular composes plain, plural
    "Tone Pattern" stays multistring).

Deliberately not here:

  • A real (non-magic) ws id stored as a selector still takes the
    multistring arm; legacy throws "unhandled ws code" on it. Pre-existing
    degenerate case, out of scope.

Verification: build.ps1 -CommentHygiene green; targeted test.ps1
composer sweep in xWorksTests 93/93 including the new test; manual
before/after in FLEx against a LIFT-imported MultiUnicode+kwsAnal custom
field.

CI-ready checklist

  • Commit messages follow .github/commit-guidelines.md.
  • Builds & tests pass locally (build.ps1 -CommentHygiene; targeted test.ps1).
  • If this is core-developer AI-assisted work, I followed Docs/workflows/ai-pr-workflow.md and ran pr-preflight or the equivalent branch-readiness review before requesting review.
  • For any Src/** folders touched, corresponding AGENTS.md files are updated or explicitly confirmed still accurate (no nested AGENTS.md under Src/xWorks; Src/AGENTS.md unaffected).
  • I have considered all comments from an AI code reviewer (such as [Devin]https://app.devin.ai/review/sillsdev/FieldWorks/pull/####)

Reading this a year from now — start here

This PR fixes one menu-parity defect found while auditing the LT-22691
field-menu work: the Avalonia composer's custom-field path classified
every multi-alternative custom field as a multistring row, while legacy
distinguishes singular from plural selectors. The reasoning below is the
full record; no working documents were created for this change.

Decisions, and why

Fix the editor choice, not the menu flag. Two candidate fix sites
existed: (1) the IsMultiStringRow assignment in the composer's text
walk could have gained a ws-plurality check, or (2) MakeCustomFieldNode
could choose the editor the way legacy SliceFactory.MakeAutoCustomSlice
chooses the slice. Option 2 was taken because legacy's real discriminator
is the slice type, which is determined by the path: layout-authored
multistring parts always become MultiStringSlices regardless of their
ws= spec (a plurality check would have broken parity for a hypothetical
singular-ws layout part), and only the auto-custom path builds a
StringSlice for singular selectors. Choosing the editor at node synthesis
makes the existing IsMultiStringRow derivation correct in both paths
with no second condition.

The fieldWs == 0 arm. Legacy treats an unset selector as singular
(case 0: // a desperate default builds a StringSlice on the default
analysis ws), so the composer does the same rather than leaving the
degenerate input to drift into the multistring arm.

Row values are unaffected by the editor switch. Text-row values
resolve from the LCM property type plus the ws spec, and both editor
strings classify to DetailEditorCategory.Text; a singular selector
already resolved to a one-entry ws list. The editor string's only other
production consumer is the IsMultiStringRow assignment — verified by
sweeping for "multistring"/EditorKindMap.MultiStringEditor across
Src/**/*.cs (two production sites, both in DetailComposer).

Evidence and repro

Reachability of the trigger. AddCustomFieldDlg creates
CellarPropertyType.String for singular-ws text fields and flips the
type when the ws choice changes, so the dialog cannot mint the trigger.
LIFT import can: FindOrCreateCustomField parses Type= and
WsSelector= verbatim from the header's qaa-x-spec form, so a header
declaring Type=kcptMultiUnicode; WsSelector=kwsAnal creates the exact
combination. Manual repro used such a file imported into a Sena 3 copy.
(Caution for future repro: opening the Custom Fields dialog and pressing
OK normalizes the type/ws mismatch and destroys the repro field.)

Manual before/after. Before the fix, the imported field's menus
showed the Writing Systems submenu / Show All in the Avalonia view;
legacy showed the plain StringSlice menus. After the fix, the Avalonia
row composes as a single analysis-ws string row with no multistring menu
group.

Automated. test.ps1 -CommentHygiene -TestProject xWorksTests -TestFilter "FullyQualifiedName~Composer": 93/93 passed, including the
new decisive test and the pre-existing plural custom-field assertions.

Preflight review details

Code Review Summary

Branch: LT-22691f

Base: main (merge base 17cdfba)

Date: 2026-08-25

Review model: Claude Fable 5 (Claude Code)

Files changed: 2

Overview

Custom fields of type MultiUnicode/MultiString whose WsSelector is
singular (kwsAnal, kwsVern, or unset) were composed with the
multistring editor, so the Avalonia detail view marked them
IsMultiStringRow and their menus gained the mnuDataTree-MultiStringSlice
group that legacy never shows for these rows. The composer now picks the
string editor for singular selectors, mirroring the legacy slice choice
in SliceFactory.MakeAutoCustomSlice.

Contract/API Changes

None. MakeCustomFieldNode is private; no public surface, serialized
format, or project file changed.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

None.

Minor - Consider

  • A custom field whose stored ws selector is a REAL writing-system
    id still takes the multistring arm, where legacy throws "unhandled ws
    code".
    (pre-existing degenerate case; the composer being more
    lenient than a legacy crash is acceptable and out of scope)

Required Validation / Evidence

  • ./build.ps1 -CommentHygiene — green.
  • Targeted ./test.ps1 composer sweep — 93/93 passed.
  • Manual before/after test by the author (2026-08-25) with a
    LIFT-imported MultiUnicode+kwsAnal custom field.

Interview Notes

  • Purpose supplied by the author: LT-22691 c5+ item (a), menu parity for
    single-WS custom text fields.
  • The author reproduced the bug manually before the fix was built, then
    confirmed the fix in the same scenario after the build.
  • The author performed the code review and approved the commit message
    before the commit. No unresolved items.

Suggested Review Focus

  • Confirm the editor-choice fix site over an IsMultiStringRow
    plurality check is the preferred shape.
  • Confirm treating an unset (0) selector as singular matches
    expectations for old-project data.

🤖 Generated with Claude Code


This change is Reviewable

A MultiUnicode/MultiString custom field whose WsSelector is singular
(kwsAnal, kwsVern, or unset) stores one fixed alternative. The Custom
Fields dialog cannot create that combination, but LIFT import and
older projects can. The composer marked every multi-alternative custom
field IsMultiStringRow, so such a field's label and in-string menus
gained the mnuDataTree-MultiStringSlice group (the Writing Systems
submenu and Show All), which the plain StringSlice legacy builds for
these fields never shows.

MakeCustomFieldNode now picks the string editor for singular
selectors, matching the slice choice in MakeAutoCustomSlice, so the
row composes as a single-ws string row and the multistring menu group
stays off.

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

Copy link
Copy Markdown

NUnit Tests

    1 files  ±0      1 suites  ±0   11m 6s ⏱️ -7s
5 857 tests +1  5 776 ✅ +1  81 💤 ±0  0 ❌ ±0 
5 866 runs  +1  5 785 ✅ +1  81 💤 ±0  0 ❌ ±0 

Results for commit 36dfac5. ± Comparison against base commit 17cdfba.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.34%. Comparing base (17cdfba) to head (36dfac5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1109   +/-   ##
=======================================
  Coverage   38.34%   38.34%           
=======================================
  Files        1507     1507           
  Lines      350586   350591    +5     
  Branches    40294    40295    +1     
=======================================
+ Hits       134432   134444   +12     
+ Misses     186927   186920    -7     
  Partials    29227    29227           
Files with missing lines Coverage Δ
Src/xWorks/Avalonia/Composer/DetailComposer.cs 67.41% <100.00%> (+0.08%) ⬆️

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

@thejambi thejambi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@thejambi made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

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.

3 participants