From 1db415f11254c77c439f5a230b004105aa8f38c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 11:25:11 +0000 Subject: [PATCH] Refuse a null chord in BindChord, ExecuteChord and FindCommandByChord whatever the profile [patch] The chord check ran after the blank-profile, blank-command and no-active-profile early returns, so a null chord returned false/null in a test with no profile and threw ArgumentNullException in an app with one. Every chord-taking overload now checks the chord first, and IKeybindingService documents the exception. Fixes ktsu-dev/Keybinding#230 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW499sBmEvZWVdPTbTv --- Keybinding.Test/NullChordArgumentTests.cs | 70 ++++++++++++++++++++++ Keybinding/Contracts/IKeybindingService.cs | 6 ++ Keybinding/Services/KeybindingService.cs | 10 +++- 3 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 Keybinding.Test/NullChordArgumentTests.cs diff --git a/Keybinding.Test/NullChordArgumentTests.cs b/Keybinding.Test/NullChordArgumentTests.cs new file mode 100644 index 0000000..598c79a --- /dev/null +++ b/Keybinding.Test/NullChordArgumentTests.cs @@ -0,0 +1,70 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using ktsu.Keybinding.Core.Models; +using ktsu.Keybinding.Core.Services; + +[TestClass] +public class NullChordArgumentTests +{ + private ProfileManager _profiles = null!; + private CommandRegistry _commands = null!; + private KeybindingService _service = null!; + + [TestInitialize] + public void Setup() + { + _profiles = new ProfileManager(); + _commands = new CommandRegistry(); + _commands.RegisterCommand(new Command("save", "Save")); + _service = new KeybindingService(_commands, _profiles); + } + + [TestMethod] + public void ActiveProfileOverloads_NullChord_ThrowWithNoActiveProfile() + { + Assert.IsNull(_service.ActiveProfile); + + Assert.ThrowsExactly(() => _service.BindChord("save", null!)); + Assert.ThrowsExactly(() => _service.ExecuteChord(null!)); + Assert.ThrowsExactly(() => _service.FindCommandByChord(null!)); + } + + [TestMethod] + public void ActiveProfileOverloads_NullChord_ThrowWithAnActiveProfile() + { + _profiles.CreateProfile("default", "Default"); + _profiles.SetActiveProfile("default"); + Assert.IsNotNull(_service.ActiveProfile); + + Assert.ThrowsExactly(() => _service.BindChord("save", null!)); + Assert.ThrowsExactly(() => _service.ExecuteChord(null!)); + Assert.ThrowsExactly(() => _service.FindCommandByChord(null!)); + } + + [TestMethod] + [DataRow("")] + [DataRow(" ")] + [DataRow(null)] + [DataRow("missing")] + public void ProfileIdOverloads_NullChord_ThrowWhateverTheProfileId(string? profileId) + { + Assert.ThrowsExactly(() => _service.BindChord(profileId!, "save", null!)); + Assert.ThrowsExactly(() => _service.ExecuteChord(profileId!, null!)); + Assert.ThrowsExactly(() => _service.FindCommandByChord(profileId!, null!)); + } + + [TestMethod] + [DataRow("")] + [DataRow(" ")] + [DataRow(null)] + public void BindChord_NullChord_ThrowsWhateverTheCommandId(string? commandId) + { + _profiles.CreateProfile("default", "Default"); + _profiles.SetActiveProfile("default"); + + Assert.ThrowsExactly(() => _service.BindChord(commandId!, null!)); + Assert.ThrowsExactly(() => _service.BindChord(_service.ActiveProfile!.Id, commandId!, null!)); + } +} diff --git a/Keybinding/Contracts/IKeybindingService.cs b/Keybinding/Contracts/IKeybindingService.cs index 7669d28..b53065d 100644 --- a/Keybinding/Contracts/IKeybindingService.cs +++ b/Keybinding/Contracts/IKeybindingService.cs @@ -60,6 +60,7 @@ public interface IKeybindingService /// A chord runs one command per profile. Binding a chord that another command in the profile already uses /// moves it: the other command loses that binding, as rebinding a shortcut does in most editors. /// + /// Thrown when chord is null, whether or not a profile is active and whatever the profile ID public bool BindChord(string commandId, Chord chord); /// @@ -73,6 +74,7 @@ public interface IKeybindingService /// A chord runs one command per profile. Binding a chord that another command in the profile already uses /// moves it: the other command loses that binding, as rebinding a shortcut does in most editors. /// + /// Thrown when chord is null, whether or not a profile is active and whatever the profile ID public bool BindChord(string profileId, string commandId, Chord chord); /// @@ -151,6 +153,7 @@ public interface IKeybindingService /// /// The chord that was pressed /// The command ID that was executed, null if no binding found + /// Thrown when chord is null, whether or not a profile is active and whatever the profile ID public string? ExecuteChord(Chord chord); /// @@ -159,6 +162,7 @@ public interface IKeybindingService /// The profile ID /// The chord that was pressed /// The command ID that was executed, null if no binding found + /// Thrown when chord is null, whether or not a profile is active and whatever the profile ID public string? ExecuteChord(string profileId, Chord chord); /// @@ -182,6 +186,7 @@ public interface IKeybindingService /// /// The chord to search for /// The command ID if found, null otherwise + /// Thrown when chord is null, whether or not a profile is active and whatever the profile ID public string? FindCommandByChord(Chord chord); /// @@ -191,5 +196,6 @@ public interface IKeybindingService /// The profile ID /// The chord to search for /// The command ID if found, null otherwise + /// Thrown when chord is null, whether or not a profile is active and whatever the profile ID public string? FindCommandByChord(string profileId, Chord chord); } diff --git a/Keybinding/Services/KeybindingService.cs b/Keybinding/Services/KeybindingService.cs index 1b42735..c4b9c03 100644 --- a/Keybinding/Services/KeybindingService.cs +++ b/Keybinding/Services/KeybindingService.cs @@ -52,6 +52,8 @@ public bool SetActiveProfile(string profileId) => /// public bool BindChord(string commandId, Chord chord) { + Ensure.NotNull(chord); + Profile? activeProfile = _profileManager.GetActiveProfile(); return activeProfile is not null && BindChord(activeProfile.Id, commandId, chord); } @@ -59,6 +61,8 @@ public bool BindChord(string commandId, Chord chord) /// public bool BindChord(string profileId, string commandId, Chord chord) { + Ensure.NotNull(chord); + if (string.IsNullOrWhiteSpace(profileId)) { return false; @@ -69,8 +73,6 @@ public bool BindChord(string profileId, string commandId, Chord chord) return false; } - Ensure.NotNull(chord); - Profile? profile = _profileManager.GetProfile(profileId); if (profile is null) { @@ -201,6 +203,8 @@ public Chord ParseChord(string chordString) => /// public string? ExecuteChord(Chord chord) { + Ensure.NotNull(chord); + Profile? activeProfile = _profileManager.GetActiveProfile(); return activeProfile is not null ? ExecuteChord(activeProfile.Id, chord) : null; } @@ -249,6 +253,8 @@ public bool HasChordBinding(string profileId, string commandId) /// public string? FindCommandByChord(Chord chord) { + Ensure.NotNull(chord); + Profile? activeProfile = _profileManager.GetActiveProfile(); return activeProfile is not null ? FindCommandByChord(activeProfile.Id, chord) : null; }