Reconcile the live bookmark list on save instead of rebuilding it

Every save reloaded the JSON sidecar into fresh instances and then replaced the
whole observable collection with Clear()+Add(). That dropped and re-added every
bookmark on each toggle or edit: the bookmarks pane keeps its selection by
reference, so it was cleared on every change (leaving the toolbar acting on a
stale or null selection), the grid's scroll reset, and a checkbox/name edit made
from the grid re-entered the DataGrid mid-edit while its ItemsSource was torn
down and rebuilt.

Reconcile in place instead: keep the existing instance for any anchor still
present (refreshing only its editable Name/Enabled), remove anchors that are
gone, and append new ones. A name/enabled edit leaves the membership unchanged,
so it now touches the collection not at all; structural changes touch only the
affected rows. Instance identity is preserved, so the pane's selection and the
gutter's references survive.

Assisted-by: Claude:claude-opus-4-8:Claude Code

ILSpy.Tests/Bookmarks/BookmarkManagerTests.cs

@@ -129,6 +129,38 @@ public class BookmarkManagerTests
 		manager.Bookmarks.Should().HaveCount(2);
 	}
 
+	[Test]
+	public void Adding_a_bookmark_preserves_existing_live_instances()
+	{
+		var manager = new BookmarkManager();
+		manager.Toggle(MakeBookmark(0x06000001, name: "A"));
+		var first = manager.Bookmarks[0];
+
+		manager.Toggle(MakeBookmark(0x06000002, name: "B"));
+
+		manager.Bookmarks.Should().HaveCount(2);
+		// A structural change must reconcile the live list, not rebuild it: the bookmarks pane holds a
+		// live selection by reference, so replacing every instance on each toggle would drop it.
+		manager.Bookmarks[0].Should().BeSameAs(first, "adding a bookmark must not replace existing instances");
+	}
+
+	[Test]
+	public void Editing_a_bookmark_keeps_the_other_live_instances_intact()
+	{
+		var manager = new BookmarkManager();
+		manager.Toggle(MakeBookmark(0x06000001, name: "A"));
+		manager.Toggle(MakeBookmark(0x06000002, name: "B"));
+		var first = manager.Bookmarks[0];
+		var second = manager.Bookmarks[1];
+
+		// Editing a name (as the pane's Name editor does) must persist without tearing down the list.
+		first.Name = "A renamed";
+
+		manager.Bookmarks[0].Should().BeSameAs(first);
+		manager.Bookmarks[1].Should().BeSameAs(second, "editing one bookmark must not replace the others");
+		new BookmarkManager().Bookmarks.Single(b => b.Token == 0x06000001).Name.Should().Be("A renamed");
+	}
+
 	[Test]
 	public void Saved_bookmarks_reload_in_a_fresh_manager()
 	{

ILSpy/Bookmarks/BookmarkManager.cs

@@ -145,12 +145,6 @@ namespace ICSharpCode.ILSpy.Bookmarks
 			return true;
 		}
 
-		/// <summary>Persists the current list to the standard sidecar location.</summary>
-		public void Save() => UpdateSavedBookmarks(bookmarks => {
-			bookmarks.Clear();
-			bookmarks.AddRange(Bookmarks);
-		});
-
 		void Load()
 		{
 			var loaded = ReadFrom(ConfigurationFiles.GetPath(FileName));
@@ -168,15 +162,41 @@ namespace ICSharpCode.ILSpy.Bookmarks
 			}
 		}
 
-		// Replaces the observable collection without treating each item as a user edit.
+		// Reconciles the observable collection with the freshly saved list, in place and without
+		// treating the changes as user edits. Entries already present (matched by AnchorKey) keep their
+		// existing instance -- so the bookmarks pane's live selection and the gutter's references stay
+		// valid -- and only their editable Name/Enabled are refreshed; entries gone from the saved list
+		// are removed and new ones appended. Rebuilding via Clear()+Add() instead would replace every
+		// instance and drop the pane's selection on each toggle or edit.
 		void ReplaceLiveBookmarks(List<Bookmark> bookmarks)
 		{
 			suppressPersist = true;
 			try
 			{
-				Bookmarks.Clear();
-				foreach (var bookmark in bookmarks)
-					Bookmarks.Add(bookmark);
+				var live = new Dictionary<string, Bookmark>();
+				foreach (var bookmark in Bookmarks)
+					live[bookmark.AnchorKey] = bookmark;
+
+				var savedKeys = new HashSet<string>(bookmarks.Select(b => b.AnchorKey));
+				for (int i = Bookmarks.Count - 1; i >= 0; i--)
+				{
+					if (!savedKeys.Contains(Bookmarks[i].AnchorKey))
+						Bookmarks.RemoveAt(i);
+				}
+
+				foreach (var saved in bookmarks)
+				{
+					if (live.TryGetValue(saved.AnchorKey, out var existing))
+					{
+						// Setters are equality-checked, so assigning an unchanged value is a no-op.
+						existing.Name = saved.Name;
+						existing.Enabled = saved.Enabled;
+					}
+					else
+					{
+						Bookmarks.Add(saved);
+					}
+				}
 			}
 			finally
 			{
@@ -198,6 +218,10 @@ namespace ICSharpCode.ILSpy.Bookmarks
 			}
 		}
 
+		// The collection is only ever mutated under suppressPersist (the initial load and the
+		// post-write reconcile in ReplaceLiveBookmarks); persistence and the Changed event are owned
+		// by UpdateSavedBookmarks. This handler's sole job is to keep each bookmark's name/enabled
+		// edit subscription in step with the live list.
 		void OnCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e)
 		{
 			if (e.OldItems != null)
@@ -210,7 +234,6 @@ namespace ICSharpCode.ILSpy.Bookmarks
 				foreach (Bookmark b in e.NewItems)
 					b.PropertyChanged += OnBookmarkPropertyChanged;
 			}
-			PersistAndNotify();
 		}
 
 		void OnBookmarkPropertyChanged(object? sender, PropertyChangedEventArgs e)
@@ -220,14 +243,6 @@ namespace ICSharpCode.ILSpy.Bookmarks
 				PersistBookmarkEdit(bookmark);
 		}
 
-		void PersistAndNotify()
-		{
-			if (suppressPersist)
-				return;
-			Save();
-			Changed?.Invoke(this, EventArgs.Empty);
-		}
-
 		void PersistBookmarkEdit(Bookmark bookmark)
 		{
 			if (suppressPersist)