Repository navigation
Conversation
The Lexicon menu authority now answers JumpToTool, SwapLexemeWithAllomorph and ConvertLexemeForm from the row itself. The Lexeme Form row's label menu and the in-string menus of the Lexeme Form, Citation Form and variant form rows (plus the two Texts Info jump menus, owned by the same predicate) build and run without the hidden WinForms DataTree or the mediator. Both detail views share the rules: The WinForms DataTree and LexEntryMenuHandler call the same code, so the two views cannot drift. SwapLexemeWithAllomorphDlg moves to DetailControls, where xWorks can reach it, and its strings and the conversion warnings move to the resources of the projects that now own them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
NUnit Tests 1 files 1 suites 10m 32s ⏱️ Results for commit 356ebdb. |
thejambi
approved these changes
Oct 15, 2026
thejambi
left a comment
Contributor
There was a problem hiding this comment.
@thejambi partially reviewed 25 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on mark-sil).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Start here:
Src/xWorks/Avalonia/Hosting/RecordEditView.LexiconMenus.csholds the three new answerers (jump, swap, convert) and the seams they run through.The Avalonia detail view now answers the jump, swap and convert commands of the Lexeme Form row natively. Its label menu and the in-string right-click menus of the Lexeme Form, Citation Form and variant form rows build and run without the hidden WinForms DataTree or the mediator. Both views share the rules:
DetailRules/DetailJumpTargets(the jump resolver, all four tool branches) andDetailRules/LexemeFormConversion(swap, convert and the data-loss warning).DataTreeandLexEntryMenuHandlernow call them.What changes for WinForms users? Nothing visible. The WinForms handlers call the same rules and get the same answers. Two crash paths go away: Swap and Convert on an entry with no lexeme form now hide the item. The review worth your time is whether the native answers match what the adapter answered.
Where to look
DetailJumpTargets.Resolvereproduces the WinForms resolver, including its field rule: a header row has no field, and a row whose field is unknown declines a concordance jump. Pinned byDetailJumpTargetsTestsand the adapter-equivalence test over every Lexicon row that binds an owned id.mnuDataTree-TextandmnuTextInfo-Notebook.LexiconMenuAuthorityTests.PinnedOwnedIdslists all 23 owned ids.LexiconMenuExecutionTests.SwapLexemeWithAllomorphDlgmoved from LexEdDll to DetailControls so xWorks can show it. Its 3 strings and the 9 conversion warnings moved resx with the English unchanged.Deliberately different: a jump with no target is disabled, where the adapter showed it enabled and inert. The other rules kept on purpose are under Decisions.
After merge: Crowdin stores translations by file path, so the moved dialog resx and the 12 moved strings arrive as new strings. They need to be approved in Crowdin.
Verification: build and both hygiene gates clean. Targeted tests pass: xWorksTests 88, FwAvaloniaTests 41, FdoUiTests 25, DetailControlsTests 16. A 7-step manual pass in Lexicon Edit, both views, passed. The full suite was not run.
Next: approve, or name a row where a native answer differs from WinForms.
Reading this a year from now -- start here
This is stage 2, PR B of LT-22691: retiring the hidden WinForms DataTree that the Avalonia detail view kept alive to answer context-menu commands. Each PR adds answerers to
LexiconMenuAuthority, which owns a menu id once every leaf under it has an answerer; the ownership test pins the resulting list.Until this PR the Avalonia view built the hidden tree to answer these commands, because a jump needed the current slice's object and field, and swap and convert lived in the WinForms menu handler.
Decisions, and why
DataTreefound no target, no colleague answered the display query and xCore's default showed the item enabled; clicking it did nothing. A native item cannot be inert, so it says so.FollowLinkthrough one host seam,RecordEditView.FollowLink. The chooser links and the anthropology filter route through it too, so when the mediator post is replaced (by a direct call or Pub/Sub) it changes in one place. The post stays deferred for now, like every otherPostMessage("FollowLink").DetailHostControl.FocusObjectOnNextShow). The new form has a new hvo, so its row ids are unknown to the outgoing view; PR A's focus-by-row-id seam cannot name them.DataTreemust call the shared rule either way. The notebook branch keeps its one side effect: a real jump from a text without a record creates and associates the record first; the rule reports that and the caller creates it.ConfigureWritingSystemsDlg. It moved to DetailControls because xWorks cannot reference LexEdDll. The conversion warning is a confirmation seam onRecordEditView, likeDeleteConfirmation.DetailRulesreturns text, not UI. The data-loss warning text moved to FdoUiStrings so the shared rule can return it; the caller shows it.Tests
DetailJumpTargetsTests(FdoUiTests): every branch of the jump rule, including the notebook record created for a text without one.LexemeFormConversionTests(FdoUiTests): swap and convert as one undo step each, and every data-loss warning case.LexiconMenuAuthorityTests: the 23-id pin, leaf coverage over every owned id, equivalence against the adapter path on all seven Lexicon rows binding an owned id, and the no-mediator spy.LexiconMenuExecutionTests: swap (accepted and cancelled), convert (silent, warned and declined, warned and accepted), the in-string jumps, and the notebook jump creating its record once.DetailFocusMemoryTests: the re-show focusing a row by object.DetailObjectCommandExecutionTests: the ownership test now expects the Lexeme Form row's label menu to be fully owned.Manual test steps (Lexicon Edit, Avalonia view unless stated)
Preflight review details
Code Review Summary
Branch: LT-22691o
Base: main (merge base 2b981ed, 1 commit ahead)
Date: 2026-10-14
Review model: Claude Opus 5.5 (Claude Code)
Files changed: 25
Overview
Stage 2, PR B of the hidden-DataTree menu-id retirement (LT-22691). The Lexicon menu
authority now answers
JumpToTool,SwapLexemeWithAllomorphandConvertLexemeFormfrom the row, so the Lexeme Form row's label menu and the in-string menus of the Lexeme
Form, Citation Form and variant form rows build and run without the hidden WinForms
DataTree or the mediator. By the ownership predicate the two Texts Info jump menus
(
mnuDataTree-Text,mnuTextInfo-Notebook) switch over too.Both views share two new rules in
Src/FdoUi/DetailRules:DetailJumpTargets(all fourbranches of the WinForms jump resolver, which now calls it) and
LexemeFormConversion(swap, convert, data-loss warning;
LexEntryMenuHandlercalls it). The analysis found thechange complete for its purpose; the adapter-equivalence test passes on every Lexicon row
that binds an owned id.
Contract/API Changes
SwapLexemeWithAllomorphDlgmoves from LexEdDll (SIL.FieldWorks.XWorks.LexEd) toDetailControls (
SIL.FieldWorks.Common.Framework.DetailControls). Its only callers,LexEntryMenuHandlerand the Avalonia host, are updated; its resources embed under thenew name.
ksSwapLexWithAllo,ksAlternateForms,ksSwapXWithYfrom LexEdStrings to DetailControlsStrings; the nineksConvertFormLose*warnings from LexEdStrings to FdoUiStrings.
DataTree.GetGuidForJumpToToolkeeps its signature and results; its body delegates tothe shared rule.
Findings
Critical - Must address before merge
None.
Important - Should address before merge
Crowdin stores translations by source path, so the moved resx file and strings(author: after merge the new strings need to be approved inarrive as new strings
Crowdin; the PR description says so, and the author will settle who approves them)
Minor - Consider
during review:
NotebookJump_FromATextWithoutARecord_CreatesTheRecord_AndShowsIt)CaptureFollowLinkwas duplicated in two fixtures (fixed during review: sharedFollowLinkSpy.Capture)Required Validation / Evidence
.\build.ps1 -CommentHygiene -TokenHygiene: clean on the rebased branch..\test.ps1 -CommentHygiene -TokenHygiene -SkipNativewith targeted filters on theamended commit (after the in-review fixes): xWorksTests 88, FwAvaloniaTests 41,
FdoUiTests 25, DetailControlsTests 16 (1 pre-existing skip), all passing.
tests as cover for the rebase onto LT-22689: Give detail section headers a +/- box and lasting state #1184 and LT-22691: Answer the Delete leaf of the Lexicon detail menus natively #1182's review fix.
Positive Observations
shared rule checks it.
FollowLinkthrough one host seam, so replacing the mediatorpost later is a one-place change.
Undo per mutation and the dialog and warning answers.
Interview Notes
affect localization. but it looks like Crowdin will automatically reuse the existing
translations."
author's request.
In-Review Quality Check
The two test-only fixes were built with both hygiene gates and the full targeted set rerun
on the amended commit.
Suggested Review Focus
DetailJumpTargets.Resolveagainst the WinForms resolver it replaced, especially thefield rule (no field for a header row, 0 for an unknown field).
on the row's entry.
🤖 Generated with Claude Code
This change is