From 73fd7c904f73b3c5e17905ccd09c702b427980b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 03:30:18 +0000 Subject: [PATCH 1/2] Reject phrase strings and keys with spaces or commas when parsing a chord [patch] SplitChord treated a ',' after a key as an ordinary character, and Note accepted any non-blank string, so Chord.Parse("Ctrl+K, Ctrl+C") returned one chord with a made-up "K, CTRL" key that could never fire, and its ToString parsed back as a two-chord phrase. SplitChord now throws when a ',' follows a key, and Note rejects keys containing whitespace, '+' or ',' other than the single-character "+" and "," keys, so "Ctrl+," and "Ctrl++" still parse. Fixes #134 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019UNSUUrn4o84JfkEVaD3xg --- Keybinding.Test/ChordRejectsPhraseTests.cs | 89 ++++++++++++++++++++++ Keybinding/Models/KeyStringTokenizer.cs | 9 ++- Keybinding/Models/MusicalTypes.cs | 18 ++++- 3 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 Keybinding.Test/ChordRejectsPhraseTests.cs diff --git a/Keybinding.Test/ChordRejectsPhraseTests.cs b/Keybinding.Test/ChordRejectsPhraseTests.cs new file mode 100644 index 0000000..6dcdabd --- /dev/null +++ b/Keybinding.Test/ChordRejectsPhraseTests.cs @@ -0,0 +1,89 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using ktsu.Keybinding.Core.Models; +using ktsu.Keybinding.Core.Services; + +/// +/// Tests that a chord field rejects a phrase string or a key containing whitespace or a comma, rather than +/// inventing a key such as "K, CTRL" that can never be pressed. +/// +[TestClass] +public class ChordRejectsPhraseTests +{ + private static KeybindingService CreateService() => new(new CommandRegistry(), new ProfileManager()); + + [TestMethod] + [DataRow("Ctrl+K, Ctrl+C")] + [DataRow("Ctrl+K,Ctrl+C")] + [DataRow("A, B")] + [DataRow("Ctrl+,,")] + public void ChordParse_PhraseString_Throws(string value) + { + Assert.ThrowsExactly(() => Chord.Parse(value)); + Assert.ThrowsExactly(() => CreateService().ParseChord(value)); + } + + [TestMethod] + [DataRow("Page Up")] + [DataRow("Ctrl+Page Up")] + [DataRow("Ctrl+,x")] + public void ChordParse_KeyWithWhitespaceOrComma_Throws(string value) + { + Assert.ThrowsExactly(() => Chord.Parse(value)); + Assert.ThrowsExactly(() => CreateService().ParseChord(value)); + } + + [TestMethod] + [DataRow("K, CTRL")] + [DataRow("PAGE UP")] + [DataRow("A\tB")] + [DataRow("A+B")] + public void Note_KeyWithWhitespaceOrSeparator_Throws(string key) + { + Assert.ThrowsExactly(() => new Note(key)); + } + + [TestMethod] + [DataRow(",", ",")] + [DataRow("+", "+")] + [DataRow(" , ", ",")] + public void Note_SeparatorKeyOnItsOwn_IsValid(string key, string expected) + { + Assert.AreEqual(expected, new Note(key).Key.ToString()); + } + + [TestMethod] + public void ChordParse_CtrlComma_StillParses() + { + Chord chord = Chord.Parse("Ctrl+,"); + + Assert.HasCount(2, chord.Notes); + Assert.AreEqual(chord, CreateService().ParseChord("Ctrl+,")); + } + + [TestMethod] + [DataRow("Ctrl+Alt+S")] + [DataRow("Ctrl+,")] + [DataRow("Ctrl++")] + [DataRow("Shift+F5")] + [DataRow(",")] + [DataRow("+")] + public void ChordParse_ToStringOfValidChord_RoundTrips(string value) + { + Chord chord = Chord.Parse(value); + + Assert.AreEqual(chord, Chord.Parse(chord.ToString())); + } + + [TestMethod] + public void PhraseParse_PhraseString_StillSplitsIntoChords() + { + Phrase phrase = Phrase.Parse("Ctrl+K, Ctrl+C"); + + Assert.HasCount(2, phrase.Sequence); + Assert.AreEqual(Chord.Parse("Ctrl+K"), phrase.Sequence[0]); + Assert.AreEqual(Chord.Parse("Ctrl+C"), phrase.Sequence[1]); + } +} diff --git a/Keybinding/Models/KeyStringTokenizer.cs b/Keybinding/Models/KeyStringTokenizer.cs index a349fa4..7659af8 100644 --- a/Keybinding/Models/KeyStringTokenizer.cs +++ b/Keybinding/Models/KeyStringTokenizer.cs @@ -20,7 +20,7 @@ internal static class KeyStringTokenizer /// /// The chord string. /// The trimmed note strings, or an empty array for whitespace input. - /// Thrown when a key is missing, as in "Ctrl+" or "A++B". + /// Thrown when a key is missing, as in "Ctrl+" or "A++B", or when a ',' follows a key, as in the phrase "Ctrl+K, Ctrl+C". internal static string[] SplitChord(string value) => Split(value, NoteSeparator, ChordSeparators); /// @@ -42,6 +42,13 @@ private static string[] Split(string value, char splitOn, string separators) { char c = value[i]; + // In a chord, a ',' after a key starts the next chord of a phrase, which a chord cannot hold. + // A ',' where a key is expected is still the comma key, as in "Ctrl+,". + if (splitOn == NoteSeparator && c == ChordSeparator && !expectKey) + { + throw new ArgumentException($"Unexpected '{ChordSeparator}' at position {i} in \"{value}\": a chord cannot contain a sequence of chords; parse it as a phrase instead", nameof(value)); + } + if (separators.Contains(c) && !expectKey) { // A separator after a key ends the note, and ends the token when it is the one being split on diff --git a/Keybinding/Models/MusicalTypes.cs b/Keybinding/Models/MusicalTypes.cs index a1abbb2..16e514c 100644 --- a/Keybinding/Models/MusicalTypes.cs +++ b/Keybinding/Models/MusicalTypes.cs @@ -14,7 +14,7 @@ public sealed class Note : IEquatable /// Initializes a new instance of the class /// /// The key that this note represents - /// Thrown when key is null or whitespace + /// Thrown when key is null or whitespace, or contains whitespace, '+' or ',' other than as the single-character key "+" or "," [JsonConstructor] public Note(NoteName key) { @@ -27,7 +27,7 @@ public Note(NoteName key) /// Initializes a new instance of the class from a string /// /// The key string that this note represents - /// Thrown when key is null or whitespace + /// Thrown when key is null or whitespace, or contains whitespace, '+' or ',' other than as the single-character key "+" or "," public Note(string key) { if (string.IsNullOrWhiteSpace(key)) @@ -44,7 +44,19 @@ public Note(string key) /// /// A key name in any case /// The canonical key name - private static string NormalizeKey(string key) => CanonicalizeKey(key.Trim().ToUpperInvariant()); + private static string NormalizeKey(string key) => CanonicalizeKey(ValidateKey(key.Trim()).ToUpperInvariant()); + + /// + /// Rejects a key that contains whitespace or a separator, such as "Page Up" or "K, Ctrl", which no key press + /// can produce. The separator keys "+" and "," are valid on their own. + /// + /// A trimmed key name + /// The key name, unchanged + /// Thrown when the key contains whitespace, '+' or ',' + private static string ValidateKey(string key) => + key.Length > 1 && key.Any(c => char.IsWhiteSpace(c) || c is ',' or '+') + ? throw new ArgumentException($"Key \"{key}\" cannot contain whitespace, '+' or ','", nameof(key)) + : key; /// /// Maps a modifier alias to its canonical key name, so that every way of building a note From 16a661e8fa4c24c0ae40a122da740523d47291f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 03:38:28 +0000 Subject: [PATCH 2/2] Move the chord-separator check out of Split to keep its complexity under the limit Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019UNSUUrn4o84JfkEVaD3xg --- Keybinding/Models/KeyStringTokenizer.cs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/Keybinding/Models/KeyStringTokenizer.cs b/Keybinding/Models/KeyStringTokenizer.cs index 7659af8..33633a5 100644 --- a/Keybinding/Models/KeyStringTokenizer.cs +++ b/Keybinding/Models/KeyStringTokenizer.cs @@ -42,12 +42,7 @@ private static string[] Split(string value, char splitOn, string separators) { char c = value[i]; - // In a chord, a ',' after a key starts the next chord of a phrase, which a chord cannot hold. - // A ',' where a key is expected is still the comma key, as in "Ctrl+,". - if (splitOn == NoteSeparator && c == ChordSeparator && !expectKey) - { - throw new ArgumentException($"Unexpected '{ChordSeparator}' at position {i} in \"{value}\": a chord cannot contain a sequence of chords; parse it as a phrase instead", nameof(value)); - } + EnsureNotChordSeparatorInChord(value, i, splitOn, expectKey); if (separators.Contains(c) && !expectKey) { @@ -93,6 +88,18 @@ private static string[] Split(string value, char splitOn, string separators) return [.. tokens]; } + /// + /// In a chord, a ',' after a key starts the next chord of a phrase, which a chord cannot hold. + /// A ',' where a key is expected is still the comma key, as in "Ctrl+,". + /// + private static void EnsureNotChordSeparatorInChord(string value, int index, char splitOn, bool expectKey) + { + if (splitOn == NoteSeparator && !expectKey && value[index] == ChordSeparator) + { + throw new ArgumentException($"Unexpected '{ChordSeparator}' at position {index} in \"{value}\": a chord cannot contain a sequence of chords; parse it as a phrase instead", nameof(value)); + } + } + /// /// A separator where a key is expected is the key itself, but only when nothing else follows it before the next /// separator. Otherwise the input has an empty key, which must not be dropped.