Repository navigation
LT-22858: Show notebook subrecord fields; drop dead prompt rows (LT-22844) - #1203
Merged
Merged
Conversation
NUnit Tests 1 files ±0 1 suites ±0 14m 12s ⏱️ + 3m 25s Results for commit e437db3. ± Comparison against base commit c734bff. This pull request removes 3 and adds 11 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1203 +/- ##
==========================================
+ Coverage 39.93% 39.94% +0.01%
==========================================
Files 1542 1542
Lines 355165 355217 +52
Branches 41317 41329 +12
==========================================
+ Hits 141833 141896 +63
+ Misses 183999 183985 -14
- Partials 29333 29336 +3
🚀 New features to boost your workflow:
|
thejambi
force-pushed
the
LT-22858-subrecord-fields
branch
from
October 26, 2026 12:56
da433be to
272009a
Compare
A subrecord header reads "1. Observation" while expanded and "1. Observation - title" while collapsed, as the record's Summary and SummaryCollapsed parts define, and stands for the summary row the subrecord's layout opens with, so no second SubRecords header follows. This reverts commit 01de10d. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The SubRecord layout brings a subrecord's fields in with <sublayout name="Normal" layoutChoiceField="Type"/>, which the layout importer dropped as a publishing construct, so each subrecord showed only its header. The importer now keeps it as a Sublayout node, and the composer lays out that layout for the subrecord's Type in place, the way the shown record's own layout is chosen. The fields sit one level under the subrecord's header, and nested subrecords repeat the pattern. A subrecord header with fields under it can now collapse, which is what shows its collapsed line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An empty list whose layout names no ghost field showed a "Type to add ..." row that could not be typed in, labelled with the raw field name: the SubRecords list on a new notebook record, and now on every subrecord with no subrecords. The WinForms DataTree shows a prompt only where the layout names a ghost field, so those rows are left out. Each affected list keeps its section header and that header's Insert command, and the sense and subsense gloss prompts, which can create a sense, stay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thejambi
force-pushed
the
LT-22858-subrecord-fields
branch
from
October 26, 2026 13:17
272009a to
62e63cf
Compare
thejambi
marked this pull request as ready for review
October 26, 2026 13:20
mark-sil
reviewed
Oct 26, 2026
Each level of subrecord nesting takes three composer levels, so the cap of 12 stopped at a fourth-level subrecord: its own Subrecords section was dropped, and Insert Subrecord, Insert Subrecord of Subrecord and Demote there made records seem to vanish. The WinForms view has no limit. The (object, layout) visited set already stops recursion, and the cap is only a backstop for malformed layouts, so it now allows 48 levels, about 16 levels of subrecords. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thejambi
commented
Oct 26, 2026
thejambi
left a comment
Contributor
Author
There was a problem hiding this comment.
@thejambi reviewed 4 files and all commit messages, resolved 1 discussion, and dismissed @mark-sil from a discussion.
Reviewable status: 0 of 8 files reviewed, all discussions resolved (waiting on mark-sil).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Start here:
DetailComposer.WalkSublayoutinSrc/xWorks/Avalonia/Composer/DetailComposer.cs-- it lays a subrecord's own layout out under its header.In Notebook > Record Edit in New mode, each subrecord now shows its fields under its header, chosen by its Type as in legacy, and nested subrecords repeat that one level further in (LT-22858). The header reads
1. Observationexpanded and1. Observation - <title>collapsed. The branch also fixes LT-22844: an empty list whose layout names no ghost field no longer shows a "Type to add ..." box that cannot be typed in.Why did subrecords show nothing? The SubRecord layout brings the fields in with
<sublayout name="Normal" layoutChoiceField="Type"/>, which the importer dropped as publishing-only; it is the only detail-view<sublayout>. What disappears with LT-22844? Only rows that could create nothing. Each affected list keeps its section header and that header's Insert command, and working prompts (sense and subsense gloss, a record's Text) stay.Where to look
WalkSublayout: the Type choice (ResolveLayoutChoiceGuid) and in-place descent --Compose_ASubrecord_ShowsItsFields_UnderItsHeader_ChosenByItsType,Compose_ASubrecordOfASubrecord_ShowsItsFieldsOneLevelFurtherIn._itemSummaryHvo) --Compose_ASubrecord_HasOneHeaderOfItsOwn_UnderTheSubrecordsSection.AddGhostPrompt's early return --Compose_AnEmptyListWithNoGhostField_HasNoPromptRow_UnlessTypingCanCreate,Compose_EmptyAllomorphSection_ShowsOnlyItsHeader_WithInsertOnItsMenu.Edit_ASubrecordsTitle_UnderItsHeader_SavesToTheSubrecord.Not here
Normalwithout its Type choice, as on the top-level record.Stack and verification. Merges into
main, on top of #1201 (merged). Hygiene build clean; suites pass (counts in the preflight section). Checked in FLEx: subrecord fields, and header text in both states. Not yet: the LT-22844 checks in the preflight section.Next: review
WalkSublayoutand the prompt rule; this leaves draft once the LT-22844 FLEx checks pass.Reading this a year from now -- start here
LT-22858 is the subrecord half of the Notebook detail view in New mode: before it, each subrecord showed only a header with nothing under it. The subrecord header wording was first written for #1201 (LT-22689) and moved here, because in #1201's build a subrecord header had nothing under it and so could never collapse to show its collapsed line. LT-22844 (the dead "Type to add SubRecords" box) joined this branch during preflight, because composing subrecord layouts made that dead row appear inside every subrecord with no subrecords. No working documents were kept on the branch; the reasoning is below.
Decisions, and why
<sublayout>imports as its own node kind (ViewNodeKind.Sublayout), carrying the layout name inTargetLayoutand the choice field inField. Both properties already exist on every node, so the override applier, the JSON serializer and every node copy carry them without change.WalkSublayoutresolves the choice withResolveLayoutChoiceGuidand compiles withCompileForObject(..., choiceGuid, overrides), exactly as the shown record does, and walks the result at the current depth. The existing (object, layout) visited set stops runaway recursion._itemMenuBindingsand_itemRootLabelledonly ever see sequence and atomic descents, which still compile with no choice; the comment over them now says so.HeavySummary) is the legacy item header, so the sequence's item header replaces it and its children (the<sublayout>) walk directly under the header rather than a level deeper.Surprising findings
Evidence
<sublayout>; a nameless one drops with a warningSublayoutElement_ImportsAsANode_NamingItsLayoutAndChoiceField,SublayoutElement_WithoutAName_IsDroppedWithAWarningSubrecord_ShowsItsNumberAndType_AndWhileCollapsed_ItsTitleCompose_ASubrecord_HasOneHeaderOfItsOwn_UnderTheSubrecordsSectionCompose_ASubrecord_ShowsItsFields_UnderItsHeader_ChosenByItsTypeCompose_ASubrecordOfASubrecord_ShowsItsFieldsOneLevelFurtherIn,Compose_DeeplyNestedSubrecords_AllShowTheirHeadersAndFieldsEdit_ASubrecordsTitle_UnderItsHeader_SavesToTheSubrecordCompose_AnEmptyListWithNoGhostField_HasNoPromptRow_UnlessTypingCanCreate,Compose_EmptyAllomorphSection_ShowsOnlyItsHeader_WithInsertOnItsMenu,Compose_ARecordWithNoSubrecords_KeepsTheSubrecordsHeader_WithNoPromptRowMutation checks: breaking the Type choice, the field indent, the importer, and the prompt rule each failed its tests, and restoring the old depth cap of 12 failed the eight-level test.
Preflight review details
Code Review Summary
Branch: LT-22858-subrecord-fields
Base: LT-22689-no-senses-banner (PR #1201, at 15c93cc), which merges into main
Date: 2026-10-23
Review model: Claude Opus 5.5 (Claude Code)
Files changed: 8 against the base (2 commits, plus staged in-review changes);
mainisone commit ahead of the base, so both branches want a rebase before merge
Overview
In Notebook > Record Edit in New mode, each subrecord showed only its header: the
SubRecord layout brings the subrecord's fields in with
<sublayout name="Normal" layoutChoiceField="Type"/>, which the layout importer droppedas a publishing construct (LT-22858). The branch imports
<sublayout>as aViewNodeKind.Sublayoutnode, and the composer lays out that layout for the subrecord'sType in place, so the fields sit one level under the subrecord's header and nested
subrecords repeat the pattern.
It also carries the subrecord header wording moved out of #1201 ("1. Observation"
expanded, "1. Observation - title" collapsed), with the skip of the duplicate SubRecords
header. The collapsed text had looked wrong in #1201's build only because a header with
nothing under it cannot collapse; with the fields under it, it does, and it matches the
WinForms view.
During review the branch also took in LT-22844: an empty list whose layout names no ghost
field no longer shows a "Type to add ..." row that cannot be typed in, as in the WinForms
DataTree. Composing subrecord layouts had made that dead row appear inside every subrecord
with no subrecords.
Tickets: LT-22858 and LT-22844 (both user-visible, testable from an install; keys present
in the commits).
Contract/API Changes
ViewNodeKind.Sublayout(FwAvalonia public enum);TargetLayoutnames the layoutand
Fieldnames the choice field.<sublayout>in any imported layout now becomes a node instead of an Info diagnostic;one without a
namedrops with a Warning under the samesublayout-droppedcode. Allother shipped
<sublayout>elements sit in jtview publishing layouts that no detail viewembeds.
Findings
Critical - Must address before merge
None.
Important - Should address before merge
now that subrecord layouts compose. (fixed during review: LT-22844 added --
AddGhostPromptleaves out a prompt that names no ghost field and can create nothing; tests updated)
Minor - Consider
using SIL.LCModel.Core.KernelInterfaces;inDetailSummaryLines.cs. (fixed during review: removed)Field Visibility / Move Field on a subrecord's rows target the(author: accepted; the top-level record already behaves this way, so this isNormallayoutwithout its Type choice (
RecordEditView.TryLocateOverrideTargetcompiles with nochoice).
not new; note it in the PR)
(each level uses three composer levels); the WinForms view has no limit. (accepted at
preflight; fixed after review in e437db3: the cap is now 48, about 16 subrecord levels,
covered by
Compose_DeeplyNestedSubrecords_AllShowTheirHeadersAndFields)Required Validation / Evidence
./build.ps1 -BuildTests -SkipNative -CommentHygiene -TokenHygiene: clean after thein-review changes.
./test.ps1 -NoBuild -TestProject xWorksTests -TestFilter 'FullyQualifiedName!~UiaSmokeTests':1844 total, 1842 passed, 2 skipped, with the LT-22844 change in place, after rebasing onto
LT-22689: Drop the Senses banner and word item headers like legacy #1201's review follow-up.
allomorph section, prompt-row and Lexicon menu tests).
./test.ps1 -NoBuild -TestProject FwAvaloniaTests: 937 total, 936 passed, 1 skipped,after the rebase.
LT-22844 rule each failed their tests.
expanded and collapsed header text matching the WinForms view.
subrecords show no "SubRecords" box; an entry with no allomorphs shows only the
Allomorphs header with Insert Allomorph; a part of speech with empty sections shows only
their headers; an entry with no senses and a sense with no subsenses still show their
working gloss rows.
Positive Observations
uses, in-place descent, and the (object, layout) recursion guard.
header carries the same Insert menu.
sense, are kept and tested.
Interview Notes
list "offers the watermarked add row rather than nothing". That row was never typeable;
the WinForms view shows only the Allomorphs header. Both tests that asserted the dead row
were rewritten.
usingfixed; override targeting and nesting depth accepted, to be notedin the PR.
one level under its header (
WalkSublayoutwithResolveLayoutChoiceGuid, and theSummary case walking the skipped row's children at the header's child depth).
In-Review Quality Check
DetailComposer.AddGhostPrompt: returns before adding a row when the layout names noghost field and
ResolveGhostCreationfound nothing to create.DetailSummaryLines.cs: unusedusingremoved.Compose_EmptyAllomorphSection_ShowsGhostAddRowbecameCompose_EmptyAllomorphSection_ShowsOnlyItsHeader_WithInsertOnItsMenu;Compose_GhostWithoutGhostField_IsNotEditable_SoTypingCannotCreateAndDiscardbecameCompose_AnEmptyListWithNoGhostField_HasNoPromptRow_UnlessTypingCanCreate; addedCompose_ARecordWithNoSubrecords_KeepsTheSubrecordsHeader_WithNoPromptRow.review follow-up (15c93cc). Hygiene build and the test runs above are on that tree.
Suggested Review Focus
DetailComposer.WalkSublayout, and the Summary case walking the skipped summaryrow's children at the header's child depth.
AddGhostPrompt's new early return, against the lists it removes rows from in thefour New-mode tools (Lexicon Edit, its popup, Notebook Record Edit, Grammar's
part-of-speech editor).
<sublayout>handling and its effect on the shipped-layout coveragereport.
🤖 Generated with Claude Code
This change is