Repository navigation
Conversation
The Lexicon detail view now answers the item menus of allomorphs and affix processes, a variant's form row and the lexical-relation rows without the hidden DataTree. Until now their Merge, Delete Reference, Add and Replace Reference, Edit Reference Set Details, Swap and Convert leaves were built by the WinForms adapter. After such a command the view now scrolls to the row it puts the keyboard on. Until now the re-show restored the old scroll position, so the focused row could sit off-screen. The rules both views need live in Src/FdoUi/DetailRules and the WinForms slices and launchers call them; the strings they share move to FdoUiStrings. The menu ids this changes: * mnuDataTree-Allomorph * mnuDataTree-AffixProcess * mnuDataTree-VariantForm * mnuDataTree-DeleteAddLexReference * mnuDataTree-DeleteReplaceLexReference * mnuDataTree-AlternateForm (removed with its four commands: no row in either view shows it) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
else if (IsTree(mapping) && !IsFirstTarget(relation, current))
{
if (relation.TargetsRS[0] == chosen)
return null; // LT-10987: already this whole
...Otherwise, reword the comment to name the exception. |
thejambi
left a comment
thejambi
left a comment
There was a problem hiding this comment.
@thejambi reviewed 36 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on mark-sil).
Replace Reference now does nothing when the user picks the tree whole or pair partner the relation already gives the shown entry or sense. Until now the view still re-showed and moved the keyboard, though the relation did not change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agreed, fixed in 8e32a0e: |
mark-sil
left a comment
mark-sil
left a comment
There was a problem hiding this comment.
@mark-sil made 1 comment.
Reviewable status: 34 of 36 files reviewed, 1 unresolved discussion (waiting on thejambi).
a discussion (no related file):
Previously, thejambi (Zach B) wrote…
AddOrReplacereturns the relation when nothing changed (LexicalRelationEditing.cs:316,:385). The doc comment says it returns null when nothing changed. When a tree part picks the whole it already belongs to (LT-10987),ReplaceTreeWholereturnsrelationunchanged, andAddOrReplacepasses it on. The nativeAddOrReplaceReferencethen does an extra full re-show and moves focus. Nothing else goes wrong: liblcm only puts a unit of work on the undo stack when it has a data change, so no blank Undo entry is left. I'd return null before the unit of work opens and change the assertion atLexicalRelationEditingTests.cs:201toIs.Null:else if (IsTree(mapping) && !IsFirstTarget(relation, current)) { if (relation.TargetsRS[0] == chosen) return null; // LT-10987: already this whole ...Otherwise, reword the comment to name the exception.
Done.
Start here:
Src/xWorks/Avalonia/Hosting/RecordEditView.LexiconMenus.cs-- the new answerers (Merge, Delete Reference, Add/Replace Reference, Edit Reference Set Details, Swap Allomorph) at the end of the file, andCommandTargetOfaboveDeleteItem.In Lexicon Edit's Avalonia detail view, the menus of an allomorph, an affix process, a variant's Variant Form row and a lexical-relation row now display and run without the hidden WinForms DataTree. Five ids switch over (
PinnedOwnedIds48 -> 53). Three shared rules carry the logic, and the legacy slices and launchers now call them:DetailObjectMerge,VariantBackReferencesandLexicalRelationEditing.Does legacy behave differently now? No. The merge dialog, the relation choosers, Reference Set Details and the delete confirmation are the same WinForms dialogs, and the shared rules keep the LT-13729, LT-14926 and LT-10987 relation rules, each with a test.
mnuDataTree-AlternateFormand its four commands are removed: no row in either UI shows them.One change reaches beyond these menus. After a command that focuses an object (merge, convert, insert, move, media), the re-show restored the old scroll offset, so the focused row could sit off-screen.
DetailFocusMemory.RestoreAfterLayoutnow brings that row into view.Where to look
VariantFormRow_DeleteActsOnTheVariantEntry_AndDeleteReferenceNeedsItsRef,DeleteReference_OnAVariantLinkedTwice_RemovesTheRefOfTheClickedGroup.LexicalRelationEditingTests.RequestedObjectFocus_ScrollsToTheObjectsRow_InsteadOfKeepingTheOffset.FdoUiStrings(keys and English unchanged) so the FdoUi rules can use them.Deliberately not here
Verification. Merges into
main, no dependency.build.ps1 -CommentHygiene -TokenHygieneis clean. Targeted runs passed: 261 tests across the rule, menu, integrity and focus fixtures, then 66, 58 and 137 after later fixes; the full suite was not run locally. A manual pass in Sena 3 passed in both UIs, with the relation menus tested on entry Cross References rows.Next: approve, or flag a divergence below to revisit.
Reading this a year from now -- start here
Stage 2 PR D2 of LT-22691, which retires the hidden WinForms DataTree command adapter from the Avalonia detail view one context-menu id at a time. The predicate-owned
LexiconMenuAuthorityowns an id once every leaf under it has a native answerer, andLexiconMenuAuthorityTests.PinnedOwnedIdspins the exact owned list. This PR adds answerers forDataTreeMerge,DataTreeDeleteReference,DataTreeAddReference,DataTreeEdit,SwapAllomorphWithLexemeandConvertAllomorph, and the five ids they complete switch over. No working documents were kept on the branch; the reasoning is below.Deliberate rules
VariantBackReferences.CommandTargetswaps in the entry that owns the row's object when that is an entry other than the shown one. That is the rule behindSlice.GetObjectForMenusToOperateOn, which now gets it fromVariantBackReferences.BackReferencedEntry. Delete Variant deletes the variant entry; Delete Reference removes the shown entry from the variant's ref and the ref from the variant.Slice.HandleDeleteReferenceCommandtakesNextSlice.LexicalRelationEditing.DeleteKind). A collection or sequence member, and a tree's part, leave the relation; a pair, a unidirectional relation and a tree seen from its whole are deleted. The confirmation note says the other members stay related, or that the whole tree goes.CmObjectUi.MergeUnderlyingObject. Its new overload returns the survivor; the legacy overload still publishesMasterRefreshafterwards, the native path re-shows and focuses the survivor instead. Whether a native sense merge needsMasterRefresh(LT-22155) is PR E's call.LexicalRelationEditingandVariantBackReferenceslive in FdoUi, which LexEdDll and DetailControls both reference, so their 20 strings move toFdoUiStringsfromLexEdStrings(18) andDetailControlsStrings(ksUndoDeleteRef,ksRedoDeleteRef). LexEdDll reaches them throughLexicalRelationEditing's public members, sinceFdoUiStringsis internal.Avalonia vs legacy divergences
MasterRefresh; the clicked slice is goneDecisions, and why
MergeSurvivor,RelationTargetChosen,RelationDetailsEditedandRelationDeleteConfirmationreplace the dialogs in tests, asDeleteConfirmationandConversionLossAccepteddo, so every native command runs headlessly.mnuDataTree-AlternateFormrather than own it. WinForms takes a row's menu only from the slice or the part ref that places it, never from a<seq>node, and a headless compose of entries with and without allomorphs shows no Avalonia row binding it. A configuration sweep found no other menu referenced only that way.mnuDataTree-CmMedia. Its media part is not in any shipped parts file, butPartGeneratorgenerates parts at runtime, so WinForms may reach it (LT-22772).Paths not taken
DetailComposer.TryRemoveLexicalRelationTarget) atLexicalRelationEditing.Delete. A chip removal is a different gesture from these menus; left as is.ReplacePairPartnerandChooser. No row composes for one, and the legacy code never checked.DisplayParenton the tree-branches and unidirectional launchers. They take the shown object as the relation's first target, which is exact for every relation a row can show.Evidence
OwnsExactlyThePinnedIds_AndAnswersEveryLeafOfEachOneOwnedMenu_RendersWhatTheAdapterPathRendered_OnEveryLexiconRowThatBindsOneOwnedMenus_AreBuiltWithoutTheAdapterOrTheMediatorRelationRows_AreShownByTheHost_OnTheEntryAndItsSenseDetailObjectMergeTests(6)VariantBackReferencesTests(4),DeleteVariant_DeletesTheVariantEntry_WhenTheUserConfirms,DeleteReference_*LexicalRelationEditingTests(9),DeleteRelation_*,AddReference_*,ReplaceReference_*,EditReferenceDetails_*DetailRulesBoundaryTestsNineteen new tests in FdoUiTests, seventeen in xWorksTests, one in FwAvaloniaTests. Manual: a pass in Sena 3 covering allomorph merge, swap and convert, variant Delete Reference and Delete Variant, and relation Add, Replace, Edit Reference Set Details and Delete on entry Cross References rows (relations created in legacy for the pair and tree cases), plus a legacy spot-check. All passed.
Preflight review details
Code Review Summary
Branch: claude/lt-22691s-9d3fff (pushed as LT-22691s)
Base: main (merge base 173a803)
Date: 2026-10-28
Review model: Claude Opus 5.5
Files changed: 37 (36 in commit 1078490, plus one in-review fix)
Overview
Stage 2 PR D2 of LT-22691: the Lexicon detail view's predicate-owned
LexiconMenuAuthoritygains native answerers for merge (E6's merge half), variant delete-reference (E10) and the
lexical-relation commands (E12), plus Swap Allomorph with Lexeme Form and Convert Allomorph.
That completes
mnuDataTree-Allomorph,-AffixProcess,-VariantForm,-DeleteAddLexReferenceand-DeleteReplaceLexReference, which no longer build the hiddenWinForms DataTree. The rules both views need are control-free in
Src/FdoUi/DetailRules(
DetailObjectMerge,VariantBackReferences,LexicalRelationEditing) and the WinFormsslices and launchers call them; the WinForms dialogs (merge, choosers, details, delete
confirmation) are reused behind seams. The unreachable
mnuDataTree-AlternateFormand itsfour commands are removed.
The analysis found no Critical issue. A manual test exposed one defect, fixed before commit:
after a command that focuses an object the re-show restored the old scroll offset, so the
focused row could sit off-screen;
DetailFocusMemory.RestoreAfterLayoutnow brings it intoview.
Contract/API Changes
LexReferenceMultiSlice(LexEdDll): publicDeleteFromReference(ILexReference)andDeleteReference(ILexReference)replaced by oneDeleteRelation(ILexReference); all sixin-repo callers (the relation slices) updated.
CmObjectUi(FdoUi): addedConfirmDelete(ITsString note)andMergeUnderlyingObject(bool, Func<IReadOnlyList<DummyCmObject>, int>)(returns thesurvivor hvo); the existing overloads keep their behaviour.
FdoUiStrings(18 fromLexEdStrings, 2 fromDetailControlsStrings), English text unchanged.mnuDataTree-AlternateFormandCmdDataTree-{MoveUp,MoveDown,Merge,Delete}-AlternateFormremoved; the
menu=onLexEntry-Detail-AlternateForms's seq node removed.DetailFocusMemory.RestoreAfterLayout: an honoured object-focus request now wins over theremembered scroll offset.
Findings
Critical - Must address before merge
None.
Important - Should address before merge
Moved strings lose their existing translations: the 20 keys leave
(author: as after PR B; once the PR merges, the author tells Jason toLexEdStringsandDetailControlsStrings, so Crowdin translations do not follow and theFdoUiStringsentriesstart untranslated.
approve them in Crowdin)
Minor - Consider
Public LexEdDll methods replaced (
(author: fine as is; all callers in-repo, no reviewer note needed)DeleteFromReference,DeleteReference->DeleteRelation).using System.Collections.Generic;inLexReferencePairLauncher.cs(fixed during review: removed)
Relation delete confirmation now goes through(author: fine as is)CmObjectUi.ConfirmDelete, whichconsults
CanDelete(no override for a relation); the wait cursor covers only theconfirmation.
ReplacePairPartner/ChooserreadTargetsRS[0]without a count check.(author: fine as is; no row composes for an empty relation, WinForms did the same)
Composer chip removal ((author: fine as is; a different gesture, outside these menus)TryRemoveLexicalRelationTarget) keeps its own removelogic.
Tree-branches and unidirectional launchers take the shown object as
(author: fine as is; only wrong for an unreachable empty relation)TargetsRS[0].Required Validation / Evidence
./build.ps1 -CommentHygiene -TokenHygiene: clean, including after the in-review fix../test.ps1 -TestFilterruns, all green: 261 tests (the three new rule fixtures,DetailRulesBoundaryTests,LexiconMenuAuthorityTests,LexiconMenuExecutionTests,DetailObjectCommandExecutionTests,WindowConfigurationIntegrityTests,DataTreeMoveReachabilityTests,DetailMenuRequestTests,DetailObjectInsertionTests,DetailFocusMemoryTests,DetailEditorParityTests,DetailObjectDeletionTests,DetailSequenceMoveTests,MediaFileInsertionTests,LexReferenceTreeRootLauncherTests);then 66, 58 and 137 after the later fixes.
/code-review high: 5 findings, 1 fixed (tie-break test for a variant linked twice), 4skipped and confirmed here as Minor 3-6.
variant Delete Reference and Delete Variant; relation Add, Replace, Edit Reference Set
Details and Delete (with notes) on entry Cross References rows; the Legacy spot-check. All
passed after the scroll fix.
testable by hand: they never compose in the New UI (LT-22846).
Positive Observations
DetailRulesBoundaryTests; each has its owntest file, and the WinForms path now calls the same code, so the two views cannot drift.
PinnedOwnedIdsrecords exactly which menus switched over; adapter equivalence holds onevery Lexicon row binding an owned id, and the no-mediator spy covers the new messages.
LT-13729 one, each with a test.
the tests replace.
Interview Notes
question to Jason beforehand.
the row's lexeme form): the explanation was drafted by the review model at the author's
request, and the author confirmed it matches their understanding. Summary: the row shows
another entry inside the shown one; its menu speaks about that variant as a whole, and the
variant is linked through an entry ref, not owned, so the commands act on the variant entry
and the ref. This is the WinForms rule (
Slice.GetObjectForMenusToOperateOn,HandleDeleteReferenceCommand), now shared asVariantBackReferences; for a variant linkedtwice the native path takes the ref whose rows follow the clicked Form row, as
NextSlicedid.
does not publish
MasterRefresh(the sense case, LT-22155, is left to PR E); the scroll fixreaches every object-focus gesture (merge, convert, insert, move, media insert); Swap
Allomorph is disabled without a lexeme form, and Delete Reference is disabled with the
cannot-delete note when the row has no ref to the shown entry.
In-Review Quality Check
usinginLexReferencePairLauncher.cs;./build.ps1 -CommentHygiene -TokenHygieneclean afterwards.Suggested Review Focus
VariantBackReferencesand its use inRecordEditView.DeleteItem/MergeItem: the rowobject vs command target split on variant rows.
LexicalRelationEditing.DeleteKindandReplaceTreeWhole: the per-mapping, per-sidebehaviour against the WinForms slices it replaces.
DetailFocusMemory.RestoreAfterLayout: the scroll offset now yields to the focusedobject's row.
CmObjectUi.MergeUnderlyingObject(bool, chooser): the WinForms overload still publishesMasterRefresh, the native caller does not.🤖 Generated with Claude Code
This change is