Simplfiied app scope key setters

Terminal.Gui/Application/Application.Keyboard.cs

@@ -5,7 +5,7 @@ namespace Terminal.Gui;
 
 public static partial class Application // Keyboard handling
 {
-    private static Key _nextTabKey = Key.Empty; // Defined in config.json
+    private static Key _nextTabKey = Key.Tab; // Resources/config.json overrrides
 
     /// <summary>Alternative key to navigate forwards through views. Ctrl+Tab is the primary key.</summary>
     [SerializableConfigurationProperty (Scope = typeof (SettingsScope))]
@@ -17,22 +17,13 @@ public static partial class Application // Keyboard handling
         {
             if (_nextTabKey != value)
             {
-                Key oldKey = _nextTabKey;
+                ReplaceKey (_nextTabKey, value);
                 _nextTabKey = value;
-
-                if (_nextTabKey == Key.Empty)
-                {
-                    KeyBindings.Remove (_nextTabKey);
-                }
-                else
-                {
-                    KeyBindings.ReplaceKey (oldKey, _nextTabKey);
-                }
             }
         }
     }
 
-    private static Key _prevTabKey = Key.Empty; // Defined in config.json
+    private static Key _prevTabKey = Key.Tab.WithShift; // Resources/config.json overrrides
 
     /// <summary>Alternative key to navigate backwards through views. Shift+Ctrl+Tab is the primary key.</summary>
     [SerializableConfigurationProperty (Scope = typeof (SettingsScope))]
@@ -44,22 +35,13 @@ public static partial class Application // Keyboard handling
         {
             if (_prevTabKey != value)
             {
-                Key oldKey = _prevTabKey;
+                ReplaceKey (_prevTabKey, value);
                 _prevTabKey = value;
-
-                if (_prevTabKey == Key.Empty)
-                {
-                    KeyBindings.Remove (_prevTabKey);
-                }
-                else
-                {
-                    KeyBindings.ReplaceKey (oldKey, _prevTabKey);
-                }
             }
         }
     }
 
-    private static Key _nextTabGroupKey = Key.Empty; // Defined in config.json
+    private static Key _nextTabGroupKey = Key.F6; // Resources/config.json overrrides
 
     /// <summary>Alternative key to navigate forwards through views. Ctrl+Tab is the primary key.</summary>
     [SerializableConfigurationProperty (Scope = typeof (SettingsScope))]
@@ -71,22 +53,13 @@ public static partial class Application // Keyboard handling
         {
             if (_nextTabGroupKey != value)
             {
-                Key oldKey = _nextTabGroupKey;
+                ReplaceKey (_nextTabGroupKey, value);
                 _nextTabGroupKey = value;
-
-                if (_nextTabGroupKey == Key.Empty)
-                {
-                    KeyBindings.Remove (_nextTabGroupKey);
-                }
-                else
-                {
-                    KeyBindings.ReplaceKey (oldKey, _nextTabGroupKey);
-                }
             }
         }
     }
 
-    private static Key _prevTabGroupKey = Key.Empty; // Defined in config.json
+    private static Key _prevTabGroupKey = Key.F6.WithShift; // Resources/config.json overrrides
 
     /// <summary>Alternative key to navigate backwards through views. Shift+Ctrl+Tab is the primary key.</summary>
     [SerializableConfigurationProperty (Scope = typeof (SettingsScope))]
@@ -98,22 +71,13 @@ public static partial class Application // Keyboard handling
         {
             if (_prevTabGroupKey != value)
             {
-                Key oldKey = _prevTabGroupKey;
+                ReplaceKey (_prevTabGroupKey, value);
                 _prevTabGroupKey = value;
-
-                if (_prevTabGroupKey == Key.Empty)
-                {
-                    KeyBindings.Remove (_prevTabGroupKey);
-                }
-                else
-                {
-                    KeyBindings.ReplaceKey (oldKey, _prevTabGroupKey);
-                }
             }
         }
     }
 
-    private static Key _quitKey = Key.Empty; // Defined in config.json
+    private static Key _quitKey = Key.Esc; // Resources/config.json overrrides
 
     /// <summary>Gets or sets the key to quit the application.</summary>
     [SerializableConfigurationProperty (Scope = typeof (SettingsScope))]
@@ -125,21 +89,29 @@ public static partial class Application // Keyboard handling
         {
             if (_quitKey != value)
             {
-                Key oldKey = _quitKey;
+                ReplaceKey (_quitKey, value);
                 _quitKey = value;
-
-                if (_quitKey == Key.Empty)
-                {
-                    KeyBindings.Remove (_quitKey);
-                }
-                else
-                {
-                    KeyBindings.ReplaceKey (oldKey, _quitKey);
-                }
             }
         }
     }
 
