Skip to content

LT-22689: Give detail section headers a +/- box and lasting state - #1184

Merged
thejambi merged 4 commits into
mainfrom
LT-22689-section-expansion
Oct 14, 2026
Merged

thejambi merged 4 commits into
mainfrom
LT-22689-section-expansion

Conversation

@thejambi

@thejambi thejambi commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Start here: Src/Common/FwAvalonia/Detail/DataTree.cs, ApplyVisibility() and ToggleSection(). Nearly everything else hangs off one change: a toggle now touches only the rows under the header.

Draft. The LT-22689 acceptance test passes in manual testing; CI pending.

Collapsible section headers in the New UI detail view now behave like the legacy DataTree's:

  • a +/- box replaces the triangle;
  • expansion state survives record switches and restarts;
  • a toggle adds or removes only that section's rows, so the header keeps focus, the view doesn't scroll, and a double-click expands then collapses as in legacy;
  • inserting into a collapsed section expands it.

Expanded section showing a minus box
Collapsed section showing a plus box; the empty Variants header keeps the box's space blank
Headless control-level captures (Avalonia Skia), not FLEx screenshots. Look at the box beside each header.

What breaks when a toggle stops rebuilding everything? The full rebuild used to reset each row's editor, reference-vector selection and label width on every toggle. One ShownRow[] record per field now holds those. The existing Tab-order, focus-memory, disposal and editing tests pass unchanged. On a 311-row headless view a toggle dropped from about 2.7 s to about 94 ms.

Where to look

  • ApplyVisibility: item positions across mixed insert/remove passes. Pinned by NestedCollapse_SurvivesParentCollapseAndReExpand_LikeLegacy and Toggling_KeepsTheHeaderWhereItWasInTheViewport.
  • Expansion keys are now {layout node}@{object GUID}; they used the per-session HVO. Pinned by DetailExpansionComposeTests.
  • The insert auto-expand leaves out only sections the user collapsed after the outgoing view's own edit, so type-then-collapse stays collapsed and other inserts still show. Pinned by RevealObjectsAddedSince_LeavesASectionCollapsedAfterTheOwnEditCollapsed and RevealObjectsAddedSince_ExpandsASectionCollapsedBeforeTheOwnEdit.

Deliberately not here (listed on LT-22689 for Jason)

  • Each section keeps its own state. Legacy shares one state per object, which looks like a legacy bug (confirmed live).
  • The "Senses" banner stays, although legacy has none. That's an open question on the ticket.
  • collapsedLayout, the alternate summary legacy shows for a collapsed sense, is a proposed follow-up.
  • Tab still skips headers (LT-22688), as in legacy.

Verification:

  • build.ps1 with comment and token hygiene: clean.
  • FwAvaloniaTests: 903 passed, 1 skipped.
  • xWorksTests: 1781 passed, 2 skipped.
  • Manual in FLEx: the acceptance test, double-click, and type-then-collapse.
  • Not checked: Notebook subrecords' doNotExpand in legacy.

Next: review ApplyVisibility; I'll mark this ready once CI is green.


Reading this a year from now -- start here

The analysis and working notes behind this change never lived in the repository, so this description is the record. The sections below hold:

  • the decisions and the reasons for them;
  • what was tried and rejected;
  • what legacy actually does (much of it surprising);
  • what was deferred.
