From bde54074c2ff3b42f6616a019eb0183b21b38e4e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 01:26:52 +0000 Subject: [PATCH 1/2] Parse "Ctrl++" and "Ctrl+," as the plus and comma keys Chord.Parse, Phrase.Parse, and KeybindingService.ParseChord/ParsePhrase split on '+' and ',' with RemoveEmptyEntries, so "Ctrl++" and "Ctrl+," silently became a lone Ctrl. Route all four through one tokenizer that treats a separator where a key is expected as the literal key, and throws ArgumentException when a key is missing ("Ctrl+", "A,,B") instead of dropping it. Fixes ktsu-dev/Keybinding#108 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8KgDr8CfZLWPQ5o6nZdcV --- Keybinding.Test/SeparatorKeyParsingTests.cs | 129 ++++++++++++++++++++ Keybinding/Models/KeyStringTokenizer.cs | 98 +++++++++++++++ Keybinding/Models/MusicalTypes.cs | 5 +- Keybinding/Services/KeybindingService.cs | 4 +- 4 files changed, 231 insertions(+), 5 deletions(-) create mode 100644 Keybinding.Test/SeparatorKeyParsingTests.cs create mode 100644 Keybinding/Models/KeyStringTokenizer.cs diff --git a/Keybinding.Test/SeparatorKeyParsingTests.cs b/Keybinding.Test/SeparatorKeyParsingTests.cs new file mode 100644 index 0000000..86fc687 --- /dev/null +++ b/Keybinding.Test/SeparatorKeyParsingTests.cs @@ -0,0 +1,129 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using ktsu.Keybinding.Core; +using ktsu.Keybinding.Core.Contracts; +using ktsu.Keybinding.Core.Models; + +/// +/// Tests that the '+' and ',' keys can be bound, and that input with a missing key is rejected rather than dropped. +/// +[TestClass] +public class SeparatorKeyParsingTests +{ + private static KeybindingManager CreateManager() => + new(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString())); + + [TestMethod] + public void ChordParse_CtrlPlus_IsCtrlAndPlusKey() + { + Chord chord = Chord.Parse("Ctrl++"); + + Assert.AreNotEqual(Chord.Parse("Ctrl"), chord); + Assert.HasCount(2, chord.Notes); + Assert.IsTrue(chord.Notes.Any(n => n.Key.ToString() == "+")); + } + + [TestMethod] + public void ChordParse_CtrlComma_IsCtrlAndCommaKey() + { + Chord chord = Chord.Parse("Ctrl+,"); + + Assert.HasCount(2, chord.Notes); + Assert.IsTrue(chord.Notes.Any(n => n.Key.ToString() == ",")); + } + + [TestMethod] + public void ChordParse_PlusAlone_IsPlusKey() + { + Chord chord = Chord.Parse("+"); + + Assert.HasCount(1, chord.Notes); + Assert.AreEqual("+", chord.Notes.Single().Key.ToString()); + } + + [TestMethod] + public void PhraseParse_CtrlComma_IsSingleChordWithCommaKey() + { + Phrase phrase = Phrase.Parse("Ctrl+,"); + + Assert.HasCount(1, phrase.Sequence); + Assert.AreEqual(Chord.Parse("Ctrl+,"), phrase.Sequence[0]); + Assert.AreNotEqual(Phrase.Parse("Ctrl"), phrase); + } + + [TestMethod] + public void PhraseParse_CommaKeyFollowedByChord_SplitsCorrectly() + { + Phrase phrase = Phrase.Parse("Ctrl+,, R"); + + Assert.HasCount(2, phrase.Sequence); + Assert.AreEqual(Chord.Parse("Ctrl+,"), phrase.Sequence[0]); + Assert.AreEqual(Chord.Parse("R"), phrase.Sequence[1]); + } + + [TestMethod] + public void PhraseParse_PlusKeyInSequence_SplitsCorrectly() + { + Phrase phrase = Phrase.Parse("Ctrl++, Ctrl+R"); + + Assert.HasCount(2, phrase.Sequence); + Assert.AreEqual(Chord.Parse("Ctrl++"), phrase.Sequence[0]); + Assert.AreEqual(Chord.Parse("Ctrl+R"), phrase.Sequence[1]); + } + + [TestMethod] + public void Parse_RoundTripsThroughToString() + { + Phrase phrase = Phrase.Parse("Ctrl+,, Ctrl++"); + + Assert.AreEqual(phrase, Phrase.Parse(phrase.ToString())); + } + + [TestMethod] + [DataRow("Ctrl+")] + [DataRow("A++B")] + [DataRow("Ctrl++A+")] + public void ChordParse_MissingKey_Throws(string value) => + Assert.ThrowsExactly(() => Chord.Parse(value)); + + [TestMethod] + [DataRow("Ctrl+R,")] + [DataRow("Ctrl+R,,R")] + [DataRow(", R")] + public void PhraseParse_MissingChord_Throws(string value) => + Assert.ThrowsExactly(() => Phrase.Parse(value)); + + [TestMethod] + public void ServiceParseChord_CtrlPlus_MatchesChordParse() + { + using KeybindingManager manager = CreateManager(); + IKeybindingService service = manager.Keybindings; + + Chord chord = service.ParseChord("Ctrl++"); + + Assert.HasCount(2, chord.Notes); + Assert.AreEqual(Chord.Parse("Ctrl++"), chord); + } + + [TestMethod] + public void ServiceParsePhrase_CtrlComma_MatchesPhraseParse() + { + using KeybindingManager manager = CreateManager(); + IKeybindingService service = manager.Keybindings; + + Phrase phrase = service.ParsePhrase("Ctrl+,"); + + Assert.AreEqual(Phrase.Parse("Ctrl+,"), phrase); + } + + [TestMethod] + public void ServiceParseChord_MissingKey_Throws() + { + using KeybindingManager manager = CreateManager(); + IKeybindingService service = manager.Keybindings; + + Assert.ThrowsExactly(() => service.ParseChord("Ctrl+")); + } +} diff --git a/Keybinding/Models/KeyStringTokenizer.cs b/Keybinding/Models/KeyStringTokenizer.cs new file mode 100644 index 0000000..578e5aa --- /dev/null +++ b/Keybinding/Models/KeyStringTokenizer.cs @@ -0,0 +1,98 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Core.Models; + +using System.Text; + +/// +/// Splits chord and phrase strings into tokens, treating a separator character that appears where a key is +/// expected as the literal key, so "Ctrl++" is Ctrl and Plus and "Ctrl+," is Ctrl and Comma. +/// +internal static class KeyStringTokenizer +{ + private const char NoteSeparator = '+'; + private const char ChordSeparator = ','; + private const string ChordSeparators = "+"; + private const string PhraseSeparators = "+,"; + + /// + /// Splits a chord string such as "Ctrl+Alt+S" or "Ctrl++" into note strings. + /// + /// 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". + internal static string[] SplitChord(string value) => Split(value, NoteSeparator, ChordSeparators); + + /// + /// Splits a phrase string such as "Ctrl+R, R" or "Ctrl+,, R" into chord strings. + /// + /// The phrase string. + /// The trimmed chord strings, or an empty array for whitespace input. + /// Thrown when a key or chord is missing, as in "Ctrl+" or "A,,B". + internal static string[] SplitPhrase(string value) => Split(value, ChordSeparator, PhraseSeparators); + + private static string[] Split(string value, char splitOn, string separators) + { + List tokens = []; + StringBuilder current = new(); + bool expectKey = true; + bool sawContent = false; + + for (int i = 0; i < value.Length; i++) + { + char c = value[i]; + + if (separators.Contains(c)) + { + if (!expectKey) + { + if (c == splitOn) + { + tokens.Add(current.ToString().Trim()); + current.Clear(); + } + else + { + current.Append(c); + } + + expectKey = true; + continue; + } + + // 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. + int next = i + 1; + while (next < value.Length && char.IsWhiteSpace(value[next])) + { + next++; + } + + if (next < value.Length && !separators.Contains(value[next])) + { + throw new ArgumentException($"Missing key before '{c}' at position {i} in \"{value}\"", nameof(value)); + } + } + + current.Append(c); + if (!char.IsWhiteSpace(c)) + { + expectKey = false; + sawContent = true; + } + } + + if (!sawContent) + { + return []; + } + + if (expectKey) + { + throw new ArgumentException($"Missing key at the end of \"{value}\"", nameof(value)); + } + + tokens.Add(current.ToString().Trim()); + return [.. tokens]; + } +} diff --git a/Keybinding/Models/MusicalTypes.cs b/Keybinding/Models/MusicalTypes.cs index 88c6502..8549fb9 100644 --- a/Keybinding/Models/MusicalTypes.cs +++ b/Keybinding/Models/MusicalTypes.cs @@ -262,7 +262,7 @@ public static Chord Parse(string value) throw new ArgumentException("Value cannot be null or whitespace", nameof(value)); } - string[] parts = [.. value.Split('+', StringSplitOptions.RemoveEmptyEntries).Select(p => p.Trim())]; + string[] parts = KeyStringTokenizer.SplitChord(value); if (parts.Length == 0) { @@ -396,8 +396,7 @@ public static Phrase Parse(string value) throw new ArgumentException("Value cannot be null or whitespace", nameof(value)); } - string[] chordStrings = [.. value.Split(',', StringSplitOptions.RemoveEmptyEntries) - .Select(s => s.Trim())]; + string[] chordStrings = KeyStringTokenizer.SplitPhrase(value); return chordStrings.Length == 0 ? throw new ArgumentException("Invalid phrase format", nameof(value)) diff --git a/Keybinding/Services/KeybindingService.cs b/Keybinding/Services/KeybindingService.cs index 6f4fcaa..c0f9a14 100644 --- a/Keybinding/Services/KeybindingService.cs +++ b/Keybinding/Services/KeybindingService.cs @@ -183,7 +183,7 @@ public Phrase ParsePhrase(string phraseString) } // Split by comma to get individual chord strings - string[] chordStrings = phraseString.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + string[] chordStrings = KeyStringTokenizer.SplitPhrase(phraseString); List chords = []; foreach (string chordString in chordStrings) @@ -204,7 +204,7 @@ public Chord ParseChord(string chordString) } // Split by + to get individual note strings - string[] noteStrings = chordString.Split('+', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + string[] noteStrings = KeyStringTokenizer.SplitChord(chordString); List notes = []; foreach (string noteString in noteStrings) From ee1d092ba0c64bef13b347d26ad97403038fb0f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 01:34:25 +0000 Subject: [PATCH 2/2] Reduce KeyStringTokenizer.Split cognitive complexity Move the lookahead that decides whether a separator is a literal key into its own method, and use Assert.Contains in the separator key tests, per SonarCloud S3776 and MSTEST0037. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Q8KgDr8CfZLWPQ5o6nZdcV --- Keybinding.Test/SeparatorKeyParsingTests.cs | 4 +- Keybinding/Models/KeyStringTokenizer.cs | 56 ++++++++++++--------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/Keybinding.Test/SeparatorKeyParsingTests.cs b/Keybinding.Test/SeparatorKeyParsingTests.cs index 86fc687..fcd6da5 100644 --- a/Keybinding.Test/SeparatorKeyParsingTests.cs +++ b/Keybinding.Test/SeparatorKeyParsingTests.cs @@ -22,7 +22,7 @@ public void ChordParse_CtrlPlus_IsCtrlAndPlusKey() Assert.AreNotEqual(Chord.Parse("Ctrl"), chord); Assert.HasCount(2, chord.Notes); - Assert.IsTrue(chord.Notes.Any(n => n.Key.ToString() == "+")); + Assert.Contains(n => n.Key.ToString() == "+", chord.Notes); } [TestMethod] @@ -31,7 +31,7 @@ public void ChordParse_CtrlComma_IsCtrlAndCommaKey() Chord chord = Chord.Parse("Ctrl+,"); Assert.HasCount(2, chord.Notes); - Assert.IsTrue(chord.Notes.Any(n => n.Key.ToString() == ",")); + Assert.Contains(n => n.Key.ToString() == ",", chord.Notes); } [TestMethod] diff --git a/Keybinding/Models/KeyStringTokenizer.cs b/Keybinding/Models/KeyStringTokenizer.cs index 578e5aa..a349fa4 100644 --- a/Keybinding/Models/KeyStringTokenizer.cs +++ b/Keybinding/Models/KeyStringTokenizer.cs @@ -42,36 +42,26 @@ private static string[] Split(string value, char splitOn, string separators) { char c = value[i]; - if (separators.Contains(c)) + if (separators.Contains(c) && !expectKey) { - if (!expectKey) + // A separator after a key ends the note, and ends the token when it is the one being split on + if (c == splitOn) { - if (c == splitOn) - { - tokens.Add(current.ToString().Trim()); - current.Clear(); - } - else - { - current.Append(c); - } - - expectKey = true; - continue; + tokens.Add(current.ToString().Trim()); + current.Clear(); } - - // 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. - int next = i + 1; - while (next < value.Length && char.IsWhiteSpace(value[next])) + else { - next++; + current.Append(c); } - if (next < value.Length && !separators.Contains(value[next])) - { - throw new ArgumentException($"Missing key before '{c}' at position {i} in \"{value}\"", nameof(value)); - } + expectKey = true; + continue; + } + + if (separators.Contains(c)) + { + EnsureSeparatorIsKey(value, i, separators); } current.Append(c); @@ -95,4 +85,22 @@ private static string[] Split(string value, char splitOn, string separators) tokens.Add(current.ToString().Trim()); return [.. tokens]; } + + /// + /// 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. + /// + private static void EnsureSeparatorIsKey(string value, int index, string separators) + { + int next = index + 1; + while (next < value.Length && char.IsWhiteSpace(value[next])) + { + next++; + } + + if (next < value.Length && !separators.Contains(value[next])) + { + throw new ArgumentException($"Missing key before '{value[index]}' at position {index} in \"{value}\"", nameof(value)); + } + } }