LT-22685: Add sense pictures to the legacy Lexicon Edit pane (split from #964)

[johnml1135]

Sense pictures in the Lexicon Edit detail pane

Jira: LT-22685 · split out of the Phase-1 Avalonia spine (#964)

One <part> element — three lines of XML plus a six-line comment, in one
configuration file. No C#, no tests, no build changes. LexSense.fwlayout:46 has
carried <part ref="Pictures" param="Normal"/> for years; the detail pane is
data-driven, so naming a LexSense-Detail-Pictures part is the entire switch.
Every piece behind it — PictureSlice, the five CmPicture-Detail-* parts,
CmFile-Detail-FileName, mnuDataTree-Picture — already ships.

The premise this PR was opened on is wrong, and that changes what it is. The
review asked for a DataTree test as a merge condition. That test was written,
and it reports the legacy pane already renders Picture / Caption / File /
Publish Picture In on pristine main. DistFiles/Parts/GeneratedParts.xml:1494
has carried an equivalent part all along, emitted at build time and gitignored
(.gitignore:131) — which is why git log -S "LexSense-Detail-Pictures" found
nothing and read as proof of absence.

So this is not a legacy fix. It is an Avalonia-visible change, because the two
UIs read different part inventories.

Where to look:

• Legacy is a no-op. Slice labels are identical with and without this part;
  the hand-written one merely overrides the generated default with two attributes.
• Avalonia is not. DetailComposer.cs:3226 loads only
  Configuration\Parts — never the generated inventory — so this is the first
  LexSense-Detail-Pictures it can see, and DetailComposer.cs:958 routes
  Picture to WalkUnsupported. Senses with pictures gain an "Unsupported" row.
• CI is red, and correctly so. ShippedLayouts_UnresolvedParts_AreExactlyTheLegacyUnresolvableSet
  fails: LexSense-Pictures leaves the baseline (63 → 62). That test's own comment
  encodes the same false premise.
• Six of the seven review points are still open. Table below.
• The menu and layout attributes are both inert. Review points 2 and 3.

Not here: no DataTree test committed, no LabelAbbreviations entries, no
DetailComposer picture support, no Jira record of the Avalonia gap.

Status: draft, CHANGES_REQUESTED, CI red. The open question is not "is the
diff right" but "does this PR still have a purpose" — see the last accordion.

---

Reading this a year from now — start here

This PR was split out of #964 on the belief that sense pictures had never
appeared in the Lexicon Edit pane. That belief was false, and the review's own
merge condition — write the DataTree test — is what disproved it. The body
above is the corrected account; the accordions hold the mechanism, the evidence,
and the review history that is otherwise only recoverable by reading a long
thread.

The Aug 27 comment on this PR is the original write-up of the falsification. Its
substance is folded in here so the description is the single record.

Why three lines of XML is the whole change

The FLEx detail pane is configuration, not code. A .fwlayout names fields as
part references; DataTree resolves <part ref="X"/> against the parts
inventory by composing {ClassName}-Detail-{X}, and silently omits the slice
when no such part id exists (DataTree.cs:2445, "Just omit the missing
part"). There is no error, no log line, no test failure — a missing part is
indistinguishable from a field nobody configured.

So making a field appear is not a code change. It is one part id. The rendering
machinery is generic and already present:

Piece	Where	Already shipped
The layout ref	LexSense.fwlayout:46	yes, for years
The picture slice	PictureSlice.cs	yes
The per-picture parts	LexSenseParts.xml:895-920 (CmPicture-Detail-*)	yes
The file-name row	CmFile-Detail-FileName, LexSenseParts.xml:887	yes
The context menu	mnuDataTree-Picture, Lexicon/DataTreeInclude.xml:403	yes
The insert command	CmdInsertPicture on mnuDataTree-Sense, Lexicon/DataTreeInclude.xml:448	yes

Nine lines added: three of XML, six of comment. git diff --stat main...HEAD
is one file, +9 -0.

The surprising finding: a gitignored half of the inventory

Build/SetupInclude.targets (GeneratePartsAndLayoutFiles) runs
PartGenerate.xslt over MasterLCModel.xml at build time and emits a default
Detail part for every field in the model. The output lands in
DistFiles/Parts/GeneratedParts.xml, which .gitignore:131 excludes.

Line 1494 of that file:

<part id="LexSense-Detail-Pictures" type="Detail"><seq field="Pictures" /></part>

The Inventory constructor unconditionally prepends
FwDirectoryFinder.GetCodeSubDirectory("Parts") to its search paths
(Inventory.cs:174), and the parts inventory's file pattern is *Parts.xml
(LayoutCache.cs:128-132) — which GeneratedParts.xml matches. LoadElements
walks the paths in order and InsertNodeInDoc replaces on a key collision
(Inventory.cs:1400-1425), so a hand-written part in Configuration/Parts
overrides the generated default rather than colliding with it.

The lesson worth keeping: git log -S cannot see the generated half of the
parts inventory. For any future configuration-resolution question, check the
built DistFiles/Parts/ output, not just the tracked XML. The absence of history
was evidence of nothing.

Why legacy and Avalonia disagree — the two inventories

	Directories read	Sees GeneratedParts.xml?
Legacy DataTree	DistFiles/Parts (via Inventory.cs:174) plus …/Configuration/Parts (LayoutCache.cs:103)	yes
Avalonia DetailComposer	…\Language Explorer\Configuration\Parts only (DetailComposer.cs:3226)	no
LayoutImportCoverage tests	same single directory (LayoutImportCoverageTests.cs:516)	no

That asymmetry is the whole story of this PR. Because the Avalonia composer never
loads the generated defaults, it has been resolving a strictly smaller inventory
than production legacy — and its unresolved-part baseline has been measuring that
smaller set while describing it as "what legacy also omits".

Adding the part to Configuration/Parts is therefore the first time
LexSense-Detail-Pictures becomes visible to Avalonia. DetailComposer.cs:958
sends DetailEditorCategory.Picture to WalkUnsupported, so the user-visible
effect lands entirely on the new UI: legacy users see no change, new-UI users
gain a labeled "Unsupported" worklist row on every sense with a picture.

Whether the Avalonia composer should load the generated inventory is a larger
question than this branch, and is the thing most worth deciding before it merges.

Review points, and where each one stands

From the CHANGES_REQUESTED review of 2026-08-24. Each was checked against the
current tree; the line numbers in the review were accurate.

#	Point	State
1	DataTree test is a merge condition	Open — written, and it falsified the premise, but never committed to this branch
2	Drop layout="Normal" (a no-op today, a silent override tomorrow, DataTree.cs:2918)	Open — still present
3	Fix or drop menu="mnuDataTree-Picture" (wrong menu — Lexicon/DataTreeInclude.xml:403 holds Properties/MoveUp/MoveDown/Delete and no insert command; and a seq node is neither CallerNode nor ConfigurationNode, so it likely never fires)	Open — still present
4	Rewrite the comment (~570 chars, six lines over the 98-column limit, an em dash, a dead ref="ExampleList" exemplar, and a misquote of the layout node)	Open — unchanged, and now also factually wrong about the part not existing
5	Body understates the blast radius; record the Avalonia picture gap in Jira	Half — the body above now leads with the Avalonia effect; no Jira record filed
6	Add LabelAbbreviations for Picture / Caption / File / Publish Picture In	Open — none added. Note this is now a main bug, not a bug of this PR: those rows already render
7	Commit title 79 chars against gitlint's 72	Fixed — now 60 chars, Check commit messages passes
—	Question: why is the thumbnail the part turned off? PictureSlice.InstallPicture does an uncapped File.ReadAllBytes → Image.FromStream per picture on the UI thread (PictureSlice.cs:59-73)	Unanswered — and it is a main question, since that code already runs

Two things the review credited that still hold, recorded so they are not
re-litigated: rejecting editor="picture" was right (SliceFactory.cs:210 hard-casts
to ICmPicture and would receive the owning LexSense), and omitting ghost was
right (MakeGhostSlice always builds a GhostStringSlice, which cannot express a
CmPicture that needs PicturePropertiesDialog to pick a file first).

Evidence

The legacy no-op. A DataTree test built against the production
FwDirectoryFinder.FlexFolder + Configuration/Parts (not the
DetailControlsTests fixtures, since what is under test is whether the shipped
XML resolves) creates a LexSense with one CmPicture and calls
ShowObject(sense, "Normal", …). On pristine origin/main, with none of this
branch applied:

LABELS=[(null) | Gloss | Definition | Grammatical Info. | Example |
        Semantic Domains | Lexical Relations |
        Picture | Caption | File | Publish Picture In]

Applying this branch leaves that list unchanged.

The Avalonia effect. FwAvaloniaTests.LayoutImportCoverageTests.ShippedLayouts_UnresolvedParts_AreExactlyTheLegacyUnresolvableSet
fails on this branch (LayoutImportCoverageTests.cs:678):

Missing (1): < ["LexSense-Pictures", 1] >

The baseline holds LexSense-Pictures as unresolvable; this part resolves it.
Merging as-is requires deleting that entry, changing the occurrence ceiling from
63 to 62, and rewriting the comment above it, which currently reads "LexSense
Pictures has no LexSense-Detail-Pictures part, so legacy DataTree (and this
importer) omit sense pictures" — false for legacy on main.

CI state: Build Debug and run tests and NUnit Tests fail on that one test.
Everything else passes: commit messages, comment hygiene, whitespace, lychee,
stray-docs, codecov.

What is left to decide

Given that legacy already works, three routes:

1. Close it. The bug it was opened to fix does not exist. The findings live
   here and in LT-22685.
2. Re-scope it to the Avalonia inventory gap — make DetailComposer load the
   generated parts, which is the actual defect this investigation found. That is
   larger than this branch and belongs with the migration work.
3. Keep the explicit part on the argument that a hand-written part in
   Configuration/Parts is more legible than a gitignored generated default, and
   that menu=/layout= can be corrected per review points 2 and 3. This still
   requires the coverage-test baseline update and accepts the "Unsupported" row.

Route 1 or 2 seems right; route 3 needs a reason to prefer explicitness that
outweighs handing new-UI users a worklist row. Reviewer's call.

Two findings deserve their own tickets regardless of what happens to this branch,
because both are true on main today: the missing LabelAbbreviations entries
for four already-visible sense-picture labels, and the uncapped full-size image
decode on the UI thread in PictureSlice.InstallPicture.

---

This change is

[github-actions]

NUnit Tests

    1 files  ±0      1 suites  ±0   10m 54s ⏱️ + 3m 35s
5 872 tests ±0  5 790 ✅  - 1  81 💤 ±0  1 ❌ +1 
5 881 runs  ±0  5 799 ✅  - 1  81 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit bde4d5d. ± Comparison against base commit db459c6.

♻️ 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.43%. Comparing base (db459c6) to head (bde4d5d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1012   +/-   ##
=======================================
  Coverage   38.43%   38.43%           
=======================================
  Files        1507     1507           
  Lines      350657   350657           
  Branches    40307    40307           
=======================================
+ Hits       134773   134789   +16     
+ Misses     186651   186638   -13     
+ Partials    29233    29230    -3     

see 3 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.

[jasonleenaylor]

The diagnosis is right and I verified it: LexSense.fwlayout:46 really does carry
<part ref="Pictures" param="Normal"/>, git log -S "LexSense-Detail-Pictures" returns
nothing, and CmdInsertPicture is live on mnuDataTree-Sense
(DataTreeInclude.xml:448) — so users can create sense pictures today and watch them
vanish. Rejecting editor="picture" was also correct: SliceFactory.cs:210 does a hard
(ICmPicture)obj cast and would receive the owning LexSense.

Two things you got right that are worth recording, because the next reader will
otherwise re-litigate them:

• No ghost is the correct call. DataTree.MakeGhostSlice (DataTree.cs:2800)
  always constructs a GhostStringSlice, whose contract is strict: ghostWs is
  mandatory (GhostStringSlice.cs:116) and MakeRealObject throws "ghost property must
  store strings!" unless the target field is String/MultiString/MultiUnicode
  (:305-318). A ghost on Pictures would therefore have to create a CmPicture from
  typed text — yielding a picture record with a caption and no PictureFileRA. Pictures
  need PicturePropertiesDialog to choose a file first (DTMenuHandler.cs:224-236),
  which a ghost cannot express. Worth one clause in the comment so nobody "fixes" this
  later.
• Senses with no pictures stay clean. DataTree.cs:2912 only adds a slice when
  cobj > 0 or a ghost is present, so there is no empty-row clutter.

What needs to change before this merges.

1. A DataTree test is a condition of this PR, not a follow-up.

This is the part I do not want to trade away. ref="Pictures" has exactly one referent
and no part has ever resolved it, so this three-line change is the switch that first
energises five CmPicture-Detail-* parts, PictureSlice, CmFile-Detail-FileName, and
a defaultVectorReference on PublishIn — roughly 35 lines of configuration that have
never executed in production, for every user with sense pictures, ungated. CI reporting
4302 tests identical to base confirms the suite is blind to it, and "verified only for
XML well-formedness" is not evidence for a change of that reach. You already crashed the
legacy DataTree in this exact area, which is the argument for the test rather than
against it.

A test asserting the five slices materialise for a sense with one picture is the
minimum. This is reachable through the DataTree slice-creation path without
constructing a Form, so the no-WinForms-dialog-tests rule is not in the way.

2. Drop layout="Normal".

DataTree.cs:2918 reads
layoutOverride = XmlUtils.GetOptionalAttributeValue(node, "layout", layoutName), where
layoutName is the caller's param. The layout node already says param="Normal", so
the attribute is a no-op today and a silent override tomorrow — any future
<part ref="Pictures" param="Something"/> would be ignored. The live exemplar,
LexSense-Detail-Examples (LexSenseParts.xml:620-623), omits it and lets param flow
through.

3. Fix or drop menu="mnuDataTree-Picture".

Two problems. It is the wrong menu: DataTreeInclude.xml:403-410 shows
mnuDataTree-Picture holds Properties / MoveUp / MoveDown / Delete and no insert
command, so the body's "insert/delete menu" is inaccurate. The established idiom for a
seq node is the plural sequence-level menu — mnuDataTree-Examples
(DataTreeInclude.xml:468) and mnuDataTree-ExtendedNotes (:500) both hold the insert
command — and no mnuDataTree-Pictures exists.

It also probably never fires: DTMenuHandler.MakeSliceContextMenu
(DTMenuHandler.cs:1708-1716) reads the menu id off slice.CallerNode /
slice.ConfigurationNode, and with no ghost this seq node becomes neither. Each
CmPicture-Detail-* slice already carries its own menu="mnuDataTree-Picture"
(LexSenseParts.xml:887, 903, 906, 909). Either add a mnuDataTree-Pictures containing
CmdInsertPicture, or drop the attribute — but do not leave a dead attribute that
implies an insert affordance that is not there.

4. Rewrite the comment.

Six lines and roughly 570 characters, against a target of 3-4 sentences for a doc
comment and 200 characters for an implementation comment. Every line exceeds the
98-column limit from .editorconfig (102, 105, 107, 105, 97, 107), and line 3 contains a
literal em dash where the standard is ASCII. .xml sits outside
comment-hygiene.ps1's globs so nothing caught it mechanically, but the convention still
applies.

Content problems, in order of consequence:

• It cites a dead exemplar. LexSense-Detail-ExampleList (:60-62) exists but nothing
  in the repo references ref="ExampleList". The part that actually resolves is
  LexSense-Detail-Examples (:620), and it uses a different form.
• It misquotes the layout node it explains: the node is
  <part ref="Pictures" param="Normal"/>, not <part ref="Pictures"/>. The dropped
  attribute is exactly the one that makes item 2 above a finding. The same misquote is in
  the PR body and the commit message.
• "the malformed form this replaces" describes a version that never existed in the
  repository, and "silently omitted there for years because no LexSense-Detail-Pictures
  part existed" narrates what was missing rather than what the part does.
• Two of the four sentences narrate the cast mechanics of the rejected alternative. That
  warning is genuinely valuable — keep it as one sentence ("Do not use
  editor="picture" here; the slice factory casts the owning object to ICmPicture.")
  rather than a paragraph.

Roughly: what the part renders, the editor="picture" trap, and the ghost note from
above.

5. The body understates the blast radius, and the Avalonia gap needs a Jira record.

The body calls this "an ungated, legacy-visible behavior change." It is not legacy-only.
DetailComposer.cs:950-954 routes DetailEditorCategory.Picture to WalkUnsupported,
so the moment this part resolves, every sense with a picture grows a labeled
"Unsupported" row in the Avalonia UI. Legacy users get an image; new-UI users get a
worklist row they did not have before.

Fixing DetailComposer is out of scope here — that is the migration work this split was
designed to escape. But please correct the body to say both modes, and record the picture
gap as a known divergence in the Avalonia conversion's Jira issue so it is tracked before
it ships rather than discovered by a user.

6. Add LabelAbbreviations entries for the newly-visible labels.

strings-en.xml group LabelAbbreviations (line 367) carries an abbreviation for every
routinely-visible sense field ("Scientific Name" txt="sc", line 427). "Picture",
"Caption", "File", and "Publish Picture In" appear nowhere in strings-en.xml, so in
abbreviated-label mode these rows fall back to raw English. No hardcoded strings were
added by this PR, so nothing is violated — the catalog is just incomplete for the surface
this exposes.

7. Commit title is 79 characters against gitlint's 72.

Already flagged by the Actions bot, still unfixed.

A question, not a change request: why is the thumbnail the part that is turned off?

The CmPicture Normal layout enables CmPicture-Detail-Picture (full size) and sets
CmPicture-Detail-Thumbnail to visibility="never" (LexSense.fwlayout:82-88).
PictureSlice.InstallPicture (PictureSlice.cs:59-73) does File.ReadAllBytes into a
MemoryStream into Image.FromStream with no size cap, per picture, per sense render on
the UI thread. Nobody has measured that on a picture-heavy lexicon because this code has
never run in this pane. Given a Thumbnail part was deliberately provided, flipping which
one is visible may be the cheaper default — but I would rather hear your reasoning than
assume mine.

[johnml1135]

Before I answer the seven points: I wrote the DataTree test you asked for as a merge
condition, and it says this PR is a no-op. Sense pictures already render on main.
Holding off on the rest until you have seen this, because it changes what the PR is.

What the test shows

I built the test against the production Parts/Layouts (FwDirectoryFinder.FlexFolder +
Configuration/Parts) rather than the fixtures in DetailControlsTests, since the thing
under test is whether the shipped XML resolves. Creating a LexSense with one
CmPicture, calling ShowObject(sense, "Normal", ...), and dumping the slice labels on
pristine origin/main, with none of this branch applied:

LABELS=[(null) | Gloss | Definition | Grammatical Info. | Example |
        Semantic Domains | Lexical Relations |
        Picture | Caption | File | Publish Picture In]

Picture, Caption, File and Publish Picture In are already there. I then applied
this branch and removed only the new part: the label list is unchanged except that
Picture stays Picture. The three lines this PR adds change nothing.

Why

DistFiles/Parts/GeneratedParts.xml:1494 already carries the part:

<part id="LexSense-Detail-Pictures" type="Detail"><seq field="Pictures" /></part>

which is functionally what this PR hand-writes. It is produced at build time by
Build/SetupInclude.targets (GeneratePartsAndLayoutFiles runs PartGenerate.xslt over
MasterLCModel.xml), which emits a default Detail part for every field in the model.
Inventory's constructor unconditionally adds FwDirectoryFinder.GetCodeSubDirectory("Parts")
to the search paths, so DataTree's classname + "-Detail-" + partName lookup finds it
before it ever needs a hand-written part.

Why we both missed it

GeneratedParts.xml is in .gitignore (line 131). So
git log -S "LexSense-Detail-Pictures" returning nothing is exactly what a gitignored
build artifact looks like -- it was evidence of nothing, and I built the PR premise on
it. Worth remembering for the next configuration-resolution question: git history cannot
see the generated half of the Parts inventory.

What this does to your seven points

You were right on all seven as statements about the diff -- I checked each against the
code and your line numbers were exact, including mnuDataTree-Picture holding no insert
command, layoutOverride making layout="Normal" a silent override, the dead
ref="ExampleList" exemplar, and the <part ref="Pictures" param="Normal"/> misquote.
But they were points about a part that never had to exist.

Point 1 is the one that matters: you made the test a condition of merge and the test is
what caught this. Point 5 gets worse rather than better -- if the part already resolves
on main, then DetailComposer's Picture -> WalkUnsupported route means Avalonia
users may be getting the "Unsupported" row today, with or without this PR. That is
worth checking independently of this branch.

What is actually left

One real behaviour change, which is your closing question. You asked why the thumbnail is
the part that is switched off, and having looked at PictureSlice.InstallPicture doing
File.ReadAllBytes -> MemoryStream -> Image.FromStream per picture per render on the
UI thread with no size cap, I think you have the better default. Flipping
CmPicture/Normal so Thumbnail is visible and the full-size Picture is
visibility="never" is a genuine improvement to a pane that already renders these, and
it is the only part of this branch that changes what a user sees.

So I would rather not land this as written. Before I restructure it, how do you want to
play it: repitch this PR as the thumbnail render-cost fix (dropping the redundant part,
keeping the LabelAbbreviations entries for the labels that are already visible, and
keeping these tests as the regression lock), or close it and open a fresh one against a
correctly-stated ticket? LT-22685 as written describes a bug that does not reproduce.

[johnml1135]

Closing: verified live in the running app that sense pictures already render in the legacy Lexicon Edit pane on main. LT-22685 is closed as not reproducible.

The premise this PR was opened on was false — DistFiles/Parts/GeneratedParts.xml has carried an equivalent LexSense-Detail-Pictures part all along, and it is gitignored, so the git log -S search that appeared to prove absence proved nothing. The full account is in the description above; the branch is deleted, and its single commit was bde4d5d95.

The real defect this investigation turned up is unrelated to sense pictures: the Avalonia DetailComposer loads only Configuration/Parts and never the generated inventory that legacy Inventory loads unconditionally. That needs its own ticket.