From 47eb35ea2a876551958939e4455d8367f042c6f8 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 17:41:29 +1000 Subject: [PATCH] Tell the menu's own items apart from the tracked ones by identity The items MenuBuilder puts in the menu were told from the ones each open adds by their Text, so a solution named after one of them - Options, Exit, Raise issue - had its group header treated as fixed. The header survived the close that removed everything under it, and the next open added another. Matching on text also made the disposal dead. The close removed the tracked items without disposing them, and the next open disposed whatever was still in the collection, which by then was only the fixed six. So every open leaked a menu's worth of controls to the finaliser. Both come from holding the fixed items as themselves. Disposal still happens at the next open rather than during the close that removed them. --- src/DiffEngineTray.Tests/MenuBuilderTest.cs | 78 +++++++++++++++++++++ src/DiffEngineTray/MenuBuilder.cs | 59 +++++++--------- 2 files changed, 102 insertions(+), 35 deletions(-) diff --git a/src/DiffEngineTray.Tests/MenuBuilderTest.cs b/src/DiffEngineTray.Tests/MenuBuilderTest.cs index c2110ead..da2cf11d 100644 --- a/src/DiffEngineTray.Tests/MenuBuilderTest.cs +++ b/src/DiffEngineTray.Tests/MenuBuilderTest.cs @@ -213,6 +213,84 @@ public async Task OnlyInline() await Verify(menu, settings); } + /// + /// The tracked items were told from the fixed ones by their text, so a solution named after + /// one of them kept its group header through the close that removed everything under it, and + /// grew another on the next open. + /// + [Test] + public async Task A_group_named_after_a_fixed_item_is_removed_with_the_rest() + { + var directory = Path.Combine(Path.GetTempPath(), $"MenuBuilderTest_{Guid.NewGuid()}"); + Directory.CreateDirectory(directory); + try + { + File.WriteAllText(Path.Combine(directory, "Options.sln"), ""); + var file = Path.Combine(directory, "file.txt"); + File.WriteAllText(file, ""); + await using var tracker = new RecordingTracker(); + tracker.AddDelete(file); + var menu = MenuBuilder.Build( + emptyAction, + emptyAction, + tracker); + var fixedCount = menu.Items.Count; + + menu.Show(0, 0); + var opened = menu.Items + .Cast() + .Skip(fixedCount) + .Select(_ => _.Text) + .ToList(); + menu.Close(); + + // That the group is there at all, and under the name the rest of this is about + await Assert.That(opened).Contains("Options"); + await Assert.That(menu.Items.Count).IsEqualTo(fixedCount); + } + finally + { + Directory.Delete(directory, true); + } + } + + /// + /// A close removed the tracked items and the next open disposed whatever was still in the + /// collection, which by then was only the fixed ones. So every open leaked a menu's worth of + /// controls to the finaliser. + /// + [Test] + public async Task Tracked_items_are_disposed_by_the_next_open() + { + await using var tracker = new RecordingTracker(); + tracker.AddDelete(file1); + var menu = MenuBuilder.Build( + emptyAction, + emptyAction, + tracker); + var fixedCount = menu.Items.Count; + + menu.Show(0, 0); + var tracked = menu.Items + .Cast() + .Skip(fixedCount) + .ToList(); + // ToolStripItem.IsDisposed stays false through a Dispose, so the event is what says it + // happened + var disposed = 0; + foreach (var item in tracked) + { + item.Disposed += (_, _) => disposed++; + } + + menu.Close(); + menu.Show(0, 0); + menu.Close(); + + await Assert.That(tracked).IsNotEmpty(); + await Assert.That(disposed).IsEqualTo(tracked.Count); + } + public MenuBuilderTest() { settings = new(); diff --git a/src/DiffEngineTray/MenuBuilder.cs b/src/DiffEngineTray/MenuBuilder.cs index c4f986cd..5a53e23c 100644 --- a/src/DiffEngineTray/MenuBuilder.cs +++ b/src/DiffEngineTray/MenuBuilder.cs @@ -7,13 +7,30 @@ public static ContextMenuStrip Build(Action exit, Action launchOptions, Tracker DefaultDropDownDirection = ToolStripDropDownDirection.AboveLeft }; var items = menu.Items; + // The items that survive a close, held as themselves. They used to be told apart by their + // text, so a solution named after one of them - Options, say - gave that group a header + // that was never removed, and another one on every open after that + var fixedItems = new HashSet(); + // Removed while closing and disposed at the next open, rather than disposed while the + // close they were removed by is still running + var removed = new List(); menu.Closed += delegate { - RemovePreviousItems(items); + foreach (var item in TrackingMenuItems(items, fixedItems)) + { + items.Remove(item); + removed.Add(item); + } }; menu.Opening += delegate { - DisposePreviousItems(items); + foreach (var item in removed) + { + item.Dispose(); + } + + removed.Clear(); + foreach (var item in BuildTrackingMenuItems(tracker)) { items.Add(item); @@ -26,48 +43,20 @@ public static ContextMenuStrip Build(Action exit, Action launchOptions, Tracker items.Add(new MenuButton("Open logs", Logging.OpenDirectory, Images.Folder)); items.Add(new MenuButton("Purge verified files", FilePurger.Launch, Images.Folder)); items.Add(new MenuButton("Raise issue", IssueLauncher.Launch, Images.Link)); + fixedItems.UnionWith(items.Cast()); return menu; } /// - /// The items that survive a close, matched by text. The tracked ones are rebuilt from scratch - /// every time the menu opens, so anything added in has to be listed here - /// too or it is removed the first time the menu closes. + /// Everything the last open added, which is everything but the items put + /// there. Materialised, because the caller is removing them from the collection it reads. /// - static readonly string[] fixedItems = - [ - "Exit", - "Options", - "Debug view", - "Open logs", - "Purge verified files", - "Raise issue" - ]; - - static List NonDefaultMenus(ToolStripItemCollection items) => + static List TrackingMenuItems(ToolStripItemCollection items, HashSet fixedItems) => items .Cast() - .Where(_ => !fixedItems.Contains(_.Text)) + .Where(_ => !fixedItems.Contains(_)) .ToList(); - static void RemovePreviousItems(ToolStripItemCollection items) - { - // Use ToList to avoid deferred execution of NonDefaultMenus - foreach (var item in NonDefaultMenus(items)) - { - items.Remove(item); - } - } - - static void DisposePreviousItems(ToolStripItemCollection items) - { - // Use ToList to avoid deferred execution of NonDefaultMenus - foreach (var item in NonDefaultMenus(items)) - { - item.Dispose(); - } - } - static IEnumerable BuildTrackingMenuItems(Tracker tracker) { // Read everything first and decide from the counts. TrackingAny is backed by the scan