Fixed Checkbox race condition

Terminal.Gui/Views/CheckBox.cs

@@ -153,6 +153,12 @@ public class CheckBox : View
         }
 
         CancelEventArgs<CheckState> e = new (in _checkedState, ref value);
+
+        if (OnCheckedStateChanging (e))
+        {
+            return true;
+        }
+
         CheckedStateChanging?.Invoke (this, e);
         if (e.Cancel)
         {
@@ -163,9 +169,36 @@ public class CheckBox : View
         UpdateTextFormatterText ();
         OnResizeNeeded ();
 
+        EventArgs<CheckState> args = new (in _checkedState);
+        OnCheckedStateChanged (args);
+
+        CheckedStateChanged?.Invoke (this, args);
+
         return false;
     }
 
+    /// <summary>Called when the <see cref="CheckBox"/> state is changing.</summary>
+    /// <remarks>
+    /// <para>
+    ///    The state cahnge can be cancelled by setting the args.Cancel to <see langword="true"/>.
+    /// </para>
+    /// </remarks>
+    protected virtual bool OnCheckedStateChanging (CancelEventArgs<CheckState> args) { return false;}
+
+    /// <summary>Raised when the <see cref="CheckBox"/> state is changing.</summary>
+    /// <remarks>
+    /// <para>
+    ///    This event can be cancelled. If cancelled, the <see cref="CheckBox"/> will not change its state.
+    /// </para>
+    /// </remarks>
+    public event EventHandler<CancelEventArgs<CheckState>>? CheckedStateChanging;
+
+    /// <summary>Called when the <see cref="CheckBox"/> state has changed.</summary>
+    protected virtual void OnCheckedStateChanged (EventArgs<CheckState> args) { }
+
+    /// <summary>Raised when the <see cref="CheckBox"/> state has changed.</summary>
+    public event EventHandler<EventArgs<CheckState>>? CheckedStateChanged;
+
     /// <summary>
     ///     Advances <see cref="CheckedState"/> to the next value. Invokes the cancelable <see cref="CheckedStateChanging"/> event.
     /// </summary>
@@ -211,14 +244,6 @@ public class CheckBox : View
         return !cancelled;
     }
 
