From 04b7986097118041ba79f81a3dd84700f3aed848 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Mon, 26 Oct 2026 07:51:17 -0400 Subject: [PATCH] Find tied containers' descendants regardless of insertion order AnnotationList.FindDepthFirst only descended into the single container that Find returned. When several containers share a range and only one owns the children, the result depended on insertion order and skip-list levels. Try every container with the same range as the one Find returned. Fixes #554 Co-Authored-By: Claude Opus 5.5 --- src/SIL.Machine/Annotations/AnnotationList.cs | 41 ++++++- .../AnnotationTiedContainerTests.cs | 102 ++++++++++++++++++ 2 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 tests/SIL.Machine.Tests/Annotations/AnnotationTiedContainerTests.cs diff --git a/src/SIL.Machine/Annotations/AnnotationList.cs b/src/SIL.Machine/Annotations/AnnotationList.cs index c57fa9e24..bce75f390 100644 --- a/src/SIL.Machine/Annotations/AnnotationList.cs +++ b/src/SIL.Machine/Annotations/AnnotationList.cs @@ -227,12 +227,49 @@ public bool FindDepthFirst(TOffset offset, Direction dir, out Annotation candidate = result; + foreach (Annotation tied in GetSameRange(candidate)) + { + if ( + !tied.IsLeaf + && tied.Range.Contains(offset, dir) + && tied.Children.FindDepthFirst(offset, dir, out Annotation inner) + ) + { + result = inner; + return true; + } + } + + if (!candidate.IsLeaf && candidate.Range.Contains(offset, dir)) + return candidate.Children.FindDepthFirst(offset, dir, out result); return false; } + private IEnumerable> GetSameRange(Annotation ann) + { + yield return ann; + if (ann == Begin || ann == End) + yield break; + + for ( + Annotation cur = ann.Prev; + cur != Begin && cur.Range.CompareTo(ann.Range) == 0; + cur = cur.Prev + ) + { + yield return cur; + } + + for (Annotation cur = ann.Next; cur != End && cur.Range.CompareTo(ann.Range) == 0; cur = cur.Next) + { + yield return cur; + } + } + public IEnumerable> GetNodes(TOffset start, TOffset end) { return GetNodes(Range.Create(start, end)); diff --git a/tests/SIL.Machine.Tests/Annotations/AnnotationTiedContainerTests.cs b/tests/SIL.Machine.Tests/Annotations/AnnotationTiedContainerTests.cs new file mode 100644 index 000000000..1a50218eb --- /dev/null +++ b/tests/SIL.Machine.Tests/Annotations/AnnotationTiedContainerTests.cs @@ -0,0 +1,102 @@ +using System.Reflection; +using NUnit.Framework; +using SIL.Machine.DataStructures; +using SIL.Machine.FeatureModel; + +namespace SIL.Machine.Annotations; + +[TestFixture] +public class AnnotationTiedContainerTests +{ + private const int ContainerCount = 4; + + private static Shape CreateShape(int seed, out ShapeNode[] nodes) + { + var annotations = new AnnotationList(); + typeof(BidirList>) + .GetField("_rand", BindingFlags.Instance | BindingFlags.NonPublic)! + .SetValue(annotations, new Random(seed)); + var shape = new Shape(begin => new ShapeNode(FeatureStruct.New().Value), annotations); + nodes = new ShapeNode[3]; + for (int i = 0; i < nodes.Length; i++) + nodes[i] = shape.Add(FeatureStruct.New().Value); + return shape; + } + + private static Annotation NewContainer(ShapeNode[] nodes) + { + return new Annotation(Range.Create(nodes[0], nodes[2]), FeatureStruct.New().Value); + } + + // Only the tied container at ownerIndex owns the segments, so the one Find returns varies with insertion order. + [Test] + public void FindDepthFirst_TiedContainers_FindsSegmentInOwner( + [Values(0, 1, 2, 3, 4, 5)] int seed, + [Range(0, ContainerCount - 1)] int ownerIndex, + [Values] Direction dir + ) + { + Shape shape = CreateShape(seed, out ShapeNode[] nodes); + for (int i = 0; i < ContainerCount; i++) + { + Annotation container = NewContainer(nodes); + if (i == ownerIndex) + container.Children.AddRange(nodes.Select(n => n.Annotation)); + shape.Annotations.Add(container, false); + } + + Assert.That(shape.Annotations.FindDepthFirst(nodes[1], dir, out Annotation result), Is.True); + Assert.That(result, Is.SameAs(nodes[1].Annotation)); + } + + // The owner is itself a container of tied containers, only one of which owns the segment annotations. + [Test] + public void FindDepthFirst_NestedTiedContainers_FindsSegmentInInnermostOwner( + [Values(0, 1, 2, 3, 4, 5)] int seed, + [Range(0, ContainerCount - 1)] int outerOwnerIndex, + [Range(0, ContainerCount - 1)] int innerOwnerIndex, + [Values] Direction dir + ) + { + Shape shape = CreateShape(seed, out ShapeNode[] nodes); + for (int i = 0; i < ContainerCount; i++) + { + Annotation outer = NewContainer(nodes); + if (i == outerOwnerIndex) + { + for (int j = 0; j < ContainerCount; j++) + { + Annotation inner = NewContainer(nodes); + if (j == innerOwnerIndex) + inner.Children.AddRange(nodes.Select(n => n.Annotation)); + outer.Children.Add(inner, false); + } + } + shape.Annotations.Add(outer, false); + } + + Assert.That(shape.Annotations.FindDepthFirst(nodes[1], dir, out Annotation result), Is.True); + Assert.That(result, Is.SameAs(nodes[1].Annotation)); + } + + // Tied containers that own only the other segments must not hide, or stand in for, an annotation that is not + // inside any of them. + [Test] + public void FindDepthFirst_TiedContainersNotOwningTheOffset_ReturnsTheUnownedAnnotation( + [Values(0, 1, 2, 3, 4, 5)] int seed, + [Values] Direction dir + ) + { + Shape shape = CreateShape(seed, out ShapeNode[] nodes); + for (int i = 0; i < ContainerCount; i++) + { + Annotation container = NewContainer(nodes); + if (i == 0) + container.Children.AddRange(new[] { nodes[0].Annotation, nodes[2].Annotation }); + shape.Annotations.Add(container, false); + } + + Assert.That(shape.Annotations.FindDepthFirst(nodes[1], dir, out Annotation result), Is.True); + Assert.That(result, Is.SameAs(nodes[1].Annotation)); + } +}