-
-
Notifications
You must be signed in to change notification settings - Fork 42
LT-22691: Copy writing-system menu choices into the view override #1108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // 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 | ||
| { | ||
| /// <summary> | ||
| /// The commands the Avalonia detail view retargets away from mediator dispatch, as ordered | ||
| /// (matcher, builder) entries. <see cref="TryBuild"/> builds the replacement item from the | ||
| /// first matching entry; null leaves the command on its normal dispatch. | ||
| /// </summary> | ||
| internal sealed class OverrideCommandRegistry | ||
| { | ||
| private readonly List<KeyValuePair<Func<ChoiceBase, bool>, | ||
| Func<ChoiceBase, UIItemDisplayProperties, DetailMenuItem>>> _entries | ||
| = new List<KeyValuePair<Func<ChoiceBase, bool>, | ||
| Func<ChoiceBase, UIItemDisplayProperties, DetailMenuItem>>>(); | ||
|
|
||
| public void Add(string helpId, Func<ChoiceBase, UIItemDisplayProperties, DetailMenuItem> build) | ||
| => Add(c => string.Equals(c.HelpId, helpId, StringComparison.Ordinal), build); | ||
|
|
||
| /// <summary>Registers by matcher, for items that carry no command id.</summary> | ||
| public void Add(Func<ChoiceBase, bool> matches, | ||
| Func<ChoiceBase, UIItemDisplayProperties, DetailMenuItem> build) | ||
| => _entries.Add(new KeyValuePair<Func<ChoiceBase, bool>, | ||
| Func<ChoiceBase, UIItemDisplayProperties, DetailMenuItem>>(matches, build)); | ||
|
|
||
| public DetailMenuItem TryBuild(ChoiceBase choice, UIItemDisplayProperties display) | ||
| { | ||
| foreach (var entry in _entries) | ||
| { | ||
| if (entry.Key(choice)) | ||
| return entry.Value(choice, display); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -563,21 +563,28 @@ private ViewDefinitionOverride ResolveViewOverride(string className, string layo | |
| + "'; using the shipped definition.", error)); | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| private Func<ChoiceBase, DetailMenuItem> BuildOverrideCommandInterceptor(DetailField field) | ||
| private Func<ChoiceBase, UIItemDisplayProperties, DetailMenuItem> BuildOverrideCommandInterceptor( | ||
| DetailField field) | ||
| { | ||
| if (field == null || string.IsNullOrEmpty(field.ClassName) || string.IsNullOrEmpty(field.LayoutName) | ||
| || ViewOverrideStore == null) | ||
| { | ||
| 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, 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 | ||
| // already applied), so visibility checkmarks and move enablement reflect the live state. | ||
|
|
@@ -596,54 +603,161 @@ private Func<ChoiceBase, DetailMenuItem> 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. | ||
|
|
||
| return choice => | ||
| // Unknown/stale target: leave the field commands on the legacy path rather than | ||
| // guess. | ||
| if (location != null) | ||
| { | ||
| 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. | ||
| } | ||
| }; | ||
| registry.Add("CmdAlwaysVisible", | ||
| (c, d) => VisibilityItem(d, field, templateId, location, ViewVisibility.Always)); | ||
| registry.Add("CmdIfData", | ||
| (c, d) => VisibilityItem(d, field, templateId, location, ViewVisibility.IfData)); | ||
| registry.Add("CmdNormallyHidden", | ||
| (c, d) => VisibilityItem(d, field, templateId, location, ViewVisibility.Never)); | ||
| registry.Add("CmdDataTree-MoveFieldUp", | ||
| (c, d) => MoveItem(d, field, location, up: true)); | ||
| registry.Add("CmdDataTree-MoveFieldDown", | ||
| (c, d) => MoveItem(d, field, location, up: false)); | ||
| } | ||
|
|
||
| return registry.TryBuild; | ||
| } | ||
|
|
||
| // 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); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| 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); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| private DetailMenuItem WritingSystemItem(ChoiceBase choice, UIItemDisplayProperties display, | ||
| DetailField field) | ||
| { | ||
| var isListToggle = choice is ListPropertyChoice; | ||
| // 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defect: every writing-system change writes both stores, and the copy then shadows the original. This is the contingency on my approval of #1097.
var writingSystems = _setWritingSystems.TryGetValue(node.StableId, out var w)
? w : node.VisibleWritingSystems;
The user-visible consequence is on the WinForms side. Set writing systems there, .fwlayout is updated, Avalonia reads it -- and then renders an older json value instead, with no recency check and no diagnostic. Either resolution works for me:
What I do not think survives a bug report is "json always" shipping as it stands. I am not asking you to pick between one store and two inside this PR -- only to close this specific hole one way or the other.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I traced the persistence path before answering, and the load side doesn't connect the way this assumes. The Avalonia view never reads the file legacy writes to:
The So there is no value for the JSON to shadow, and the stated consequence — "set writing systems in WinForms, Avalonia renders an older JSON value instead" — cannot occur, because Avalonia was not showing the WinForms value before this PR either. That gap is real, predates both PRs, and is unchanged by them. This PR changes the opposite direction: before it, a writing-system change made in the new UI was invisible there. That is the bug it fixes. Whether the same intent should live in two stores is a separate, legitimate question — and it is the same sparse-patch-vs-whole-copy question as point 1 of your #1097 review, which is with Jason as a #964 foundation decision. #1111 is one answer to it. It gets settled there, not as a merge condition on a leaf PR, because whatever we choose governs every op kind. |
||
| CopyWritingSystemSelectionToOverride(field, isListToggle, before); | ||
| }); | ||
| } | ||
|
|
||
| // 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<string> 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<string> 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defect: one identifier, three comparers. Reusing legacy's own filter here is exactly right, and it is also what exposes the inconsistency.
So the same identifier is written case-sensitively, diffed case-insensitively, and read case-insensitively. In practice ids come from Pick one comparer and use it on all three paths. Ordinal matches legacy, which is the side we are calling authoritative, so that is my preference -- but ignore-case applied consistently would also be defensible. The split is the defect.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The observation is correct — I verified all four sites. It is also not something this PR introduced:
Unifying the comparer means changing shared code WinForms depends on and foundation code from #964, and it needs verification against existing projects. Please open an issue for it naming the three sites, and it can be scheduled on its own evidence. |
||
| 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<string> 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) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a defect -- keep this, whatever happens to the storage format.
This matcher overload is the important part of the file, and the reason deserves to be recorded where someone will find it:
ListPropertyChoicedoes not overrideHelpId(it returns the empty string,Choice.cs:538), so the per-writing-system toggles can never be matched by command id. Matching onParentPropertyis the right fix.The hardcoded HelpId switch this replaces carries the same blind spot everywhere it is used, so this is a general improvement rather than a writing-system detail. If this work gets reshaped onto a different store, this is the piece to carry across.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.