Enable Views to declare key bindings for their SuperView #3042

tig · 2023-12-10T22:54:32Z

In v1 and current v2, MenuBar and StatusBar (effectively) provide global key bindings. It worked in a hacky way where both classes were tightly coupled to Toplevel (and didn't actually use key bindings).

In #2927 these classes now are based on Command/KeyBindings. However, in order for it to work, I have the following hack:

public virtual bool? OnInvokeKeyBindings (KeyEventArgs keyEvent)
{
	// fire event
	InvokingKeyBindings?.Invoke (this, keyEvent);
	if (keyEvent.Handled) {
		return true;
	}

	// * If no key binding was found, `InvokeKeyBindings` returns `null`.
	//   Continue passing the event (return `false` from `OnInvokeKeyBindings`).
	// * If key bindings were found, but none handled the key (all `Command`s returned `false`),
	//   `InvokeKeyBindings` returns `false`. Continue passing the event (return `false` from `OnInvokeKeyBindings`)..
	// * If key bindings were found, and any handled the key (at least one `Command` returned `true`),
	//   `InvokeKeyBindings` returns `true`. Continue passing the event (return `false` from `OnInvokeKeyBindings`).
	var handled = InvokeKeyBindings (keyEvent);
	if (handled != null && (bool)handled) {
		return true;
	}

	// TODO: Refactor this per https://github.com/gui-cs/Terminal.Gui/pull/2927#discussion_r1420415162
	// TODO: The problem that needs to be solved is:
	// TODO: - How to enable a view to define global key bindings like StatusBar and MenuBar?
	foreach (var view in Subviews.Where (v => v is StatusBar or MenuBar)) {
		handled = view.OnInvokeKeyBindings (keyEvent);
		if (handled != null && (bool)handled) {
			return true;
		}
	}
	return handled;
}

We need a better mechanism for classes to say "I'd like to provide some key bindings that get fired regardless of whether I have focus or not."

My best current idea for this is:

Add a public virtual Dictionary<Key, Command []> GetGlobalKeyBindings() method to View.
Change View.Add() to call this method and add the returned bindings to the view's key bindings.
Change View.Delete() to remove any key bindings added thusly.

Anyway, for now, my hack above seems to work and can let me finish #2927 without solving this problem "correctly".

The text was updated successfully, but these errors were encountered:

BDisp · 2023-12-11T12:04:26Z

@tig I think you didn't already understood why this is wrong:

	if (handled != null && (bool)handled) {
		return true;
	}

When returned from InvokeKeyBindings and handled != null you must return true or false and exit. Only if handled == null resume the method. So this have to be as the follow:

	if (handled != null) {
		return handled; // don't need to cast here because the compiler already know it's a bool
	}

Same here:

	foreach (var view in Subviews.Where (v => v is StatusBar or MenuBar)) {
		handled = view.OnInvokeKeyBindings (keyEvent);
		if (handled != null) {
			return handled;
		}
	}

tig · 2023-12-11T17:36:42Z

handled != null &&

I do think I now understand. However, I think you and I may disagree on the correct behavior.

You seem to think:

Keypress event handling should stop if any key binding was FOUND, even if none of the command handlers return true.

I assert:

Keypress event handling should stop if any key binding results in a command handler returns true. Otherwise, it should continue.

If we do as you suggest, then we will be ignoring what command handlers return.

Do you have a specific user-use case where you think what I'm proposing will not be correct?

BDisp · 2023-12-11T17:49:00Z

If we do as you suggest, then we will be ignoring what command handlers return.

Do you have a specific user-use case where you think what I'm proposing will not be correct?

That easy. For e.g. a TextView receive a key binding has Key.Tab which normally is used by the Toplevel navigation. When AllowsTabs is true then continue processing it and return true. But if AllowsTabs is false then it will return false for the superview take the correct action which will be Toplevel navigate to the next view. That's enough?

tig · 2023-12-11T19:32:59Z

AllowsTabs

Please run the latest in #2927 and I think you'll see the code now behaves properly with my latest refactor.

BDisp · 2023-12-11T19:41:25Z

I only be trying to explain why the OnInvokeKeyBindings must return and exit no matter is true or false and only continue if it's null. It's really working because the code after this return false. But I'm not waste more time discussing this because I'm being tired.

tznind · 2023-12-11T19:41:30Z

Currently the return values are

Return	Meaning
null	No keybinding exists
true	Break flow of control because key is bound to a command that executed successfully
false	Break flow of control becausekey is bound to a command even though that command was not possible to complete

The case is made that null and false should in fact be treated the same.

Let us consider an example case where behaviour would differ.

TextView has a key binding for ProcessReturn.
If user binds 'A' to ProcessReturn
User sets AllowsReturn to false on the TextView
User focus the TextView and presses A

In the previous behaviour nothing would happen. In the new behaviour the letter A would appear in the control. This is because the ProcessReturn determines that no carriage return can be added therefore we drop through to the rest of the behaviour of the TextView.

I can see arguments for both behaviours.

Initially when I created this feature it was just bool, bool? came later as we worked on particular views. But it is possible that with more thought we can avoid this difficulty.

BDisp · 2023-12-11T22:43:33Z

The case is made that null and false should in fact be treated the same.

I agree with this and the current @tig implementation is correct. Sorry for that. So, the difference from null and false is the first indicates no key binding exist, which it's useful information, but both as the same behavior afterwards. Thanks.

tig · 2023-12-11T23:56:56Z

Now back to our regularly scheduled episode:

We need a better mechanism for classes to say "I'd like to provide some key bindings that get fired regardless of whether I have focus or not."

Current code in #2927:

/// <summary>
/// Low-level API called when a user presses a key; invokes any key bindings set on the view.
/// This is called during <see cref="ProcessKeyDown"/> after <see cref="OnKeyDown"/> has returned.
/// </summary>
/// <remarks>
/// <para>
/// Fires the <see cref="InvokingKeyBindings"/> event.
/// </para>
/// <para>
/// See <see href="../docs/keyboard.md">for an overview of Terminal.Gui keyboard APIs.</see>
/// </para>
/// </remarks>
/// <param name="keyEvent">Contains the details about the key that produced the event.</param>
/// <returns><see langword="false"/> if the key press was not handled. <see langword="true"/> if
/// the keypress was handled and no other view should see it.</returns>
public virtual bool? OnInvokingKeyBindings (KeyEventArgs keyEvent)
{
	// fire event
	InvokingKeyBindings?.Invoke (this, keyEvent);
	if (keyEvent.Handled) {
		return true;
	}

	// * If no key binding was found, `InvokeKeyBindings` returns `null`.
	//   Continue passing the event (return `false` from `OnInvokeKeyBindings`).
	// * If key bindings were found, but none handled the key (all `Command`s returned `false`),
	//   `InvokeKeyBindings` returns `false`. Continue passing the event (return `false` from `OnInvokeKeyBindings`)..
	// * If key bindings were found, and any handled the key (at least one `Command` returned `true`),
	//   `InvokeKeyBindings` returns `true`. Continue passing the event (return `false` from `OnInvokeKeyBindings`).
	var handled = InvokeKeyBindings (keyEvent);
	if (handled != null && (bool)handled) {
		// Stop processing if any key binding handled the key.
		// DO NOT stop processing if there are no matching key bindings or none of the key bindings handled the key
		return true;
	}

	// TODO: Refactor this per https://github.com/gui-cs/Terminal.Gui/pull/2927#discussion_r1420415162
	// TODO: The problem that needs to be solved is:
	// TODO: - How to enable a view to define global key bindings like StatusBar and MenuBar?
	// see https://github.com/gui-cs/Terminal.Gui/issues/3042
	foreach (var view in Subviews.Where (v => v is StatusBar or MenuBar or RadioGroup || keyEvent.BareKey == v.HotKey)) {
		handled = view.OnInvokingKeyBindings (keyEvent);
		if (handled != null && (bool)handled) {
			return true;
		}
	}
	return handled;
}

Related code for HotKey handling:

/// <summary>
/// Gets or sets the hot key defined for this view. Pressing the hot key on the keyboard while this view has
/// focus will invoke the <see cref="Command.Default"/> command, which, by default, causes the view to be
/// focused.
/// By default, the HotKey is automatically set to the first
/// character of <see cref="Text"/> that is prefixed with with <see cref="HotKeySpecifier"/>.
/// <para>
/// A HotKey is a keypress that selects a visible UI item. For selecting items across <see cref="View"/>`s
/// (e.g.a <see cref="Button"/> in a <see cref="Dialog"/>) the keypress must include the <see cref="Key.AltMask"/> modifier.
/// For selecting items within a View that are not Views themselves, the keypress can be key without the Alt modifier.
/// For example, in a Dialog, a Button with the text of "_Text" can be selected with Alt-T.
/// Or, in a <see cref="Menu"/> with "_File _Edit", Alt-F will select (show) the "_File" menu.
/// If the "_File" menu has a sub-menu of "_New" `Alt-N` or `N` will ONLY select the "_New" sub-menu if the "_File" menu is already opened.
/// </para>
/// </summary>
/// <remarks>
/// <para>
/// See <see href="../docs/keyboard.md"/> for an overview of Terminal.Gui keyboard APIs.
/// </para>
/// <para>
/// This is a helper API for configuring a key binding for the hot key. By default, this property is set whenever <see cref="Text"/> changes.
/// </para>
/// <para>
/// By default, when the Hot Key is set, key bindings are added for both the base key (e.g. <see cref="Key.D3"/>) and
/// the Alt-shifted key (e.g. <see cref="Key.D3"/> | <see cref="Key.AltMask"/>).
/// This behavior can be overriden by overriding <see cref="AddKeyBindingsForHotKey"/>.
/// </para>
/// <para>
/// By default, when the HotKey is set to <see cref="Key.A"/> through <see cref="Key.Z"/> key bindings will be added for both the un-shifted and shifted
/// versions. This means if the HotKey is <see cref="Key.A"/>, key bindings for <see cref="Key.A"/> and <see cref="Key.A"/> | <see cref="Key.ShiftMask"/>
/// will be added. This behavior can be overriden by overriding <see cref="AddKeyBindingsForHotKey"/>.
/// </para>
/// <para>
/// If the hot key is changed, the <see cref="HotKeyChanged"/> event is fired.
/// </para>
/// </remarks>
public virtual Key HotKey {
	get => _hotKey;
	set {
		if (AddKeyBindingsForHotKey (_hotKey, value)) {
			// This will cause TextFormatter_HotKeyChanged to be called, firing HotKeyChanged
			_hotKey = TextFormatter.HotKey = value;
		}
	}
}

/// <summary>
/// Adds key bindings for the specified HotKey. Useful for views that contain multiple items that each have their own HotKey
/// such as <see cref="RadioGroup"/>.
/// </summary>
/// <remarks>
/// <para>
/// By default key bindings are added for both the base key (e.g. <see cref="Key.D3"/>) and
/// the Alt-shifted key (e.g. <see cref="Key.D3"/> | <see cref="Key.AltMask"/>).
/// This behavior can be overriden by overriding <see cref="AddKeyBindingsForHotKey"/>.
/// </para>
/// <para>
/// By default, when <paramref name="hotKey"/> is <see cref="Key.A"/> through <see cref="Key.Z"/> key bindings will be added for both the un-shifted and shifted
/// versions. This means if the HotKey is <see cref="Key.A"/>, key bindings for <see cref="Key.A"/> and <see cref="Key.A"/> | <see cref="Key.ShiftMask"/>
/// will be added. This behavior can be overriden by overriding <see cref="AddKeyBindingsForHotKey"/>.
/// </para>
/// </remarks>
/// <param name="prevHotKey">The HotKey <paramref name="hotKey"/> is replacing. Key bindings for this key will be removed.</param>
/// <param name="hotKey">The new HotKey. If <see cre="Key.Null"/> <paramref name="prevHotKey"/> bindings will be removed.</param>
/// <returns><see langword="true"/> if the HotKey bindings were added.</returns>
/// <exception cref="ArgumentException"></exception>
public virtual bool AddKeyBindingsForHotKey (Key prevHotKey, Key hotKey)
{
	if (_hotKey == hotKey) {
		return false;
	}

	var newKey = hotKey == Key.Unknown ? Key.Null : hotKey;

	var baseKey = newKey & ~Key.CtrlMask & ~Key.AltMask & ~Key.ShiftMask;
	if (newKey != Key.Null && (baseKey == Key.Space || Rune.IsControl (KeyEventArgs.ToRune (baseKey)))) {
		throw new ArgumentException (@$"HotKey must be a printable (and non-space) key ({hotKey}).");
	}

	if (newKey != baseKey) {
		if ((newKey & Key.CtrlMask) != 0) {
			throw new ArgumentException (@$"HotKey does not support CtrlMask ({hotKey}).");
		}
		// Strip off the shift mask if it's A...Z
		if (baseKey is >= Key.A and <= Key.Z && (newKey & Key.ShiftMask) != 0) {
			newKey &= ~Key.ShiftMask;
		}
		// Strip off the Alt mask
		newKey &= ~Key.AltMask;
	}

	// Remove base version
	if (TryGetKeyBinding (prevHotKey, out _)) {
		ClearKeyBinding (prevHotKey);
	}

	// Remove the Alt version
	if (TryGetKeyBinding (prevHotKey | Key.AltMask, out _)) {
		ClearKeyBinding (prevHotKey | Key.AltMask);
	}

	if (_hotKey is >= Key.A and <= Key.Z) {
		// Remove the shift version
		if (TryGetKeyBinding (prevHotKey | Key.ShiftMask, out _)) {
			ClearKeyBinding (prevHotKey | Key.ShiftMask);
		}
		// Remove alt | shift version
		if (TryGetKeyBinding (prevHotKey | Key.ShiftMask | Key.AltMask, out _)) {
			ClearKeyBinding (prevHotKey | Key.ShiftMask | Key.AltMask);
		}
	}

	// Add the new 
	if (newKey != Key.Null) {
		// Add the base and Alt key
		AddKeyBinding (newKey, Command.Accept);
		AddKeyBinding (newKey | Key.AltMask, Command.Accept);

		// If the Key is A..Z, add ShiftMask and AltMask | ShiftMask
		if (newKey is >= Key.A and <= Key.Z) {
			AddKeyBinding (newKey | Key.ShiftMask, Command.Accept);
			AddKeyBinding (newKey | Key.ShiftMask | Key.AltMask, Command.Accept);
		}

		AddKeyBinding (newKey, Command.Accept);
	}
	return true;
}

Which is used by RadioGroup to support hot keys for the radio items:

/// <summary>
/// The radio labels to display. A key binding will be added for each radio radio enabling the user
/// to select and/or focus the radio label using the keyboard. See <see cref="View.HotKey"/> for details
/// on how HotKeys work.
/// </summary>
/// <value>The radio labels.</value>
public string [] RadioLabels {
	get => _radioLabels.ToArray ();
	set {
		// Remove old hot key bindings
		foreach (var label in _radioLabels) {
			if (TextFormatter.FindHotKey (label, HotKeySpecifier, true, out _, out Key hotKey)) {
				AddKeyBindingsForHotKey (hotKey, Key.Null);
			}
		}
		var prevCount = _radioLabels.Count;
		_radioLabels = value.ToList ();
		foreach (var label in _radioLabels) {
			if (TextFormatter.FindHotKey (label, HotKeySpecifier, true, out _, out Key hotKey)) {
				AddKeyBindingsForHotKey (Key.Null, hotKey);
			}
		}
		if (prevCount != _radioLabels.Count) {
			SetWidthHeight (_radioLabels);
		}
		SelectedItem = 0;
		_cursor = 0;
		SetNeedsDisplay ();
	}
}

/// <inheritdoc/>
public override bool? OnInvokingKeyBindings (KeyEventArgs keyEvent)
{
	// This is a bit of a hack. We want to handle the key bindings for the radio group but
	// InvokeKeyBindings doesn't pass any context so we can't tell if the key binding is for
	// the radio group or for one of the radio buttons. So before we call the base class
	// we set SelectedItem appropriately.

	// Force upper case
	var key = keyEvent.Key;
	if (TryGetKeyBinding (key, out _)) {
		// Search RadioLabels 
		for (int i = 0; i < _radioLabels.Count; i++) {
			if (TextFormatter.FindHotKey (_radioLabels [i], HotKeySpecifier, true, out _, out Key hotKey) 
				&& (key & ~Key.ShiftMask & ~Key.AltMask) == hotKey) {
				SelectedItem = i;
				break;
			}
		}

	}
	return base.OnInvokingKeyBindings (keyEvent);
}

tig · 2023-12-12T00:10:40Z

Some terms to help us navigate this:

SuperView Scope Key Binding - Applies to a single View and it's SubViews. If the view itself, or any of the views SubViews handle the key, any View Scope Key Bindings will be invoked.
Global Key Binding == Application Scope Key Binding - Applies to Application-scope. If no view that has been added to a Toplevel handles the key, any Application Scope Key Bindings will be invoked.

The above code sort of implements SuperView Scope Key Bindings.

I can't think of any Application Scope Key Binding scenario that could not be served with just SuperView Scoped.

For this reason, I suggest we ignore that for now.

(Remember, for v2 we will enable MenuBar and StatusBar in ANY view that contains LayoutStyle.Overlapped ... details TBD).

tig · 2023-12-12T00:34:00Z

SuperView Scope Key Binding Rules

Not all key bindings on a view should be run at SuperView Scope.

For example, MenuBar has

AddKeyBinding (Key.CursorLeft, Command.Left) for menu nav - SHOULD NOT run as a "SuperView Scoped Key Binding"
AddKeyBinding (menu.Shortcut, Command.Default) for shortcuts. - SHOULD run as a "SuperView Scoped Key Binding"

The implication of this is we need a way of distinguishing key bindings held in View.KeyBindings, currently defined as:

private Dictionary<Key, Command []> KeyBindings { get; set; } = new Dictionary<Key, Command []> ();

One idea would be to change the Command enum to have a SuperViewScoped (and eventually AppScoped) flag.

 `AddKeyBinding (menu.Shortcut, Command.Default | Command.SuperViewScoped)`

I dispise flags on enum. So I vote no on this idea.

Another idea would be to make KeyBindings be a dictionary where the values were a tuple, like this:

	public enum CommandScope {
		/// <summary>
		/// The command is scoped to just the view that adds the key binding.
		/// </summary>
		View = 0,

		/// <summary>
		/// The command is scoped to the view that adds the key binding and that view's <see cref="View.SuperView"/>.
		/// </summary>
		SuperView,

		/// <summary>
		/// The command is scoped to the view that adds the key binding and <see cref="Application"/>.
		/// </summary>
		App
	}
...
/// <summary>
/// Gets the key bindings for this view.
/// </summary>
private Dictionary<Key, (Command [] commands, CommandScope scope)> KeyBindings { get; set; } = new Dictionary<Key, (Command [], CommandScope)> ();

I like this.

Thoughs?

BDisp · 2023-12-12T00:42:15Z

You mean that the View is equivalent to the old ProcessKey, SuperView to the old ProcessHotKey and App to the old ProcessColdKey. If it's really that, I like it too.

tig · 2023-12-12T00:51:23Z

You mean that the View is equivalent to the old ProcessKey, SuperView to the old ProcessHotKey and App to the old ProcessColdKey. If it's really that, I like it too.

I think so.

tig · 2023-12-12T01:24:25Z

Check out the latest in #2927 - I just implemented the CommandScope.SuperView concept for MenuBar.

It's inefficient, but works!

public virtual bool? OnInvokingKeyBindings (KeyEventArgs keyEvent)
{
	// fire event
	InvokingKeyBindings?.Invoke (this, keyEvent);
	if (keyEvent.Handled) {
		return true;
	}

	// * If no key binding was found, `InvokeKeyBindings` returns `null`.
	//   Continue passing the event (return `false` from `OnInvokeKeyBindings`).
	// * If key bindings were found, but none handled the key (all `Command`s returned `false`),
	//   `InvokeKeyBindings` returns `false`. Continue passing the event (return `false` from `OnInvokeKeyBindings`)..
	// * If key bindings were found, and any handled the key (at least one `Command` returned `true`),
	//   `InvokeKeyBindings` returns `true`. Continue passing the event (return `false` from `OnInvokeKeyBindings`).
	var handled = InvokeKeyBindings (keyEvent);
	if (handled != null && (bool)handled) {
		// Stop processing if any key binding handled the key.
		// DO NOT stop processing if there are no matching key bindings or none of the key bindings handled the key
		return true;
	}

	// TODO: Refactor this per https://github.com/gui-cs/Terminal.Gui/pull/2927#discussion_r1420415162
	// TODO: The problem that needs to be solved is:
	// TODO: - How to enable a view to define global key bindings like StatusBar and MenuBar?
	// see https://github.com/gui-cs/Terminal.Gui/issues/3042
	foreach (var view in Subviews.Where (v => v is StatusBar or RadioGroup || keyEvent.BareKey == v.HotKey)) {
		handled = view.OnInvokingKeyBindings (keyEvent);
		if (handled != null && (bool)handled) {
			return true;
		}
	}

	foreach (var view in Subviews.Where (v => v is MenuBar || keyEvent.BareKey == v.HotKey)) {
		var binding = view.GetKeyBindingWithScope (keyEvent.Key);
		if (binding.commands.Length > 0 && binding.scope == CommandScope.SuperView) {
			handled = view.OnInvokingKeyBindings (keyEvent);
			if (handled != null && (bool)handled) {
				return true;
			}
		}
	}

	return handled;
}

tig added design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...) v2 For discussions, issues, etc... relavant for v2 labels Dec 10, 2023

