From 34e6ef11b60e7a8d2a3bba9e620eea7ebf1850e2 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 3 Jun 2026 07:59:10 +0200 Subject: [PATCH] Sync the tree selection to the active document tab Switching to a document tab is supposed to pull the tree selection over to what that tab shows, but it didn't reliably -- the gap was masked until the right-click change stopped moving the selection on its own. Two problems: The SelectedItem setter replaced the collection with Clear()+Add(), and the transient empty step made the grid sync defer its completion flag, which then suppressed the sync for the real new value -- so the tree visual stopped following tab activation. Route every selection replacement through a single batched SelectNodes() that fires the selection-changed fan-out once, with the final set, so consumers never observe the transient empty (or a transient multi, which would break metadata-tab reuse). And a tab decompiled from several nodes carries no single SourceNode, so activating it restored nothing. Restore the tab's full node set from CurrentNodes (one or many), and mirror a multi-selection into the grid so every restored node is highlighted, not just the primary. Assisted-by: Claude:claude-opus-4-8:Claude Code --- .../ContextMenus/DecompileInNewViewTests.cs | 165 ++++++++++++++++++ ILSpy/AssemblyTree/AssemblyListPane.axaml.cs | 88 ++++++++-- ILSpy/AssemblyTree/AssemblyTreeModel.cs | 60 ++++++- ILSpy/Docking/DockWorkspace.cs | 15 +- 4 files changed, 304 insertions(+), 24 deletions(-) diff --git a/ILSpy.Tests/ContextMenus/DecompileInNewViewTests.cs b/ILSpy.Tests/ContextMenus/DecompileInNewViewTests.cs index e4de6a9c4..cd8b1f75c 100644 --- a/ILSpy.Tests/ContextMenus/DecompileInNewViewTests.cs +++ b/ILSpy.Tests/ContextMenus/DecompileInNewViewTests.cs @@ -283,7 +283,172 @@ public class DecompileInNewViewTests "B's highlight must be gone"); } + [AvaloniaTest] + public async Task Opening_A_Node_In_A_New_Tab_Syncs_The_Tree_Selection_To_It() + { + // Activating the new tab must pull the tree selection over to its node -- both the model + // selection AND the grid's visual selection. Previously masked because the right-click + // already moved the selection; now that right-click leaves it put, the tab-activation + // sync has to do the work. + var window = AppComposition.Current.GetExport(); + window.Show(); + var vm = (MainWindowViewModel)window.DataContext!; + await vm.AssemblyTreeModel.WaitForAssembliesAsync(minimumCount: 3); + var pane = await window.WaitForComponent(); + var grid = await pane.WaitForComponent(); + + var nodeA = vm.AssemblyTreeModel.FindNode("System.Linq"); + var nodeB = vm.AssemblyTreeModel.FindNode(TreeNavigation.CoreLibName); + + vm.AssemblyTreeModel.SelectNode(nodeA); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + for (int i = 0; i < 6; i++) + { + Dispatcher.UIThread.RunJobs(); + grid.UpdateLayout(); + await Task.Delay(20); + } + + var registry = AppComposition.Current.GetExport(); + var menu = pane.BuildContextMenuForCurrentState(registry.Entries, rightClickedNode: nodeB); + menu!.ClickItem(Resources.DecompileToNewPanel); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + for (int i = 0; i < 6; i++) + { + Dispatcher.UIThread.RunJobs(); + grid.UpdateLayout(); + await Task.Delay(20); + } + + // The model selection follows the new active tab... + ReferenceEquals(vm.AssemblyTreeModel.SelectedItem, nodeB).Should().BeTrue( + "the tree model selection must follow the newly-activated tab"); + // ...and so does the grid's visual selection (the grid stores either the raw node or its + // HierarchicalNode wrapper depending on the path that set it). + ReferenceEquals(GridSelectedNode(grid), nodeB).Should().BeTrue( + "the grid's visual selection must follow the newly-activated tab"); + } + + [AvaloniaTest] + public async Task Activating_A_Single_Node_Tab_Syncs_The_Tree_After_A_Multi_Node_Selection() + { + // Regression: once the tree has held a multi-selection (e.g. a tab decompiled from + // several nodes), activating a single-node tab must still pull the tree selection over. + // The SelectedItem setter replaces the collection via Clear()+Add(); for the count>1 case + // that exposed a transient empty selection, whose grid sync deferred its done-flag and + // then suppressed the sync for the real value -- so the tree stopped following the tab. + var window = AppComposition.Current.GetExport(); + window.Show(); + var vm = (MainWindowViewModel)window.DataContext!; + await vm.AssemblyTreeModel.WaitForAssembliesAsync(minimumCount: 3); + var pane = await window.WaitForComponent(); + var grid = await pane.WaitForComponent(); + + var assemblies = vm.AssemblyTreeModel.Root!.Children.OfType().Take(3).ToArray(); + assemblies.Length.Should().BeGreaterThanOrEqualTo(3, "need three top-level rows"); + var nodeA = assemblies[0]; + var nodeB = assemblies[1]; + var nodeC = assemblies[2]; + + // Hold a multi-selection (A + B), as a multi-node decompile tab would. + vm.AssemblyTreeModel.SelectedItems.Clear(); + vm.AssemblyTreeModel.SelectedItems.Add(nodeA); + vm.AssemblyTreeModel.SelectedItems.Add(nodeB); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + for (int i = 0; i < 6; i++) + { + Dispatcher.UIThread.RunJobs(); + grid.UpdateLayout(); + await Task.Delay(20); + } + vm.AssemblyTreeModel.SelectedItems.Count.Should().Be(2, "precondition: a multi-selection is held"); + + // Open C in a new tab -> activates it -> the tree must follow to C. + var registry = AppComposition.Current.GetExport(); + var menu = pane.BuildContextMenuForCurrentState(registry.Entries, rightClickedNode: nodeC); + menu!.ClickItem(Resources.DecompileToNewPanel); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + for (int i = 0; i < 6; i++) + { + Dispatcher.UIThread.RunJobs(); + grid.UpdateLayout(); + await Task.Delay(20); + } + + ReferenceEquals(vm.AssemblyTreeModel.SelectedItem, nodeC).Should().BeTrue( + "the tree model selection must follow the newly-activated single-node tab"); + vm.AssemblyTreeModel.SelectedItems.Count.Should().Be(1, + "activating the single-node tab must collapse the multi-selection to that one node"); + ReferenceEquals(GridSelectedNode(grid), nodeC).Should().BeTrue( + "the grid's visual selection must follow even after a prior multi-selection"); + } + + [AvaloniaTest] + public async Task Activating_A_Multi_Node_Tab_Restores_Its_Multi_Selection_In_The_Tree() + { + // A tab decompiled from several nodes carries no single SourceNode, so activating it has + // to restore the WHOLE set in the tree -- not nothing, and not just one. Scenario: select + // two nodes (multi-node preview tab), open a third in a new tab, then re-activate the + // multi-node tab; the tree selection must come back to both original nodes. + var window = AppComposition.Current.GetExport(); + window.Show(); + var vm = (MainWindowViewModel)window.DataContext!; + await vm.AssemblyTreeModel.WaitForAssembliesAsync(minimumCount: 3); + var pane = await window.WaitForComponent(); + var grid = await pane.WaitForComponent(); + var documents = ((ILSpyDockFactory)vm.DockWorkspace.Factory).Documents!; + + var assemblies = vm.AssemblyTreeModel.Root!.Children.OfType().Take(3).ToArray(); + assemblies.Length.Should().BeGreaterThanOrEqualTo(3, "need three top-level rows"); + var nodeA = assemblies[0]; + var nodeB = assemblies[1]; + var nodeC = assemblies[2]; + + // 1) Multi-select A + B -> the active (preview) tab decompiles both. + vm.AssemblyTreeModel.SelectedItems.Clear(); + vm.AssemblyTreeModel.SelectedItems.Add(nodeA); + vm.AssemblyTreeModel.SelectedItems.Add(nodeB); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + var multiTab = (ContentTabPage)documents.ActiveDockable!; + + // 2) Open C in a new (single-node) tab -> it becomes active, tree -> C. + var registry = AppComposition.Current.GetExport(); + var menu = pane.BuildContextMenuForCurrentState(registry.Entries, rightClickedNode: nodeC); + menu!.ClickItem(Resources.DecompileToNewPanel); + await vm.DockWorkspace.WaitForDecompiledTextAsync(); + ReferenceEquals(multiTab, documents.ActiveDockable).Should().BeFalse( + "precondition: the new single-node tab is active, not the multi-node one"); + + // 3) Re-activate the multi-node tab. + vm.DockWorkspace.Factory.SetActiveDockable(multiTab); + for (int i = 0; i < 8; i++) + { + Dispatcher.UIThread.RunJobs(); + grid.UpdateLayout(); + await Task.Delay(20); + } + + // 4) The tree selection must contain BOTH original nodes again -- in the model... + vm.AssemblyTreeModel.SelectedItems.Should().Contain(nodeA) + .And.Contain(nodeB); + vm.AssemblyTreeModel.SelectedItems.Count.Should().Be(2, + "re-activating the multi-node tab restores exactly its two nodes"); + // ...and visually in the grid (both rows highlighted, not just the primary). + GridSelectedNodes(grid).Should().Contain(nodeA).And.Contain(nodeB); + GridSelectedNodes(grid).Should().HaveCount(2, + "the grid must visually highlight every restored node"); + } + static bool RowNodeEquals(DataGridRow row, SharpTreeNode node) => row.DataContext is HierarchicalNode hn && ReferenceEquals(hn.Item, node); + static SharpTreeNode? GridSelectedNode(DataGrid grid) + => grid.SelectedItem is HierarchicalNode hn ? hn.Item as SharpTreeNode : grid.SelectedItem as SharpTreeNode; + + static System.Collections.Generic.List GridSelectedNodes(DataGrid grid) + => grid.SelectedItems.Cast() + .Select(o => o is HierarchicalNode hn ? hn.Item as SharpTreeNode : o as SharpTreeNode) + .Where(n => n != null) + .Select(n => n!) + .ToList(); } diff --git a/ILSpy/AssemblyTree/AssemblyListPane.axaml.cs b/ILSpy/AssemblyTree/AssemblyListPane.axaml.cs index ff6da7618..e1e9c88f6 100644 --- a/ILSpy/AssemblyTree/AssemblyListPane.axaml.cs +++ b/ILSpy/AssemblyTree/AssemblyListPane.axaml.cs @@ -383,40 +383,90 @@ namespace ILSpy.AssemblyTree if (TreeGrid.HierarchicalModel is not IHierarchicalModel hm) return; + // Everything that can touch the grid -- including the ancestor Expand calls, which + // realise rows and can fire a SelectionChanged -- must run inside BeginSync so that + // echo doesn't bounce back into the model and disturb the in-flight navigation. + BeginSync(); + try + { + var primaryWrapper = ExpandAncestors(hm, target); + if (primaryWrapper == null) + return; + if (!ReferenceEquals(TreeGrid.SelectedItem, target)) + { + TreeGrid.SelectedItem = target; + // Defer past Expand's pending child-realization notifications — synchronously + // the wrapper isn't in the visible list yet. + var scrollTarget = primaryWrapper; + Dispatcher.UIThread.Post( + () => CenterRowInView(scrollTarget), + DispatcherPriority.Background); + } + // Multi-selection: mirror every OTHER selected node into the grid so a tab + // restored from several nodes highlights all of them, not just the primary set + // above. Single selection makes this a no-op. + if (DataContext is AssemblyTreeModel model && model.SelectedItems.Count > 1) + MirrorMultiSelectionToGrid(hm, model, target); + } + finally + { + EndSync(); + } + } + + // Expands every ancestor of so its row is realised, returning the + // node's own wrapper (or null if any level can't be located yet). + static HierarchicalNode? ExpandAncestors(IHierarchicalModel hm, SharpTreeNode node) + { var path = new System.Collections.Generic.List(); - for (var n = target; n.Parent != null; n = n.Parent) + for (var n = node; n.Parent != null; n = n.Parent) path.Add(n); path.Reverse(); - // Each ancestor must be expanded before FindNode can locate the next level's wrapper. - BeginSync(); HierarchicalNode? hNode = null; for (int i = 0; i < path.Count; i++) { hNode = hm.FindNode(path[i]); if (hNode == null) - { - EndSync(); - return; - } + return null; if (i < path.Count - 1 && !hNode.IsExpanded) hm.Expand(hNode); } + return hNode; + } - if (ReferenceEquals(TreeGrid.SelectedItem, target)) + // Brings the grid's selected set in line with the model's multi-selection: adds each + // selected node's wrapper that isn't highlighted yet (expanding its ancestors first) and + // drops any grid row no longer selected in the model. The primary is the caller's job. + void MirrorMultiSelectionToGrid(IHierarchicalModel hm, AssemblyTreeModel model, SharpTreeNode primary) + { + foreach (var node in model.SelectedItems) { - EndSync(); - return; + if (ReferenceEquals(node, primary)) + continue; + var wrapper = ExpandAncestors(hm, node); + if (wrapper != null && !GridSelectionContains(node)) + TreeGrid.SelectedItems.Add(wrapper); + } + for (int i = TreeGrid.SelectedItems.Count - 1; i >= 0; i--) + { + var node = GridItemNode(TreeGrid.SelectedItems[i]); + if (node != null && !model.SelectedItems.Contains(node)) + TreeGrid.SelectedItems.RemoveAt(i); } - TreeGrid.SelectedItem = target; - // Pass the HierarchicalNode wrapper (DataGrid.IndexOf is against the wrappers, not - // the underlying SharpTreeNodes), and defer past Expand's pending child-realization - // notifications — synchronously the wrapper isn't in the visible list yet. - var scrollTarget = hNode!; - Dispatcher.UIThread.Post( - () => CenterRowInView(scrollTarget), - DispatcherPriority.Background); - EndSync(); + } + + static SharpTreeNode? GridItemNode(object? item) + => item is HierarchicalNode hn ? hn.Item as SharpTreeNode : item as SharpTreeNode; + + bool GridSelectionContains(SharpTreeNode node) + { + foreach (var item in TreeGrid.SelectedItems) + { + if (ReferenceEquals(GridItemNode(item), node)) + return true; + } + return false; } // DataGrid.ScrollIntoView only brings the row to the nearest viewport edge; we want diff --git a/ILSpy/AssemblyTree/AssemblyTreeModel.cs b/ILSpy/AssemblyTree/AssemblyTreeModel.cs index 1622ade40..678ef0a96 100644 --- a/ILSpy/AssemblyTree/AssemblyTreeModel.cs +++ b/ILSpy/AssemblyTree/AssemblyTreeModel.cs @@ -88,10 +88,52 @@ namespace ILSpy.AssemblyTree set { if (SelectedItem == value) return; + SelectNodes(value == null ? System.Array.Empty() : new[] { value }); + } + } + + /// + /// Replaces the whole selection with in ONE logical change. The + /// collection can't be swapped atomically (Clear()+Add() passes through a transient empty, + /// add-before-remove through a transient multi), so the selection-changed fan-out is + /// batched: per-element IsSelected toggling still happens, but the PropertyChanged / path / + /// message-bus notifications fire once, AFTER, with the final set. Without this a transient + /// empty poisons the grid sync's deferred guard (tree stops following tab activation) and a + /// transient multi confuses count-sensitive consumers (metadata-tab reuse). Drives both the + /// single-node setter and multi-node tab-activation restore. + /// + public void SelectNodes(IReadOnlyList nodes) + { + ArgumentNullException.ThrowIfNull(nodes); + if (SelectionMatches(nodes)) + return; + batchingSelectionChange = true; + try + { SelectedItems.Clear(); - if (value != null) - SelectedItems.Add(value); + foreach (var node in nodes) + { + if (node != null && !SelectedItems.Contains(node)) + SelectedItems.Add(node); + } + } + finally + { + batchingSelectionChange = false; } + RaiseSelectionChanged(); + } + + bool SelectionMatches(IReadOnlyList nodes) + { + if (SelectedItems.Count != nodes.Count) + return false; + for (int i = 0; i < nodes.Count; i++) + { + if (!SelectedItems.Contains(nodes[i])) + return false; + } + return true; } [ObservableProperty] @@ -155,6 +197,11 @@ namespace ILSpy.AssemblyTree catch { return null; } } + // True while the SelectedItem setter is replacing the collection via Clear()+Add(); + // suppresses the selection-changed fan-out until the final state is in place so consumers + // never observe the transient empty/multi mid-replace. + bool batchingSelectionChange; + void OnSelectedItemsChanged(object? sender, NotifyCollectionChangedEventArgs e) { if (e.NewItems != null) @@ -164,6 +211,15 @@ namespace ILSpy.AssemblyTree foreach (SharpTreeNode n in e.OldItems) n.IsSelected = false; + // During a SelectedItem-setter batch the fan-out is deferred to the single + // RaiseSelectionChanged() the setter issues once the final selection is in place. + if (batchingSelectionChange) + return; + RaiseSelectionChanged(); + } + + void RaiseSelectionChanged() + { // SelectedItem is a wrapper over this collection — anyone bound to it must be // notified, and the saved path must follow the new primary. OnPropertyChanged(nameof(SelectedItem)); diff --git a/ILSpy/Docking/DockWorkspace.cs b/ILSpy/Docking/DockWorkspace.cs index 7c3736cce..d4974612f 100644 --- a/ILSpy/Docking/DockWorkspace.cs +++ b/ILSpy/Docking/DockWorkspace.cs @@ -451,14 +451,23 @@ namespace ILSpy.Docking // Carve-out tab → active: pull the tree's selection over to whatever entity the // new active tab is showing. Fires on user-driven tab clicks (via the dock model's // ActiveDockable setter) and on programmatic SetActiveDockable calls. - if (factory.Documents?.ActiveDockable is not ContentTabPage tab || tab.SourceNode is not { } node) + if (factory.Documents?.ActiveDockable is not ContentTabPage tab) return; - if (ReferenceEquals(assemblyTreeModel.SelectedItem, node)) + // Restore the tab's FULL selection. A decompiler tab carries its node(s) in + // CurrentNodes (one or many) — a multi-node tab has no single SourceNode, so reading + // CurrentNodes is what lets the tree restore the whole multi-selection. Other content + // (metadata, compare) carries a single SourceNode. + System.Collections.Generic.IReadOnlyList nodes; + if (UnwrapDecompilerTab(tab) is { CurrentNodes.Count: > 0 } dec) + nodes = dec.CurrentNodes; + else if (tab.SourceNode is { } single) + nodes = new[] { single }; + else return; syncingTreeFromActiveTab = true; try { - assemblyTreeModel.SelectedItem = node; + assemblyTreeModel.SelectNodes(nodes); } finally {