`Button.IsDefault` is confusing

[tig]

The other issue, where any other peer view (in this case a StatusBar) of an IsDefault button that raises Accept causing the button to Accept is actually BY DESIGN. This is literally what Button.IsDefault means.

That said, I think the design of IsDefault is confusing.

Originally posted by @tig in #4167

[tig]

Moving @tznind comment here:

If a peer-View receives Accept and does not handle it

This is where the issue lies - most people would consider registering an Accepting handler and doing something in it to be 'handling it'.

Its only after seeing odd behaviour that you explore e and see it has a Handled property. I wonder if we could have static analyzer flag this as warning.

CSX - Accepting event handler is not setting Handled in any branches

[BDisp]

I think it shouldn't be the Button that decides whether it is Default or not, but the Toplevel that decides. There should be the AcceptButton and the CancelButton. It doesn't make sense to have more than one default button for both Accept and Cancel. The Toplevel can have precedence over the keyboard execution in relation to the currently focused view by defining a property. My only doubt is whether it should add these properties in the Application class or in the Toplevel class.

I propose the following:

• Replace the Button's IsDefault property with a method to check whether it is Default or not.
• Add the AcceptButton, CancelButton and KeyPreview or KeyPrecedence properties to the Toplevel as an instance or to the Application as static.

[BDisp]

Do you already have some idea for this and for my suggestion?

[tig]

Moving @BDisp's comment from an unrelated PR to here, where it belongs:

---

The bellow unit test proves that the Button still fail in this branch when IsDefault is true and the another non-default Button is clicked. When a key binding is not handled it must propagate to the subViews first and then to the superView until it is null. When a mouse binding is not handled it must propagate to the subViews first and then return true at end without propagate to the superView. We need to know how about to handle with the #4170 to take the appropriate actions for default AcceptButton and default CancelButton. Doesn't make sense that a default button that accept some action has priority when the Esc key is pressed. One idea that occur to me, since MouseBinding now have the MouseEventArgs property, is to leverage the View property of the MouseEventArgs. So, if the current view is the as the View property then it's responsible to return true at the end.

I submitted the PR tig#47 which fix this.

    [Fact]
    [AutoInitShutdown]
    public void MouseClick_From_Non_Default_Button_Raise_Accept_In_The_Default_Button ()
    {
        var acceptOk = 0;
        var acceptCancel = 0;
        Button btnOk = new () { Id = "Ok", Text = "Ok", IsDefault = true };
        btnOk.Accepting += (s, e) => acceptOk++;
        Button btnCancel = new () { Id = "Cancel", Y = 1, Text = "Cancel" };
        btnCancel.Accepting += (s, e) => acceptCancel++;
        Application.Top = new Toplevel ();
        Application.Top.Add (btnOk, btnCancel);
        var rs = Application.Begin (Application.Top);

        Application.RaiseMouseEvent (new () { ScreenPosition = new (0, 1), Flags = MouseFlags.Button1Clicked });
        Application.RunIteration (ref rs);
        Assert.Equal (0, acceptOk);
        Assert.Equal (1, acceptCancel);

        Application.End (rs);
    }

[tig]

Moving @BDisp's comment from an unrelated PR to here, where it belongs:

The bellow unit test proves that the Button still fail in this branch when IsDefault is true and the another non-default Button is clicked. When a key binding is not handled it must propagate to the subViews first and then to the superView until it is null. When a mouse binding is not handled it must propagate to the subViews first and then return true at end without propagate to the superView. We need to know how about to handle with the #4170 to take the appropriate actions for default AcceptButton and default CancelButton. Doesn't make sense that a default button that accept some action has priority when the Esc key is pressed. One idea that occur to me, since MouseBinding now have the MouseEventArgs property, is to leverage the View property of the MouseEventArgs. So, if the current view is the as the View property then it's responsible to return true at the end.

What you describe IS BY DESIGN.

The documentation, which I've referred to multiple times since you first brough this up says:

