Skip to content

LT-22691: Answer the main-menu inserts and View toggles natively - #1228

Merged
mark-sil merged 1 commit into
mainfrom
claude/lt-22691-main-menu-ba73fa
Nov 4, 2026
Merged

mark-sil merged 1 commit into
mainfrom
claude/lt-22691-main-menu-ba73fa

Conversation

@mark-sil

@mark-sil mark-sil commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

In the New UI, Lexicon Edit's Insert menu and pane-bar inserts and
conversions, and View > Show Hidden Fields and Show Dictionary Preview, are
answered by the detail view. Until now only the hidden DataTree answered
them, so they worked only after a right-click had built it, against the row
it last showed.

Menus, toolbars and pane bars now ask the window's command authorities
before the mediator, and the mediator never sees what one answers; a menu
raised on a row, item or header does not ask them. The detail view
registers one while it is shown. Its commands act on the row that last had
keyboard focus in the shown record, else on the shown entry.

The commands it answers:

  • CmdInsertSense, CmdInsertSubsense, CmdInsertExtNote,
    CmdDataTree-Insert-AlternateForm, CmdDataTree-Insert-Pronunciation,
    CmdDataTree-Insert-Etymology
  • CmdInsertMediaFile, CmdInsertPicture, CmdInsertVariant
  • CmdChangeToComplexForm, CmdChangeToVariant
  • the ShowHiddenFields and Show_DictionaryPubPreview toggles, in every
    New UI tool

Reviewer notes

Start here: Src/XCore/xCoreInterfaces/Choice.cs, the hook: a command or check-mark item asks the window's registered ICommandAuthority first, and goes to the mediator only when none answers. Then Src/xWorks/Avalonia/Hosting/RecordEditView.MainMenus.cs, the detail view's answers.

The question to bring: does the hook take over menus it shouldn't? It answers only when all of these hold: a New UI detail view is active in the window and has registered; the item is not in a menu raised on a target (ChoiceGroup.IsRaisedOnTarget, set by XWindow.ShowContextMenu and GetContextMenuChoiceGroup); and the authority holds an answer for that message or property. Everything else reaches the mediator as before, and the Legacy UI never registers an authority.

Where to look

  • Row menus still answer for their row: the LexiconMenuAuthorityTests equivalence tests against the WinForms baseline stay green.
  • No mediator contact and no hidden tree: the spy tests in LexiconMenuExecutionTests.MainMenu.cs.
  • The current row survives rebuilds and focus leaving the view: DetailCurrentRow, DetailCurrentRowTests.
  • The parity rules are shared, not copied: DetailObjectInsertion.CanInsert(..., commandId, ...), EntryConversion, ShownRecordCommandScope, DictionaryPreviewAvailability; DTMenuHandler, DataTree and LexEntryMenuHandler call them.
  • Unregistering on teardown and on a switch to the Legacy UI: SwitchingToTheLegacyUI_LeavesTheCommandsToTheMediator.

Deliberately not here

  • The Lexicon Edit Popup's pane-bar inserts. The pop-up shares the main window's property table and runs in the New UI only when opened from Notebook's Insert Entry, "Create and Edit"; the Notebook stage 4 PR fixes it.
  • Item menus built by GetChoiceGroupForMenu and WinForms hotlinks (GetWindowsFormsContextMenu) are not flagged as target menus. None carries a message these authorities answer; noted for stage 4.
  • The posEdit and Notebook main-menu commands come with their stage 4 PRs.
  • Insert Picture works, but no picture row shows in the New UI yet (LT-22772).

Verification: build, comment hygiene and token hygiene clean; 265 targeted tests passed, then 97 after the review fixes; 15 manual steps on Sena 3 in the New UI passed. Not covered by automated tests: the pop-up window gate and the keyboard-shortcut path (no command here has a shortcut). Full suite not run.

Next: approve, or name a menu that should not ask the authorities.