Decisions, and why
  • Expansion keys are per section and per object GUID: {layout node StableId}@{guid:N}. Item headers use {node}/item@{item guid}.
    • The HVO was dropped because LCModel assigns it per session, so a saved state could miss its object after a restart.
    • Item keys name the item, not its position, so a collapsed sense stays collapsed after a reorder.
    • DetailField.StableId is unchanged. Automation ids, focus memory and tests depend on its format, so the key is a separate ExpansionKey that falls back to StableId.
  • Each section keeps its own state, unlike legacy. See "Surprising findings": legacy's linking of sections reads as a bug, not a design.
  • The insert auto-expand runs only on a re-show of the same record, and leaves out sections the user collapsed after the outgoing view's own edit.
    • Insert commands come in through the refresh controller, so they expand the section, matching legacy's Slice.InsertObject.
    • Typing into a section and then clicking its +/- box saves on the press and collapses on the release, so that section stays collapsed. A re-show after an edit can also carry an insert from another window or a menu command (the refresh controller merges them), so every other collapsed section that hides a new object still opens.
  • A header with nothing under it keeps the box's space blank so labels line up. Legacy hides the button, and the label shifts left.
  • Header text and the "Field Options" link have their size and colour set on the text itself.
    • A string inside a Button renders as AccessText, which the detail view's OfType<TextBlock>() style doesn't match. So header size depended on which style happened to reach it: 14px from the button theme, 11px from the view's text style.
    • Both header kinds now share DetailSectionHeader.CreateLabel, at 13px (legacy's 10pt) in FwSectionHeaderBrush (legacy's ControlDarkDark).
  • DetailField.ExpansionKey has a public setter, like HelpTopicId and the other properties the composer sets after construction. That avoids a 29th constructor parameter.
Paths not taken
  • Copying legacy's per-object key, so that collapsing Variants also collapses Allomorphs on return. Rejected as a legacy bug. An exact copy would also need separate in-session and on-return state, because legacy only links the sections when you come back to the entry.
  • Making headers Tab stops with Space/Enter toggles. Rejected. It would reverse LT-22688's tab order, and legacy Tab skips headers too (checked live).
  • Building every row once and hiding collapsed rows instead of removing them. Rejected:
    • hidden editors would stay alive;
    • sections that start collapsed would still be built;
    • the tests that pin "collapsed rows are absent and disposed" would have to be rewritten.
  • Making the full rebuild faster. Rejected: it would shrink the window in which a double-click's second click is lost, but not close it.
  • Removing the "Senses" banner to match legacy. Deferred to Jason (see the ticket). It's a small composer change plus indentation and test updates.
Reversals inside this commit

These are not visible in git log, because the branch is one commit.

  • Focus and scroll anchoring were built, then removed. After each full rebuild, the clicked header's viewport position was captured and restored, and focus was moved to its replacement once attached (FocusOnceAttached, ScrollToViewportTop). Once toggles became incremental, the header is never replaced, so both were deleted. The tests they introduced, Toggling_KeepsTheHeaderWhereItWasInTheViewport and Toggling_KeepsFocusOnTheHeader, stayed and now pin the incremental behaviour.

  • DetailSectionHeader.IsExpanded went from settable to read-only and back. It was read-only while every toggle rebuilt the header, and became settable again when the same header started flipping its box in place.

  • The insert auto-expand's own-edit rule was narrowed after PR feedback (a follow-up commit). It first skipped every re-show that followed the view's own edit; that also hid inserts from another window, or from a menu command whose refresh merged into the edit's re-show. It now leaves out only sections collapsed after the edit.

Surprising findings about legacy
  • Legacy's generic tree +/- box is dead code. SliceTreeNode draws it only when ShowPlusMinus is true, and nothing sets that. The visible control is SummarySlice.ExpandCollapseButton, a WinForms Button drawing the TreeView glyph. Even so, legacy Tab skips it (checked live).
  • Legacy reads expansion from the <slice> element, not the layout <part>. So all 85 expansion="expanded" attributes in the shipped layouts are ignored, and the default is expanded anyway. The single doNotExpand (Notebook subrecords) is on a <part> too, so by the code it has no effect. That hasn't been confirmed live.
  • Legacy's expansion key is "expand" + Base64(Object.Guid), with nothing identifying the section.
    • Its own doc comment says "we currently base it on the Guid of the Object".
    • Every summary section on an entry (Variants, Allomorphs, Grammatical Info. Details, Etymology) shares one saved state. When you return to the entry they all take whichever value was set last (confirmed live).
    • The state goes in the global settings file, not per project.
  • Legacy has no "Senses" row. Each sense's own HeavySummary header collapses. The Avalonia composer adds the banner.
