Repository navigation
LT-22691: Apply a layout's part overrides through a conditional - #1246
Conversation
A row whose part wraps its slice in a conditional now shows the label and binds the menu its layout asks for. Until now the Avalonia detail view dropped a part ref's overrides when the part's content sat inside an <if>, <ifnot> or <choice>, so the row kept the part's own values. In Grammar > Category Edit, an affix template's name row now reads "Template Name" and offers the template's menu. It read "Name" and offered only Help. The part ref's abbreviation, hotlinks, visibility, expansion, target layout, shown writing systems and child rows reach such a row the same way. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1246 +/- ##
==========================================
+ Coverage 40.83% 40.85% +0.02%
==========================================
Files 1573 1573
Lines 357159 357289 +130
Branches 41812 41840 +28
==========================================
+ Hits 145831 145973 +142
+ Misses 182022 182011 -11
+ Partials 29306 29305 -1
🚀 New features to boost your workflow:
|
thejambi
left a comment
thejambi
left a comment
There was a problem hiding this comment.
Verdict: approve with nits. One should-fix (fine as a follow-up ticket instead) and one nit, both inline.
The goal is right: DataTree.ProcessSubpartNode passes the part ref through to the content of if/ifnot/where/otherwise (DataTree.cs:2628), and the importer now does the same. One mechanism change, no per-row special cases, StableIds unchanged, the shipped layout pinned by a test, and the part ref's diagnostics reported once. The teardown fix is correct.
Verified
- The composer applies
Neverto every node kind atWalk(DetailComposer.cs:657), and a Conditional draws no heading of its own (WalkConditional), so moving visibility onto the wrapped rows loses neither hiding nor an ifdata section's header collapse. - The 44-part count holds, and
NestedGroupNameis the only layout ref with child elements. - No shipped
<if>/<ifnot>/<choice>carries its ownvisibility. - Notebook's Subrecords section keeps no label on its inner
<obj>, as before.
Not verified: the manual pass on Sena 3 and the targeted test runs; I did not build or run tests locally.
| children, diagnostics); | ||
| AddConditionalChildren(contentEl, hasPartRef ? callerEl : null, parts, className, | ||
| layoutType, stableId, indented, children, diagnostics); | ||
| // The part ref's visibility is held by the content nodes, one per row. |
There was a problem hiding this comment.
Should fix (or follow-up): Field Visibility only reaches one branch of a two-branch part.
The Avalonia override is stored against the row's own node (ApplyFieldVisibility in RecordEditView.Avalonia.cs writes SetVisibility for the row's stable id). About 27 of the 44 affected parts are <if Disabled=true> / <if Disabled=false> pairs, and each branch's slice is a separate node.
Scenario: in Category Edit, with the template active, right-click Template Name > Field Visibility > Normally hidden. The row hides. Untick Active: the row comes back, because the other branch's slice is still Always. WinForms keeps visibility on the part ref (DataTree.cs:2424), so there it holds in both states. The nested ad hoc group's child rows are imported once per branch and share the same gap.
Not a regression (before this PR the change didn't stick at all), but the PR body says Field Visibility on these rows now works, and the manual check was on "Variant of", which has one branch. Suggest writing the override for the matching child of each sibling conditional from the same part ref, or filing a follow-up and narrowing the claim in the PR body.
There was a problem hiding this comment.
Fixed in this PR, in 6ff99ad. Each node that comes from a part ref inside a conditional now records that part ref (ViewNode.PartRefId). LocateTarget returns every node from the same part ref as VisibilityTargets, and ApplyFieldVisibility writes one SetVisibility for each of them. Hiding the template's Name or Description now holds when Active is unticked. Covered by FieldVisibility_OnATemplatesDescription_HoldsWhenActiveIsUnticked and PartRefBranches_ShareOneVisibilityTarget.
One effect, which matches WinForms: an entry ref's "Components" and "Variant of" rows come from one part ref, so hiding one now hides the other.
One case is still open: a part ref nested inside another part's conditional still sets only one state's row. The only shipped case is the nested ad hoc group, and Ad hoc Rules isn't shown in the Avalonia detail view. The PR description now says so, and its Field Visibility claim is updated.
| visibility, expansion, indented, null, children, localizationKey, automationId, | ||
| routing, menuId: menuId, contextMenuId: contextMenuId, hotlinksId: hotlinksId, | ||
| condition: condition); | ||
| ViewVisibility.Always, expansion, indented, null, children, localizationKey, |
There was a problem hiding this comment.
Nit: this (and the ChoiceGroup below) forces Always even when there is no part ref, e.g. a conditional written directly in a layout or inline in a slice, while the comment above describes only the part-ref case. No shipped conditional has its own visibility, so nothing changes today; hasPartRef ? ViewVisibility.Always : visibility would keep the old behavior for the other cases.
There was a problem hiding this comment.
Done in eb24bed: hasPartRef ? ViewVisibility.Always : visibility on both the Conditional and the ChoiceGroup. Pinned by ChoiceWrittenInALayout_KeepsItsOwnVisibility.
thejambi
left a comment
thejambi
left a comment
There was a problem hiding this comment.
@thejambi reviewed 5 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on mark-sil).
A <choice> written directly in a layout keeps its own visibility again. Until now every imported conditional was always visible; that rule is meant only for a conditional inside a part, whose part ref's visibility moves to the rows it holds. No shipped layout has such a choice, so nothing on screen changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Field Visibility on a row whose part holds one conditional per state now sets the row in every state. Until now it set only the row on screen, so an affix template's Description hidden while Active was ticked came back once Active was unticked. An entry ref's "Components" and "Variant of" rows come from one part ref, so hiding one now hides the other, as the WinForms detail view does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thejambi
left a comment
thejambi
left a comment
There was a problem hiding this comment.
@thejambi reviewed 9 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on mark-sil).
A row whose part wraps its slice in a conditional now shows the label
and binds the menu its layout asks for. Until now the Avalonia detail
view dropped a part ref's overrides when the part's content sat inside
an
<if>,<ifnot>or<choice>, so the row kept the part's own values.In Grammar > Category Edit, an affix template's name row now reads
"Template Name" and offers the template's menu. It read "Name" and
offered only Help.
The part ref's abbreviation, hotlinks, visibility, expansion, target
layout, shown writing systems and child rows reach such a row the same
way.
Reviewer notes
Start here:
Src/Common/FwAvalonia/ViewDefinition/XmlLayoutImporter.cs,CreateNodeandAddConditionalChildren. Content inside a part's conditional now takes the part ref as its caller, as content written directly in the part already did. ThenViewDefinitionOverrideEditor.LocateTarget: a Field Visibility change now writes every state's row that came from the same part ref.The question to bring: which rows change? Every row whose part wraps its content in a conditional and whose layout overrides something on the part ref: 44 shipped parts have that shape. Nothing changes in the WinForms detail view. Node StableIds are unchanged, so saved view overrides still match.
Where to look
ShippedMoInflAffixTemplateDefault_NameRow_TakesTheLayoutLabelAndMenu, and composed for both states of Active inDetailConditionalPartRefTests.<if>,<ifnot>,<where>,<otherwise>, nested): thePartRefOverrides_Reach...tests inConditionalImportTests.PartRefVisibility_IsHeldByTheContent_NotTheConditional.FieldVisibility_OnATemplatesDescription_HoldsWhenActiveIsUnticked,PartRefBranches_ShareOneVisibilityTarget). Hiding "Variant of" also hides "Components", as in the WinForms detail view.Compose_Subcategories_KeepOneHeading_AndOneNameRowPerSubcategory.CategoryEditMenuTests).Deliberately not here
<part>or<indent>children does not pass the part ref on. No shipped part has that shape, and the WinForms detail view draws nothing for it.Also here: a one-line teardown fix in
LexiconFirstSliceEditContextEdgeCaseTests. One test left an edit session open, so the fixture's teardown threw.Verification: build, comment and token hygiene clean. Targeted tests passed: 272 in xWorksTests over the Composer and Category Edit menu fixtures, 194 in FwAvaloniaTests over the import and view-definition fixtures; after the last commit, FwAvaloniaTests 149/149 and xWorksTests 285/285 and 89/89. Manual steps on Sena 3 in the New UI passed: the template rows and their menu, Subcategories, Field Visibility on "Variant of", the template's Description staying hidden with Active unticked, and "Components" following "Variant of". Full suite not run.
Next: approve, or name a row that now shows the wrong label or menu.
Decisions, and why
<seq>or<obj>inside a conditional keeps its own label. The part ref's label already names the section heading above it; passed down, it would show as a second heading (Subcategories in Category Edit). The sequence still takes the part ref's target layout and child rows.<choice>written directly in a layout keeps its own visibility.Preflight review details
Code Review Summary
Branch: claude/lt-22691-template-name-row-7398db
Base: main
Date: 2026-11-12
Review model: Claude Fable 5.2
Files changed: 12
Overview
In the Avalonia detail view, a layout's overrides on a part ref (label, menu, abbreviation,
hotlinks, visibility, expansion, target layout, shown writing systems, child rows) were lost
when the part wraps its content in an
<if>,<ifnot>or<choice>. The importer now handsthe part ref to the content inside the conditional, as it already did for content written
directly in the part. The confirmed case is the affix template's name row in Grammar >
Category Edit, which now reads "Template Name" and binds
mnuDataTree-POS-AffixTemplate.One production file changes (
XmlLayoutImporter.cs). The rest is tests, plus a one-lineteardown fix in an existing test fixture.
Two follow-up commits after review: a
<choice>written in a layout keeps its ownvisibility, and Field Visibility writes every state's row of a part (production changes in
the view-definition model, importer, override applier, JSON serializer and override editor,
and in
RecordEditViewandObjectMenuAuthority).Contract/API Changes
XmlLayoutImporter: content inside a part-level conditional takes the part ref as itscaller. A Conditional or ChoiceGroup node is always imported with visibility Always; the
part ref's visibility is on the content nodes.
ViewNode.PartRefId(also in the canonical JSON aspartRef) andViewNodeLocation.VisibilityTargetsare new.Findings
Critical - Must address before merge
None.
Important - Should address before merge
None.
Minor - Consider
what the node holds)
hasPartRefalready names(fixed: uses
hasPartRef)<obj>inside a conditional, for the ref's<part>children under awrapped slice, or for the template row with Active unticked (fixed: three tests added)
A part-level conditional holding only(author: leave as is. None of the 44 shipped parts that<part>or<indent>children has no holderfor the part ref's visibility
wrap content in a conditional has that shape, and the WinForms detail view draws nothing
for it)
Diagnostics about the part ref's own dropped children repeat once per wrapped content(author: leave as is. They feed only the coverage report; of the 156 shipped layoutnode
refs naming such a part, the only one with children holds an
<indent>, which isimported)
The importer withholds the part ref's label from a(author: accepted in the design<seq>or<obj>inside aconditional, which serves a display rule of the composer
discussion; the label already names the section heading and would repeat as a second
heading)
Required Validation / Evidence
.\build.ps1 -CommentHygiene -TokenHygiene: clean..\test.ps1 -CommentHygiene -TokenHygiene -TestFilter ...: xWorksTests 272/272 over theComposer fixtures, CategoryEditMenuTests, LexiconMenuAuthorityTests and
DataTreeMoveReachabilityTests; FwAvaloniaTests 194/194 over the import, view-definition,
canonical-JSON, override and projector fixtures; then AdhocCoProhibComposeTests 6/6 and
LexiconFirstSliceEditContextEdgeCaseTests 6/6 after the last two test edits, and 43/43 over
the three fixtures in LayoutImportCoverageTests.cs after a whitespace-only re-indent of
the test XML.
their menu in Category Edit, the Subcategories section, and Field Visibility on an
entry's "Variant of" row); all passed.
detail view.
Positive Observations
ShippedMoInflAffixTemplateDefault_NameRow_TakesTheLayoutLabelAndMenu.Interview Notes
Decisions settled with the author during the session (not re-asked here):
<seq>or<obj>inside a conditional keeps its own label; it takes the part ref'starget layout and child rows.
LexiconFirstSliceEditContextEdgeCaseTests(a test left an editsession open, so the fixture's teardown threw) is folded into this PR.
Suggested Review Focus
XmlLayoutImporter.CreateNode: which of the part ref's values reach content inside aconditional, and that the part ref is reported once.
🤖 Generated with Claude Code
This change is