From 0826bae4a50b7cccb286727e7c689f11dbe95eb2 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Mon, 8 Jun 2026 13:36:05 +0200 Subject: [PATCH] Record fast distinct navigations instead of collapsing them NavigationHistory.Record's 0.5s debounce collapsed ANY two selections inside the window -- including two DIFFERENT nodes -- so a navigation whose decompile finished quickly (cheap targets, or a fast machine) never pushed the previous node onto the back stack, silently losing history. Its real purpose is only to swallow the double-fire a single click produces (SelectedItems.CollectionChanged + SelectedItem PropertyChanged, same node) and tree-refresh re-selects, so gate the collapse on the rapid entry being equal to the current one; a distinct node now records normally. The view-state/navigation tests navigate between two cheap namespace nodes (a C# namespace decompiles to just its "// Some.Name.Space" line) instead of decompiling CoreLib types in full -- fast enough for the headless 15s budget on slow CI runners, and a direct regression test for the collapse bug above. Assisted-by: Claude:claude-opus-4-8:Claude Code --- ILSpy.Tests/Docking/RunInNewTabTests.cs | 11 ++-- .../Navigation/ViewStateRoundTripTests.cs | 56 ++++++++++--------- ILSpy/NavigationHistory.cs | 20 ++++--- 3 files changed, 51 insertions(+), 36 deletions(-) diff --git a/ILSpy.Tests/Docking/RunInNewTabTests.cs b/ILSpy.Tests/Docking/RunInNewTabTests.cs index ba24db236..e821277fb 100644 --- a/ILSpy.Tests/Docking/RunInNewTabTests.cs +++ b/ILSpy.Tests/Docking/RunInNewTabTests.cs @@ -69,10 +69,13 @@ public class RunInNewTabTests dock.Documents!.VisibleDockables!.OfType().Count() .Should().BeGreaterThan(tabsBefore, "the long op opens its own tab"); - // Navigate while the op is running: select a type -> the preview tab decompiles. - var typeNode = vm.AssemblyTreeModel.FindNode( - "System.Linq", "System.Linq", "System.Linq.Enumerable"); - vm.AssemblyTreeModel.SelectNode(typeNode); + // Navigate while the op is running: select a node -> the preview tab decompiles. A C# + // namespace node decompiles to just its "// Some.Name.Space" comment line, which keeps + // this inside the headless decompile-wait budget on slow CI runners (a full type decompile + // would not). + var navNode = vm.AssemblyTreeModel.FindNode( + TreeNavigation.CoreLibName, "System.Runtime.Versioning"); + vm.AssemblyTreeModel.SelectNode(navNode); await dock.WaitForDecompiledTextAsync(); for (int i = 0; i < 6; i++) { diff --git a/ILSpy.Tests/Navigation/ViewStateRoundTripTests.cs b/ILSpy.Tests/Navigation/ViewStateRoundTripTests.cs index d3721ebef..8629596e6 100644 --- a/ILSpy.Tests/Navigation/ViewStateRoundTripTests.cs +++ b/ILSpy.Tests/Navigation/ViewStateRoundTripTests.cs @@ -23,6 +23,7 @@ using Avalonia.Headless.NUnit; using AwesomeAssertions; +using ILSpy.AssemblyTree; using ILSpy.Navigation; using ILSpy.TextView; using ILSpy.TreeNodes; @@ -41,6 +42,18 @@ namespace ICSharpCode.ILSpy.Tests.Navigation; [TestFixture] public class ViewStateRoundTripTests { + // These tests only exercise the navigation pipeline; what gets decompiled is irrelevant. + // Decompiling a CoreLib type in full is the slowest thing the suite does and overruns the + // headless 15s wait on slower CI runners. A namespace node in C# decompiles to just its + // "// Some.Name.Space" comment line (Language.DecompileNamespace) -- the cheapest possible + // target -- so navigate between two of those. Both namespaces are always present in CoreLib. + static (NamespaceTreeNode A, NamespaceTreeNode B) CheapNodes(AssemblyTreeModel atm) + { + var a = atm.FindNode(TreeNavigation.CoreLibName, "System.Runtime.Versioning"); + var b = atm.FindNode(TreeNavigation.CoreLibName, "System.Text"); + return (a, b); + } + [AvaloniaTest] public async Task Back_Restores_Caret_Position_The_User_Left_On_The_Previous_Node() { @@ -48,16 +61,13 @@ public class ViewStateRoundTripTests var dockWorkspace = vm.DockWorkspace; - // Pick two distinct decompiler targets so the navigation actually moves between - // them. CoreLib's System.Object and System.String are both always present. - var coreLibName = typeof(object).Assembly.GetName().Name!; - var objectNode = vm.AssemblyTreeModel.FindNode(coreLibName, "System", "System.Object"); - var stringNode = vm.AssemblyTreeModel.FindNode(coreLibName, "System", "System.String"); + // Two distinct, cheap navigation targets so the navigation actually moves between them. + var (nodeA, nodeB) = CheapNodes(vm.AssemblyTreeModel); - vm.AssemblyTreeModel.SelectNode(objectNode); + vm.AssemblyTreeModel.SelectNode(nodeA); await dockWorkspace.WaitForDecompiledTextAsync(); var tab = dockWorkspace.ActiveDecompilerTab!; - TestCapture.Step("object-decompiled"); + TestCapture.Step("node-a-decompiled"); // State is pulled from the editor on demand (CaptureViewState) when DockWorkspace records // a navigation away -- not pushed per caret/scroll event. Headless has no laid-out editor, @@ -66,16 +76,16 @@ public class ViewStateRoundTripTests // Navigate to a different node — DockWorkspace pulls A's state into the current history // entry and records B as the new current. - vm.AssemblyTreeModel.SelectNode(stringNode); + vm.AssemblyTreeModel.SelectNode(nodeB); await dockWorkspace.WaitForDecompiledTextAsync(); - TestCapture.Step("string-decompiled"); + TestCapture.Step("node-b-decompiled"); // Verify the capture: the back stack's most recent entry should be A's, with the pulled // caret + scroll values. var backEntries = dockWorkspace.BackHistory.OfType().ToList(); backEntries.Should().NotBeEmpty("Select(B) must push A onto the back stack"); var captured = backEntries.Last(); - ReferenceEquals(captured.Node, objectNode).Should().BeTrue( + ReferenceEquals(captured.Node, nodeA).Should().BeTrue( "captured back-stack entry must reference node A"); captured.CaretOffset.Should().Be(500); captured.VerticalOffset.Should().Be(120.5); @@ -111,14 +121,12 @@ public class ViewStateRoundTripTests var (_, vm) = await TestHarness.BootAsync(); var dockWorkspace = vm.DockWorkspace; - var coreLibName = typeof(object).Assembly.GetName().Name!; - var objectNode = vm.AssemblyTreeModel.FindNode(coreLibName, "System", "System.Object"); - var stringNode = vm.AssemblyTreeModel.FindNode(coreLibName, "System", "System.String"); + var (nodeA, nodeB) = CheapNodes(vm.AssemblyTreeModel); - vm.AssemblyTreeModel.SelectNode(objectNode); + vm.AssemblyTreeModel.SelectNode(nodeA); await dockWorkspace.WaitForDecompiledTextAsync(); var tab = dockWorkspace.ActiveDecompilerTab!; - TestCapture.Step("object-decompiled"); + TestCapture.Step("node-a-decompiled"); // Deterministic foldings snapshot — two expanded regions over a four-folding layout. // Compute via the helper to keep the checksum honest. (The snapshot itself is exercised @@ -133,13 +141,13 @@ public class ViewStateRoundTripTests // Navigate to a different node — DockWorkspace pulls the state and stamps the foldings // onto the OUTGOING entry on the back stack. - vm.AssemblyTreeModel.SelectNode(stringNode); + vm.AssemblyTreeModel.SelectNode(nodeB); await dockWorkspace.WaitForDecompiledTextAsync(); - TestCapture.Step("string-decompiled"); + TestCapture.Step("node-b-decompiled"); // Verify the capture: the back-stack entry for node A carries the snapshot. var captured = dockWorkspace.BackHistory.OfType().Last(); - ReferenceEquals(captured.Node, objectNode).Should().BeTrue( + ReferenceEquals(captured.Node, nodeA).Should().BeTrue( "captured back-stack entry must reference node A"); captured.Foldings.Should().NotBeNull("Select(B) must record A's foldings into the back stack"); captured.Foldings!.Value.Checksum.Should().Be(seeded.Checksum); @@ -168,14 +176,12 @@ public class ViewStateRoundTripTests ((object?)vm.DockWorkspace.NavigateBackCommand).Should().NotBeNull(); // Build up history: A → B, then go Back so Forward is enabled. - var coreLibName = typeof(object).Assembly.GetName().Name!; - var objectNode = vm.AssemblyTreeModel.FindNode(coreLibName, "System", "System.Object"); - var stringNode = vm.AssemblyTreeModel.FindNode(coreLibName, "System", "System.String"); - vm.AssemblyTreeModel.SelectNode(objectNode); + var (nodeA, nodeB) = CheapNodes(vm.AssemblyTreeModel); + vm.AssemblyTreeModel.SelectNode(nodeA); await vm.DockWorkspace.WaitForDecompiledTextAsync(); - vm.AssemblyTreeModel.SelectNode(stringNode); + vm.AssemblyTreeModel.SelectNode(nodeB); await vm.DockWorkspace.WaitForDecompiledTextAsync(); - TestCapture.Step("string-decompiled"); + TestCapture.Step("node-b-decompiled"); vm.DockWorkspace.NavigateBackCommand.Execute(null); await vm.DockWorkspace.WaitForDecompiledTextAsync(); @@ -188,7 +194,7 @@ public class ViewStateRoundTripTests TestCapture.Step("navigated-forward"); // Back through Forward should land on B again. - ReferenceEquals(vm.AssemblyTreeModel.SelectedItem, stringNode).Should().BeTrue( + ReferenceEquals(vm.AssemblyTreeModel.SelectedItem, nodeB).Should().BeTrue( "NavigateForward must restore the tree selection to the entry that was just popped from forward"); } } diff --git a/ILSpy/NavigationHistory.cs b/ILSpy/NavigationHistory.cs index d7db288c6..2139da574 100644 --- a/ILSpy/NavigationHistory.cs +++ b/ILSpy/NavigationHistory.cs @@ -22,10 +22,11 @@ using System.Collections.Generic; namespace ILSpy.Navigation { /// - /// Two-stack browser-style history. Rapid successive calls (within - /// 0.5 s) replace the current entry instead of pushing, so a tree refresh that re-selects - /// the same node doesn't pollute the back stack with duplicates. Equality is delegated - /// to the entry's own implementation. + /// Two-stack browser-style history. A rapid successive of the SAME entry + /// (within 0.5 s) replaces the current entry instead of pushing, so the double-fire a single + /// click produces -- and tree refreshes that re-select the same node -- don't pollute the back + /// stack with duplicates. A rapid selection of a DIFFERENT entry still records normally. + /// Equality is delegated to the entry's own implementation. /// internal sealed class NavigationHistory where T : class, IEquatable { @@ -120,10 +121,15 @@ namespace ILSpy.Navigation var navigationTime = DateTime.Now; var period = navigationTime - lastNavigationTime; - if (period.TotalSeconds < NavigationSecondsBeforeNewEntry) + if (period.TotalSeconds < NavigationSecondsBeforeNewEntry && current != null && current.Equals(entry)) { - // Rapid successive selections collapse into a single entry — protects against - // tree refreshes that re-issue SelectedItem. + // A rapid RE-SELECTION of the SAME target collapses into the current entry. This + // swallows the double-fire a single click produces (SelectedItems.CollectionChanged + // and SelectedItem PropertyChanged both fan in) and tree refreshes that re-issue the + // same SelectedItem. A rapid selection of a DIFFERENT node is a genuine navigation + // and must fall through to push the old current onto the back stack -- gating only on + // elapsed time (the previous behaviour) silently dropped fast navigations whenever + // the decompile finished inside the window (cheap targets, fast machines). current = entry; } else