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