Tell the menu's own items apart from the tracked ones by identity

src/DiffEngineTray.Tests/MenuBuilderTest.cs

@@ -213,6 +213,84 @@ public async Task OnlyInline()
         await Verify(menu, settings);
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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<System.Windows.Forms.ToolStripItem>()
+                .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);
+        }
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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<System.Windows.Forms.ToolStripItem>()
+            .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();

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<ToolStripItem>();
+        // 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<ToolStripItem>();
         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<ToolStripItem>());
         return menu;
     }
 
     /// <summary>
-    /// 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 <see cref="Build"/> 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 <see cref="Build" /> put
+    /// there. Materialised, because the caller is removing them from the collection it reads.
     /// </summary>
-    static readonly string[] fixedItems =
-    [
-        "Exit",
-        "Options",
-        "Debug view",
-        "Open logs",
-        "Purge verified files",
-        "Raise issue"
-    ];
-
-    static List<ToolStripItem> NonDefaultMenus(ToolStripItemCollection items) =>
+    static List<ToolStripItem> TrackingMenuItems(ToolStripItemCollection items, HashSet<ToolStripItem> fixedItems) =>
         items
             .Cast<ToolStripItem>()
-            .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<ToolStripItem> BuildTrackingMenuItems(Tracker tracker)
     {
         // Read everything first and decide from the counts. TrackingAny is backed by the scan