Skip to content

LT-22691: Answer the environment menus natively - #1175

Merged
mark-sil merged 4 commits into
mainfrom
LT-22691l
Oct 7, 2026
Merged

mark-sil merged 4 commits into
mainfrom
LT-22691l

Conversation

@mark-sil

@mark-sil mark-sil commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Start here: Src/xWorks/Avalonia/Hosting/EnvironmentMenuLeaves.cs, the one builder behind all three menus. Then FwReferenceVectorField in Src/Common/FwAvalonia/Detail/FwFieldControls.cs, the editor seam the inserts type through.

What it does. The environment menus of the Avalonia detail view are answered natively: the item menu of an environment chip (mnuEnvReferenceChoices) and the two label menus that carry only the five inserts (mnuDataTree-Environments-Insert, mnuDataTree-StringRepresentation-Insert). The five inserts type into the row's current editor at its caret and commit when the item is left. Describe Error explains the text in the editor, unsaved edits included. Show in Environments list and Ctrl+click jump as before. With both item-menu ids owned, the colleague path is deleted.

The question to settle. Where does an insert go when the row is a set of TextBoxes rather than one rootbox? Into the editor, never the model: each staged edit reconciles against the project, so writing per insert would mint an environment for every intermediate state. The review is therefore about focus and timing. The request snapshots the editor's text and selection before the menu takes focus, the editor holds its commit-on-blur while the menu or the chooser has focus, and a label gesture focuses its row as a WinForms tree-node click does.

Where to look.

  • IDetailTextSelection: the snapshot and the write-back, pinned by RetypableVectorItemTests.
  • BeginMenuGesture, EndMenuGesture and DataTree.TakeRowFocus: pending text survives the menu, the chooser and a click elsewhere; focus returns to the row. Tested headlessly.
  • EnvironmentInsertRules: one 29-case table; the two WinForms views now call it.
  • A right press focuses the item, places the caret and keeps a range the pointer is inside, as SimpleRootSite does.
  • The String Representation id is owned with its inserts disabled: its tool is not in the Avalonia catalog.

Deliberately not here. Consolidating the three XML spellings of the insert commands. Converting the natural-class chooser and PhEnvStrRepresentationSlice to Avalonia. A right-click on the typed slot opens the label menu, so Describe Error is not offered there, where WinForms offers it on its empty line.

Verification. Main merged after #1169. Both hygiene gates clean; 202 targeted tests green across FdoUi, FwAvalonia and xWorks. Manual pass of 15 scenarios in Lexicon Edit, plus five covering the review fixes; the first pass found four row-editor defects, all fixed here. Not run: the full suite.

Next: approve, or say if the row-focus change in DataTree should be its own PR.


Reading this a year from now -- start here

