From f9160a9acd622e886b77587b4a3c62b6b178296b Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Tue, 22 Sep 2026 12:59:28 -0400 Subject: [PATCH 01/14] LT-22672: Let an environment already on an allomorph be retyped The Environments row could add and remove, never change. Raised by Mark K: an environment already on the field cannot be edited, and the five insert commands have no caret to insert at. This is the domain half. An environment is identified by its text with spaces stripped, and every case follows from that: - Text stripping to what the item already names leaves the reference alone and writes the new spelling onto the shared PhEnvironment, so every allomorph referencing it shows it. That reach is surprising and it is what PhoneEnvReferenceView.ConnectToRealCache does; matched deliberately rather than softened, so the two views cannot disagree about shared data. - Text stripping differently re-points the item, creating the target only when the project has no match. - Resolution prefers a match the field already carries that no OTHER item claims, which stops retyping one item from stealing another's environment. - A malformed string is staged and kept verbatim, never corrected. Addressed by item key, not position: PhoneEnv is a reference COLLECTION, so an index means only "wherever it sat when the row was composed". ConnectToRealCache sidesteps this by rebuilding the whole vector; addressing by key is the same guarantee for a single edit. IReferenceItemCreation becomes IReferenceTextEditing, since it now covers creating from typed text AND retyping an existing item -- the capability-area naming IStructuredTextEditing already uses. Eight references, all in-repo. Six tests, none trusted until falsified. Dropping the write-back fails the shared-rename case with the other allomorph still reading "/_#"; ignoring what other items claim fails the no-stealing case. Nothing in this area had a test before: PhoneEnvReferenceViewTests covers one unrelated edge case. Editable items in the view are the next commit; the insert commands need the caret seam and belong with the menu work in LT-22691. FwAvaloniaTests 748 passed, xWorksTests filter Avalonia 1648 passed, 0 failed. Co-Authored-By: Claude Opus 5 --- .../FwAvalonia/Detail/FwFieldControls.cs | 4 +- ...emCreation.cs => IReferenceTextEditing.cs} | 32 ++- .../Composer/ComposedDetailEditContext.cs | 17 +- .../Avalonia/Composer/DetailComposer.cs | 90 +++++++ .../AllomorphEnvironmentAndEditingTests.cs | 8 +- .../Composer/EnvironmentItemEditingTests.cs | 224 ++++++++++++++++++ 6 files changed, 362 insertions(+), 13 deletions(-) rename Src/Common/FwAvalonia/Detail/{IReferenceItemCreation.cs => IReferenceTextEditing.cs} (52%) create mode 100644 Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index ae7f894921..38ba66798c 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -1328,7 +1328,7 @@ private static Action FindSink(FlyoutBase flyout) /// legacy STORES an invalid environment and marks it rather than refusing it. /// /// CREATE-ON-TYPE (opt-in): a row whose edit context implements for it also lets the user mint a target object by typing + /// cref="IReferenceTextEditing"/> for it also lets the user mint a target object by typing /// into the picker's filter box -- the list offers a create row when the text matches /// nothing. This is what makes an environments row reach an environment the project does not /// own yet, which the picker alone cannot do. Every other vector row passes allowCreate: @@ -1517,7 +1517,7 @@ public FwReferenceVectorField( // object offers it (environments find-or-create a PhEnvironment from the typed // string). Every other vector row passes allowCreate: false and behaves exactly as // before. - var creation = editContext as IReferenceItemCreation; + var creation = editContext as IReferenceTextEditing; var canCreate = creation != null && creation.CanCreateReferenceItem(field); var picker = new FwOptionChooser(field.Options, field.SearchOptions, automationId, field.Items.Select(i => i.Key), multiSelect: true, allowCreate: canCreate, diff --git a/Src/Common/FwAvalonia/Detail/IReferenceItemCreation.cs b/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs similarity index 52% rename from Src/Common/FwAvalonia/Detail/IReferenceItemCreation.cs rename to Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs index 779fd53087..1ca647b82d 100644 --- a/Src/Common/FwAvalonia/Detail/IReferenceItemCreation.cs +++ b/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs @@ -5,11 +5,12 @@ namespace SIL.FieldWorks.Common.FwAvalonia.Detail { /// - /// The optional create-from-typed-text capability of a reference field, kept off the core - /// so only a context that can actually mint a target object - /// carries it. A caller acquires it with ctx as IReferenceItemCreation and treats a - /// null result as "this row picks from the list only", exactly as is acquired. + /// The optional typed-text capability of a reference field -- creating a target object from + /// what the user types, and re-pointing an existing item at what they type over it. Kept off + /// the core so only a context that can actually reconcile + /// text against the domain carries it. A caller acquires it with + /// ctx as IReferenceTextEditing and treats a null result as "this row picks from the + /// list only", exactly as is acquired. /// /// It exists because some legacy reference slices are BOTH a chooser and a typed editor. /// Environments is the case in hand: PhoneEnvReferenceLauncher opens a @@ -19,7 +20,7 @@ namespace SIL.FieldWorks.Common.FwAvalonia.Detail /// PhEnvironment or creating one. A picker alone cannot reach an environment the /// project does not have yet. /// - public interface IReferenceItemCreation + public interface IReferenceTextEditing { /// /// Whether accepts creation from typed text. Drives whether the @@ -41,5 +42,24 @@ public interface IReferenceItemCreation /// discard what the user typed. /// bool TryCreateAndAddReferenceItem(DetailField field, string text); + + /// + /// Re-points the item named by at whatever + /// names, staging the change. Returns false -- without opening + /// the session -- for a field that cannot do this, or a key the field does not carry. + /// + /// Addressed by key rather than position because these rows are reference COLLECTIONS. + /// PhoneEnv is unordered, so an index means only "wherever it sat when composed". + /// + /// Reconciliation belongs to the domain, and is NOT simply "find or create". An + /// environment is identified by its text with spaces stripped. Text stripping to what + /// the item already names leaves the reference alone and RENAMES the shared object, + /// which every field referencing it then shows. Text stripping differently re-points + /// the item, creating the target only when the project has none. + /// + /// Validity is not a precondition, for the same reason it is not on creation: the + /// value is staged and annotated, never corrected or discarded. + /// + bool TrySetReferenceItemText(DetailField field, string itemKey, string text); } } diff --git a/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs b/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs index 73c117fec6..345a362a64 100644 --- a/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs +++ b/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs @@ -29,6 +29,10 @@ public sealed class FieldEditHandler // presence is what makes the picker offer a create row. public Func ReferenceCreate; + // Set only by a field whose items can be retyped in place (environments); (item key, + // text) -> staged. Reconciliation lives in the handler, not here. + public Func ReferenceSetText; + /// (item key, forward) -> moved; null on rows whose items cannot be /// reordered. public Func ReferenceMove; @@ -45,7 +49,7 @@ public sealed class FieldEditHandler /// (one shared session lifecycle + required-lexeme validation). /// public sealed class ComposedDetailEditContext : DetailEditContextBase, IStructuredTextEditing, - IReferenceItemCreation + IReferenceTextEditing { // One handler per composed field, keyed by StableId; a null delegate slot means the field's kind // does not support that gesture (rejected like an unknown field). Replaces the former nine parallel @@ -114,6 +118,17 @@ public bool TryCreateAndAddReferenceItem(DetailField field, string text) return Stage(() => creator(text), FieldLabelFor(field)); } + public bool TrySetReferenceItemText(DetailField field, string itemKey, string text) + { + var setter = Handler(field)?.ReferenceSetText; + if (setter == null || string.IsNullOrWhiteSpace(itemKey) + || string.IsNullOrWhiteSpace(text)) + { + return false; + } + return Stage(() => setter(itemKey, text), FieldLabelFor(field)); + } + public override bool TryAddReferenceItem(DetailField field, string optionKey) { var setter = Handler(field)?.ReferenceAdd; diff --git a/Src/xWorks/Avalonia/Composer/DetailComposer.cs b/Src/xWorks/Avalonia/Composer/DetailComposer.cs index d748be529a..d1fe167dce 100644 --- a/Src/xWorks/Avalonia/Composer/DetailComposer.cs +++ b/Src/xWorks/Avalonia/Composer/DetailComposer.cs @@ -1692,7 +1692,97 @@ private void AddGenericReferenceVector(ViewNode node, ICmObject obj, int depth, _sda.Replace(hvo, flid, size, size, new[] { env.Hvo }, 1); return true; }; + + HandlerFor(stableId).ReferenceSetText = (itemKey, text) => + { + var position = PositionOfItem(hvo, flid, itemKey); + if (position < 0) + return false; + + var target = ResolveEnvironmentForItem(hvo, flid, position, StripSpaces(text)) + ?? FindOrCreateEnvironment(text); + if (target == null) + return false; + + // The typed string is written onto the resolved environment whether or + // not + // it moved, so a re-spelling reaches every field referencing it -- + // writing system as much as spelling. + target.StringRepresentation = + TsStringUtils.MakeString(text, _cache.DefaultVernWs); + + var current = _sda.get_VecItem(hvo, flid, position); + if (target.Hvo != current) + _sda.Replace(hvo, flid, position, position + 1, new[] { target.Hvo }, 1); + return true; + }; + } + } + + // Where the item this key names currently sits. These rows are reference + // COLLECTIONS, so a position is only meaningful for the length of one write. + private int PositionOfItem(int hvo, int flid, string itemKey) + { + if (!Guid.TryParse(itemKey, out var guid)) + return -1; + var size = _sda.get_VecSize(hvo, flid); + for (var i = 0; i < size; i++) + { + var item = _cache.ServiceLocator.ObjectRepository.GetObject( + _sda.get_VecItem(hvo, flid, i)); + if (item.Guid == guid) + return i; + } + + return -1; + } + + /// + /// The environment an item should point at once its text becomes + /// (already space-stripped): one this field already + /// references and no OTHER item claims, else one from the project inventory, else + /// the first match regardless of who claims it. Null when the project has no match + /// at all, which is the caller's signal to create one. + /// + /// Preferring what the field already references is what stops retyping one item from + /// stealing the environment another item is using. + /// + private IPhEnvironment ResolveEnvironmentForItem(int hvo, int flid, int index, + string wanted) + { + var size = _sda.get_VecSize(hvo, flid); + var claimedByOthers = new HashSet(); + for (var i = 0; i < size; i++) + if (i != index) + claimedByOthers.Add(_sda.get_VecItem(hvo, flid, i)); + + for (var i = 0; i < size; i++) + { + var itemHvo = _sda.get_VecItem(hvo, flid, i); + if (claimedByOthers.Contains(itemHvo)) + continue; + if (_cache.ServiceLocator.ObjectRepository.GetObject(itemHvo) is IPhEnvironment env + && StripSpaces(env.StringRepresentation?.Text) == wanted) + { + return env; + } + } + + var inventory = _cache.LanguageProject.PhonologicalDataOA?.EnvironmentsOS; + if (inventory == null) + return null; + IPhEnvironment firstMatch = null; + foreach (var env in inventory) + { + if (StripSpaces(env.StringRepresentation?.Text) != wanted) + continue; + if (firstMatch == null) + firstMatch = env; + if (!claimedByOthers.Contains(env.Hvo)) + return env; } + + return firstMatch; } /// diff --git a/Src/xWorks/xWorksTests/Avalonia/Composer/AllomorphEnvironmentAndEditingTests.cs b/Src/xWorks/xWorksTests/Avalonia/Composer/AllomorphEnvironmentAndEditingTests.cs index f992c8b2b8..09fdb0fdb9 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Composer/AllomorphEnvironmentAndEditingTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Composer/AllomorphEnvironmentAndEditingTests.cs @@ -21,8 +21,8 @@ namespace SIL.FieldWorks.XWorks /// project's existing environments, and its inline PhoneEnvReferenceView lets the user type a /// new environment string that ConnectToRealCache reconciles into the project /// (find-or-create, matching with spaces stripped). So the Avalonia row composes as an - /// ordinary reference vector -- the chooser half -- whose edit context also offers - /// IReferenceItemCreation. + /// ordinary reference vector -- the chooser half -- whose edit context also offers the + /// IReferenceTextEditing capability. /// /// Both data states are covered: the empty project and the populated one compose through /// different branches, so a fixture without environments can pass for the wrong reason. @@ -296,7 +296,7 @@ private struct ComposedEnvironments { public DetailField Row; public IDetailEditContext Context; - public IReferenceItemCreation Creation; + public IReferenceTextEditing Creation; } private ComposedEnvironments ComposeEnvironments() @@ -307,7 +307,7 @@ private ComposedEnvironments ComposeEnvironments() Row = composed.Model.Fields.FirstOrDefault( f => f.Field == "PhoneEnv" && f.ObjectHvo == m_allomorph.Hvo), Context = composed.EditContext, - Creation = composed.EditContext as IReferenceItemCreation + Creation = composed.EditContext as IReferenceTextEditing }; } diff --git a/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs b/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs new file mode 100644 index 0000000000..700c47bf45 --- /dev/null +++ b/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs @@ -0,0 +1,224 @@ +// 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.Linq; +using NUnit.Framework; +using SIL.FieldWorks.Common.FwAvalonia.Detail; +using SIL.LCModel; +using SIL.LCModel.Core.Text; +using SIL.LCModel.Infrastructure; + +namespace SIL.FieldWorks.XWorks +{ + /// + /// Retyping an environment that is already on an allomorph. An environment's IDENTITY is its + /// text with spaces stripped, and everything here follows from that one fact: text that + /// strips the same leaves the reference alone and RENAMES the shared object, text that strips + /// differently re-points the reference and creates the target only when the project has none. + /// + /// PhoneEnvReferenceView.ConnectToRealCache is the behaviour being matched, and no test of + /// its own pins any of it -- these are the first. Each was confirmed by suppressing the + /// behaviour and watching it fail. + /// + [TestFixture] + public class EnvironmentItemEditingTests : MemoryOnlyBackendProviderTestBase + { + private ILexEntry m_entry; + private IMoStemAllomorph m_allomorph; + private IMoStemAllomorph m_otherAllomorph; + + public override void TestSetup() + { + base.TestSetup(); + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + m_entry = Cache.ServiceLocator.GetInstance().Create(); + var lexemeForm = Cache.ServiceLocator.GetInstance().Create(); + m_entry.LexemeFormOA = lexemeForm; + lexemeForm.Form.set_String(Cache.DefaultVernWs, + TsStringUtils.MakeString("barigi", Cache.DefaultVernWs)); + + m_allomorph = MakeAllomorph("barigi-one"); + m_otherAllomorph = MakeAllomorph("barigi-two"); + }); + } + + public override void TestTearDown() + { + // Editing mints environments too, and NonUndoableUnitOfWorkHelper bypasses UndoAll. + var inventory = Cache.LanguageProject.PhonologicalDataOA?.EnvironmentsOS; + if (inventory != null && inventory.Count > 0) + { + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, + () => { foreach (var env in inventory.ToList()) env.Delete(); }); + } + base.TestTearDown(); + } + + private IMoStemAllomorph MakeAllomorph(string form) + { + var allomorph = Cache.ServiceLocator.GetInstance().Create(); + m_entry.AlternateFormsOS.Add(allomorph); + allomorph.Form.set_String(Cache.DefaultVernWs, + TsStringUtils.MakeString(form, Cache.DefaultVernWs)); + return allomorph; + } + + private IPhEnvironment GiveProjectAnEnvironment(string representation) + { + IPhEnvironment env = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + env = Cache.ServiceLocator.GetInstance().Create(); + Cache.LanguageProject.PhonologicalDataOA.EnvironmentsOS.Add(env); + env.StringRepresentation = TsStringUtils.MakeString( + representation, Cache.DefaultVernWs); + }); + return env; + } + + private void Attach(IMoStemAllomorph allomorph, IPhEnvironment env) + => NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, + () => allomorph.PhoneEnvRC.Add(env)); + + private int EnvironmentCount + => Cache.LanguageProject.PhonologicalDataOA?.EnvironmentsOS.Count ?? 0; + + // Retypes the item naming 'env' on m_allomorph, and commits. + private bool Retype(IPhEnvironment env, string text) + { + var composed = DetailComposer.Compose(m_entry, Cache, showHiddenFields: true); + var row = composed.Model.Fields.Single( + f => f.Field == "PhoneEnv" && f.ObjectHvo == m_allomorph.Hvo); + var editing = (IReferenceTextEditing)composed.EditContext; + + var staged = editing.TrySetReferenceItemText(row, env.Guid.ToString(), text); + if (staged) + composed.EditContext.Commit(); + return staged; + } + + /// + /// Case 1, the surprising one. Respacing makes no new environment and does not move the + /// reference -- it renames the shared object, so every OTHER allomorph referencing it + /// shows the new spelling too. Asserted from the second allomorph, which is what makes + /// the project-wide reach visible. + /// + [Test] + public void RetypingToTheSameStrippedText_RenamesTheSharedEnvironment() + { + var shared = GiveProjectAnEnvironment("/_#"); + Attach(m_allomorph, shared); + Attach(m_otherAllomorph, shared); + var before = EnvironmentCount; + + Assert.That(Retype(shared, "/ _ #"), Is.True, "the edit staged"); + + Assert.That(EnvironmentCount, Is.EqualTo(before), + "the identity did not change, so nothing was created"); + Assert.That(m_allomorph.PhoneEnvRC.Single(), Is.EqualTo(shared), + "and the reference did not move"); + Assert.That(m_otherAllomorph.PhoneEnvRC.Single().StringRepresentation.Text, + Is.EqualTo("/ _ #"), + "the shared environment was renamed, so the OTHER allomorph shows it too -- this " + + "is the project-wide reach of an edit that looks local"); + } + + /// Case 2: a different identity that the project already has. + [Test] + public void RetypingToAnExistingEnvironment_MovesTheReference_AndKeepsTheOldOne() + { + var original = GiveProjectAnEnvironment("/_#"); + var other = GiveProjectAnEnvironment("/_a"); + Attach(m_allomorph, original); + var before = EnvironmentCount; + + Assert.That(Retype(original, "/_a"), Is.True); + + Assert.That(EnvironmentCount, Is.EqualTo(before), "an existing match is reused"); + Assert.That(m_allomorph.PhoneEnvRC.Single(), Is.EqualTo(other), + "the reference moved to the environment the typed text names"); + Assert.That(original.IsValidObject, Is.True, + "and the one it left is untouched -- it stays in the project inventory"); + Assert.That(original.StringRepresentation.Text, Is.EqualTo("/_#")); + } + + /// Case 3: an identity the project does not have yet. + [Test] + public void RetypingToAnUnknownEnvironment_CreatesItAndMovesTheReference() + { + var original = GiveProjectAnEnvironment("/_#"); + Attach(m_allomorph, original); + var before = EnvironmentCount; + + Assert.That(Retype(original, "/_zz"), Is.True); + + Assert.That(EnvironmentCount, Is.EqualTo(before + 1), "the project gained one"); + Assert.That(m_allomorph.PhoneEnvRC.Single().StringRepresentation.Text, + Is.EqualTo("/_zz")); + Assert.That(original.StringRepresentation.Text, Is.EqualTo("/_#"), + "the environment it left keeps its own text"); + } + + /// + /// Case 4: retyping one item must not steal the environment another item on the same + /// field is using. Resolution prefers a match this field already carries that no OTHER + /// item claims, which is the rule that keeps the two apart. + /// + [Test] + public void RetypingOneItem_DoesNotStealTheEnvironmentAnotherItemUses() + { + var first = GiveProjectAnEnvironment("/_#"); + var second = GiveProjectAnEnvironment("/ _ a"); + var third = GiveProjectAnEnvironment("/_a"); + Attach(m_allomorph, first); + Attach(m_allomorph, second); + + Assert.That(Retype(first, "/_a"), Is.True); + + Assert.That(m_allomorph.PhoneEnvRC, Does.Contain(second), + "the item that already named that identity keeps its environment"); + Assert.That(m_allomorph.PhoneEnvRC, Does.Contain(third), + "and the retyped item took the other match rather than colliding"); + Assert.That(m_allomorph.PhoneEnvRC.Count, Is.EqualTo(2), + "still two items, on two distinct environments"); + } + + /// + /// Case 5: malformed text is staged and kept verbatim. Legacy annotates rather than + /// blocking, so nothing here may correct or discard what the user typed. + /// + [Test] + public void RetypingToAMalformedEnvironment_IsAcceptedAndKeptVerbatim() + { + var original = GiveProjectAnEnvironment("/_#"); + Attach(m_allomorph, original); + + Assert.That(Retype(original, "/ _ ["), Is.True, + "a malformed environment is still staged -- rejecting it would discard typing"); + + Assert.That(m_allomorph.PhoneEnvRC.Single().StringRepresentation.Text, + Is.EqualTo("/ _ ["), + "and it is stored exactly as typed, not corrected"); + } + + [Test] + public void RetypingAnItemTheFieldDoesNotCarry_IsRejected_WithoutOpeningASession() + { + var attached = GiveProjectAnEnvironment("/_#"); + var elsewhere = GiveProjectAnEnvironment("/_b"); + Attach(m_allomorph, attached); + + var composed = DetailComposer.Compose(m_entry, Cache, showHiddenFields: true); + var row = composed.Model.Fields.Single( + f => f.Field == "PhoneEnv" && f.ObjectHvo == m_allomorph.Hvo); + var editing = (IReferenceTextEditing)composed.EditContext; + + Assert.That(editing.TrySetReferenceItemText(row, elsewhere.Guid.ToString(), "/_c"), + Is.False, "the key names no item of this field"); + Assert.That(composed.EditContext.IsOpen, Is.False, + "and a rejected edit must not leave a session open"); + } + } +} From 3e92f85b54b8270aa85bda8bd10685309763d2c0 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Tue, 22 Sep 2026 13:52:11 -0400 Subject: [PATCH 02/14] LT-22672: Render a retypable vector row's items as editors The view half of retyping an environment. A row whose edit context reports it can reconcile typed text renders its items as text boxes instead of labels, so an item already on the field can be changed and not only removed and re-added. Every other vector row is untouched: the capability is asked for per field, and a row that does not claim it keeps the labels it had. Staged when an edit FINISHES -- focus leaves, or Enter -- not per keystroke, which is the opposite of how the text rows work and is deliberate. Each stage reconciles against the project, so staging every keystroke of "/_zz" would leave environments behind for "/", "/_" and "/_z". The handler sits on the box, so it runs before the view's focus-loss autosave commits. Three knock-on decisions: - Backspace and Delete no longer remove the focused item on such a row; on an editor those keys are text editing. Removal stays on the item menu. - The selection highlight paints only a label. An editor shows its own focus, and painting its background would fight its chrome. - Editors are tab stops, so the keyboard walks the items; labels stay out of the tab order as before. Five tests, falsified by forcing the row to render labels: the editor test fails on the missing box and the three staging tests follow. The helper that finds an editor now asserts rather than returning null, so that failure reads as "rendered no editor" instead of a NullReferenceException further down. FwAvaloniaTests 753 passed, xWorksTests filter Avalonia 1648 passed, 0 failed. Co-Authored-By: Claude Opus 5 --- .../FwAvalonia/Detail/FwFieldControls.cs | 102 ++++++++-- .../Detail/IReferenceTextEditing.cs | 7 + .../Detail/RetypableVectorItemTests.cs | 188 ++++++++++++++++++ .../Composer/ComposedDetailEditContext.cs | 3 + 4 files changed, 279 insertions(+), 21 deletions(-) create mode 100644 Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index 38ba66798c..1b3e148f7a 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -1342,7 +1342,7 @@ public sealed class FwReferenceVectorField : StackPanel, IHoverAffordanceProvide // gear click, and the option flyout, so a recycled vector cell releases every closure. private readonly List _teardown = new List(); private readonly IReadOnlyList _items; - private readonly List _itemBlocks = new List(); + private readonly List _itemBlocks = new List(); private int _selectedIndex = -1; private bool _disposed; @@ -1375,25 +1375,74 @@ public FwReferenceVectorField( _items = field.Items; var editable = editContext != null && field.IsEditable; + // A row whose domain can reconcile typed text renders its items as editors rather + // than labels, so an item already on the field can be changed and not only replaced. + var textEditing = editContext as IReferenceTextEditing; + var retypable = editable && textEditing != null + && textEditing.CanEditReferenceItemText(field); for (var index = 0; index < field.Items.Count; index++) { var item = field.Items[index]; - var text = new TextBlock + Control text; + if (retypable) { - Text = item.Name, - VerticalAlignment = VerticalAlignment.Center, - Margin = FwAvaloniaDensity.TrailingItemGap, - // 14.2: a null background only hit-tests the glyphs -- the whole item must - // take - // the right-click or the Remove flyout only opens over ink. - Background = FwAvaloniaDensity.TransparentBrush - }; + var box = new TextBox + { + Text = item.Name, + VerticalAlignment = VerticalAlignment.Center, + Margin = FwAvaloniaDensity.TrailingItemGap, + Padding = FwAvaloniaDensity.EditorPadding, + MinWidth = 0, + MinHeight = 0 + }; + var itemKey = item.Key; + // Staged when the edit FINISHES, not per keystroke: each stage reconciles + // against the project, so "/", "/_", "/_a" would leave two junk objects + // behind. This handler runs before the view's autosave. + var original = item.Name; + Action commitText = () => + { + if (box.Text == original) + return; + if (textEditing.TrySetReferenceItemText(field, itemKey, box.Text)) + gestureCompleted?.Invoke(); + }; + EventHandler commitOnBlur = + (s2, e2) => commitText(); + box.LostFocus += commitOnBlur; + EventHandler commitOnEnter = (s2, e2) => + { + if (e2.Key != Key.Enter) + return; + e2.Handled = true; + commitText(); + }; + box.KeyDown += commitOnEnter; + _teardown.Add(() => + { + box.LostFocus -= commitOnBlur; + box.KeyDown -= commitOnEnter; + }); + text = box; + } + else + { + text = new TextBlock + { + Text = item.Name, + VerticalAlignment = VerticalAlignment.Center, + Margin = FwAvaloniaDensity.TrailingItemGap, + // 14.2: a null background only hit-tests the glyphs -- the whole item + // must take the right-click, or the Remove flyout only opens over ink. + Background = FwAvaloniaDensity.TransparentBrush + }; + } AutomationProperties.SetAutomationId(text, ItemAutomationId(automationId, item.Key)); // Any button selects, so a right-click's menu acts on the item under the pointer; // focus selects too. Items are focusable (a click focuses one) but not tab stops. var itemIndex = index; text.Focusable = true; - KeyboardNavigation.SetIsTabStop(text, false); + KeyboardNavigation.SetIsTabStop(text, retypable); EventHandler focusSelect = (s, e) => SelectItem(itemIndex); text.GotFocus += focusSelect; EventHandler select = (s, e) => @@ -1417,16 +1466,26 @@ public FwReferenceVectorField( }); if (item.HasValidationMessage) { - // Legacy draws a red squiggle. Avalonia has no wavy decoration, so this is - // colour PLUS an underline -- colour alone would carry the whole signal. - text.Foreground = FwAvaloniaDensity.ValidationErrorBrush; - text.TextDecorations = TextDecorations.Underline; + // No wavy underline exists here, so this is colour PLUS an underline -- + // colour alone would carry the whole signal. An editor takes the colour + // only; its chrome owns the rest. + if (text is TextBlock label) + { + label.Foreground = FwAvaloniaDensity.ValidationErrorBrush; + label.TextDecorations = TextDecorations.Underline; + } + else if (text is TextBox editor) + { + editor.Foreground = FwAvaloniaDensity.ValidationErrorBrush; + } + ToolTip.SetTip(text, item.ValidationMessage); AutomationProperties.SetHelpText(text, item.ValidationMessage); } - if (editable) + // Backspace and Delete remove the focused item -- but on an editor those keys + // are text editing, and removal stays on the item menu. + if (editable && !retypable) { - // Backspace or Delete removes the focused item. EventHandler keyRemove = (s, e) => { if (e.Key != Key.Back && e.Key != Key.Delete) @@ -1655,11 +1714,12 @@ private void SelectItem(int index) { if (index == _selectedIndex) return; - if (_selectedIndex >= 0) - _itemBlocks[_selectedIndex].Background = FwAvaloniaDensity.TransparentBrush; + // An editable item shows its own focus, so only a read-only one is painted. + if (_selectedIndex >= 0 && _itemBlocks[_selectedIndex] is TextBlock previous) + previous.Background = FwAvaloniaDensity.TransparentBrush; _selectedIndex = index; - if (index >= 0) - _itemBlocks[index].Background = FwAvaloniaDensity.SelectedRowBrush; + if (index >= 0 && _itemBlocks[index] is TextBlock current) + current.Background = FwAvaloniaDensity.SelectedRowBrush; SelectionChanged?.Invoke(this, EventArgs.Empty); } diff --git a/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs b/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs index 1ca647b82d..d6a151bf43 100644 --- a/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs +++ b/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs @@ -43,6 +43,13 @@ public interface IReferenceTextEditing /// bool TryCreateAndAddReferenceItem(DetailField field, string text); + /// + /// Whether lets its existing items be retyped, which decides + /// whether the row renders them as editable text at all. Independent of any particular + /// item, and of what the user has typed. + /// + bool CanEditReferenceItemText(DetailField field); + /// /// Re-points the item named by at whatever /// names, staging the change. Returns false -- without opening diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs new file mode 100644 index 0000000000..0fbb860e96 --- /dev/null +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs @@ -0,0 +1,188 @@ +// 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.Collections.Generic; +using System.Linq; +using Avalonia.Automation; +using Avalonia.Controls; +using Avalonia.Headless.NUnit; +using Avalonia.Input; +using Avalonia.Threading; +using Avalonia.VisualTree; +using NUnit.Framework; +using SIL.FieldWorks.Common.FwAvalonia.Detail; +using SIL.FieldWorks.Common.FwAvalonia.ViewDefinition; + +namespace FwAvaloniaTests.Detail +{ + /// + /// A reference-vector row whose domain can reconcile typed text renders its items as editors, + /// so an item already on the field can be changed rather than only added and removed. Every + /// other vector row keeps the read-only items it has always had, which most of these pin. + /// + [TestFixture] + public class RetypableVectorItemTests + { + /// Records what the row stages, and whether it claims the capability. + private sealed class FakeTextEditing : IDetailEditContext, IReferenceTextEditing + { + public bool Retypable = true; + public bool SetResult = true; + public readonly List<(string Key, string Text)> Edits = new List<(string, string)>(); + + public bool CanEditReferenceItemText(DetailField field) => Retypable; + + public bool TrySetReferenceItemText(DetailField field, string itemKey, string text) + { + Edits.Add((itemKey, text)); + return SetResult; + } + + public bool CanCreateReferenceItem(DetailField field) => false; + public bool TryCreateAndAddReferenceItem(DetailField field, string text) => false; + + public bool IsOpen => false; + public bool TrySetText(DetailField f, string ws, string v) => false; + public bool TrySetRichText(DetailField f, string ws, DetailRichTextValue v) => false; + public bool TrySetOption(DetailField f, string key) => false; + public bool TryAddReferenceItem(DetailField f, string key) => false; + public bool TryRemoveReferenceItem(DetailField f, string key) => true; + public bool TryMoveReferenceItem(DetailField f, string key, bool forward) => false; + public IReadOnlyList Validate() => new List(); + public void Commit() { } + public void Cancel() { } + } + + private static DetailField Row() => new DetailField( + "MoStemAllomorph/x/#0", "Environments", "PhoneEnv", null, + DetailFieldKind.ReferenceVector, EditorClassification.Known, "PhoneEnv", null, + HostRouting.Inherit, null, null, null, isEditable: true, + items: new List + { + new DetailChoiceOption("e1", "/_#"), + new DetailChoiceOption("e2", "/_a") + }); + + private static (FwReferenceVectorField Row, Window Window) Show( + FakeTextEditing context, System.Action gestureCompleted = null) + { + var row = new FwReferenceVectorField(Row(), "PhoneEnv", context, gestureCompleted); + var window = new Window { Content = row, Width = 480, Height = 200 }; + window.Show(); + Dispatcher.UIThread.RunJobs(); + return (row, window); + } + + // Fails with the reason rather than handing back null, so a row that rendered labels + // says so instead of surfacing as a NullReferenceException three lines later. + private static TextBox Editor(FwReferenceVectorField row, string key) + { + var box = row.GetVisualDescendants().OfType().FirstOrDefault( + b => AutomationProperties.GetAutomationId(b) + == FwReferenceVectorField.ItemAutomationId("PhoneEnv", key)); + Assert.That(box, Is.Not.Null, "the row rendered no editor for item '" + key + "'"); + return box; + } + + [AvaloniaTest] + public void ARetypableRow_RendersItsItemsAsEditors() + { + var (row, _) = Show(new FakeTextEditing()); + + Assert.That(Editor(row, "e1").Text, Is.EqualTo("/_#"), + "an item the domain can reconcile has to be typeable and show its own text, or " + + "it can only be removed and re-added"); + } + + [AvaloniaTest] + public void ARowThatCannotRetype_KeepsReadOnlyItems() + { + var (row, _) = Show(new FakeTextEditing { Retypable = false }); + + Assert.That(row.GetVisualDescendants().OfType(), Is.Empty, + "every other vector row in the app must be untouched by this"); + Assert.That(row.GetVisualDescendants().OfType().Select(t => t.Text), + Does.Contain("/_#")); + } + + /// + /// Staged when the edit FINISHES, not per keystroke. Each stage reconciles the text + /// against the project, so staging every keystroke of "/_zz" would leave junk + /// environments behind for "/", "/_" and "/_z". + /// + [AvaloniaTest] + public void TypingAlone_StagesNothing_UntilTheEditIsFinished() + { + var context = new FakeTextEditing(); + var (row, _) = Show(context); + var box = Editor(row, "e1"); + + box.Text = "/_zz"; + Dispatcher.UIThread.RunJobs(); + + Assert.That(context.Edits, Is.Empty, + "a stage per keystroke would mint an environment per keystroke"); + + box.RaiseEvent(new KeyEventArgs + { + RoutedEvent = InputElement.KeyDownEvent, + Key = Key.Enter + }); + Dispatcher.UIThread.RunJobs(); + + Assert.That(context.Edits, Is.EqualTo(new[] { ("e1", "/_zz") }), + "finishing the edit stages it once, against the item's own key"); + } + + [AvaloniaTest] + public void FinishingAnUnchangedEdit_StagesNothing() + { + var context = new FakeTextEditing(); + var (row, _) = Show(context); + var box = Editor(row, "e1"); + + box.RaiseEvent(new KeyEventArgs + { + RoutedEvent = InputElement.KeyDownEvent, + Key = Key.Enter + }); + Dispatcher.UIThread.RunJobs(); + + Assert.That(context.Edits, Is.Empty, + "text that did not change must not reconcile, which would rewrite the shared " + + "environment for every field referencing it"); + } + + [AvaloniaTest] + public void AStagedEdit_CompletesTheGesture_AndARefusedOneDoesNot() + { + var gestures = 0; + var context = new FakeTextEditing(); + var (row, _) = Show(context, () => gestures++); + Editor(row, "e1").Text = "/_zz"; + Editor(row, "e1").RaiseEvent(new KeyEventArgs + { + RoutedEvent = InputElement.KeyDownEvent, + Key = Key.Enter + }); + Dispatcher.UIThread.RunJobs(); + Assert.That(gestures, Is.EqualTo(1), "a staged edit commits and re-shows"); + + context.SetResult = false; + var refused = new FakeTextEditing { SetResult = false }; + var (other, _) = Show(refused, () => gestures++); + Editor(other, "e2").Text = "/_yy"; + Editor(other, "e2").RaiseEvent(new KeyEventArgs + { + RoutedEvent = InputElement.KeyDownEvent, + Key = Key.Enter + }); + Dispatcher.UIThread.RunJobs(); + + Assert.That(gestures, Is.EqualTo(1), + "a refused edit completes no gesture, so the row is not re-shown over an edit " + + "the domain did not take"); + } + } +} diff --git a/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs b/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs index 345a362a64..384ebca402 100644 --- a/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs +++ b/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs @@ -118,6 +118,9 @@ public bool TryCreateAndAddReferenceItem(DetailField field, string text) return Stage(() => creator(text), FieldLabelFor(field)); } + public bool CanEditReferenceItemText(DetailField field) + => Handler(field)?.ReferenceSetText != null; + public bool TrySetReferenceItemText(DetailField field, string itemKey, string text) { var setter = Handler(field)?.ReferenceSetText; From 09acaa0b51f1480cbeb56c993fac5aa8f584280e Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Tue, 22 Sep 2026 16:11:50 -0400 Subject: [PATCH 03/14] LT-22672: Add a typed slot so a new environment needs no chooser PhoneEnvReferenceView keeps an always-present empty line at the end of its view, so a new environment is typed rather than chosen. The Avalonia row had only the "+" picker. This adds the slot; the picker stays, because it is the other route and not a worse one. No new domain work: the slot commits through TryCreateAndAddReferenceItem, which already exists and is what the picker's create row calls. Committed when the edit finishes rather than per keystroke, for the same reason retyping is -- each commit reconciles against the project. It also gives an empty row somewhere to start. Until now an Environments row with nothing on it offered only the "+" button and a thin separator bar. The slot names no item, so focusing it clears the row's current one. Without that, a menu request raised from the slot would carry whichever item was clicked beforehand and act on that instead -- reproduced by suppressing the clear, which fails the new test with 'Expected: null, But was: "e1"'. Its own right-click menu stays inert for now. The host resolves an item menu through the selected key and returns early without one, so nothing opens. Making a caret-bearing, item-less request build the insert commands belongs with the menu work in LT-22691. Five more tests, falsified by suppressing the clear and by forcing the slot off. FwAvaloniaTests 758 passed, xWorksTests filter Avalonia 1648 passed, 0 failed. Co-Authored-By: Claude Opus 5 --- .../FwAvalonia/Detail/FwFieldControls.cs | 58 +++++++++- .../Detail/RetypableVectorItemTests.cs | 103 +++++++++++++++++- 2 files changed, 154 insertions(+), 7 deletions(-) diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index 1b3e148f7a..c81e24886f 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -1544,11 +1544,61 @@ public FwReferenceVectorField( return; } - // The legacy empty add slot: a trailing bar (added above for the last item; one leads - // the launcher when the vector is empty) plus the chooser launcher. + // A trailing bar (added above for the last item; one leads the launcher when the + // vector is empty) plus the chooser launcher. if (field.Items.Count == 0) AddSeparatorBar(); + var canCreate = textEditing != null && textEditing.CanCreateReferenceItem(field); + if (canCreate) + { + // Typing a new one never has to go through the chooser: + // PhoneEnvReferenceView keeps an always-present empty line at the end for + // exactly this, and it is where an empty row offers somewhere to start. + var newItem = new TextBox + { + VerticalAlignment = VerticalAlignment.Center, + Margin = FwAvaloniaDensity.TrailingItemGap, + Padding = FwAvaloniaDensity.EditorPadding, + MinWidth = FwAvaloniaDensity.PickerMinWidth, + MinHeight = 0, + Watermark = FwAvaloniaStrings.AddItem + }; + AutomationProperties.SetAutomationId(newItem, automationId + ".New"); + AutomationProperties.SetName(newItem, FwAvaloniaStrings.AddItem); + Action commitNew = () => + { + var typed = newItem.Text; + if (string.IsNullOrWhiteSpace(typed)) + return; + if (textEditing.TryCreateAndAddReferenceItem(field, typed)) + gestureCompleted?.Invoke(); + }; + EventHandler newOnBlur = + (s2, e2) => commitNew(); + newItem.LostFocus += newOnBlur; + EventHandler newOnEnter = (s2, e2) => + { + if (e2.Key != Key.Enter) + return; + e2.Handled = true; + commitNew(); + }; + newItem.KeyDown += newOnEnter; + // This slot names no item, so it must not leave a stale one current: a menu + // request from here would otherwise act on whichever item was clicked before. + EventHandler newClearsSelection = (s2, e2) => ClearSelection(); + newItem.GotFocus += newClearsSelection; + _teardown.Add(() => + { + newItem.LostFocus -= newOnBlur; + newItem.KeyDown -= newOnEnter; + newItem.GotFocus -= newClearsSelection; + }); + Children.Add(newItem); + AddSeparatorBar(); + } + var addButton = new Button { Content = "+", @@ -1576,8 +1626,6 @@ public FwReferenceVectorField( // object offers it (environments find-or-create a PhEnvironment from the typed // string). Every other vector row passes allowCreate: false and behaves exactly as // before. - var creation = editContext as IReferenceTextEditing; - var canCreate = creation != null && creation.CanCreateReferenceItem(field); var picker = new FwOptionChooser(field.Options, field.SearchOptions, automationId, field.Items.Select(i => i.Key), multiSelect: true, allowCreate: canCreate, normalizeName: field.NormalizeOptionName); @@ -1609,7 +1657,7 @@ public FwReferenceVectorField( // and a failed create leaves the row untouched rather than completing the gesture. Action created = text => { - var added = canCreate && creation.TryCreateAndAddReferenceItem(field, text); + var added = canCreate && textEditing.TryCreateAndAddReferenceItem(field, text); flyout.Hide(); addButton.Focus(); if (added) diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs index 0fbb860e96..f329df7b3d 100644 --- a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs @@ -39,8 +39,17 @@ public bool TrySetReferenceItemText(DetailField field, string itemKey, string te return SetResult; } - public bool CanCreateReferenceItem(DetailField field) => false; - public bool TryCreateAndAddReferenceItem(DetailField field, string text) => false; + public bool Creatable; + public bool CreateResult = true; + public readonly List Created = new List(); + + public bool CanCreateReferenceItem(DetailField field) => Creatable; + + public bool TryCreateAndAddReferenceItem(DetailField field, string text) + { + Created.Add(text); + return CreateResult; + } public bool IsOpen => false; public bool TrySetText(DetailField f, string ws, string v) => false; @@ -76,6 +85,20 @@ private static (FwReferenceVectorField Row, Window Window) Show( // Fails with the reason rather than handing back null, so a row that rendered labels // says so instead of surfacing as a NullReferenceException three lines later. + private static TextBox NewItemSlot(FwReferenceVectorField row) + => row.GetVisualDescendants().OfType().FirstOrDefault( + b => AutomationProperties.GetAutomationId(b) == "PhoneEnv.New"); + + private static void PressEnter(TextBox box) + { + box.RaiseEvent(new KeyEventArgs + { + RoutedEvent = InputElement.KeyDownEvent, + Key = Key.Enter + }); + Dispatcher.UIThread.RunJobs(); + } + private static TextBox Editor(FwReferenceVectorField row, string key) { var box = row.GetVisualDescendants().OfType().FirstOrDefault( @@ -154,6 +177,82 @@ public void FinishingAnUnchangedEdit_StagesNothing() + "environment for every field referencing it"); } + /// + /// PhoneEnvReferenceView keeps an always-present empty line at the end, so a new + /// environment can be typed without going near the chooser. The "+" picker stays: it is + /// the other route, not the only one. + /// + [AvaloniaTest] + public void ARowThatCanCreate_OffersATypedSlot_AlongsideThePicker() + { + var (row, _) = Show(new FakeTextEditing { Creatable = true }); + + Assert.That(NewItemSlot(row), Is.Not.Null, + "adding by typing must not require the chooser"); + Assert.That(row.GetVisualDescendants().OfType public static double PickerMinWidth => FwThemeResources.RequireDouble(GeneratedTokenKeys.DataTree_PickerMinWidth); + /// + /// Empty width of the slot that types a NEW reference-vector item. It sits inline among + /// the existing items and grows with what is typed, so this is only how much of a target + /// it offers when empty. + /// + public static double NewItemSlotMinWidth => FwThemeResources.RequireDouble(GeneratedTokenKeys.DataTree_NewItemSlotMinWidth); + /// /// The DETERMINISTIC, GLOBAL small-glyph icon size (px), the gear/kebab counterpart of /// -- the same 14px so every small glyph (checkbox, radio, diff --git a/Src/Common/FwAvaloniaTheme/Tokens/DataTree/DataTreeTokens.axaml b/Src/Common/FwAvaloniaTheme/Tokens/DataTree/DataTreeTokens.axaml index d6e72a08dc..be5f6482ee 100644 --- a/Src/Common/FwAvaloniaTheme/Tokens/DataTree/DataTreeTokens.axaml +++ b/Src/Common/FwAvaloniaTheme/Tokens/DataTree/DataTreeTokens.axaml @@ -170,4 +170,8 @@ DataTree.DropdownMinWidth, which sizes the COLLAPSED dropdown chooser. --> 180 + + 70 + From 0fcf88c5aa671fcd338a6503c35df2486cb5bb29 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Tue, 22 Sep 2026 17:35:42 -0400 Subject: [PATCH 05/14] LT-22672: Draw the environment editors flat, like the other editors Each item editor carried the default TextBox border and fill, so it cost more width than the label it replaced and a row that used to fit started clipping at its right edge. Every other editor in this view is already flat -- no border, no background -- and these now match. The row itself has never wrapped or scrolled: it is a horizontal StackPanel, so enough items have always overflowed and been clipped. Flat editors put that threshold back roughly where the labels had it, but they do not raise it. FwAvaloniaTests 758 passed, 0 failed. Co-Authored-By: Claude Opus 5 --- Src/Common/FwAvalonia/Detail/FwFieldControls.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index 4d109a98fd..de965ffab9 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -1386,6 +1386,8 @@ public FwReferenceVectorField( Control text; if (retypable) { + // Flat, like every other editor in this view: an item must not cost more + // room than the label it replaces, or a row that fitted starts clipping. var box = new TextBox { Text = item.Name, @@ -1393,7 +1395,9 @@ public FwReferenceVectorField( Margin = FwAvaloniaDensity.TrailingItemGap, Padding = FwAvaloniaDensity.EditorPadding, MinWidth = 0, - MinHeight = 0 + MinHeight = 0, + BorderThickness = new Thickness(0), + Background = FwAvaloniaDensity.TransparentBrush }; var itemKey = item.Key; // Staged when the edit FINISHES, not per keystroke: each stage reconciles @@ -1562,6 +1566,8 @@ public FwReferenceVectorField( Padding = FwAvaloniaDensity.EditorPadding, MinWidth = FwAvaloniaDensity.NewItemSlotMinWidth, MinHeight = 0, + BorderThickness = new Thickness(0), + Background = FwAvaloniaDensity.TransparentBrush, Watermark = FwAvaloniaStrings.AddItem }; AutomationProperties.SetAutomationId(newItem, automationId + ".New"); From 06f85d8c8ac7dfef564a30cfa4772a2163100ab4 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Tue, 22 Sep 2026 20:07:41 -0400 Subject: [PATCH 06/14] LT-22672: Wrap the reference-vector row instead of cutting it FwReferenceVectorField was a horizontal StackPanel, which arranges every child at its desired width whatever the row's own width is. Once the items exceeded the value column they carried on past the right edge and were cut there, mid-item if that is where the width ran out -- reported as an environment losing its last character. PhoneEnvReferenceView puts every item in one Views paragraph, which breaks to a new line at the edge instead. A WrapPanel is the same thing: only Orientation was ever used, and WrapPanel has it. The test arranges five items in a row too narrow for them and asserts that none is arranged past the row's own width. Reverted to StackPanel it fails, reporting an item reaching x=169 in a row 150 wide. This reaches every reference-vector row, not only Environments. All of them could be cut this way. Co-Authored-By: Claude Opus 5 --- .../FwAvalonia/Detail/FwFieldControls.cs | 4 +- .../Detail/RetypableVectorItemTests.cs | 39 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index de965ffab9..e7ec4b23a1 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -1334,7 +1334,7 @@ private static Action FindSink(FlyoutBase flyout) /// own yet, which the picker alone cannot do. Every other vector row passes allowCreate: /// false and is unaffected. /// - public sealed class FwReferenceVectorField : StackPanel, IHoverAffordanceProvider, + public sealed class FwReferenceVectorField : WrapPanel, IHoverAffordanceProvider, IDetailItemSelection, IDisposable { private readonly List _affordances = new List(); @@ -1365,6 +1365,8 @@ public FwReferenceVectorField( Action linkRequested = null, Action menuRequested = null) { + // Wraps, like the one Views paragraph PhoneEnvReferenceView puts its items in: + // a stack runs off the right edge, cutting the item the width ran out in. Orientation = Orientation.Horizontal; // 14.2-style hit-testing rule: a null background only hit-tests the glyphs -- the // WHOLE diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs index f329df7b3d..113cce0450 100644 --- a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs @@ -73,6 +73,12 @@ public void Cancel() { } new DetailChoiceOption("e2", "/_a") }); + private static DetailField RowOf(params string[] names) => new DetailField( + "MoStemAllomorph/x/#0", "Environments", "PhoneEnv", null, + DetailFieldKind.ReferenceVector, EditorClassification.Known, "PhoneEnv", null, + HostRouting.Inherit, null, null, null, isEditable: true, + items: names.Select((n, i) => new DetailChoiceOption("e" + i, n)).ToList()); + private static (FwReferenceVectorField Row, Window Window) Show( FakeTextEditing context, System.Action gestureCompleted = null) { @@ -108,6 +114,39 @@ private static TextBox Editor(FwReferenceVectorField row, string key) return box; } + /// + /// PhoneEnvReferenceView puts every item in one Views paragraph, which breaks to a new + /// line when it runs out of width. A horizontal stack ran off the right edge instead, + /// cutting whichever item the width ran out in -- the end of an environment simply + /// disappeared. + /// + [AvaloniaTest] + public void ARowNarrowerThanItsItems_WrapsThemRatherThanCuttingOne() + { + var row = new FwReferenceVectorField( + RowOf("/_#", "/_a", "/ _ zt", "/_[V]", "/_[C]"), "PhoneEnv", + new FakeTextEditing(), null); + var host = new Border { Child = row, Width = 150 }; + var window = new Window { Content = host, Width = 200, Height = 300 }; + window.Show(); + Dispatcher.UIThread.RunJobs(); + window.UpdateLayout(); + Dispatcher.UIThread.RunJobs(); + + var overhang = row.Children + .Where(c => c.Bounds.Right > row.Bounds.Width + 0.5) + .Select(c => $"{c.GetType().Name} right={c.Bounds.Right:F1}") + .ToList(); + + Assert.That(overhang, Is.Empty, + "an item arranged past the row's own width is cut off at the edge, which is " + + "how the end of an environment went missing; row width " + + row.Bounds.Width.ToString("F1")); + Assert.That(row.Children.Any(c => c.Bounds.Y > 0.5), Is.True, + "and they must actually have wrapped -- if everything still sits on one line " + + "the row was wide enough and this test proves nothing"); + } + [AvaloniaTest] public void ARetypableRow_RendersItsItemsAsEditors() { From 9790c9b5532fb152e8be90f9994222cb3662dd87 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Tue, 22 Sep 2026 21:53:27 -0400 Subject: [PATCH 07/14] LT-22672: Hold an item editor to the width its text measures A TextBox derives its width from its content, and that width comes out about a character narrower than what it then draws, so an environment lost its last character -- at any row width, focused or not. FloorWidthToText measures the text with FormattedText, the way a TextBlock does, and holds the box to at least that. The first measure waits for the box to be in the visual tree, since font size and family arrive with the theme, and it repeats on TextChanged so the box keeps up while typing. The typed slot takes the same floor above its empty width. Suppressing the floor fails the test, reporting a MinWidth of 0 against a text that measures 20. Headless cannot reproduce the defect itself: Avalonia's headless platform shapes text with a stub of uniform advances, so a TextBox's measurement matches its rendering there. The test pins the invariant rather than the mechanism -- an editor is never narrower than its text. Co-Authored-By: Claude Opus 5 --- .../FwAvalonia/Detail/FwFieldControls.cs | 32 +++++++++++++++++++ .../Detail/RetypableVectorItemTests.cs | 18 +++++++++++ .../Composer/EnvironmentItemEditingTests.cs | 18 +++++++++++ 3 files changed, 68 insertions(+) diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index e7ec4b23a1..a2825f8278 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -1401,6 +1401,9 @@ public FwReferenceVectorField( BorderThickness = new Thickness(0), Background = FwAvaloniaDensity.TransparentBrush }; + // Held to the width its text measures, so the last character is not + // cut, and kept in step while typing. + FloorWidthToText(box); var itemKey = item.Key; // Staged when the edit FINISHES, not per keystroke: each stage reconciles // against the project, so "/", "/_", "/_a" would leave two junk objects @@ -1572,6 +1575,8 @@ public FwReferenceVectorField( Background = FwAvaloniaDensity.TransparentBrush, Watermark = FwAvaloniaStrings.AddItem }; + // Keeps its empty width until what is typed needs more than that. + FloorWidthToText(newItem, FwAvaloniaDensity.NewItemSlotMinWidth); AutomationProperties.SetAutomationId(newItem, automationId + ".New"); AutomationProperties.SetName(newItem, FwAvaloniaStrings.AddItem); Action commitNew = () => @@ -1802,6 +1807,33 @@ public void Dispose() _teardown.Clear(); } + // The width a TextBox derives from its own content falls short of what it draws, + // cutting the end off. This is the width the text measures. + private void FloorWidthToText(TextBox box, double emptyWidth = 0) + { + Action measure = () => + { + var text = box.Text ?? string.Empty; + var typeface = new Typeface(box.FontFamily, box.FontStyle, box.FontWeight); + var measured = new FormattedText(text, CultureInfo.CurrentCulture, + FlowDirection.LeftToRight, typeface, box.FontSize, null); + box.MinWidth = Math.Max(emptyWidth, + measured.WidthIncludingTrailingWhitespace + + box.Padding.Left + box.Padding.Right); + }; + // Font size and family arrive with the theme, so the first measure waits for + // the box to be in the tree. + EventHandler onAttached = (s, e) => measure(); + EventHandler onTextChanged = (s, e) => measure(); + box.AttachedToVisualTree += onAttached; + box.TextChanged += onTextChanged; + _teardown.Add(() => + { + box.AttachedToVisualTree -= onAttached; + box.TextChanged -= onTextChanged; + }); + } + // The legacy VwSeparatorBox: a ~2px, font-height, light grey vertical bar after each item // (and fronting the add slot) -- the affordance that marks where content can be added. private void AddSeparatorBar() diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs index 113cce0450..fdedc912a9 100644 --- a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs @@ -157,6 +157,24 @@ public void ARetypableRow_RendersItsItemsAsEditors() + "it can only be removed and re-added"); } + /// + /// A TextBox measures its own text short of what it draws, so the last character was + /// cut off. The editor is held to the width the text actually measures. + /// + [AvaloniaTest] + public void AnItemEditor_IsNeverNarrowerThanItsOwnText() + { + var (row, _) = Show(new FakeTextEditing()); + var box = Editor(row, "e1"); + + var label = new TextBlock { Text = box.Text, FontSize = box.FontSize }; + label.Measure(new Avalonia.Size(double.PositiveInfinity, double.PositiveInfinity)); + + Assert.That(box.MinWidth, Is.GreaterThanOrEqualTo(label.DesiredSize.Width), + "an editor narrower than its own text cuts the end off it; text '" + box.Text + + "' measures " + label.DesiredSize.Width.ToString("F1")); + } + [AvaloniaTest] public void ARowThatCannotRetype_KeepsReadOnlyItems() { diff --git a/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs b/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs index 700c47bf45..943d4232ff 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs @@ -99,6 +99,24 @@ private bool Retype(IPhEnvironment env, string text) return staged; } + /// + /// The row shows what the user typed. The item's display text comes from ShortName, + /// which is not always the whole of an object's text. + /// + [Test] + public void AnEnvironmentItem_ShowsItsWholeStringRepresentation() + { + var env = GiveProjectAnEnvironment("/ _ zt"); + Attach(m_allomorph, env); + + var composed = DetailComposer.Compose(m_entry, Cache, showHiddenFields: true); + var row = composed.Model.Fields.Single( + f => f.Field == "PhoneEnv" && f.ObjectHvo == m_allomorph.Hvo); + + Assert.That(row.Items.Single().Name, Is.EqualTo("/ _ zt"), + "the item must display the whole environment, not an abbreviated form of it"); + } + /// /// Case 1, the surprising one. Respacing makes no new environment and does not move the /// reference -- it renames the shared object, so every OTHER allomorph referencing it From b10f1373d45b4f71cd34a34a3da85f909b738f51 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Tue, 22 Sep 2026 22:34:17 -0400 Subject: [PATCH 08/14] LT-22672: Say why the item editors are flat The reason recorded on the comment was that an item costing more width than the label it replaced would make a row that fitted start clipping. The row wraps now, so it does not, and the reason was never the whole one: the editors are flat because every other editor in this view is. No behaviour changes. Co-Authored-By: Claude Opus 5 --- Src/Common/FwAvalonia/Detail/FwFieldControls.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index a2825f8278..3891adb13d 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -1388,8 +1388,8 @@ public FwReferenceVectorField( Control text; if (retypable) { - // Flat, like every other editor in this view: an item must not cost more - // room than the label it replaces, or a row that fitted starts clipping. + // Flat -- no border, no fill -- because every other editor in this view + // is, and an item reads as part of the row rather than a control in it. var box = new TextBox { Text = item.Name, From 318e84fc80cc64679b80e801042bc7722b8797b1 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Wed, 23 Sep 2026 08:39:54 -0400 Subject: [PATCH 09/14] LT-22672: Let clearing an environment's text remove it PhoneEnvReferenceView removes a blank line rather than keeping it: EnvsBeingRequestedForThisEntry drops any line whose text trims to nothing, and the rebuilt vector then goes back without it. Its one existing test pins the same thing from the other side -- three cached lines, one with text, one result. Ours could not do this at all. TrySetReferenceItemText rejected blank text outright, and had it not, the handler would have fallen through to find-or-create and minted an empty PhEnvironment for the item to point at. So the guard was hiding a second defect behind it. Blank now removes the item and creates nothing. Whitespace alone counts as blank, matching the Trim. The environment itself stays in the project, since other allomorphs may still reference it. Suppressing the removal fails exactly the three cases that cover it and nothing else. Co-Authored-By: Claude Opus 5 --- .../Detail/IReferenceTextEditing.cs | 3 + .../Detail/RetypableVectorItemTests.cs | 21 +++++++ .../Composer/ComposedDetailEditContext.cs | 7 +-- .../Avalonia/Composer/DetailComposer.cs | 16 ++++-- .../Composer/EnvironmentItemEditingTests.cs | 57 +++++++++++++++++++ 5 files changed, 96 insertions(+), 8 deletions(-) diff --git a/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs b/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs index d6a151bf43..8ee6f7b47a 100644 --- a/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs +++ b/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs @@ -64,6 +64,9 @@ public interface IReferenceTextEditing /// which every field referencing it then shows. Text stripping differently re-points /// the item, creating the target only when the project has none. /// + /// Blank text is a REMOVAL, not a rejected edit: emptying an item takes it off the + /// field and creates nothing. Whitespace alone counts as blank. + /// /// Validity is not a precondition, for the same reason it is not on creation: the /// value is staged and annotated, never corrected or discarded. /// diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs index fdedc912a9..b2c896263e 100644 --- a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs @@ -215,6 +215,27 @@ public void TypingAlone_StagesNothing_UntilTheEditIsFinished() "finishing the edit stages it once, against the item's own key"); } + /// + /// Emptying an item is how the user removes it, so the row must pass the blank text + /// through to the domain rather than treating it as nothing to do. + /// + [AvaloniaTest] + public void EmptyingAnItemEditor_StagesTheBlankText() + { + var context = new FakeTextEditing(); + var (row, _) = Show(context); + var box = Editor(row, "e1"); + + box.Text = string.Empty; + PressEnter(box); + + Assert.That(context.Edits.Count, Is.EqualTo(1), + "a cleared item must reach the domain, which is what removes it"); + Assert.That(context.Edits[0].Key, Is.EqualTo("e1")); + Assert.That(string.IsNullOrEmpty(context.Edits[0].Text), Is.True, + "and it must arrive blank, not filtered out on the way"); + } + [AvaloniaTest] public void FinishingAnUnchangedEdit_StagesNothing() { diff --git a/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs b/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs index 384ebca402..1982974f87 100644 --- a/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs +++ b/Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs @@ -124,11 +124,10 @@ public bool CanEditReferenceItemText(DetailField field) public bool TrySetReferenceItemText(DetailField field, string itemKey, string text) { var setter = Handler(field)?.ReferenceSetText; - if (setter == null || string.IsNullOrWhiteSpace(itemKey) - || string.IsNullOrWhiteSpace(text)) - { + // Blank text is not rejected here: it is how the user removes an item, so the + // handler is the one that decides what an emptied item means. + if (setter == null || string.IsNullOrWhiteSpace(itemKey)) return false; - } return Stage(() => setter(itemKey, text), FieldLabelFor(field)); } diff --git a/Src/xWorks/Avalonia/Composer/DetailComposer.cs b/Src/xWorks/Avalonia/Composer/DetailComposer.cs index d1fe167dce..8dc8b1ceb5 100644 --- a/Src/xWorks/Avalonia/Composer/DetailComposer.cs +++ b/Src/xWorks/Avalonia/Composer/DetailComposer.cs @@ -1699,15 +1699,23 @@ private void AddGenericReferenceVector(ViewNode node, ICmObject obj, int depth, if (position < 0) return false; + // Clearing the text drops the item, as + // EnvsBeingRequestedForThisEntry does. Whitespace alone counts as + // cleared, and the environment itself stays in the project. + if (string.IsNullOrWhiteSpace(text)) + { + _sda.Replace(hvo, flid, position, position + 1, new int[0], 0); + return true; + } + var target = ResolveEnvironmentForItem(hvo, flid, position, StripSpaces(text)) ?? FindOrCreateEnvironment(text); if (target == null) return false; - // The typed string is written onto the resolved environment whether or - // not - // it moved, so a re-spelling reaches every field referencing it -- - // writing system as much as spelling. + // The typed string is written onto the resolved environment whether + // or not it moved, so a re-spelling reaches every field referencing + // it -- writing system as much as spelling. target.StringRepresentation = TsStringUtils.MakeString(text, _cache.DefaultVernWs); diff --git a/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs b/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs index 943d4232ff..9bcd8f3b71 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs @@ -117,6 +117,63 @@ public void AnEnvironmentItem_ShowsItsWholeStringRepresentation() "the item must display the whole environment, not an abbreviated form of it"); } + /// + /// Clearing an item's text removes it, which is what + /// EnvsBeingRequestedForThisEntry does with a blank line. The environment itself + /// survives: it is shared, and other allomorphs may still be using it. + /// + [Test] + public void ClearingAnItemsText_RemovesTheItem_AndKeepsTheEnvironment() + { + var shared = GiveProjectAnEnvironment("/_#"); + Attach(m_allomorph, shared); + Attach(m_otherAllomorph, shared); + var before = EnvironmentCount; + + Assert.That(Retype(shared, string.Empty), Is.True, "the edit staged"); + + Assert.That(m_allomorph.PhoneEnvRC, Is.Empty, + "an environment whose text the user cleared is no longer on the allomorph"); + Assert.That(EnvironmentCount, Is.EqualTo(before), + "and clearing creates nothing -- the old path minted an empty environment"); + Assert.That(shared.IsValidObject, Is.True); + Assert.That(m_otherAllomorph.PhoneEnvRC.Single(), Is.EqualTo(shared), + "the environment is shared, so removing this reference must not disturb it"); + } + + /// Trim, not Length: spaces alone are as blank as nothing at all. + [Test] + public void ClearingAnItemToWhitespace_CountsAsCleared() + { + var env = GiveProjectAnEnvironment("/_#"); + Attach(m_allomorph, env); + var before = EnvironmentCount; + + Assert.That(Retype(env, " "), Is.True); + + Assert.That(m_allomorph.PhoneEnvRC, Is.Empty, + "whitespace is not an environment; it removes the item like an empty string"); + Assert.That(EnvironmentCount, Is.EqualTo(before), + "and must not mint an environment whose whole text is spaces"); + } + + /// + /// Only the cleared item goes. A row-wide rebuild would drop the others with it. + /// + [Test] + public void ClearingOneItem_LeavesTheOtherItemsAlone() + { + var first = GiveProjectAnEnvironment("/_#"); + var second = GiveProjectAnEnvironment("/_a"); + Attach(m_allomorph, first); + Attach(m_allomorph, second); + + Assert.That(Retype(first, string.Empty), Is.True); + + Assert.That(m_allomorph.PhoneEnvRC.Single(), Is.EqualTo(second), + "the item that was not cleared keeps its environment"); + } + /// /// Case 1, the surprising one. Respacing makes no new environment and does not move the /// reference -- it renames the shared object, so every OTHER allomorph referencing it From 69efbbc9b71bcb3698ea8f5bc11ce792ef325891 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Wed, 23 Sep 2026 09:53:22 -0400 Subject: [PATCH 10/14] LT-22672: Record the reference-row inline editing lessons Five rules from converting the Environments row, and the evidence they came from. Two are worth naming here: a gesture's handling may live in a helper that filters what the write-back ever sees, so reading the commit path alone produces a model that looks complete and is not; and a guard that rejects an input can conceal what the guarded path would have done with it, which is how an empty domain object nearly got created. Status is left as proposed, and human review as pending, since the area README reserves that judgement for a reviewer. Co-Authored-By: Claude Opus 5 --- Docs/lessons/avalonia-migration/README.md | 1 + .../reference-row-inline-editing.md | 111 ++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 Docs/lessons/avalonia-migration/reference-row-inline-editing.md diff --git a/Docs/lessons/avalonia-migration/README.md b/Docs/lessons/avalonia-migration/README.md index de75eabff5..93f54aeb9b 100644 --- a/Docs/lessons/avalonia-migration/README.md +++ b/Docs/lessons/avalonia-migration/README.md @@ -15,6 +15,7 @@ tree and the live legacy behavior. | Browse virtualization; stable selection; clerk sorting/filtering; bulk edit; RDE; accessibility; activation breadth | [Browse-table activation](browse-table-activation.md) | | Picture editing; properties dialog; dormant view-models; localization pair removal; exchange DTO lifetime | [Avalonia picture editing](avalonia-picture-editing.md) | | Options-only utilities; features with no WinForms counterpart; parity divergence cost; entry-point unwinding | [Lexicon feature manager](lexicon-feature-manager.md) | +| Inline editing of reference-vector items; gesture characterization beyond the write-back path; blank input as removal; input guards masking defects; shared-object edits; text-metric and wrapping defects | [Reference-row inline editing](reference-row-inline-editing.md) | ## How to use these records diff --git a/Docs/lessons/avalonia-migration/reference-row-inline-editing.md b/Docs/lessons/avalonia-migration/reference-row-inline-editing.md new file mode 100644 index 0000000000..26d487e524 --- /dev/null +++ b/Docs/lessons/avalonia-migration/reference-row-inline-editing.md @@ -0,0 +1,111 @@ +# Inline text editing in converted reference rows + +Status: proposed as current principle; pending human review +Sources: PR for LT-22672 (Allomorphs Environments row); `PhoneEnvReferenceView` +and `PhoneEnvReferenceSlice` as the characterized source +Human review: pending + +## Question tested + +Can a converted reference-vector row gain inline typed editing -- so an item +already on the field can be changed, not only added and removed -- while +matching a source view whose editing semantics are reference reconciliation +rather than text entry? + +## Observations + +- The row's editing semantics were not text semantics. An item's identity was + its text with literal spaces removed, so a purely cosmetic re-spacing renamed + a shared object across the whole project, while any other change re-pointed + the reference and created a target only when none existed. +- The full behaviour was **not** recoverable from the source view's commit + method. One of six user-visible outcomes -- emptying an item to remove it -- + lived in a helper that decided which lines the commit would consider at all. + Reading the commit path alone produced a five-case model that looked complete + and was not. +- An input guard rejecting blank text made the conversion look merely + incomplete. It was also masking a second defect: with the guard removed, the + unguarded path created an empty domain object for the item to point at. The + guard had made a wrong answer unreachable rather than correct. +- The source view carried no tests for any of this. Its single test covered an + adjacent edge case, and was the only written evidence that blank lines were + treated differently from populated ones. +- Two rendering defects were indistinguishable from one another by report. A + row that arranged items past its own width, and an editor whose measured + width fell short of the text it drew, both presented as "the end of the value + is cut off". +- Headless rendering could not reproduce the measurement defect. The headless + text shaper uses uniform advances, so measurement and rendering agree there + by construction and the defect cannot arise. +- Repeated small changes each produced "no visible difference" in the running + application. That observation was consistent with a wrong diagnosis, with a + correct diagnosis and an insufficient change, and with the build not reaching + the application at all. A single deliberately extreme change separated all + three at once. + +## What failed or was retired + +An earlier attempt replaced the row with a bespoke control. It was reverted +because it lost the chooser, the item menu, reordering, and per-item +validation, all of which the row already provided. The retained approach +changed only how a row renders its items, leaving the row's identity intact. + +Three diagnoses of the rendering defects were tried and discarded: reducing +each item's width by removing editor chrome, widening padding to leave room for +a caret, and treating the row's overflow as the whole cause. The first two were +wrong; the third was a real and separate defect that did not explain the report. + +## Durable lessons + +1. Characterize a source view by enumerating the user's gestures against it, + not by reading the method that writes changes back. A gesture's handling may + live in a helper that filters what the write-back ever sees. +2. Before adding a guard that rejects an input, establish what the guarded path + would do with it. A guard that makes a wrong answer unreachable conceals the + defect instead of fixing it, and hides it from tests as well. +3. Treat blank input as a possible gesture rather than an absent value. In this + area emptying an item is how a user removes it. +4. Where a behaviour depends on text metrics or on arrangement against a real + surface, headless tests cannot reproduce it. Pin the invariant that must + hold rather than the mechanism, and require a pass in the running + application. +5. When a change yields no visible difference, force an unmistakable variant of + it before reasoning further. "No change" cannot distinguish a wrong + diagnosis from a change too small to see or from a build that never arrived. + +## Evidence needed next time + +- The complete gesture list for the row -- add, remove, retype, clear, reorder, + menu, and the keyboard equivalents -- each exercised against the source view + before any of it is designed. +- The source view's helper methods, not only its commit method, read for + gestures that never reach the write-back. +- For every behavioural test, a suppression run showing that it, and only it, + fails. +- A manual pass in the running application for anything touching text metrics, + wrapping, or available width, with the result recorded rather than assumed. +- The behaviour of a shared domain object when one reference to it is edited or + removed, asserted from a *second* holder of that reference. A single-holder + fixture cannot show project-wide reach. + +## Decision boundary + +This record constrains how a reference-vector row's editing behaviour is +discovered and evidenced. It does not decide which rows should become editable, +what a row should do when a single item exceeds the whole row's width, or +whether matching a surprising source behaviour is preferable to correcting it. +Those remain domain-owner decisions, taken per row. + +## Do not infer + +- That reproducing a source view's surprising behaviour is generally correct. + It was chosen here because divergence would have made two views disagree + about shared data, and the choice was recorded rather than assumed. +- That every reference-vector row should render editable items. The capability + is asked per field and only one row answers to it. +- That an unconditional write-back on commit is a design to copy. It is matched + parity with a specific source view. +- That the rendering fixes settle row layout generally. Wrapping happens + between items, not within one. +- That the tests here cover the rendering defects themselves. They cover the + invariants those defects violated. From 45419b7c9924234ca37c528ebc073a1f9c52f7d0 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Wed, 23 Sep 2026 11:39:54 -0400 Subject: [PATCH 11/14] LT-22672: Implement the new edit-context member on the test fake main added TryResetReferenceOrder to IDetailEditContext while this branch was out. FakeTextEditing implements that interface, so the merge does not compile -- and a local build on the un-merged branch could not see it, since the member did not exist there. CI builds the merge, and failed at the build step with every test skipped. Returns false, as InMemoryDetailEditContext and the other minimal fakes do. Nothing here exercises the order reset. Full suite on the merged branch: 6227 run, 6165 passed, 62 skipped, none failed. Co-Authored-By: Claude Opus 5 --- .../FwAvaloniaTests/Detail/RetypableVectorItemTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs index b2c896263e..9b6dd70def 100644 --- a/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/Detail/RetypableVectorItemTests.cs @@ -58,6 +58,7 @@ public bool TryCreateAndAddReferenceItem(DetailField field, string text) public bool TryAddReferenceItem(DetailField f, string key) => false; public bool TryRemoveReferenceItem(DetailField f, string key) => true; public bool TryMoveReferenceItem(DetailField f, string key, bool forward) => false; + public bool TryResetReferenceOrder(DetailField f) => false; public IReadOnlyList Validate() => new List(); public void Commit() { } public void Cancel() { } From 480705e9d9348af51be2e3f54671a7603a6ecc0a Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Wed, 23 Sep 2026 12:25:39 -0400 Subject: [PATCH 12/14] LT-22672: Drop a document section number from a comment The comment standard bans internal doc pointers: nothing in the tree resolves "14.2", and it points at nothing once that document is renumbered or moved. The sentence after it already gives the reason in the code's own terms, so only the pointer goes. This is the one such reference these changes introduced. Others predate them. Co-Authored-By: Claude Opus 5 --- Src/Common/FwAvalonia/Detail/FwFieldControls.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index 08e9915f8e..3a8894b04d 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -1453,8 +1453,8 @@ public FwReferenceVectorField( Text = item.Name, VerticalAlignment = VerticalAlignment.Center, Margin = FwAvaloniaDensity.TrailingItemGap, - // 14.2: a null background only hit-tests the glyphs -- the whole item - // must take the right-click, or the Remove flyout only opens over ink. + // A null background only hit-tests the glyphs -- the whole item must + // take the right-click, or the Remove flyout only opens over ink. Background = FwAvaloniaDensity.TransparentBrush }; } From ba6f2ac3e42ebaf174e2ebedcea70cd04a9a6f9e Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Wed, 23 Sep 2026 12:43:06 -0400 Subject: [PATCH 13/14] LT-22672: Replace document section numbers in comments The comment standard bans internal doc pointers: nothing in the tree resolves "14.2" or "12.1", and they name nothing once that document is renumbered. Each comment now states its point in the code's own terms. Thirty-three across ten files. Most carried a second violation in the same sentence -- the word "legacy", banned for the same reason -- so those went too; where a name was genuinely needed the class is named instead. Four of those were added by this branch and had passed the hygiene gate, which does not check that rule. Also unpicks five comments the hygiene auto-fixer had re-wrapped into a stranded single word. Comments only. Full suite 6227 run, 6164 passed, 62 skipped; the one failure is a Windows clipboard API error in an untouched drag-and-drop test, which passed on this same branch an hour earlier. Co-Authored-By: Claude Opus 5 --- Src/Common/FwAvalonia/Detail/DataTree.cs | 40 +++++++++---------- .../FwAvalonia/Detail/DetailFocusMemory.cs | 2 +- .../FwAvalonia/Detail/DetailMenuFlyout.cs | 9 ++--- .../FwAvalonia/Detail/FwFieldControls.cs | 22 +++++----- .../Detail/IReferenceTextEditing.cs | 8 ++-- .../FwAvaloniaTests/DetailFocusMemoryTests.cs | 2 +- .../DetailViewingParityTests.cs | 12 +++--- .../FwMultiWsTextFieldTests.cs | 3 +- .../Avalonia/DetailEditContextHolder.cs | 4 +- .../Composer/EnvironmentItemEditingTests.cs | 5 ++- 10 files changed, 51 insertions(+), 56 deletions(-) diff --git a/Src/Common/FwAvalonia/Detail/DataTree.cs b/Src/Common/FwAvalonia/Detail/DataTree.cs index 2437d1a60a..a8b1397140 100644 --- a/Src/Common/FwAvalonia/Detail/DataTree.cs +++ b/Src/Common/FwAvalonia/Detail/DataTree.cs @@ -87,12 +87,12 @@ public sealed class DataTree : UserControl, IDetailPopupSink new List<(TextBlock Label, double Reserved)>(); /// - /// Optional expansion-state hooks (11.8): supplies the + /// Optional expansion-state hooks: supplies the /// persisted state per header stable id (overriding the layout's initial state) and /// records toggles, so collapse state survives record - /// switches/re-shows -- the legacy PropertyTable expansion persistence. + /// switches and re-shows. /// / persist - /// the splitter position the same way (11.15): the host owns the remembered width so it + /// the splitter position the same way: the host owns the remembered width so it /// survives re-shows WITHOUT a process-global field -- each host/window keeps its own. /// public DataTree(DetailModel model, IDetailEditContext editContext = null, @@ -162,7 +162,7 @@ public DataTree(DetailModel model, IDetailEditContext editContext = null, var splitter = new GridSplitter { ResizeDirection = GridResizeDirection.Columns, - Background = FwAvaloniaDensity.TransparentBrush, // legacy splitter is window-colored/invisible (12.6) + Background = FwAvaloniaDensity.TransparentBrush, // the splitter is invisible, not chrome Width = FwAvaloniaDensity.SplitterWidth }; AutomationProperties.SetAutomationId(splitter, "DataTree.Splitter"); @@ -219,8 +219,7 @@ public DataTree(DetailModel model, IDetailEditContext editContext = null, AddHandler(Avalonia.Input.InputElement.KeyDownEvent, OnViewKeyDown, Avalonia.Interactivity.RoutingStrategies.Bubble); - // Auto-save (14.4): legacy slices commit as the user moves on -- any editor losing - // focus + // Auto-save: the view commits as the user moves on, so any editor losing focus // while a session is open commits it (validation-gated; one undo step per field). AddHandler(Avalonia.Input.InputElement.LostFocusEvent, (s, e) => { @@ -354,8 +353,8 @@ private static void ApplyRowTabIndex(Control root, int row) /// public event EventHandler InteractionCompleted; - // 14.4: no Save/Cancel buttons -- the legacy view saves as you go. The footer carries - // only the inline validation messages (a failed autosave is never silent). + // No Save/Cancel buttons: the view saves as you go. The footer carries only the + // inline validation messages (a failed autosave is never silent). private Control CreateEditFooter() { _validationBlock = new TextBlock @@ -482,7 +481,7 @@ private void RebuildItems() } // A header's recorded expansion state prefers this session's own toggles over the - // host-supplied persisted state (11.8), so a toggle applies immediately rather than + // host-supplied persisted state, so a toggle applies immediately rather than // waiting on the host's round-trip. private bool? GetRecordedExpansion(string stableId) => _expansionState.TryGetValue(stableId, out var v) ? (bool?)v : _getExpansionState?.Invoke(stableId); @@ -503,8 +502,8 @@ private Control BuildItem(int index, DetailField field) return content; } - // 12.1: the legacy 1px inter-slice rule renders as a per-item bottom border; the last - // field gets none. + // The 1px inter-slice rule renders as a per-item bottom border; the last field + // gets none. private Control ApplyRule(Control content, int index) { if (index >= Model.Fields.Count - 1) @@ -578,7 +577,7 @@ private FieldContent AddField(int row, DetailField field) Text = field.Label ?? field.Field ?? string.Empty, FontWeight = FontWeight.Bold, Margin = new Thickness(indent.Left, 4, 0, FwAvaloniaDensity.FieldSpacing), - // 14.2: a null background only hit-tests the glyphs; the whole header area + // A null background only hit-tests the glyphs; the whole header area // must take the right-click. Background = FwAvaloniaDensity.TransparentBrush }; @@ -586,7 +585,7 @@ private FieldContent AddField(int row, DetailField field) AutomationProperties.SetAutomationId(header, automationId); AutomationProperties.SetName(header, field.Label ?? string.Empty); - // 13.3/13.5: the header answers right-click with its slice menu; the hover + // The header answers right-click with its slice menu; the hover // "..." field-menu button (in a thin gutter to the left of the header) // opens the section menu/hotlinks. var headerCell = WrapWithFieldMenu(header, field, automationId, out var headerKebab); @@ -599,9 +598,9 @@ private FieldContent AddField(int row, DetailField field) // through the existing host bridge identically. var hotlinkStrip = CreateHotlinkStrip(field, automationId, indent); - // Viewing parity (11.15): top-level sections get the legacy heavy-weight separator rule. - // The header cell and its inline hotlink strip always travel together (the strip is part of - // the header row, hidden/shown with it by the collapse logic). + // Top-level sections get the heavy-weight separator rule. The header cell and + // its hotlink strip travel together: the strip is part of the header row, + // hidden and shown with it. Control headerControl; if (field.Indent == 0 && row > 0) { @@ -658,15 +657,14 @@ private FieldContent AddField(int row, DetailField field) // this // local value wins for our own TextBlock and keeps labels regular, like legacy. FontWeight = FontWeight.Normal, - // 14.2: a null background only hit-tests the glyphs; the whole label area must - // take - // the right-click for the slice menu. + // A null background only hit-tests the glyphs; the whole label area must + // take the right-click for the slice menu. Background = FwAvaloniaDensity.TransparentBrush }; _labelBlocks.Add((labelBlock, labelReserved)); AutomationProperties.SetAutomationId(labelBlock, automationId + ".Label"); AutomationProperties.SetName(labelBlock, field.Label ?? field.Field ?? string.Empty); - ToolTip.SetTip(labelBlock, field.Label ?? field.Field); // 11.17: legacy label tooltips + ToolTip.SetTip(labelBlock, field.Label ?? field.Field); // the label text is its own tip var editor = CreateEditor(field, automationId); editor.Margin = new Thickness(0, 0, 0, FwAvaloniaDensity.FieldSpacing); @@ -680,7 +678,7 @@ private FieldContent AddField(int row, DetailField field) vector.SelectionChanged += OnVectorSelectionChanged; } - // 13.3: the field's slice menu opens from the label cell's right-click or the + // The field's slice menu opens from the label cell's right-click or the // gutter "..." button; the editor's current item rides each request it raises. var labelCell = WrapWithFieldMenu(labelBlock, field, automationId, out var labelKebab, editor as IDetailItemSelection); diff --git a/Src/Common/FwAvalonia/Detail/DetailFocusMemory.cs b/Src/Common/FwAvalonia/Detail/DetailFocusMemory.cs index 711d00edd5..12d25c50dc 100644 --- a/Src/Common/FwAvalonia/Detail/DetailFocusMemory.cs +++ b/Src/Common/FwAvalonia/Detail/DetailFocusMemory.cs @@ -13,7 +13,7 @@ namespace SIL.FieldWorks.Common.FwAvalonia.Detail /// /// Keeps keyboard focus stable across detail-view re-shows. The host re-resolves and REPLACES the /// whole detail view after every committed edit and every delivered external refresh; without - /// this, tabbing out of a field (which auto-commits, 14.4) would tear down the editor the user + /// this, tabbing out of a field (which auto-commits) would tear down the editor the user /// just moved into and dump focus on the floor. Capture reads the focused editor's stable /// automation id (and caret) from the outgoing view; restore finds the same id in the incoming /// view and gives it focus -- automation ids are stable per field/writing system by design, diff --git a/Src/Common/FwAvalonia/Detail/DetailMenuFlyout.cs b/Src/Common/FwAvalonia/Detail/DetailMenuFlyout.cs index 0cbd69e545..b0fa79ac87 100644 --- a/Src/Common/FwAvalonia/Detail/DetailMenuFlyout.cs +++ b/Src/Common/FwAvalonia/Detail/DetailMenuFlyout.cs @@ -9,7 +9,7 @@ namespace SIL.FieldWorks.Common.FwAvalonia.Detail { /// - /// Framework-neutral context-menu item (15.1): what the host resolved from its menu system + /// Framework-neutral context-menu item: what the host resolved from its menu system /// (for FieldWorks, the xCore ChoiceGroup -- labels, enablement, checkmarks, submenus, and an /// execute action that dispatches through the mediator). FwAvalonia renders these natively; /// it knows nothing about xCore, preserving the engine-isolation boundary. @@ -45,10 +45,9 @@ private DetailMenuItem() /// /// Renders host-built trees as a native Avalonia - /// (15.1) -- the same items, enablement, checkmarks, and submenus - /// the - /// legacy WinForms adapter menu shows, rendered with native Avalonia controls. Density: every item carries the - /// explicit compact padding/height of the legacy WinForms menus + /// -- the same items, enablement, checkmarks and submenus the + /// host resolved, rendered with native Avalonia controls. Density: every item carries + /// explicit compact padding and height /// (/, /// not the Fluent theme defaults); long menus keep the presenter's scrolling. /// diff --git a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs index 3a8894b04d..8c72d11e9e 100644 --- a/Src/Common/FwAvalonia/Detail/FwFieldControls.cs +++ b/Src/Common/FwAvalonia/Detail/FwFieldControls.cs @@ -753,7 +753,7 @@ private void AddValueRow(DetailField field, string automationId, Children.Add(rowPanel); } - // Legacy look (12.3): small raised blue abbreviation, a superscript-style label kept in its + // A small raised blue abbreviation, a superscript-style label kept in its // own fixed gutter column (see the row Grid below) so a bold vernacular value can never crowd // or overlap it. ClipToBounds keeps an unusually long abbreviation inside the gutter width // rather than bleeding into the value column. @@ -788,7 +788,7 @@ private static TextBox CreateValueBox(DetailField field, DetailWsValue value, bo FlowDirection = value.RightToLeft ? FlowDirection.RightToLeft : FlowDirection.LeftToRight, BorderThickness = new Thickness(0), Background = FwAvaloniaDensity.TransparentBrush, - TextWrapping = TextWrapping.Wrap // 14.5: long values wrap; the row grows vertically + TextWrapping = TextWrapping.Wrap // long values wrap; the row grows vertically }; // A voice/audio writing system has no sound player in this view yet, so the row // is read-only and says why (a distinct message from the rich-content read-only case). @@ -801,7 +801,7 @@ private static TextBox CreateValueBox(DetailField field, DetailWsValue value, bo if (value.FontSize > 0) box.FontSize = value.FontSize; if (value.Bold) - box.FontWeight = FontWeight.Bold; // legacy (11.15) + box.FontWeight = FontWeight.Bold; // the value's own metadata asked for bold return box; } @@ -809,9 +809,8 @@ private void WireGhostPrompt(TextBox box, DetailField field) { if (!string.IsNullOrEmpty(field.GhostPrompt)) { - // 14.1: the legacy ghost add-prompt is a watermark -- it disappears the moment - // the - // user clicks in (focus), and reappears only if they leave without typing. + // The ghost add-prompt is a watermark: it disappears the moment the user + // clicks in, and reappears only if they leave without typing. box.Watermark = field.GhostPrompt; EventHandler ghostGot = (s2, e2) => box.Watermark = string.Empty; EventHandler ghostLost = (s2, e2) => @@ -836,7 +835,7 @@ private bool WireBridgeContextMenu(TextBox box, DetailField field, var hasBridge = menuRequested != null && !string.IsNullOrEmpty(field.ContextMenuId); if (hasBridge) { - // 15.2: exactly ONE menu -- drop the TextBox flyout (Cut/Copy/Paste) so + // Exactly ONE menu -- drop the TextBox flyout (Cut/Copy/Paste) so // only the bridged menu shows. Tunnelling puts this handler ahead of // anything the box or the whole-row handler would open. box.ContextFlyout = null; @@ -892,7 +891,7 @@ private static Grid CreateRowPanel(TextBlock abbrev, Control valueContent, bool { var rowPanel = new Grid { - // 14.2: a null background only hit-tests the glyphs -- the whole row must + // A null background only hit-tests the glyphs -- the whole row must // receive hover/right-click over the gaps too. Background = FwAvaloniaDensity.TransparentBrush }; @@ -1380,9 +1379,8 @@ public FwReferenceVectorField( // Wraps, like the one Views paragraph PhoneEnvReferenceView puts its items in: // a stack runs off the right edge, cutting the item the width ran out in. Orientation = Orientation.Horizontal; - // 14.2-style hit-testing rule: a null background only hit-tests the glyphs -- the - // WHOLE - // row must receive hover so the reveal affordances work over the gaps between items. + // A null background only hit-tests the glyphs -- the WHOLE row must receive + // hover so the reveal affordances work over the gaps between items. Background = FwAvaloniaDensity.TransparentBrush; AutomationProperties.SetAutomationId(this, automationId); AutomationProperties.SetName(this, field.Label ?? field.Field ?? automationId); @@ -1905,7 +1903,7 @@ public FwDialogLauncherField(string value, string label, Action launch) VerticalAlignment = VerticalAlignment.Center, TextWrapping = TextWrapping.Wrap, Margin = FwAvaloniaDensity.TrailingGap, - Background = FwAvaloniaDensity.TransparentBrush // 14.2 again: the value text is the hover surface + Background = FwAvaloniaDensity.TransparentBrush // the value text is the hover surface }; AutomationProperties.SetName(text, label ?? string.Empty); diff --git a/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs b/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs index 8ee6f7b47a..39035a978d 100644 --- a/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs +++ b/Src/Common/FwAvalonia/Detail/IReferenceTextEditing.cs @@ -12,7 +12,7 @@ namespace SIL.FieldWorks.Common.FwAvalonia.Detail /// ctx as IReferenceTextEditing and treats a null result as "this row picks from the /// list only", exactly as is acquired. /// - /// It exists because some legacy reference slices are BOTH a chooser and a typed editor. + /// It exists because some reference rows are BOTH a chooser and a typed editor. /// Environments is the case in hand: PhoneEnvReferenceLauncher opens a /// SimpleListChooser over the existing environments, while its inline /// PhoneEnvReferenceView lets the user type a new environment string that @@ -34,10 +34,10 @@ public interface IReferenceTextEditing /// field that cannot create, or for text the domain cannot turn into an object at all. /// /// Matching is the domain's business, not the caller's: environments match with spaces - /// stripped (legacy's RemoveSpaces), so "/ # _" and "/#_" must resolve to the SAME - /// object rather than creating a second one. + /// stripped (PhoneEnvReferenceView.RemoveSpaces), so "/ # _" and "/#_" must + /// resolve to the SAME object rather than creating a second one. /// - /// Validity is NOT a precondition. Legacy creates the object whether or not it passes + /// Validity is NOT a precondition. The object is created whether or not it passes /// domain validation and annotates the invalid one instead; rejecting it here would /// discard what the user typed. /// diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/DetailFocusMemoryTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/DetailFocusMemoryTests.cs index a27bbde92a..41dc5defa0 100644 --- a/Src/Common/FwAvalonia/FwAvaloniaTests/DetailFocusMemoryTests.cs +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/DetailFocusMemoryTests.cs @@ -16,7 +16,7 @@ namespace FwAvaloniaTests { /// - /// Focus continuity across detail-view re-shows (14.4 usability): the host replaces the entire view + /// Focus continuity across detail-view re-shows: the host replaces the entire view /// after every committed edit, so the focused editor (identified by its stable automation id) /// and caret must carry over to the rebuilt view -- otherwise tabbing out of a field would /// destroy the editor the user just moved into. diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/DetailViewingParityTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/DetailViewingParityTests.cs index ec78cbc924..46ccdd491f 100644 --- a/Src/Common/FwAvalonia/FwAvaloniaTests/DetailViewingParityTests.cs +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/DetailViewingParityTests.cs @@ -53,8 +53,8 @@ private static DataTree Show(params DetailField[] fields) return view; } - // 14.3/14.5 -- the 1px rule underlines only the value side (the label panel stays clean, - // like legacy lines between entries), and long values wrap so the row grows vertically. + // The 1px rule underlines only the value side, leaving the label panel clean, and + // long values wrap so the row grows vertically. [AvaloniaTest] public void Rules_UnderlineOnlyTheValueColumn_AndValuesWrap() { @@ -216,7 +216,7 @@ public void InitiallyCollapsedSection_StartsHidden_PerLayoutExpansion() [AvaloniaTest] public void ExpansionState_PersistsThroughTheSuppliedStore_AndAppliesOnRebuild() { - // 11.8: toggles record into the store; a new view (re-show/record switch) applies them. + // Toggles record into the store; a new view (re-show/record switch) applies them. var store = new Dictionary(); var model = new DetailModel("LexEntry", "Normal", new List { Header("h1", "Senses", 0), Text("g1", "Gloss", 1) }, @@ -280,19 +280,19 @@ public void VisualFidelity_FlatEditors_SliceRules_AndLegacyTokens() { var view = Show(Text("f1", "Lexeme Form", 0), Text("f2", "Citation Form", 0)); - // 12.2: values are flat like RootSite views -- no box. + // Values are flat, as RootSite views are -- no box. var box = view.GetVisualDescendants().OfType().First(); Assert.That(box.BorderThickness, Is.EqualTo(new Avalonia.Thickness(0))); Assert.That(box.Background, Is.EqualTo(Avalonia.Media.Brushes.Transparent)); - // 12.1: a 1px LightGray rule under the slice row (and none inside multistring rows -- + // A 1px LightGray rule under the slice row (and none inside multistring rows -- // FwMultiWsTextField stacks rows with no rule elements at all). var rule = view.GetVisualDescendants().OfType() .FirstOrDefault(b => AutomationProperties.GetAutomationId(b) == "SliceRule.0"); Assert.That(rule, Is.Not.Null); Assert.That(rule.Background, Is.EqualTo(SIL.FieldWorks.Common.FwAvalonia.FwAvaloniaDensity.SliceRuleBrush)); - // 12.3/12.4: WS abbreviation + label use the legacy-sampled tokens. + // The WS abbreviation and label use the sampled tokens. var abbrev = view.GetVisualDescendants().OfType().First(t => t.Text == "en"); Assert.That(abbrev.Foreground, Is.EqualTo(SIL.FieldWorks.Common.FwAvalonia.FwAvaloniaDensity.WsAbbrevBrush)); Assert.That(abbrev.FontSize, Is.EqualTo(SIL.FieldWorks.Common.FwAvalonia.FwAvaloniaDensity.WsAbbrevFontSize)); diff --git a/Src/Common/FwAvalonia/FwAvaloniaTests/FwMultiWsTextFieldTests.cs b/Src/Common/FwAvalonia/FwAvaloniaTests/FwMultiWsTextFieldTests.cs index 3e079e3952..bc0d85b007 100644 --- a/Src/Common/FwAvalonia/FwAvaloniaTests/FwMultiWsTextFieldTests.cs +++ b/Src/Common/FwAvalonia/FwAvaloniaTests/FwMultiWsTextFieldTests.cs @@ -20,8 +20,7 @@ namespace FwAvaloniaTests /// /// The per-writing-system multistring editor (): the owned detail-view /// control behind every multistring field. It renders ONE row per writing system -- a small - /// raised WS - /// abbreviation hanging at the value start (the legacy 12.3 look) plus a flat, borderless value editor + /// raised WS abbreviation hanging at the value start, plus a flat, borderless value editor /// (RootSite parity: no per-value box). These pin the structure (one row per WS, the WS label, empty vs /// populated, single- vs multi-WS) and emit a PNG per stage for subjective review, paired with the /// AssertNoCrowding tripwire. The editing/commit/teardown behavior itself lives in the Detail tests. diff --git a/Src/xWorks/Avalonia/DetailEditContextHolder.cs b/Src/xWorks/Avalonia/DetailEditContextHolder.cs index ea9d732f87..a7db67df09 100644 --- a/Src/xWorks/Avalonia/DetailEditContextHolder.cs +++ b/Src/xWorks/Avalonia/DetailEditContextHolder.cs @@ -21,7 +21,7 @@ namespace SIL.FieldWorks.XWorks /// fresh context and the displaced one is cancelled first (an orphaned open undo task makes /// every later IUndoStackManager.Save() throw "Commit at wrong place.", which is fatal /// at shutdown); - /// (2) is the single auto-save policy (14.4) every host path shares: + /// (2) is the single auto-save policy every host path shares: /// commit when validation is clean, roll back otherwise -- navigation, go-away, undo and /// dispose all settle the same way; /// (3) the undo guard intercepts global Undo/Redo while a session is open: LCModel's @@ -70,7 +70,7 @@ public void Clear() } /// - /// Auto-save (14.4): closes any open session -- committing when validation is clean, + /// Auto-save: closes any open session -- committing when validation is clean, /// rolling back otherwise (an invalid state is never silently persisted). No-op when /// nothing is open. /// ITEM 2: when the close is a rollback FORCED BY a validation failure, the validation diff --git a/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs b/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs index 9bcd8f3b71..cf7be4c2fd 100644 --- a/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs +++ b/Src/xWorks/xWorksTests/Avalonia/Composer/EnvironmentItemEditingTests.cs @@ -261,8 +261,9 @@ public void RetypingOneItem_DoesNotStealTheEnvironmentAnotherItemUses() } /// - /// Case 5: malformed text is staged and kept verbatim. Legacy annotates rather than - /// blocking, so nothing here may correct or discard what the user typed. + /// Case 5: malformed text is staged and kept verbatim. PhoneEnvReferenceView + /// annotates rather than blocking, so nothing here may correct or discard what the + /// user typed. /// [Test] public void RetypingToAMalformedEnvironment_IsAcceptedAndKeptVerbatim() From 54e41daaca06a92c157e1e447536b66afb8dfed6 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Wed, 23 Sep 2026 14:59:38 -0400 Subject: [PATCH 14/14] LT-22672: Move the lessons record to its own branch The record and its index row are about how the conversion was done, not what it does, and they sit in a directory this branch otherwise does not touch. They land separately so this PR stays a code change. Reverts the file addition only; nothing else on the branch referenced it. Co-Authored-By: Claude Opus 5 --- Docs/lessons/avalonia-migration/README.md | 1 - .../reference-row-inline-editing.md | 111 ------------------ 2 files changed, 112 deletions(-) delete mode 100644 Docs/lessons/avalonia-migration/reference-row-inline-editing.md diff --git a/Docs/lessons/avalonia-migration/README.md b/Docs/lessons/avalonia-migration/README.md index 93f54aeb9b..de75eabff5 100644 --- a/Docs/lessons/avalonia-migration/README.md +++ b/Docs/lessons/avalonia-migration/README.md @@ -15,7 +15,6 @@ tree and the live legacy behavior. | Browse virtualization; stable selection; clerk sorting/filtering; bulk edit; RDE; accessibility; activation breadth | [Browse-table activation](browse-table-activation.md) | | Picture editing; properties dialog; dormant view-models; localization pair removal; exchange DTO lifetime | [Avalonia picture editing](avalonia-picture-editing.md) | | Options-only utilities; features with no WinForms counterpart; parity divergence cost; entry-point unwinding | [Lexicon feature manager](lexicon-feature-manager.md) | -| Inline editing of reference-vector items; gesture characterization beyond the write-back path; blank input as removal; input guards masking defects; shared-object edits; text-metric and wrapping defects | [Reference-row inline editing](reference-row-inline-editing.md) | ## How to use these records diff --git a/Docs/lessons/avalonia-migration/reference-row-inline-editing.md b/Docs/lessons/avalonia-migration/reference-row-inline-editing.md deleted file mode 100644 index 26d487e524..0000000000 --- a/Docs/lessons/avalonia-migration/reference-row-inline-editing.md +++ /dev/null @@ -1,111 +0,0 @@ -# Inline text editing in converted reference rows - -Status: proposed as current principle; pending human review -Sources: PR for LT-22672 (Allomorphs Environments row); `PhoneEnvReferenceView` -and `PhoneEnvReferenceSlice` as the characterized source -Human review: pending - -## Question tested - -Can a converted reference-vector row gain inline typed editing -- so an item -already on the field can be changed, not only added and removed -- while -matching a source view whose editing semantics are reference reconciliation -rather than text entry? - -## Observations - -- The row's editing semantics were not text semantics. An item's identity was - its text with literal spaces removed, so a purely cosmetic re-spacing renamed - a shared object across the whole project, while any other change re-pointed - the reference and created a target only when none existed. -- The full behaviour was **not** recoverable from the source view's commit - method. One of six user-visible outcomes -- emptying an item to remove it -- - lived in a helper that decided which lines the commit would consider at all. - Reading the commit path alone produced a five-case model that looked complete - and was not. -- An input guard rejecting blank text made the conversion look merely - incomplete. It was also masking a second defect: with the guard removed, the - unguarded path created an empty domain object for the item to point at. The - guard had made a wrong answer unreachable rather than correct. -- The source view carried no tests for any of this. Its single test covered an - adjacent edge case, and was the only written evidence that blank lines were - treated differently from populated ones. -- Two rendering defects were indistinguishable from one another by report. A - row that arranged items past its own width, and an editor whose measured - width fell short of the text it drew, both presented as "the end of the value - is cut off". -- Headless rendering could not reproduce the measurement defect. The headless - text shaper uses uniform advances, so measurement and rendering agree there - by construction and the defect cannot arise. -- Repeated small changes each produced "no visible difference" in the running - application. That observation was consistent with a wrong diagnosis, with a - correct diagnosis and an insufficient change, and with the build not reaching - the application at all. A single deliberately extreme change separated all - three at once. - -## What failed or was retired - -An earlier attempt replaced the row with a bespoke control. It was reverted -because it lost the chooser, the item menu, reordering, and per-item -validation, all of which the row already provided. The retained approach -changed only how a row renders its items, leaving the row's identity intact. - -Three diagnoses of the rendering defects were tried and discarded: reducing -each item's width by removing editor chrome, widening padding to leave room for -a caret, and treating the row's overflow as the whole cause. The first two were -wrong; the third was a real and separate defect that did not explain the report. - -## Durable lessons - -1. Characterize a source view by enumerating the user's gestures against it, - not by reading the method that writes changes back. A gesture's handling may - live in a helper that filters what the write-back ever sees. -2. Before adding a guard that rejects an input, establish what the guarded path - would do with it. A guard that makes a wrong answer unreachable conceals the - defect instead of fixing it, and hides it from tests as well. -3. Treat blank input as a possible gesture rather than an absent value. In this - area emptying an item is how a user removes it. -4. Where a behaviour depends on text metrics or on arrangement against a real - surface, headless tests cannot reproduce it. Pin the invariant that must - hold rather than the mechanism, and require a pass in the running - application. -5. When a change yields no visible difference, force an unmistakable variant of - it before reasoning further. "No change" cannot distinguish a wrong - diagnosis from a change too small to see or from a build that never arrived. - -## Evidence needed next time - -- The complete gesture list for the row -- add, remove, retype, clear, reorder, - menu, and the keyboard equivalents -- each exercised against the source view - before any of it is designed. -- The source view's helper methods, not only its commit method, read for - gestures that never reach the write-back. -- For every behavioural test, a suppression run showing that it, and only it, - fails. -- A manual pass in the running application for anything touching text metrics, - wrapping, or available width, with the result recorded rather than assumed. -- The behaviour of a shared domain object when one reference to it is edited or - removed, asserted from a *second* holder of that reference. A single-holder - fixture cannot show project-wide reach. - -## Decision boundary - -This record constrains how a reference-vector row's editing behaviour is -discovered and evidenced. It does not decide which rows should become editable, -what a row should do when a single item exceeds the whole row's width, or -whether matching a surprising source behaviour is preferable to correcting it. -Those remain domain-owner decisions, taken per row. - -## Do not infer - -- That reproducing a source view's surprising behaviour is generally correct. - It was chosen here because divergence would have made two views disagree - about shared data, and the choice was recorded rather than assumed. -- That every reference-vector row should render editable items. The capability - is asked per field and only one row answers to it. -- That an unconditional write-back on commit is a design to copy. It is matched - parity with a specific source view. -- That the rendering fixes settle row layout generally. Wrapping happens - between items, not within one. -- That the tests here cover the rendering defects themselves. They cover the - invariants those defects violated.