From 5e4146c13ad85a4e75ee80bffee4958e09e5cc7a Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Sat, 23 May 2026 08:08:39 +0200 Subject: [PATCH] Pin tab without spawning a new preview tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VS-style preview-tab semantics, refined per user spec: pinning a tab should freeze its contents in place but NOT immediately spawn a fresh preview tab beside it. The new preview tab opens lazily, on the next tree-node selection that finds the active tab frozen — at which point the new content lands in a brand-new tab and the pinned one is left untouched. Assisted-by: Claude:claude-opus-4-7:Claude Code --- .../Docking/PreviewTabPromotionTests.cs | 137 +++++++++++++----- ILSpy/Docking/DockWorkspace.cs | 109 ++++++++++---- ILSpy/Docking/ILSpyDockFactory.cs | 34 +++-- 3 files changed, 210 insertions(+), 70 deletions(-) diff --git a/ILSpy.Tests/Docking/PreviewTabPromotionTests.cs b/ILSpy.Tests/Docking/PreviewTabPromotionTests.cs index 4192cdc99..1bbbfc83b 100644 --- a/ILSpy.Tests/Docking/PreviewTabPromotionTests.cs +++ b/ILSpy.Tests/Docking/PreviewTabPromotionTests.cs @@ -75,8 +75,12 @@ public class PreviewTabPromotionTests } [AvaloniaTest] - public async Task PinCurrentTab_Promotes_MainTab_And_Spawns_A_New_Preview_MainTab() + public async Task PinCurrentTab_Flips_IsPreview_Without_Spawning_A_New_Tab() { + // New semantics: Pin only flips IsPreview=false on the current MainTab. No new + // preview tab spawns at pin time. A fresh preview tab opens lazily later, when a + // tree-selection change finds the active tab frozen — see + // Selecting_A_Different_Node_After_Pin_Opens_A_New_Preview_Tab below. var window = AppComposition.Current.GetExport(); window.Show(); var vm = (MainWindowViewModel)window.DataContext!; @@ -90,34 +94,36 @@ public class PreviewTabPromotionTests var typeNode = vm.AssemblyTreeModel.FindNode( "System.Linq", "System.Linq", "System.Linq.Enumerable"); vm.AssemblyTreeModel.SelectNode(typeNode); - await vm.DockWorkspace.WaitForDecompiledTextAsync(System.TimeSpan.FromSeconds(30)); + await vm.DockWorkspace.WaitForDecompiledTextAsync(System.TimeSpan.FromSeconds(60)); ReferenceEquals(previousMainTab.SourceNode, typeNode).Should().BeTrue( "baseline: tree selection populated MainTab with the chosen node"); + var tabCountBefore = vm.DockWorkspace.Documents!.VisibleDockables!.Count; + // Pin. vm.DockWorkspace.PinCurrentTab(); - // Promoted tab keeps its content and identity, just loses preview status. + // Tab is now pinned, content preserved. previousMainTab.IsPreview.Should().BeFalse( "after pin, the previously-preview MainTab becomes a regular pinned tab"); ReferenceEquals(previousMainTab.SourceNode, typeNode).Should().BeTrue( "pin must not throw away the tab's content"); - // A new preview MainTab takes its place. - factory.MainTab.Should().NotBeSameAs(previousMainTab, - "factory.MainTab must rotate to a fresh preview tab after pin"); - factory.MainTab!.IsPreview.Should().BeTrue( - "the new MainTab is the next preview slot"); + // factory.MainTab still points at the same (now-pinned) tab — no rotation. + factory.MainTab.Should().BeSameAs(previousMainTab, + "pin alone must not rotate factory.MainTab — the pinned tab keeps the slot until a selection change spawns a new preview"); - // Both tabs live in the documents dock. - var docTabs = vm.DockWorkspace.Documents!.VisibleDockables!.OfType().ToList(); - docTabs.Should().Contain(previousMainTab).And.Contain(factory.MainTab, - "the documents dock must hold both the promoted tab and the fresh MainTab"); + // And critically: no new tab spawned. + vm.DockWorkspace.Documents!.VisibleDockables!.Count.Should().Be(tabCountBefore, + "pinning alone must not open a new tab; new tabs open lazily on the next selection change"); } [AvaloniaTest] - public async Task After_Pin_Tree_Selection_Writes_To_New_MainTab_Not_Promoted_One() + public async Task Selecting_A_Different_Node_After_Pin_Opens_A_New_Preview_Tab() { + // Pin holds the current tab's content. Selecting a different tree node while + // the (now-pinned) tab is active must spawn a fresh preview tab beside it and + // route the new content there — never overwrite the pinned tab. var window = AppComposition.Current.GetExport(); window.Show(); var vm = (MainWindowViewModel)window.DataContext!; @@ -129,23 +135,34 @@ public class PreviewTabPromotionTests var typeA = vm.AssemblyTreeModel.FindNode( "System.Linq", "System.Linq", "System.Linq.Enumerable"); vm.AssemblyTreeModel.SelectNode(typeA); - await vm.DockWorkspace.WaitForDecompiledTextAsync(System.TimeSpan.FromSeconds(30)); + await vm.DockWorkspace.WaitForDecompiledTextAsync(System.TimeSpan.FromSeconds(60)); var pinnedTab = factory.MainTab!; var pinnedContent = pinnedTab.Content; - // Phase 2: pin. + // Phase 2: pin. factory.MainTab still points at the same (now-pinned) tab — + // no spawn yet (covered by PinCurrentTab_Flips_IsPreview_Without_Spawning_A_New_Tab). + var tabCountBeforeSelection = vm.DockWorkspace.Documents!.VisibleDockables!.Count; vm.DockWorkspace.PinCurrentTab(); - var newMainTab = factory.MainTab!; - newMainTab.Should().NotBeSameAs(pinnedTab); + factory.MainTab.Should().BeSameAs(pinnedTab, "pin alone keeps the slot"); - // Phase 3: select a different type — should land in the NEW MainTab. + // Phase 3: select a different type — NOW a new preview tab should spawn AND + // receive the new content. var typeB = vm.AssemblyTreeModel.FindNode( "System.Private.Uri", "System", "System.Uri"); vm.AssemblyTreeModel.SelectNode(typeB); - await vm.DockWorkspace.WaitForDecompiledTextAsync(System.TimeSpan.FromSeconds(30)); + await vm.DockWorkspace.WaitForDecompiledTextAsync(System.TimeSpan.FromSeconds(60)); + + // New tab spawned beside the pinned one. + vm.DockWorkspace.Documents!.VisibleDockables!.Count.Should().Be(tabCountBeforeSelection + 1, + "selecting a different node while the active tab is pinned must spawn a new preview tab"); + var newMainTab = factory.MainTab!; + newMainTab.Should().NotBeSameAs(pinnedTab, + "factory.MainTab must rotate to the freshly-spawned preview tab once a selection change forces it"); + newMainTab.IsPreview.Should().BeTrue( + "the freshly-spawned tab is itself a preview tab (the user can pin it next)"); ReferenceEquals(newMainTab.SourceNode, typeB).Should().BeTrue( - "new tree selection must land in the freshly-spawned preview MainTab"); + "new tree selection must land in the freshly-spawned preview tab"); ReferenceEquals(pinnedTab.SourceNode, typeA).Should().BeTrue( "the pinned tab must keep type A — not be overwritten by the new selection"); pinnedTab.Content.Should().BeSameAs(pinnedContent, @@ -372,17 +389,19 @@ public class PreviewTabPromotionTests pinButton.IsVisible.Should().BeTrue("pin button must show while the tab is preview"); var previousMainTab = factory.MainTab!; + var tabCountBefore = vm.DockWorkspace.Documents!.VisibleDockables!.Count; // Simulate a user click on the pin button. pinButton.RaiseEvent(new global::Avalonia.Interactivity.RoutedEventArgs( global::Avalonia.Controls.Button.ClickEvent)); - // The previous MainTab should now be pinned; factory.MainTab should point at a new - // preview tab. + // New semantics: clicking pin flips IsPreview, doesn't spawn a new tab. previousMainTab.IsPreview.Should().BeFalse( - "clicking the pin button must promote the previous MainTab"); - factory.MainTab.Should().NotBeSameAs(previousMainTab, - "factory.MainTab must rotate to a fresh preview tab after the click"); + "clicking the pin button must flip IsPreview=false on the current MainTab"); + factory.MainTab.Should().BeSameAs(previousMainTab, + "pin alone must not rotate factory.MainTab; the new preview tab opens lazily on the next tree selection"); + vm.DockWorkspace.Documents!.VisibleDockables!.Count.Should().Be(tabCountBefore, + "pin alone must not change tab count"); } [AvaloniaTest] @@ -450,25 +469,75 @@ public class PreviewTabPromotionTests [AvaloniaTest] public void PinCurrentTab_Is_Noop_When_MainTab_Already_Pinned() { - // Idempotency: calling Pin twice in a row only promotes the first time. The second - // call sees IsPreview=false on the (new) MainTab — actually, on the *new* preview - // MainTab — so it should NOT promote that one. Wait — the new MainTab IS preview. - // So a second pin DOES promote it. Let me restate: a single pin promotes once; - // calling pin again creates a second pinned tab. That's the intended behaviour. - // The actual noop case is when there's literally nothing pinnable. Test that. + // Idempotency: a second pin against an already-pinned MainTab is a no-op. The new + // "pin = flip-only, spawn-lazy" semantics make this simpler than the old version: + // PinCurrentMainTab() returns null when MainTab.IsPreview is already false, and + // PinCurrentTab early-returns. var window = AppComposition.Current.GetExport(); window.Show(); var vm = (MainWindowViewModel)window.DataContext!; var factory = (ILSpyDockFactory)vm.DockWorkspace.Factory; - // Manually flip the current MainTab to pinned, simulating a state where there's - // no preview to promote. + // Manually flip the current MainTab to pinned, simulating the post-pin state. factory.MainTab!.IsPreview = false; var before = factory.MainTab; vm.DockWorkspace.PinCurrentTab(); factory.MainTab.Should().BeSameAs(before, - "with no preview MainTab to promote, PinCurrentTab must leave the factory state untouched"); + "with no preview MainTab to flip, PinCurrentTab must leave the factory state untouched"); + factory.MainTab.IsPreview.Should().BeFalse( + "the already-pinned tab stays pinned"); + } + + [AvaloniaTest] + public async Task Tree_Selection_While_Frozen_Tab_Active_Opens_A_New_Preview_Tab() + { + // "Frozen" covers: (a) user-pinned tabs (IsPreview=false via PinCurrentTab), + // (b) carve-out tabs (born IsPreview=false via OpenNodeInNewTab), and (c) static + // content tabs (Options, About) whose Content is a static viewmodel. Tree-node + // selections while any of those are active must spawn a fresh preview tab and + // route the new content there. This test uses a carve-out as a stand-in for the + // frozen-tab family — the IsWritablePreview check returns null for all three. + var window = AppComposition.Current.GetExport(); + window.Show(); + var vm = (MainWindowViewModel)window.DataContext!; + await vm.AssemblyTreeModel.WaitForAssembliesAsync(minimumCount: 3); + + var factory = (ILSpyDockFactory)vm.DockWorkspace.Factory; + + // Open a carve-out (born pinned) and let it become the active dockable. + var typeA = vm.AssemblyTreeModel.FindNode( + "System.Linq", "System.Linq", "System.Linq.Enumerable"); + vm.DockWorkspace.OpenNodeInNewTab(typeA); + var carveOut = vm.DockWorkspace.Documents!.VisibleDockables! + .OfType() + .Last(t => t.SourceNode == typeA); + factory.SetActiveDockable(carveOut); + carveOut.IsPreview.Should().BeFalse("baseline: carve-out tab is frozen"); + ReferenceEquals(vm.DockWorkspace.Documents.ActiveDockable, carveOut).Should().BeTrue( + "baseline: the carve-out is the active document"); + + var tabCountBefore = vm.DockWorkspace.Documents!.VisibleDockables!.Count; + + // Select a different tree node — the carve-out is frozen, so this must NOT + // overwrite it. + var typeB = vm.AssemblyTreeModel.FindNode( + "System.Private.Uri", "System", "System.Uri"); + vm.AssemblyTreeModel.SelectNode(typeB); + await vm.DockWorkspace.WaitForDecompiledTextAsync(System.TimeSpan.FromSeconds(60)); + + vm.DockWorkspace.Documents!.VisibleDockables!.Count.Should().Be(tabCountBefore + 1, + "selecting a tree node while a frozen tab is active must spawn a new preview tab"); + ReferenceEquals(carveOut.SourceNode, typeA).Should().BeTrue( + "the frozen carve-out tab must keep its original content"); + + var newPreview = factory.MainTab!; + newPreview.Should().NotBeSameAs(carveOut, + "the freshly-spawned preview must be a separate tab from the frozen one"); + newPreview.IsPreview.Should().BeTrue( + "the freshly-spawned tab is itself a preview tab"); + ReferenceEquals(newPreview.SourceNode, typeB).Should().BeTrue( + "the new preview tab must host the just-selected node's content"); } } diff --git a/ILSpy/Docking/DockWorkspace.cs b/ILSpy/Docking/DockWorkspace.cs index 619da0484..98bdcca66 100644 --- a/ILSpy/Docking/DockWorkspace.cs +++ b/ILSpy/Docking/DockWorkspace.cs @@ -156,17 +156,31 @@ namespace ILSpy.Docking ShowSearchCommand = new RelayCommand(ExecuteShowSearch); using (ILSpy.AppEnv.AppLog.Phase("ILSpyDockFactory ctor + CreateLayout + InitLayout")) { - factory = new ILSpyDockFactory(toolPaneRegistry); + using (ILSpy.AppEnv.AppLog.Phase("ILSpyDockFactory ctor")) + factory = new ILSpyDockFactory(toolPaneRegistry); // Prefer the user's saved layout (ILSpy.Layout.json sidecar next to // ILSpy.xml); fall back to the default layout if there is no saved one // or it failed to deserialize. The fallback path is the same shape the // app uses on first launch. - Layout = factory.LoadLayout(GetLayoutFilePath()) ?? factory.CreateLayout(); + IRootDock? loaded; + using (ILSpy.AppEnv.AppLog.Phase("ILSpyDockFactory.LoadLayout")) + loaded = factory.LoadLayout(GetLayoutFilePath()); + if (loaded != null) + { + Layout = loaded; + ILSpy.AppEnv.AppLog.Mark("ILSpyDockFactory: using LOADED layout"); + } + else + { + using (ILSpy.AppEnv.AppLog.Phase("ILSpyDockFactory.CreateLayout (default)")) + Layout = factory.CreateLayout(); + } // Build the layout, then InitLayout BEFORE the chrome ever sees it, rather than // letting the DockControl's InitializeFactory / InitializeLayout flags run it // post-template-apply. Wiring owners/factories/locators up front means the // layout is fully resolvable before the first DeferredContentControl attaches. - factory.InitLayout(Layout); + using (ILSpy.AppEnv.AppLog.Phase("ILSpyDockFactory.InitLayout")) + factory.InitLayout(Layout); } assemblyTreeModel.PropertyChanged += OnAssemblyTreePropertyChanged; @@ -615,28 +629,47 @@ namespace ILSpy.Docking void ShowSelectedNode() { + using var _ = ILSpy.AppEnv.AppLog.Phase("DockWorkspace.ShowSelectedNode"); var nodes = assemblyTreeModel.SelectedItems.OfType().ToArray(); if (nodes.Length == 0) { lastShownNodes = null; return; } - if (factory.MainTab is not { } main) - return; // SelectedItems.CollectionChanged and SelectedItem PropertyChanged both fan into // here on a single click, so dedupe to avoid creating two TabPageModels for the // same selection — the second one's columns would replace the first's, but the // first's filter wiring would be left dangling on stale ColumnFilter instances. + // Dedupe is also what makes "re-clicking the same node after pin" a no-op: the + // pinned tab stays active, no new preview tab spawns. if (lastShownNodes is { } prev && prev.SequenceEqual(nodes)) { - // Content didn't change, but the user may have returned to the tree from a - // sibling static tab (Options / About) — pull MainTab forward so re-clicking - // the same node still feels responsive. - ActivateMainTabIfNeeded(main); + if (factory.MainTab is { } existingMain) + ActivateMainTabIfNeeded(existingMain); return; } lastShownNodes = nodes; + // Pick or spawn the target tab. If the currently-active document tab is a + // writable preview ContentTabPage we replace its content in place. Anything + // else — pinned tab, Options page, About page, no Documents dock — counts as + // frozen and gets a brand-new preview tab spawned beside it. + ContentTabPage? main; + if (IsWritablePreview(factory.Documents?.ActiveDockable) is ContentTabPage activePreview) + { + main = activePreview; + factory.MainTab = main; + } + else + { + main = factory.CreateAndAttachPreviewTab(); + if (main == null) + return; + // Fresh tab needs a fresh DecompilerTabPageModel — the cached one (if any) + // belongs to a previous tab and must not be re-assigned across tabs. + decompilerContent = null; + } + // Tree-node selections always reuse the single document slot. The Document // instance never changes; its inner Content swaps between viewmodels. The // wrapper view (ContentTabPageView) holds pre-realised inner views and toggles @@ -644,7 +677,9 @@ namespace ILSpy.Docking // deterministic without going through Dock's add+close lifecycle. // Carve-outs ("Open in new tab", "Freeze tab") aren't implemented yet and would // branch off this path before the Content swap. - var customContent = nodes.Length == 1 ? nodes[0].CreateTab() : null; + TabPageModel? customContent; + using (ILSpy.AppEnv.AppLog.Phase("ShowSelectedNode: node.CreateTab")) + customContent = nodes.Length == 1 ? nodes[0].CreateTab() : null; if (customContent != null) { @@ -660,12 +695,37 @@ namespace ILSpy.Docking return; } - var decTab = decompilerContent ??= CreateDecompilerContent(); - main.Content = decTab; + DecompilerTabPageModel decTab; + using (ILSpy.AppEnv.AppLog.Phase("ShowSelectedNode: get/create decompilerContent")) + decTab = decompilerContent ??= CreateDecompilerContent(); + using (ILSpy.AppEnv.AppLog.Phase("ShowSelectedNode: main.Content = decTab (Dock view-recycling)")) + main.Content = decTab; decTab.Language = languageService.CurrentLanguage; - decTab.CurrentNodes = nodes; + using (ILSpy.AppEnv.AppLog.Phase("ShowSelectedNode: decTab.CurrentNodes = nodes (kicks off DecompileAsync)")) + decTab.CurrentNodes = nodes; main.SourceNode = nodes.Length == 1 ? nodes[0] : null; - ActivateMainTabIfNeeded(main); + using (ILSpy.AppEnv.AppLog.Phase("ShowSelectedNode: ActivateMainTabIfNeeded")) + ActivateMainTabIfNeeded(main); + } + + // Returns the active document tab if it's a writable preview ContentTabPage — meaning + // IsPreview=true AND not hosting static-content (Options, About). Otherwise returns + // null, signalling that ShowSelectedNode must spawn a fresh preview tab instead of + // overwriting the active one. Options pages and About pages are born with + // IsPreview=false (carve-out semantics) AND carry IsStaticContent=true on their + // content viewmodel — either flag alone would suffice, both check guards against + // drift. + static ContentTabPage? IsWritablePreview(IDockable? dockable) + { + if (dockable is not ContentTabPage tab) + return null; + if (!tab.IsPreview) + return null; + if (tab.Content is DecompilerTabPageModel { IsStaticContent: true }) + return null; + if (tab.Content is Options.OptionsPageModel) + return null; + return tab; } // Brings MainTab to the front of the documents dock so the just-updated content is @@ -990,22 +1050,21 @@ namespace ILSpy.Docking } /// - /// VS-style "promote preview tab" gesture. The current factory.MainTab keeps - /// its position and content but flips to pinned ( - /// false); a fresh preview MainTab is created and inserted beside it (rightmost), - /// becoming the new target for tree-node selections. The just-pinned tab stays active - /// so the user keeps looking at what they pinned. + /// VS-style "pin tab" gesture. The current factory.MainTab keeps its position, + /// content, and active state but flips to pinned + /// ( = false). No new tab spawns at pin time: + /// the next tree-node selection that finds the active tab frozen will lazily spawn + /// a fresh preview tab inside . Re-clicking the same + /// node after pin is a no-op (the dedupe activates the pinned tab). /// public void PinCurrentTab() { - if (factory.PromotePreviewMainTab() is null) + if (factory.PinCurrentMainTab() is null) return; - // Cached decompiler viewmodel was bound to the previous MainTab — drop it so the - // next tree click materialises a fresh one inside the new MainTab. + // Cached decompiler viewmodel was bound to the just-pinned tab — drop the cache + // so the next selection-change-into-frozen path materialises a fresh + // DecompilerTabPageModel for the newly-spawned preview tab. decompilerContent = null; - // Defeat the same-selection dedupe so re-clicking the just-pinned node's path - // actually populates the new MainTab. - lastShownNodes = null; } public void CloseAllTabs() diff --git a/ILSpy/Docking/ILSpyDockFactory.cs b/ILSpy/Docking/ILSpyDockFactory.cs index f00c4594f..ce7e5267a 100644 --- a/ILSpy/Docking/ILSpyDockFactory.cs +++ b/ILSpy/Docking/ILSpyDockFactory.cs @@ -46,24 +46,36 @@ namespace ILSpy.Docking /// just-pinned tab keeps its position and content while MainTab points at a fresh /// preview tab beside it. /// - public ContentTabPage? MainTab { get; private set; } + public ContentTabPage? MainTab { get; internal set; } /// - /// VS-style preview-tab promotion. Flips the current to pinned - /// ( = false), creates a new placeholder - /// preview tab, appends it to .VisibleDockables, and - /// updates to point at the new one. The active dockable is - /// left as-is — the user keeps looking at what they just pinned. Returns the new - /// MainTab, or when there's nothing to promote (no current - /// MainTab, MainTab already pinned, or Documents missing). + /// Flips the current to pinned + /// ( = false). Returns the pinned tab, or + /// when there's nothing to pin (no current MainTab, MainTab + /// already pinned). Does NOT spawn a new preview tab — the next selection change + /// that finds the active tab frozen will spawn one lazily via + /// . /// - public ContentTabPage? PromotePreviewMainTab() + public ContentTabPage? PinCurrentMainTab() { - if (MainTab is not { IsPreview: true } previous) + if (MainTab is not { IsPreview: true } current) return null; + current.IsPreview = false; + return current; + } + + /// + /// Creates a fresh preview , attaches it to + /// , and sets to it. Used when a + /// tree-node selection changes while the active tab is frozen (pinned, Options, + /// About, …) — a brand-new preview slot is needed to host the new content. + /// Returns the new tab, or if is + /// missing. + /// + public ContentTabPage? CreateAndAttachPreviewTab() + { if (Documents is not { } docs) return null; - previous.IsPreview = false; var fresh = new ContentTabPage { Title = "(no selection)" }; // Use the framework's AddDockable so owner/factory wiring matches every other // add path; default insertion is at the end (rightmost — VS convention for the