This PR is stage 1, track A, row 2 of the plan that retires the hidden WinForms DataTree the Avalonia detail view keeps alive to answer context-menu commands. The unit of work is one menu id, answered in full by one IDetailMenuAuthority. Row 1 (#1172) answered mnuReferenceChoices; earlier rows shipped mnuReorderVector (#1143), the help-topic engine (#1151), the owned-menu bridge (#1153) and the per-object and Help menus (#1161); track B answered mnuDataTree-MultiStringSlice (#1169). The plan of record lives in gitignored working notes under Docs/migration/working/, so the reasoning behind these ids is recorded here rather than in the tree.

Decisions, and why

Inserts edit the editor, not the model. The composer's text setter reconciles every staged string against the project: text that strips to the item's own name renames the shared environment, text that strips differently re-points the item and creates the target when the project has none. An insert staged through it would mint an environment for /()_ on the way to /(a)_. The row editor already stages only when an edit finishes for exactly this reason, so the inserts write into the TextBox and ride that commit. Undo granularity then matches WinForms: one step per committed item.

A separate authority for the label menus. ReferenceItemMenuAuthority is built per item request around the clicked item's object UI and rejects a null item. The label menu arrives with no item UI and possibly no current item, and its ids must be owned regardless of selection because OwnsAll decides whether the adapter is built. EnvironmentInsertMenuAuthority is built from the request alone. The leaf logic is shared through EnvironmentMenuLeaves, routed by command message because the three menus are configured in three files, each with its own command ids for the same five messages.

The WinForms views call the shared rule. PhoneEnvReferenceView and PhEnvStrRepresentationSlice carried two private copies of the enablement rules, neither tested. Both now call EnvironmentInsertRules, as Slice, DataTree and DTMenuHandler call the earlier shared helpers. The one corner that changed is no corner at all: the Environments tool passed position -1 to the recognizer when it had no selection, and the recognizer already answered false.

The String Representation id is owned now. Its only row is on the EnvironmentEdit tool, inert in UIFrameworkRegistry, and composes as Unsupported because its editor is a CustomWithParams plugin slice. Owning it costs one id in Owns and a test; the leaves come out disabled through the same builder, and go live without change the day the slice becomes a text row implementing the seam.

No current editor: the inserts are disabled. WinForms enables Insert Environment slash on its always-present empty last line, and also when it has no selection at all, where choosing it does nothing. The typed slot the row already has is that empty line: a right-click on it makes it the menu's editor whatever had focus before, so the slash is enabled and types there. The remaining case, a label right-click on a row never clicked into since the record was shown, disables all five.

The field-options button no longer takes focus on a pointer press. A press focused the button, which blurred the row's editor and committed its pending text before the menu opened. The press is swallowed on the gutter rail and the release raises the menu; keyboard activation still runs Click. SliceTreeNode.OnMouseDown deliberately skips its base call for the same reason.

A label gesture focuses its row. Once the button stopped taking focus, a menu opened on one row closed back onto whichever row held focus, scrolling the view there. Slice.OnTreeNodeClick calls TakeFocus; DataTree.TakeRowFocus does the same unless focus is already in the row. A vector row focuses its button rather than an item, because focusing an item makes it the current item the menu acts on, and a label menu on an unclicked vector row must carry none.

A right press focuses the item and places the caret. Avalonia's TextBox applies the keep-the-range-or-move rule on the right-button release, after the context request has already been raised from the presenter and snapshotted. Applying the same rule on the press lets the request see it. A right press does not focus a TextBox by itself, so the row focuses it, matching the WinForms view, where a right-click installs a selection at the point.

Describe Error judges the editor's text. The five inserts are enabled from the text the request snapshotted, unsaved edits included, so Describe Error judges the same text, as the WinForms view judges the text in its view; a read-only row falls back to the saved name. The check is EnvironmentErrors in Src/FdoUi/DetailRules, built on the recognizer over the project's phonemes and natural classes, and both WinForms environment views call it.

The theme Cut/Copy/Paste flyout is removed from the item editors and the slot, as the in-string editors already removed it, so only the bridged menu can show.

Paths not taken
  • Staging each insert through TrySetReferenceItemText. Rejected: it mints an environment per intermediate state, and needs two undo steps where WinForms needs one.
  • Focusable = false on the field-options button. Rejected: three tests pin that the button is keyboard-focusable, which is the keyboard route to a label menu on a row with no focusable editor.
  • Keeping the colleague path for the environments id. Rejected before this PR: the bridge hides a leaf before any interceptor sees it, so the six commands came out disabled from a hidden view that has no selection.
  • Reading the editor's live selection at execute time. Rejected: a TextBox collapses a span selection when it loses focus to the menu, so the insert acted on a caret the enablement had not been judged on. The request's snapshot is the span the insert replaces.
  • A shared non-focusing button for all hover affordances. The general form of the press interception; not taken because nothing else needs it yet.
Surprising findings
  • The typed slot already existed. The row editor's "Add item" slot from LT-22672 is the WinForms empty last line, so the no-editor divergence shrank to a label right-click on a row never clicked into.
  • The two WinForms views formatted the error differently. PhoneEnvReferenceView used StringServices.CreateErrorMessageFromXml; the Environments tool used PhonEnvRecognizer.CreateErrorMessageFromXml, which drops the space after the colon. EnvironmentErrors uses the former, which is also what PhEnvironment.CheckConstraints produces, so the Environments tool's message gains the space. DescribeError_IsEnabledForAMalformedEnvironment_WithTheExplanationWinFormsShows proves the Avalonia text matches the allomorph view's.
  • The order of release-time events. The context request is raised from the text presenter, which holds pointer capture, before the TextBox's own release handler moves the caret. Any snapshot taken in the request sees the pre-click caret unless the press places it first.
  • The theme flyout won once in manual testing on a right-click inside a selection, although it is an ordinary subscriber added after the row's own handler. The cause was not found; removing the flyout makes the question moot.
  • The manual pass found four defects the design discussion had not anticipated, all in the row editor's focus handling: the unfocused right-click, the collapsed selection, the slot's own flyout, and the scroll-back after the button stopped taking focus. Each is now pinned by a headless test.
Deferred, and what would unblock it
  • The natural-class chooser stays WinForms, reached through a new choose-only overload of ReallySimpleListChooser.ChooseNaturalClass. Its Avalonia replacement is a dialog conversion of its own.
  • The String Representation row goes live when PhEnvStrRepresentationSlice is converted to an Avalonia text row implementing IDetailTextSelection and the EnvironmentEdit tool is moved out of Phase1FollowUpTools.
  • One set of command ids for the three menus is a configuration cleanup the WinForms UI shares; this branch routes by message so it does not depend on it.
  • Stage 2, the sense cluster, is next per the plan. Stage 1 is complete with this PR: track B (the writing-systems engine and mnuDataTree-MultiStringSlice) merged as LT-22691: Answer the Writing Systems menu natively on multi-string rows #1169.
Evidence

The contracts every authority carries, in DetailObjectCommandExecutionTests:

  1. Leaf coverage. EnvironmentItemAuthority_AnswersEveryLeafOfItsMenu_AndTakesTheJumpAsDefault populates the real mnuEnvReferenceChoices group, asserts its seven leaves each have a configuration node and build, then builds the id through XCoreMenuBridge. EnvironmentsLabelMenu_IsFullyOwned_AndItsInsertsTypeIntoTheSlot and StringRepresentationLabelMenu_IsOwned_WithItsInsertsDisabled do the same for the two label ids, the latter composing a real PhEnvironment with the Environments layout.
  2. Rejection. EnvironmentInsertAuthority_RejectsALeafItDoesNotAnswer.
  3. Equivalence. The colleague baseline is gone with the colleague path; ReferenceItemMenu_RendersThePinnedTree_ForEachRowKind pins the trees captured from it for the Subentries and Anthropology Categories items. For the environment ids the mediator path rendered every command disabled, so enablement is asserted against the rules by caret position instead: EnvironmentItemMenu_EnablesTheInserts_ByTheEditorsCaret_AndTypesIntoIt.
  4. No mediator. EnvironmentItemMenu_IsBuiltWithoutTheAdapterOrTheMediator and the label-menu test above assert the display spy was never asked and the hidden tree never built. ItemMenu_IsOwned_ForReferenceChoices_AndForEnvironments states that both item-menu ids are owned.

Row editor, RetypableVectorItemTests: the request snapshots the focused editor's text and selection; a right press makes an editor the current one, focuses it and places the caret; a right press inside a selection keeps it; the slot is the editor for a label menu and its right-click raises the label menu without committing, makes the slot the editor whether an item had focus or nothing did, and stages an item's unsaved text as focus leaves it; ReplaceEditorSelection types at the caret, replaces a span, replaces the snapshotted span after the editor collapsed its own, and stages nothing; a menu gesture holds the commit while focus is elsewhere and returns focus after, and leaves focus alone when it never moved.

Detail tree, DetailMenuRequestTests: a pointer click on the field-options button keeps focus in the row's editor and still raises the menu; a button press or label right-click on another row focuses that row's editor; the same on a vector row focuses its button and makes no item current.

Leaves, EnvironmentMenuLeavesTests through a fake host: the natural-class insert types the abbreviation in brackets at the snapshotted caret, types nothing when cancelled; the optional item leaves the caret between its parentheses; Describe Error shows the host's explanation of the text it is given and is disabled when there is none. DescribeError_IsEnabled_WhenAnUnsavedEditBreaksAWellFormedEnvironment and DescribeError_IsDisabled_WhenAnUnsavedEditFixesAMalformedEnvironment in the hosting fixture run the real recognizer over unsaved editor text, the second against a malformed saved environment; both fail if Describe Error judges the saved text.

Rules, EnvironmentInsertRulesTests in FdoUiTests: 29 cases over the four rules, including no-selection, reversed selection and the recognizer's positions; EnvironmentErrorsTests for the error rule; DetailRulesBoundaryTests unchanged and passing.

Manual (Lexicon Edit, Avalonia unless noted), 15 scenarios: insert at the caret, commit on Tab, one undo step; right-click on an unfocused item; right-click inside a selection replacing it; Escape and click-elsewhere dismissal keeping pending text; the slot with Insert Environment bar and with Escape; the field-options button with pending text; Shift+F10 on an item; Insert Natural Class chosen and cancelled; Describe Error on a malformed item; Ctrl+click on an environment and on a subentry; Subentries and Gloss menus unchanged including focus staying on the row; the no-editor comparison against WinForms; the two WinForms views' insert rules. After review: the slot right-click with an item focused and on an unclicked row, an item's unsaved text surviving it, Describe Error following unsaved text, and the Environments tool's message.

Preflight review details

Code Review Summary

Branch: LT-22691l
Base: main
Date: 2026-10-06
Review model: Claude Fable 5.1
Files changed: 24

Overview

Stage 1, track A, row 2 of the hidden-DataTree retirement plan (LT-22691): the three
environment menus are answered natively. ReferenceItemMenuAuthority takes
mnuEnvReferenceChoices (the item menu of an environment chip) and a new
EnvironmentInsertMenuAuthority takes mnuDataTree-Environments-Insert and
mnuDataTree-StringRepresentation-Insert (label menus carrying only the five inserts).
Both build their leaves through EnvironmentMenuLeaves over EnvironmentInsertRules in
Src/FdoUi/DetailRules, which the two WinForms views (PhoneEnvReferenceView,
PhEnvStrRepresentationSlice) now call too. With every id an item's object UI can name
owned, the colleague path (BuildItemMenuThroughTheColleague, AddMoveCommands,
MoveCommandItem, the Ctrl+click fallback) is deleted.

The inserts type into the row's current editor at its caret and commit when the item is
left, as the WinForms view types into its scratch cache. That needed a text-editor seam on
the row editor (IDetailTextSelection, snapshotted into DetailMenuRequest), a menu
gesture that holds the editor's commit-on-blur while the menu or the natural-class chooser
has focus, a field-options button that no longer takes focus on a pointer press, and a
label gesture that focuses its row as Slice.TakeFocus does. Describe Error judges the
editor's text, unsaved edits included, through a shared EnvironmentErrors rule both
WinForms views also call.

Analysis found no Critical issues. Two high-effort review passes produced eleven
findings, nine fixed in review; the author's own review added two comment fixes. The
author's 15-scenario manual pass found and drove four fixes in the row editor before the
code was settled.

Contract/API Changes

  • New public interface IDetailTextSelection (FwAvalonia); FwReferenceVectorField
    implements it. DetailMenuRequest gains HasTextEditor, EditorText,
    EditorSelectionAnchor, EditorSelectionEnd, ReplaceEditorSelection,
    BeginMenuGesture, EndMenuGesture.
  • New public static DetailMenuItem.Disabled(label).
  • New public static class EnvironmentInsertRules in SIL.FieldWorks.Common.DetailRules.
  • New public overload ReallySimpleListChooser.ChooseNaturalClass(cache, persistence, mediator, propertyTable) returning the chosen class; the rootbox overload calls it.
  • New resource xWorksStrings.ksEnvironmentErrorTitle ("Error in Environment"); the
    DetailControls copy is internal to its assembly.
  • Internal: IReferenceItemMenuHost extends the new IEnvironmentMenuHost;
    RecordEditView.BuildItemMenuThroughTheColleague, AddMoveCommands and
    MoveCommandItem removed; an item-menu id without an authority now logs and shows
    nothing (only two ids exist, both owned).
  • Behaviour: item editors and the typed slot of a bridged vector row have no theme
    Cut/Copy/Paste flyout; a right-click on the slot raises the row's label menu.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

  • Insert used the editor's live selection, which a TextBox collapses when the menu
    takes focus
    (fixed during review: the insert replaces the span the request
    snapshotted; ReplaceEditorText takes the span)

  • Natural-class insert and Describe Error execution untested (fixed during
    review: EnvironmentMenuLeavesTests drives both through a fake host)

  • A modal chooser can close the flyout before the insert runs, leaving the insert
    outside the gesture
    (fixed during review: an insert with no gesture active focuses
    the editor itself)

  • Right-click on an unfocused item left focus elsewhere, so Escape did not return
    to the item
    (fixed during review, found by manual testing: a right press focuses the
    editor)

  • Right-click inside a selection collapsed it, so the insert replaced nothing
    (fixed during review, found by manual testing: a press inside a range keeps it, the
    SimpleRootSite rule)

  • The typed slot had no bridged menu; its theme flyout took focus and committed the
    typed text
    (fixed during review, found by manual testing: the slot raises the label
    menu with itself as editor)

  • A label gesture no longer focused its row, so the menu closed back onto the
    previously focused row and scrolled to it
    (fixed during review, found by manual
    testing: TakeRowFocus, matching Slice.TakeFocus; a vector row focuses its button,
    never an item)

  • Right-clicking the Add item slot acted on the previously current editor, or none
    (fixed after review, thejambi: the slot takes the item editors' right-press handling)

  • Describe Error judged the saved environment while the inserts judged the editor's
    text
    (fixed after review, thejambi: both judge the editor's text through
    EnvironmentErrors)

Minor - Consider

  • Kebab press flag not reset on lost pointer capture (fixed during review:
    PointerCaptureLost resets it)
  • Disabled(label) duplicated across authorities (fixed during review:
    DetailMenuItem.Disabled)
  • Theme-flyout removal written three times in the row editor (fixed during
    review: WireBridgedMenu for the item editors and the slot)
  • Two test fakes for IDetailTextSelection (fixed during review:
    TextEditorStub)
  • Light dismiss always refocuses the editor, undocumented (fixed during review:
    stated in EndMenuGesture's doc comment; matches a WinForms context menu)
  • A test comment cited a PR number (fixed during review, author's finding)
  • "Spell the same commands three ways" was unclear (fixed during review,
    author's finding: the comment now says the three menus are configured in three files
    with their own command ids for the same messages)
  • The kebab's press interception is specific to one rail; a shared non-focusing
    button for the hover affordances would be the general form
    (author: nothing else
    needs it yet)
  • A right press focuses the pressed item, committing another item's pending
    text
    (author: the same path a left press has always taken; the re-show is queued
    through the refresh controller, not synchronous)

Required Validation / Evidence

  • .\build.ps1 -CommentHygiene -TokenHygiene: clean on the final tree.
  • .\test.ps1 -CommentHygiene -TokenHygiene -SkipNative -TestFilter ... on the final
    tree: 161/161 across RetypableVectorItemTests, DetailMenuRequestTests,
    DetailEditorParityTests, HoverRevealTests, DetailEditingTests,
    DetailFocus*, DetailObjectCommandExecutionTests. The previous run of the wider
    set, adding EnvironmentInsertRulesTests, DetailRulesBoundaryTests and
    EnvironmentMenuLeavesTests: 184/184.
  • Whole FwAvaloniaTests assembly (2026-10-05, before the last four row-editor fixes):
    805 passed, 1 pre-existing skip. xWorksTests detail-related subset: 326/326.
  • Manual: the author's 15-scenario pass in Lexicon Edit on 2026-10-06, all passed,
    including the WinForms twins of the shared rules and the no-editor comparison.
  • After the review fixes and the merge of main: 202/202 across the fixtures above plus
    EnvironmentErrorsTests and EnvironmentMenuLeavesTests; the merge alone, adding
    LT-22691: Answer the Writing Systems menu natively on multi-string rows #1169's writing-system fixtures: 207/207.
  • Manual, after review: five scenarios on the slot right-click, unsaved item text,
    Describe Error on unsaved text and the Environments tool's message, all passed.
  • Not run: the full suite.
  • Jira: LT-22691 carried in the branch and title; user-visible change.

Positive Observations

  • Every leaf of the three owned ids is answered; the contract tests build each id
    through the bridge, so an unanswered leaf fails a test instead of reverting the menu.
  • The Describe Error wording is proved identical to the WinForms allomorph view's by
    test, and the check exists once for all three views.
  • The String Representation row is owned as a declaration: composed from the real
    Environments layout in a test, with its inserts disabled, so the day the tool flips
    nothing reaches for the adapter.
  • The insert rules exist once, with a 29-case table, and the WinForms views lost two
    private copies of them.

Interview Notes

  • The author performed the code review and the manual pass personally and reviewed the
    commit message before the commit.
  • Decisions settled before coding and not reopened: inserts edit the editor rather than
    the model (a model edit per insert would mint an environment for every intermediate
    state); a separate label-menu authority; the WinForms views re-pointed at the shared
    rule; the String Representation id owned now; the no-editor case disabled, with the
    typed slot covering the WinForms empty line.
  • Divergence the author chose to keep after comparing in the product: a right-click on
    the slot opens the label menu (inserts plus the field items) where WinForms shows the
    environment item menu without the jump, so Describe Error is not offered on the slot.
  • thejambi's two review comments were taken as suggested; for Describe Error the author
    chose to judge the editor's text with the recognizer over disabling it during an edit.
  • No unresolved items.

In-Review Quality Check

All in-review fixes rebuilt with both hygiene gates and rerun through the fixtures
listed above before the commit.

Suggested Review Focus

  • FwReferenceVectorField's focus and gesture rules: right press, slot, menu
    gesture, light dismiss.
  • DataTree.TakeRowFocus and the kebab's press interception.
  • EnvironmentInsertRules against the two WinForms call sites.

🤖 Generated with Claude Code


This change is Reviewable

The item menu of an environment and the two label menus that carry only
the environment inserts are now answered from the row alone, so they do
not need the hidden DataTree adapter any more. The five inserts type
into the row's current editor at its caret and commit when the item is
left, as the WinForms view types into its scratch cache; editing the
model per insert would mint an environment for every intermediate state.
Describe Error reports the domain's own explanation, which is the text
the WinForms view shows.

The enablement rules move to FdoUi/DetailRules and the two WinForms views
call them there. A menu request now snapshots the editor's text and
selection before the menu can take focus, and the editor holds its
commit while the menu or the natural-class chooser has focus. A label
gesture focuses its row as a WinForms tree-node click does, without
taking focus from the row's own editor, so the menu closes back onto
that row. A right press on an item focuses it and places the caret under
the pointer, keeping a selection the pointer is inside.

With no current editor the inserts are disabled, where the WinForms view
offers a slash that acts on its empty last line; the typed slot covers
that case once the caret is in it, and a right-click on the slot opens
the row's label menu so its inserts can type there.

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

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

NUnit Tests

    1 files  ± 0      1 suites  ±0   14m 6s ⏱️ + 1m 2s
6 474 tests +68  6 389 ✅ +68  85 💤 ±0  0 ❌ ±0 
6 483 runs  +68  6 398 ✅ +68  85 💤 ±0  0 ❌ ±0 

Results for commit d775138. ± Comparison against base commit 44b1d04.

This pull request removes 2 and adds 70 tests. Note that renamed tests count towards both.
SIL.FieldWorks.XWorks.DetailObjectCommandExecutionTests ‑ ItemMenu_IsOwned_ForReferenceChoices_ButNotForEnvironments
SIL.FieldWorks.XWorks.DetailObjectCommandExecutionTests ‑ ReferenceItemMenu_NativeAuthority_RendersWhatTheColleaguePathRendered_ForEveryItem
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ AMenuGesture_HoldsTheCommit_WhileTheMenuHasFocus_AndReturnsFocusAfter
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ AMenuGesture_ThatNeverTookFocus_LeavesFocusWhereItWas
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ AMenuRequest_SnapshotsTheFocusedEditorsTextAndSelection
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ ARowOfReadOnlyItems_HasNoTextEditor
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ ReplaceEditorSelection_ReplacesASelectedSpan
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ ReplaceEditorSelection_ReplacesTheSnapshottedSpan_AfterTheEditorCollapsedIts
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ ReplaceEditorSelection_TypesAtTheCaret_AndStagesNothingYet
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ RightClickingAnItemEditor_MakesItTheEditorTheMenuActsOn
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ RightClickingAnotherItem_StagesTheFirstItemsUnsavedText
FwAvaloniaTests.Detail.RetypableVectorItemTests ‑ RightClickingInsideASelection_KeepsIt_ForTheMenu
…

♻️ This comment has been updated with latest results.

@codecov-commenter

codecov-commenter commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 67.19577% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.32%. Comparing base (44b1d04) to head (d775138).

Files with missing lines Patch % Lines
...nia/Hosting/RecordEditView.ReferenceVectorMenus.cs 15.15% 25 Missing and 3 partials ⚠️
Src/Common/FwAvalonia/Detail/FwFieldControls.cs 81.13% 8 Missing and 12 partials ⚠️
...ommon/Controls/XMLViews/ReallySimpleListChooser.cs 0.00% 15 Missing and 3 partials ⚠️
.../LexText/Morphology/PhEnvStrRepresentationSlice.cs 0.00% 11 Missing and 1 partial ⚠️
...n/Controls/DetailControls/PhoneEnvReferenceView.cs 8.33% 8 Missing and 3 partials ⚠️
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs 10.00% 9 Missing ⚠️
Src/Common/FwAvalonia/Detail/DataTree.cs 82.92% 2 Missing and 5 partials ⚠️
...rks/Avalonia/Hosting/ReferenceItemMenuAuthority.cs 66.66% 2 Missing and 5 partials ⚠️
...Avalonia/Hosting/EnvironmentInsertMenuAuthority.cs 78.57% 1 Missing and 5 partials ⚠️
...c/xWorks/Avalonia/Hosting/EnvironmentMenuLeaves.cs 88.46% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1175      +/-   ##
==========================================
- Coverage   39.36%   39.32%   -0.04%     
==========================================
  Files        1529     1533       +4     
  Lines      353428   353646     +218     
  Branches    40818    40869      +51     
==========================================
- Hits       139124   139087      -37     
- Misses     185005   185225     +220     
- Partials    29299    29334      +35     
Files with missing lines Coverage Δ
Src/Common/FwAvalonia/Detail/DetailMenuFlyout.cs 95.31% <100.00%> (+0.07%) ⬆️
Src/Common/FwAvalonia/Detail/DetailModel.cs 78.95% <100.00%> (+0.39%) ⬆️
Src/FdoUi/DetailRules/EnvironmentErrors.cs 100.00% <100.00%> (ø)
Src/FdoUi/DetailRules/EnvironmentInsertRules.cs 100.00% <100.00%> (ø)
Src/xWorks/Avalonia/Hosting/ObjectMenuAuthority.cs 67.30% <100.00%> (+0.64%) ⬆️
...Avalonia/Hosting/EnvironmentInsertMenuAuthority.cs 78.57% <78.57%> (ø)
...c/xWorks/Avalonia/Hosting/EnvironmentMenuLeaves.cs 88.46% <88.46%> (ø)
Src/Common/FwAvalonia/Detail/DataTree.cs 94.81% <82.92%> (-1.08%) ⬇️
...rks/Avalonia/Hosting/ReferenceItemMenuAuthority.cs 71.69% <66.66%> (-1.56%) ⬇️
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs 60.68% <10.00%> (-0.54%) ⬇️
... and 5 more

... and 15 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 commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Right-clicking the "Add item" slot acts on the wrong editor
Src/Common/FwAvalonia/Detail/FwFieldControls.cs L1684–1701

The slot only becomes the editor the menu's inserts type into when it takes keyboard focus (newClearsSelection). A right press doesn't focus a TextBox, which the item editors handle in their press handler. The slot has no such handler, so its menu (WireBridgedMenu) captures whichever editor was current before, or none.

Repro A: the insert lands in the previous item

  1. In Lexicon Edit, use an allomorph with an environment /_#.
  2. Left-click inside it just after the / (offset 1).
  3. Without left-clicking the slot, right-click the empty Add item slot.
  4. Choose Insert Optional Item.

Expected: () is typed into the slot.
Actual: the existing item becomes /()_# with the caret between the parentheses; the slot stays empty. The request also carries the old item's SelectedItemKey, which the comment this PR replaced on the slot's focus handler warned against.

Repro B: the inserts are disabled on a fresh row

  1. Go to another entry and come back; don't click in the Environments row.
  2. Right-click the empty Add item slot.

Expected: Insert Environment slash is enabled and types into the slot, as on the WinForms empty last line.
Actual: all five inserts are disabled.

Control: left-click the slot first, then right-click it, and both repros behave correctly. That's the path the current tests take: TheSlot_IsTheEditorTheLabelMenuActsOn_WhileItHasTheCaret and RightClickingTheSlot_RaisesTheLabelMenu_WithTheSlotAsEditor_AndCommitsNothing both call slot.Focus() before the right-click.

Suggested fix

Give the slot the item editors' right-press handling, through one shared helper rather than a second copy:

// A right press does not focus a TextBox by itself, so the press focuses the editor and
// puts the caret at the pointer, as a right-click in PhoneEnvReferenceView does.
private void TakeEditorOnRightPress(TextBox box, PointerPressedEventArgs e)
{
	if (!e.GetCurrentPoint(box).Properties.IsRightButtonPressed)
		return;
	_currentEditor = box;
	if (!box.IsFocused)
		box.Focus();
	PlaceCaretAtPointer(box, e);
}
  • The item press handler calls the helper in place of its inline block.
  • The slot gets a press handler that calls ClearSelection() and then the helper, registered with handledEventsToo: true like the items' handler and removed in _teardown.
  • Setting _currentEditor and clearing the selection explicitly, rather than leaning on GotFocus, keeps the request right even if the focus call fails.
  • PlaceCaretAtPointer already keeps a selection the press lands inside.

Suggested tests (RetypableVectorItemTests)

  • Item focused, right press on the slot: the request has HasTextEditor, empty EditorText, and null SelectedItemKey; ReplaceEditorSelection("()", 1) types into the slot and leaves the item unchanged.
  • Fresh row, right press on the slot: the request has an editor at offset 0, so slash is enabled.
  • Optional, unsaved text survives: type into an item, right-click the slot, insert /, close the menu; after the row refreshes, both the item's text and the slot's / remain. Focusing the slot saves the item's text before the menu opens, so this exercises the refresh being held while text is unsaved. The same untested case exists for right-pressing item B while item A has unsaved text.

PR description

With the fix, the "No current editor: the inserts are disabled" divergence narrows to a label right-click on a row that has never been clicked into.

@thejambi

thejambi commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Describe Error judges the saved environment, not the text in the editor
Src/xWorks/Avalonia/Hosting/EnvironmentMenuLeaves.cs L108–118

BuildDescribeError enables the leaf from item.HasValidationMessage. The composer computes that from the saved environment when the row is built (ValidationMessageFor, CheckConstraints). The WinForms view judges the string currently in the view, unsaved edits included (CanShowEnvironmentError / CanGetEnvironmentStringRep).

So within one menu, the leaves read different text: the five inserts are enabled from the editor's text the request captured (EditorText), while Describe Error reads the saved text. Because this PR holds the editor's commit while the menu is open, an unsaved edit is always still in the editor when the menu appears.

Repro A: a fixed environment still reports its old error

  1. Have an allomorph environment /# (no bar, so malformed; the item shows red).
  2. Click into it and type _ at the end, so it reads /#_. Don't leave the item.
  3. Right-click the item.

Expected (WinForms): Describe Error is disabled; /#_ is well formed.
Actual: Describe Error is enabled and reports the missing-bar error for /#. Insert Environment bar is correctly disabled in the same menu, because the editor's text already has one.

Repro B: a broken environment offers no explanation

  1. Have a well-formed environment /#_.
  2. Click into it and delete the _, so it reads /#. Don't leave the item.
  3. Right-click the item.

Expected (WinForms): Describe Error is enabled and explains the missing bar.
Actual: Describe Error is disabled.

Both correct themselves once the edit is committed and the row redraws, so the impact is small, but the menu contradicts itself while it lasts.

Options

  1. Judge the captured text. Run PhonEnvRecognizer over request.EditorText, built and formatted as PhoneEnvReferenceView does (StringServices.CreateErrorMessageFromXml). This matches WinForms exactly, at the cost of the second recognizer the PR chose not to build.
  2. Disable Describe Error while the item has an unsaved edit (request.EditorText != item.Name). Cheap, never shows a stale explanation, and leaves the existing test path untouched; it diverges only by not explaining an unsaved malformed edit.
  3. Keep the behavior and add it to the PR description's divergences.

mark-sil and others added 2 commits October 6, 2026 20:38
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A right-click on the Add item slot now makes the slot the menu's editor
and focuses it, as a right-click on an item already did, so its inserts
type into the slot even when an item had focus or nothing did.

Describe Error now judges the text in the item's editor, unsaved edits
included, as the WinForms view judges the text in its view; the inserts
in the same menu already did. The check moves to FdoUi/DetailRules.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mark-sil

mark-sil commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Right-clicking the "Add item" slot acts on the wrong editor

Fixed in 59c818c, as you suggested. The item editors and the slot now share one
right-press helper, TakeEditorOnRightPress, which makes the pressed editor current,
focuses it and places the caret. The slot's press handler also clears the current
item first. It is registered with handledEventsToo and removed in _teardown, like
the items' handler.

Both repros now behave as expected. Tests added to RetypableVectorItemTests:

  • RightClickingTheSlot_WhileAnItemHasFocus_MakesTheSlotTheEditor
  • RightClickingTheSlot_OnARowNotYetClickedInto_MakesTheSlotTheEditor
  • RightClickingTheSlot_StagesAnItemsUnsavedText_AndTheInsertStaysPending
  • RightClickingAnotherItem_StagesTheFirstItemsUnsavedText

The third one checks the parts the row owns: the item's unsaved text is saved as
focus moves to the slot, and the insert leaves the slot with unsaved text of its own,
which is what holds the host's refresh. The host holding it is covered by the
existing PropChanged_WhileBusy_HoldsTheRefresh_AndReleaseDeliversItOnce.

As you suggested, the PR description now says the inserts are disabled in only one
case: a right-click on the row's label when nothing in the row has been clicked since
the record was shown.

@mark-sil

mark-sil commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Describe Error judges the saved environment, not the text in the editor

Fixed in 59c818c with option 1. Describe Error now runs the recognizer over the
editor's text, unsaved edits included, and falls back to the item's saved text when
the row has no editor. That way it judges the same text as the inserts in the same
menu.

The check is EnvironmentErrors in Src/FdoUi/DetailRules, and both WinForms
environment views now call it too. Their two formatters differed by one character:
the Environments tool's PhonEnvRecognizer.CreateErrorMessageFromXml drops the space
after the colon. The shared rule uses StringServices.CreateErrorMessageFromXml, the
one PhoneEnvReferenceView and PhEnvironment.CheckConstraints use, so the
Environments tool's message gains that space.

Both repros now behave as WinForms does. Tests:

  • DescribeError_JudgesTheEditorsUnsavedText_NotTheSavedEnvironment, a hosting test
    with the real recognizer over broken and fixed unsaved text;
  • EnvironmentErrorsTests, the rule itself;
  • DescribeError_IsEnabledForAMalformedEnvironment_WithTheExplanationWinFormsShows,
    which now also checks that the Avalonia explanation matches the allomorph view's.

@thejambi

thejambi commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Non-blocking: the second half of DescribeError_JudgesTheEditorsUnsavedText_NotTheSavedEnvironment saves /_# and puts the same /_# in the editor, so it would also have passed before the fix. Could it save a malformed /# and put the fixed /#_ in the editor, then assert Describe Error is disabled? That pins the stale-error case (Repro A) and would catch a later change that combines the saved verdict with the editor's text.

@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 reviewed 24 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on mark-sil).

@mark-sil

mark-sil commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor Author

Could it save a malformed /# and put the fixed /#_ in the editor, then assert
Describe Error is disabled?

Done in d775138. The test is now two:

  • DescribeError_IsEnabled_WhenAnUnsavedEditBreaksAWellFormedEnvironment: saved
    /_#, editor /#, enabled.
  • DescribeError_IsDisabled_WhenAnUnsavedEditFixesAMalformedEnvironment: saved /#,
    editor /#_, disabled, after asserting the saved text is flagged.

EnvironmentsFieldWithOneItem now takes the text to save, defaulting to /_#. I
checked both tests fail when Describe Error is switched back to judging the saved
text.

@github-actions

This comment has been minimized.

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

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

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mark-sil
mark-sil merged commit dc0004c into main Oct 7, 2026
8 of 9 checks passed
@mark-sil
mark-sil deleted the LT-22691l branch October 7, 2026 14:34
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