Do not let a hand edited key name take the tray down

src/DiffEngineTray.Tests/KeyNameTests.cs

@@ -0,0 +1,116 @@
+using Keys = System.Windows.Forms.Keys;
+
+/// <summary>
+/// The key half of a hot key, which reaches the tray as whatever text settings.json holds. The
+/// Options form only ever writes a letter, so everything rejected here arrives by hand edit.
+/// </summary>
+[TUnit.Core.Executors.STAThreadExecutor]
+public class KeyNameTests
+{
+    [Test]
+    public async Task Reads_a_key_name()
+    {
+        var parsed = KeyName.TryParse("A", out var key);
+
+        await Assert.That(parsed).IsTrue();
+        await Assert.That(key).IsEqualTo(Keys.A);
+    }
+
+    [Test]
+    public async Task Reads_a_key_name_in_any_case()
+    {
+        var parsed = KeyName.TryParse("f12", out var key);
+
+        await Assert.That(parsed).IsTrue();
+        await Assert.That(key).IsEqualTo(Keys.F12);
+    }
+
+    /// <summary>
+    /// Which is why this is not a round trip through <c>ToString</c>: Keys gives several values
+    /// two names, and prints the other one.
+    /// </summary>
+    [Test]
+    public async Task Reads_an_alias()
+    {
+        var parsed = KeyName.TryParse("Enter", out var key);
+
+        await Assert.That(parsed).IsTrue();
+        await Assert.That(key).IsEqualTo(Keys.Return);
+    }
+
+    /// <summary>
+    /// Enum.Parse reads the underlying number as readily as the name, so "1" used to bind the
+    /// left mouse button to a hot key that looked like it was for the digit.
+    /// </summary>
+    [Test]
+    public async Task Rejects_a_number()
+    {
+        var parsed = KeyName.TryParse("1", out var key);
+
+        await Assert.That(parsed).IsFalse();
+        await Assert.That(key).IsNotEqualTo(Keys.LButton);
+    }
+
+    /// <summary>
+    /// The modifiers are checkboxes of their own, so a key holding them is a misunderstanding of
+    /// the file rather than a key.
+    /// </summary>
+    [Test]
+    public async Task Rejects_a_modifier_combination() =>
+        await Assert.That(KeyName.TryParse("Ctrl+A", out _)).IsFalse();
+
+    [Test]
+    public async Task Rejects_a_flag_list() =>
+        await Assert.That(KeyName.TryParse("A,B", out _)).IsFalse();
+
+    [Test]
+    public async Task Rejects_nothing()
+    {
+        await Assert.That(KeyName.TryParse(null, out _)).IsFalse();
+        await Assert.That(KeyName.TryParse("", out _)).IsFalse();
+        await Assert.That(KeyName.TryParse(" ", out _)).IsFalse();
+    }
+
+    [Test]
+    public async Task A_bad_key_leaves_the_hot_key_unbound()
+    {
+        using var register = new KeyRegister(0);
+
+        // Nothing is registered with the OS for a key that is not one, so the handle above is
+        // never used and no hot key is taken from the machine running this
+        var bound = register.TryAddBinding(
+            KeyBindingIds.AcceptAll,
+            shift: true,
+            control: false,
+            alt: false,
+            "Ctrl+A",
+            () => throw new("Not bound, so never invoked"));
+
+        await Assert.That(bound).IsFalse();
+    }
+
+    /// <summary>
+    /// The tray binds its hot keys at startup, so a key name it cannot read used to throw out of
+    /// startup and take the tray down at every login until settings.json was deleted by hand.
+    /// </summary>
+    [Test]
+    public async Task A_bad_key_does_not_stop_the_tray_starting()
+    {
+        var settings = new Settings
+        {
+            AcceptAllHotKey = new()
+            {
+                Control = true,
+                Key = "Ctrl+A"
+            }
+        };
+        await using var tracker = new RecordingTracker();
+        using var register = new KeyRegister(0);
+        var warnings = new List<string>();
+
+        Program.ReBindKeys(settings, register, tracker, warnings.Add);
+
+        await Assert.That(warnings).HasSingleItem();
+        await Assert.That(warnings[0]).Contains("Ctrl+A");
+    }
+}

src/DiffEngineTray.Tests/SettingsValidatorTests.cs

@@ -45,6 +45,28 @@ public async Task Hotkey_without_key_is_invalid()
         await Assert.That(errors.Contains("HotKey: key is required")).IsTrue();
     }
 