Deferred, and what would unblock it
  • collapsedLayout. Legacy swaps a collapsed sense, Etymology, ExtendedNote or subrecord header to a SummaryCollapsed summary line. Needs a second summary composition per header; proposed as a follow-up ticket.
  • "Senses" banner removal. Waits on Jason's answer on LT-22689.
  • The auto-expand also fires on same-record re-shows that aren't inserts: composition recovery, Show Hidden Fields, a refresh from another window. Accepted: rare, at worst reopens a collapsed section once, and loses no data. Narrowing it would mean telling the view why it is being re-shown.
  • A collapse through the automation pattern can leave an edit in that section pending, because no focus change happens. The edit stays open until the next focus change, Enter or Escape. This is a pre-existing difference in how the Avalonia edit session saves, not parity: legacy writes as you type. The old full rebuild had the same gap, and no data is lost.
  • DetailHostControl's same-record check has no test. That WinForms host has no test harness; the logic underneath it has three tests.
  • Old LexEditExpansion:...@{hvo} settings are orphaned. Harmless for a preview UI, and legacy doesn't prune its keys either.
Evidence
Claim Pinned by
The +/- box flips with the toggle; an empty header has no box CollapsibleHeader_DrawsAPlusMinusBox_ThatFlipsWithTheToggle, HeaderWithNothingUnderIt_DrawsNoBox_AndIgnoresClicks
The header survives a toggle, keeping focus and position Toggling_KeepsFocusOnTheHeader, Toggling_KeepsTheHeaderWhereItWasInTheViewport
Double-click toggles twice DoubleClickingAHeader_TogglesItTwice. Headless also passed before the fix, because there the rebuild finished before the second click; confirmed manually in FLEx.
Screen-reader state AutomationPeer_ReportsAndDrivesTheExpandCollapseState
Header and link text size and colour SectionHeaderAndFieldOptionsText_TakeTheirOwnSizeAndColour
Keys use the GUID, one per section, and follow a reorder DetailExpansionComposeTests, ExpansionState_IsRecordedUnderTheExpansionKey, ComputeVisibility_ReadsStateByExpansionKey_NotStableId
Settings round-trip ExpansionState_PersistsToLocalSetting_AndIsReadBackInANewSession
Default expansion from the layout Import_SectionExpansion_ComesFromThePartReference (4 cases), Compose_ShippedLayout_StartsEverySectionExpanded
Insert reveal, and the collapsed-after-own-edit exception RevealObjectsAddedSince_* (3), GetHeadersHidingNewObjects_* (3)
A removed row's current vector item is cleared CollapsingTheSectionOfTheCurrentVectorItem_LeavesNoCurrentItem
  • The tests added during review were mutation-checked: each fails with its behaviour disabled.
  • Timing: median of 10 toggles on a synthetic 311-row view, headless Debug build, from a throwaway probe test that is not committed.

Header text and the Field Options link at the slice label size; both header kinds in the same grey
Headless control-level capture. The collapsible "Allomorphs" and the plain "Grammatical Info." header share size and colour; "Field options" is regular weight.

Preflight review details

Code Review Summary

Branch: LT-22689-section-expansion
Base: origin/main (a90754c)
Date: 2026-10-13
Review model: Claude Opus 5.5 (Claude Code), with an independent read-only reviewer subagent
Files changed: 15 (one commit, 1a0e95a, in-review fixes folded in)

Overview

LT-22689 asks for section collapse/expand in the Avalonia detail view to match the
WinForms DataTree:

  • an expander close to the WinForms +/- box,
  • which nodes collapse,
  • default expansion from the layout,
  • mouse and keyboard interaction,
  • persistence across record switches and restarts.

The branch adds:

  • a DetailSectionHeader control with a token-styled +/- box;
  • an explicit size and colour for header text and the "Field Options" link;
  • expansion state keyed per section by object GUID (previously by per-session HVO);
  • an incremental toggle that adds or removes only the rows under the header, keeping the
    header, its focus, its scroll position and a double-click's second click;
  • auto-expand when an object is inserted into a collapsed section;
  • the UI Automation ExpandCollapse pattern on headers.

