From 0b1f3d7934c3e823f8b1d5b442f809f7f5cd9a6c Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Mon, 29 Jun 2026 23:20:01 +0200 Subject: [PATCH] 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 | 32 +++++++++++ ILSpy/Bookmarks/BookmarkManager.cs | 53 ++++++++++++------- 2 files changed, 66 insertions(+), 19 deletions(-) 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)