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..33633a5 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,8 @@ private static string[] Split(string value, char splitOn, string separators) { char c = value[i]; + EnsureNotChordSeparatorInChord(value, i, splitOn, expectKey); + 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 @@ -86,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. 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