Setting Menus causes unexpected Exception

[tznind]

Describe the bug

MenuBar.Menus magic property behaves inconsistently when set multiple times.

The setter should replace existing values with the new values.

Regarding the new Exception that is throw, it is problematic for designer where user might easily create multiple menu items with the same name or shortcut key.

For this use case we need a setting which suppress the Exception.

Test

    [Fact]
    public void Test_SetMenus()
    {
        var mb = new MenuBar ();

        var i1 = new MenuBarItem ("heey", "fff", () => { }, () => { return true; })
        {
            HotKey = new Rune('I')
        };

        mb.Menus = new MenuBarItem [] { i1 };
        mb.Menus = new MenuBarItem [] { i1 };
    }

Throws

  Message: 
System.InvalidOperationException : A key binding for i exists (KeyBinding { Commands = Terminal.Gui.Command[], Scope = Focused, BoundView = , Context = 0 }).

  Stack Trace: 
KeyBindings.Add(Key key, KeyBinding binding, View boundViewForAppScope) line 32
MenuBar.set_Menus(MenuBarItem[] value) line 178
MenuTests.Test_SetMenus() line 29
RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[BDisp]

I already facing this issue with the DynamicMenuBar scenario. I really working on it now and trying providing improvements to deal with properties changed. I'm also changed KeyCode to Key for the Shortcut which I renamed to ShortcutKey to distinguish from the Shortcut class. Currently it's very hard to deal with properties changed dynamically but I'm working on that.

[tznind]

Maybe a good solution would be to hold the shortcuts collection in a member and evaluating them 'just in time' at the keybinding resolution step?

In this approach we would not modify KeyBindings directly but instead look at the shortcuts then fall through to KeyBindings if none match?

[BDisp]

There is no need to modify the KeyBindings class. The bug is in the MenusBar on the Menus setter where it isn't removing the old HotKey and the old ShortcutKey.

[tznind]

There is no need to modify the KeyBindings class.

Sorry I meant maybe we can avoid modifying the KeyBindings instance (in memory) - not code changes.

In this case we would just look at the shortcuts as part of key resolution rather than trying to add/remove keybindings?

So it would be something like:

    /// <inheritdoc />
    public override bool? OnInvokingKeyBindings (Key keyEvent, KeyBindingScope scope)
    {
        foreach (var shortcut in Menus)
        {
            if (shortcut.HotKey == keyEvent.KeyCode)
            {
                // Do thing
                return true;
            }
        }
        return base.OnInvokingKeyBindings (keyEvent, scope);
    }

[BDisp]

But there another issue on the MenuBar that hold the old keybinding when another keybinding is added and we end up with a MenuIem with two Keybindings. Each menu item must only have one keybinding for HotKey and only one keybinding for ShortcutKey. So, it's MenuBar responsibility to ensure this condition. Sorry if I wasn't clear.

[tznind]

Each menu item must only have one keybinding for HotKey and only one keybinding for ShortcutKey. So, it's MenuBar responsibility to ensure this condition

Why? why not just use the first when it finds a collision at runtime? we don't really need this to be an Exception do we?

[BDisp]

Why? why not just use the first when it finds a collision at runtime? we don't really need this to be an Exception do we?

Of course not. That why I'm working on this now and I'm not getting any exception with keybinding now. I'm still work with code that will handing updating with properties changed. Because of the collision I found this bug in the menu and thus it's benefic it throw an exception in these situations.

[BDisp]

I would like suggest to change the Key.TryParse method to set key to null and return false instead of Key.Empty and true, if the text parameter is null or a empty string. The reason for my request is because there are many conditions on the keybindings that compare with null value before add or remove them. Any objections?

        if (string.IsNullOrEmpty (text))
        {
            //key = Key.Empty;
            key = null;

            //return true;
            return false;
        }

[tig]

Key.Empty == null.

[tig]

Regardless of the meaning of Key.Empty v null, it is my understanding that the semantics of a "TryXXX" method is if it returns false the value of the out params are indeterminate.

IOW if there is code that relies on an out parameter not being changed when false is returned that code is buggy.

[BDisp]

Regardless of the meaning of Key.Empty v null, it is my understanding that the semantics of a "TryXXX" method is if it returns false the value of the out params are indeterminate.

IOW if there is code that relies on an out parameter not being changed when false is returned that code is buggy.

It isn't the case I'll set the Key.Empty as default for the ShortcutKey and resolves this, I think.

[BDisp]

HotKeys are defined by prefixing the Title of a MenuItem with an underscore ('_').
So the hotkey is only highlighted when is prefixed by the underscore. Make sense to the HotKey property having a public setter?

[BDisp]

I'll change the HotKey property to have a private setter because as @tig already said before, a title without underscore assumes the user don't want a hot key but want using a shortcut key or the mouse. Furthermore, change the Title always update the HotKey and thus doesn't make sense to have a public setter. If you guys don't agree please speak something now, otherwise, keep quiet forever 😄