Rename profiles in place and keep their description

Keybinding.Test/ProfileRenameTests.cs

@@ -0,0 +1,108 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.Keybinding.Test;
+
+using ktsu.Keybinding.Core;
+using ktsu.Keybinding.Core.Models;
+using ktsu.Keybinding.Core.Services;
+
+[TestClass]
+public class ProfileRenameTests
+{
+	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);
+		}
+	}
+
+	[TestMethod]
+	public void RenameProfile_WithoutDescription_KeepsTheExistingDescription()
+	{
+		ProfileManager profiles = new();
+		profiles.CreateProfile("p", "Old", "my desc");
+
+		Assert.IsTrue(profiles.RenameProfile("p", "New Name"));
+
+		Profile? renamed = profiles.GetProfile("p");
+		Assert.IsNotNull(renamed);
+		Assert.AreEqual("New Name", renamed.Name);
+		Assert.AreEqual("my desc", renamed.Description, "Leaving out the optional description should not erase it.");
+	}
+
+	[TestMethod]
+	public void RenameProfile_WithDescription_ReplacesIt()
+	{
+		ProfileManager profiles = new();
+		profiles.CreateProfile("p", "Old", "my desc");
+
+		Assert.IsTrue(profiles.RenameProfile("p", " New Name ", " new desc "));
+
+		Profile? renamed = profiles.GetProfile("p");
+		Assert.IsNotNull(renamed);
+		Assert.AreEqual("New Name", renamed.Name);
+		Assert.AreEqual("new desc", renamed.Description);
+	}
+
+	[TestMethod]
+	public void RenameProfile_UnknownProfile_ReturnsFalse()
+	{
+		ProfileManager profiles = new();
+
+		Assert.IsFalse(profiles.RenameProfile("missing", "New Name"));
+	}
+
+	[TestMethod]
+	public void RenameProfile_KeepsHeldReferencesAttached()
+	{
+		ProfileManager profiles = new();
+		Profile held = profiles.CreateProfile("p", "Old", "my desc");
+
+		profiles.RenameProfile("p", "New Name");
+
+		Assert.AreSame(held, profiles.GetProfile("p"), "The renamed profile should be the same object callers already hold.");
+
+		Chord chord = Chord.Parse("Ctrl+A");
+		held.SetChord("x.y", chord);
+		Assert.AreEqual(chord, profiles.GetProfile("p")!.GetChord("x.y"), "A chord set through a held reference should reach the stored profile.");
+	}
+
+	[TestMethod]
+	public async Task RenameProfile_ChordsSetThroughAHeldReference_ArePersisted()
+	{
+		{
+			using KeybindingManager manager = new(_testDataDirectory);
+			await manager.InitializeAsync().ConfigureAwait(false);
+
+			manager.Commands.RegisterCommand(new Command("x.y", "X Y"));
+			Profile? held = manager.CreateDefaultProfile();
+			Assert.IsNotNull(held);
+
+			Assert.IsTrue(manager.Profiles.RenameProfile(held.Id, "Renamed"));
+			held.SetChord("x.y", manager.Keybindings.ParseChord("Ctrl+T"));
+
+			await manager.SaveAsync().ConfigureAwait(false);
+		}
+
+		{
+			using KeybindingManager manager = new(_testDataDirectory);
+			await manager.InitializeAsync().ConfigureAwait(false);
+
+			Profile? reloaded = manager.Profiles.GetProfile("default");
+			Assert.IsNotNull(reloaded);
+			Assert.AreEqual("Renamed", reloaded.Name);
+			Assert.AreEqual("Ctrl+T", reloaded.GetChord("x.y")?.ToString());
+		}
+	}
+}

Keybinding/Contracts/IProfileManager.cs

@@ -85,7 +85,8 @@ public interface IProfileManager
 	/// </summary>
 	/// <param name="profileId">The ID of the profile to rename</param>
 	/// <param name="newName">The new name for the profile</param>
