LT-22690: Test ghost add rows, and create nothing when one is cleared (#1194)

* LT-22690: Test ghost add rows, and create nothing when one is cleared

Typing into a ghost row creates its object on the first keystroke, so
typing and then clearing the row before leaving it saved an empty
object. The edit context now records which row opened the edit
session. Clearing a ghost row whose edits are the whole session
cancels that session: nothing is created and no undo step remains.
An edit another row staged into the same session is never cancelled.

New tests cover the ticket's acceptance steps. Typing into the Example
and SubSenses ghost rows creates the object and the view re-composes
into its rows. Creating and typing are one undo step, and undoing it
brings the ghost row back. Focusing and leaving an untouched ghost row
stages and saves nothing.

* LT-22690: Trim the comments on the ghost row change

Shorten the session-ownership doc comments to their contracts, and drop
test comments that restate the helper or test name. Comment-only.

* Comment cleanup

* LT-22690: Cancel a cleared ghost row only when it can roll back

Two gaps in clearing a ghost row before leaving it:

- When the edit session joined an undo task opened elsewhere,
  cancelling it rolled nothing back, so the created Example kept the
  text the user had just cleared. The cancel path now also requires a
  session that can roll back; otherwise the empty text is written.
- A rejected write from another row dropped the ghost row's ownership
  of the session. Ownership now changes only on a successful write,
  and is cleared when a setter throws in an already-open session.

A test for each reproduces the gap and passes with its fix.

Co-Authored-By: Zachary Burnham <zachary_burnham@sil.org>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: 3 people

Src/Common/FwAvalonia/FwAvaloniaTests/DetailEditingTests.cs

@@ -1127,7 +1127,8 @@ public void GhostRow_WatermarkClearsOnFocus_AndRestoresWhenLeftEmpty()
 				isEditable: true, indent: 0, ghostPrompt: "Click here to add Lexeme Form");
 			var model = new DetailModel("LexEntry", "Normal",
 				new List<DetailField> { ghost }, new List<ViewDiagnostic>());
-			var view = new DataTree(model, new FakeDetailEditContext());
+			var context = new FakeDetailEditContext();
+			var view = new DataTree(model, context);
 			var window = new Window { Content = view, Width = 480, Height = 200 };
 			window.Show();
 			Dispatcher.UIThread.RunJobs();
@@ -1144,6 +1145,9 @@ public void GhostRow_WatermarkClearsOnFocus_AndRestoresWhenLeftEmpty()
 			Dispatcher.UIThread.RunJobs();
 			Assert.That(box.Watermark, Is.EqualTo("Click here to add Lexeme Form"),
 				"leaving without typing restores the prompt");
+			Assert.That(context.TextEdits, Is.Empty,
+				"focusing and leaving the ghost row stages nothing, so nothing is created");
+			Assert.That(context.CommitCount, Is.Zero, "with nothing staged there is nothing to save");
 		}
 
 		// A search-backed reference vector (SearchOptions

Src/xWorks/Avalonia/Composer/ComposedDetailEditContext.cs

@@ -7,7 +7,6 @@
 using System.Linq;
 using SIL.FieldWorks.Common.FwAvalonia.Detail;
 using SIL.LCModel;
-using SIL.LCModel.Core.KernelInterfaces;
 
 namespace SIL.FieldWorks.XWorks
 {
@@ -82,8 +81,15 @@ public override bool TrySetText(DetailField field, string ws, string value)
 			var setter = Handler(field)?.Text;
 			if (setter == null)
 				return false;
+
+			if (string.IsNullOrEmpty(value) && field.GhostPrompt != null
+				&& IsSessionOwnedBy(field.StableId) && CanRollBackSession)
+			{
+				Cancel();
+				return true;
+			}
 			// A single field's edit names the undo label (e.g. "Undo change to Gloss").
-			return Stage(() => setter(ws, value), FieldLabelFor(field));
+			return Stage(() => setter(ws, value), FieldLabelFor(field), field.StableId);
 		}
 
 		public override bool TrySetRichText(DetailField field, string ws, DetailRichTextValue value)

