From db6f64e1fae1315969b1869bc527fa6b2110a1b4 Mon Sep 17 00:00:00 2001 From: mark-sil <83427558+mark-sil@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:23:38 -0400 Subject: [PATCH 1/3] LT-22691: Refactor the menu interceptor switch into a registry Promote BuildOverrideCommandInterceptor's HelpId switch into an OverrideCommandRegistry of (command id, item builder) entries. Behavior is identical; an unregistered command still falls through to normal mediator dispatch. A registry, unlike a switch, makes "which commands are handled natively" enumerable data: a later per-menu-group completion check can ask it what it covers and skip building the hidden adapter for a fully covered menu. It also gives the next commit's writing-system items, which carry no command id and must register by matcher, the same shape as id-keyed commands. Co-Authored-By: Claude Fable 5 --- .../Hosting/OverrideCommandRegistry.cs | 37 +++++++++++++++++++ .../Hosting/RecordEditView.Avalonia.cs | 29 ++++++--------- 2 files changed, 48 insertions(+), 18 deletions(-) create mode 100644 Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs diff --git a/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs b/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs new file mode 100644 index 0000000000..acdbaaa241 --- /dev/null +++ b/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs @@ -0,0 +1,37 @@ +// 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; +using XCore; + +namespace SIL.FieldWorks.XWorks +{ + /// + /// The commands the Avalonia detail view retargets away from mediator dispatch, as ordered + /// entries keyed by command HelpId. is the interceptor shape + /// consumes: the first matching entry builds the replacement + /// item; null leaves the command on its normal dispatch. + /// + internal sealed class OverrideCommandRegistry + { + private readonly List>> _entries + = new List>>(); + + public void Add(string helpId, Func build) + => _entries.Add(new KeyValuePair>(helpId, build)); + + public DetailMenuItem TryBuild(ChoiceBase choice) + { + foreach (var entry in _entries) + { + if (string.Equals(choice.HelpId, entry.Key, StringComparison.Ordinal)) + return entry.Value(choice); + } + + return null; + } + } +} diff --git a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs index 6349454c14..ce8740aa83 100644 --- a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs +++ b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs @@ -602,24 +602,17 @@ private Func BuildOverrideCommandInterceptor(DetailF if (location == null) return null; // unknown/stale target: leave commands on the legacy path rather than guess. - return choice => - { - switch (choice.HelpId) - { - case "CmdAlwaysVisible": - return VisibilityItem(choice, field, templateId, location, ViewVisibility.Always); - case "CmdIfData": - return VisibilityItem(choice, field, templateId, location, ViewVisibility.IfData); - case "CmdNormallyHidden": - return VisibilityItem(choice, field, templateId, location, ViewVisibility.Never); - case "CmdDataTree-MoveFieldUp": - return MoveItem(choice, field, location, up: true); - case "CmdDataTree-MoveFieldDown": - return MoveItem(choice, field, location, up: false); - default: - return null; // not a field command: keep its normal mediator dispatch. - } - }; + // An unregistered command falls through TryBuild as null: normal mediator dispatch. + var registry = new OverrideCommandRegistry(); + registry.Add("CmdAlwaysVisible", + c => VisibilityItem(c, field, templateId, location, ViewVisibility.Always)); + registry.Add("CmdIfData", + c => VisibilityItem(c, field, templateId, location, ViewVisibility.IfData)); + registry.Add("CmdNormallyHidden", + c => VisibilityItem(c, field, templateId, location, ViewVisibility.Never)); + registry.Add("CmdDataTree-MoveFieldUp", c => MoveItem(c, field, location, up: true)); + registry.Add("CmdDataTree-MoveFieldDown", c => MoveItem(c, field, location, up: false)); + return registry.TryBuild; } // A Field Visibility menu item: checked when it is the field's current visibility, executes the From 65f8937e784bc0096398b301c1fbf4265904bd79 Mon Sep 17 00:00:00 2001 From: mark-sil <83427558+mark-sil@users.noreply.github.com> Date: Tue, 25 Aug 2026 06:09:40 -0400 Subject: [PATCH 2/3] LT-22691: Copy writing-system menu choices into the view override Writing-system toggles and the Configure dialog dispatch to the hidden adapter slice as before; the resulting selection is then copied into the project view override so the Avalonia detail view recomposes with it. Toggles read the selection property (written before OnClick returns; the slice reacts later) and canonicalize it to the slice's option order. Configure is copied only when the dialog changed the selection, so Cancel writes nothing. The copy is skipped, with a log entry, when the adapter slice is unreadable or is not the clicked row's. Show all right now is deliberately not copied: it is a transient reveal in the slice, and persisting it would pin the full set. In the Avalonia view it currently does nothing visible; the native transient reveal is a planned follow-up. Co-Authored-By: Claude Fable 5 --- .../Hosting/OverrideCommandRegistry.cs | 18 +- .../Hosting/RecordEditView.Avalonia.cs | 159 +++++++++++++++--- .../DetailObjectCommandExecutionTests.cs | 156 +++++++++++++++++ 3 files changed, 307 insertions(+), 26 deletions(-) diff --git a/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs b/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs index acdbaaa241..687136b7c2 100644 --- a/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs +++ b/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs @@ -11,23 +11,27 @@ namespace SIL.FieldWorks.XWorks { /// /// The commands the Avalonia detail view retargets away from mediator dispatch, as ordered - /// entries keyed by command HelpId. is the interceptor shape - /// consumes: the first matching entry builds the replacement - /// item; null leaves the command on its normal dispatch. + /// (matcher, builder) entries. builds the replacement item from the + /// first matching entry; null leaves the command on its normal dispatch. /// internal sealed class OverrideCommandRegistry { - private readonly List>> _entries - = new List>>(); + private readonly List, Func>> _entries + = new List, Func>>(); public void Add(string helpId, Func build) - => _entries.Add(new KeyValuePair>(helpId, build)); + => Add(c => string.Equals(c.HelpId, helpId, StringComparison.Ordinal), build); + + /// Registers by matcher, for items that carry no command id. + public void Add(Func matches, Func build) + => _entries.Add( + new KeyValuePair, Func>(matches, build)); public DetailMenuItem TryBuild(ChoiceBase choice) { foreach (var entry in _entries) { - if (string.Equals(choice.HelpId, entry.Key, StringComparison.Ordinal)) + if (entry.Key(choice)) return entry.Value(choice); } diff --git a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs index ce8740aa83..67aac51a94 100644 --- a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs +++ b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs @@ -563,12 +563,12 @@ private ViewDefinitionOverride ResolveViewOverride(string className, string layo + "'; using the shipped definition.", error)); /// - /// Builds the interceptor that retargets the per-field Field Visibility and - /// Move Field commands to the project override layer for the Avalonia detail view. Returns null - /// (intercept nothing -- every command keeps its normal mediator dispatch) when the - /// clicked row - /// carries no (class, layout) context, e.g. the first-slice fallback rows; that keeps the legacy - /// behavior intact when the override layer cannot be addressed. + /// Builds the interceptor that retargets the per-field Field Visibility, Move Field, and + /// writing-system commands to the project override layer for the Avalonia detail view. + /// Returns null (intercept nothing -- every command keeps its normal mediator dispatch) + /// when the clicked row carries no (class, layout) context, e.g. the first-slice fallback + /// rows; that keeps the legacy behavior intact when the override layer cannot be + /// addressed. /// private Func BuildOverrideCommandInterceptor(DetailField field) { @@ -578,6 +578,12 @@ private Func BuildOverrideCommandInterceptor(DetailF return null; } + // Writing-system items dispatch normally; the resulting selection is then copied + // into the override. They need no located template node, so they stay registered + // even when locating fails. + var registry = new OverrideCommandRegistry(); + registry.Add(IsWritingSystemVisibilityChoice, c => WritingSystemItem(c, 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. @@ -596,22 +602,24 @@ private Func BuildOverrideCommandInterceptor(DetailF { Logger.WriteError("Resolving the field's override target failed; the gear-menu field " + "commands fall back to the legacy path for this row.", e); - return null; + return registry.TryBuild; } - if (location == null) - return null; // unknown/stale target: leave commands on the legacy path rather than guess. + // Unknown/stale target: leave the field commands on the legacy path rather than + // guess. + if (location != null) + { + registry.Add("CmdAlwaysVisible", + c => VisibilityItem(c, field, templateId, location, ViewVisibility.Always)); + registry.Add("CmdIfData", + c => VisibilityItem(c, field, templateId, location, ViewVisibility.IfData)); + registry.Add("CmdNormallyHidden", + c => VisibilityItem(c, field, templateId, location, ViewVisibility.Never)); + registry.Add("CmdDataTree-MoveFieldUp", c => MoveItem(c, field, location, up: true)); + registry.Add("CmdDataTree-MoveFieldDown", + c => MoveItem(c, field, location, up: false)); + } - // An unregistered command falls through TryBuild as null: normal mediator dispatch. - var registry = new OverrideCommandRegistry(); - registry.Add("CmdAlwaysVisible", - c => VisibilityItem(c, field, templateId, location, ViewVisibility.Always)); - registry.Add("CmdIfData", - c => VisibilityItem(c, field, templateId, location, ViewVisibility.IfData)); - registry.Add("CmdNormallyHidden", - c => VisibilityItem(c, field, templateId, location, ViewVisibility.Never)); - registry.Add("CmdDataTree-MoveFieldUp", c => MoveItem(c, field, location, up: true)); - registry.Add("CmdDataTree-MoveFieldDown", c => MoveItem(c, field, location, up: false)); return registry.TryBuild; } @@ -637,6 +645,119 @@ private DetailMenuItem MoveItem(ChoiceBase choice, DetailField field, execute: canMove ? (Action)(() => ApplyMoveField(field, location, up)) : null); } + /// + /// Whether this menu item makes a persistent change to which writing systems a + /// multi-writing-system field shows: a per-writing-system toggle (recognized by the + /// property its group drives -- the toggles carry no command id) or the Configure + /// dialog. Show all right now is excluded: it is a transient reveal on the slice, + /// not a configuration change, so persisting it would wrongly pin the full set. + /// + private static bool IsWritingSystemVisibilityChoice(ChoiceBase choice) + { + if (choice is ListPropertyChoice list) + { + return string.Equals(list.ParentProperty, + PropertyConstants.CurrentContextMenuSelectedWsIds, StringComparison.Ordinal); + } + + return string.Equals(choice.HelpId, "CmdDataTree-WritingSystemMenu-Configure", + StringComparison.Ordinal); + } + + /// + /// A writing-system item that dispatches normally and then copies the resulting + /// selection into the override: the hidden adapter slice owns the picker and the + /// Configure dialog, while the Avalonia detail view composes from its own override + /// store. + /// + private DetailMenuItem WritingSystemItem(ChoiceBase choice, DetailField field) + { + var display = choice.GetDisplayProperties(); + var isListToggle = choice is ListPropertyChoice; + // No execute when disabled: clicking the last checked toggle would EMPTY the set. + var execute = display.Enabled + ? (Action)(() => + { + // Snapshot first, so a dialog that changes nothing (e.g. Cancel) copies + // nothing. + var before = isListToggle ? null : CurrentSliceSelectedWritingSystems(); + choice.OnClick(null, EventArgs.Empty); + CopyWritingSystemSelectionToOverride(field, isListToggle, before); + }) + : null; + return new DetailMenuItem(XCoreMenuBridge.StripAccelerator(display.Text), display.Enabled, + display.Checked, children: null, execute: execute); + } + + // Copies the click's selection into the row's override and recomposes. A toggle + // updates its property BEFORE the slice: read the property, in option order; + // Configure reads the slice. + private void CopyWritingSystemSelectionToOverride(DetailField field, bool fromListToggle, + IReadOnlyList sliceSetBeforeClick) + { + try + { + var slice = m_dataEntryForm?.CurrentSlice as MultiStringSlice; + if (slice == null) + { + // The command dispatched, but the result is unreadable: say so, or the + // symptom is "the menu did nothing" with no trail. + Logger.WriteEvent("Writing-system selection was not copied: the adapter " + + "slice is unreadable; the view override was not updated."); + return; + } + + // A stale adapter target would store another row's set under this row's id. + if (slice.Object == null || slice.Object.Hvo != field.ObjectHvo) + { + Logger.WriteEvent("Writing-system selection was not copied: the adapter " + + "slice is not the clicked row's; the view override was not updated."); + return; + } + + List selected; + if (fromListToggle) + { + var ids = m_propertyTable.GetStringProperty( + PropertyConstants.CurrentContextMenuSelectedWsIds, null); + // Canonicalize: option order, junk tokens dropped -- the stored order is the + // render order. + selected = string.IsNullOrEmpty(ids) + ? null + : StringSliceUtils.GetVisibleWritingSystems(ids, + slice.WritingSystemOptionsForDisplay).Select(ws => ws.Id).ToList(); + } + else + { + selected = slice.WritingSystemsSelectedForDisplay?.Select(ws => ws.Id).ToList(); + if (selected != null && sliceSetBeforeClick != null + && selected.SequenceEqual(sliceSetBeforeClick, StringComparer.Ordinal)) + { + return; // the dialog changed nothing (e.g. Cancel): no override write. + } + } + + // The menu disables the last checked toggle, so an empty set only means "nothing + // to copy" -- and an empty op would CLEAR the restriction, so bail instead. + if (selected == null || selected.Count == 0) + return; + + var op = new ViewOverrideOperation(ViewOverrideOperationKind.SetVisibleWritingSystems, + ViewDefinitionOverrideEditor.StripRuntimeSuffix(field.StableId), + writingSystems: selected); + MutateOverrideAndRefresh(field, op); + } + catch (Exception e) + { + Logger.WriteError("Copying the writing-system selection into the view override failed.", e); + } + } + + // The adapter slice's current selection, or null when it cannot be read. + private IReadOnlyList CurrentSliceSelectedWritingSystems() + => (m_dataEntryForm?.CurrentSlice as MultiStringSlice) + ?.WritingSystemsSelectedForDisplay?.Select(ws => ws.Id).ToList(); + // Writes a SetVisibility op for the field's template id into the project override and recomposes. private void ApplyFieldVisibility(DetailField field, string templateId, ViewVisibility target) { diff --git a/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs b/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs index 1539a74cb5..8080a49dd9 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs @@ -13,9 +13,11 @@ using SIL.FieldWorks.Common.Controls; using SIL.FieldWorks.Common.FwAvalonia; using SIL.FieldWorks.Common.FwAvalonia.Detail; +using SIL.FieldWorks.Common.FwAvalonia.ViewDefinition; using SIL.FieldWorks.Common.Framework.DetailControls; using SIL.FieldWorks.Common.FwUtils; using SIL.LCModel; +using SIL.LCModel.Core.WritingSystems; using SIL.LCModel.Infrastructure; using XCore; // Both namespaces above define DataTree; the adapter tests mean the legacy WinForms one. @@ -363,10 +365,164 @@ public void LexemeFormSliceMenu_WritingSystemsSubmenu_ListsTheProjectWritingSyst "the spliced entries are the project's vernacular writing systems (the Lexeme Form's ws set)"); } + // THE decisive copy test: unchecking one of two vernaculars must store the REDUCED + // set (the slice still holds the pre-click set right after OnClick), and re-checking + // must store the restored set. + [Test] + public void WritingSystemToggle_TwoVernaculars_StoresTheReducedThenRestoredSet() + { + CoreWritingSystemDefinition second = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + Cache.ServiceLocator.WritingSystemManager.GetOrSet("es", out second); + Cache.ServiceLocator.WritingSystems.AddToCurrentVernacularWritingSystems(second); + }); + var field = LexemeFormField(); + try + { + EnsureAdapter(m_entry.LexemeFormOA.Hvo, "Form"); + var writingSystems = BuildWritingSystemsSubmenu(field); + var checkedEntries = writingSystems.Children + .Where(c => !c.IsSeparator && c.IsChecked && c.Execute != null).ToList(); + TestContext.WriteLine("checked: " + string.Join(", ", + checkedEntries.Select(e => e.Label))); + Assert.That(checkedEntries.Count, Is.EqualTo(2), + "precondition: both vernacular writing systems start visible"); + var toggled = checkedEntries[0].Label; + + checkedEntries[0].Execute(); + DrainMediatorAndIdleQueues(); + + var reduced = StoredWritingSystems(field); + TestContext.WriteLine("reduced: " + string.Join(",", reduced)); + Assert.That(reduced, Is.EqualTo(new[] { "es" }), + "the stored op holds the set AFTER the uncheck, not the pre-click set"); + + // Re-check the same writing system on a freshly built menu: the ADD direction. + writingSystems = BuildWritingSystemsSubmenu(field); + var reAdd = writingSystems.Children.Single(c => !c.IsSeparator && c.Label == toggled); + Assert.That(reAdd.IsChecked, Is.False, "the unchecked toggle stays unchecked"); + Assert.That(reAdd.Execute, Is.Not.Null, "an unchecked toggle is clickable"); + reAdd.Execute(); + DrainMediatorAndIdleQueues(); + + var restored = StoredWritingSystems(field); + TestContext.WriteLine("restored: " + string.Join(",", restored)); + // Exact order matters: the property appends the re-checked writing system at the + // end, but the copy canonicalizes to option order so rows never reorder. + Assert.That(restored, Is.EqualTo(new[] { "fr", "es" }), + "re-checking stores the restored set in option order"); + } + finally + { + DeleteOverrideFor(field); + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + Cache.ServiceLocator.WritingSystems.CurrentVernacularWritingSystems.Remove(second)); + } + } + + // The Lexeme Form row is keyed by the MoForm's own descended (class, layout). Compose + // must resolve an override for it, or the row's customizations are written and never + // read. + [Test] + public void Compose_ResolvesTheOverrideForADescendedLayout() + { + var field = LexemeFormField(); + var store = GetOverrideStore(); + var templateId = ViewDefinitionOverrideEditor.StripRuntimeSuffix(field.StableId); + TestContext.WriteLine( + $"class={field.ClassName} layout={field.LayoutName} template={templateId}"); + try + { + store.Save(new ViewDefinitionOverride(field.ClassName, field.LayoutName, "detail", + new[] + { + new ViewOverrideOperation(ViewOverrideOperationKind.SetLabel, templateId, + label: "OverrideMarker") + }, null)); + + var asked = new List(); + var recomposed = DetailComposer.Compose(m_entry, Cache, showHiddenFields: false, + overrides: (cls, layout) => + { + asked.Add(cls + "/" + layout); + return store.TryGet(cls, layout); + }); + TestContext.WriteLine("compose asked for: " + string.Join(", ", asked)); + + var row = recomposed.Model.Fields.Single(f => f.Field == "Form" + && f.Kind == DetailFieldKind.Text && f.ObjectHvo == m_entry.LexemeFormOA.Hvo); + Assert.That(row.Label, Is.EqualTo("OverrideMarker"), + "an override keyed by the row's own (class, layout) must reach the composed row"); + } + finally + { + DeleteOverrideFor(field); + } + } + // ----------------------------------------------------------------- // Helpers -- production-path command drivers // ----------------------------------------------------------------- + // The composed Lexeme Form row, as the production host resolves it before raising a + // menu request. + private DetailField LexemeFormField() + => DetailComposer.Compose(m_entry, Cache).Model.Fields.Single(f => f.Field == "Form" + && f.Kind == DetailFieldKind.Text && f.ObjectHvo == m_entry.LexemeFormOA.Hvo); + + private ViewDefinitionOverrideStore GetOverrideStore() + { + var storeProperty = typeof(RecordEditView).GetProperty("ViewOverrideStore", + BindingFlags.Instance | BindingFlags.NonPublic); + Assert.That(storeProperty, Is.Not.Null, "the override store seam must exist"); + var store = (ViewDefinitionOverrideStore)storeProperty.GetValue(m_view); + Assert.That(store, Is.Not.Null, "the project must have a reachable override store"); + return store; + } + + // Materializes a menu the way OnDetailMenuRequested does, WITH the override interceptor, + // so the intercepted writing-system items are the ones under test. + private IReadOnlyList BuildItemsWithOverrideInterceptor(string[] menuIds, + DetailField field) + { + var build = typeof(RecordEditView).GetMethod("BuildOverrideCommandInterceptor", + BindingFlags.Instance | BindingFlags.NonPublic); + Assert.That(build, Is.Not.Null, "the override interceptor seam must exist"); + var interceptor = (Func)build.Invoke(m_view, new object[] { field }); + TestContext.WriteLine("interceptor built: " + (interceptor != null)); + var window = m_propertyTable.GetValue("window"); + return XCoreMenuBridge.CreateMenuItems(window, menuIds, interceptor); + } + + private ViewDefinitionOverride ReadOverrideFor(DetailField field) + => GetOverrideStore().TryGet(field.ClassName, field.LayoutName); + + private DetailMenuItem BuildWritingSystemsSubmenu(DetailField field) + { + var items = BuildItemsWithOverrideInterceptor( + new[] { "mnuDataTree-LexemeForm", "mnuDataTree-MultiStringSlice" }, field); + var writingSystems = FindItem(items, "Writing Systems"); + Assert.That(writingSystems, Is.Not.Null, + "precondition: the Writing Systems submenu is present"); + return writingSystems; + } + + // The writing systems the stored override op carries for the field's row. + private IReadOnlyList StoredWritingSystems(DetailField field) + { + var stored = ReadOverrideFor(field); + Assert.That(stored, Is.Not.Null, + "toggling a writing system must write a project override"); + return stored.Operations.Single(o => + o.Kind == ViewOverrideOperationKind.SetVisibleWritingSystems).WritingSystems; + } + + // Saving an empty patch deletes the file, so overrides never leak between tests. + private void DeleteOverrideFor(DetailField field) + => GetOverrideStore().Save(new ViewDefinitionOverride( + field.ClassName, field.LayoutName, "detail", null, null)); + // Drives a SLICE-menu command exactly as OnDetailMenuRequested(Kind=SliceMenu) does: ensure the // adapter targets the object, materialize the menu (menuId + the host-appended mnuDataTree-Object) // through XCoreMenuBridge, find the item by label, invoke its Execute (mediator dispatch). Then From e22fd4ee3a05ee03f80af920b35bf9c0782af8da Mon Sep 17 00:00:00 2001 From: mark-sil <83427558+mark-sil@users.noreply.github.com> Date: Tue, 25 Aug 2026 06:43:21 -0400 Subject: [PATCH 3/3] LT-22691: Enforce the menu leaf contract in XCoreMenuBridge Pass each leaf's already-computed display properties through to the interceptor, so retargeted item builders stop re-querying GetDisplayProperties -- a second mediator Display* round trip per intercepted item. Normalize every leaf, default or retargeted, so a disabled item carries no execute action. "Execute != null" now means invokable for every consumer, including programmatic invokers; the writing-system item's local guard is replaced by this invariant. Co-Authored-By: Claude Fable 5 --- .../Hosting/OverrideCommandRegistry.cs | 19 +++++---- .../Hosting/RecordEditView.Avalonia.cs | 40 +++++++++---------- .../Avalonia/Hosting/XCoreMenuBridge.cs | 27 +++++++++---- .../DetailObjectCommandExecutionTests.cs | 3 +- 4 files changed, 52 insertions(+), 37 deletions(-) diff --git a/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs b/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs index 687136b7c2..84ff78a1bd 100644 --- a/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs +++ b/Src/xWorks/Avalonia/Hosting/OverrideCommandRegistry.cs @@ -16,23 +16,26 @@ namespace SIL.FieldWorks.XWorks /// internal sealed class OverrideCommandRegistry { - private readonly List, Func>> _entries - = new List, Func>>(); + private readonly List, + Func>> _entries + = new List, + Func>>(); - public void Add(string helpId, Func build) + public void Add(string helpId, Func build) => Add(c => string.Equals(c.HelpId, helpId, StringComparison.Ordinal), build); /// Registers by matcher, for items that carry no command id. - public void Add(Func matches, Func build) - => _entries.Add( - new KeyValuePair, Func>(matches, build)); + public void Add(Func matches, + Func build) + => _entries.Add(new KeyValuePair, + Func>(matches, build)); - public DetailMenuItem TryBuild(ChoiceBase choice) + public DetailMenuItem TryBuild(ChoiceBase choice, UIItemDisplayProperties display) { foreach (var entry in _entries) { if (entry.Key(choice)) - return entry.Value(choice); + return entry.Value(choice, display); } return null; diff --git a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs index 67aac51a94..9595d33b8c 100644 --- a/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs +++ b/Src/xWorks/Avalonia/Hosting/RecordEditView.Avalonia.cs @@ -570,7 +570,8 @@ private ViewDefinitionOverride ResolveViewOverride(string className, string layo /// rows; that keeps the legacy behavior intact when the override layer cannot be /// addressed. /// - private Func BuildOverrideCommandInterceptor(DetailField field) + private Func BuildOverrideCommandInterceptor( + DetailField field) { if (field == null || string.IsNullOrEmpty(field.ClassName) || string.IsNullOrEmpty(field.LayoutName) || ViewOverrideStore == null) @@ -582,7 +583,7 @@ private Func BuildOverrideCommandInterceptor(DetailF // into the override. They need no located template node, so they stay registered // even when locating fails. var registry = new OverrideCommandRegistry(); - registry.Add(IsWritingSystemVisibilityChoice, c => WritingSystemItem(c, field)); + registry.Add(IsWritingSystemVisibilityChoice, (c, d) => WritingSystemItem(c, d, field)); var templateId = ViewDefinitionOverrideEditor.StripRuntimeSuffix(field.StableId); // Locate the clicked node in the field's OWN compiled model (with any current override @@ -610,14 +611,15 @@ private Func BuildOverrideCommandInterceptor(DetailF if (location != null) { registry.Add("CmdAlwaysVisible", - c => VisibilityItem(c, field, templateId, location, ViewVisibility.Always)); + (c, d) => VisibilityItem(d, field, templateId, location, ViewVisibility.Always)); registry.Add("CmdIfData", - c => VisibilityItem(c, field, templateId, location, ViewVisibility.IfData)); + (c, d) => VisibilityItem(d, field, templateId, location, ViewVisibility.IfData)); registry.Add("CmdNormallyHidden", - c => VisibilityItem(c, field, templateId, location, ViewVisibility.Never)); - registry.Add("CmdDataTree-MoveFieldUp", c => MoveItem(c, field, location, up: true)); + (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 => MoveItem(c, field, location, up: false)); + (c, d) => MoveItem(d, field, location, up: false)); } return registry.TryBuild; @@ -626,20 +628,20 @@ private Func BuildOverrideCommandInterceptor(DetailF // 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(ChoiceBase choice, DetailField field, + private DetailMenuItem VisibilityItem(UIItemDisplayProperties display, DetailField field, string templateId, ViewNodeLocation location, ViewVisibility target) { - var label = XCoreMenuBridge.StripAccelerator(choice.GetDisplayProperties().Text); + 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(ChoiceBase choice, DetailField field, + private DetailMenuItem MoveItem(UIItemDisplayProperties display, DetailField field, ViewNodeLocation location, bool up) { - var label = XCoreMenuBridge.StripAccelerator(choice.GetDisplayProperties().Text); + 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); @@ -670,23 +672,21 @@ private static bool IsWritingSystemVisibilityChoice(ChoiceBase choice) /// Configure dialog, while the Avalonia detail view composes from its own override /// store. /// - private DetailMenuItem WritingSystemItem(ChoiceBase choice, DetailField field) + private DetailMenuItem WritingSystemItem(ChoiceBase choice, UIItemDisplayProperties display, + DetailField field) { - var display = choice.GetDisplayProperties(); var isListToggle = choice is ListPropertyChoice; - // No execute when disabled: clicking the last checked toggle would EMPTY the set. - var execute = display.Enabled - ? (Action)(() => + // The bridge strips execute from disabled items, so the last checked toggle + // (disabled) can never be invoked to EMPTY the set. + return new DetailMenuItem(XCoreMenuBridge.StripAccelerator(display.Text), display.Enabled, + display.Checked, children: null, execute: () => { // Snapshot first, so a dialog that changes nothing (e.g. Cancel) copies // nothing. var before = isListToggle ? null : CurrentSliceSelectedWritingSystems(); choice.OnClick(null, EventArgs.Empty); CopyWritingSystemSelectionToOverride(field, isListToggle, before); - }) - : null; - return new DetailMenuItem(XCoreMenuBridge.StripAccelerator(display.Text), display.Enabled, - display.Checked, children: null, execute: execute); + }); } // Copies the click's selection into the row's override and recomposes. A toggle diff --git a/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs b/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs index 5efffbd95e..9e9f775742 100644 --- a/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs +++ b/Src/xWorks/Avalonia/Hosting/XCoreMenuBridge.cs @@ -32,16 +32,18 @@ public static IReadOnlyList CreateMenuItems(XWindow window, stri /// /// As , but lets the host RETARGET specific leaf /// commands for the Avalonia detail view (advanced-entry-view). For each command leaf, the - /// is offered the leaf (so the host can - /// read the localized label and command id from it); if it returns a non-null + /// is offered the leaf and its + /// already-computed display properties (so the host reads the localized label and state + /// without a second Display* round trip); if it returns a non-null /// , that item (its label/checked/enabled/execute) is used INSTEAD of /// the default xCore-dispatched item. Returning null leaves the command on its normal mediator /// path. This is how the per-field Field Visibility / Move Field commands route to the project /// override layer while Help and every other item keep working unchanged. The interceptor only - /// sees leaf commands (submenus pass through). + /// sees leaf commands (submenus pass through). Every leaf, default or retargeted, is + /// normalized so a disabled item carries no execute action. /// public static IReadOnlyList CreateMenuItems(XWindow window, string[] menuIds, - Func interceptor) + Func interceptor) { var group = window?.GetContextMenuChoiceGroup(menuIds); if (group == null) @@ -50,7 +52,8 @@ public static IReadOnlyList CreateMenuItems(XWindow window, stri return Convert(group, interceptor); } - private static List Convert(ChoiceGroup group, Func interceptor) + private static List Convert(ChoiceGroup group, + Func interceptor) { var items = new List(); foreach (var member in group) @@ -90,16 +93,17 @@ private static List Convert(ChoiceGroup group, Func captured.OnClick(null, EventArgs.Empty))); + display.Checked, null, + display.Enabled ? (Action)(() => captured.OnClick(null, EventArgs.Empty)) : null)); } } @@ -107,6 +111,13 @@ private static List Convert(ChoiceGroup group, Func item.IsEnabled || item.Execute == null + ? item + : new DetailMenuItem(item.Label, isEnabled: false, item.IsChecked, item.Children, null); + // xCore marks the accelerator with a single '_' before the mnemonic; WinForms // translates it to '&'. Avalonia shows text raw, so strip only the first // marker: any later underscore is literal content. diff --git a/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs b/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs index 8080a49dd9..834c867b24 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Hosting/DetailObjectCommandExecutionTests.cs @@ -489,7 +489,8 @@ private IReadOnlyList BuildItemsWithOverrideInterceptor(string[] var build = typeof(RecordEditView).GetMethod("BuildOverrideCommandInterceptor", BindingFlags.Instance | BindingFlags.NonPublic); Assert.That(build, Is.Not.Null, "the override interceptor seam must exist"); - var interceptor = (Func)build.Invoke(m_view, new object[] { field }); + var interceptor = (Func)build.Invoke( + m_view, new object[] { field }); TestContext.WriteLine("interceptor built: " + (interceptor != null)); var window = m_propertyTable.GetValue("window"); return XCoreMenuBridge.CreateMenuItems(window, menuIds, interceptor);