+    private static void ReplaceKey (Key oldKey, Key newKey)
+    {
+        if (KeyBindings.Bindings.Count == 0)
+        {
+            return;
+        }
+
+        if (newKey == Key.Empty)
+        {
+            KeyBindings.Remove (oldKey);
+        }
+        else
+        {
+            KeyBindings.ReplaceKey (oldKey, newKey);
+        }
+    }
+
     /// <summary>
     ///     Event fired when the user presses a key. Fired by <see cref="OnKeyDown"/>.
     ///     <para>
@@ -413,6 +385,13 @@ public static partial class Application // Keyboard handling
 
         KeyBindings.Clear ();
 
+        // Resources/config.json overrrides
+        NextTabKey = Key.Tab;
+        PrevTabKey = Key.Tab.WithShift;
+        NextTabGroupKey = Key.F6;
+        PrevTabGroupKey = Key.F6.WithShift;
+        QuitKey = Key.Esc;
+
         KeyBindings.Add (QuitKey, KeyBindingScope.Application, Command.QuitToplevel);
 
         KeyBindings.Add (Key.CursorRight, KeyBindingScope.Application, Command.NextView);

Terminal.Gui/Application/Application.cs

@@ -142,14 +142,12 @@ public static partial class Application
         UnGrabbedMouse = null;
 
         // Keyboard
-        PrevTabGroupKey = Key.Empty;
-        NextTabGroupKey = Key.Empty;
-        QuitKey = Key.Empty;
         KeyDown = null;
         KeyUp = null;
         SizeChanging = null;
 
         Navigation = null;
+
         AddApplicationKeyBindings ();
 
         Colors.Reset ();

Terminal.Gui/Input/KeyBindings.cs

@@ -136,10 +136,9 @@ public class KeyBindings
             throw new ArgumentException ("Application scoped KeyBindings must be added via Application.KeyBindings.Add");
         }
 
-        if (key is null || !key.IsValid)
+        if (key == Key.Empty || !key.IsValid)
         {
-            //throw new ArgumentException ("Invalid Key", nameof (commands));
-            return;
+            throw new ArgumentException (@"Invalid Key", nameof (commands));
         }
 
         if (commands.Length == 0)
