Reject phrase strings and keys with spaces or commas when parsing a chord [patch]

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+[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<ArgumentException>(() => Chord.Parse(value));
+		Assert.ThrowsExactly<ArgumentException>(() => CreateService().ParseChord(value));
+	}
+
+	[TestMethod]
+	[DataRow("Page Up")]
+	[DataRow("Ctrl+Page Up")]
+	[DataRow("Ctrl+,x")]
+	public void ChordParse_KeyWithWhitespaceOrComma_Throws(string value)
+	{
+		Assert.ThrowsExactly<ArgumentException>(() => Chord.Parse(value));
+		Assert.ThrowsExactly<ArgumentException>(() => 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<ArgumentException>(() => 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]);
+	}
+}

Keybinding/Models/KeyStringTokenizer.cs

@@ -20,7 +20,7 @@ internal static class KeyStringTokenizer
 	/// </summary>
 	/// <param name="value">The chord string.</param>
 	/// <returns>The trimmed note strings, or an empty array for whitespace input.</returns>
-	/// <exception cref="ArgumentException">Thrown when a key is missing, as in "Ctrl+" or "A++B".</exception>
+	/// <exception cref="ArgumentException">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".</exception>
 	internal static string[] SplitChord(string value) => Split(value, NoteSeparator, ChordSeparators);
 
 	/// <summary>
@@ -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];
 	}
 
+	/// <summary>
+	/// 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+,".
+	/// </summary>
+	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));
+		}
+	}
+
 	/// <summary>
 	/// 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.

Keybinding/Models/MusicalTypes.cs

@@ -14,7 +14,7 @@ public sealed class Note : IEquatable<Note>
 	/// Initializes a new instance of the <see cref="Note"/> class
 	/// </summary>
 	/// <param name="key">The key that this note represents</param>
-	/// <exception cref="ArgumentException">Thrown when key is null or whitespace</exception>
+	/// <exception cref="ArgumentException">Thrown when key is null or whitespace, or contains whitespace, '+' or ',' other than as the single-character key "+" or ","</exception>
 	[JsonConstructor]
 	public Note(NoteName key)
 	{
@@ -27,7 +27,7 @@ public Note(NoteName key)
 	/// Initializes a new instance of the <see cref="Note"/> class from a string
 	/// </summary>
 	/// <param name="key">The key string that this note represents</param>
-	/// <exception cref="ArgumentException">Thrown when key is null or whitespace</exception>
+	/// <exception cref="ArgumentException">Thrown when key is null or whitespace, or contains whitespace, '+' or ',' other than as the single-character key "+" or ","</exception>
 	public Note(string key)
 	{
 		if (string.IsNullOrWhiteSpace(key))
@@ -44,7 +44,19 @@ public Note(string key)
 	/// </summary>
 	/// <param name="key">A key name in any case</param>
 	/// <returns>The canonical key name</returns>
-	private static string NormalizeKey(string key) => CanonicalizeKey(key.Trim().ToUpperInvariant());
+	private static string NormalizeKey(string key) => CanonicalizeKey(ValidateKey(key.Trim()).ToUpperInvariant());
+
+	/// <summary>
+	/// 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.
+	/// </summary>
+	/// <param name="key">A trimmed key name</param>
+	/// <returns>The key name, unchanged</returns>
+	/// <exception cref="ArgumentException">Thrown when the key contains whitespace, '+' or ','</exception>
+	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;
 
 	/// <summary>
 	/// Maps a modifier alias to its canonical key name, so that every way of building a note