Why does InvokeKeyBindings return a bool? and not just bool

[tig]

I don't understand why InvokeKeyBindings doesn't just return a bool

	protected bool? InvokeKeyBindings (KeyEventArgs keyEvent)
	{
		bool? toReturn = null;

		if (KeyBindings.ContainsKey (keyEvent.Key)) {

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

				// if ever see a true then that's what we will return
				if (thisReturn ?? false) {
					toReturn = true;
				}
			}
		}

		return toReturn;
	}

@tznind & @BDisp, what am I missing? It seems needlessly complex.

Most calling code looks like this:

/// <inheritdoc/>
public override bool OnKeyPressed (KeyEventArgs keyEvent)
{
	if (HasFocus && CanFocus) {
		var result = InvokeKeyBindings (keyEvent);
		if (result != null)
			return (bool)result;
	}

	return base.OnKeyPressed (keyEvent);
}

But in a few cases (e.g. TableView) we do this:

var result = InvokeKeyBindings (keyEvent);
if (result != null) {
	PositionCursor ();
	return true;
}

I don't understand this logic!

[tig]

Writing this helped ;-).

The docs should read:

		/// <summary>
		/// Invokes all <see cref="Command"/>s that are bound to <paramref name="keyEvent"/> in <see cref="KeyBindings"/>.
		/// </summary>
		/// <param name="keyEvent">The key event passed.</param>
		/// <returns><see langword="null"/> if no matching command was found in <see cref="KeyBindings"/>. Otherwise, <see langword="true"/> if any invoked command indicated
		/// the key was handled; <see langword="false"/> otherwise.</returns>

I still don't understand the TableView logic though as all bindings return true:

			AddCommand (Command.Right, () => { ChangeSelectionByOffset (1, 0, false); return true; });
			AddCommand (Command.Left, () => { ChangeSelectionByOffset (-1, 0, false); return true; });
			AddCommand (Command.LineUp, () => { ChangeSelectionByOffset (0, -1, false); return true; });
			AddCommand (Command.LineDown, () => { ChangeSelectionByOffset (0, 1, false); return true; });
			AddCommand (Command.PageUp, () => { PageUp (false); return true; });
			AddCommand (Command.PageDown, () => { PageDown (false); return true; });
			AddCommand (Command.LeftHome, () => { ChangeSelectionToStartOfRow (false); return true; });
			AddCommand (Command.RightEnd, () => { ChangeSelectionToEndOfRow (false); return true; });
			AddCommand (Command.TopHome, () => { ChangeSelectionToStartOfTable (false); return true; });
			AddCommand (Command.BottomEnd, () => { ChangeSelectionToEndOfTable (false); return true; });

			AddCommand (Command.RightExtend, () => { ChangeSelectionByOffset (1, 0, true); return true; });
			AddCommand (Command.LeftExtend, () => { ChangeSelectionByOffset (-1, 0, true); return true; });
			AddCommand (Command.LineUpExtend, () => { ChangeSelectionByOffset (0, -1, true); return true; });
			AddCommand (Command.LineDownExtend, () => { ChangeSelectionByOffset (0, 1, true); return true; });
			AddCommand (Command.PageUpExtend, () => { PageUp (true); return true; });
			AddCommand (Command.PageDownExtend, () => { PageDown (true); return true; });
			AddCommand (Command.LeftHomeExtend, () => { ChangeSelectionToStartOfRow (true); return true; });
			AddCommand (Command.RightEndExtend, () => { ChangeSelectionToEndOfRow (true); return true; });
			AddCommand (Command.TopHomeExtend, () => { ChangeSelectionToStartOfTable (true); return true; });
			AddCommand (Command.BottomEndExtend, () => { ChangeSelectionToEndOfTable (true); return true; });

			AddCommand (Command.SelectAll, () => { SelectAll (); return true; });
			AddCommand (Command.Accept, () => { OnCellActivated (new CellActivatedEventArgs (Table, SelectedColumn, SelectedRow)); return true; });

			AddCommand (Command.ToggleChecked, () => { ToggleCurrentCellSelection (); return true; });

