From 42022a3a9e7075f3af4718174cc318cdee5c2b94 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Mon, 7 Sep 2026 16:32:01 +0200 Subject: [PATCH] Empty the panes when the assembly list is cleared Removing assemblies one at a time cleaned up the tabs that showed them, but clearing the whole list did not: Clear() raises a Reset whose OldItems is null, and the handler pruned the history and returned before the loop that empties the main tab and closes the orphaned ones. The last decompiled member stayed on screen over an empty list. Sorting had to be fixed first. It rebuilt the collection through Clear() plus AddRange, so it raised the same Reset and dropped the whole navigation history as a side effect - and once Reset closes tabs, it would have closed all of those too. It now reorders in place, and a Move is ignored where a removal would be handled, because a sort removes nothing. Assisted-by: Claude:claude-opus-5:Claude Code --- ICSharpCode.ILSpyX/AssemblyList.cs | 20 +++- .../AssemblyList/AssemblyListResetTests.cs | 110 ++++++++++++++++++ ILSpy/AssemblyTree/AssemblyTreeModel.cs | 4 + ILSpy/Docking/DockWorkspace.cs | 28 ++--- 4 files changed, 147 insertions(+), 15 deletions(-) create mode 100644 ILSpy.Tests/AssemblyList/AssemblyListResetTests.cs diff --git a/ICSharpCode.ILSpyX/AssemblyList.cs b/ICSharpCode.ILSpyX/AssemblyList.cs index d53f91a19..49d9fbae3 100644 --- a/ICSharpCode.ILSpyX/AssemblyList.cs +++ b/ICSharpCode.ILSpyX/AssemblyList.cs @@ -451,8 +451,24 @@ namespace ICSharpCode.ILSpyX { List list = new List(assemblies); list.Sort(index, Math.Min(count, list.Count - index), comparer); - assemblies.Clear(); - assemblies.AddRange(list); + // Reorder in place. Rebuilding the collection through Clear() would raise a Reset, + // which says every entry went away - and consumers that hold on to what the list + // contained (the navigation history, the open tabs) would throw it all away for a + // change that removes nothing. + for (int i = 0; i < list.Count; i++) + { + if (ReferenceEquals(assemblies[i], list[i])) + continue; + // Both hold the same entries, so the item is somewhere after i. + for (int j = i + 1; j < assemblies.Count; j++) + { + if (ReferenceEquals(assemblies[j], list[i])) + { + assemblies.Move(j, i); + break; + } + } + } } } diff --git a/ILSpy.Tests/AssemblyList/AssemblyListResetTests.cs b/ILSpy.Tests/AssemblyList/AssemblyListResetTests.cs new file mode 100644 index 000000000..42cd7ce19 --- /dev/null +++ b/ILSpy.Tests/AssemblyList/AssemblyListResetTests.cs @@ -0,0 +1,110 @@ +// Copyright (c) 2026 Siegfried Pammer +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System.Linq; +using System.Threading.Tasks; + +using Avalonia.Headless.NUnit; + +using AwesomeAssertions; + +using ICSharpCode.ILSpy.TreeNodes; + +using NUnit.Framework; + +namespace ICSharpCode.ILSpy.Tests; + +/// +/// What a wholesale change to the assembly list does to the panes that show its content. +/// Clearing the list and sorting it both raise a Reset on the collection, but they mean +/// opposite things: after a clear nothing that was on screen exists any more, while a sort +/// only reorders and everything on screen is still valid. +/// +[TestFixture] +public class AssemblyListResetTests +{ + static async Task DecompileAMethodAsync(ViewModels.MainWindowViewModel vm) + { + var typeNode = vm.AssemblyTreeModel.FindNode( + "System.Linq", "System.Linq", "System.Linq.Enumerable"); + typeNode.IsExpanded = true; + var method = typeNode.Children.OfType() + .Single(m => m.MethodDefinition.Name == "AsEnumerable"); + vm.AssemblyTreeModel.SelectNode(method); + var tab = await vm.DockWorkspace.WaitForDecompiledTextAsync(); + tab.Text.Should().Contain("AsEnumerable", "the pane must hold something before it can go stale"); + return method; + } + + [AvaloniaTest] + public async Task Clearing_The_Assembly_List_Empties_The_Decompiled_View() + { + // Clear() raises Reset, whose OldItems is null. The pane used to keep showing the last + // decompiled member over an empty list, while removing the same assemblies one at a + // time cleaned it up correctly. + + // Arrange - boot and decompile something. + var (_, vm) = await TestHarness.BootAsync(3); + await DecompileAMethodAsync(vm); + + // Act - clear the whole list, as the "Clear assembly list" command does. + vm.AssemblyTreeModel.AssemblyList!.Clear(); + await Waiters.WaitForIdleAsync(); + + // Assert - nothing from the cleared assemblies is left on screen. + vm.AssemblyTreeModel.AssemblyList!.GetAssemblies().Should().BeEmpty(); + var tab = vm.DockWorkspace.ActiveDecompilerTab; + if (tab != null) + tab.Text.Should().NotContain("AsEnumerable", "the assembly it came from is gone"); + } + + [AvaloniaTest] + public async Task Sorting_The_Assembly_List_Keeps_The_Navigation_History() + { + // Sort() rebuilt the collection through Clear() + AddRange, so it raised the same Reset + // a wholesale clear does and every history entry was dropped with it - even though + // sorting removes nothing. + + // Arrange - boot, decompile two members so there is a back entry to lose. + var (_, vm) = await TestHarness.BootAsync(3); + var typeNode = vm.AssemblyTreeModel.FindNode( + "System.Linq", "System.Linq", "System.Linq.Enumerable"); + typeNode.IsExpanded = true; + var first = typeNode.Children.OfType() + .Single(m => m.MethodDefinition.Name == "AsEnumerable"); + var second = typeNode.Children.OfType() + .First(m => m.MethodDefinition.Name == "Empty"); + vm.AssemblyTreeModel.SelectNode(first); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + // NavigationHistory collapses selections inside 0.5s into one entry. + await Task.Delay(600); + vm.AssemblyTreeModel.SelectNode(second); + await Waiters.WaitForAsync(() => ReferenceEquals(vm.AssemblyTreeModel.SelectedItem, second)); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + vm.DockWorkspace.BackHistory.Should().NotBeEmpty("the test needs history to survive"); + int assemblyCount = vm.AssemblyTreeModel.AssemblyList!.GetAssemblies().Length; + + // Act - sort the list. + vm.AssemblyTreeModel.SortAssemblyList(); + await Waiters.WaitForIdleAsync(); + + // Assert - the list is only reordered, so nothing it held became invalid. + vm.AssemblyTreeModel.AssemblyList!.GetAssemblies().Should().HaveCount(assemblyCount); + vm.DockWorkspace.BackHistory.Should().NotBeEmpty("sorting reorders the list, it removes nothing"); + } +} diff --git a/ILSpy/AssemblyTree/AssemblyTreeModel.cs b/ILSpy/AssemblyTree/AssemblyTreeModel.cs index 1a77c1987..8eefbae53 100644 --- a/ILSpy/AssemblyTree/AssemblyTreeModel.cs +++ b/ILSpy/AssemblyTree/AssemblyTreeModel.cs @@ -969,6 +969,10 @@ namespace ICSharpCode.ILSpy.AssemblyTree /// void OnActiveAssemblyListCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) { + // A Move carries the moved entry in OldItems, but nothing left the list: sorting must + // not look like removal to anything downstream. + if (e.Action == NotifyCollectionChangedAction.Move) + return; // Prune navigation-history entries that pointed at tree nodes inside removed // assemblies BEFORE re-publishing — Back/Forward consumers (the toolbar // commands + dropdowns) re-evaluate their CanExecute when the bus fires, so diff --git a/ILSpy/Docking/DockWorkspace.cs b/ILSpy/Docking/DockWorkspace.cs index 3e32ee93f..ccb673ded 100644 --- a/ILSpy/Docking/DockWorkspace.cs +++ b/ILSpy/Docking/DockWorkspace.cs @@ -243,20 +243,22 @@ namespace ICSharpCode.ILSpy.Docking { var inner = e.Inner; - // On Reset (assembly list wholesale-cleared), drop ALL history — every entry is - // stale by definition. - if (inner.Action == System.Collections.Specialized.NotifyCollectionChangedAction.Reset) - { - PruneHistoryAfterAssemblyListChange(removed: null); + // A Move carries the moved entry in OldItems, but the list only got reordered. + if (inner.Action == System.Collections.Specialized.NotifyCollectionChangedAction.Move) return; - } - if (inner.OldItems is not { Count: > 0 } oldItems) - return; - var removed = new HashSet( - oldItems.OfType()); - if (removed.Count == 0) - return; + // On Reset the list was cleared wholesale: every entry is stale by definition, and + // `removed == null` below stands for "all of them". + HashSet? removed = null; + if (inner.Action != System.Collections.Specialized.NotifyCollectionChangedAction.Reset) + { + if (inner.OldItems is not { Count: > 0 } oldItems) + return; + removed = new HashSet( + oldItems.OfType()); + if (removed.Count == 0) + return; + } PruneHistoryAfterAssemblyListChange(removed); @@ -277,7 +279,7 @@ namespace ICSharpCode.ILSpy.Docking var owner = n.AncestorsAndSelf().OfType().LastOrDefault(); if (owner is null) continue; - if (removed.Contains(owner.LoadedAssembly)) + if (removed == null || removed.Contains(owner.LoadedAssembly)) anyTouchesRemoved = true; else anyAlive = true;