From 8cc24e1cfb6c0c6363fc2c903d5c2bd3d9065734 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 22:53:57 +1000 Subject: [PATCH] Re-register the hot keys as one thing or not at all Saving Options cleared and re-registered all three hot keys and only then looked at whether any of them had failed. A collision on the second returned "Binding already registered" with the first already live on its new combination, the second unbound, and settings.json - written after this - still holding the old ones. So the dialog said the save had failed while the keys said otherwise, until the tray was restarted. A failed attempt now puts back the bindings that were live before it. That the restore itself succeeded is not reported: they are the bindings the tray already had, and there is nothing to be done at the dialog about one of them having been taken since. --- .../OptionsFormLauncherTests.cs | 96 +++++++++++++++++++ .../Settings/OptionsFormLauncher.cs | 42 ++++++-- 2 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 src/DiffEngineTray.Tests/OptionsFormLauncherTests.cs diff --git a/src/DiffEngineTray.Tests/OptionsFormLauncherTests.cs b/src/DiffEngineTray.Tests/OptionsFormLauncherTests.cs new file mode 100644 index 00000000..11639abc --- /dev/null +++ b/src/DiffEngineTray.Tests/OptionsFormLauncherTests.cs @@ -0,0 +1,96 @@ +using Keys = System.Windows.Forms.Keys; + +/// +/// Saving the Options form re-registers all three hot keys. The combinations used here are held +/// for the length of a test and are ones nothing else is likely to want. +/// +[TUnit.Core.Executors.STAThreadExecutor] +public class OptionsFormLauncherTests +{ + /// + /// Each hot key used to be bound as it was reached, so a collision on the second returned + /// with the first already live on its new combination - while settings.json, which is written + /// after this, still held the old one. The dialog said the save failed and the keys disagreed + /// with it until the tray was restarted. + /// + [Test] + public async Task A_collision_leaves_every_hot_key_as_it_was() + { + var previous = new Settings + { + AcceptAllHotKey = HotKey(Keys.F13) + }; + // Both on one combination, so the second registration is refused by the OS rather than by + // whatever else happens to be running + var settings = new Settings + { + AcceptAllHotKey = HotKey(Keys.F14), + DiscardAllHotKey = HotKey(Keys.F14) + }; + await using var tracker = new RecordingTracker(); + using var register = new KeyRegister(0); + Bind(register, previous, tracker); + + var errors = OptionsFormLauncher.ReBind(register, tracker, previous, settings); + + await Assert.That(errors).IsNotEmpty(); + // What the OS thinks, which is the only account of it that matters + await Assert.That(IsHeld(Keys.F14)).IsFalse(); + await Assert.That(IsHeld(Keys.F13)).IsTrue(); + } + + [Test] + public async Task A_save_that_binds_takes_every_hot_key() + { + var previous = new Settings + { + AcceptAllHotKey = HotKey(Keys.F15) + }; + var settings = new Settings + { + AcceptAllHotKey = HotKey(Keys.F16), + DiscardAllHotKey = HotKey(Keys.F17) + }; + await using var tracker = new RecordingTracker(); + using var register = new KeyRegister(0); + Bind(register, previous, tracker); + + var errors = OptionsFormLauncher.ReBind(register, tracker, previous, settings); + + await Assert.That(errors).IsEmpty(); + await Assert.That(IsHeld(Keys.F16)).IsTrue(); + await Assert.That(IsHeld(Keys.F17)).IsTrue(); + // The one it replaced is given up, rather than left registered for a key the settings no + // longer mention + await Assert.That(IsHeld(Keys.F15)).IsFalse(); + } + + static void Bind(KeyRegister register, Settings settings, RecordingTracker tracker) => + Program.ReBindKeys(settings, register, tracker); + + /// + /// Whether anything holds the combination, asked by trying to take it. A register of one that + /// is already taken is what the OS refuses, so a success means it was free - and gives it + /// straight back. + /// + static bool IsHeld(Keys key) + { + using var probe = new KeyRegister(0); + return !probe.TryAddBinding( + id: 9, + KeyModifiers.Control | KeyModifiers.Shift | KeyModifiers.Alt, + key, + () => + { + }); + } + + static HotKey HotKey(Keys key) => + new() + { + Control = true, + Shift = true, + Alt = true, + Key = key.ToString() + }; +} diff --git a/src/DiffEngineTray/Settings/OptionsFormLauncher.cs b/src/DiffEngineTray/Settings/OptionsFormLauncher.cs index 1b40bdde..fcca3225 100644 --- a/src/DiffEngineTray/Settings/OptionsFormLauncher.cs +++ b/src/DiffEngineTray/Settings/OptionsFormLauncher.cs @@ -13,24 +13,20 @@ public static async Task Launch(KeyRegister keyRegister, Tracker tracker) var settings = await SettingsHelper.Read(); using var form = new OptionsForm( settings, - newSettings => Save(keyRegister, tracker, newSettings)); + newSettings => Save(keyRegister, tracker, settings, newSettings)); instance = form; await form.ShowDialogAsync(); instance = null; } - static async Task> Save(KeyRegister keyRegister, Tracker tracker, Settings settings) + static async Task> Save(KeyRegister keyRegister, Tracker tracker, Settings previous, Settings settings) { if (!settings.IsValidate(out var errors)) { return errors; } - var saveErrors = new List(); - - AddHotKey(keyRegister, settings.AcceptAllHotKey, KeyBindingIds.AcceptAll, () => tracker.AcceptAll(), saveErrors); - AddHotKey(keyRegister, settings.DiscardAllHotKey, KeyBindingIds.DiscardAll, tracker.Clear, saveErrors); - AddHotKey(keyRegister, settings.AcceptOpenHotKey, KeyBindingIds.AcceptOpen, () => tracker.AcceptOpen(), saveErrors); + var saveErrors = ReBind(keyRegister, tracker, previous, settings); if (saveErrors.Count != 0) { @@ -51,6 +47,38 @@ static async Task> Save(KeyRegister keyRegister, Tra return []; } + /// + /// The three hot keys, all of them or none. + /// + /// Each one used to be bound as it was reached, and a collision on the second returned with + /// the first already live on its new combination while settings.json still held the old one - + /// so the dialog said the save had failed, and the keys said otherwise until the next restart. + /// + /// + internal static List ReBind(KeyRegister keyRegister, Tracker tracker, Settings previous, Settings settings) + { + var saveErrors = new List(); + Bind(keyRegister, tracker, settings, saveErrors); + + if (saveErrors.Count == 0) + { + return saveErrors; + } + + // Back to what was live before this attempt. Its own outcome is not reported: these are + // the bindings the tray already had, and there is nothing the person at the dialog could + // do about one of them having been taken in the meantime + Bind(keyRegister, tracker, previous, []); + return saveErrors; + } + + static void Bind(KeyRegister keyRegister, Tracker tracker, Settings settings, List saveErrors) + { + AddHotKey(keyRegister, settings.AcceptAllHotKey, KeyBindingIds.AcceptAll, () => tracker.AcceptAll(), saveErrors); + AddHotKey(keyRegister, settings.DiscardAllHotKey, KeyBindingIds.DiscardAll, tracker.Clear, saveErrors); + AddHotKey(keyRegister, settings.AcceptOpenHotKey, KeyBindingIds.AcceptOpen, () => tracker.AcceptOpen(), saveErrors); + } + static void AddHotKey(KeyRegister keyRegister, HotKey? hotKey, int id, Action action, List saveErrors) { keyRegister.ClearBinding(id);