-	/// <param name="newDescription">Optional new description for the profile</param>
+	/// <param name="newDescription">Optional new description for the profile; when null, the current description is kept</param>
 	/// <returns>True if the profile was renamed, false if it doesn't exist</returns>
+	/// <remarks>The profile is renamed in place, so references already held to it remain valid.</remarks>
 	public bool RenameProfile(string profileId, string newName, string? newDescription = null);
 }

Keybinding/Models/Profile.cs

@@ -42,12 +42,30 @@
 	/// <summary>
 	/// Gets the profile name
 	/// </summary>
-	public string Name { get; }
+	public string Name { get; private set; }
 
 	/// <summary>
 	/// Gets the profile description
 	/// </summary>
-	public string? Description { get; }
+	public string? Description { get; private set; }
+
+	/// <summary>
+	/// Renames this profile in place, so every reference already held to it stays attached to the
+	/// manager that stores it.
+	/// </summary>
+	/// <param name="name">The new profile name</param>
+	/// <param name="description">The new profile description</param>
+	/// <exception cref="ArgumentException">Thrown when name is null or whitespace</exception>
+	internal void Rename(string name, string? description)
+	{
+		if (string.IsNullOrWhiteSpace(name))
+		{
+			throw new ArgumentException("Profile name cannot be null or whitespace", nameof(name));
+		}
+
+		Name = name.Trim();
+		Description = description?.Trim();
+	}
 
 	/// <summary>
 	/// Gets the chord bindings for this profile (command ID to chord mapping)
@@ -65,7 +83,7 @@
 	{
 		if (string.IsNullOrWhiteSpace(commandId))
 		{
 			throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId));
 		}
 
 		Ensure.NotNull(chord);
@@ -83,7 +101,7 @@
 	{
 		return string.IsNullOrWhiteSpace(commandId)
 			? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId))
 			: Chords.TryGetValue(commandId.Trim(), out Chord? chord)
 			? chord
 			: null;
 	}

Keybinding/Services/ProfileManager.cs

@@ -32,7 +32,7 @@
 	{
 		if (string.IsNullOrWhiteSpace(id))
 		{
 			throw new ArgumentException("Profile ID cannot be null or whitespace", nameof(id));
 		}
 
 		if (string.IsNullOrWhiteSpace(name))
@@ -79,7 +79,7 @@
 	{
 		return string.IsNullOrWhiteSpace(profileId)
 			? throw new ArgumentException("Profile ID cannot be null or whitespace", nameof(profileId))
 			: _profiles.TryGetValue(profileId.Trim(), out Profile? profile) ? profile : null;
 	}
 
 	/// <inheritdoc/>
@@ -182,23 +182,21 @@
 			throw new ArgumentException("New name cannot be null or whitespace", nameof(newName));
 		}
 
-		Profile? profile = GetProfile(profileId);
-		if (profile is null)
+		lock (_lock)
 		{
-			return false;
-		}
+			Profile? profile = GetProfile(profileId);
+			if (profile is null)
+			{
+				return false;
+			}
 
-		// Create a new profile with the same ID but new name/description
-		Profile updatedProfile = new(profile.Id, newName.Trim(), newDescription);
-
-		// Copy all chords
-		foreach (KeyValuePair<string, Chord> kvp in profile.Chords)
-		{
-			updatedProfile.SetChord(kvp.Key, kvp.Value);
+			// Rename the stored instance rather than swapping in a copy. A copy would detach every
+			// reference a caller already holds (from CreateProfile, GetActiveProfile and the like), so
+			// chords set through it would never reach GetProfile or SaveAsync. It would also leave a
+			// window between removing and re-adding in which the profile did not exist at all.
+			// A description that is not passed is kept, as the optional parameter implies.
+			profile.Rename(newName, newDescription ?? profile.Description);
+			return true;
 		}
-
-		// Replace the profile (this will preserve the same ID)
-		_profiles.TryRemove(profile.Id, out _);
-		return _profiles.TryAdd(profile.Id, updatedProfile);
 	}
 }