diff --git a/Keybinding.Test/ProfileChordAccessTests.cs b/Keybinding.Test/ProfileChordAccessTests.cs new file mode 100644 index 0000000..3f54cb4 --- /dev/null +++ b/Keybinding.Test/ProfileChordAccessTests.cs @@ -0,0 +1,87 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using ktsu.Keybinding.Core.Models; +using ktsu.Keybinding.Core.Services; + +[TestClass] +public class ProfileChordAccessTests +{ + private static readonly Chord CtrlA = Chord.Parse("Ctrl+A"); + private static readonly Chord CtrlB = Chord.Parse("Ctrl+B"); + private static readonly string[] BothCommands = ["a", "b"]; + + [TestMethod] + public void ChordAccessors_ReflectSetRemoveAndClear() + { + Profile profile = new("p", "Profile"); + + profile.SetChord(" a ", CtrlA); + profile.SetChord("b", CtrlB); + + Assert.AreEqual(2, profile.ChordCount); + Assert.AreEqual(CtrlA, profile.GetChord("a")); + Assert.IsNull(profile.GetChord("missing")); + Assert.IsTrue(profile.HasChord(" a ")); + Assert.IsFalse(profile.HasChord("missing")); + CollectionAssert.AreEquivalent(BothCommands, profile.BoundCommands.ToArray()); + + Assert.IsTrue(profile.RemoveChord("a")); + Assert.IsFalse(profile.RemoveChord("a")); + Assert.AreEqual(1, profile.ChordCount); + + profile.ClearChords(); + Assert.AreEqual(0, profile.ChordCount); + Assert.IsEmpty(profile.GetAllChords()); + } + + [TestMethod] + public void ChordAccessors_RejectBlankCommandIds() + { + Profile profile = new("p", "Profile"); + + Assert.ThrowsExactly(() => profile.SetChord(" ", CtrlA)); + Assert.ThrowsExactly(() => profile.GetChord(" ")); + Assert.ThrowsExactly(() => profile.HasChord(" ")); + Assert.ThrowsExactly(() => profile.RemoveChord(" ")); + } + + [TestMethod] + public void ObsoleteChords_StillExposesTheLiveBindings() + { + Profile profile = new("p", "Profile"); + profile.SetChord("a", CtrlA); + +#pragma warning disable CS0618 // Type or member is obsolete + Dictionary chords = profile.Chords; +#pragma warning restore CS0618 // Type or member is obsolete + + Assert.HasCount(1, chords); + profile.SetChord("b", CtrlB); + Assert.HasCount(2, chords, "Chords keeps returning the live dictionary for existing callers"); + } + + [TestMethod] + public void FindAndExecuteChord_UseTheLockedSnapshot() + { + CommandRegistry registry = new(); + ProfileManager profiles = new(); + profiles.CreateProfile("p", "Profile"); + profiles.SetActiveProfile("p"); + KeybindingService service = new(registry, profiles); + registry.RegisterCommand(new Command("a", "A")); + registry.RegisterCommand(new Command("b", "B")); + + Assert.IsTrue(service.BindChord("a", CtrlA)); + Assert.IsTrue(service.BindChord("b", CtrlB)); + + Assert.AreEqual("a", service.FindCommandByChord(CtrlA)); + Assert.IsNull(service.FindCommandByChord(Chord.Parse("Ctrl+C"))); + Assert.AreEqual("b", service.ExecuteChord(CtrlB)); + + registry.UnregisterCommand("b"); + Assert.IsNull(service.ExecuteChord(CtrlB), "A binding whose command is unregistered is skipped"); + Assert.AreEqual("b", service.FindCommandByChord(CtrlB), "FindCommandByChord does not filter by registration"); + } +} diff --git a/Keybinding.Test/ProfileChordConcurrencyTests.cs b/Keybinding.Test/ProfileChordConcurrencyTests.cs new file mode 100644 index 0000000..3408b35 --- /dev/null +++ b/Keybinding.Test/ProfileChordConcurrencyTests.cs @@ -0,0 +1,49 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using System.Globalization; +using ktsu.Keybinding.Core.Models; +using ktsu.Keybinding.Core.Services; + +[TestClass] +public class ProfileChordConcurrencyTests +{ + private const int CommandCount = 20_000; + + [TestMethod] + public void BindUnbindAndFind_CalledInParallel_KeepEveryBindingAndDoNotThrow() + { + CommandRegistry registry = new(); + ProfileManager profiles = new(); + profiles.CreateProfile("p", "Profile"); + profiles.SetActiveProfile("p"); + KeybindingService service = new(registry, profiles); + + for (int i = 0; i < CommandCount; i++) + { + registry.RegisterCommand(new Command($"c{i}", $"C{i}")); + } + + Chord chord = Chord.Parse("Ctrl+K"); + + // Odd commands stay bound, even ones are unbound again, and reads race the writes. + Parallel.For(0, CommandCount, i => + { + string commandId = $"c{i}"; + Assert.IsTrue(service.BindChord(commandId, chord)); + _ = service.FindCommandByChord(chord); + _ = service.GetAllChords(); + + if (i % 2 == 0) + { + Assert.IsTrue(service.UnbindChord(commandId)); + } + }); + + Profile profile = profiles.GetProfile("p")!; + Assert.AreEqual(CommandCount / 2, profile.ChordCount); + Assert.HasCount(CommandCount / 2, service.GetAllChords()); + Assert.IsTrue(service.GetAllChords().Keys.All(id => int.Parse(id[1..], CultureInfo.InvariantCulture) % 2 == 1)); + } +} diff --git a/Keybinding/KeybindingManager.cs b/Keybinding/KeybindingManager.cs index 66c4ae3..b73c08d 100644 --- a/Keybinding/KeybindingManager.cs +++ b/Keybinding/KeybindingManager.cs @@ -215,7 +215,7 @@ public KeybindingSummary GetSummary() Profile? activeProfile = Profiles.GetActiveProfile(); int totalCommands = Commands.GetAllCommands().Count; int totalProfiles = Profiles.GetAllProfiles().Count; - int activeKeybindings = activeProfile?.Chords.Count ?? 0; + int activeKeybindings = activeProfile?.ChordCount ?? 0; return new KeybindingSummary { diff --git a/Keybinding/Models/Profile.cs b/Keybinding/Models/Profile.cs index 5891d3f..2e33422 100644 --- a/Keybinding/Models/Profile.cs +++ b/Keybinding/Models/Profile.cs @@ -31,9 +31,14 @@ public Profile(string id, string name, string? description = null) Id = id.Trim(); Name = name.Trim(); Description = description?.Trim(); - Chords = []; } + [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0032:Use auto property", Justification = "The only property over this field is obsolete, and the field is what the lock guards.")] + private readonly Dictionary _chords = []; + + // Every read and write of _chords takes this lock, so bindings can change from any thread. + private readonly Lock _chordsLock = new(); + /// /// Gets the unique profile identifier /// @@ -68,9 +73,29 @@ internal void Rename(string name, string? description) } /// - /// Gets the chord bindings for this profile (command ID to chord mapping) + /// Gets the live chord bindings for this profile (command ID to chord mapping) /// - public Dictionary Chords { get; } + /// + /// Reading or changing this dictionary bypasses the profile's synchronization, so it is not + /// thread-safe. Use , , + /// and instead. + /// + [Obsolete("Chords is not thread-safe. Use GetAllChords, SetChord, RemoveChord or ClearChords instead.")] + public Dictionary Chords => _chords; + + /// + /// Gets the number of chord bindings in this profile + /// + public int ChordCount + { + get + { + lock (_chordsLock) + { + return _chords.Count; + } + } + } /// /// Sets a chord binding for a command in this profile @@ -88,7 +113,10 @@ public void SetChord(string commandId, Chord chord) Ensure.NotNull(chord); - Chords[commandId.Trim()] = chord; + lock (_chordsLock) + { + _chords[commandId.Trim()] = chord; + } } /// @@ -99,18 +127,47 @@ public void SetChord(string commandId, Chord chord) /// Thrown when commandId is null or whitespace public Chord? GetChord(string commandId) { - 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; + if (string.IsNullOrWhiteSpace(commandId)) + { + throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)); + } + + lock (_chordsLock) + { + return _chords.TryGetValue(commandId.Trim(), out Chord? chord) ? chord : null; + } } /// /// Gets all chord bindings for this profile /// /// Dictionary of command ID to chord mappings - public IReadOnlyDictionary GetAllChords() => new Dictionary(Chords).AsReadOnly(); + public IReadOnlyDictionary GetAllChords() + { + lock (_chordsLock) + { + return new Dictionary(_chords).AsReadOnly(); + } + } + + /// + /// Finds the first command bound to a chord in this profile that satisfies a condition + /// + /// The chord to look up + /// An optional condition the command ID must satisfy + /// The command ID if found, null otherwise + internal string? FindCommand(Chord chord, Func? predicate = null) + { + foreach (KeyValuePair binding in GetAllChords()) + { + if (binding.Value.Equals(chord) && (predicate is null || predicate(binding.Key))) + { + return binding.Key; + } + } + + return null; + } /// /// Checks if a command has a chord binding in this profile @@ -120,9 +177,15 @@ public void SetChord(string commandId, Chord chord) /// Thrown when commandId is null or whitespace public bool HasChord(string commandId) { - return string.IsNullOrWhiteSpace(commandId) - ? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)) - : Chords.ContainsKey(commandId.Trim()); + if (string.IsNullOrWhiteSpace(commandId)) + { + throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)); + } + + lock (_chordsLock) + { + return _chords.ContainsKey(commandId.Trim()); + } } /// @@ -133,21 +196,42 @@ public bool HasChord(string commandId) /// Thrown when commandId is null or whitespace public bool RemoveChord(string commandId) { - return string.IsNullOrWhiteSpace(commandId) - ? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)) - : Chords.Remove(commandId.Trim()); + if (string.IsNullOrWhiteSpace(commandId)) + { + throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId)); + } + + lock (_chordsLock) + { + return _chords.Remove(commandId.Trim()); + } } /// /// Gets all command IDs that have chord bindings in this profile /// /// Collection of command IDs - public IReadOnlyCollection BoundCommands => [.. Chords.Keys]; + public IReadOnlyCollection BoundCommands + { + get + { + lock (_chordsLock) + { + return [.. _chords.Keys]; + } + } + } /// /// Clears all chord bindings from this profile /// - public void ClearChords() => Chords.Clear(); + public void ClearChords() + { + lock (_chordsLock) + { + _chords.Clear(); + } + } /// /// Returns a string representation of the profile diff --git a/Keybinding/Services/JsonKeybindingRepository.cs b/Keybinding/Services/JsonKeybindingRepository.cs index a98c957..304f36b 100644 --- a/Keybinding/Services/JsonKeybindingRepository.cs +++ b/Keybinding/Services/JsonKeybindingRepository.cs @@ -59,7 +59,7 @@ public async Task SaveProfileAsync(Profile profile) Id = p.Id, Name = p.Name, Description = p.Description, - Chords = p.Chords.ToDictionary( + Chords = p.GetAllChords().ToDictionary( kvp => kvp.Key, kvp => new ChordDto { @@ -137,7 +137,7 @@ public async Task DeleteProfileAsync(string profileId) Id = p.Id, Name = p.Name, Description = p.Description, - Chords = p.Chords.ToDictionary( + Chords = p.GetAllChords().ToDictionary( kvp => kvp.Key, kvp => new ChordDto { diff --git a/Keybinding/Services/KeybindingService.cs b/Keybinding/Services/KeybindingService.cs index 03e2035..647bc27 100644 --- a/Keybinding/Services/KeybindingService.cs +++ b/Keybinding/Services/KeybindingService.cs @@ -247,9 +247,7 @@ public Chord ParseChord(string chordString) // bindings in place, so skip bindings whose command is no longer registered rather than // giving up on the first match. Profile? profile = _profileManager.GetProfile(profileId); - string? commandId = profile?.Chords - .FirstOrDefault(kvp => kvp.Value.Equals(chord) && _commandRegistry.IsCommandRegistered(kvp.Key)) - .Key; + string? commandId = profile?.FindCommand(chord, _commandRegistry.IsCommandRegistered); // In a real implementation, this would trigger command execution // For now, we just return the command ID that would be executed @@ -298,8 +296,6 @@ public bool HasChordBinding(string profileId, string commandId) } Profile? profile = _profileManager.GetProfile(profileId); - return profile?.Chords - .FirstOrDefault(kvp => kvp.Value.Equals(chord)) - .Key; + return profile?.FindCommand(chord); } } diff --git a/Keybinding/Services/ProfileManager.cs b/Keybinding/Services/ProfileManager.cs index 7d46fb1..923182d 100644 --- a/Keybinding/Services/ProfileManager.cs +++ b/Keybinding/Services/ProfileManager.cs @@ -155,7 +155,7 @@ public bool ProfileExists(string profileId) Profile newProfile = new(normalizedNewId, newProfileName.Trim(), newDescription); // Copy all chords from source profile - foreach (KeyValuePair kvp in sourceProfile.Chords) + foreach (KeyValuePair kvp in sourceProfile.GetAllChords()) { newProfile.SetChord(kvp.Key, kvp.Value); }