We discussed this previously here when we decided on Cancel->Handled: #3913

The intent of this design, which I've also explained multiple times is as follows:

• Nested hierarchies of subviews need a standard way for views lower in the hierarchy to convey up the hierarchy that the user has done something to "accept state". This is accomplished by the propagation of Command.Accept up the hierarchy at the end of the default RaiseAccepting logic.
• This drives the requirement that any view that does something based on Command.Accept indicate it handled the event.
• Related, there is a requirement that a single Button at some level of the hierachy a) have a visual indication that it is the "default button", b) be able to be the place where developers put their "do the default action" thing (vs. handling Command.Accept in the button's superview).

This design requires that any view that gets Command.Accept invoked on it set .Handled = true to indicate Command.Accept was handled.

Therefore, your test has a bug in it. The btnCancel.Accepting event handler MUST set args.Handled = true.

This design is DIFFERENT than what was in v1/early v2. Perhaps that is why you are confused?

Your proposed test would be correct as:

    [Fact]
    public void Command_Accept_Handled_Stops_Propagation_To_IsDefault_Button_Peer ()
    {
        var acceptOk = 0;
        var acceptCancel = 0;
        Button btnOk = new () { Id = "btnOk", Text = "Ok", IsDefault = true };
        btnOk.Accepting += (s, e) => acceptOk++;
        Button btnCancel = new () { Id = "btnCancel", Text = "Cancel", IsDefault = false };

        btnCancel.Accepting += (s, e) =>
                               {
                                   acceptCancel++;
                                   e.Handled = true;
                               };

        View superView = new View () { Id = "superView" };
        superView.Add (btnOk, btnCancel);

        btnCancel.InvokeCommand (Command.Accept);
        Assert.Equal (0, acceptOk);
        Assert.Equal (1, acceptCancel);
    }

    [Fact]
    public void Command_Accept_Not_Handled_Propagates_To_IsDefault_Button_Peer ()
    {
        var acceptOk = 0;
        var acceptCancel = 0;
        Button btnOk = new () { Id = "btnOk", Text = "Ok", IsDefault = true };
        btnOk.Accepting += (s, e) => acceptOk++;
        Button btnCancel = new () { Id = "btnCancel", Text = "Cancel", IsDefault = false };

        btnCancel.Accepting += (s, e) =>
                               {
                                   acceptCancel++;
                               };
        View superView = new View () { Id = "superView" };
        superView.Add (btnOk, btnCancel);

        btnCancel.InvokeCommand (Command.Accept);
        Assert.Equal (1, acceptOk);
        Assert.Equal (1, acceptCancel);
    }

Now, the purpose if THIS issue (#4170) is to debate whether this DESIGN is correct, confusing, or something else.

In your text above you state something about the Esc key being pressed. By default Key.Esc is bound to Command.Quit at the application level. So that behavior has really nothing to do with Command.Accept propagation.

[tig]

• Add the AcceptButton, CancelButton and KeyPreview or KeyPrecedence properties to the Toplevel as an instance or to the Application as static.

I do not like this idea. It drives coupling into Toplevel which we are trying to make go completely away.

I think it shouldn't be the Button that decides whether it is Default or not, but the Toplevel that decides. There should be the AcceptButton and the CancelButton. It doesn't make sense to have more than one default button for both Accept and Cancel. The Toplevel can have precedence over the keyboard execution in relation to the currently focused view by defining a property. My only doubt is whether it should add these properties in the Application class or in the Toplevel class.

I propose the following:

• Replace the Button's IsDefault property with a method to check whether it is Default or not.

Let's step back and make sure we're aligned on the overall intent of IsDefault. Start with the end-user (not developer):

• "As a user, looking at a TUI with multiple Views, I expect to see a visual indicator of which View will perform the default action if I perform the accept action (e.g. press ENTER or click on the View in question)."
• "As a user, I expect consistency across and within TUI applications for how the default View performs."
• "As a user, I am used to Buttons having a default indicator, because that is common on Windows and Mac. I am not used to other controls having such a feature because neither Windows or Mac do that."

Is this right? Am I missing anything from a user's perspective?

From a TUI app developer's perspective:

• "As a developer, building a TUI where there are multiple views and there is a need to know when the user wants to accept the current state, I want an easy way of defining and implementing the code that will be invoked when the user accepts."
• "As a developer, building a TUI ..., I want an easy way to indicate which View is the View the user should interact with to indicate they are accepting the state."
• "As a developer, building a TUI ..., I am used to Buttons having a default indicator, because that is common on Windows and Mac. I am not used to other controls having such a feature because neither Windows or Mac do that."

Based on this, the current design is just fine.

For end-users:

• Button.IsDefault provides a visual indicator. If developers use the API correctly, it works as users expect.
• The default RaiseAccepting logic drives consistency, which users expect.
• It only impacts Button which users are fine with.

For Developers:

• A dev can choose to put their Accept logic in either this.Accepting/OnAccepting or in buttonSubView.Accepting, as long as they set buttonSubView.IsDefault = true. Seems pretty simple and flexible.
• Setting Button.IsDefault is super easy. It might be nice if it were somehow automatic, and it would be nice if there was an error if I accidentally made multiple default.
• No dev has asked for a use-case where IsDefault applies to other Views.

However, repeated feedback indicates that Button.IsDefault is confusing. So, the above requirements must be missing something.

I, personally, am at a loss to describe what it is. I DO know:

• As a library maintainer, I hate that the logic for dealing with Button.IsDefault is in View; that's just horrible coupling.
• As a library maintainer, I hate that the Command.Accepting applies to all scenarios and can't be disabled without careful overriding.
• As a library maintainer, we need an similar propagation model for Command.Activating

[tig]

My proposal for addressing this issue ("Button.IsDefault is confusing") is to:

• Add an IDefaultAcceptView

  
    /// <summary>
    ///     When a View implements this interface, it will act as the default handler for <see cref="Command.Accept"/>.
    /// </summary>
    public interface IDefaultAcceptView
    {
        /// <summary>
        ///     Gets whether the <see cref="View"/> will act as the default handler for <see cref="Command.Accept"/>
        ///     commands on the <see cref="View.SuperView"/>.
        /// </summary>
        /// <remarks>
        ///     <para>
        ///         If <see langword="true"/>:
        ///     </para>
        ///     <para>
        ///         - The View will display an indicator that it is the default View.
        ///     </para>
        ///     <para>
        ///         - If the view does not handle <see cref="Command.Accept"/>, <see cref="Command.Accept"/> will be
        ///         invoked on the SuperView.
        ///     </para>
        ///     <para>
        ///         - If a peer-View receives <see cref="Command.Accept"/> and does not handle it, the command will be passed to
        ///         the first View in the SuperView that returns <see langword="true"/>.
        ///     </para>
        /// </remarks>
        public bool GetIsDefaultAcceptView ();

        /// <summary>
        ///     Sets whether the <see cref="View"/> will act as the default handler for <see cref="Command.Accept"/>
        ///     commands on the <see cref="View.SuperView"/>.
        /// </summary>
        /// <remarks>
        ///     <para>
        ///         If <see langword="true"/>:
        ///     </para>
        ///     <para>
        ///         - The View will display an indicator that it is the default View.
        ///     </para>
        ///     <para>
        ///         - If the view does not handle <see cref="Command.Accept"/>, <see cref="Command.Accept"/> will be
        ///         invoked on the SuperView.
        ///     </para>
        ///     <para>
        ///         - If a peer-View receives <see cref="Command.Accept"/> and does not handle it, the command will be passed to
        ///         the first View in the SuperView that returns <see langword="true"/>.
        ///     </para>
        /// </remarks>
        public void SetIsDefaultAcceptView (bool value);
    }

• Have Button implement this interface, deleting IsDefault as a property.

• Change the logic in RaiseAccepting to:

        // Accept is a special case where if the event is not canceled, the event is
        //  - Invoked on any peer-View where IDefaultAcceptView.GetIsDefaultAcceptView() returns true
        //  - propagated up the SuperView hierarchy.
        if (!args.Handled)
        {
            // If there's a default accept view peer view in SubViews, try it
            View? defaultAcceptView = SuperView?.InternalSubViews.FirstOrDefault (v => v is IDefaultAcceptView defaultAccept && defaultAccept.GetIsDefaultAcceptView ());

            if (defaultAcceptView != this && defaultAcceptView is IDefaultAcceptView defaultAccept && defaultAccept.GetIsDefaultAcceptView ()))
            {
                bool? handled = defaultAcceptView?.InvokeCommand (Command.Accept, ctx);

                if (handled == true)
                {
                    return true;
                }
            }

This:

• Fixes the coupling problem
• Enables any View that may want to be "IsDefault" to do so without the libary having to change
• Doesn't complicate View or Toplevel with more stuff.
• Makes things more clear, because the name of DefaultAcceptView is more precise.

[BDisp]

I absolutely agree that if a peer-View receives Accept and doesn't handle it, the command will be passed to the first Button in the SuperView that has IsDefault set to true, if it's managed by a KeyBinding but not managed by a MouseBinding. If you click on a Button that has IsDefault set to false why you want the command to be passed to the first Button in the SuperView that has IsDefault set to true? This has no sense for me, really. Justifying it by saying it IS BY DESIGN, isn't a escuse to me.
You also mention that the Key.Esc is bound to Command.Quit at the application level. Imagine that a cancel Button want to personalize a quit procedure with custom code, which may be invoke by the keyboard or by mouse? If you have two defaults Button, one for Ok and another for Cancel, Key.Eenter will invoke both if none set Handled to true. Key.Esc for sure wont execute the code in the Cancel Button event.
The IDefaultAcceptView interface will allow any view can be default accept view, but this is really needed? With a scenario where you can have multiple Window with default Buttons for Ok and Cancel on each one. Does your solution will handle this properly?

[tig]

This has no sense for me, really. Justifying it by saying it IS BY DESIGN, isn't a escuse to me.

You are seeing something I'm not.

Here's more unit tests to get us on the same page. These all pass:

    [Fact]
    public void Command_Accept_Handled_Stops_Propagation_To_Not_IsDefault_Button_Peer ()
    {
        var acceptOk = 0;
        var acceptCancel = 0;
        Button btnOk = new () { Id = "btnOk", Text = "Ok", IsDefaultAcceptView = false };
        btnOk.Accepting += (s, e) => acceptOk++;
        Button btnCancel = new () { Id = "btnCancel", Text = "Cancel", IsDefaultAcceptView = false };

        btnCancel.Accepting += (s, e) =>
                               {
                                   acceptCancel++;
                                   e.Handled = true;
                               };

        View superView = new View () { Id = "superView" };
        superView.Add (btnOk, btnCancel);

        btnCancel.InvokeCommand (Command.Accept);
        Assert.Equal (0, acceptOk);
        Assert.Equal (1, acceptCancel);
    }

    [Fact]
    public void Command_Accept_Not_Handled_Propagates_To_Not_IsDefault_Button_Peer ()
    {
        var acceptOk = 0;
        var acceptCancel = 0;
        Button btnOk = new () { Id = "btnOk", Text = "Ok", IsDefaultAcceptView = false };
        btnOk.Accepting += (s, e) => acceptOk++;
        Button btnCancel = new () { Id = "btnCancel", Text = "Cancel", IsDefaultAcceptView = false };

        btnCancel.Accepting += (s, e) =>
                               {
                                   acceptCancel++;
                               };
        View superView = new View () { Id = "superView" };
        superView.Add (btnOk, btnCancel);

        btnCancel.InvokeCommand (Command.Accept);
        Assert.Equal (0, acceptOk);
        Assert.Equal (1, acceptCancel);
    }