Src/xWorks/Avalonia/Composer/LcmDetailEditSession.cs

@@ -43,6 +43,12 @@ public LcmDetailEditSession(LcmCache cache, string undoLabel, string redoLabel)
 		/// <inheritdoc />
 		public bool IsOpen { get; private set; }
 
+		/// <summary>
+		/// Whether <see cref="Cancel"/> would roll back this session's writes: it is open and
+		/// owns its undo task, which is still the open one.
+		/// </summary>
+		public bool CanRollBack => IsOpen && _ownsTask && TaskStillOpen;
+
 		/// <inheritdoc />
 		public void Commit()
 		{

Src/xWorks/Avalonia/DetailEditContextBase.cs

@@ -21,6 +21,7 @@ namespace SIL.FieldWorks.XWorks
 	public abstract class DetailEditContextBase : IDetailEditContext
 	{
 		private LcmDetailEditSession _session;
+		private string _sessionOwnerKey;
 		private readonly Dictionary<string, Func<bool>> _pendingEditFlushes =
 			new Dictionary<string, Func<bool>>(StringComparer.Ordinal);
 
@@ -154,15 +155,31 @@ public bool FlushPendingEdits()
 		/// <inheritdoc />
 		public void Commit()
 		{
+			_sessionOwnerKey = null;
 			_session?.Commit();
 		}
 
 		/// <inheritdoc />
 		public void Cancel()
 		{
+			_sessionOwnerKey = null;
 			_session?.Cancel();
 		}
 
+		/// <summary>
+		/// Whether every write in the open session was staged under <paramref name="ownerKey"/>.
+		/// False when no session is open or the key is null.
+		/// </summary>
+		protected bool IsSessionOwnedBy(string ownerKey)
+			=> IsOpen && ownerKey != null
+				&& string.Equals(_sessionOwnerKey, ownerKey, StringComparison.Ordinal);
+
+		/// <summary>
+		/// Whether <see cref="Cancel"/> would roll back the open session's writes. False when the
+		/// session joined an undo task opened elsewhere, which only that task's opener can end.
+		/// </summary>
+		protected bool CanRollBackSession => _session != null && _session.CanRollBack;
+
 		/// <summary>
 		/// Opens the fenced session on the first staged edit; later calls are no-ops. The generic
 		/// "Undo/Redo Edit Entry" label is used -- the batch/bulk path that stages several fields
@@ -230,6 +247,15 @@ public bool Stage(Func<bool> setter)
 		/// already-open session leaves that session's label untouched.
 		/// </summary>
 		public bool Stage(Func<bool> setter, string fieldLabel)
+		{
+			return Stage(setter, fieldLabel, null);
+		}
+
+		/// <summary>
+		/// As <see cref="Stage(Func{bool}, string)"/>, recording a write that stages under
+		/// <paramref name="ownerKey"/> for <see cref="IsSessionOwnedBy"/>.
+		/// </summary>
+		protected bool Stage(Func<bool> setter, string fieldLabel, string ownerKey)
 		{
 			if (setter == null)
 				return false;
@@ -238,14 +264,25 @@ public bool Stage(Func<bool> setter, string fieldLabel)
 			try
 			{
 				var staged = setter();
-				if (!staged && !wasOpen)
+				if (staged)
+				{
+					if (!wasOpen)
+						_sessionOwnerKey = ownerKey;
+					else if (!string.Equals(_sessionOwnerKey, ownerKey, StringComparison.Ordinal))
+						_sessionOwnerKey = null;
+				}
+				else if (!wasOpen)
+				{
 					Cancel();
+				}
 				return staged;
 			}
 			catch
 			{
 				if (!wasOpen)
 					Cancel();
+				else
+					_sessionOwnerKey = null; // a throwing setter may have written before it threw
 				throw;
 			}
 		}

Src/xWorks/xWorksTests/Avalonia/Composer/DetailEditContextEditingTests.cs

@@ -1555,6 +1555,203 @@ public void Compose_GhostWithoutGhostField_IsNotEditable_SoTypingCannotCreateAnd
 				"no bare object was created behind the user's back");
 		}
 