Decisions, and why
  • A hook in XCore, not new mediator receivers. On*/OnDisplay* methods on RecordEditView would have added mediator use that the Pub/Sub work must later remove. The hook is a direct interface call. Pub/Sub cannot carry it, because enabling an item is a question that needs an answer back.
  • The current row is the row of the shown record that last had keyboard focus. It stays current while focus is in the browse view, is found again after every rebuild (by row id, then by object), and is forgotten when another record is shown. Rejected: the row focused right now, which would change what is enabled with every focus move. With no current row the commands act on the shown entry, which is where WinForms' fallback to its first slice lands.
  • A variant's rows count as no current row, so entry-level inserts land on the shown entry, not on the variant.
  • Parity, not new behaviour: Insert Subsense, Picture and Extended Note need the current row's object to be a sense, as in WinForms. The right-click menus' rule, which climbs to the owning sense, was not used.
  • Focus after the two conversions goes to the new reference's Components or Variant of row. On a reference row that is the row's "..." button, as for every reference-row focus.
  • A new IsRaisedOnTarget flag rather than the existing ChoiceGroup.IsContextMenu, which means "defined under <contextMenus>" and so also covers the pane-bar menus.
  • The registry holds several authorities, most recent first, because the Lexicon Edit Popup shares the main window's property table; an authority answers only while its view is in that window.
Preflight review details

Code Review Summary

Branch: claude/lt-22691-main-menu-ba73fa
Base: main (merge base 936ae91; main has 6 newer commits that touch none of these files and merge cleanly)
Date: 2026-11-04
Review model: Claude Opus 5.5
Files changed: 29 (1 commit, 93d83ba)

Overview

LT-22691 main-menu PR (items 3a and 3b). In the New UI, Lexicon Edit's Insert menu and
pane-bar inserts and conversions, and View > Show Hidden Fields and Show Dictionary Preview,
had only the hidden DataTree as answerer; after the sense-menu PR (#1214) nothing in Lexicon
Edit builds that tree, so they were permanently disabled. The detail view now answers them
itself through a new XCore hook: menus, toolbars and pane bars ask the window's registered
command authorities before the mediator, and the mediator never sees what one answers. Menus
raised on a target (a row, item or header) are kept out of the hook. Commands act on the
"current row" (the row of the shown record that last had keyboard focus), else the shown
entry.

The four analysis passes found no Critical or Important issue. Earlier /code-review high
findings were resolved or decided with the author (below).

Contract/API Changes

  • XCore (xCoreInterfaces): new ICommandAuthority and CommandAuthorities (per-property-table
    registry, most recent first); Command.InvokeCommand(ICommandAuthority);
    CommandChoice.QueryDisplayProperties(Mediator, PropertyTable, ...);
    ChoiceGroup.IsRaisedOnTarget / IsInMenuRaisedOnTarget; ChoiceBase.CommandAuthority.
    XWindow.ShowContextMenu (both overloads) and GetContextMenuChoiceGroup flag their groups.
  • FwAvalonia: DataTree.RowFocused; DetailCurrentRow; DetailHostControl.CurrentRow;
    optional field name on DataTree.RequestObjectFocusOnRebuild and
    DetailHostControl.FocusObjectOnNextShow; DetailFocusMemory.Memento.ObjectField.
  • DetailRules: DetailObjectInsertion.CanInsert(root, row, commandId, field, out index),
    EntryConversion, ShownRecordCommandScope, DictionaryPreviewAvailability. WinForms
    DTMenuHandler, DataTree and LexEntryMenuHandler call them; behaviour unchanged except
    that an invalid root now disables Insert Variant and the conversions.
  • No configuration, resource, native or installer change.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

None.

Minor - Consider

  • Item menus built through GetChoiceGroupForMenu (FdoUiCore) and WinForms hotlinks via GetWindowsFormsContextMenu are not flagged as raised on a target, so they would ask the window's authorities (author: no such menu carries a message these authorities answer today; leave it and note it for stage 4)
  • The Lexicon Edit Popup window shares the main window's property table; its pane-bar inserts stay unanswered in the New UI (author: fix in the Notebook stage 4 PR, where Notebook's Insert Entry "Create and Edit" is the route that opens the pop-up in the New UI)
  • Three wrappers read className/restrictToTool before ShownRecordCommandScope.Applies (author: leave as is)
  • CommandAuthorities copied its list on every display query (fixed: iterates by index)
  • No test that a switch to the Legacy UI unregisters the view (fixed: SwitchingToTheLegacyUI_LeavesTheCommandsToTheMediator)

