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