I believe the TableView code can be changed to, and it would be more correct:

public override bool OnKeyPressed (KeyEventArgs keyEvent)
{
	if (base.OnKeyPressed (keyEvent)) {
		return true;
	}
	if (TableIsNullOrInvisible ()) {
		PositionCursor ();
		return false;
	}

	var result = InvokeKeyBindings (keyEvent);
	if (result ?? true) {
		PositionCursor ();
		return true;
	}

	if (CollectionNavigator != null &&
		this.HasFocus &&
		Table.Rows != 0 &&
		Terminal.Gui.CollectionNavigator.IsCompatibleKey (keyEvent) &&
		!keyEvent.Key.HasFlag (Key.CtrlMask) &&
		!keyEvent.Key.HasFlag (Key.AltMask) &&
		char.IsLetterOrDigit ((char)keyEvent.KeyValue)) {
		return CycleToNextTableEntryBeginningWith (keyEvent);
	}

	return false;
}

RIght?

[BDisp]

The reason of the bool? was to differentiate from true or false when the keybinding was really handled. If it wasn't handled (not binding) then it return null. Me and @tznind come to this concluding for to have sure what action needed to do. The documents need to be changed to add the null explanation.

[BDisp]

/// <inheritdoc/>
public override bool OnKeyPressed (KeyEventArgs keyEvent)
{
	if (HasFocus && CanFocus) {
		var result = InvokeKeyBindings (keyEvent);
		if (result != null)
			return (bool)result;
	}

	return base.OnKeyPressed (keyEvent);
}

This explain what I said before. If result isn't null then returns true or false. But if it's null it not return and continuous executing the code after it, the base or the other code. See the TextField or TextView.

[BDisp]

I still don't understand the TableView logic though as all bindings return true:

If all them return true and return your solution is right for this particular case.

[tig]

Got it. All good.

I'll ensure the docs are correct via #2926

[tznind]

	var result = InvokeKeybindings (keyEvent);
	if (result != null) {
		PositionCursor ();
		return true;
	}

I think this code is about preventing re-entry into the key handling logic e.g. via a base class call. It is probably just defensive coding. It is saying 'if keybinding existed for this keystroke swallow the event'. Could also be a precaution against going into key navigator when theres a keybinding existing.

I believe the TableView code can be changed to, and it would be more correct:

I think your proposed new code if (result ?? true) might break collection navigation. If there is a null return from keybindings (e.g. if user presses the letter 'd') then you still want to pass that on to the collection navigator and consider whether to CycleToNextTableEntryBeginningWith.

[tig]

Next, related question:

Keys, ConsoleKeys, and KeyModifiers is muy confused when it comes to Shift.

Specifically:

• BackTab is special cased (in our library I assert it should not be defined in Key at all and should be handled as a Command bound to Key.Tab | Key.Shift).
• Key has members for both lower and upper case alphas. I believe in the olden days it just had lower (but as Key.A). @BDisp, I think, added Key.a...z. I think he agrees this was an error.

I'd like to simplify this. Any objections or concerns?

[BDisp]

Next, related question:

Keys, ConsoleKeys, and KeyModifiers is muy confused when it comes to Shift.

Specifically:

• BackTab is special cased (in our library I assert it should not be defined in Key at all and should be handled as a Command bound to Key.Tab | Key.Shift).

Specially on Linux the BackTab is specific. Test on both Windows and Linux, before.

• Key has members for both lower and upper case alphas. I believe in the olden days it just had lower (but as Key.A). @BDisp, I think, added Key.a...z. I think he agrees this was an error.

Yes, I really regret implementing it. You can remove them without any problems.

I'd like to simplify this. Any objections or concerns?

For me, go for it. Thanks.