@@ -150,7 +149,6 @@ public class KeyBindings
         if (TryGet (key, out KeyBinding binding))
         {
             throw new InvalidOperationException (@$"A key binding for {key} exists ({binding}).");
-            //Bindings [key] = new (commands, scope, BoundView);
         }
         else
         {
@@ -313,12 +311,17 @@ public class KeyBindings
     /// <summary>Replaces a key combination already bound to a set of <see cref="Command"/>s.</summary>
     /// <remarks></remarks>
     /// <param name="oldKey">The key to be replaced.</param>
-    /// <param name="newKey">The new key to be used.</param>
+    /// <param name="newKey">The new key to be used. If <see cref="Key.Empty"/> no action will be taken.</param>
     public void ReplaceKey (Key oldKey, Key newKey)
     {
         if (!TryGet (oldKey, out KeyBinding _))
         {
-            return;
+            throw new InvalidOperationException ($"Key {oldKey} is not bound.");
+        }
+
+        if (!newKey.IsValid)
+        {
+            throw new InvalidOperationException ($"Key {newKey} is is not valid.");
         }
 
         KeyBinding value = Bindings [oldKey];

Terminal.Gui/Views/Shortcut.cs

@@ -681,7 +681,7 @@ public class Shortcut : View, IOrientation, IDesignable
 
     private void UpdateKeyBinding (Key oldKey)
     {
-        if (Key != null)
+        if (Key != null && Key.IsValid)
         {
             // Disable the command view key bindings
             CommandView.KeyBindings.Remove (Key);

Terminal.Gui/Views/TableView/TableView.cs

@@ -324,11 +324,15 @@ public class TableView : View
         {
             if (cellActivationKey != value)
             {
-                KeyBindings.ReplaceKey (cellActivationKey, value);
+                if (KeyBindings.TryGet (cellActivationKey, out _))
+                {
+                    KeyBindings.ReplaceKey (cellActivationKey, value);
+                }
+                else
+                {
+                    KeyBindings.Add (value, Command.Accept);
+                }
 
-                // of API user is mixing and matching old and new methods of keybinding then they may have lost
-                // the old binding (e.g. with ClearKeybindings) so KeyBindings.Replace alone will fail
-                KeyBindings.Add (value, Command.Accept);
                 cellActivationKey = value;
             }
         }
@@ -792,7 +796,7 @@ public class TableView : View
     }
 
     ///<inheritdoc/>
-    protected internal override bool OnMouseEvent  (MouseEvent me)
+    protected internal override bool OnMouseEvent (MouseEvent me)
     {
         if (!me.Flags.HasFlag (MouseFlags.Button1Clicked)
             && !me.Flags.HasFlag (MouseFlags.Button1DoubleClicked)

UnitTests/Application/ApplicationTests.cs

@@ -183,9 +183,11 @@ public class ApplicationTests
             Assert.Null (Application.Driver);
             Assert.Null (Application.MainLoop);
             Assert.False (Application.EndAfterFirstIteration);
-            Assert.Equal (Key.Empty, Application.PrevTabGroupKey);
-            Assert.Equal (Key.Empty, Application.NextTabGroupKey);
-            Assert.Equal (Key.Empty, Application.QuitKey);
+            Assert.Equal (Key.Tab.WithShift, Application.PrevTabKey);
+            Assert.Equal (Key.Tab, Application.NextTabKey);
+            Assert.Equal (Key.F6.WithShift, Application.PrevTabGroupKey);
+            Assert.Equal (Key.F6, Application.NextTabGroupKey);
+            Assert.Equal (Key.Esc, Application.QuitKey);
             Assert.Null (ApplicationOverlapped.OverlappedChildren);
             Assert.Null (ApplicationOverlapped.OverlappedTop);
 
@@ -236,7 +238,7 @@ public class ApplicationTests
         Application.PrevTabGroupKey = Key.A;
         Application.NextTabGroupKey = Key.B;
         Application.QuitKey = Key.C;
-        Application.KeyBindings.Add (Key.A, KeyBindingScope.Application, Command.Cancel);
+        Application.KeyBindings.Add (Key.D, KeyBindingScope.Application, Command.Cancel);
 
         //ApplicationOverlapped.OverlappedChildren = new List<View> ();
         //ApplicationOverlapped.OverlappedTop = 

UnitTests/Application/KeyboardTests.cs

@@ -65,7 +65,7 @@ public class KeyboardTests
     {
         Application.ResetState (true);
         // Before Init
-        Assert.Equal (Key.Empty, Application.QuitKey);
+        Assert.Equal (Key.Esc, Application.QuitKey);
 
         Application.Init (new FakeDriver ());
         // After Init

UnitTests/Configuration/SettingsScopeTests.cs

@@ -29,9 +29,9 @@ public class SettingsScopeTests
         Settings.Apply ();
 
         // assert
-        Assert.Equal (KeyCode.Q, Application.QuitKey.KeyCode);
-        Assert.Equal (KeyCode.F, Application.NextTabGroupKey.KeyCode);
-        Assert.Equal (KeyCode.B, Application.PrevTabGroupKey.KeyCode);
+        Assert.Equal (Key.Q, Application.QuitKey);
+        Assert.Equal (Key.F, Application.NextTabGroupKey);
+        Assert.Equal (Key.B, Application.PrevTabGroupKey);
     }
 
     [Fact]

UnitTests/Input/KeyBindingTests.cs

@@ -8,11 +8,20 @@ public class KeyBindingTests
     public KeyBindingTests (ITestOutputHelper output) { _output = output; }
 
     [Fact]
-    public void Add_Empty_Throws ()
+    public void Add_No_Commands_Throws ()
     {
         var keyBindings = new KeyBindings ();
         List<Command> commands = new ();
         Assert.Throws<ArgumentException> (() => keyBindings.Add (Key.A, commands.ToArray ()));
+
+    }
+
+    [Fact]
+    public void Add_Invalid_Key_Throws ()
+    {
+        var keyBindings = new KeyBindings ();
+        List<Command> commands = new ();
+        Assert.Throws<ArgumentException> (() => keyBindings.Add (Key.Empty, KeyBindingScope.HotKey, Command.Accept));
     }
 
     [Fact]
@@ -193,7 +202,7 @@ public class KeyBindingTests
     }
 
     [Fact]
-    public void Replace_Key ()
+    public void ReplaceKey_Replaces ()
     {
         var keyBindings = new KeyBindings ();
         keyBindings.Add (Key.A, KeyBindingScope.Application, Command.HotKey);
@@ -218,6 +227,21 @@ public class KeyBindingTests
         Assert.Contains (Command.HotKey, keyBindings.GetCommands (Key.H));
     }
 
+    [Fact]
+    public void ReplaceKey_Throws_If_DoesNotContain_Old ()
+    {
+        var keyBindings = new KeyBindings ();
+        Assert.Throws<InvalidOperationException> (() => keyBindings.ReplaceKey (Key.A, Key.B));
+    }
+
+    [Fact]
+    public void ReplaceKey_Throws_If_New_Is_Empty ()
+    {
+        var keyBindings = new KeyBindings ();
+        keyBindings.Add (Key.A, KeyBindingScope.Application, Command.HotKey);
+        Assert.Throws<InvalidOperationException> (() => keyBindings.ReplaceKey (Key.A, Key.Empty));
+    }
+
     // Add with scope does the right things
     [Theory]
     [InlineData (KeyBindingScope.Focused)]