From aba6bbf8341bc48ae4683ba336602a7931599f36 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Sun, 27 Sep 2026 10:28:07 +0000 Subject: [PATCH] Only prune stored profiles this manager loaded or saved [patch] SaveAsync treated every stored profile missing from memory as deleted, so a manager that never called InitializeAsync deleted every profile on disk when it saved. The manager now records the ids of the profiles it loaded or saved, and SaveAsync deletes only those that are no longer in memory. Profiles it never saw are left alone, and deleting a loaded or previously saved profile still sticks as #106 requires. Fixes #124 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01X65VbCUfvG15D4o8afrvpk --- Keybinding.Test/SaveWithoutInitializeTests.cs | 90 +++++++++++++++++++ Keybinding/KeybindingManager.cs | 29 +++++- 2 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 Keybinding.Test/SaveWithoutInitializeTests.cs diff --git a/Keybinding.Test/SaveWithoutInitializeTests.cs b/Keybinding.Test/SaveWithoutInitializeTests.cs new file mode 100644 index 0000000..939144e --- /dev/null +++ b/Keybinding.Test/SaveWithoutInitializeTests.cs @@ -0,0 +1,90 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using ktsu.Keybinding.Core; +using ktsu.Keybinding.Core.Models; + +[TestClass] +public class SaveWithoutInitializeTests +{ + private string _testDataDirectory = null!; + + [TestInitialize] + public void Setup() + { + _testDataDirectory = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); + Directory.CreateDirectory(_testDataDirectory); + } + + [TestCleanup] + public void Cleanup() + { + if (Directory.Exists(_testDataDirectory)) + { + Directory.Delete(_testDataDirectory, recursive: true); + } + } + + private async Task LoadProfileIdsAsync() + { + using KeybindingManager manager = new(_testDataDirectory); + await manager.InitializeAsync().ConfigureAwait(false); + return string.Join(',', manager.Profiles.GetAllProfiles().Select(p => p.Id).Order(StringComparer.Ordinal)); + } + + [TestMethod] + public async Task SaveAsync_WithoutInitialize_KeepsStoredProfiles() + { + { + using KeybindingManager manager = new(_testDataDirectory); + await manager.InitializeAsync().ConfigureAwait(false); + manager.CreateDefaultProfile(); + manager.Profiles.CreateProfile("vim", "Vim"); + await manager.SaveAsync().ConfigureAwait(false); + } + + { + using KeybindingManager manager = new(_testDataDirectory); + manager.CreateDefaultProfile("other", "Other"); + await manager.SaveAsync().ConfigureAwait(false); + } + + Assert.AreEqual("default,other,vim", await LoadProfileIdsAsync().ConfigureAwait(false)); + } + + [TestMethod] + public async Task DeleteProfile_CreatedAndSavedInSameManager_StaysDeleted() + { + { + using KeybindingManager manager = new(_testDataDirectory); + manager.Profiles.CreateProfile("default", "Default"); + manager.Profiles.CreateProfile("vim", "Vim"); + await manager.SaveAsync().ConfigureAwait(false); + + Assert.IsTrue(manager.Profiles.DeleteProfile("vim")); + await manager.SaveAsync().ConfigureAwait(false); + } + + Assert.AreEqual("default", await LoadProfileIdsAsync().ConfigureAwait(false)); + } + + [TestMethod] + public async Task DeleteProfile_ThenRecreate_IsSavedAgain() + { + { + using KeybindingManager manager = new(_testDataDirectory); + await manager.InitializeAsync().ConfigureAwait(false); + manager.Profiles.CreateProfile("vim", "Vim"); + await manager.SaveAsync().ConfigureAwait(false); + + Assert.IsTrue(manager.Profiles.DeleteProfile("vim")); + await manager.SaveAsync().ConfigureAwait(false); + + manager.Profiles.CreateProfile(new Profile("vim", "Vim again")); + await manager.SaveAsync().ConfigureAwait(false); + } + + Assert.AreEqual("vim", await LoadProfileIdsAsync().ConfigureAwait(false)); + } +} diff --git a/Keybinding/KeybindingManager.cs b/Keybinding/KeybindingManager.cs index 1717799..66c4ae3 100644 --- a/Keybinding/KeybindingManager.cs +++ b/Keybinding/KeybindingManager.cs @@ -13,6 +13,12 @@ namespace ktsu.Keybinding.Core; public sealed class KeybindingManager : IDisposable { private bool _disposed; + + // Ids of the stored profiles this manager has loaded or saved. SaveAsync deletes a stored profile only if + // it is in here and no longer in memory, so profiles this manager never saw are not taken as deleted. + private readonly HashSet _persistedProfileIds = []; + private readonly Lock _persistedProfileIdsLock = new(); + /// /// Initializes a new instance of the class with default services /// @@ -87,6 +93,11 @@ public async Task InitializeAsync() Profiles.CreateProfile(profile); } + lock (_persistedProfileIdsLock) + { + _persistedProfileIds.UnionWith(profiles.Select(p => p.Id)); + } + // Load active profile string? activeProfileId = await Repository.LoadActiveProfileAsync().ConfigureAwait(false); if (!string.IsNullOrEmpty(activeProfileId) && Profiles.ProfileExists(activeProfileId)) @@ -107,16 +118,28 @@ public async Task SaveAsync() IReadOnlyCollection commands = Commands.GetAllCommands(); await Repository.SaveCommandsAsync(commands).ConfigureAwait(false); - // Remove stored profiles that were deleted in memory, so they do not come back on the next load + // Remove stored profiles that were deleted in memory, so they do not come back on the next load. Only + // profiles this manager loaded or saved count: one it never saw is not in memory because it was never + // loaded, not because it was deleted. IReadOnlyCollection profiles = Profiles.GetAllProfiles(); HashSet profileIds = [.. profiles.Select(p => p.Id)]; - IReadOnlyCollection storedProfiles = await Repository.LoadAllProfilesAsync().ConfigureAwait(false); - IEnumerable deletedProfileIds = storedProfiles.Select(p => p.Id).Where(id => !profileIds.Contains(id)); + List deletedProfileIds; + lock (_persistedProfileIdsLock) + { + deletedProfileIds = [.. _persistedProfileIds.Where(id => !profileIds.Contains(id))]; + } + await AsyncBatchHelper.ForEachAsync(deletedProfileIds, Repository.DeleteProfileAsync).ConfigureAwait(false); // Save profiles using batch helper await AsyncBatchHelper.ForEachAsync(profiles, Repository.SaveProfileAsync).ConfigureAwait(false); + lock (_persistedProfileIdsLock) + { + _persistedProfileIds.ExceptWith(deletedProfileIds); + _persistedProfileIds.UnionWith(profileIds); + } + // Save active profile Profile? activeProfile = Profiles.GetActiveProfile(); await Repository.SaveActiveProfileAsync(activeProfile?.Id).ConfigureAwait(false);