From cd4573606e11aa182026bfb26bb69752b702d732 Mon Sep 17 00:00:00 2001 From: mark-sil <83427558+mark-sil@users.noreply.github.com> Date: Wed, 23 Sep 2026 10:30:39 -0400 Subject: [PATCH] LT-22691: Let a menu authority answer a whole menu id, submenus included XCoreMenuBridge populated the merged context menu through the mediator even when a native authority owned an id: ChoiceGroup asked the colleagues whether each submenu had a visible item, so the hidden DataTree adapter still took part in every owned menu that carried a submenu. mnuDataTree-Object, the next id to move across, is made of submenus. The bridge now builds one ChoiceGroup per menu id, so each id's ownership is known, and populates an owned group through the new ChoiceGroup.PopulateNow(querySubmenuVisibility: false), which keeps every submenu instead of asking the colleagues. Every leaf under an owned id, submenus included, is answered by the authority. A submenu whose leaves are all hidden is omitted, an inline choice list is spliced as on the mediator path, and a list-populated submenu under an owned id is refused with NotSupportedException, since no authority can answer one yet. Edge separators are trimmed once on the merged menu rather than per group, so the separator between a row's own menu and mnuDataTree-Object survives. The host's Field Visibility and Move Field item builders (VisibilityItem, MoveItem, ShowAllWritingSystemsItem) now take the item's label as a parameter, so the coming mnuDataTree-Object authority can call them without display properties from the mediator. Locating a row's override target is split out as TryLocateOverrideTarget, which logs why a row has no target instead of failing silently. Nothing renders differently today: the only owned id is mnuReorderVector, which has no submenus. Co-Authored-By: Claude Fable 5.1 --- Src/XCore/xCoreInterfaces/ChoiceGroup.cs | 28 +++- .../Hosting/RecordEditView.Avalonia.cs | 80 ++++++----- .../Avalonia/Hosting/XCoreMenuBridge.cs | 125 ++++++++++++------ .../DetailObjectCommandExecutionTests.cs | 120 +++++++++++++++++ 4 files changed, 277 insertions(+), 76 deletions(-) diff --git a/Src/XCore/xCoreInterfaces/ChoiceGroup.cs b/Src/XCore/xCoreInterfaces/ChoiceGroup.cs index 115c6ec360..1e8f5b3889 100644 --- a/Src/XCore/xCoreInterfaces/ChoiceGroup.cs +++ b/Src/XCore/xCoreInterfaces/ChoiceGroup.cs @@ -427,6 +427,11 @@ public string ListId } } protected override void Populate() + { + Populate(querySubmenuVisibility: true); + } + + private void Populate(bool querySubmenuVisibility) { Clear(); if (IsAListGroup) @@ -437,12 +442,12 @@ protected override void Populate() { foreach (XmlNode n in m_configurationNodes) { - Populate(n); + Populate(n, querySubmenuVisibility); } } else { - Populate(m_configurationNode); + Populate(m_configurationNode, querySubmenuVisibility); } } @@ -455,6 +460,16 @@ public void PopulateNow() Populate(); } + /// + /// Populates the group, keeping every nested submenu when + /// is false instead of asking the colleagues + /// whether one of its items is visible; the caller then decides the submenu's fate. + /// + public void PopulateNow(bool querySubmenuVisibility) + { + Populate(querySubmenuVisibility); + } + protected void PopulateFromList() { /// Just before this group is displayed, allow the group's contents to be modified by colleagues @@ -521,6 +536,11 @@ public bool HasSubGroups() } protected void Populate(XmlNode node) + { + Populate(node, querySubmenuVisibility: true); + } + + private void Populate(XmlNode node, bool querySubmenuVisibility) { Debug.Assert( node != null); XmlNodeList items = node.SelectNodes("item | menu | group"); @@ -534,11 +554,11 @@ protected void Populate(XmlNode node) break; case "menu": ChoiceGroup group = new ChoiceGroup(m_mediator, m_propertyTable, m_adapter, childNode, this); - group.Populate(childNode); + group.Populate(childNode, querySubmenuVisibility); //Only add the submenu if it contains a list of items what will be visible. //We do not want an empty submenu LT-8791. string hasList = XmlUtils.GetAttributeValue(childNode, "list"); - if (hasList != null || ASubmenuItemIsVisible(group)) + if (hasList != null || !querySubmenuVisibility || ASubmenuItemIsVisible(group)) this.Add(group); break; case "group": //for tree views in the sidebar diff --git a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs index cf448f708a..59ee74c27b 100644 --- a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs +++ b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs @@ -624,12 +624,44 @@ internal void AddOverrideCommands(OverrideCommandRegistry registry, DetailField // Show all right now never dispatches or persists: it only marks the row for the // host's transient reveal. registry.Add("CmdDataTree-WritingSystemMenu-ShowAllRightNow", - (c, d) => ShowAllWritingSystemsItem(d, field)); + (c, d) => ShowAllWritingSystemsItem(XCoreMenuBridge.StripAccelerator(d.Text), field)); - var templateId = ViewDefinitionOverrideEditor.StripRuntimeSuffix(field.StableId); - // Locate the clicked node in the field's OWN compiled model (with any current override - // already applied), so visibility checkmarks and move enablement reflect the live state. - ViewNodeLocation location = null; + // Unknown/stale target: leave the field commands on mediator dispatch rather than + // guess. + if (!TryLocateOverrideTarget(field, out var templateId, out var location)) + return; + registry.Add("CmdAlwaysVisible", + (c, d) => VisibilityItem(LabelOf(d), field, templateId, location, ViewVisibility.Always)); + registry.Add("CmdIfData", + (c, d) => VisibilityItem(LabelOf(d), field, templateId, location, ViewVisibility.IfData)); + registry.Add("CmdNormallyHidden", + (c, d) => VisibilityItem(LabelOf(d), field, templateId, location, ViewVisibility.Never)); + registry.Add("CmdDataTree-MoveFieldUp", + (c, d) => MoveItem(LabelOf(d), field, location, up: true)); + registry.Add("CmdDataTree-MoveFieldDown", + (c, d) => MoveItem(LabelOf(d), field, location, up: false)); + } + + private static string LabelOf(UIItemDisplayProperties display) + => XCoreMenuBridge.StripAccelerator(display.Text); + + /// + /// Locates the row's node in its own compiled model, with the current override applied, + /// so visibility checkmarks and move enablement reflect the live state. False, with the + /// reason logged, when the row lacks class or layout context or an override store, when + /// the compile fails, or when the model has no node for the row's template id. + /// + internal bool TryLocateOverrideTarget(DetailField field, out string templateId, + out ViewNodeLocation location) + { + templateId = null; + location = null; + if (field == null || string.IsNullOrEmpty(field.ClassName) || string.IsNullOrEmpty(field.LayoutName) + || ViewOverrideStore == null) + { + return false; + } + templateId = ViewDefinitionOverrideEditor.StripRuntimeSuffix(field.StableId); try { if (Cache.ServiceLocator.ObjectRepository.TryGetObject(field.ObjectHvo, out var fieldObj)) @@ -642,45 +674,34 @@ internal void AddOverrideCommands(OverrideCommandRegistry registry, DetailField } catch (Exception e) { - Logger.WriteError("Resolving the field's override target failed; this row's " - + "menu-button commands fall back to ordinary command dispatch.", e); - return; + Logger.WriteError("Resolving the field's override target failed; its Field Visibility " + + "and Move Field commands are not retargeted to the override layer.", e); + return false; } - - // Unknown/stale target: leave the field commands on the legacy path rather than - // guess. - if (location != null) + if (location == null) { - registry.Add("CmdAlwaysVisible", - (c, d) => VisibilityItem(d, field, templateId, location, ViewVisibility.Always)); - registry.Add("CmdIfData", - (c, d) => VisibilityItem(d, field, templateId, location, ViewVisibility.IfData)); - registry.Add("CmdNormallyHidden", - (c, d) => VisibilityItem(d, field, templateId, location, ViewVisibility.Never)); - registry.Add("CmdDataTree-MoveFieldUp", - (c, d) => MoveItem(d, field, location, up: true)); - registry.Add("CmdDataTree-MoveFieldDown", - (c, d) => MoveItem(d, field, location, up: false)); + Logger.WriteEvent(string.Format("Detail row '{0}' has no node in its compiled model; its " + + "Field Visibility and Move Field commands are not retargeted to the override layer.", + templateId)); + return false; } + return true; } // A Field Visibility menu item: checked when it is the field's current visibility, executes the // SetVisibility override mutation (idempotent -- re-choosing the current value is a // harmless write). - private DetailMenuItem VisibilityItem(UIItemDisplayProperties display, DetailField field, + private DetailMenuItem VisibilityItem(string label, DetailField field, string templateId, ViewNodeLocation location, ViewVisibility target) { - var label = XCoreMenuBridge.StripAccelerator(display.Text); var isChecked = location.Visibility == target; return new DetailMenuItem(label, isEnabled: true, isChecked: isChecked, children: null, execute: () => ApplyFieldVisibility(field, templateId, target)); } // A Move Field item: disabled at the first sibling (up) / last sibling (down) / when alone. - private DetailMenuItem MoveItem(UIItemDisplayProperties display, DetailField field, - ViewNodeLocation location, bool up) + private DetailMenuItem MoveItem(string label, DetailField field, ViewNodeLocation location, bool up) { - var label = XCoreMenuBridge.StripAccelerator(display.Text); var canMove = up ? location.CanMoveUp : location.CanMoveDown; return new DetailMenuItem(label, isEnabled: canMove, isChecked: false, children: null, execute: canMove ? (Action)(() => ApplyMoveField(field, location, up)) : null); @@ -692,9 +713,8 @@ private DetailMenuItem MoveItem(UIItemDisplayProperties display, DetailField fie /// record) and recomposes. The reveal is view state, not a command, so the item /// dispatches nothing and never writes the override. /// - private DetailMenuItem ShowAllWritingSystemsItem(UIItemDisplayProperties display, DetailField field) - => new DetailMenuItem(XCoreMenuBridge.StripAccelerator(display.Text), isEnabled: true, - isChecked: false, children: null, execute: () => + private DetailMenuItem ShowAllWritingSystemsItem(string label, DetailField field) + => new DetailMenuItem(label, isEnabled: true, isChecked: false, children: null, execute: () => { m_showAllWsFields.Add(ViewDefinitionOverrideEditor.StripRuntimeSuffix(field.StableId)); RefreshAvaloniaDetail(); diff --git a/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs b/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs index 9beebed694..ae4e118583 100644 --- a/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs +++ b/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs @@ -10,14 +10,13 @@ namespace SIL.FieldWorks.XWorks { /// - /// Converts an xCore context-menu into the neutral - /// model the Avalonia detail view renders as a native MenuFlyout. - /// Labels, enablement, checkmarks, submenus, and execution all run through the SAME xCore - /// machinery the WinForms adapter uses (GetDisplayProperties -> mediator Display* round-trip; - /// OnClick -> mediator command dispatch) -- only the rendering changes. Because this consumes - /// the - /// shared engine, it serves every DTMenuHandler-hosting tool (Grammar, Notebook, Lists, - /// Words), not just the Lexicon. + /// Converts xCore context menus into the neutral model the + /// Avalonia detail view renders as a native MenuFlyout. A menu id without a native + /// authority runs through the SAME xCore machinery the WinForms adapter uses + /// (GetDisplayProperties -> mediator Display* round-trip; OnClick -> mediator command + /// dispatch), only the rendering changes; an owned id is answered by its authority alone. + /// Because this consumes the shared engine, it serves every DTMenuHandler-hosting tool + /// (Grammar, Notebook, Lists, Words), not just the Lexicon. /// public static class XCoreMenuBridge { @@ -58,23 +57,47 @@ public static IReadOnlyList CreateMenuItems(XWindow window, stri => CreateMenuItems(window, menuIds, interceptor, temporaryColleague, null); /// - /// As the interceptor overload, plus a native that answers - /// every leaf under the menu ids it owns BEFORE the mediator is asked: those leaves get - /// no Display* round trip and no interceptor call, so nothing on the mediator (the - /// hidden DataTree adapter included) takes part in them. Leaves under other ids keep - /// the mediator path. + /// As the interceptor overload, plus a native . A menu id it + /// owns is populated without any mediator display query and every leaf under it, + /// submenus included, is answered by the authority, so nothing on the mediator (the + /// hidden DataTree adapter included) takes part in it. Other ids keep the mediator path. /// + /// An owned id contains a list-populated + /// submenu, which no authority can answer yet. public static IReadOnlyList CreateMenuItems(XWindow window, string[] menuIds, Func interceptor, IxCoreColleague temporaryColleague, IDetailMenuAuthority authority) { - var group = window?.GetContextMenuChoiceGroup(menuIds); - if (group == null) - return new List(); + var items = new List(); + if (window == null || menuIds == null) + return items; + + // One group per id keeps each id's ownership known; the source menus contribute + // their items in order, as the merged group's population did. + var groups = new List<(ChoiceGroup Group, string OwnedId)>(); + foreach (var id in menuIds) + { + if (string.IsNullOrEmpty(id)) + continue; + var group = window.GetContextMenuChoiceGroup(new[] { id }); + if (group != null) + groups.Add((group, authority != null && authority.Owns(id) ? id : null)); + } + if (groups.Count == 0) + return items; + if (temporaryColleague != null) window.Mediator.AddTemporaryColleague(temporaryColleague); - group.PopulateNow(); - return Convert(group, interceptor, authority); + foreach (var (group, ownedId) in groups) + { + // An owned group keeps its submenus regardless of what colleagues would say; + // Convert drops a submenu only when the authority hides every leaf in it. + group.PopulateNow(querySubmenuVisibility: ownedId == null); + items.AddRange(Convert(group, interceptor, authority, ownedId)); + } + + TrimSeparators(items); + return items; } /// @@ -94,26 +117,11 @@ public static bool OwnsAll(IDetailMenuAuthority authority, IEnumerable m return true; } - // The owned menu id a leaf belongs to, or null. A merged group flattens its source - // menus, so ownership comes from the nearest enclosing menu element the authority owns. - private static string OwnedMenuIdOf(ChoiceBase leaf, IDetailMenuAuthority authority) - { - if (authority == null) - return null; - for (var node = leaf.ConfigurationNode?.ParentNode; node != null; node = node.ParentNode) - { - if (node.Name != "menu") - continue; - var id = node.Attributes?["id"]?.Value; - if (!string.IsNullOrEmpty(id) && authority.Owns(id)) - return id; - } - return null; - } - + // ownedId: the menu id the authority answers for this group and its submenus, or + // null on the mediator path. Edge separators stay: they divide merged groups. private static List Convert(ChoiceGroup group, Func interceptor, - IDetailMenuAuthority authority) + IDetailMenuAuthority authority, string ownedId) { var items = new List(); foreach (var member in group) @@ -125,8 +133,14 @@ private static List Convert(ChoiceGroup group, } else if (member is ChoiceGroup submenu) { + if (ownedId != null) + { + items.AddRange(ConvertOwnedSubmenu(submenu, authority, ownedId)); + continue; + } + submenu.PopulateNow(); - var children = Convert(submenu, interceptor, authority); + var children = ConvertChildren(submenu, interceptor, authority, null); if (children.Count == 0) continue; @@ -146,11 +160,9 @@ private static List Convert(ChoiceGroup group, } else if (member is ChoiceBase choice) { - // A natively owned leaf is answered whole (hidden, or label/state/execute) - // with no mediator round trip. - var ownedId = OwnedMenuIdOf(choice, authority); if (ownedId != null) { + // The authority answers the leaf whole: hidden, or label/state/execute. var native = authority.Build(ownedId, choice); if (native != null) items.Add(WithoutExecuteWhenDisabled(native)); @@ -177,11 +189,40 @@ private static List Convert(ChoiceGroup group, display.Enabled ? (Action)(() => captured.OnClick(null, EventArgs.Empty)) : null)); } } - - TrimSeparators(items); return items; } + // A submenu's children. Hiding items can leave a separator first or last; those go. + private static List ConvertChildren(ChoiceGroup submenu, + Func interceptor, + IDetailMenuAuthority authority, string ownedId) + { + var children = Convert(submenu, interceptor, authority, ownedId); + TrimSeparators(children); + return children; + } + + // An owned submenu takes its label from the configuration and its children from the + // authority. Omitted when no child is visible, spliced when inline. A list submenu is + // refused, not left to the mediator. + private static IEnumerable ConvertOwnedSubmenu(ChoiceGroup submenu, + IDetailMenuAuthority authority, string ownedId) + { + if (!string.IsNullOrEmpty(submenu.ListId)) + { + throw new NotSupportedException(string.Format( + "Menu '{0}' has a list-populated submenu '{1}' that no native authority can answer yet.", + ownedId, submenu.ListId)); + } + var children = ConvertChildren(submenu, null, authority, ownedId); + if (children.Count == 0 || submenu.IsInlineChoiceList) + return children; + return new[] + { + new DetailMenuItem(StripAccelerator(submenu.Label), isEnabled: true, isChecked: false, children) + }; + } + // A disabled leaf carries no execute action, so "Execute != null" means invokable for // every consumer -- programmatic invokers included, not just the pointer UI. private static DetailMenuItem WithoutExecuteWhenDisabled(DetailMenuItem item) diff --git a/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs b/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs index f91c54469a..842218691d 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs @@ -360,6 +360,26 @@ public void ReorderVectorLabelMenu_NativeAuthority_RendersTheSameTreeAsTheAdapte Assert.That(adapter, Does.Contain("Alphabetical Order [enabled=True"), "reorder='true': Alphabetical Order offered and enabled"); } + // Both sides of the equivalence tests render through the bridge, so a rendering change + // they share passes them; this pins the merge itself. + [Test] + public void ReorderVectorLabelMenu_KeepsTheSeparator_BetweenTheReorderMenu_AndTheObjectMenu() + { + MakeTwoSubentries(); + var field = SubentriesField(); + var ids = LabelMenuIds(field); + EnsureAdapter(field.ObjectHvo, field.Field); + + var items = BuildItems(ids, m_view.CreateReorderVectorAuthority(LabelMenuRequest(field, NoItem))).ToList(); + + var alphabetical = items.FindIndex(i => i.Label == "Alphabetical Order"); + var visibility = items.FindIndex(i => i.Label == "Field Visibility"); + Assert.That(alphabetical, Is.GreaterThanOrEqualTo(0), "the row's own menu ends with Alphabetical Order"); + Assert.That(visibility, Is.EqualTo(alphabetical + 2), "exactly one item lies between the two source menus"); + Assert.That(items[alphabetical + 1].IsSeparator, Is.True, + "mnuDataTree-Object's leading separator divides it from the row's own menu"); + } + [Test] public void ReorderVectorLabelMenu_ReadOnlyComplexFormsRow_RendersTheSameTreeAsTheAdapter() { @@ -521,6 +541,106 @@ public void AlphabeticalOrder_ThroughTheHost_DiscardsTheVirtualOrdering() "the reset is its own undo step"); } + // The bridge's owned-menu path: an owned id is populated and converted without the + // mediator, submenus included. + + // Answers every leaf of the ids it owns with the leaf's own label, hiding the command + // ids it is told to hide. + private sealed class EchoAuthority : IDetailMenuAuthority + { + private readonly HashSet _owned; + private readonly HashSet _hidden; + public EchoAuthority(IEnumerable owned, params string[] hidden) + { + _owned = new HashSet(owned, StringComparer.Ordinal); + _hidden = new HashSet(hidden, StringComparer.Ordinal); + } + public bool Owns(string menuId) => _owned.Contains(menuId); + public DetailMenuItem Build(string menuId, ChoiceBase leaf) + => _hidden.Contains(leaf.HelpId) ? null + : new DetailMenuItem(XCoreMenuBridge.StripAccelerator(leaf.Label), isEnabled: true); + } + + // Records whether the mediator asked anyone to display the Always-visible command. + private sealed class DisplaySpyColleague : IxCoreColleague + { + public bool Asked { get; private set; } + public void Init(Mediator mediator, PropertyTable propertyTable, XmlNode configurationParameters) { } + public IxCoreColleague[] GetMessageTargets() => new IxCoreColleague[] { this }; + public bool ShouldNotCall => false; + public int Priority => (int)ColleaguePriority.High; + public bool OnDisplayShowFieldAlwaysVisible(object commandObject, ref UIItemDisplayProperties display) + { + Asked = true; + return false; + } + } + + private IReadOnlyList BuildWithSpy(string[] ids, IDetailMenuAuthority authority, + out bool mediatorAsked) + { + var window = m_propertyTable.GetValue("window"); + var spy = new DisplaySpyColleague(); + window.Mediator.AddColleague(spy); + try + { + var items = XCoreMenuBridge.CreateMenuItems(window, ids, null, null, authority); + mediatorAsked = spy.Asked; + return items; + } + finally + { + window.Mediator.RemoveColleague(spy); + } + } + + [Test] + public void OwnedMenu_WithSubmenus_IsBuiltWithoutAskingTheMediator() + { + var ids = new[] { RecordEditView.ObjectMenuId }; + // No EnsureAdapter: the hidden tree never exists. + + var items = BuildWithSpy(ids, new EchoAuthority(ids), out var asked); + + Assert.That(asked, Is.False, "an owned id never reaches the mediator, submenu leaves included"); + var visibility = FindItem(items, "Field Visibility"); + var move = FindItem(items, "Move Field"); + Assert.That(visibility, Is.Not.Null, "the Field Visibility submenu is built from its configuration"); + Assert.That(visibility.Children.Select(c => c.Label), + Is.EqualTo(new[] { "Always visible", "Normally hidden, unless non-empty", "Normally hidden" })); + Assert.That(move?.Children.Select(c => c.Label), Is.EqualTo(new[] { "Move Up", "Move Down" })); + Assert.That(FindItem(items, "Help..."), Is.Not.Null); + Assert.That(items[0].IsSeparator, Is.False, "the menu's leading separator is trimmed"); + } + + [Test] + public void UnownedMenu_StillAsksTheMediator_ForSubmenuLeaves() + { + BuildWithSpy(new[] { RecordEditView.ObjectMenuId }, null, out var asked); + Assert.That(asked, Is.True, "the mediator path decides submenu visibility by asking colleagues"); + } + + [Test] + public void OwnedSubmenu_WhoseLeavesAreAllHidden_IsOmitted() + { + var ids = new[] { RecordEditView.ObjectMenuId }; + var authority = new EchoAuthority(ids, "CmdAlwaysVisible", "CmdIfData", "CmdNormallyHidden"); + + var items = BuildWithSpy(ids, authority, out _); + + Assert.That(FindItem(items, "Field Visibility"), Is.Null, "a submenu with no visible leaf is dropped"); + Assert.That(FindItem(items, "Move Field"), Is.Not.Null); + } + + [Test] + public void OwnedMenu_WithAListSubmenu_IsRefused() + { + var ids = new[] { RecordEditView.MultiStringSliceMenuId }; + Assert.That(() => BuildWithSpy(ids, new EchoAuthority(ids), out _), + Throws.TypeOf().With.Message.Contains("WritingSystemOptionsForSlice"), + "a list-populated submenu has no configured leaves an authority could answer"); + } + // ---------------------------------------------------------------------------------------- // Delete Sense / Delete object // ----------------------------------------------------------------------------------------