The analysis found no critical issues. One important issue was found and fixed during
review: the auto-expand could undo a collapse made in the same gesture as a commit. The
acceptance test passes in manual testing by the author.

Jira: LT-22689 (user-visible; a ticket exists).

Contract/API Changes

New public members in FwAvalonia (internal consumers only, same build):

  • New types: DetailSectionHeader and DetailExpanderBox.
  • New members:
    • DetailField.ExpansionKey (settable; falls back to StableId)
    • DetailVisibility.GetHeadersHidingNewObjects
    • DataTree.RevealObjectsAddedSince(DataTree)
    • FwAvaloniaDensity: SectionHeaderFontSize, HotlinkFontSize, SectionHeaderBrush,
      and the TreeBox* members
  • Changed: DetailHostControl.ShowDetail gains an optional rootHvo parameter. Its
    only caller is RecordEditView.
  • New theme tokens:
    • DataTree.TreeBoxSize, TreeBoxGlyphLength, TreeBoxGlyphThickness, TreeBoxGap
    • FwTreeBoxBorderBrush, FwTreeBoxGlyphBrush, FwSectionHeaderBrush

Persisted settings: the LexEditExpansion: local-setting keys now end in
@{guid:N} instead of @{hvo}. Old keys are orphaned and harmless; there is no
migration, which is acceptable for a preview UI.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

  • Auto-expand on insert could undo a collapse made in the same gesture as a
    commit.
    Typing in a ghost row, then clicking the section's +/- box, committed on the
    press, collapsed on the release, and the commit's re-show then auto-expanded and saved
    "expanded". (fixed during review: RevealObjectsAddedSince takes the outgoing view and leaves out the sections
    the user collapsed after that view's own edit. A first version skipped every re-show after an
    own edit, which also hid inserts from other windows or a merged menu-command refresh; PR
    feedback narrowed it. Tests RevealObjectsAddedSince_LeavesASectionCollapsedAfterTheOwnEditCollapsed
    and RevealObjectsAddedSince_ExpandsASectionCollapsedBeforeTheOwnEdit; each confirmed by mutation.)

