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 // ----------------------------------------------------------------------------------------