Fixes #1962 - Change KeyBindings to allow chaining commands (#1966)

* Change KeyBindings to allow chaining commands

* Added more asserts for repeating the keybinding till at bottom of list

* Added tests for 'no command' and multiple commands return type

Terminal.Gui/Core/View.cs

@@ -271,7 +271,7 @@ internal Direction FocusDirection {
 		/// <summary>
 		/// Configurable keybindings supported by the control
 		/// </summary>
-		private Dictionary<Key, Command> KeyBindings { get; set; } = new Dictionary<Key, Command> ();
+		private Dictionary<Key, Command []> KeyBindings { get; set; } = new Dictionary<Key, Command []> ();
 		private Dictionary<Command, Func<bool?>> CommandImplementations { get; set; } = new Dictionary<Command, Func<bool?>> ();
 
 		/// <summary>
@@ -1716,17 +1716,32 @@ public override bool ProcessKey (KeyEvent keyEvent)
 		/// <param name="keyEvent">The key event passed.</param>
 		protected bool? InvokeKeybindings (KeyEvent keyEvent)
 		{
+			bool? toReturn = null;
+
 			if (KeyBindings.ContainsKey (keyEvent.Key)) {
-				var command = KeyBindings [keyEvent.Key];
 
-				if (!CommandImplementations.ContainsKey (command)) {
-					throw new NotSupportedException ($"A KeyBinding was set up for the command {command} ({keyEvent.Key}) but that command is not supported by this View ({GetType ().Name})");
-				}
+				foreach (var command in KeyBindings [keyEvent.Key]) {
+
+					if (!CommandImplementations.ContainsKey (command)) {
+						throw new NotSupportedException ($"A KeyBinding was set up for the command {command} ({keyEvent.Key}) but that command is not supported by this View ({GetType ().Name})");
+					}
+
+					// each command has its own return value
+					var thisReturn = CommandImplementations [command] ();
+
+					// if we haven't got anything yet, the current command result should be used
+					if (toReturn == null) {
+						toReturn = thisReturn;
+					}
 
-				return CommandImplementations [command] ();
+					// if ever see a true then that's what we will return
+					if (thisReturn ?? false) {
+						toReturn = thisReturn.Value;
+					}
+				}
 			}
 
-			return null;
+			return toReturn;
 		}
 
 
@@ -1736,11 +1751,19 @@ public override bool ProcessKey (KeyEvent keyEvent)
 		/// </para>
 		/// <para>If the key is already bound to a different <see cref="Command"/> it will be
 		/// rebound to this one</para>
+		/// <remarks>Commands are only ever applied to the current <see cref="View"/>(i.e. this feature
+		/// cannot be used to switch focus to another view and perform multiple commands there)</remarks>
 		/// </summary>
 		/// <param name="key"></param>
-		/// <param name="command"></param>
-		public void AddKeyBinding (Key key, Command command)
+		/// <param name="command">The command(s) to run on the <see cref="View"/> when <paramref name="key"/> is pressed.
+		/// When specifying multiple, all commands will be applied in sequence.  The bound <paramref name="key"/> strike
+		/// will be consumed if any took effect.</param>
+		public void AddKeyBinding (Key key, params Command [] command)
 		{
+			if (command.Length == 0) {
+				throw new ArgumentException ("At least one command must be specified", nameof (command));
+			}
+
 			if (KeyBindings.ContainsKey (key)) {
 				KeyBindings [key] = command;
 			} else {
@@ -1756,7 +1779,7 @@ public void AddKeyBinding (Key key, Command command)
 		protected void ReplaceKeyBinding (Key fromKey, Key toKey)
 		{
 			if (KeyBindings.ContainsKey (fromKey)) {
-				Command value = KeyBindings [fromKey];
+				var value = KeyBindings [fromKey];
 				KeyBindings.Remove (fromKey);
 				KeyBindings [toKey] = value;
 			}
@@ -1795,9 +1818,9 @@ public void ClearKeybinding (Key key)
 		/// keys bound to the same command and this method will clear all of them.
 		/// </summary>
 		/// <param name="command"></param>
-		public void ClearKeybinding (Command command)
+		public void ClearKeybinding (params Command [] command)
 		{
-			foreach (var kvp in KeyBindings.Where (kvp => kvp.Value == command).ToArray ()) {
+			foreach (var kvp in KeyBindings.Where (kvp => kvp.Value.SequenceEqual (command))) {
 				KeyBindings.Remove (kvp.Key);
 			}
 		}
@@ -1837,9 +1860,9 @@ public IEnumerable<Command> GetSupportedCommands ()
 		/// </summary>
 		/// <param name="command">The command to search.</param>
 		/// <returns>The <see cref="Key"/> used by a <see cref="Command"/></returns>
-		public Key GetKeyFromCommand (Command command)
+		public Key GetKeyFromCommand (params Command [] command)
 		{
-			return KeyBindings.First (x => x.Value == command).Key;
+			return KeyBindings.First (x => x.Value.SequenceEqual (command)).Key;
 		}
 
 		/// <inheritdoc/>

UnitTests/ListViewTests.cs

@@ -30,6 +30,97 @@ public void Constructors_Defaults ()
 			Assert.Equal (new Rect (0, 1, 10, 20), lv.Frame);
 		}
 
+		[Fact]
+		public void ListViewSelectThenDown ()
+		{
+			var lv = new ListView (new List<string> () { "One", "Two", "Three" });
+			lv.AllowsMarking = true;
+
+			Assert.NotNull (lv.Source);
+
+			// first item should be selected by default
+			Assert.Equal (0, lv.SelectedItem);
+
+			// nothing is ticked
+			Assert.False (lv.Source.IsMarked (0));
+			Assert.False (lv.Source.IsMarked (1));
+			Assert.False (lv.Source.IsMarked (2));
+
+			lv.AddKeyBinding (Key.Space | Key.ShiftMask, Command.ToggleChecked, Command.LineDown);
+
+			var ev = new KeyEvent (Key.Space | Key.ShiftMask, new KeyModifiers () { Shift = true });
+
+			// view should indicate that it has accepted and consumed the event
+			Assert.True (lv.ProcessKey (ev));
+
+			// second item should now be selected
+			Assert.Equal (1, lv.SelectedItem);
+
+			// first item only should be ticked
+			Assert.True (lv.Source.IsMarked (0));
+			Assert.False (lv.Source.IsMarked (1));
+			Assert.False (lv.Source.IsMarked (2));
+
+			// Press key combo again
+			Assert.True (lv.ProcessKey (ev));
+			Assert.Equal (2, lv.SelectedItem);
+			Assert.True (lv.Source.IsMarked (0));
+			Assert.True (lv.Source.IsMarked (1));
+			Assert.False (lv.Source.IsMarked (2));
+
+			// Press key combo again
+			Assert.True (lv.ProcessKey (ev));
+			Assert.Equal (2, lv.SelectedItem); // cannot move down any further
+			Assert.True (lv.Source.IsMarked (0));
+			Assert.True (lv.Source.IsMarked (1));
+			Assert.True (lv.Source.IsMarked (2)); // but can toggle marked
+
+			// Press key combo again 
+			Assert.True (lv.ProcessKey (ev));
+			Assert.Equal (2, lv.SelectedItem); // cannot move down any further
+			Assert.True (lv.Source.IsMarked (0));
+			Assert.True (lv.Source.IsMarked (1));
+			Assert.False (lv.Source.IsMarked (2)); // untoggle toggle marked
+		}
+		[Fact]
+		public void SettingEmptyKeybindingThrows ()
+		{
+			var lv = new ListView (new List<string> () { "One", "Two", "Three" });
+			Assert.Throws<ArgumentException> (() => lv.AddKeyBinding (Key.Space));
+		}
+
+
+		/// <summary>
+		/// Tests that when none of the Commands in a chained keybinding are possible
+		/// the <see cref="View.ProcessKey(KeyEvent)"/> returns the appropriate result
+		/// </summary>
+		[Fact]
+		public void ListViewProcessKeyReturnValue_WithMultipleCommands ()
+		{
+			var lv = new ListView (new List<string> () { "One", "Two", "Three", "Four" });
+
+			Assert.NotNull (lv.Source);
+
+			// first item should be selected by default
+			Assert.Equal (0, lv.SelectedItem);
+
+			// bind shift down to move down twice in control
+			lv.AddKeyBinding (Key.CursorDown | Key.ShiftMask, Command.LineDown, Command.LineDown);
+
+			var ev = new KeyEvent (Key.CursorDown | Key.ShiftMask, new KeyModifiers () { Shift = true });
+
+			Assert.True (lv.ProcessKey (ev), "The first time we move down 2 it should be possible");
+
+			// After moving down twice from One we should be at 'Three'
+			Assert.Equal (2, lv.SelectedItem);
+
+			// clear the items
+			lv.SetSource (null);
+
+			// Press key combo again - return should be false this time as none of the Commands are allowable
+			Assert.False (lv.ProcessKey (ev), "We cannot move down so will not respond to this");
+		}
+
 		private class NewListDataSource : IListDataSource {
 			public int Count => throw new NotImplementedException ();