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);