Required Validation / Evidence

  • .\build.ps1 -CommentHygiene -TokenHygiene: clean (comment and token hygiene clean).
  • .\test.ps1 -CommentHygiene -TokenHygiene -TestFilter ...: 265/265 over CommandAuthoritiesTests,
    DetailObjectInsertionTests, EntryConversionTests, ShownRecordCommandScopeTests,
    DictionaryPreviewAvailabilityTests, DetailRulesBoundaryTests, DetailCurrentRowTests,
    DetailFocusMemoryTests, LexiconMenuExecutionTests, LexiconMenuAuthorityTests,
    DetailObjectCommandExecutionTests, DetailMenuRequestTests, DetailEditorParityTests,
    WindowConfigurationIntegrityTests, DataTreeMoveReachabilityTests,
    UIFrameworkAndHostContractTests; then 97/97 after the review fixes.
  • Manual: the author ran 15 steps on Sena 3 (entry siy) in the New UI; all passed.
  • Not covered by automated tests: the pop-up window gate (IsInOwningWindow) and the
    keyboard-shortcut path through the hook (no 3a/3b command has a shortcut).
  • Full suite not run (targeted runs only).

Positive Observations

  • No new mediator receivers: the hook is a direct interface call, and the WinForms rules
    moved to Src/FdoUi/DetailRules instead of being copied.
  • No-mediator spy tests prove the Insert menu, pane bar and View toggles never reach a
    colleague, and that the hidden tree is never built.
  • Equivalence tests against the WinForms baseline stay green because target menus skip the hook.

Interview Notes

Decisions settled with the author during the session (not re-asked here):

  • "Current row" = the row of the shown record that last had keyboard focus, kept when focus
    leaves the view, re-found after rebuilds, cleared on a record change; a variant's rows count
    as none; with no current row, commands act on the shown entry.
  • Parity, not new behaviour: Insert Subsense, Picture and Extended Note need the current row's
    object to be a sense.
  • Insert Picture is answered although no picture row composes yet (LT-22772).
  • Focus after the conversions goes to the new reference's Components / Variant of row, which
    lands on that row's "..." button; the author chose to keep it there.
  • The XCore hook was chosen over new On*/OnDisplay* mediator receivers so the change does
    not expand mediator use.

Suggested Review Focus

  • Choice.cs / Command.cs / xWindow.cs: the hook only answers when an authority is registered and the item is not in a menu raised on a target.
  • RecordEditView.MainMenus.cs: registration lifecycle and MainMenuRow.
  • DataTree.RowFocused and DetailCurrentRow: how the current row survives rebuilds.

🤖 Generated with Claude Code


This change is Reviewable

In the New UI, Lexicon Edit's Insert menu and pane-bar inserts and
conversions, and View > Show Hidden Fields and Show Dictionary Preview, are
answered by the detail view. Until now only the hidden DataTree answered
them, so they worked only after a right-click had built it, against the row
it last showed.

Menus, toolbars and pane bars now ask the window's command authorities
before the mediator, and the mediator never sees what one answers; a menu
raised on a row, item or header does not ask them. The detail view
registers one while it is shown. Its commands act on the row that last had
keyboard focus in the shown record, else on the shown entry.

