OutConsoleGridView no longer works - ENTER does not work

[tig]

Recent changes to keyboard handling has broken ocgv.

https://github.com/PowerShell/GraphicalTools/blob/master/docs/Microsoft.PowerShell.ConsoleGuiTools/Out-ConsoleGridView.md

It implements StatusBar like this:

private void AddStatusBar()
        {
            var statusBar = new StatusBar(
                    _applicationData.OutputMode != OutputModeOption.None
                    ? new StatusItem[]
                    {
                        // Use Key.Unknown for SPACE with no delegate because ListView already
                        // handles SPACE
                        new StatusItem(Key.Unknown, "~SPACE~ Mark Item", null),
                        new StatusItem(Key.Enter, "~ENTER~ Accept", () =>
                        {
                            if (Application.Top.MostFocused == _listView)
                            {
                                // If nothing was explicitly marked, we return the item that was selected
                                // when ENTER is pressed in Single mode. If something was previously selected
                                // (using SPACE) then honor that as the single item to return
                                if (_applicationData.OutputMode == OutputModeOption.Single &&
                                    _itemSource.GridViewRowList.Find(i => i.IsMarked) == null)
                                {
                                    _listView.MarkUnmarkRow();
                                }
                                Accept();
                            }
                            else if (Application.Top.MostFocused == _filterField)
                            {
                                _listView.SetFocus();
                            }
                        }),
                        new StatusItem(Key.Esc, "~ESC~ Close", () => Close())
                    }
                    : new StatusItem[]
                    {
                        new StatusItem(Key.Esc, "~ESC~ Close",  () => Close())
                    }
            );

            Application.Top.Add(statusBar);
        }

With recent changes to Terminal.Gui something in how ProcessKey works broke this. The ENTER handler never gets called.

In my opinion, if an app uses StatusBar and defines a hotkey for the StatusBar, that should override all other controls; they should never get that key. This is how it used to work.

I did a test by modifying the ComputedLayout scenario:

var statusBar = new StatusBar (new StatusItem [] {
				new StatusItem(Key.CtrlMask | Key.Q, "~^Q~ Quit", () => Quit()),
				new StatusItem(Key.Enter, "~ENTER~ Message", () => MessageBox.Query ("Message", $"Hi")),
			});

By doing this, I would expect StatusBar to always win and ENTER would go to StatusBar first. But, that's not what happens.

This is a breaking behavior change that I believe is a ship-stopper for V1.0.

@BDisp - please help me figure out what changed and a fix. Thanks.

[tig]

The change that broke this is this one:

I think this fix is CORRECT.

ListView does this:

public override bool ProcessKey (KeyEvent kb)
		{
			if (source == null)
				return base.ProcessKey (kb);

			switch (kb.Key) {
			case Key.CursorUp:
			case Key.P | Key.CtrlMask:
				return MoveUp ();

			case Key.CursorDown:
			case Key.N | Key.CtrlMask:
				return MoveDown ();

			case Key.V | Key.CtrlMask:
			case Key.PageDown:
				return MovePageDown ();

			case Key.PageUp:
				return MovePageUp ();

			case Key.Space:
				if (MarkUnmarkRow ())
					return true;
				else
					break;

			case Key.Enter:
				OnOpenSelectedItem ();
				break;

			case Key.End:
				return MoveEnd ();

			case Key.Home:
				return MoveHome ();

			default:
				return false;
			}

			return true;
		}

It returns true if the ENTER key was pressed. Which is correct.

So, I'm not sure what the right thing to do is.

One approach would be for me to change ocgv to explicitly handle OpenSelectedItem from ListView.

But, I worry that there may be other apps out there that depend on the old behavior.

Any suggestions or thoughts?

[BDisp]

@tig it had to be a Toplevel fix and on the view itself. I'm finishing the fix, please wait.

Edit:
Well fix it but is always open the message :-)

[tig]

Proposal:

Change ListView's 'OnOpenSelectedItem' and ProcessKey thusly:

Before:

public virtual bool OnOpenSelectedItem ()
{
	if (source.Count <= selected || selected < 0) return false;
	var value = source.ToList () [selected];


	OpenSelectedItem?.Invoke (new ListViewItemEventArgs (selected, value));

	return true;
}

After:

public virtual bool OnOpenSelectedItem ()
{
	if (source.Count <= selected || selected < 0 || OpenSelectedItem == null) {
		return false;
	}

	var value = source.ToList () [selected];

	OpenSelectedItem?.Invoke (new ListViewItemEventArgs (selected, value));

	return true;
}

Then, have ProcessKey utilize the return value from OnOpenSelectedItem:

public override bool ProcessKey (KeyEvent kb)
		{
...
			case Key.Enter:
				return OnOpenSelectedItem ();
				break;

...
			default:
				return false;
			}

			return true;
		}

[BDisp]

@tig try this on the Toplevel.cs:

		public override bool ProcessKey (KeyEvent keyEvent)
		{
			if (StatusBar != null && StatusBar.ProcessHotKey (keyEvent)
				|| SuperView != null && ((Toplevel)SuperView).StatusBar != null
				&& ((Toplevel)SuperView).StatusBar.ProcessHotKey (keyEvent)) {
				return true;
			}
			if (base.ProcessKey (keyEvent))
				return true;

			switch (ShortcutHelper.GetModifiersKey (keyEvent)) {
			case Key.Q | Key.CtrlMask:
				// FIXED: stop current execution of this container
				Application.RequestStop ();
				break;
			case Key.Z | Key.CtrlMask:
				Driver.Suspend ();
				return true;

[tig]

My fix above is a fix (and should probably be incorporated regardless because it is more correct).

I'm curious to see what your proposed fix is!

[tig]

@tig try this on the Toplevel.cs:

		public override bool ProcessKey (KeyEvent keyEvent)
		{
			if (StatusBar != null && StatusBar.ProcessHotKey (keyEvent)
				|| SuperView != null && ((Toplevel)SuperView).StatusBar != null
				&& ((Toplevel)SuperView).StatusBar.ProcessHotKey (keyEvent)) {
				return true;
			}
			if (base.ProcessKey (keyEvent))
				return true;

			switch (ShortcutHelper.GetModifiersKey (keyEvent)) {
			case Key.Q | Key.CtrlMask:
				// FIXED: stop current execution of this container
				Application.RequestStop ();
				break;
			case Key.Z | Key.CtrlMask:
				Driver.Suspend ();
				return true;

Interesting.

This fixes the issue too. But I wonder if my original suggestion that StatusBar should always win is actually correct now.

What do you think?

[BDisp]

The problem of my fix is that the StatusBar will always caught all the keystrokes and if it is used by the StatusBar, a view need it will never receive it.

[BDisp]

I'm a little split here. Can anyone else be able to pronounce? @tznind, @jmperricone , please.

[tig]

How about we take my fix now, since it's lower risk (more surgical)? It fixes the pressing issue of unblocking 1.0.

[BDisp]

@tig sometimes a view that handle some keystroke need to check if it's necessary to do something or not, but need to set it to true because it has the focus and don't want another view take another action for that keystroke. Think well :-)

But if it's a fix with a lower risk.

[tig]

Fixed.