Repository navigation
Conversation
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>
NUnit Tests 1 files ± 0 1 suites ±0 14m 6s ⏱️ + 1m 2s 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.♻️ This comment has been updated with latest results. |
|
Right-clicking the "Add item" slot acts on the wrong editor The slot only becomes the editor the menu's inserts type into when it takes keyboard focus ( Repro A: the insert lands in the previous item
Expected: Repro B: the inserts are disabled on a fresh row
Expected: Insert Environment slash is enabled and types into the slot, as on the WinForms empty last line. Control: left-click the slot first, then right-click it, and both repros behave correctly. That's the path the current tests take: Suggested fixGive 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);
}
Suggested tests (
|
|
Describe Error judges the saved environment, not the text in the editor
So within one menu, the leaves read different text: the five inserts are enabled from the editor's text the request captured ( Repro A: a fixed environment still reports its old error
Expected (WinForms): Describe Error is disabled; Repro B: a broken environment offers no explanation
Expected (WinForms): Describe Error is enabled and explains the missing bar. 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
|
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>
Fixed in 59c818c, as you suggested. The item editors and the slot now share one Both repros now behave as expected. Tests added to
The third one checks the parts the row owns: the item's unsaved text is saved as As you suggested, the PR description now says the inserts are disabled in only one |
Fixed in 59c818c with option 1. Describe Error now runs the recognizer over the The check is Both repros now behave as WinForms does. Tests:
|
|
Non-blocking: the second half of |
thejambi
left a comment
There was a problem hiding this comment.
@thejambi reviewed 24 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on mark-sil).
Done in d775138. The test is now two:
|
This comment has been minimized.
This comment has been minimized.
thejambi
left a comment
There was a problem hiding this comment.
@thejambi reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on mark-sil).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Start here:
Src/xWorks/Avalonia/Hosting/EnvironmentMenuLeaves.cs, the one builder behind all three menus. ThenFwReferenceVectorFieldinSrc/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 byRetypableVectorItemTests.BeginMenuGesture,EndMenuGestureandDataTree.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.SimpleRootSitedoes.Deliberately not here. Consolidating the three XML spellings of the insert commands. Converting the natural-class chooser and
PhEnvStrRepresentationSliceto 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
DataTreeshould 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
DataTreethe Avalonia detail view keeps alive to answer context-menu commands. The unit of work is one menu id, answered in full by oneIDetailMenuAuthority. Row 1 (#1172) answeredmnuReferenceChoices; earlier rows shippedmnuReorderVector(#1143), the help-topic engine (#1151), the owned-menu bridge (#1153) and the per-object and Help menus (#1161); track B answeredmnuDataTree-MultiStringSlice(#1169). The plan of record lives in gitignored working notes underDocs/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.
ReferenceItemMenuAuthorityis 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 becauseOwnsAlldecides whether the adapter is built.EnvironmentInsertMenuAuthorityis built from the request alone. The leaf logic is shared throughEnvironmentMenuLeaves, 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.
PhoneEnvReferenceViewandPhEnvStrRepresentationSlicecarried two private copies of the enablement rules, neither tested. Both now callEnvironmentInsertRules, asSlice,DataTreeandDTMenuHandlercall 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 aCustomWithParamsplugin slice. Owning it costs one id inOwnsand 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.OnMouseDowndeliberately 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.OnTreeNodeClickcallsTakeFocus;DataTree.TakeRowFocusdoes 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
EnvironmentErrorsinSrc/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
TrySetReferenceItemText. Rejected: it mints an environment per intermediate state, and needs two undo steps where WinForms needs one.Focusable = falseon 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.Surprising findings
PhoneEnvReferenceViewusedStringServices.CreateErrorMessageFromXml; the Environments tool usedPhonEnvRecognizer.CreateErrorMessageFromXml, which drops the space after the colon.EnvironmentErrorsuses the former, which is also whatPhEnvironment.CheckConstraintsproduces, so the Environments tool's message gains the space.DescribeError_IsEnabledForAMalformedEnvironment_WithTheExplanationWinFormsShowsproves the Avalonia text matches the allomorph view's.Deferred, and what would unblock it
ReallySimpleListChooser.ChooseNaturalClass. Its Avalonia replacement is a dialog conversion of its own.PhEnvStrRepresentationSliceis converted to an Avalonia text row implementingIDetailTextSelectionand the EnvironmentEdit tool is moved out ofPhase1FollowUpTools.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:EnvironmentItemAuthority_AnswersEveryLeafOfItsMenu_AndTakesTheJumpAsDefaultpopulates the realmnuEnvReferenceChoicesgroup, asserts its seven leaves each have a configuration node and build, then builds the id throughXCoreMenuBridge.EnvironmentsLabelMenu_IsFullyOwned_AndItsInsertsTypeIntoTheSlotandStringRepresentationLabelMenu_IsOwned_WithItsInsertsDisableddo the same for the two label ids, the latter composing a realPhEnvironmentwith the Environments layout.EnvironmentInsertAuthority_RejectsALeafItDoesNotAnswer.ReferenceItemMenu_RendersThePinnedTree_ForEachRowKindpins 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.EnvironmentItemMenu_IsBuiltWithoutTheAdapterOrTheMediatorand the label-menu test above assert the display spy was never asked and the hidden tree never built.ItemMenu_IsOwned_ForReferenceChoices_AndForEnvironmentsstates 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;ReplaceEditorSelectiontypes 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,
EnvironmentMenuLeavesTeststhrough 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_WhenAnUnsavedEditBreaksAWellFormedEnvironmentandDescribeError_IsDisabled_WhenAnUnsavedEditFixesAMalformedEnvironmentin 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,
EnvironmentInsertRulesTestsin FdoUiTests: 29 cases over the four rules, including no-selection, reversed selection and the recognizer's positions;EnvironmentErrorsTestsfor the error rule;DetailRulesBoundaryTestsunchanged 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.
ReferenceItemMenuAuthoritytakesmnuEnvReferenceChoices(the item menu of an environment chip) and a newEnvironmentInsertMenuAuthoritytakesmnuDataTree-Environments-InsertandmnuDataTree-StringRepresentation-Insert(label menus carrying only the five inserts).Both build their leaves through
EnvironmentMenuLeavesoverEnvironmentInsertRulesinSrc/FdoUi/DetailRules, which the two WinForms views (PhoneEnvReferenceView,PhEnvStrRepresentationSlice) now call too. With every id an item's object UI can nameowned, 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 intoDetailMenuRequest), a menugesture 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.TakeFocusdoes. Describe Error judges theeditor's text, unsaved edits included, through a shared
EnvironmentErrorsrule bothWinForms 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
IDetailTextSelection(FwAvalonia);FwReferenceVectorFieldimplements it.
DetailMenuRequestgainsHasTextEditor,EditorText,EditorSelectionAnchor,EditorSelectionEnd,ReplaceEditorSelection,BeginMenuGesture,EndMenuGesture.DetailMenuItem.Disabled(label).EnvironmentInsertRulesinSIL.FieldWorks.Common.DetailRules.ReallySimpleListChooser.ChooseNaturalClass(cache, persistence, mediator, propertyTable)returning the chosen class; the rootbox overload calls it.xWorksStrings.ksEnvironmentErrorTitle("Error in Environment"); theDetailControls copy is internal to its assembly.
IReferenceItemMenuHostextends the newIEnvironmentMenuHost;RecordEditView.BuildItemMenuThroughTheColleague,AddMoveCommandsandMoveCommandItemremoved; an item-menu id without an authority now logs and showsnothing (only two ids exist, both owned).
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;
ReplaceEditorTexttakes the span)Natural-class insert and Describe Error execution untested (fixed during
review:
EnvironmentMenuLeavesTestsdrives 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
SimpleRootSiterule)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, matchingSlice.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
PointerCaptureLostresets it)Disabled(label)duplicated across authorities (fixed during review:DetailMenuItem.Disabled)review:
WireBridgedMenufor the item editors and the slot)IDetailTextSelection(fixed during review:TextEditorStub)stated in
EndMenuGesture's doc comment; matches a WinForms context menu)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(author: nothing elsebutton for the hover affordances would be the general form
needs it yet)
A right press focuses the pressed item, committing another item's pending(author: the same path a left press has always taken; the re-show is queuedtext
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 finaltree: 161/161 across
RetypableVectorItemTests,DetailMenuRequestTests,DetailEditorParityTests,HoverRevealTests,DetailEditingTests,DetailFocus*,DetailObjectCommandExecutionTests. The previous run of the widerset, adding
EnvironmentInsertRulesTests,DetailRulesBoundaryTestsandEnvironmentMenuLeavesTests: 184/184.FwAvaloniaTestsassembly (2026-10-05, before the last four row-editor fixes):805 passed, 1 pre-existing skip.
xWorksTestsdetail-related subset: 326/326.including the WinForms twins of the shared rules and the no-editor comparison.
EnvironmentErrorsTestsandEnvironmentMenuLeavesTests; the merge alone, addingLT-22691: Answer the Writing Systems menu natively on multi-string rows #1169's writing-system fixtures: 207/207.
Describe Error on unsaved text and the Environments tool's message, all passed.
Positive Observations
through the bridge, so an unanswered leaf fails a test instead of reverting the menu.
test, and the check exists once for all three views.
Environments layout in a test, with its inserts disabled, so the day the tool flips
nothing reaches for the adapter.
private copies of them.
Interview Notes
commit message before the commit.
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.
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.
chose to judge the editor's text with the recognizer over disabling it during an edit.
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, menugesture, light dismiss.
DataTree.TakeRowFocusand the kebab's press interception.EnvironmentInsertRulesagainst the two WinForms call sites.🤖 Generated with Claude Code
This change is