The commands it answers:
* CmdInsertSense, CmdInsertSubsense, CmdInsertExtNote,
  CmdDataTree-Insert-AlternateForm, CmdDataTree-Insert-Pronunciation,
  CmdDataTree-Insert-Etymology
* CmdInsertMediaFile, CmdInsertPicture, CmdInsertVariant
* CmdChangeToComplexForm, CmdChangeToVariant
* the ShowHiddenFields and Show_DictionaryPubPreview toggles, in every
  New UI tool

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

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

NUnit Tests

    1 files  ± 0      1 suites  ±0   13m 32s ⏱️ + 2m 24s
7 031 tests +30  6 946 ✅ +30  85 💤 ±0  0 ❌ ±0 
7 040 runs  +30  6 955 ✅ +30  85 💤 ±0  0 ❌ ±0 

Results for commit 93d83ba. ± Comparison against base commit 7f6f5a9.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.43750% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.48%. Comparing base (936ae91) to head (93d83ba).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...Works/Avalonia/Hosting/RecordEditView.MainMenus.cs 77.27% 10 Missing and 15 partials ⚠️
Src/xWorks/DTMenuHandler.cs 4.16% 18 Missing and 5 partials ⚠️
.../xWorks/Avalonia/Hosting/DetailCommandAuthority.cs 72.05% 7 Missing and 12 partials ⚠️
Src/Common/Controls/DetailControls/DataTree.cs 0.00% 8 Missing and 1 partial ⚠️
Src/XCore/xCoreInterfaces/CommandAuthorities.cs 83.33% 3 Missing and 4 partials ⚠️
Src/XCore/xWindow.cs 20.00% 4 Missing ⚠️
Src/Common/FwAvalonia/Detail/DetailFocusMemory.cs 66.66% 2 Missing and 1 partial ⚠️
Src/XCore/xCoreInterfaces/Command.cs 57.14% 3 Missing ⚠️
Src/Common/FwAvalonia/Detail/DataTree.cs 90.47% 0 Missing and 2 partials ⚠️
Src/LexText/Lexicon/LexEntryMenuHandler.cs 0.00% 2 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1228      +/-   ##
==========================================
+ Coverage   40.36%   40.48%   +0.12%     
==========================================
  Files        1550     1559       +9     
  Lines      355615   356086     +471     
  Branches    41476    41583     +107     
==========================================
+ Hits       143536   144173     +637     
+ Misses     182770   182589     -181     
- Partials    29309    29324      +15     
Files with missing lines Coverage Δ
Src/FdoUi/DetailRules/DetailObjectInsertion.cs 89.22% <100.00%> (+0.54%) ⬆️
...FdoUi/DetailRules/DictionaryPreviewAvailability.cs 100.00% <100.00%> (ø)
Src/FdoUi/DetailRules/EntryConversion.cs 100.00% <100.00%> (ø)
Src/FdoUi/DetailRules/ShownRecordCommandScope.cs 100.00% <100.00%> (ø)
...xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs 63.30% <100.00%> (+0.09%) ⬆️
...ks/Avalonia/Hosting/RecordEditView.LexiconMenus.cs 70.73% <100.00%> (-0.16%) ⬇️
Src/Common/FwAvalonia/Detail/DetailCurrentRow.cs 90.90% <90.90%> (ø)
Src/Common/FwAvalonia/DetailHostControl.cs 69.44% <80.00%> (+3.81%) ⬆️
Src/XCore/xCoreInterfaces/ChoiceGroup.cs 60.23% <0.00%> (-0.18%) ⬇️
Src/Common/FwAvalonia/Detail/DataTree.cs 93.10% <90.47%> (+0.04%) ⬆️
... and 10 more

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

@mark-sil
mark-sil merged commit 8afbeb5 into main Nov 4, 2026
9 checks passed
@mark-sil
mark-sil deleted the claude/lt-22691-main-menu-ba73fa branch November 4, 2026 14:14
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