+    /// <summary>
+    /// Only a hand edit produces one, the form offering nothing but letters, and it used to pass
+    /// validation and then throw out of the hot key registration at every startup.
+    /// </summary>
+    [Test]
+    public async Task Hotkey_with_a_key_that_is_not_a_key_name_is_invalid()
+    {
+        var settings = new Settings
+        {
+            AcceptAllHotKey = new()
+            {
+                Shift = true,
+                Key = "Ctrl+A"
+            }
+        };
+
+        var valid = settings.IsValidate(out var errors);
+
+        await Assert.That(valid).IsFalse();
+        await Assert.That(errors.Contains("HotKey: 'Ctrl+A' is not a key name")).IsTrue();
+    }
+
     [Test]
     public async Task Valid_hotkey_passes()
     {

src/DiffEngineTray/HotKey/KeyName.cs

@@ -0,0 +1,28 @@
+/// <summary>
+/// Reads the key half of a <see cref="HotKey"/>. It is a name in settings.json, so it is whatever
+/// a hand edit left there rather than one of the twenty six letters the Options form offers.
+/// <para>
+/// Numbers and lists are rejected before parsing, because <see cref="Enum.TryParse{TEnum}(string, bool, out TEnum)"/>
+/// reads both: "1" is <see cref="Keys.LButton" />, silently binding the left mouse button, and
+/// "A,B" is a flag combination rather than a key. Aliases are accepted, which is why this is not
+/// a round trip through <see cref="Enum.ToString()" /> - <c>Enum.Parse</c> takes "Enter" and
+/// prints "Return".
+/// </para>
+/// </summary>
+static class KeyName
+{
+    public static bool TryParse([NotNullWhen(true)] string? name, out Keys key)
+    {
+        key = Keys.None;
+
+        if (string.IsNullOrWhiteSpace(name) ||
+            name.Contains(',') ||
+            long.TryParse(name, out _))
+        {
+            return false;
+        }
+
+        return Enum.TryParse(name, true, out key) &&
+               key != Keys.None;
+    }
+}

src/DiffEngineTray/HotKey/KeyRegister.cs

@@ -37,7 +37,15 @@ public bool TryAddBinding(int id, bool shift, bool control, bool alt, string key
             modifiers |= KeyModifiers.Alt;
         }
 
-        return TryAddBinding(id, modifiers, Enum.Parse<Keys>(key, true), action);
+        if (!KeyName.TryParse(key, out var keys))
+        {
+            // Unbound rather than thrown. This runs at startup for every configured hot key, and
+            // a hand edited settings.json used to take the tray down at every login
+            Log.Error("'{Key}' is not a key name. The hot key was not bound.", key);
+            return false;
+        }
+
+        return TryAddBinding(id, modifiers, keys, action);
     }
 
     public bool TryAddBinding(int id, KeyModifiers modifiers, Keys keys, Action action)

src/DiffEngineTray/Program.cs

@@ -97,7 +97,7 @@ void Warn(string message) =>
         var task = StartServer(tracker, cancel);
 
         using var keyRegister = new KeyRegister(icon.Handle());
-        ReBindKeys(settings, keyRegister, tracker);
+        ReBindKeys(settings, keyRegister, tracker, Warn);
 
         var menuStrip = MenuBuilder.Build(
             Application.Exit,
@@ -133,18 +133,27 @@ void Warn(string message) =>
     [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "ShowContextMenu")]
     static extern void ShowContextMenu(NotifyIcon icon);
 
-    internal static void ReBindKeys(Settings settings, KeyRegister keyRegister, Tracker tracker)
+    internal static void ReBindKeys(Settings settings, KeyRegister keyRegister, Tracker tracker, Action<string>? warn = null)
     {
         foreach (var binding in BuildKeyBindings(settings, tracker))
         {
             var hotKey = binding.HotKey;
-            keyRegister.TryAddBinding(
-                binding.Id,
-                hotKey.Shift,
-                hotKey.Control,
-                hotKey.Alt,
-                hotKey.Key,
-                binding.Action);
+            if (keyRegister.TryAddBinding(
+                    binding.Id,
+                    hotKey.Shift,
+                    hotKey.Control,
+                    hotKey.Alt,
+                    hotKey.Key,
+                    binding.Action))
+            {
+                continue;
+            }
+
+            // Said out loud, because the alternative is a hot key that quietly does nothing. Only
+            // settings.json can produce a key name the Options form cannot, so only a hand edit
+            // reaches the first half of this
+            warn?.Invoke(
+                $"Could not bind the hot key '{hotKey.Key}'. It is either not a key name, or already registered by another application.");
         }
     }
 

src/DiffEngineTray/Settings/SettingsValidator.cs

@@ -26,6 +26,12 @@ static void ValidateHotKey(List<string> errors, HotKey? hotKey)
         if (string.IsNullOrWhiteSpace(hotKey.Key))
         {
             errors.Add("HotKey: key is required");
+            return;
+        }
+
+        if (!KeyName.TryParse(hotKey.Key, out _))
+        {
+            errors.Add($"HotKey: '{hotKey.Key}' is not a key name");
         }
     }
 }