diff --git a/ILSpy.Tests/Bookmarks/BookmarkManagerTests.cs b/ILSpy.Tests/Bookmarks/BookmarkManagerTests.cs index 8ef6c40a8..6b142338d 100644 --- a/ILSpy.Tests/Bookmarks/BookmarkManagerTests.cs +++ b/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() { diff --git a/ILSpy/Bookmarks/BookmarkManager.cs b/ILSpy/Bookmarks/BookmarkManager.cs index ad7b74370..296aab628 100644 --- a/ILSpy/Bookmarks/BookmarkManager.cs +++ b/ILSpy/Bookmarks/BookmarkManager.cs @@ -145,12 +145,6 @@ namespace ICSharpCode.ILSpy.Bookmarks return true; } - /// Persists the current list to the standard sidecar location. - 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 bookmarks) { suppressPersist = true; try { - Bookmarks.Clear(); - foreach (var bookmark in bookmarks) - Bookmarks.Add(bookmark); + var live = new Dictionary(); + foreach (var bookmark in Bookmarks) + live[bookmark.AnchorKey] = bookmark; + + var savedKeys = new HashSet(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)