From 9b95314e714be39f34bd70d3b2ea0307f4d15f37 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Wed, 15 Jul 2026 19:07:54 -0400 Subject: [PATCH 1/2] Fix AutoLayoutAlgorithm: re-evaluate auto at every nested scope Fixes a KeyNotFoundException thrown when a graph explicitly declares CoreOptions.Algorithm = auto and any nested container's own children scope inherits that value without re-declaring an algorithm override. Root cause: AutoLayoutAlgorithm's internal HierarchicalLayoutAlgorithm instance was built with a leaf-only registry (layered + containment), deliberately excluding auto to avoid infinite self-reference. When a hierarchical-routed group's cascaded options still resolved Algorithm=auto (the normal way a caller selects auto), the nested engine's own top-scope resolution tried to resolve auto against that leaf-only registry and threw. An earlier fix in this area worked around the exception by resetting Algorithm to the layered default before handing options to the nested engine. That masked the crash but broke CoreOptions.Algorithm's documented inheritance semantics: it locked in a single concrete choice at the boundary and cascaded that fixed choice to every descendant scope, instead of letting auto keep re-evaluating connectivity at each level. A container whose own children mixed a connected component with an unrelated singleton no longer got auto's distinct layered/containment routing - it was silently flattened to plain layered for the whole scope. The real fix: register the AutoLayoutAlgorithm instance itself under its own auto identifier in the registry its internal HierarchicalLayoutAlgorithm recurses with. When a nested scope's cascaded effective options resolve to auto, the recursion registry now resolves back to this same instance instead of throwing, re-running the full component classification (connected -> layered, singleton -> containment, container -> hierarchical) at that scope - exactly as if a caller had selected auto for it directly. This let the two prior "reset Algorithm to layered" workarounds (single-group fast path and multi-group split path) be reverted to their original zero-copy, unmodified-options form, since the fix is now purely additive at the registry level. A raw HierarchicalLayoutAlgorithm constructed independently of AutoLayoutAlgorithm still has zero knowledge of auto, preserving its documented independence. Adds a regression test proving auto is re-evaluated (not just non-throwing) at a nested scope with mixed connected/unconnected children, alongside the existing non-throw regression tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AutoLayoutAlgorithm.cs | 51 ++++++--- .../AutoLayoutAlgorithmTests.cs | 107 +++++++++++++++++- 2 files changed, 140 insertions(+), 18 deletions(-) diff --git a/src/DemaConsulting.Rendering.Layout/AutoLayoutAlgorithm.cs b/src/DemaConsulting.Rendering.Layout/AutoLayoutAlgorithm.cs index 2250eb8..04dffc4 100644 --- a/src/DemaConsulting.Rendering.Layout/AutoLayoutAlgorithm.cs +++ b/src/DemaConsulting.Rendering.Layout/AutoLayoutAlgorithm.cs @@ -124,10 +124,35 @@ public sealed class AutoLayoutAlgorithm : LayoutAlgorithmBase /// private const double ComponentAspectRatio = 4.0 / 3.0; - private readonly HierarchicalLayoutAlgorithm _hierarchical = new(); + private readonly HierarchicalLayoutAlgorithm _hierarchical; private readonly LayeredLayoutAlgorithm _layered = new(); private readonly ContainmentLayoutAlgorithm _containment = new(); + /// + /// Initializes a new instance of the class. + /// + /// + /// The internal instance used to recurse into + /// container-routed groups is built with a registry that includes this engine itself under its + /// own , in addition to the bundled leaf algorithms. This is what makes + /// "auto" behave as documented for 's inheritance rules: a + /// nested container scope that does not re-declare its own algorithm inherits "auto" like any + /// other cascaded option value, and — because the recursion registry can resolve "auto" back to + /// this same instance — that scope is re-classified by this algorithm's own connectivity-based + /// routing, exactly as if a caller had selected "auto" for it directly. "Auto" is therefore + /// re-evaluated at every scope it cascades to, never resolved once and locked in as a fixed + /// concrete choice for descendant scopes. A raw + /// constructed independently of this class still has zero knowledge of "auto" and continues to + /// surface a resolution error for it, preserving that engine's documented independence. + /// + public AutoLayoutAlgorithm() + { + _hierarchical = new HierarchicalLayoutAlgorithm(new LayoutAlgorithmRegistry() + .Register(_layered) + .Register(_containment) + .Register(this)); + } + /// public override string Id => AlgorithmId; @@ -244,7 +269,12 @@ protected internal override LayoutTree ApplyCore(LayoutGraph graph, LayoutOption } // Fast path: exactly one group overall means nothing needs to be split — delegate straight to - // that group's algorithm on the original, unmodified graph. + // that group's algorithm on the original, unmodified graph. This is safe for every routed + // algorithm, including HierarchicalLayoutAlgorithm: when this graph's own cascaded options + // resolve CoreOptions.Algorithm to "auto" (the normal way a caller selects this engine), that + // same "auto" value survives into HierarchicalLayoutAlgorithm's own top-scope resolution, but + // its recursion registry (built in this class's constructor) resolves "auto" back to this same + // instance rather than throwing — re-running this algorithm's own classification for that scope. if (routedGroups.Count == 1 && singletons.Count == 0) { return routedGroups[0].Algorithm.ApplyCore(graph, options); @@ -256,19 +286,12 @@ protected internal override LayoutTree ApplyCore(LayoutGraph graph, LayoutOption } // Genuine multi-group case: capture the graph's own cascaded options once (so a graph-level - // override still applies to every split-off piece), split each group into its own freshly-built - // sub-graph, lay each out independently, and pack the results into one combined tree. - // - // The captured options must not keep carrying this graph's own CoreOptions.Algorithm value - // (typically "auto" itself, since that is how a caller selected this algorithm in the first - // place): each split-off group's leaf/hierarchical algorithm was already chosen by the routing - // rule above, but HierarchicalLayoutAlgorithm re-reads CoreOptions.Algorithm from its own - // effective options to resolve ITS OWN top scope's leaf algorithm, and "auto" is never a - // registered leaf identifier there. Resetting it to the layered default (the same default - // HierarchicalLayoutAlgorithm itself falls back to when nothing declares an override) restores - // the cascade to exactly what an ordinary caller not using "auto" would see. + // override, including "auto" itself, still applies to every split-off piece), split each group + // into its own freshly-built sub-graph, lay each out independently, and pack the results into one + // combined tree. A split-off piece routed to HierarchicalLayoutAlgorithm resolves "auto" the same + // way the fast path above does — back to this instance, via the recursion registry — so nested + // container scopes are re-classified rather than defaulting to a fixed leaf choice. var effective = graph.OverlayOnto(options); - effective.Set(CoreOptions.Algorithm, LayeredLayoutAlgorithm.AlgorithmId); var trees = new List(routedGroups.Count + (singletons.Count > 0 ? 1 : 0)); foreach (var (members, algorithm) in routedGroups) diff --git a/test/DemaConsulting.Rendering.Layout.Tests/AutoLayoutAlgorithmTests.cs b/test/DemaConsulting.Rendering.Layout.Tests/AutoLayoutAlgorithmTests.cs index a45c642..14ef244 100644 --- a/test/DemaConsulting.Rendering.Layout.Tests/AutoLayoutAlgorithmTests.cs +++ b/test/DemaConsulting.Rendering.Layout.Tests/AutoLayoutAlgorithmTests.cs @@ -295,10 +295,12 @@ static double SiblingGap(double nodeSpacing) /// Proves that when the graph itself explicitly declares CoreOptions.Algorithm = "auto" /// (as a caller resolving the algorithm from the graph's own options, rather than passing it /// directly, would do) and the multi-group split path routes one group to hierarchical, the - /// hierarchical algorithm's own recursive scope resolution does not attempt to resolve "auto" - /// from its own leaf-only registry and throw. Regression test for a bug where the captured - /// effective options still carried the root graph's own "auto" value down into a nested - /// algorithm's cascade. + /// hierarchical algorithm's own recursive scope resolution does not throw when it re-reads + /// "auto" from its cascaded options: its recursion registry resolves "auto" back to this same + /// instance instead of a leaf-only registry that lacks it. + /// Regression test for a bug where the captured effective options still carried the root graph's + /// own "auto" value down into a nested algorithm's cascade, and that cascade had no way to + /// resolve it. /// [Fact] public void Apply_GraphDeclaresAutoAlgorithmWithNestedContainerGroup_DoesNotThrow() @@ -326,6 +328,103 @@ public void Apply_GraphDeclaresAutoAlgorithmWithNestedContainerGroup_DoesNotThro Assert.Equal(5, CountBoxesRecursively(tree.Nodes)); } + /// + /// Proves that when the graph itself explicitly declares CoreOptions.Algorithm = "auto" + /// and the entire graph is a single component containing a nested container (so routing + /// takes the zero-copy single-group fast path straight to + /// on the original, unmodified graph — not the split-and-pack path), the hierarchical algorithm's + /// own top-scope resolution does not throw when it re-reads + /// "auto" from its cascaded options. Regression test for a bug where the fast path handed the + /// untouched original options straight to , whose first + /// step (graph.OverlayOnto(options)) picks up the root graph's own "auto" override + /// unchanged, and its recursion registry (at the time) had no way to resolve that identifier. + /// + [Fact] + public void Apply_GraphDeclaresAutoAlgorithmAsSoleHierarchicalGroup_DoesNotThrow() + { + // Arrange: the graph itself declares "auto", and its only top-level node is a two-level-deep + // nested container with no other top-level node or edge, so routing produces exactly one group + // (routed to hierarchical) and no singletons — the single-group fast path. + var graph = new LayoutGraph(); + graph.Set(CoreOptions.Algorithm, "auto"); + + var outer = graph.AddNode("outer", 10, 10); + var inner = outer.Children.AddNode("inner", 10, 10); + var leaf1 = inner.Children.AddNode("leaf1", 80, 40); + var leaf2 = inner.Children.AddNode("leaf2", 80, 40); + inner.Children.AddEdge("leaf1-leaf2", leaf1, leaf2); + + // Act + var tree = new AutoLayoutAlgorithm().Apply(graph); + + // Assert: no exception was thrown, and every box (the outer container, the inner container, and + // the two leaves) made it into the tree. + Assert.Equal(4, CountBoxesRecursively(tree.Nodes)); + } + + /// + /// Proves that "auto" is re-evaluated at every scope it cascades to, rather than being resolved + /// once at the root and then locked in as a fixed concrete choice for every descendant scope. + /// A container whose own children mix a connected pair with an unrelated singleton — inheriting + /// "auto" from the root without re-declaring it — must be classified by this algorithm's own + /// connectivity-based routing (packing the singleton separately via containment) exactly like a + /// top-level "auto" graph would, not simply handed to a single fixed leaf algorithm (which would + /// lay every member out uniformly, with no special packing for the singleton). Regression test + /// for a design gap where a container-scope fix merely avoided throwing by forcing a fixed leaf + /// choice onto every descendant scope, rather than honoring "auto"'s documented inheritance rule + /// that an unset option keeps re-evaluating at each level. + /// + [Fact] + public void Apply_AutoInheritedByNestedContainer_ReclassifiesMixedConnectivityAtThatScope() + { + // Arrange: two structurally-identical graphs, differing only in how the container's own children + // scope resolves its algorithm. Both containers hold a connected pair ("a"-"b") plus an unrelated + // singleton ("solo"). "inherited" lets the root's "auto" cascade down unset; "forcedLayered" + // instead explicitly overrides the children scope to plain "layered", so its singleton is laid + // out uniformly alongside the pair rather than packed separately via containment. + static LayoutTree Build(bool forceLayeredOnChildren) + { + var graph = new LayoutGraph(); + graph.Set(CoreOptions.Algorithm, "auto"); + + var outer = graph.AddNode("outer", 10, 10); + outer.Label = "outer"; + if (forceLayeredOnChildren) + { + outer.Children.Set(CoreOptions.Algorithm, LayeredLayoutAlgorithm.AlgorithmId); + } + + var a = outer.Children.AddNode("a", 80, 40); + var b = outer.Children.AddNode("b", 80, 40); + outer.Children.AddEdge("a-b", a, b); + outer.Children.AddNode("solo", 80, 40); + + return new AutoLayoutAlgorithm().Apply(graph); + } + + // Act + var inheritedTree = Build(forceLayeredOnChildren: false); + var forcedLayeredTree = Build(forceLayeredOnChildren: true); + + var inheritedOuter = inheritedTree.Nodes.OfType().Single(box => box.Label == "outer"); + var forcedLayeredOuter = forcedLayeredTree.Nodes.OfType().Single(box => box.Label == "outer"); + + // Assert: every box made it into both trees (the outer container, "a", "b", and "solo"). + Assert.Equal(4, CountBoxesRecursively(inheritedTree.Nodes)); + Assert.Equal(4, CountBoxesRecursively(forcedLayeredTree.Nodes)); + + // Assert: the inherited-"auto" container is sized differently from the forced-plain-"layered" + // container, proving the inherited case actually re-ran this algorithm's own component + // classification (splitting the singleton into its own containment-packed bucket) instead of + // resolving to the same fixed leaf treatment as an explicit "layered" override. + Assert.True( + Math.Abs(inheritedOuter.Width - forcedLayeredOuter.Width) > 0.5 || + Math.Abs(inheritedOuter.Height - forcedLayeredOuter.Height) > 0.5, + $"expected the inherited-\"auto\" container ({inheritedOuter.Width:R}x{inheritedOuter.Height:R}) to be " + + $"sized differently from the forced-\"layered\" container ({forcedLayeredOuter.Width:R}x{forcedLayeredOuter.Height:R}), " + + "proving the nested scope was reclassified by \"auto\" rather than defaulting to a fixed leaf choice"); + } + /// Counts every in a node list, recursing into each box's children. /// The nodes to count within. /// The total number of boxes found. From 471b33d09d9c0434ca5ae069db47cb15fe2704b3 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Wed, 15 Jul 2026 19:31:29 -0400 Subject: [PATCH 2/2] Add 3-level nested gallery sample; fix recurring blank-line bug in generated table Adds AutoDeepNestedMixedConnectivity to the gallery: a three-level-deep container whose middle and innermost scopes each mix a connected pair with an unrelated singleton, none re-declaring CoreOptions.Algorithm. Visually demonstrates the AutoLayoutAlgorithm fix: every level correctly re-evaluates auto's connectivity classification instead of being locked to a single fixed leaf algorithm. While regenerating the gallery, found and fixed the recurring docs/gallery/README.md blank-table-line bug at its actual source. GalleryIndex.BuildTopIndex() called the shared AppendParagraph helper (which always appends a trailing blank line) separately for the table's header row and its separator row, so every regeneration reintroduced a blank line between them - silently undoing any manual fix applied directly to the committed Markdown, since the generator itself was never corrected. The header and separator rows are now written directly, matching how the data rows already were, so the table is emitted as one contiguous Markdown block on every future regeneration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/gallery/nested-hierarchy/README.md | 6 +++ .../auto-deep-nested-mixed-connectivity.svg | 53 +++++++++++++++++++ .../auto-nested-routes-hierarchical.svg | 20 +++---- .../GalleryCatalog.cs | 9 ++++ .../GalleryDiagrams.cs | 40 ++++++++++++++ .../GalleryIndex.cs | 7 ++- .../GalleryShowcaseTests.cs | 24 +++++++++ 7 files changed, 147 insertions(+), 12 deletions(-) create mode 100644 docs/gallery/nested-hierarchy/auto-deep-nested-mixed-connectivity.svg diff --git a/docs/gallery/nested-hierarchy/README.md b/docs/gallery/nested-hierarchy/README.md index 956f35f..bfa44bd 100644 --- a/docs/gallery/nested-hierarchy/README.md +++ b/docs/gallery/nested-hierarchy/README.md @@ -35,6 +35,12 @@ independently, and packs the results into one combined canvas. "auto" routes any component containing a container node through the hierarchical algorithm regardless of its size, while the unrelated isolated sibling is packed alongside it through the shared containment bucket. +![Three-level nested container mixing connected pairs and singletons](auto-deep-nested-mixed-connectivity.svg) + +Every nested scope inherits "auto" without re-declaring it, and each one is independently re-classified there: the +connected pair routes through layered and the singleton is packed alongside it through containment, at every level of +nesting — not just the root. + ## Boundary and delegation ports The hierarchical engine's support for boundary (delegation) ports: a container may expose a named port carrying BOTH an diff --git a/docs/gallery/nested-hierarchy/auto-deep-nested-mixed-connectivity.svg b/docs/gallery/nested-hierarchy/auto-deep-nested-mixed-connectivity.svg new file mode 100644 index 0000000..3db8525 --- /dev/null +++ b/docs/gallery/nested-hierarchy/auto-deep-nested-mixed-connectivity.svg @@ -0,0 +1,53 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Outer + + Mid1 + + Mid2 + + MidSolo + + DeepContainer + + D1 + + D2 + + + DSolo + + + RootSolo + diff --git a/docs/gallery/nested-hierarchy/auto-nested-routes-hierarchical.svg b/docs/gallery/nested-hierarchy/auto-nested-routes-hierarchical.svg index cf47ae9..6796001 100644 --- a/docs/gallery/nested-hierarchy/auto-nested-routes-hierarchical.svg +++ b/docs/gallery/nested-hierarchy/auto-nested-routes-hierarchical.svg @@ -1,4 +1,4 @@ - + @@ -30,13 +30,13 @@ - - Group - - Inner1 - - Inner2 - - - Solo + + Group + + Inner1 + + Inner2 + + + Solo diff --git a/test/DemaConsulting.Rendering.Gallery/GalleryCatalog.cs b/test/DemaConsulting.Rendering.Gallery/GalleryCatalog.cs index 1454e2d..801ab4f 100644 --- a/test/DemaConsulting.Rendering.Gallery/GalleryCatalog.cs +++ b/test/DemaConsulting.Rendering.Gallery/GalleryCatalog.cs @@ -85,6 +85,8 @@ internal static class GalleryCatalog public const string HierarchicalNestedPng = "nested-hierarchy/hierarchical-nested.png"; public const string AutoNestedRoutesHierarchicalSvg = "nested-hierarchy/auto-nested-routes-hierarchical.svg"; + public const string AutoDeepNestedMixedConnectivitySvg = + "nested-hierarchy/auto-deep-nested-mixed-connectivity.svg"; public const string BoundaryPortsShowcaseHorizontalSvg = "nested-hierarchy/boundary-ports-showcase-horizontal.svg"; public const string BoundaryPortsShowcaseHorizontalPng = @@ -353,6 +355,13 @@ internal static class GalleryCatalog + "hierarchical algorithm regardless of its size, while the unrelated " + "isolated sibling is packed alongside it through the shared containment " + "bucket."), + new GalleryImage( + AutoDeepNestedMixedConnectivitySvg, + "Three-level nested container mixing connected pairs and singletons", + "Every nested scope inherits \"auto\" without re-declaring it, and each one " + + "is independently re-classified there: the connected pair routes through " + + "layered and the singleton is packed alongside it through containment, at " + + "every level of nesting — not just the root."), ]), new GallerySection( "Boundary and delegation ports", diff --git a/test/DemaConsulting.Rendering.Gallery/GalleryDiagrams.cs b/test/DemaConsulting.Rendering.Gallery/GalleryDiagrams.cs index 93e4024..1cd5103 100644 --- a/test/DemaConsulting.Rendering.Gallery/GalleryDiagrams.cs +++ b/test/DemaConsulting.Rendering.Gallery/GalleryDiagrams.cs @@ -1010,6 +1010,46 @@ public static LayoutGraph AutoNestedRoutesHierarchical() return graph; } + /// + /// A three-level-deep compound container whose intermediate and innermost scopes each mix a + /// connected pair with an unrelated singleton, none of them re-declaring + /// . Every nested scope inherits "auto" from the root + /// graph and must be independently re-classified there — proving "auto" is re-evaluated + /// at each level it cascades to (routing the connected pair through layered and the singleton + /// through containment at every scope), rather than being resolved once at the root and then + /// applied uniformly to every descendant scope. + /// + /// A graph with a two-level nested container plus an unrelated root-level singleton. + public static LayoutGraph AutoDeepNestedMixedConnectivity() + { + var graph = new LayoutGraph(); + + // Level 1: the outer container. + var outer = graph.AddNode("outer", 10, 10); + outer.Label = "Outer"; + + // Level 2 (inside outer.Children): a connected pair, an unrelated singleton, and another + // container going one level deeper still. + var mid1 = AddLabelled(outer.Children, "mid1", "Mid1"); + var mid2 = AddLabelled(outer.Children, "mid2", "Mid2"); + Connect(outer.Children, "mid1-mid2", mid1, mid2); + AddLabelled(outer.Children, "midSolo", "MidSolo"); + + var deepContainer = outer.Children.AddNode("deepContainer", 10, 10); + deepContainer.Label = "DeepContainer"; + + // Level 3 (inside deepContainer.Children): again a connected pair plus an unrelated singleton. + var d1 = AddLabelled(deepContainer.Children, "d1", "D1"); + var d2 = AddLabelled(deepContainer.Children, "d2", "D2"); + Connect(deepContainer.Children, "d1-d2", d1, d2); + AddLabelled(deepContainer.Children, "dSolo", "DSolo"); + + // An unrelated root-level singleton forces the genuine multi-group split path at the root too. + AddLabelled(graph, "rootSolo", "RootSolo"); + + return graph; + } + /// /// Twelve small, wide sibling boxes with no edges, suited to the containment algorithm's /// column-count-based content-width candidate: without it, the area-based width estimate alone diff --git a/test/DemaConsulting.Rendering.Gallery/GalleryIndex.cs b/test/DemaConsulting.Rendering.Gallery/GalleryIndex.cs index c6e9b33..a0048dc 100644 --- a/test/DemaConsulting.Rendering.Gallery/GalleryIndex.cs +++ b/test/DemaConsulting.Rendering.Gallery/GalleryIndex.cs @@ -47,8 +47,11 @@ public static string BuildTopIndex() + "each one demonstrates and, where relevant, which bug it guards against. This page just " + "points the way and shows a taste of each."); - AppendParagraph(builder, "| Group | What it's about |"); - AppendParagraph(builder, "| --- | --- |"); + // Written directly (not via AppendParagraph) so the header and separator rows are not + // blank-line-separated from each other or from the data rows: a Markdown table is one + // contiguous block, and a blank line between its rows would break it into separate tables. + builder.Append("| Group | What it's about |").Append('\n'); + builder.Append("| --- | --- |").Append('\n'); foreach (var group in GalleryCatalog.Groups) { builder diff --git a/test/DemaConsulting.Rendering.Gallery/GalleryShowcaseTests.cs b/test/DemaConsulting.Rendering.Gallery/GalleryShowcaseTests.cs index baf9806..99c181f 100644 --- a/test/DemaConsulting.Rendering.Gallery/GalleryShowcaseTests.cs +++ b/test/DemaConsulting.Rendering.Gallery/GalleryShowcaseTests.cs @@ -753,6 +753,30 @@ public void Gallery_AutoNestedRoutesHierarchical_RendersSvg() Themes.Dark); } + /// + /// Renders a three-level-deep nested container to SVG, where the middle and innermost scopes + /// each mix a connected pair with an unrelated singleton and neither re-declares + /// . Proves "auto" is re-evaluated at every scope it cascades + /// to (routing each scope's connected pair through layered and its singleton through + /// containment independently) rather than being resolved once at the root and applied + /// uniformly to every descendant scope. Regression coverage for a bug where a nested scope that + /// inherited "auto" without re-declaring it either threw or, + /// under an earlier incomplete fix, was silently flattened to a single fixed leaf algorithm. + /// + [Fact] + public void Gallery_AutoDeepNestedMixedConnectivity_RendersSvg() + { + // Arrange + var graph = GalleryDiagrams.AutoDeepNestedMixedConnectivity(); + graph.Set(CoreOptions.Algorithm, "auto"); + + // Act / Assert + GalleryWriter.Svg( + GalleryCatalog.AutoDeepNestedMixedConnectivitySvg, + graph, + Themes.Dark); + } + /// /// Renders twelve small, wide boxes packed by the containment algorithm to SVG, proving the /// column-count-based content-width candidate keeps the algorithm from packing them into one