From 91133c12728be21ba9130a19af8f2bc518309d18 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 3 Jun 2026 07:57:20 +0200 Subject: [PATCH] Keep the tree selection put when right-clicking a node Right-clicking a tree row to reach its context menu used to move the real selection there first, because ProDataGrid selects the row on press. With the preview document bound to the selection, that meant 'Decompile to new tab' on node B (while viewing A) jumped the preview to B before the command ran, so you ended up with B twice instead of the intended A + B. Middle-click avoided it, but not every mouse has a usable one. Capture the right-clicked row in ContextRequested (which fires even when a previous menu's light-dismiss popup swallows the press) and swallow the right-press so the grid never reselects: the menu now acts on the clicked row as a Thunderbird-style context target while the selection -- and the document -- stay put. The targeted row gets a faint focus-box highlight, cleared when the menu closes (guarded so a stale menu's close can't wipe a newer target). Also adds TestHarness.ClickItem to collapse the repeated Items.OfType().Single(...).RaiseEvent(...) menu-click dance. Assisted-by: Claude:claude-opus-4-8:Claude Code --- .../ReloadAssemblyContextMenuTests.cs | 4 +- .../RemoveAssemblyContextMenuTests.cs | 4 +- .../ContextMenus/DecompileInNewViewTests.cs | 180 +++++++++++++++--- ILSpy.Tests/TestHarness.cs | 20 ++ ILSpy/App.axaml | 2 + ILSpy/AssemblyTree/AssemblyListPane.axaml | 9 + ILSpy/AssemblyTree/AssemblyListPane.axaml.cs | 120 +++++++++++- 7 files changed, 307 insertions(+), 32 deletions(-) diff --git a/ILSpy.Tests/AssemblyList/ReloadAssemblyContextMenuTests.cs b/ILSpy.Tests/AssemblyList/ReloadAssemblyContextMenuTests.cs index 35b57774c..f3b50c9ca 100644 --- a/ILSpy.Tests/AssemblyList/ReloadAssemblyContextMenuTests.cs +++ b/ILSpy.Tests/AssemblyList/ReloadAssemblyContextMenuTests.cs @@ -116,9 +116,7 @@ public class ReloadAssemblyContextMenuTests var menu = pane.BuildContextMenuForCurrentState(registry.Entries); menu.Should().NotBeNull(); - var reloadItem = menu!.Items.OfType() - .Single(i => (string?)i.Header == Resources._Reload); - reloadItem.RaiseEvent(new RoutedEventArgs(MenuItem.ClickEvent)); + menu!.ClickItem(Resources._Reload); window.Capture("reload-clicked"); // Assert โ€” the LoadedAssembly for that file name is a different instance now. diff --git a/ILSpy.Tests/AssemblyList/RemoveAssemblyContextMenuTests.cs b/ILSpy.Tests/AssemblyList/RemoveAssemblyContextMenuTests.cs index f9193e90e..e6b6e3045 100644 --- a/ILSpy.Tests/AssemblyList/RemoveAssemblyContextMenuTests.cs +++ b/ILSpy.Tests/AssemblyList/RemoveAssemblyContextMenuTests.cs @@ -121,9 +121,7 @@ public class RemoveAssemblyContextMenuTests var menu = pane.BuildContextMenuForCurrentState(registry.Entries); menu.Should().NotBeNull(); - var removeItem = menu!.Items.OfType() - .Single(i => (string?)i.Header == Resources._Remove); - removeItem.RaiseEvent(new RoutedEventArgs(MenuItem.ClickEvent)); + menu!.ClickItem(Resources._Remove); window.Capture("remove-clicked"); // Assert โ€” the assembly is gone from the list; the survivor is still there. diff --git a/ILSpy.Tests/ContextMenus/DecompileInNewViewTests.cs b/ILSpy.Tests/ContextMenus/DecompileInNewViewTests.cs index 2eebe4374..e4de6a9c4 100644 --- a/ILSpy.Tests/ContextMenus/DecompileInNewViewTests.cs +++ b/ILSpy.Tests/ContextMenus/DecompileInNewViewTests.cs @@ -19,9 +19,15 @@ using System.Linq; using System.Threading.Tasks; +using Avalonia; using Avalonia.Controls; +using Avalonia.Controls.DataGridHierarchical; +using Avalonia.Headless; using Avalonia.Headless.NUnit; +using Avalonia.Input; using Avalonia.Interactivity; +using Avalonia.Threading; +using Avalonia.VisualTree; using AwesomeAssertions; @@ -99,13 +105,14 @@ public class DecompileInNewViewTests .Should().NotContain(header); } - // audit (2026-05-12): converted ๐Ÿ“ฆ โ†’ ๐Ÿงช. Was firing `entry.Execute(synthetic ctx)` - // directly; now selects the second method via the model and clicks the - // "Decompile in new tab" MenuItem produced by the live menu-build path. The Click - // routed event hits the same handler ContextMenuProvider attached during Build โ€” - // matching what the user's right-click โ†’ menu-item-click triggers. + // audit (2026-06-02): rewritten for the Thunderbird-style context target. Previously this + // test selected the second method via the model first โ€” encoding the OLD bug where + // right-clicking a row moved the selection there before the menu opened, so you ended up + // with the right-clicked node twice (the preview reused for it + the new tab) instead of + // "keep A, open B". Now A stays the active document and the menu targets B directly, so the + // result is A + B: the new tab shows B and A is untouched. [AvaloniaTest] - public async Task Clicking_DecompileInNewView_Spawns_A_New_Tab_For_The_Selected_Method() + public async Task Clicking_DecompileInNewView_Opens_The_Right_Clicked_Method_While_Keeping_The_Active_One() { var window = AppComposition.Current.GetExport(); window.Show(); @@ -119,41 +126,164 @@ public class DecompileInNewViewTests var secondMethod = typeNode.Children.OfType() .First(m => m.MethodDefinition.Name == "Empty"); - // Seed an existing decompile in the active tab so we have a "current" tab on screen - // before the gesture; that tab will be reused by the right-click's selection move - // (production behaviour โ€” right-clicking a row in the DataGrid moves selection there - // before the menu opens), so the assertion we care about is "a *new* tab spawned in - // addition to the existing one and shows the dispatched method". + // A is the active document: select firstMethod and decompile it into the preview tab. vm.AssemblyTreeModel.SelectNode(firstMethod); var firstTab = await vm.DockWorkspace.WaitForDecompiledTextAsync(); - window.Capture("first-method-decompiled-into-active-tab"); + firstTab.Text.Should().Contain("AsEnumerable"); + window.Capture("first-method-active"); var pane = await window.WaitForComponent(); var registry = AppComposition.Current.GetExport(); var documents = ((ILSpyDockFactory)vm.DockWorkspace.Factory).Documents!; var initialCount = documents.VisibleDockables?.Count ?? 0; - // Select the second method and click the menu entry through the live context-menu path. - vm.AssemblyTreeModel.SelectNode(secondMethod); - await vm.DockWorkspace.WaitForDecompiledTextAsync(); - window.Capture("second-method-replaced-active-tab"); - - var menu = pane.BuildContextMenuForCurrentState(registry.Entries); + // Right-click B (secondMethod) WITHOUT moving the selection, and click "Decompile to + // new tab" from the menu the right-click would produce. + var menu = pane.BuildContextMenuForCurrentState(registry.Entries, rightClickedNode: secondMethod); menu.Should().NotBeNull(); - var item = menu!.Items.OfType() - .Single(i => (string?)i.Header == Resources.DecompileToNewPanel); - item.RaiseEvent(new RoutedEventArgs(MenuItem.ClickEvent)); + menu!.ClickItem(Resources.DecompileToNewPanel); await Waiters.WaitForAsync( () => (documents.VisibleDockables?.Count ?? 0) > initialCount); var newTab = await vm.DockWorkspace.WaitForDecompiledTextAsync(); + + // The new tab shows B... ReferenceEquals(newTab, firstTab).Should().BeFalse( "a fresh decompiler tab must be created instead of reusing the existing one"); newTab.Text.Should().Contain("Empty"); - // Count grew by one โ€” the click added a tab on top of the right-click selection's reuse - // of firstTab. (The right-click selection move into secondMethod re-uses firstTab; the - // menu-click then creates an additional tab. Net effect: +1 dockable.) + // ...the original tab still shows A (it was NOT yanked to B by the right-click โ€” this is + // the whole point: you keep A and gain B, instead of ending up with B twice)... + firstTab.Text.Should().Contain("AsEnumerable"); + // ...and exactly one tab was added (A + B), not two copies of B. documents.VisibleDockables!.Count.Should().Be(initialCount + 1); - window.Capture("after-click-new-tab-spawned"); + window.Capture("a-kept-b-opened"); + } + + [AvaloniaTest] + public async Task Right_Clicking_An_Unselected_Row_Does_Not_Move_The_Selection() + { + // Thunderbird-style context target: right-clicking a row the user has NOT selected must + // leave the real selection (and therefore the preview document) untouched. Today + // ProDataGrid's default selects the right-clicked row on press, which yanks the preview + // to B before the menu opens โ€” so "Decompile to new tab" ends up with B twice instead of + // the intended A + B. This probe asserts the selection stays put under a real right-click. + + 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); + + // Make A the active document/selection. + vm.AssemblyTreeModel.SelectNode(nodeA); + + // Let the top-level rows realise and layout settle. + for (int i = 0; i < 8; i++) + { + Dispatcher.UIThread.RunJobs(); + grid.UpdateLayout(); + await Task.Delay(25); + } + + var rowB = grid.GetVisualDescendants().OfType() + .FirstOrDefault(r => RowNodeEquals(r, nodeB)); + rowB.Should().NotBeNull("the top-level CoreLib row must be realised"); + + // Suppressing the right-press must not also suppress the context menu โ€” it still opens + // from ContextRequested (raised on release). Watch for it. + var menuRequested = false; + grid.AddHandler(Control.ContextRequestedEvent, (_, _) => menuRequested = true, + handledEventsToo: true); + + // Right-click the centre of B's row (clear of the far-left expander glyph). + var point = rowB!.TranslatePoint(new Point(rowB.Bounds.Width / 2, rowB.Bounds.Height / 2), window); + point.Should().NotBeNull(); + HeadlessWindowExtensions.MouseDown(window, point!.Value, MouseButton.Right); + HeadlessWindowExtensions.MouseUp(window, point.Value, MouseButton.Right); + for (int i = 0; i < 4; i++) + { + Dispatcher.UIThread.RunJobs(); + await Task.Delay(20); + } + + ReferenceEquals(vm.AssemblyTreeModel.SelectedItem, nodeA).Should().BeTrue( + "right-clicking an unselected row must not change the selection (Thunderbird-style context target)"); + menuRequested.Should().BeTrue( + "the context menu must still be requested after the suppressed right-press"); + rowB.Classes.Should().Contain("contextTarget", + "the right-clicked row must carry the focus-box style class while it is the menu target"); } + + [AvaloniaTest] + public async Task Right_Clicking_A_Second_Row_Moves_The_Context_Highlight_To_It() + { + // Regression: the focus box must follow every right-click, not stick after the first one + // and then never appear again. Reproduces "right-click B (highlight), dismiss, right-click + // C (no highlight)". + 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 to click"); + var nodeA = assemblies[0]; + var nodeB = assemblies[1]; + var nodeC = assemblies[2]; + + vm.AssemblyTreeModel.SelectNode(nodeA); + for (int i = 0; i < 8; i++) + { + Dispatcher.UIThread.RunJobs(); + grid.UpdateLayout(); + await Task.Delay(25); + } + + DataGridRow Row(SharpTreeNode node) => grid.GetVisualDescendants().OfType() + .First(r => RowNodeEquals(r, node)); + + async Task RightClick(SharpTreeNode node) + { + var row = Row(node); + var pt = row.TranslatePoint(new Point(row.Bounds.Width / 2, row.Bounds.Height / 2), window); + HeadlessWindowExtensions.MouseDown(window, pt!.Value, MouseButton.Right); + HeadlessWindowExtensions.MouseUp(window, pt.Value, MouseButton.Right); + for (int i = 0; i < 4; i++) + { + Dispatcher.UIThread.RunJobs(); + await Task.Delay(20); + } + } + + async Task Dismiss() + { + window.KeyPress(Key.Escape, RawInputModifiers.None, PhysicalKey.Escape, keySymbol: null); + for (int i = 0; i < 4; i++) + { + Dispatcher.UIThread.RunJobs(); + await Task.Delay(20); + } + } + + await RightClick(nodeB); + Row(nodeB).Classes.Should().Contain("contextTarget", "the first right-click highlights B"); + + // Dismiss B's menu, then right-click C โ€” the user's exact sequence. + await Dismiss(); + await RightClick(nodeC); + Row(nodeC).Classes.Should().Contain("contextTarget", + "a second right-click (after dismissing the first menu) must highlight C"); + Row(nodeB).Classes.Should().NotContain("contextTarget", + "B's highlight must be gone"); + } + + static bool RowNodeEquals(DataGridRow row, SharpTreeNode node) + => row.DataContext is HierarchicalNode hn && ReferenceEquals(hn.Item, node); + } diff --git a/ILSpy.Tests/TestHarness.cs b/ILSpy.Tests/TestHarness.cs index f0728cf8e..39671d189 100644 --- a/ILSpy.Tests/TestHarness.cs +++ b/ILSpy.Tests/TestHarness.cs @@ -21,6 +21,9 @@ using System.Linq; using System.Threading.Tasks; using System.Windows.Input; +using Avalonia.Controls; +using Avalonia.Interactivity; + using ICSharpCode.ILSpy.Properties; using ICSharpCode.ILSpyX; @@ -83,6 +86,23 @@ public static class TestHarness .Value; } + /// + /// Finds the whose Header equals (pass + /// the resolved resource string, e.g. Resources._Reload) in a menu built by + /// AssemblyListPane.BuildContextMenuForCurrentState and raises its Click โ€” the + /// same routed event the production menu fires when the user picks the item. Collapses the + /// Items.OfType<MenuItem>().Single(...).RaiseEvent(...) dance every context-menu + /// test repeats. + /// + public static void ClickItem(this ContextMenu menu, string header) + { + ArgumentNullException.ThrowIfNull(menu); + ArgumentNullException.ThrowIfNull(header); + var item = menu.Items.OfType() + .Single(i => string.Equals((string?)i.Header, header, StringComparison.Ordinal)); + item.RaiseEvent(new RoutedEventArgs(MenuItem.ClickEvent)); + } + /// /// Opens through the production Open command, waits for it to appear /// in the assembly list, awaits its load result, and returns the resulting diff --git a/ILSpy/App.axaml b/ILSpy/App.axaml index 0241d76c4..1bef450a4 100644 --- a/ILSpy/App.axaml +++ b/ILSpy/App.axaml @@ -55,6 +55,7 @@ + @@ -92,6 +93,7 @@ + diff --git a/ILSpy/AssemblyTree/AssemblyListPane.axaml b/ILSpy/AssemblyTree/AssemblyListPane.axaml index 7a839a8d2..56aff2cea 100644 --- a/ILSpy/AssemblyTree/AssemblyListPane.axaml +++ b/ILSpy/AssemblyTree/AssemblyListPane.axaml @@ -16,6 +16,15 @@ + + { + if (!ReferenceEquals(contextTargetRow, contextMenuOpenRow)) + return; + contextMenuTargetNode = null; + SetContextTargetRow(null); + }; TreeGrid.ContextMenu = menu; } @@ -172,6 +222,9 @@ namespace ILSpy.AssemblyTree { if (sender is not ContextMenu menu) return; + // Remember which row this menu belongs to, so its later Closed only clears the + // highlight if no newer right-click has moved the target to a different row. + contextMenuOpenRow = contextTargetRow; var built = BuildContextMenuForCurrentState(contextMenuEntries); if (built == null) { @@ -196,10 +249,38 @@ namespace ILSpy.AssemblyTree internal ContextMenu? BuildContextMenuForCurrentState(IReadOnlyList entries) => ContextMenuProvider.Build(entries, CreateContextMenuContext()); + /// + /// Test seam: builds the menu as if had been + /// right-clicked (the Thunderbird-style context target), without simulating a pointer + /// gesture on a possibly-unrealised deep row. Production sets the same target from the + /// tunnel right-press handler. The built menu captures the resulting context, so the + /// target is cleared again before returning. + /// + internal ContextMenu? BuildContextMenuForCurrentState( + IReadOnlyList entries, SharpTreeNode? rightClickedNode) + { + contextMenuTargetNode = rightClickedNode; + try + { + return ContextMenuProvider.Build(entries, CreateContextMenuContext()); + } + finally + { + contextMenuTargetNode = null; + } + } + TextViewContext CreateContextMenuContext() { - var nodes = (DataContext as AssemblyTreeModel)?.SelectedItems.ToArray() + var selection = (DataContext as AssemblyTreeModel)?.SelectedItems.ToArray() ?? System.Array.Empty(); + // A right-click outside the current selection targets just the clicked row, leaving + // the real selection (and preview document) untouched. A right-click inside the + // selection, or a keyboard-invoked menu (no target), acts on the whole selection. + var target = contextMenuTargetNode; + var nodes = target != null && !System.Array.Exists(selection, n => ReferenceEquals(n, target)) + ? new SharpTreeNode[] { target } + : selection; return new TextViewContext { TreeGrid = TreeGrid, SelectedTreeNodes = nodes, @@ -403,6 +484,43 @@ namespace ILSpy.AssemblyTree } } + void OnTreeGridContextRequested(object? sender, ContextRequestedEventArgs e) + { + // Resolve the row under the pointer (mouse / touch context gesture). A keyboard-invoked + // menu (Shift+F10 / Menu key) carries no position -> no target -> the menu acts on the + // real selection. Each request is a new generation so a stale menu's Closed can't wipe + // this fresh highlight. + SharpTreeNode? node = null; + DataGridRow? row = null; + if (e.TryGetPosition(TreeGrid, out var pos) && TreeGrid.InputHitTest(pos) is Visual hit) + { + node = HitTestRowNode(hit); + row = hit.GetVisualAncestors().OfType().FirstOrDefault(); + } + contextMenuTargetNode = node; + SetContextTargetRow(node != null ? row : null); + } + + void OnTreeGridPointerPressedPreview(object? sender, PointerPressedEventArgs e) + { + if (e.Source is not Visual hit) + return; + if (!e.GetCurrentPoint(hit).Properties.IsRightButtonPressed) + { + // Any non-right press (left navigation, middle new-tab) starts a fresh gesture โ€” + // drop a stale right-click target and its focus box. + contextMenuTargetNode = null; + SetContextTargetRow(null); + return; + } + // Right press over a row: swallow it so ProDataGrid doesn't move the selection there. + // We do NOT capture the menu target here โ€” when a previous menu is open this press is + // eaten by that menu's light-dismiss popup and never reaches us, so capture happens in + // OnTreeGridContextRequested instead (it fires on every menu open, including reopens). + if (HitTestRowNode(hit) != null) + e.Handled = true; + } + void OnTreeGridPointerPressed(object? sender, PointerPressedEventArgs e) { // MMB single-click is the only "open the row's node in a new tab" pointer gesture โ€”