diff --git a/src/DiffEngineTray.Tests/KeyNameTests.cs b/src/DiffEngineTray.Tests/KeyNameTests.cs new file mode 100644 index 00000000..0193e078 --- /dev/null +++ b/src/DiffEngineTray.Tests/KeyNameTests.cs @@ -0,0 +1,116 @@ +using Keys = System.Windows.Forms.Keys; + +/// +/// 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. +/// +[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); + } + + /// + /// Which is why this is not a round trip through ToString: Keys gives several values + /// two names, and prints the other one. + /// + [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); + } + + /// + /// 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. + /// + [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); + } + + /// + /// The modifiers are checkboxes of their own, so a key holding them is a misunderstanding of + /// the file rather than a key. + /// + [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(); + } + + /// + /// 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. + /// + [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(); + + Program.ReBindKeys(settings, register, tracker, warnings.Add); + + await Assert.That(warnings).HasSingleItem(); + await Assert.That(warnings[0]).Contains("Ctrl+A"); + } +} diff --git a/src/DiffEngineTray.Tests/SettingsValidatorTests.cs b/src/DiffEngineTray.Tests/SettingsValidatorTests.cs index ef92f875..544040ca 100644 --- a/src/DiffEngineTray.Tests/SettingsValidatorTests.cs +++ b/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(); } + /// + /// 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. + /// + [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() { diff --git a/src/DiffEngineTray/HotKey/KeyName.cs b/src/DiffEngineTray/HotKey/KeyName.cs new file mode 100644 index 00000000..a1c0b183 --- /dev/null +++ b/src/DiffEngineTray/HotKey/KeyName.cs @@ -0,0 +1,28 @@ +/// +/// Reads the key half of a . 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. +/// +/// Numbers and lists are rejected before parsing, because +/// reads both: "1" is , 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 - Enum.Parse takes "Enter" and +/// prints "Return". +/// +/// +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; + } +} diff --git a/src/DiffEngineTray/HotKey/KeyRegister.cs b/src/DiffEngineTray/HotKey/KeyRegister.cs index 97a49615..553e3236 100644 --- a/src/DiffEngineTray/HotKey/KeyRegister.cs +++ b/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(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) diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index c5684f14..3cdd7717 100644 --- a/src/DiffEngineTray/Program.cs +++ b/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? 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."); } } diff --git a/src/DiffEngineTray/Settings/SettingsValidator.cs b/src/DiffEngineTray/Settings/SettingsValidator.cs index 447bc023..322ab1d9 100644 --- a/src/DiffEngineTray/Settings/SettingsValidator.cs +++ b/src/DiffEngineTray/Settings/SettingsValidator.cs @@ -26,6 +26,12 @@ static void ValidateHotKey(List 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"); } } } \ No newline at end of file