-    /// <summary>Raised when the <see cref="CheckBox"/> state is changing.</summary>
-    /// <remarks>
-    /// <para>
-    ///    This event can be cancelled. If cancelled, the <see cref="CheckBox"/> will not change its state.
-    /// </para>
-    /// </remarks>
-    public event EventHandler<CancelEventArgs<CheckState>>? CheckedStateChanging;
-
     /// <inheritdoc/>
     protected override void UpdateTextFormatterText ()
     {

Terminal.Gui/Views/Shortcut.cs

@@ -1,4 +1,6 @@
-namespace Terminal.Gui;
+using System.ComponentModel;
+
+namespace Terminal.Gui;
 
 /// <summary>
 ///     Displays a command, help text, and a key binding. When the key specified by <see cref="Key"/> is pressed, the
@@ -15,9 +17,9 @@
 ///         - Pressing the HotKey specified by <see cref="CommandView"/>.
 ///     </para>
 ///     <para>
-///         If <see cref="KeyBindingScope"/> is <see cref="KeyBindingScope.Application"/>, <see cref="Key"/> will invoked
+///         If <see cref="KeyBindingScope"/> is <see cref="KeyBindingScope.Application"/>, <see cref="Key"/> will invoke
 ///         <see cref="Command.Accept"/>
-///         command regardless of what View has focus, enabling an application-wide keyboard shortcut.
+///         regardless of what View has focus, enabling an application-wide keyboard shortcut.
 ///     </para>
 ///     <para>
 ///         By default, a Shortcut displays the command text on the left side, the help text in the middle, and the key
@@ -38,6 +40,42 @@
 /// </remarks>
 public class Shortcut : View, IOrientation, IDesignable
 {
+    /// <summary>
+    ///     Creates a new instance of <see cref="Shortcut"/>.
+    /// </summary>
+    public Shortcut () : this (Key.Empty, string.Empty, null) { }
+
+    /// <summary>
+    ///     Creates a new instance of <see cref="Shortcut"/>, binding it to <paramref name="targetView"/> and
+    ///     <paramref name="command"/>. The Key <paramref name="targetView"/>
+    ///     has bound to <paramref name="command"/> will be used as <see cref="Key"/>.
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         This is a helper API that simplifies creation of multiple Shortcuts when adding them to <see cref="Bar"/>-based
+    ///         objects, like <see cref="MenuBarv2"/>.
+    ///     </para>
+    /// </remarks>
+    /// <param name="targetView">
+    ///     The View that <paramref name="command"/> will be invoked when user does something that causes the Shortcut's Accept
+    ///     event to be raised.
+    /// </param>
+    /// <param name="command">
+    ///     The Command to invoke on <paramref name="targetView"/>. The Key <paramref name="targetView"/>
+    ///     has bound to <paramref name="command"/> will be used as <see cref="Key"/>
+    /// </param>
+    /// <param name="commandText">The text to display for the command.</param>
+    /// <param name="helpText">The help text to display.</param>
+    public Shortcut (View targetView, Command command, string commandText, string helpText = null) : this (
+                                                                                                           targetView?.KeyBindings.GetKeyFromCommands (command),
+                                                                                                           commandText,
+                                                                                                           null,
+                                                                                                           helpText)
+    {
+        _targetView = targetView;
+        _command = command;
+    }
+
     /// <summary>
     ///     Creates a new instance of <see cref="Shortcut"/>.
     /// </summary>
@@ -47,9 +85,9 @@ public class Shortcut : View, IOrientation, IDesignable
     ///     </para>
     /// </remarks>
     /// <param name="key"></param>
-    /// <param name="commandText"></param>
+    /// <param name="commandText">The text to display for the command.</param>
     /// <param name="action"></param>
-    /// <param name="helpText"></param>
+    /// <param name="helpText">The help text to display.</param>
     public Shortcut (Key key, string commandText, Action action, string helpText = null)
     {
         Id = "_shortcut";
@@ -62,8 +100,8 @@ public class Shortcut : View, IOrientation, IDesignable
         _orientationHelper.OrientationChanging += (sender, e) => OrientationChanging?.Invoke (this, e);
         _orientationHelper.OrientationChanged += (sender, e) => OrientationChanged?.Invoke (this, e);
 
-        AddCommand (Command.HotKey, ctx => OnAccept (ctx));
-        AddCommand (Command.Accept, ctx => OnAccept (ctx));
+        AddCommand (Command.HotKey, ctx => DispatchAcceptCommand (ctx));
+        AddCommand (Command.Accept, ctx => DispatchAcceptCommand (ctx));
         AddCommand (Command.Select, ctx => OnSelect (ctx));
 
         TitleChanged += Shortcut_TitleChanged; // This needs to be set before CommandView is set
@@ -132,10 +170,10 @@ public class Shortcut : View, IOrientation, IDesignable
         }
     }
 
-    /// <summary>
-    ///     Creates a new instance of <see cref="Shortcut"/>.
-    /// </summary>
-    public Shortcut () : this (Key.Empty, string.Empty, null) { }
+    [CanBeNull]
+    private readonly View _targetView; // If set, _command will be invoked
+
+    private readonly Command _command; // Used when _targetView is set
 
     private readonly OrientationHelper _orientationHelper;
 
@@ -144,7 +182,7 @@ public class Shortcut : View, IOrientation, IDesignable
     // This is used to calculate the minimum width of the Shortcut when the width is NOT Dim.Auto
     private int? _minimumDimAutoWidth;
 
-    /// <inheritdoc />
+    /// <inheritdoc/>
     protected override bool OnHighlight (CancelEventArgs<HighlightStyle> args)
     {
         if (args.NewValue.HasFlag (HighlightStyle.Hover))
@@ -447,6 +485,7 @@ public class Shortcut : View, IOrientation, IDesignable
 
             if (_commandView is { })
             {
+                _commandView.Accept -= CommandViewOnAccept;
                 Remove (_commandView);
                 _commandView?.Dispose ();
             }
@@ -459,18 +498,34 @@ public class Shortcut : View, IOrientation, IDesignable
             _commandView.CanFocus = false;
 
             _commandView.HotKeyChanged += (s, e) =>
-                                          {
-                                              if (e.NewKey != Key.Empty)
-                                              {
-                                                  // Add it 
-                                                  AddKeyBindingsForHotKey (e.OldKey, e.NewKey);
-                                              }
-                                          };
+            {
+                if (e.NewKey != Key.Empty)
+                {
+                    // Add it 
+                    AddKeyBindingsForHotKey (e.OldKey, e.NewKey);
+                }
+            };
 
             _commandView.HotKeySpecifier = new ('_');
 
             Title = _commandView.Text;
 
+            _commandView.Accept += CommandViewOnAccept;
+
+            void CommandViewOnAccept (object sender, HandledEventArgs e)
+            {
+                KeyBinding binding = new ([Command.Select], KeyBindingScope.Focused, _commandView, null);
+                e.Handled = DispatchAcceptCommand (new CommandContext (Command.Select, null, binding)) == true;
+            }
+
+            _commandView.Select += CommandViewOnSelect;
+
+            void CommandViewOnSelect (object sender, HandledEventArgs e)
+            {
+                SetFocus ();
+                //OnAccept ();
+            }
+
             SetCommandViewDefaultLayout ();
             SetHelpViewDefaultLayout ();
             SetKeyViewDefaultLayout ();
@@ -663,9 +718,12 @@ public class Shortcut : View, IOrientation, IDesignable
     {
         if (Key != null && Key.IsValid)
         {
-            // Disable the command view key bindings
+            // Disable the command view HotKey bindings
             CommandView.KeyBindings.Remove (Key);
             CommandView.KeyBindings.Remove (CommandView.HotKey);
+            CommandView.KeyBindings.Remove (CommandView.HotKey.WithShift);
+            CommandView.KeyBindings.Remove (CommandView.HotKey.WithAlt);
+            CommandView.KeyBindings.Remove (CommandView.HotKey.WithShift.WithAlt);
 
             if (KeyBindingScope.FastHasFlags (KeyBindingScope.Application))
             {
@@ -702,49 +760,20 @@ public class Shortcut : View, IOrientation, IDesignable
     ///     - if the user presses the HotKey specified by CommandView
     ///     - if HasFocus and the user presses Space or Enter (or any other key bound to Command.Accept).
     /// </summary>
-    protected bool? OnAccept (CommandContext ctx)
+    protected bool? DispatchAcceptCommand (CommandContext ctx)
     {
         var cancel = false;
 
-        switch (ctx.KeyBinding?.Scope)
-        {
-            case KeyBindingScope.Application:
-                cancel = base.RaiseAcceptEvent () == true;
-
-                break;
-
-            case KeyBindingScope.Focused:
-                base.RaiseAcceptEvent ();
-
-                // cancel if we're focused
-                cancel = true;
+        // We don't care if CommandView handles the command or not
+        CommandView.InvokeCommand (Command.Select, ctx.Key, ctx.KeyBinding);
 
-                break;
+        cancel = RaiseAcceptEvent () == true;
 
-            case KeyBindingScope.HotKey:
-                //if (!CanBeVisible(this))
-                //{
-                //    return true;
-                //}
-                cancel = base.RaiseAcceptEvent () == true;
-
-                if (CanFocus)
-                {
-                    SetFocus ();
-                    cancel = true;
-                }
-
-                break;
-
-            default:
-                // Mouse
-                cancel = base.RaiseAcceptEvent () == true;
-
-                break;
+        if (!cancel && ctx.KeyBinding?.BoundView != CommandView)
+        {
+           // CommandView.InvokeCommand (Command.Select, ctx.Key, ctx.KeyBinding);
         }
 
-        CommandView.InvokeCommand (Command.Accept, ctx.Key, ctx.KeyBinding);
-
         if (Action is { })
         {
             Action.Invoke ();
@@ -753,6 +782,12 @@ public class Shortcut : View, IOrientation, IDesignable
             cancel = true;
         }
 
+        if (_targetView is { })
+        {
+            _targetView.InvokeCommand (_command);
+        }
+
+
         return cancel;
     }
 
@@ -786,7 +821,10 @@ public class Shortcut : View, IOrientation, IDesignable
     internal void SetColors (bool highlight = false)
     {
         // Border should match superview.
-        Border.ColorScheme = SuperView?.ColorScheme;
+        if (Border is { })
+        {
+            Border.ColorScheme = SuperView?.ColorScheme;
+        }
 
         if (HasFocus || highlight)
         {
@@ -819,10 +857,7 @@ public class Shortcut : View, IOrientation, IDesignable
     }
 
     /// <inheritdoc/>
-    protected override void OnHasFocusChanged (bool newHasFocus, View previousFocusedView, View view)
-    {
-        SetColors ();
-    }
+    protected override void OnHasFocusChanged (bool newHasFocus, View previousFocusedView, View view) { SetColors (); }
 
     #endregion Focus
-}
+}

UICatalog/Scenarios/DynamicMenuBar.cs

@@ -247,9 +247,9 @@ public class DynamicMenuBar : Scenario
                                          }
                                      };
 
-            CkbSubMenu.CheckedStateChanging += (s, e) =>
+            CkbSubMenu.CheckedStateChanged += (s, e) =>
                                   {
-                                      if (e.NewValue == CheckState.Checked)
+                                      if (e.CurrentValue == CheckState.Checked)
                                       {
                                           CkbIsTopLevel.CheckedState = CheckState.UnChecked;
                                           CkbIsTopLevel.SetNeedsDisplay ();
@@ -275,16 +275,16 @@ public class DynamicMenuBar : Scenario
                                           if (_hasParent)
                                           {
                                               TextShortcutKey.Enabled = CkbIsTopLevel.CheckedState == CheckState.UnChecked
-                                                                     && e.NewValue == CheckState.UnChecked;
+                                                                     && e.CurrentValue == CheckState.UnChecked;
                                           }
                                       }
                                   };
 
-            CkbNullCheck.CheckedStateChanging += (s, e) =>
+            CkbNullCheck.CheckedStateChanged += (s, e) =>
                                     {
                                         if (_menuItem != null)
                                         {
-                                            _menuItem.AllowNullChecked = e.NewValue == CheckState.Checked;
+                                            _menuItem.AllowNullChecked = e.CurrentValue == CheckState.Checked;
                                         }
                                     };
 

UICatalog/Scenarios/Shortcuts.cs

@@ -82,18 +82,6 @@ public class Shortcuts : Scenario
                                                                         eventLog.MoveDown ();
                                                                     };
 
-        vShortcut2.Accept += (o, args) =>
-                            {
-                                // Cycle to next item. If at end, set 0
-                                if (((RadioGroup)vShortcut2.CommandView).SelectedItem < ((RadioGroup)vShortcut2.CommandView).RadioLabels.Length - 1)
-                                {
-                                    ((RadioGroup)vShortcut2.CommandView).SelectedItem++;
-                                }
-                                else
-                                {
-                                    ((RadioGroup)vShortcut2.CommandView).SelectedItem = 0;
-                                }
-                            };
         Application.Top.Add (vShortcut2);
 
         var vShortcut3 = new Shortcut
@@ -150,7 +138,7 @@ public class Shortcuts : Scenario
             KeyBindingScope = KeyBindingScope.HotKey,
         };
         Button button = (Button)vShortcut4.CommandView;
-        vShortcut4.CommandView.Accept += Button_Clicked;
+        vShortcut4.Accept += Button_Clicked;
 
         Application.Top.Add (vShortcut4);
 
@@ -358,16 +346,29 @@ public class Shortcuts : Scenario
         {
             if (sh is Shortcut shortcut)
             {
-                shortcut.Accept += (o, args) =>
+                shortcut.Select += (o, args) =>
                                    {
-                                       eventSource.Add ($"Accept: {shortcut!.CommandView.Text}");
+                                       eventSource.Add ($"Select: {shortcut!.CommandView.Text} {shortcut!.CommandView.GetType ().Name}");
                                        eventLog.MoveDown ();
                                        args.Handled = true;
                                    };
 
+                shortcut.CommandView.Select += (o, args) =>
+                                               {
+                                                   eventSource.Add ($"CommandView.Select: {shortcut!.CommandView.Text} {shortcut!.CommandView.GetType ().Name}");
+                                                   eventLog.MoveDown ();
+                                               };
+
+                shortcut.Accept += (o, args) =>
+                                       {
+                                           eventSource.Add ($"Accept: {shortcut!.CommandView.Text} {shortcut!.CommandView.GetType ().Name}");
+                                           eventLog.MoveDown ();
+                                           args.Handled = true;
+                                       };
+
                 shortcut.CommandView.Accept += (o, args) =>
                                                {
-                                                   eventSource.Add ($"CommandView.Accept: {shortcut!.CommandView.Text}");
+                                                   eventSource.Add ($"CommandView.Accept: {shortcut!.CommandView.Text} {shortcut!.CommandView.GetType ().Name}");
                                                    eventLog.MoveDown ();
                                                };
             }
@@ -378,7 +379,7 @@ public class Shortcuts : Scenario
 
     private void Button_Clicked (object sender, HandledEventArgs e)
     {
-        //e.Cancel = true;
+        e.Handled = true;
         MessageBox.Query ("Hi", $"You clicked {sender}");
     }
 }

UnitTests/Views/ShortcutTests.cs

@@ -379,18 +379,18 @@ public class ShortcutTests
     //  0123456789
     // " C  0  A "
     [InlineData (-1, 0, 0)]
-    [InlineData (0, 1, 1)]
-    [InlineData (1, 1, 1, Skip = "BUGBUG: This breaks. We need to fix the logic in the Shortcut class.")]
-    [InlineData (2, 1, 1)]
-    [InlineData (3, 1, 1)]
-    [InlineData (4, 1, 1)]
-    [InlineData (5, 1, 1)]
-    [InlineData (6, 1, 1)]
-    [InlineData (7, 1, 1)]
-    [InlineData (8, 1, 1)]
+    [InlineData (0, 1, 0)]
+    [InlineData (1, 1, 1)]
+    [InlineData (2, 1, 0)]
+    [InlineData (3, 1, 0)]
+    [InlineData (4, 1, 0)]
+    [InlineData (5, 1, 0)]
+    [InlineData (6, 1, 0)]
+    [InlineData (7, 1, 0)]
+    [InlineData (8, 1, 0)]
     [InlineData (9, 0, 0)]
     [AutoInitShutdown]
-    public void MouseClick_Button_CommandView_Fires_Accept (int x, int expectedAccept, int expectedButtonAccept)
+    public void MouseClick_Button_CommandView_Fires_Shortcut_Accept (int mouseX, int expectedAccept, int expectedButtonAccept)
     {
         var current = new Toplevel ();
 
@@ -404,10 +404,15 @@ public class ShortcutTests
         {
             Title = "C",
             NoDecorations = true,
-            NoPadding = true
+            NoPadding = true,
+            CanFocus = false
         };
         var buttonAccepted = 0;
-        shortcut.CommandView.Accept += (s, e) => { buttonAccepted++; };
+        shortcut.CommandView.Accept += (s, e) => {
+                                           buttonAccepted++;
+                                           // Must indicate handled
+                                           e.Handled = true;
+                                       };
         current.Add (shortcut);
 
         Application.Begin (current);
@@ -420,12 +425,12 @@ public class ShortcutTests
         Application.OnMouseEvent (
                                   new ()
                                   {
-                                      Position = new (x, 0),
+                                      Position = new (mouseX, 0),
                                       Flags = MouseFlags.Button1Clicked
                                   });
 
-        Assert.Equal (expectedAccept, accepted);
         Assert.Equal (expectedButtonAccept, buttonAccepted);
+        Assert.Equal (expectedAccept, accepted);
 
         current.Dispose ();
     }