tig added this to Terminal.Gui V2 Beta Dec 10, 2023

tig mentioned this issue Dec 11, 2023

Fixes #2926 - Refactor KeyEvent and KeyEventEventArgs to simplify #2927

Merged

30 tasks

tig mentioned this issue Dec 12, 2023

Should StatusBar always win and capture any defined hotkeys? #1259

Closed

tig closed this as completed Dec 17, 2023

github-project-automation bot moved this to ✅ Done in Terminal.Gui V2 Beta Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Views to declare key bindings for their SuperView #3042

Enable Views to declare key bindings for their SuperView #3042

tig commented Dec 10, 2023

BDisp commented Dec 11, 2023 •

edited

Loading

tig commented Dec 11, 2023

BDisp commented Dec 11, 2023 •

edited

Loading

tig commented Dec 11, 2023

BDisp commented Dec 11, 2023

tznind commented Dec 11, 2023 •

edited

Loading

BDisp commented Dec 11, 2023

tig commented Dec 11, 2023

tig commented Dec 12, 2023 •

edited

Loading

tig commented Dec 12, 2023 •

edited

Loading

BDisp commented Dec 12, 2023

tig commented Dec 12, 2023

tig commented Dec 12, 2023

Enable Views to declare key bindings for their SuperView #3042

Enable Views to declare key bindings for their SuperView #3042

Comments

tig commented Dec 10, 2023

BDisp commented Dec 11, 2023 • edited Loading

tig commented Dec 11, 2023

BDisp commented Dec 11, 2023 • edited Loading

tig commented Dec 11, 2023

BDisp commented Dec 11, 2023

tznind commented Dec 11, 2023 • edited Loading

BDisp commented Dec 11, 2023

tig commented Dec 11, 2023

tig commented Dec 12, 2023 • edited Loading

tig commented Dec 12, 2023 • edited Loading

SuperView Scope Key Binding Rules

Not all key bindings on a view should be run at SuperView Scope.

BDisp commented Dec 12, 2023

tig commented Dec 12, 2023

tig commented Dec 12, 2023

BDisp commented Dec 11, 2023 •

edited

Loading

BDisp commented Dec 11, 2023 •

edited

Loading

tznind commented Dec 11, 2023 •

edited

Loading

tig commented Dec 12, 2023 •

edited

Loading

tig commented Dec 12, 2023 •

edited

Loading