Minor - Consider

  • Auto-expand also fires on same-root re-shows that are not inserts (composition
    recovery, Show Hidden Fields, a refresh from another window), and saves the expansion.

    (author: accept; rare, at worst reopens a collapsed section once, no data loss; note
    in PR)
  • Two stale comments: the DetailComposer item-header comment did not say which
    key is item-based, and the DetailVisibility class summary compared itself to how
    DataTree used to work. (fixed during review: both reworded; the rewritten summary
    also drops the word "legacy")
  • DetailField.ExpansionKey has a public setter on an otherwise read-only model.
    (author: dismiss; DetailField already has several properties the composer sets after
    construction, and a 29th constructor parameter isn't worth the churn)
  • Test gaps in the incremental toggle. (fixed during review: added
    CollapsingTheSectionOfTheCurrentVectorItem_LeavesNoCurrentItem, confirmed by
    mutation. Already covered: removed editors disposed. Accepted gaps: the unsaved-text
    check ignoring removed rows, and the DetailHostControl same-root gate, a WinForms host
    with no test harness; the logic beneath it has three tests.)
  • A collapse through the automation pattern (no focus move) can leave an edit
    inside the section pending until the next focus change, Enter or Escape.
    (author:
    accept and note in PR as a pre-existing difference in how the Avalonia edit session
    saves; not a parity match. The WinForms DataTree writes as you type; the old full
    rebuild had the same gap; no data is lost)

Required Validation / Evidence

Run on the rebased branch with all in-review fixes staged:

  • ./build.ps1 -BuildTests -SkipNative -CommentHygiene -TokenHygiene: succeeded; both
    hygiene checks clean. Native was skipped because no native files changed on the branch
    or upstream since the last full native build.
  • ./test.ps1 -TestProject Src/Common/FwAvalonia/FwAvaloniaTests: 903 passed, 1 skipped.
  • ./test.ps1 -TestProject Src/xWorks/xWorksTests: 1781 passed, 2 skipped.
  • gitlint on the branch commit: clean.
  • Mutation checks: the two new review tests each fail with their behaviour disabled.
  • Headless captures: Output/ManualEvidence/LT-22689/Detail-SectionExpander-0{1,2,3}
    (control-level headless, not FLEx screenshots).

Manual validation:

  • Acceptance test steps 1-5 in FLEx (author confirmed).
  • WinForms DataTree checks (author, live): Tab skips headers; Variants, Allomorphs and
    the other entry sections share one saved state on return; no "Senses" banner, only
    per-sense headers.
  • Double-click on a +/- box after the incremental toggle change (author confirmed).
  • Type in a section, then click its +/- box: the text is saved and the section stays
    collapsed (author confirmed).
  • Hover or press background on headers (Semi button theme), not yet looked at.
  • Notebook subrecords with doNotExpand in the WinForms DataTree (Q6), not yet
    checked.

Positive Observations

  • ApplyVisibility replaces four parallel lists with one ShownRow[], so a row's parts
    can't get out of step. Toggle cost went from about 2.7 s to about 94 ms on a 311-row
    headless view.
  • GUID-based expansion keys fix a real cross-session bug, and item keys follow a sense
    through a reorder.
  • Header click and the automation pattern share one toggle path. Header text sizing is
    explicit, so it no longer depends on which style reaches it.
  • Behaviour changes came with tests written first where practical (scroll, identity,
    double-click, font size), and the review fixes were mutation-checked.

Interview Notes

  • Purpose (author): make section collapse/expand match the WinForms DataTree, per
    LT-22689.
  • Walkthrough (author's words): "Double click on detail section header in Avalonia
    can expand and then collapse now that the view only rebuilds the related fields." This
    is correct in substance. Clarified in discussion: the header itself is never replaced,
    so the second click lands on the same control. The author asked for a chunk-by-chunk
    explanation of the branch first, and it was given.
  • Decisions:
    • Per-section expansion state is kept on purpose. The WinForms DataTree's shared
      per-object state is judged a bug and is not copied.
    • The "Senses" banner stays as a stated divergence, with an open question to Jason in
      the ticket comment.
    • Keyboard matches the WinForms DataTree: Tab skips headers, and LT-22688's tab order
      is unchanged.
    • Single-click on header text toggles, an approved divergence.
    • collapsedLayout is proposed as a follow-up ticket.
  • Anything else to flag: author had nothing further.

In-Review Quality Check

In-review changes, folded into the branch commit: DataTree.cs, DetailHostControl.cs,
DetailVisibility.cs, DetailComposer.cs, DetailViewingParityTests.cs. The build with
both hygiene checks and both full test suites pass with them in place (figures above).

Suggested Review Focus

  • DataTree.ApplyVisibility: the position bookkeeping for _form.Items across mixed
    insert/remove passes, and removing a row before disposing its editor.
  • RevealObjectsAddedSince and _collapsedAfterOwnEdit: when an insert reveals a section
    and when it must not.
  • ExpansionKey format and the per-section divergence from the WinForms DataTree's
    key.
  • DetailSectionHeader automation peer: ExpandCollapse state and its change
    notification.

This change is Reviewable

Collapsible section headers in the Avalonia detail view show a +/- box,
drawn from new tokens, in place of a text triangle. Header text and the
"Field Options" link take the slice label size, and header text takes a
colour token of its own instead of the button theme's link blue.

A toggle inserts or removes only the rows under the header. The header
itself stays, so it keeps its focus, its place in the view, and the
second click of a double-click. On a 311-row test view a toggle went
from about 2.7 s to under 0.1 s.

Expansion state is keyed per section by the object's GUID, not its
per-session HVO, so it survives restarts and sense reorders. The
WinForms DataTree keys by object alone, which links an entry's sections
together on return; that is not copied.

Inserting into a collapsed section expands it, and headers expose the
UI Automation ExpandCollapse pattern. The LT-22688 tab order is
unchanged: Tab still skips headers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

NUnit Tests

    1 files  ± 0      1 suites  ±0   10m 11s ⏱️ -11s
6 619 tests +24  6 534 ✅ +24  85 💤 ±0  0 ❌ ±0 
6 628 runs  +24  6 543 ✅ +24  85 💤 ±0  0 ❌ ±0 

Results for commit d488ed9. ± Comparison against base commit a90754c.

♻️ This comment has been updated with latest results.

@codecov-commenter

codecov-commenter commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.61753% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.52%. Comparing base (a90754c) to head (d488ed9).

Files with missing lines Patch % Lines
Src/Common/FwAvalonia/Detail/DataTree.cs 95.83% 1 Missing and 3 partials ⚠️
Src/Common/FwAvalonia/Detail/DetailVisibility.cs 90.32% 1 Missing and 2 partials ⚠️
...rc/Common/FwAvalonia/Detail/DetailSectionHeader.cs 97.67% 1 Missing and 1 partial ⚠️
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs 66.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1184      +/-   ##
==========================================
+ Coverage   39.49%   39.52%   +0.03%     
==========================================
  Files        1537     1538       +1     
  Lines      354392   354569     +177     
  Branches    41058    41090      +32     
==========================================
+ Hits       139974   140154     +180     
+ Misses     185044   185038       -6     
- Partials    29374    29377       +3     
Files with missing lines Coverage Δ
Src/Common/FwAvalonia/Detail/DetailModel.cs 79.00% <100.00%> (+0.04%) ⬆️
Src/Common/FwAvalonia/DetailHostControl.cs 72.41% <100.00%> (+3.18%) ⬆️
Src/Common/FwAvalonia/FwAvaloniaDensity.cs 97.01% <100.00%> (+0.40%) ⬆️
...AvaloniaTheme/Tokens/DataTree/DataTreeTokens.axaml 100.00% <100.00%> (ø)
.../Common/FwAvaloniaTheme/Tokens/FwColorTokens.axaml 100.00% <100.00%> (ø)
Src/xWorks/Avalonia/Composer/DetailComposer.cs 71.00% <100.00%> (+0.07%) ⬆️
...rc/Common/FwAvalonia/Detail/DetailSectionHeader.cs 97.67% <97.67%> (ø)
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs 61.91% <66.66%> (+0.85%) ⬆️
Src/Common/FwAvalonia/Detail/DetailVisibility.cs 88.88% <90.32%> (+0.20%) ⬆️
Src/Common/FwAvalonia/Detail/DataTree.cs 95.13% <95.83%> (+0.11%) ⬆️

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

Zachary Burnham and others added 3 commits October 13, 2026 17:38
Drop the reasons for not using other colours, styles or keys from the
token and header comments, and fix two test comments that still
described a toggle as rebuilding every row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The insert auto-expand skipped every re-show that followed the view's
own edit. A re-show can carry an insert from another window, or from a
menu command whose refresh joined the edit's re-show, and those inserts
stayed hidden in their collapsed sections.

The view now records the sections the user collapsed after its own edit
completed, and the reveal leaves out only those. Typing into a section
and then collapsing it still leaves it collapsed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The insert auto-expand judged each collapsed header on its own. When a
parent the user collapsed after an edit stayed closed, a collapsed child
under it was still expanded and saved, so reopening the parent later
also reopened the child.

The reveal now decides per new row: a row that a must-stay-collapsed
header hides contributes none of its headers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@thejambi
thejambi marked this pull request as ready for review October 13, 2026 22:18

@mark-sil mark-sil 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.

@mark-sil reviewed 15 files and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on thejambi).

@thejambi
thejambi merged commit 2c2a4c3 into main Oct 14, 2026
9 checks passed
@thejambi
thejambi deleted the LT-22689-section-expansion branch October 14, 2026 11:36
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