From 5affdb1453425c2871bbe8b9aaf027ea1bc7332b Mon Sep 17 00:00:00 2001 From: mark-sil <83427558+mark-sil@users.noreply.github.com> Date: Thu, 8 Oct 2026 10:06:51 -0400 Subject: [PATCH 1/2] LT-22691: Build owned detail menus from the menu XML, not ChoiceGroup Groundwork for the Lexicon menu PRs that follow: each of them will add answerers to the authority introduced here and switch over the menus those answerers complete, so this change defines the framework they will build on. The bridge now resolves an owned menu id through the XWindow's loaded configuration and walks the menu XML itself. Until now an owned id was still populated through xCore's ChoiceGroup, which kept every authority coupled to ChoiceBase (and so to a live XWindow) and tied the hidden-adapter retirement to xCore's population rules. The earlier authorities never needed a menu definition of their own: each owns a fixed list of menu ids and only answers a leaf it is handed. The new LexiconMenuAuthority owns by predicate instead, deciding each id when asked, from the menu's contents: every leaf's message must have an answerer, and the menu may carry neither a list submenu nor an item no authority can answer. That decision has to be made before any menu is built and from the same tree the bridge then renders. Co-Authored-By: Claude Fable 5.1 --- Src/XCore/xCoreInterfaces/ChoiceGroup.cs | 28 +-- .../Hosting/CompositeMenuAuthority.cs | 3 +- .../Avalonia/Hosting/DetailMenuDefinition.cs | 126 ++++++++++ Src/xWorks/Avalonia/Hosting/DetailMenuLeaf.cs | 38 +++ .../Hosting/EnvironmentInsertMenuAuthority.cs | 9 +- .../Avalonia/Hosting/IDetailMenuAuthority.cs | 6 +- .../Avalonia/Hosting/LexiconMenuAuthority.cs | 87 +++++++ .../Hosting/MultiStringMenuAuthority.cs | 5 +- .../Avalonia/Hosting/ObjectMenuAuthority.cs | 7 +- .../Hosting/RecordEditView.Avalonia.cs | 26 +- .../Hosting/ReferenceItemMenuAuthority.cs | 44 ++-- .../Hosting/ReorderVectorMenuAuthority.cs | 7 +- .../Avalonia/Hosting/XCoreMenuBridge.cs | 222 ++++++++++++----- .../DetailObjectCommandExecutionTests.cs | 233 +++++++++++------- .../Hosting/LexiconMenuAuthorityTests.cs | 194 +++++++++++++++ 15 files changed, 819 insertions(+), 216 deletions(-) create mode 100644 Src/xWorks/Avalonia/Hosting/DetailMenuDefinition.cs create mode 100644 Src/xWorks/Avalonia/Hosting/DetailMenuLeaf.cs create mode 100644 Src/xWorks/Avalonia/Hosting/LexiconMenuAuthority.cs create mode 100644 Src/xWorks/xWorksTests/Avalonia/Hosting/LexiconMenuAuthorityTests.cs diff --git a/Src/XCore/xCoreInterfaces/ChoiceGroup.cs b/Src/XCore/xCoreInterfaces/ChoiceGroup.cs index 1e8f5b3889..115c6ec360 100644 --- a/Src/XCore/xCoreInterfaces/ChoiceGroup.cs +++ b/Src/XCore/xCoreInterfaces/ChoiceGroup.cs @@ -427,11 +427,6 @@ public string ListId } } protected override void Populate() - { - Populate(querySubmenuVisibility: true); - } - - private void Populate(bool querySubmenuVisibility) { Clear(); if (IsAListGroup) @@ -442,12 +437,12 @@ private void Populate(bool querySubmenuVisibility) { foreach (XmlNode n in m_configurationNodes) { - Populate(n, querySubmenuVisibility); + Populate(n); } } else { - Populate(m_configurationNode, querySubmenuVisibility); + Populate(m_configurationNode); } } @@ -460,16 +455,6 @@ 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 @@ -536,11 +521,6 @@ 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"); @@ -554,11 +534,11 @@ private void Populate(XmlNode node, bool querySubmenuVisibility) break; case "menu": ChoiceGroup group = new ChoiceGroup(m_mediator, m_propertyTable, m_adapter, childNode, this); - group.Populate(childNode, querySubmenuVisibility); + group.Populate(childNode); //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 || !querySubmenuVisibility || ASubmenuItemIsVisible(group)) + if (hasList != null || ASubmenuItemIsVisible(group)) this.Add(group); break; case "group": //for tree views in the sidebar diff --git a/Src/xWorks/Avalonia/Hosting/CompositeMenuAuthority.cs b/Src/xWorks/Avalonia/Hosting/CompositeMenuAuthority.cs index d8246fab2d..4951b937b1 100644 --- a/Src/xWorks/Avalonia/Hosting/CompositeMenuAuthority.cs +++ b/Src/xWorks/Avalonia/Hosting/CompositeMenuAuthority.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Generic; using SIL.FieldWorks.Common.FwAvalonia.Detail; -using XCore; namespace SIL.FieldWorks.XWorks { @@ -25,7 +24,7 @@ public CompositeMenuAuthority(params IDetailMenuAuthority[] members) public bool Owns(string menuId) => Owner(menuId) != null; - public DetailMenuItem Build(string menuId, ChoiceBase leaf) + public DetailMenuItem Build(string menuId, DetailMenuLeaf leaf) => Owning(menuId).Build(menuId, leaf); public IReadOnlyList BuildList(string menuId, string listId) diff --git a/Src/xWorks/Avalonia/Hosting/DetailMenuDefinition.cs b/Src/xWorks/Avalonia/Hosting/DetailMenuDefinition.cs new file mode 100644 index 0000000000..6380371b4a --- /dev/null +++ b/Src/xWorks/Avalonia/Hosting/DetailMenuDefinition.cs @@ -0,0 +1,126 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) + +using System; +using System.Collections.Generic; + +namespace SIL.FieldWorks.XWorks +{ + /// + /// The item tree of one context-menu id as its configuration defines it: separators, + /// command leaves, submenus (a list-populated one carrying only its list id), and the + /// items no authority can answer. Built by ; an + /// owned menu is rendered from it and an authority decides ownership from it. + /// + public sealed class DetailMenuDefinition + { + private readonly List _leaves = new List(); + private readonly List _listIds = new List(); + private readonly List _unanswerable = new List(); + + public DetailMenuDefinition(string menuId, IReadOnlyList entries) + { + MenuId = menuId ?? throw new ArgumentNullException(nameof(menuId)); + Entries = entries ?? throw new ArgumentNullException(nameof(entries)); + Collect(entries); + } + + /// The menu id the definition was resolved from. + public string MenuId { get; } + + /// The top-level entries in configuration order. + public IReadOnlyList Entries { get; } + + /// Every command leaf, submenus included, in configuration order. + public IReadOnlyList Leaves => _leaves; + + /// The list ids of the list-populated submenus, submenus included. + public IReadOnlyList ListIds => _listIds; + + /// + /// Why each item no authority can answer cannot be answered, submenus included: a + /// property toggle belongs to the property table, and an undefined command has no + /// message. A menu with any such item is never owned. + /// + public IReadOnlyList Unanswerable => _unanswerable; + + private void Collect(IReadOnlyList entries) + { + foreach (var entry in entries) + { + if (entry.Leaf != null) + _leaves.Add(entry.Leaf); + else if (entry.Unanswerable != null) + _unanswerable.Add(entry.Unanswerable); + else if (entry.ListId != null) + _listIds.Add(entry.ListId); + else if (entry.Children != null) + Collect(entry.Children); + } + } + } + + /// + /// One entry of a : a separator, a command leaf, an + /// item no authority can answer, or a submenu whose children are configured items or, for + /// a list-populated one, come from the authority at build time. + /// + public sealed class DetailMenuEntry + { + private DetailMenuEntry(DetailMenuLeaf leaf, string label, string unanswerable, string listId, + bool isInline, IReadOnlyList children) + { + Leaf = leaf; + Label = label ?? string.Empty; + Unanswerable = unanswerable; + ListId = listId; + IsInline = isInline; + Children = children; + } + + public static DetailMenuEntry Separator() => new DetailMenuEntry(null, null, null, null, false, null); + + public static DetailMenuEntry ForLeaf(DetailMenuLeaf leaf) + => new DetailMenuEntry(leaf ?? throw new ArgumentNullException(nameof(leaf)), null, null, null, false, null); + + /// The item's localized label with its accelerator marker. + /// Why no authority can answer the item. + public static DetailMenuEntry ForUnanswerable(string label, string reason) + => new DetailMenuEntry(null, label, reason ?? throw new ArgumentNullException(nameof(reason)), + null, false, null); + + /// The submenu's localized label with its accelerator marker. + /// Whether its items splice into the parent instead of + /// nesting. + public static DetailMenuEntry ForSubmenu(string label, bool isInline, IReadOnlyList children) + => new DetailMenuEntry(null, label, null, null, isInline, + children ?? throw new ArgumentNullException(nameof(children))); + + /// The list attribute naming the items' source. + public static DetailMenuEntry ForList(string label, bool isInline, string listId) + => new DetailMenuEntry(null, label, null, listId ?? throw new ArgumentNullException(nameof(listId)), + isInline, null); + + public bool IsSeparator => Leaf == null && Unanswerable == null && Children == null && ListId == null; + + /// The command leaf, or null for any other entry. + public DetailMenuLeaf Leaf { get; } + + /// A submenu's or unanswerable item's localized label; empty + /// otherwise. + public string Label { get; } + + /// Why no authority can answer this item; null for any other entry. + public string Unanswerable { get; } + + /// A list-populated submenu's list id; null otherwise. + public string ListId { get; } + + /// Whether a submenu's items splice into the parent menu. + public bool IsInline { get; } + + /// A configured submenu's entries; null for a leaf, separator or list. + public IReadOnlyList Children { get; } + } +} diff --git a/Src/xWorks/Avalonia/Hosting/DetailMenuLeaf.cs b/Src/xWorks/Avalonia/Hosting/DetailMenuLeaf.cs new file mode 100644 index 0000000000..720957fdba --- /dev/null +++ b/Src/xWorks/Avalonia/Hosting/DetailMenuLeaf.cs @@ -0,0 +1,38 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) + +using System; +using XCore; + +namespace SIL.FieldWorks.XWorks +{ + /// + /// One command leaf of a context menu as its configuration defines it: the resolved + /// and the menu text. Built by the bridge's walk of the menu XML, + /// so no xCore choice object and no mediator query stands behind it. + /// + public sealed class DetailMenuLeaf + { + /// The command the item's command attribute names. + /// The localized menu text, the item's own label when it has one, + /// else the command's; the accelerator marker is kept. + public DetailMenuLeaf(Command command, string label) + { + Command = command ?? throw new ArgumentNullException(nameof(command)); + Label = label ?? string.Empty; + } + + /// The resolved command, for answers that call its target's object UI. + public Command Command { get; } + + /// The command id, e.g. CmdDataTree-Help. + public string CommandId => Command.Id; + + /// The message the command sends, e.g. DataTreeDelete. + public string Message => Command.Message; + + /// The localized menu text with its accelerator marker. + public string Label { get; } + } +} diff --git a/Src/xWorks/Avalonia/Hosting/EnvironmentInsertMenuAuthority.cs b/Src/xWorks/Avalonia/Hosting/EnvironmentInsertMenuAuthority.cs index bb7af51e12..beaa5dd0c2 100644 --- a/Src/xWorks/Avalonia/Hosting/EnvironmentInsertMenuAuthority.cs +++ b/Src/xWorks/Avalonia/Hosting/EnvironmentInsertMenuAuthority.cs @@ -6,7 +6,6 @@ using System.Collections.Generic; using SIL.FieldWorks.Common.FwAvalonia.Detail; using SIL.Reporting; -using XCore; namespace SIL.FieldWorks.XWorks { @@ -38,17 +37,17 @@ public bool Owns(string menuId) => string.Equals(menuId, EnvironmentsMenuId, StringComparison.Ordinal) || string.Equals(menuId, StringRepresentationMenuId, StringComparison.Ordinal); - public DetailMenuItem Build(string menuId, ChoiceBase leaf) + public DetailMenuItem Build(string menuId, DetailMenuLeaf leaf) { if (leaf == null) throw new ArgumentNullException(nameof(leaf)); - if (!(leaf is CommandChoice command) || !EnvironmentMenuLeaves.IsInsertMessage(command.Message)) + if (!EnvironmentMenuLeaves.IsInsertMessage(leaf.Message)) { throw new InvalidOperationException(string.Format( - "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.HelpId)); + "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.CommandId)); } LogUnsupportedRowOnce(); - return EnvironmentMenuLeaves.BuildInsert(command.Message, XCoreMenuBridge.StripAccelerator(leaf.Label), + return EnvironmentMenuLeaves.BuildInsert(leaf.Message, XCoreMenuBridge.StripAccelerator(leaf.Label), _request, _host); } diff --git a/Src/xWorks/Avalonia/Hosting/IDetailMenuAuthority.cs b/Src/xWorks/Avalonia/Hosting/IDetailMenuAuthority.cs index 253c48b6b1..cffadd4134 100644 --- a/Src/xWorks/Avalonia/Hosting/IDetailMenuAuthority.cs +++ b/Src/xWorks/Avalonia/Hosting/IDetailMenuAuthority.cs @@ -4,7 +4,6 @@ using System.Collections.Generic; using SIL.FieldWorks.Common.FwAvalonia.Detail; -using XCore; namespace SIL.FieldWorks.XWorks { @@ -28,12 +27,11 @@ public interface IDetailMenuAuthority /// The rendered item for a leaf under an owned menu id, or null when the leaf is hidden. /// /// The owned menu id the leaf belongs to. - /// The xCore leaf; its label is the menu text, its - /// the command id. + /// The configured leaf: its command, message and menu text. /// The leaf's command is not one the /// authority answers: an owned id must be answered in full, or a leaf would leak as /// visible-but-disabled. - DetailMenuItem Build(string menuId, ChoiceBase leaf); + DetailMenuItem Build(string menuId, DetailMenuLeaf leaf); /// /// The items of a list-populated submenu under an owned menu id, in display order, each diff --git a/Src/xWorks/Avalonia/Hosting/LexiconMenuAuthority.cs b/Src/xWorks/Avalonia/Hosting/LexiconMenuAuthority.cs new file mode 100644 index 0000000000..3e1fe57e41 --- /dev/null +++ b/Src/xWorks/Avalonia/Hosting/LexiconMenuAuthority.cs @@ -0,0 +1,87 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) + +using System; +using System.Collections.Generic; +using SIL.FieldWorks.Common.FwAvalonia.Detail; + +namespace SIL.FieldWorks.XWorks +{ + /// + /// The native authority for the Lexicon detail menus, answered by the message each + /// command sends (Insert, Delete, Move, Merge and the rest) and parameterized by the + /// command's configuration. It owns a menu id by predicate: when it has an answerer for + /// the message of every leaf under the id and the id carries neither a list-populated + /// submenu nor an item no authority can answer. Adding an answerer therefore switches + /// over every id it completes, and the ownership test pins the resulting list. + /// + internal sealed class LexiconMenuAuthority : IDetailMenuAuthority + { + private readonly Func _resolve; + private readonly Dictionary _resolved + = new Dictionary(StringComparer.Ordinal); + private readonly Dictionary> _answerers + = new Dictionary>(StringComparer.Ordinal); + + /// Resolves a menu id to its configured item tree. + public LexiconMenuAuthority(Func resolve) + { + _resolve = resolve ?? throw new ArgumentNullException(nameof(resolve)); + } + + /// The messages this authority answers, for the ownership test. + internal IEnumerable AnsweredMessages => _answerers.Keys; + + /// + /// Registers the answerer for every leaf whose command sends ; + /// the ids it completes become owned. One answerer per message. + /// + internal void Answer(string message, Func answerer) + { + if (string.IsNullOrEmpty(message)) + throw new ArgumentException("A message is required.", nameof(message)); + _answerers.Add(message, answerer ?? throw new ArgumentNullException(nameof(answerer))); + } + + public bool Owns(string menuId) + { + var menu = Resolve(menuId); + if (menu.Leaves.Count == 0 || menu.ListIds.Count > 0 || menu.Unanswerable.Count > 0) + return false; + foreach (var leaf in menu.Leaves) + { + if (!_answerers.ContainsKey(leaf.Message)) + return false; + } + return true; + } + + public DetailMenuItem Build(string menuId, DetailMenuLeaf leaf) + { + if (leaf == null) + throw new ArgumentNullException(nameof(leaf)); + if (_answerers.TryGetValue(leaf.Message, out var answer)) + return answer(leaf); + throw new InvalidOperationException(string.Format( + "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.CommandId)); + } + + // An id with a list-populated submenu is never owned. + public IReadOnlyList BuildList(string menuId, string listId) + => throw new InvalidOperationException(string.Format( + "Menu '{0}' has a list submenu '{1}' this authority does not answer.", menuId, listId)); + + // Resolved once per id for the authority's lifetime, one menu request. + private DetailMenuDefinition Resolve(string menuId) + { + if (!_resolved.TryGetValue(menuId, out var menu)) + { + menu = _resolve(menuId) ?? throw new InvalidOperationException(string.Format( + "Menu '{0}' did not resolve.", menuId)); + _resolved.Add(menuId, menu); + } + return menu; + } + } +} diff --git a/Src/xWorks/Avalonia/Hosting/MultiStringMenuAuthority.cs b/Src/xWorks/Avalonia/Hosting/MultiStringMenuAuthority.cs index 24172201d4..2fa814e871 100644 --- a/Src/xWorks/Avalonia/Hosting/MultiStringMenuAuthority.cs +++ b/Src/xWorks/Avalonia/Hosting/MultiStringMenuAuthority.cs @@ -7,7 +7,6 @@ using System.Linq; using SIL.FieldWorks.Common.DetailRules; using SIL.FieldWorks.Common.FwAvalonia.Detail; -using XCore; namespace SIL.FieldWorks.XWorks { @@ -56,12 +55,12 @@ public MultiStringMenuAuthority(DetailField field, IDetailMenuAuthority sharedLe public bool Owns(string menuId) => _field.IsMultiStringRow && string.Equals(menuId, MenuId, StringComparison.Ordinal); - public DetailMenuItem Build(string menuId, ChoiceBase leaf) + public DetailMenuItem Build(string menuId, DetailMenuLeaf leaf) { if (leaf == null) throw new ArgumentNullException(nameof(leaf)); var label = XCoreMenuBridge.StripAccelerator(leaf.Label); - switch (leaf.HelpId) + switch (leaf.CommandId) { case ShowAllCommandId: return _showAll(label, _field); diff --git a/Src/xWorks/Avalonia/Hosting/ObjectMenuAuthority.cs b/Src/xWorks/Avalonia/Hosting/ObjectMenuAuthority.cs index abaec2d939..5e444e124c 100644 --- a/Src/xWorks/Avalonia/Hosting/ObjectMenuAuthority.cs +++ b/Src/xWorks/Avalonia/Hosting/ObjectMenuAuthority.cs @@ -6,7 +6,6 @@ using System.Collections.Generic; using SIL.FieldWorks.Common.FwAvalonia.Detail; using SIL.FieldWorks.Common.FwAvalonia.ViewDefinition; -using XCore; namespace SIL.FieldWorks.XWorks { @@ -65,12 +64,12 @@ public bool Owns(string menuId) => string.Equals(menuId, MenuId, StringComparison.Ordinal) || string.Equals(menuId, HelpMenuId, StringComparison.Ordinal); - public DetailMenuItem Build(string menuId, ChoiceBase leaf) + public DetailMenuItem Build(string menuId, DetailMenuLeaf leaf) { if (leaf == null) throw new ArgumentNullException(nameof(leaf)); var label = XCoreMenuBridge.StripAccelerator(leaf.Label); - switch (leaf.HelpId) + switch (leaf.CommandId) { case AlwaysVisibleCommandId: return VisibilityItem(label, ViewVisibility.Always); @@ -86,7 +85,7 @@ public DetailMenuItem Build(string menuId, ChoiceBase leaf) return HelpItem(label); default: throw new InvalidOperationException(string.Format( - "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.HelpId)); + "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.CommandId)); } } diff --git a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs index 2632b4fb57..dbf01a0b72 100644 --- a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs +++ b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs @@ -737,18 +737,30 @@ private static string LabelOf(UIItemDisplayProperties display) => XCoreMenuBridge.StripAccelerator(display.Text); /// - /// The native authorities for the request's row: the reorder-vector menu and the shared - /// per-object and Help menus, so a label menu made only of those ids needs nothing from - /// the hidden command adapter. + /// The native authorities for the request's row, asked in order: the four that own + /// their menu ids outright, then the Lexicon authority that owns by predicate. A label + /// menu made only of owned ids needs nothing from the hidden command adapter. /// internal IDetailMenuAuthority CreateMenuAuthority(DetailMenuRequest request) { - // One per-object authority serves both, so the row's override target is located - // once per menu, not once per authority that asks. + // One per-object authority serves both the multi-string menu and the composite, so + // the row's override target is located once per menu, not once per authority. var objectMenu = CreateObjectMenuAuthority(request.Field); - return new CompositeMenuAuthority(CreateReorderVectorAuthority(request), + return new CompositeMenuAuthority( + CreateReorderVectorAuthority(request), new EnvironmentInsertMenuAuthority(request, this), - CreateMultiStringMenuAuthority(request.Field, objectMenu), objectMenu); + CreateMultiStringMenuAuthority(request.Field, objectMenu), + objectMenu, + CreateLexiconMenuAuthority()); + } + + /// + /// The message-keyed authority for the Lexicon menus, owning by predicate. + /// + internal LexiconMenuAuthority CreateLexiconMenuAuthority() + { + var window = m_propertyTable.GetValue("window"); + return new LexiconMenuAuthority(id => XCoreMenuBridge.ResolveMenu(window, id)); } /// The native authority for a multi-writing-system row's label menu. diff --git a/Src/xWorks/Avalonia/Hosting/ReferenceItemMenuAuthority.cs b/Src/xWorks/Avalonia/Hosting/ReferenceItemMenuAuthority.cs index 12ea6600fb..e68c8cae2e 100644 --- a/Src/xWorks/Avalonia/Hosting/ReferenceItemMenuAuthority.cs +++ b/Src/xWorks/Avalonia/Hosting/ReferenceItemMenuAuthority.cs @@ -102,40 +102,33 @@ public bool Owns(string menuId) => string.Equals(menuId, MenuId, StringComparison.Ordinal) || string.Equals(menuId, EnvironmentMenuId, StringComparison.Ordinal); - public DetailMenuItem Build(string menuId, ChoiceBase leaf) + public DetailMenuItem Build(string menuId, DetailMenuLeaf leaf) { if (leaf == null) throw new ArgumentNullException(nameof(leaf)); var label = XCoreMenuBridge.StripAccelerator(leaf.Label); - var command = leaf as CommandChoice; - switch (leaf.HelpId) + switch (leaf.CommandId) { case ReorderVectorMenuAuthority.MoveLeftCommandId: return MoveItem(label, forward: false); case ReorderVectorMenuAuthority.MoveRightCommandId: return MoveItem(label, forward: true); case ShowSubentryUnderComponentCommandId: - return ShowSubentryItem(label, ToolOf(command)); + return ShowSubentryItem(label, ToolOf(leaf)); case VisibleComplexFormCommandId: return VisibleComplexFormItem(label); case FilterLexiconCommandId: case FilterNotebookCommandId: - return FilterItem(label, ToolOf(command)); + return FilterItem(label, ToolOf(leaf)); default: - if (command != null && string.Equals(command.Message, CmObjectUi.JumpToToolMessage, - StringComparison.Ordinal)) - { - return JumpItem(command); - } - if (command != null && string.Equals(command.Message, EnvironmentMenuLeaves.ShowErrorMessage, - StringComparison.Ordinal)) - { + if (string.Equals(leaf.Message, CmObjectUi.JumpToToolMessage, StringComparison.Ordinal)) + return JumpItem(leaf); + if (string.Equals(leaf.Message, EnvironmentMenuLeaves.ShowErrorMessage, StringComparison.Ordinal)) return EnvironmentMenuLeaves.BuildDescribeError(label, ClickedItemText(), _host); - } - if (command != null && EnvironmentMenuLeaves.IsInsertMessage(command.Message)) - return EnvironmentMenuLeaves.BuildInsert(command.Message, label, _request, _host); + if (EnvironmentMenuLeaves.IsInsertMessage(leaf.Message)) + return EnvironmentMenuLeaves.BuildInsert(leaf.Message, label, _request, _host); throw new InvalidOperationException(string.Format( - "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.HelpId)); + "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.CommandId)); } } @@ -163,15 +156,18 @@ private string ClickedItemText() } // The tool a command's parameters name, or null. - private static string ToolOf(CommandChoice command) - => command == null ? null : XmlUtils.GetOptionalAttributeValue(command.CommandObject.Parameters[0], "tool"); + private static string ToolOf(DetailMenuLeaf leaf) + { + var parameters = leaf.Command.Parameters[0]; + return parameters == null ? null : XmlUtils.GetOptionalAttributeValue(parameters, "tool"); + } // The item's object UI decides a jump as it would for any menu; the first enabled jump // is the Ctrl+click default and its label says so. - private DetailMenuItem JumpItem(CommandChoice choice) + private DetailMenuItem JumpItem(DetailMenuLeaf leaf) { - var command = choice.CommandObject; - var display = new UIItemDisplayProperties(null, choice.Label, true, command.IconName, true); + var command = leaf.Command; + var display = new UIItemDisplayProperties(null, leaf.Label, true, command.IconName, true); _itemUi.OnDisplayJumpToTool(command, ref display); // The display sets the shared command's target; the object UI resolves its own on // execute, so clear it here rather than leave it for a later menu to pick up. @@ -182,9 +178,9 @@ private DetailMenuItem JumpItem(CommandChoice choice) if (!display.Enabled) return new DetailMenuItem(label, isEnabled: false, display.Checked); Action execute = () => _host.RunJump(() => _itemUi.OnJumpToTool(command)); - if (_defaultCommandId == null || string.Equals(_defaultCommandId, choice.HelpId, StringComparison.Ordinal)) + if (_defaultCommandId == null || string.Equals(_defaultCommandId, leaf.CommandId, StringComparison.Ordinal)) { - _defaultCommandId = choice.HelpId; + _defaultCommandId = leaf.CommandId; DefaultActivation = execute; label += CmObjectUi.CtrlClickSuffix; } diff --git a/Src/xWorks/Avalonia/Hosting/ReorderVectorMenuAuthority.cs b/Src/xWorks/Avalonia/Hosting/ReorderVectorMenuAuthority.cs index 6e7917fb3f..a72d46583b 100644 --- a/Src/xWorks/Avalonia/Hosting/ReorderVectorMenuAuthority.cs +++ b/Src/xWorks/Avalonia/Hosting/ReorderVectorMenuAuthority.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Generic; using SIL.FieldWorks.Common.FwAvalonia.Detail; -using XCore; namespace SIL.FieldWorks.XWorks { @@ -41,12 +40,12 @@ public ReorderVectorMenuAuthority(DetailMenuRequest request, public bool Owns(string menuId) => string.Equals(menuId, MenuId, StringComparison.Ordinal); - public DetailMenuItem Build(string menuId, ChoiceBase leaf) + public DetailMenuItem Build(string menuId, DetailMenuLeaf leaf) { if (leaf == null) throw new ArgumentNullException(nameof(leaf)); var label = XCoreMenuBridge.StripAccelerator(leaf.Label); - switch (leaf.HelpId) + switch (leaf.CommandId) { case MoveLeftCommandId: return MoveItem(label, forward: false); @@ -56,7 +55,7 @@ public DetailMenuItem Build(string menuId, ChoiceBase leaf) return AlphabeticalOrderItem(label); default: throw new InvalidOperationException(string.Format( - "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.HelpId)); + "Menu '{0}' has a leaf '{1}' this authority does not answer.", menuId, leaf.CommandId)); } } diff --git a/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs b/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs index 3baea28557..3bae6fc06d 100644 --- a/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs +++ b/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs @@ -4,7 +4,9 @@ using System; using System.Collections.Generic; +using System.Xml; using SIL.FieldWorks.Common.FwAvalonia.Detail; +using SIL.Utils; using XCore; namespace SIL.FieldWorks.XWorks @@ -14,7 +16,8 @@ namespace SIL.FieldWorks.XWorks /// 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. + /// dispatch), only the rendering changes. An owned id never becomes a ChoiceGroup: the + /// bridge walks its menu XML and the authority answers every leaf. /// Because this consumes the shared engine, it serves every DTMenuHandler-hosting tool /// (Grammar, Notebook, Lists, Words), not just the Lexicon. /// @@ -58,11 +61,10 @@ public static IReadOnlyList CreateMenuItems(XWindow window, stri /// /// 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. - /// A list-populated submenu is answered too: the authority supplies its items, so the - /// group is never populated through the mediator. + /// owns is built from its configuration alone, with no ChoiceGroup and no mediator + /// display query: every leaf under it, submenus included, is answered by the authority, + /// and a list-populated submenu takes its items from the authority too. Other ids keep + /// the mediator path. /// public static IReadOnlyList CreateMenuItems(XWindow window, string[] menuIds, Func interceptor, @@ -72,28 +74,36 @@ public static IReadOnlyList CreateMenuItems(XWindow window, stri 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. - var groups = new List<(ChoiceGroup Group, string OwnedId)>(); + // One group or definition per id keeps each id's ownership known; the source menus + // contribute their items in order. + var menus = new List<(ChoiceGroup Group, DetailMenuDefinition Owned)>(); foreach (var id in menuIds) { if (string.IsNullOrEmpty(id)) continue; + if (authority != null && authority.Owns(id)) + { + menus.Add((null, ResolveMenu(window, id))); + continue; + } var group = window.GetContextMenuChoiceGroup(new[] { id }); if (group != null) - groups.Add((group, authority != null && authority.Owns(id) ? id : null)); + menus.Add((group, null)); } - if (groups.Count == 0) + if (menus.Count == 0) return items; if (temporaryColleague != null) window.Mediator.AddTemporaryColleague(temporaryColleague); - foreach (var (group, ownedId) in groups) + foreach (var (group, owned) in menus) { - // 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)); + if (owned != null) + { + items.AddRange(ConvertOwned(owned.Entries, authority, owned.MenuId)); + continue; + } + group.PopulateNow(); + items.AddRange(Convert(group, interceptor)); } TrimSeparators(items); @@ -117,11 +127,145 @@ public static bool OwnsAll(IDetailMenuAuthority authority, IEnumerable m return true; } - // 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, + /// + /// The configured item tree of one context-menu id, resolved through the window (so + /// the id lookup stays in one place) and the window's command set, with no ChoiceGroup + /// and no mediator query. Labels are localized as xCore localizes them. An item + /// ChoiceGroup would carry but no authority can answer (a property toggle, an + /// undefined command) is kept as such rather than refused, so the menu it belongs to + /// stays on the mediator path. + /// + /// The id is not defined, or an item is of a + /// kind ChoiceBase.Make would refuse. + public static DetailMenuDefinition ResolveMenu(XWindow window, string menuId) + { + if (window == null) + throw new ArgumentNullException(nameof(window)); + if (string.IsNullOrEmpty(menuId)) + throw new ArgumentException("A menu id is required.", nameof(menuId)); + var node = window.GetContextMenuNodeFromMenuId(menuId); + // A list-populated root is one list, as ChoiceGroup.IsAListGroup reads it. + var listId = XmlUtils.GetOptionalAttributeValue(node, "list"); + var entries = listId != null + ? new List + { + DetailMenuEntry.ForList(XmlUtils.GetLocalizedAttributeValue(node, "label", null), false, listId) + } + : ResolveEntries(node, window.Mediator.CommandSet); + return new DetailMenuDefinition(menuId, entries); + } + + // A menu node's children as ChoiceGroup.Populate reads them: command, separator ("-") + // and property items, and nested menus. An undefined command fails xCore only on display. + private static List ResolveEntries(XmlNode menuNode, CommandSet commands) + { + var entries = new List(); + foreach (XmlNode child in menuNode.ChildNodes) + { + if (child.NodeType != XmlNodeType.Element) + continue; + switch (child.Name) + { + case "item": + entries.Add(ResolveItem(child, commands)); + break; + case "menu": + var label = XmlUtils.GetLocalizedAttributeValue(child, "label", null); + var isInline = XmlUtils.GetOptionalBooleanAttributeValue(child, "inline", false); + var listId = XmlUtils.GetOptionalAttributeValue(child, "list"); + entries.Add(listId != null + ? DetailMenuEntry.ForList(label, isInline, listId) + : DetailMenuEntry.ForSubmenu(label, isInline, ResolveEntries(child, commands))); + break; + default: + // A sidebar-style group or an unknown element: ChoiceGroup keeps the + // menu, so the walk does too, as something no authority can answer. + entries.Add(DetailMenuEntry.ForUnanswerable( + XmlUtils.GetLocalizedAttributeValue(child, "label", null), + string.Format("element '{0}' is not a menu item", child.Name))); + break; + } + } + return entries; + } + + private static DetailMenuEntry ResolveItem(XmlNode itemNode, CommandSet commands) + { + var label = XmlUtils.GetLocalizedAttributeValue(itemNode, "label", null); + var commandId = XmlUtils.GetOptionalAttributeValue(itemNode, "command"); + if (string.IsNullOrEmpty(commandId)) + { + // The same precedence as ChoiceBase.Make: a bool property, then a separator, + // then a single list-property value. + var property = XmlUtils.GetOptionalAttributeValue(itemNode, "boolProperty"); + if (string.IsNullOrEmpty(property)) + { + if (XmlUtils.GetOptionalAttributeValue(itemNode, "label") == "-") + return DetailMenuEntry.Separator(); + property = XmlUtils.GetOptionalAttributeValue(itemNode, "property"); + } + if (string.IsNullOrEmpty(property)) + throw new ConfigurationException("A context-menu item must name a command or a property.", itemNode); + return DetailMenuEntry.ForUnanswerable(label, string.Format( + "property item '{0}' is answered by the property table", property)); + } + if (!(commands[commandId] is Command command)) + { + return DetailMenuEntry.ForUnanswerable(label, string.Format( + "command '{0}' is not defined", commandId)); + } + return DetailMenuEntry.ForLeaf(new DetailMenuLeaf(command, label ?? command.Label)); + } + + // The authority answers each leaf whole and supplies a list submenu's items. A submenu + // keeps its configured label, is omitted when empty and spliced when inline. + private static List ConvertOwned(IReadOnlyList entries, IDetailMenuAuthority authority, string ownedId) + { + var items = new List(); + foreach (var entry in entries) + { + if (entry.IsSeparator) + { + items.Add(DetailMenuItem.Separator()); + } + else if (entry.Leaf != null) + { + var native = authority.Build(ownedId, entry.Leaf); + if (native != null) + items.Add(WithoutExecuteWhenDisabled(native)); + } + else if (entry.Unanswerable != null) + { + throw new InvalidOperationException(string.Format( + "Menu '{0}' has an item no authority can answer: {1}.", ownedId, entry.Unanswerable)); + } + else + { + List children; + if (entry.ListId != null) + { + children = Normalized(authority.BuildList(ownedId, entry.ListId)); + } + else + { + children = ConvertOwned(entry.Children, authority, ownedId); + TrimSeparators(children); + } + if (children.Count == 0) + continue; + if (entry.IsInline) + items.AddRange(children); + else + items.Add(new DetailMenuItem(StripAccelerator(entry.Label), isEnabled: true, isChecked: false, children)); + } + } + return items; + } + + // The mediator path. Edge separators stay: they divide merged groups. + private static List Convert(ChoiceGroup group, + Func interceptor) { var items = new List(); foreach (var member in group) @@ -133,14 +277,8 @@ 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 = ConvertChildren(submenu, interceptor, authority, null); + var children = ConvertChildren(submenu, interceptor); if (children.Count == 0) continue; @@ -160,15 +298,6 @@ private static List Convert(ChoiceGroup group, } else if (member is ChoiceBase choice) { - 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)); - continue; - } - var display = choice.GetDisplayProperties(); if (!display.Visible) continue; @@ -194,30 +323,13 @@ private static List Convert(ChoiceGroup group, // 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) + Func interceptor) { - var children = Convert(submenu, interceptor, authority, ownedId); + var children = Convert(submenu, interceptor); TrimSeparators(children); return children; } - // An owned submenu takes its label from the configuration and its children from the - // authority, a list-populated one included. Omitted when empty, spliced when inline. - private static IEnumerable ConvertOwnedSubmenu(ChoiceGroup submenu, - IDetailMenuAuthority authority, string ownedId) - { - var children = string.IsNullOrEmpty(submenu.ListId) - ? ConvertChildren(submenu, null, authority, ownedId) - : Normalized(authority.BuildList(ownedId, submenu.ListId)); - if (children.Count == 0 || submenu.IsInlineChoiceList) - return children; - return new[] - { - new DetailMenuItem(StripAccelerator(submenu.Label), isEnabled: true, isChecked: false, children) - }; - } - // The authority's list items as the renderer needs them: disabled items lose their // execute action like any other leaf, and stranded separators go. private static List Normalized(IReadOnlyList items) diff --git a/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs b/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs index 0d8008381f..7582582843 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs @@ -283,24 +283,19 @@ public void ReferenceItemAuthority_AnswersEveryLeafOfItsMenu() using (itemUi) { var window = m_propertyTable.GetValue("window"); - var menu = window.GetContextMenuChoiceGroup(new[] { ReferenceItemMenuAuthority.MenuId }); - menu.PopulateNow(querySubmenuVisibility: false); - - var leaves = Leaves(menu).ToList(); + var leaves = LeavesOf(ReferenceItemMenuAuthority.MenuId); Assert.That(leaves.Count, Is.GreaterThanOrEqualTo(40), "the shipped menu carries the jumps, the filters, the two marks and the moves"); foreach (var leaf in leaves) { - Assert.That(leaf.ConfigurationNode, Is.Not.Null, - "leaf '{0}' has no configuration node, so no authority can claim it", leaf.Label); Assert.That(() => authority.Build(ReferenceItemMenuAuthority.MenuId, leaf), Throws.Nothing, - "the authority does not answer leaf '{0}'", leaf.HelpId); + "the authority does not answer leaf '{0}'", leaf.CommandId); } Assert.That(() => XCoreMenuBridge.CreateMenuItems(window, new[] { ReferenceItemMenuAuthority.MenuId }, null, null, authority), Throws.Nothing, "the whole menu builds through the bridge"); - var targets = leaves.OfType() - .Where(c => c.Message == SIL.FieldWorks.FdoUi.CmObjectUi.JumpToToolMessage) - .Select(c => c.CommandObject.TargetId).Distinct().ToList(); + var targets = leaves + .Where(l => l.Message == SIL.FieldWorks.FdoUi.CmObjectUi.JumpToToolMessage) + .Select(l => l.Command.TargetId).Distinct().ToList(); Assert.That(targets, Is.EqualTo(new[] { Guid.Empty }), "building the menu leaves no jump command carrying a target for a later menu"); } @@ -313,10 +308,7 @@ public void ReferenceItemAuthority_RejectsALeafItDoesNotAnswer() var authority = m_view.CreateReferenceItemAuthority(ItemRequest(SubentriesField(), 0), out var itemUi); using (itemUi) { - var window = m_propertyTable.GetValue("window"); - var objectMenu = window.GetContextMenuChoiceGroup(new[] { RecordEditView.ObjectMenuId }); - objectMenu.PopulateNow(); - var foreignLeaf = objectMenu.OfType().First(c => c.HelpId == ObjectMenuAuthority.HelpCommandId); + var foreignLeaf = LeafOf(RecordEditView.ObjectMenuId, ObjectMenuAuthority.HelpCommandId); Assert.That(() => authority.Build(ReferenceItemMenuAuthority.MenuId, foreignLeaf), Throws.InvalidOperationException, "an owned id must be answered in full, never partially"); @@ -598,17 +590,12 @@ public void EnvironmentItemAuthority_AnswersEveryLeafOfItsMenu_AndTakesTheJumpAs using (itemUi) { var window = m_propertyTable.GetValue("window"); - var menu = window.GetContextMenuChoiceGroup(new[] { ReferenceItemMenuAuthority.EnvironmentMenuId }); - menu.PopulateNow(querySubmenuVisibility: false); - - var leaves = Leaves(menu).ToList(); + var leaves = LeavesOf(ReferenceItemMenuAuthority.EnvironmentMenuId); Assert.That(leaves.Count, Is.EqualTo(7), "the jump, Describe Error and the five inserts"); foreach (var leaf in leaves) { - Assert.That(leaf.ConfigurationNode, Is.Not.Null, - "leaf '{0}' has no configuration node, so no authority can claim it", leaf.Label); Assert.That(() => authority.Build(ReferenceItemMenuAuthority.EnvironmentMenuId, leaf), - Throws.Nothing, "the authority does not answer leaf '{0}'", leaf.HelpId); + Throws.Nothing, "the authority does not answer leaf '{0}'", leaf.CommandId); } var items = XCoreMenuBridge.CreateMenuItems(window, new[] { ReferenceItemMenuAuthority.EnvironmentMenuId }, null, null, authority); @@ -734,10 +721,7 @@ public void EnvironmentInsertAuthority_RejectsALeafItDoesNotAnswer() { var field = EnvironmentsFieldWithOneItem(); var authority = new EnvironmentInsertMenuAuthority(LabelMenuRequest(field, NoItem), m_view); - var window = m_propertyTable.GetValue("window"); - var objectMenu = window.GetContextMenuChoiceGroup(new[] { RecordEditView.ObjectMenuId }); - objectMenu.PopulateNow(); - var foreignLeaf = objectMenu.OfType().First(c => c.HelpId == ObjectMenuAuthority.HelpCommandId); + var foreignLeaf = LeafOf(RecordEditView.ObjectMenuId, ObjectMenuAuthority.HelpCommandId); Assert.That(() => authority.Build(EnvironmentInsertMenuAuthority.EnvironmentsMenuId, foreignLeaf), Throws.InvalidOperationException, "an owned id must be answered in full, never partially"); @@ -1223,8 +1207,7 @@ public void ReorderVectorMenu_RealSequenceRow_OffersTheMoves_ButNoAlphabeticalOr Assert.That(FindItem(items, "Alphabetical Order"), Is.Null, "no reorder='true', no Alphabetical Order"); } - // Every leaf under the owned menu, submenus included, must be one the authority knows - // and must carry the configuration node ownership is read from. + // Every leaf under the owned menu, submenus included, must be one the authority knows. [Test] public void ReorderVectorAuthority_AnswersEveryLeafOfItsMenu() { @@ -1233,50 +1216,31 @@ public void ReorderVectorAuthority_AnswersEveryLeafOfItsMenu() menuId: ReorderVectorMenuAuthority.MenuId, objectHvo: m_entry.Hvo, items: new[] { new DetailChoiceOption("a", "A") }, canReorderItems: true, canResetItemOrder: true); var authority = m_view.CreateReorderVectorAuthority(LabelMenuRequest(field, NoItem)); - var window = m_propertyTable.GetValue("window"); - var menu = window.GetContextMenuChoiceGroup(new[] { ReorderVectorMenuAuthority.MenuId }); - menu.PopulateNow(); - var leaves = Leaves(menu).ToList(); + var leaves = LeavesOf(ReorderVectorMenuAuthority.MenuId); Assert.That(leaves, Is.Not.Empty, "the menu must define at least one leaf"); foreach (var leaf in leaves) { - Assert.That(leaf.ConfigurationNode, Is.Not.Null, - "leaf '{0}' has no configuration node, so no authority can claim it", leaf.Label); Assert.That(() => authority.Build(ReorderVectorMenuAuthority.MenuId, leaf), Throws.Nothing, - "the authority does not answer leaf '{0}'", leaf.HelpId); + "the authority does not answer leaf '{0}'", leaf.CommandId); } } - // Every non-separator leaf of a choice group, descending into submenus. - private static IEnumerable Leaves(ChoiceGroup group) - { - foreach (var member in group) - { - if (member is SeparatorChoice) - continue; - if (member is ChoiceGroup submenu) - { - submenu.PopulateNow(); - foreach (var leaf in Leaves(submenu)) - yield return leaf; - } - else if (member is ChoiceBase leaf) - { - yield return leaf; - } - } - } + // Every command leaf of a menu id, submenus included, as the bridge resolves it from + // the configuration. + private IReadOnlyList LeavesOf(string menuId) + => XCoreMenuBridge.ResolveMenu(m_propertyTable.GetValue("window"), menuId).Leaves; + + // One command leaf of a menu id, by command id. + private DetailMenuLeaf LeafOf(string menuId, string commandId) + => LeavesOf(menuId).First(l => l.CommandId == commandId); [Test] public void ReorderVectorAuthority_RejectsALeafItDoesNotAnswer() { MakeTwoSubentries(); var authority = m_view.CreateReorderVectorAuthority(LabelMenuRequest(SubentriesField(), NoItem)); - var window = m_propertyTable.GetValue("window"); - var objectMenu = window.GetContextMenuChoiceGroup(new[] { RecordEditView.ObjectMenuId }); - objectMenu.PopulateNow(); - var foreignLeaf = objectMenu.OfType().First(c => c.HelpId == "CmdDataTree-Help"); + var foreignLeaf = LeafOf(RecordEditView.ObjectMenuId, "CmdDataTree-Help"); Assert.That(() => authority.Build(ReorderVectorMenuAuthority.MenuId, foreignLeaf), Throws.InvalidOperationException, "an owned id must be answered in full, never partially"); @@ -1347,8 +1311,8 @@ public EchoAuthority(IEnumerable owned, params string[] hidden) _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 + public DetailMenuItem Build(string menuId, DetailMenuLeaf leaf) + => _hidden.Contains(leaf.CommandId) ? null : new DetailMenuItem(XCoreMenuBridge.StripAccelerator(leaf.Label), isEnabled: true); // Echoes the list's id as one item, so a test can see where it spliced in. @@ -1421,6 +1385,120 @@ public void UnownedMenu_StillAsksTheMediator_ForSubmenuLeaves() Assert.That(asked, Is.True, "the mediator path decides submenu visibility by asking colleagues"); } + // The XML walk must see what ChoiceGroup.Populate sees. The mediator path drops a + // submenu no colleague shows, so nested leaves compare as a subset, top-level exactly. + [Test] + public void ResolveMenu_ReadsWhatChoiceGroupPopulates_ForEveryContextMenuId() + { + var window = m_propertyTable.GetValue("window"); + var configuration = m_propertyTable.GetValue("WindowConfiguration"); + var ids = configuration.SelectNodes("//contextMenus/menu[@id]").Cast() + .Select(n => n.Attributes["id"].Value).Distinct().ToList(); + Assert.That(ids.Count, Is.GreaterThan(48), "the window configuration defines the tools' context menus"); + + var undefinedCommands = new List(); + foreach (var id in ids) + { + var resolved = XCoreMenuBridge.ResolveMenu(window, id); + var group = window.GetContextMenuChoiceGroup(new[] { id }); + group.PopulateNow(); + + var groupTopLevel = group.OfType().Select(Describe).ToList(); + var resolvedTopLevel = resolved.Entries.Where(e => e.Leaf != null || IsUndefinedCommand(e)) + .Select(Describe).ToList(); + Assert.That(resolvedTopLevel, Is.EqualTo(groupTopLevel), "menu '{0}': top-level leaves", id); + var resolvedLeaves = resolved.Leaves.Select(l => Describe(l.CommandId, l.Label)) + .Concat(AllEntries(resolved.Entries).Where(IsUndefinedCommand).Select(Describe)).ToList(); + Assert.That(resolvedLeaves, Is.SupersetOf(ChoiceLeaves(group).Select(Describe)), + "menu '{0}': nested leaves", id); + Assert.That(resolved.ListIds, Is.EquivalentTo(ListIds(group)), "menu '{0}': list submenus", id); + undefinedCommands.AddRange(resolved.Unanswerable.Where(u => u.StartsWith("command", StringComparison.Ordinal)) + .Select(u => id + ": " + u)); + } + TestContext.WriteLine("Context-menu items naming an undefined command (ChoiceGroup fails on them at display time):" + + Environment.NewLine + string.Join(Environment.NewLine, undefinedCommands)); + } + + // Walks the ChoiceGroup side exactly as the configuration-walk side describes a leaf; + // a CommandChoice whose command is undefined has no label to read. + private static string Describe(CommandChoice choice) + { + try + { + return Describe(choice.HelpId, choice.Label); + } + catch (SIL.Utils.ConfigurationException) + { + return Describe(choice.HelpId, null); + } + } + + private static string Describe(DetailMenuEntry entry) + => entry.Leaf != null + ? Describe(entry.Leaf.CommandId, entry.Leaf.Label) + : Describe(UndefinedCommandId(entry), null); + + private static string Describe(string commandId, string label) + => label == null ? commandId : commandId + " = " + label; + + private static bool IsUndefinedCommand(DetailMenuEntry entry) => UndefinedCommandId(entry) != null; + + // The reason text names the id: "command 'X' is not defined". + private static string UndefinedCommandId(DetailMenuEntry entry) + { + const string prefix = "command '"; + if (entry.Unanswerable == null || !entry.Unanswerable.StartsWith(prefix, StringComparison.Ordinal)) + return null; + var end = entry.Unanswerable.IndexOf('\'', prefix.Length); + return entry.Unanswerable.Substring(prefix.Length, end - prefix.Length); + } + + private static IEnumerable AllEntries(IReadOnlyList entries) + { + foreach (var entry in entries) + { + yield return entry; + if (entry.Children == null) + continue; + foreach (var nested in AllEntries(entry.Children)) + yield return nested; + } + } + + // Every command leaf of a populated choice group, submenus included. + private static IEnumerable ChoiceLeaves(ChoiceGroup group) + { + foreach (var member in group) + { + if (member is ChoiceGroup submenu) + { + submenu.PopulateNow(); + foreach (var leaf in ChoiceLeaves(submenu)) + yield return leaf; + } + else if (member is CommandChoice leaf) + { + yield return leaf; + } + } + } + + private static IEnumerable ListIds(ChoiceGroup group) + { + foreach (var member in group) + { + if (!(member is ChoiceGroup submenu)) + continue; + if (!string.IsNullOrEmpty(submenu.ListId)) + { + yield return submenu.ListId; + continue; + } + foreach (var listId in ListIds(submenu)) + yield return listId; + } + } + [Test] public void OwnedSubmenu_WhoseLeavesAreAllHidden_IsOmitted() { @@ -1482,23 +1560,23 @@ public void MultiStringMenuAuthority_AnswersEveryLeafOfItsMenu() { var authority = m_view.CreateMultiStringMenuAuthority(LexemeFormField()); var window = m_propertyTable.GetValue("window"); - var menu = window.GetContextMenuChoiceGroup(new[] { MultiStringMenuAuthority.MenuId }); - menu.PopulateNow(querySubmenuVisibility: false); + var menu = XCoreMenuBridge.ResolveMenu(window, MultiStringMenuAuthority.MenuId); - var leaves = Leaves(menu).ToList(); - Assert.That(leaves.Select(l => l.HelpId), Is.EquivalentTo(new[] + Assert.That(menu.Leaves.Select(l => l.CommandId), Is.EquivalentTo(new[] { MultiStringMenuAuthority.ShowAllCommandId, MultiStringMenuAuthority.ConfigureCommandId, ObjectMenuAuthority.AlwaysVisibleCommandId, ObjectMenuAuthority.IfDataCommandId, ObjectMenuAuthority.NormallyHiddenCommandId, ObjectMenuAuthority.MoveFieldUpCommandId, ObjectMenuAuthority.MoveFieldDownCommandId, ObjectMenuAuthority.HelpCommandId }), "the shipped menu defines exactly the leaves the authority knows"); - foreach (var leaf in leaves) + foreach (var leaf in menu.Leaves) { Assert.That(() => authority.Build(MultiStringMenuAuthority.MenuId, leaf), Throws.Nothing, - "the authority does not answer leaf '{0}'", leaf.HelpId); + "the authority does not answer leaf '{0}'", leaf.CommandId); } // The list submenu has no configured leaves; the authority supplies its items. + Assert.That(menu.ListIds, Is.EqualTo(new[] { MultiStringMenuAuthority.WritingSystemListId }), + "the writing-system list is the menu's one list submenu"); Assert.That(authority.BuildList(MultiStringMenuAuthority.MenuId, MultiStringMenuAuthority.WritingSystemListId), Is.Not.Empty, "the writing-system list offers the row's options"); @@ -1508,11 +1586,7 @@ public void MultiStringMenuAuthority_AnswersEveryLeafOfItsMenu() public void MultiStringMenuAuthority_RejectsWhatItDoesNotAnswer() { var authority = m_view.CreateMultiStringMenuAuthority(LexemeFormField()); - var window = m_propertyTable.GetValue("window"); - var reorderMenu = window.GetContextMenuChoiceGroup(new[] { ReorderVectorMenuAuthority.MenuId }); - reorderMenu.PopulateNow(); - var foreignLeaf = reorderMenu.OfType() - .First(c => c.HelpId == ReorderVectorMenuAuthority.AlphabeticalOrderCommandId); + var foreignLeaf = LeafOf(ReorderVectorMenuAuthority.MenuId, ReorderVectorMenuAuthority.AlphabeticalOrderCommandId); Assert.That(() => authority.Build(MultiStringMenuAuthority.MenuId, foreignLeaf), Throws.InvalidOperationException, "a foreign leaf is refused, never left to the mediator"); @@ -1601,12 +1675,9 @@ private bool AdapterTargets(DetailField field) public void ObjectMenuAuthority_AnswersEveryLeafOfItsMenus() { var authority = m_view.CreateObjectMenuAuthority(LexemeFormField()); - var window = m_propertyTable.GetValue("window"); - var menu = window.GetContextMenuChoiceGroup(new[] { ObjectMenuAuthority.MenuId }); - menu.PopulateNow(querySubmenuVisibility: false); + var leaves = LeavesOf(ObjectMenuAuthority.MenuId); - var leaves = Leaves(menu).ToList(); - Assert.That(leaves.Select(l => l.HelpId), Is.EquivalentTo(new[] + Assert.That(leaves.Select(l => l.CommandId), Is.EquivalentTo(new[] { ObjectMenuAuthority.AlwaysVisibleCommandId, ObjectMenuAuthority.IfDataCommandId, ObjectMenuAuthority.NormallyHiddenCommandId, ObjectMenuAuthority.MoveFieldUpCommandId, @@ -1615,12 +1686,10 @@ public void ObjectMenuAuthority_AnswersEveryLeafOfItsMenus() foreach (var leaf in leaves) { Assert.That(() => authority.Build(ObjectMenuAuthority.MenuId, leaf), Throws.Nothing, - "the authority does not answer leaf '{0}'", leaf.HelpId); + "the authority does not answer leaf '{0}'", leaf.CommandId); } - var help = window.GetContextMenuChoiceGroup(new[] { ObjectMenuAuthority.HelpMenuId }); - help.PopulateNow(querySubmenuVisibility: false); - Assert.That(Leaves(help), Is.Empty, "mnuDataTree-Help defines no leaf of its own"); + Assert.That(LeavesOf(ObjectMenuAuthority.HelpMenuId), Is.Empty, "mnuDataTree-Help defines no leaf of its own"); Assert.That(authority.Owns(ObjectMenuAuthority.HelpMenuId), Is.True); } @@ -1822,11 +1891,7 @@ public void GhostRow_LocatesItsSequenceNode_AndItsVisibilityWritesThatNode() public void ObjectMenuAuthority_RejectsALeafItDoesNotAnswer() { var authority = m_view.CreateObjectMenuAuthority(LexemeFormField()); - var window = m_propertyTable.GetValue("window"); - var reorderMenu = window.GetContextMenuChoiceGroup(new[] { ReorderVectorMenuAuthority.MenuId }); - reorderMenu.PopulateNow(); - var foreignLeaf = reorderMenu.OfType() - .First(c => c.HelpId == ReorderVectorMenuAuthority.AlphabeticalOrderCommandId); + var foreignLeaf = LeafOf(ReorderVectorMenuAuthority.MenuId, ReorderVectorMenuAuthority.AlphabeticalOrderCommandId); Assert.That(() => authority.Build(ObjectMenuAuthority.MenuId, foreignLeaf), Throws.InvalidOperationException, "an owned id must be answered in full, never partially"); diff --git a/Src/xWorks/xWorksTests/Avalonia/Hosting/LexiconMenuAuthorityTests.cs b/Src/xWorks/xWorksTests/Avalonia/Hosting/LexiconMenuAuthorityTests.cs new file mode 100644 index 0000000000..d8ac7ae5aa --- /dev/null +++ b/Src/xWorks/xWorksTests/Avalonia/Hosting/LexiconMenuAuthorityTests.cs @@ -0,0 +1,194 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Xml; +using NUnit.Framework; +using SIL.FieldWorks.Common.FwAvalonia.Detail; +using SIL.FieldWorks.Common.FwUtils; +using XCore; + +namespace SIL.FieldWorks.XWorks +{ + /// + /// The message-keyed authority for the Lexicon detail menus, as the product's + /// wires it: which context-menu ids it owns (pinned, so a PR + /// that adds an answerer shows the menus it switched over), the predicate behind that + /// list, and the contracts every owned id must meet. + /// + [TestFixture] + [Apartment(System.Threading.ApartmentState.STA)] + public class LexiconMenuAuthorityTests : XWorksAppTestBase + { + // The ids the product authority owns today. Every answerer a PR adds extends this. + private static readonly string[] PinnedOwnedIds = new string[0]; + + // The messages of mnuDataTree-Object's six leaves, the test subject for the predicate. + private static readonly string[] ObjectMenuMessages = + { + "ShowFieldAlwaysVisible", "ShowFieldIfData", "ShowFieldNormallyHidden", + "MoveFieldUp", "MoveFieldDown", "DataTreeHelp" + }; + + private PropertyTable m_propertyTable; + private RecordEditView m_view; + + protected override void Init() + { + m_application = new MockFwXApp(new MockFwManager { Cache = Cache }, null, null); + m_configFilePath = Path.Combine(FwDirectoryFinder.CodeDirectory, m_application.DefaultConfigurationPathname); + } + + [SetUp] + public void SetUpWindow() + { + m_window = new MockFwXWindow(m_application, m_configFilePath); + ((MockFwXWindow)m_window).Init(Cache); + m_propertyTable = m_window.PropTable; + m_propertyTable.RemoveLocalAndGlobalSettings(); + m_window.LoadUI(m_configFilePath); + m_propertyTable.SetProperty("UIMode", "New", true); + m_propertyTable.SetPropertyPersistence("UIMode", false); + LoadRecordEditView("lexiconEdit"); + DrainMediatorAndIdleQueues(); + m_view = m_propertyTable.GetValue("currentContentControlObject", null) as RecordEditView; + Assert.That(m_view, Is.Not.Null, "expected the lexicon edit RecordEditView to load"); + } + + [TearDown] + public void TearDownWindow() + { + m_view = null; + m_propertyTable?.RemoveLocalAndGlobalSettings(); + m_propertyTable = null; + if (m_window != null && !m_window.IsDisposed) + { + m_window.Dispose(); + m_window = null; + } + } + + // Contract: the owned-id list is exact, and every owned id builds in full through the + // bridge, so an unanswered leaf fails here instead of reverting a menu to the adapter. + [Test] + public void OwnsExactlyThePinnedIds_AndAnswersEveryLeafOfEachOne() + { + var authority = m_view.CreateLexiconMenuAuthority(); + var window = Window; + + var owned = ContextMenuIds().Where(authority.Owns).ToList(); + + Assert.That(owned, Is.EquivalentTo(PinnedOwnedIds), + "the authority's owned ids changed; update the pin so the PR shows which menus switched over"); + foreach (var id in owned) + { + foreach (var leaf in XCoreMenuBridge.ResolveMenu(window, id).Leaves) + { + Assert.That(() => authority.Build(id, leaf), Throws.Nothing, + "menu '{0}': the authority does not answer leaf '{1}'", id, leaf.CommandId); + } + Assert.That(() => XCoreMenuBridge.CreateMenuItems(window, new[] { id }, null, null, authority), + Throws.Nothing, "menu '{0}' builds in full through the bridge", id); + } + } + + [Test] + public void Owns_WhenEveryLeafsMessageIsAnswered_AndNotOneMessageShort() + { + var authority = NewAuthority(); + foreach (var message in ObjectMenuMessages.Take(ObjectMenuMessages.Length - 1)) + authority.Answer(message, Echo); + Assert.That(authority.Owns(RecordEditView.ObjectMenuId), Is.False, + "one unanswered message leaves the id with the adapter"); + + authority.Answer(ObjectMenuMessages.Last(), Echo); + + Assert.That(authority.Owns(RecordEditView.ObjectMenuId), Is.True, + "the id is owned the moment its last message has an answerer"); + var items = XCoreMenuBridge.CreateMenuItems(Window, new[] { RecordEditView.ObjectMenuId }, + null, null, authority); + Assert.That(items.Select(i => i.Label), Does.Contain("Help..."), + "the owned menu builds from the answerers alone"); + } + + [Test] + public void NeverOwns_AMenuWithoutLeaves_OrWithAListSubmenu_OrWithAPropertyToggle() + { + var authority = NewAuthority(); + foreach (var message in ObjectMenuMessages) + authority.Answer(message, Echo); + authority.Answer("DataTreeWritingSystemsShowAll", Echo); + authority.Answer("DataTreeWritingSystemsConfigureDlg", Echo); + + Assert.That(authority.Owns(ObjectMenuAuthority.HelpMenuId), Is.False, + "mnuDataTree-Help has no leaf, so there is nothing to answer"); + Assert.That(XCoreMenuBridge.ResolveMenu(Window, RecordEditView.MultiStringSliceMenuId).Leaves + .All(l => authority.AnsweredMessages.Contains(l.Message)), Is.True, + "precondition: every configured leaf of the multi-string menu is answered"); + Assert.That(authority.Owns(RecordEditView.MultiStringSliceMenuId), Is.False, + "a list-populated submenu has no configured leaves to answer, so the id is not owned"); + + var window = Window; + var withToggle = ContextMenuIds().Select(id => XCoreMenuBridge.ResolveMenu(window, id)) + .First(m => m.Unanswerable.Any(u => u.StartsWith("property", StringComparison.Ordinal)) && m.Leaves.Count > 0); + foreach (var message in withToggle.Leaves.Select(l => l.Message).Distinct() + .Where(m => !authority.AnsweredMessages.Contains(m))) + { + authority.Answer(message, Echo); + } + Assert.That(authority.Owns(withToggle.MenuId), Is.False, + "menu '{0}': a property toggle belongs to the property table, so the id is not owned", withToggle.MenuId); + } + + [Test] + public void Build_RejectsALeafWhoseMessageHasNoAnswerer_AndEveryList() + { + var authority = m_view.CreateLexiconMenuAuthority(); + var help = XCoreMenuBridge.ResolveMenu(Window, RecordEditView.ObjectMenuId).Leaves + .First(l => l.CommandId == ObjectMenuAuthority.HelpCommandId); + + Assert.That(() => authority.Build(RecordEditView.ObjectMenuId, help), + Throws.InvalidOperationException, "an owned id must be answered in full, never partially"); + Assert.That(() => authority.BuildList(RecordEditView.MultiStringSliceMenuId, + MultiStringMenuAuthority.WritingSystemListId), Throws.InvalidOperationException); + } + + private XWindow Window => m_propertyTable.GetValue("window"); + + private LexiconMenuAuthority NewAuthority() + { + var window = Window; + return new LexiconMenuAuthority(id => XCoreMenuBridge.ResolveMenu(window, id)); + } + + private static DetailMenuItem Echo(DetailMenuLeaf leaf) + => new DetailMenuItem(XCoreMenuBridge.StripAccelerator(leaf.Label), isEnabled: true); + + // Every context-menu id the window configuration defines, each once. + private IEnumerable ContextMenuIds() + { + var configuration = m_propertyTable.GetValue("WindowConfiguration"); + Assert.That(configuration, Is.Not.Null); + return configuration.SelectNodes("//contextMenus/menu[@id]").Cast() + .Select(n => n.Attributes["id"].Value).Distinct(StringComparer.Ordinal).ToList(); + } + + private void LoadRecordEditView(string toolValue) + { + var windowConfiguration = m_propertyTable.GetValue("WindowConfiguration"); + Assert.That(windowConfiguration, Is.Not.Null); + var controlNode = windowConfiguration.SelectSingleNode( + string.Format("//tool[@value='{0}']/control//control[dynamicloaderinfo/@class='SIL.FieldWorks.XWorks.RecordEditView']", toolValue)); + Assert.That(controlNode, Is.Not.Null, "Expected the RecordEditView configuration node for tool '{0}'.", toolValue); + + m_propertyTable.SetProperty("currentContentControlParameters", controlNode, true); + m_propertyTable.SetPropertyPersistence("currentContentControlParameters", false); + m_propertyTable.SetProperty("currentContentControl", toolValue, true); + m_propertyTable.SetPropertyPersistence("currentContentControl", false); + } + } +} From bb05c39a7d00c70ba9141bc0fe629b1274c7fb21 Mon Sep 17 00:00:00 2001 From: mark-sil <83427558+mark-sil@users.noreply.github.com> Date: Thu, 8 Oct 2026 13:23:53 -0400 Subject: [PATCH 2/2] LT-22691: Splice a list-populated root menu's items flat The menu walk now marks a list at the root of a menu as inline, so its items splice into the menu as ChoiceGroup.PopulateFromList shows them, instead of nesting under the menu's label. No shipped context menu has a list at its root, and no authority owns a menu with a list, so nothing rendered differently; the comment beside the case also cited IsAListGroup, which tests the behavior attribute rather than list. Co-Authored-By: Claude Fable 5.1 --- Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs b/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs index 3bae6fc06d..cd86024e18 100644 --- a/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs +++ b/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs @@ -144,12 +144,12 @@ public static DetailMenuDefinition ResolveMenu(XWindow window, string menuId) if (string.IsNullOrEmpty(menuId)) throw new ArgumentException("A menu id is required.", nameof(menuId)); var node = window.GetContextMenuNodeFromMenuId(menuId); - // A list-populated root is one list, as ChoiceGroup.IsAListGroup reads it. + // A list-populated root shows its items flat, as ChoiceGroup.PopulateFromList does. var listId = XmlUtils.GetOptionalAttributeValue(node, "list"); var entries = listId != null ? new List { - DetailMenuEntry.ForList(XmlUtils.GetLocalizedAttributeValue(node, "label", null), false, listId) + DetailMenuEntry.ForList(XmlUtils.GetLocalizedAttributeValue(node, "label", null), isInline: true, listId) } : ResolveEntries(node, window.Mediator.CommandSet); return new DetailMenuDefinition(menuId, entries);