+		private static DetailField ExampleGhost(ComposedDetail composed, ILexSense sense)
+			=> composed.Model.Fields.FirstOrDefault(f => f.Field == "Examples"
+				&& f.StableId.EndsWith(DetailField.GhostStableIdSuffix) && f.ObjectHvo == sense.Hvo);
+
+		// The layout gives SubSenses no ghost= attribute; LT-22690 approves its ghost row.
+		private static DetailField SubsenseGhost(ComposedDetail composed, ILexSense sense)
+			=> composed.Model.Fields.FirstOrDefault(f => f.Field == "Senses"
+				&& f.StableId.EndsWith(DetailField.GhostStableIdSuffix) && f.ObjectHvo == sense.Hvo);
+
+		// One stage per keystroke, as the view does.
+		private static void TypeInto(ComposedDetail composed, DetailField field, string text)
+		{
+			for (var length = 1; length <= text.Length; length++)
+			{
+				Assert.That(composed.EditContext.TrySetText(field, field.Values[0].WsAbbrev,
+					text.Substring(0, length)), Is.True, "each keystroke stages");
+			}
+		}
+
+		[Test]
+		public void Compose_GhostExample_TypingCreatesTheExample_AndRecomposesIntoItsRows()
+		{
+			var sense = m_entry.SensesOS[0];
+			var composed = DetailComposer.Compose(m_entry, Cache);
+			var ghost = ExampleGhost(composed, sense);
+			Assert.That(ghost, Is.Not.Null, "a sense with no examples shows the Example ghost row");
+			Assert.That(ghost.IsEditable, Is.True);
+			Assert.That(ghost.GhostPrompt, Does.Contain("Example"));
+
+			TypeInto(composed, ghost, "una casa");
+			composed.EditContext.Commit();
+
+			Assert.That(sense.ExamplesOS.Count, Is.EqualTo(1), "typing created the example");
+			var example = sense.ExamplesOS[0];
+			Assert.That(example.Example.get_String(Cache.DefaultVernWs).Text, Is.EqualTo("una casa"),
+				"the typed text landed in the Example field, in the vernacular writing system");
+
+			var recomposed = DetailComposer.Compose(m_entry, Cache);
+			Assert.That(ExampleGhost(recomposed, sense), Is.Null,
+				"with an example present the ghost row gives way to the example's own rows");
+			var exampleRow = recomposed.Model.Fields.FirstOrDefault(f =>
+				f.Field == "Example" && f.ObjectHvo == example.Hvo);
+			Assert.That(exampleRow, Is.Not.Null, "the new example composes its Example row");
+			Assert.That(exampleRow.Values.Any(v => v.Value == "una casa"), Is.True,
+				"the Example row shows the typed text");
+		}
+
+		[Test]
+		public void Compose_GhostExample_CreationAndTyping_AreOneUndoStep()
+		{
+			var sense = m_entry.SensesOS[0];
+			var composed = DetailComposer.Compose(m_entry, Cache);
+			var stepsBefore = Cache.ActionHandlerAccessor.UndoableSequenceCount;
+
+			TypeInto(composed, ExampleGhost(composed, sense), "una casa");
+			composed.EditContext.Commit();
+			Assert.That(Cache.ActionHandlerAccessor.UndoableSequenceCount, Is.EqualTo(stepsBefore + 1),
+				"creating the example and every keystroke typed into it are one undo step");
+
+			Cache.ActionHandlerAccessor.Undo();
+			Assert.That(sense.ExamplesOS.Count, Is.Zero, "one undo removes the example");
+			Assert.That(ExampleGhost(DetailComposer.Compose(m_entry, Cache), sense), Is.Not.Null,
+				"after the undo the sense shows its Example ghost row again");
+		}
+
+		[Test]
+		public void Compose_GhostExample_TypedThenCleared_CreatesNothing()
+		{
+			var sense = m_entry.SensesOS[0];
+			var composed = DetailComposer.Compose(m_entry, Cache);
+			var ghost = ExampleGhost(composed, sense);
+			var stepsBefore = Cache.ActionHandlerAccessor.UndoableSequenceCount;
+
+			TypeInto(composed, ghost, "un");
+			Assert.That(sense.ExamplesOS.Count, Is.EqualTo(1), "the first keystroke creates the example");
+			Assert.That(composed.EditContext.TrySetText(ghost, ghost.Values[0].WsAbbrev, "u"), Is.True);
+			Assert.That(composed.EditContext.TrySetText(ghost, ghost.Values[0].WsAbbrev, ""), Is.True,
+				"clearing the row is accepted");
+			var sessionOpenAfterClear = composed.EditContext.IsOpen;
+			var examplesAfterClear = sense.ExamplesOS.Count;
+			// Close the session first, so a failure cannot break the next test's setup.
+			composed.EditContext.Commit();
+
+			Assert.That(sessionOpenAfterClear, Is.False, "clearing the row ends its edit session");
+			Assert.That(examplesAfterClear, Is.Zero, "clearing the row removes the example it created");
+			Assert.That(Cache.ActionHandlerAccessor.UndoableSequenceCount, Is.EqualTo(stepsBefore),
+				"nothing reaches the undo stack, not even a step that changes nothing");
+
+			TypeInto(composed, ghost, "b");
+			composed.EditContext.Commit();
+			Assert.That(sense.ExamplesOS.Count, Is.EqualTo(1), "typing again creates the example afresh");
+			Assert.That(sense.ExamplesOS[0].Example.get_String(Cache.DefaultVernWs).Text, Is.EqualTo("b"));
+		}
+
+		[Test]
+		public void Compose_GhostExample_ClearedAfterAnotherRowsEdit_KeepsThatEdit()
+		{
+			var sense = m_entry.SensesOS[0];
+			var composed = DetailComposer.Compose(m_entry, Cache);
+			var gloss = composed.Model.Fields.First(f => f.Field == "Gloss" && f.ObjectHvo == sense.Hvo);
+			var ghost = ExampleGhost(composed, sense);
+
+			var glossStaged = composed.EditContext.TrySetText(gloss, gloss.Values[0].WsAbbrev, "home");
+			var typedStaged = composed.EditContext.TrySetText(ghost, ghost.Values[0].WsAbbrev, "u");
+			var clearStaged = composed.EditContext.TrySetText(ghost, ghost.Values[0].WsAbbrev, "");
+			var sessionOpenAfterClear = composed.EditContext.IsOpen;
+			// Close the session first, so a failure cannot break the next test's setup.
+			composed.EditContext.Commit();
+
+			Assert.That(glossStaged && typedStaged && clearStaged, Is.True, "every edit stages");
+			Assert.That(sessionOpenAfterClear, Is.True,
+				"the session holds another row's edit, so clearing the ghost row does not cancel it");
+			Assert.That(sense.Gloss.get_String(Cache.DefaultAnalWs).Text, Is.EqualTo("home"),
+				"the other row's edit is saved");
+		}
+
+		[Test]
+		public void Compose_GhostExample_ClearedAfterAnotherRowsRejectedEdit_CreatesNothing()
+		{
+			var sense = m_entry.SensesOS[0];
+			var composed = DetailComposer.Compose(m_entry, Cache);
+			var gloss = composed.Model.Fields.First(f => f.Field == "Gloss" && f.ObjectHvo == sense.Hvo);
+			var ghost = ExampleGhost(composed, sense);
+
+			TypeInto(composed, ghost, "u");
+			var glossStaged = composed.EditContext.TrySetText(gloss, "no-such-ws", "home");
+			var clearStaged = composed.EditContext.TrySetText(ghost, ghost.Values[0].WsAbbrev, "");
+			var sessionOpenAfterClear = composed.EditContext.IsOpen;
+			var examplesAfterClear = sense.ExamplesOS.Count;
+			// Close the session first, so a failure cannot break the next test's setup.
+			composed.EditContext.Commit();
+
+			Assert.That(glossStaged, Is.False, "precondition: the gloss row rejects an unknown writing system");
+			Assert.That(clearStaged, Is.True);
+			Assert.That(sessionOpenAfterClear, Is.False,
+				"a rejected edit wrote nothing, so the session still holds only the ghost row's edits");
+			Assert.That(examplesAfterClear, Is.Zero, "clearing the row removes the example it created");
+		}
+
+		// An undo task opened before the row is typed into belongs to its opener, and cancelling
+		// the edit session cannot roll it back.
+		[Test]
+		public void Compose_GhostExample_ClearedInsideAnOuterUndoTask_KeepsNoTypedText()
+		{
+			var sense = m_entry.SensesOS[0];
+			var composed = DetailComposer.Compose(m_entry, Cache);
+			var ghost = ExampleGhost(composed, sense);
+
+			Cache.DomainDataByFlid.BeginUndoTask("outer", "outer");
+			try
+			{
+				TypeInto(composed, ghost, "u");
+				Assert.That(composed.EditContext.TrySetText(ghost, ghost.Values[0].WsAbbrev, ""), Is.True,
+					"clearing the row is accepted");
+				composed.EditContext.Commit();
+			}
+			finally
+			{
+				Cache.DomainDataByFlid.EndUndoTask();
+			}
+
+			Assert.That(sense.ExamplesOS.Select(e => e.Example.get_String(Cache.DefaultVernWs).Text ?? ""),
+				Has.All.Empty, "the text the user cleared is not saved by the outer task");
+		}
+
+		[Test]
+		public void Compose_GhostSubsense_TypingCreatesASubsense_WithTheTypedGloss()
+		{
+			var sense = m_entry.SensesOS[0];
+			var composed = DetailComposer.Compose(m_entry, Cache);
+			var ghost = SubsenseGhost(composed, sense);
+			Assert.That(ghost, Is.Not.Null, "a sense with no subsenses shows the SubSenses ghost row");
+			Assert.That(ghost.IsEditable, Is.True);
+			var stepsBefore = Cache.ActionHandlerAccessor.UndoableSequenceCount;
+
+			TypeInto(composed, ghost, "small house");
+			composed.EditContext.Commit();
+
+			Assert.That(sense.SensesOS.Count, Is.EqualTo(1), "typing created the subsense");
+			var subsense = sense.SensesOS[0];
+			Assert.That(subsense.Gloss.get_String(Cache.DefaultAnalWs).Text, Is.EqualTo("small house"),
+				"the typed text became the subsense's gloss");
+			Assert.That(Cache.ActionHandlerAccessor.UndoableSequenceCount, Is.EqualTo(stepsBefore + 1),
+				"creating the subsense and typing its gloss are one undo step");
+
+			var recomposed = DetailComposer.Compose(m_entry, Cache);
+			Assert.That(SubsenseGhost(recomposed, sense), Is.Null,
+				"with a subsense present the ghost row gives way to the subsense's own rows");
+			Assert.That(recomposed.Model.Fields.Any(f => f.Field == "Gloss" && f.ObjectHvo == subsense.Hvo),
+				Is.True, "the new subsense composes its Gloss row");
+
+			Cache.ActionHandlerAccessor.Undo();
+			Assert.That(sense.SensesOS.Count, Is.Zero, "one undo removes the subsense");
+			Assert.That(SubsenseGhost(DetailComposer.Compose(m_entry, Cache), sense), Is.Not.Null,
+				"after the undo the sense shows its SubSenses ghost row again");
+		}
+
 		// Conditional display: the real shipped variant/complex-form
 		// divergence. LexEntryRef/Normal's VariantEntryTypes and ComplexEntryTypes parts are
 		// <if field="RefType" intequals="0|1"> twins (LexEntryParts.xml:1133-1162); exactly one may