Terminal.Gui `Button.IsDefault` is confusing

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

Jun 19 '25 17:06 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

Jun 19 '25 18:06 tig

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.

Jun 23 '25 12:06 BDisp

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

Jun 29 '25 10:06 BDisp

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 https://github.com/tig/Terminal.Gui/pull/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);
    }

Jun 30 '25 18:06 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: https://github.com/gui-cs/Terminal.Gui/issues/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.

Jun 30 '25 19:06 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

Jun 30 '25 19:06 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.

Jun 30 '25 19:06 tig

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?

Jun 30 '25 20:06 BDisp

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);
    }

Jun 30 '25 21:06 tig

The IDefaultAcceptView interface will allow any view can be default accept view, but this is really needed?

Not really. I argue against it above. However, ONLY Button will implement this interface, so really only Button will have the feature.

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?

Yes.

Jun 30 '25 21:06 tig

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

I also don't agree to put this configuration in the Application static class. But in the class that will supersede the Toplevel class.

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

If you get rid of the Toplevel class is the Window class that will supersede it? I agree but some properties and probably some methods will have to be moved to the Window class, right? I think we should avoid that the View class itself could be a top-level app. For a top-level app some properties and methods should impose some rules, in my opinion. Any view now can have a MenuBar and StatusBar which is very good, but there are some events which only will be in a top-level that can handle tham, like the Load and Unload, Closing and Closed, etc.

Jun 30 '25 21:06 BDisp

You are seeing something I'm not.

I think you don't want to see the obvious. Yours unit tests pass because you aren't call Application.RaiseMouseEvent method which simulate a real running app and only use the button you want to call InvokeCommand method. My unit test is correct, contrary you said before, and is only running one iteration. You have to understand that some fails occurs when the unit test is running as a real app. You must test with this unit test which is also included in the PR https://github.com/tig/Terminal.Gui/pull/47.

    [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", IsDefaultAcceptView = true };
        btnOk.Accepting += (s, e) => acceptOk++;
        Button btnCancel = new () { Id = "Cancel", Y = 1, Text = "_Cancel" };
        btnCancel.Accepting += (s, e) => acceptCancel++;
        Application.Top = new ();
        Application.Top.Add (btnOk, btnCancel);
        var rs = Application.Begin (Application.Top);

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

        Application.End (rs);
        Application.Top.Dispose ();
        Application.ResetState ();
    }

Jun 30 '25 22:06 BDisp

What we disagree is the need to set e.Handled = true; to avoid the default button raising the Accepting event to performing a obvious action which is executing the code that is in the Cancel button. That will be tedious to code with always the concern of set e.Handled = true; only for a default view that's only is used by the Button view. For me I only give that task to the Window class. Thus we can avoid to propagate the Accept to all views that probably want to accept (Buttons). Only hot keys must be propagate until is handled or not by keyboard and not by mouse. It's only me that is seeing this?

Jun 30 '25 22:06 BDisp

One thing that can make sense is if the Accepting event is canceled then the Accepted (doesn't exist) event won't be invoked. In this case in most of the cases only the Accepted event will be used and the Accepting event is only used if we could cancel it based on some condition.

Jun 30 '25 22:06 BDisp

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

I also don't agree to put this configuration in the Application static class. But in the class that will supersede the Toplevel class.

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

If you get rid of the Toplevel class is the Window class that will supersede it? I agree but some properties and probably some methods will have to be moved to the Window class, right? I think we should avoid that the View class itself could be a top-level app. For a top-level app some properties and methods should impose some rules, in my opinion. Any view now can have a MenuBar and StatusBar which is very good, but there are some events which only will be in a top-level that can handle tham, like the Load and Unload, Closing and Closed, etc.

See https://github.com/gui-cs/Terminal.Gui/issues/2491 where this is all discussed.

Jun 30 '25 23:06 tig

I think you don't want to see the obvious.

I may not be seeing the obvious, but it's NOT because I don't want to. If I didn't, I'd just shut this conversation down and ignore you. But I value your opinion and input and recognize that I'm often missing something.

Yours unit tests pass because you aren't call Application.RaiseMouseEvent method which simulate a real running app and only use the button you want to call InvokeCommand method. My unit test is correct, contrary you said before, and is only running one iteration. You have to understand that some fails occurs when the unit test is running as a real app. You must test with this unit test which is also included in the PR tig#47.
[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", IsDefaultAcceptView = true };
    btnOk.Accepting += (s, e) => acceptOk++;
    Button btnCancel = new () { Id = "Cancel", Y = 1, Text = "_Cancel" };
    btnCancel.Accepting += (s, e) => acceptCancel++;
    Application.Top = new ();
    Application.Top.Add (btnOk, btnCancel);
    var rs = Application.Begin (Application.Top);

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

    Application.End (rs);
    Application.Top.Dispose ();
    Application.ResetState ();
}

I still disagree. The above test is wrong. It has a bug. btnCancel.Accepting is not setting e.Handled = true, and as a result, the Command.Accept is CORRECTLY propagating to the Application.Top which then forwards it on to the IsDefaultAcceptView button. Thus it CORRECTLY fails (as written) because btnOk.Accepted SHOULD be raised.

If you want this test to pass, you MUST set e.Handled = true in btnCancel.Accepting.

This has nothing to do with mouse handling. Or keyboard handling. Changing the above test to:

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

        Application.RaiseKeyDownEvent(Key.C);
        Assert.Equal (0, acceptOk);
        Assert.Equal (1, acceptCancel);

        Application.End (rs);
        Application.Top.Dispose ();
        Application.ResetState ();
    }

...produces exactly the same result: acceptOk is 1 and acceptCancel is 1. Both should be raised once given how the code is written.

Jun 30 '25 23:06 tig

What we disagree is the need to set e.Handled = true; to avoid the default button raising the Accepting event to performing a obvious action which is executing the code that is in the Cancel button.

What code is "in the Cancel button"? What are you talking about? There is no code "in the Cancel button" in the test code above (except acceptCancel++).

That will be tedious to code with always the concern of set e.Handled = true; only for a default view that's only is used by the Button view.

I don't think so. If an Accepted handler does something it should indicate it did something. Thus it should set e.Handled = true. That's not tedious. That's how events in dotnet that use HandledEventArgs work. That's the design. I didn't invent that.

For me I only give that task to the Window class. Thus we can avoid to propagate the Accept to all views that probably want to accept (Buttons). Only hot keys must be propagate until is handled or not by keyboard and not by mouse. It's only me that is seeing this?

Are you suggesting we make it so that Button.IsDefault ONLY means:"Show an indicator that you're the default button", and require devs to put their Accepting logic on the superview? I am not opposed to that idea. It means that devs will NOT be able to put "accepting" logic in btnOk.Accepting though.

I think you may not be understanding something that I take for granted: One of the BIGGEST motivators of me building out all of this command invocation logic is to enable NESTED subview scenarios, which were next to impossible to make work in v1. Examples that now not only require this, but are far simpler than previously are:

Shortcut
Bar
MenuV2 etc...
FlagSelector/OptionSelector

Command propagation up the hierarchy is required for this.

Jun 30 '25 23:06 tig

What we disagree is the need to set e.Handled = true; to avoid the default button raising the Accepting event to performing a obvious action which is executing the code that is in the Cancel button.

What code is "in the Cancel button"? What are you talking about? There is no code "in the Cancel button" in the test code above (except acceptCancel++).

You know that this is a unit test. But it could have more code, like doing some cleanup before call RequestStop.

That will be tedious to code with always the concern of set e.Handled = true; only for a default view that's only is used by the Button view.

I don't think so. If an Accepted handler does something it should indicate it did something. Thus it should set e.Handled = true. That's not tedious. That's how events in dotnet that use HandledEventArgs work. That's the design. I didn't invent that.

Accepting event should cancel the event if it don't want the Accepted event is invoked. So, in this case, you're right to set e.Handled = true, for me the more correct is e.Cancel = true, if we don't want invoke the Accepted event. The Accepting event should remain e.Handled = false if we want to invoke the Accepted event. This is also how HandledEventArgs work.

For me I only give that task to the Window class. Thus we can avoid to propagate the Accept to all views that probably want to accept (Buttons). Only hot keys must be propagate until is handled or not by keyboard and not by mouse. It's only me that is seeing this?

Are you suggesting we make it so that Button.IsDefault ONLY means:"Show an indicator that you're the default button", and require devs to put their Accepting logic on the superview? I am not opposed to that idea. It means that devs will NOT be able to put "accepting" logic in btnOk.Accepting though.

Yes, show the indicator that is the default AcceptButton and the logic is put in the btnOk.Accepting where devs must deal. The superview only has to raise the Accepting in the default button. The devs only have to code normally because the API is responsible for handle this behavior. Thus, you only concern to propagate hot keys until accept one or none and not worry about default buttons which will be the top-level responsibility.

I think you may not be understanding something that I take for granted: One of the BIGGEST motivators of me building out all of this command invocation logic is to enable NESTED subview scenarios, which were next to impossible to make work in v1. Examples that now not only require this, but are far simpler than previously are:

Shortcut

Bar

MenuV2 etc...

FlagSelector/OptionSelector

Command propagation up the hierarchy is required for this.

I understand and thank you very much for your efforts and it isn't my intention to censure for something and I beg apologies if I offend you, wasn't my intention. Without you this API won't be what is as today. Thanks.

Jul 01 '25 00:07 BDisp

This should fix the issue!

https://github.com/gui-cs/Terminal.Gui/pull/4182

Its just POC but should be doable.

If user really wants to not have Handled true they would need to suppress e.g.

        Button b = new Button ();
        #pragma warning disable TGUIG001
        b.Accepting += (s, e) => {
            Logging.Information ("hello");
                       };
        #pragma warning restore TGUIG001

Jul 01 '25 01:07 tznind

Yes, show the indicator that is the default AcceptButton and the logic is put in the btnOk.Accepting where devs must deal. The superview only has to raise the Accepting in the default button. The devs only have to code normally because the API is responsible for handle this behavior.

You've confused me with this. You wrote "Yes, ..." and then wrote exactly the opposite of what I suggested.

Thus, you only concern to propagate hot keys until accept one or none and not worry about default buttons which will be the top-level responsibility.

No, this is not correct. The "only concern" is NOT just to propagate hotkey presses. One example:

If a user double clicks on a CheckBox that is a subview of a Shortcut that is a subview of a StatusBar that is a Subview of Application.Top.

Right now, Shortcut has a bug/issue where it treats ALL clicks as Accept events. This is not right. It should be raising Activating in most cases, but that will be fixed in #4126.

It is also not correct to say "the top-level responsibility" because there are many scenarios (e.g. an overlapped View) where it is an arbitrary superview's responsibility, NOT a Toplevel.

Jul 01 '25 03:07 tig

You've confused me with this. You wrote "Yes, ..." and then wrote exactly the opposite of what I suggested.

When I said "Yes, ..." was only about to show the default indicator in the button. You know I badly spelling English and I apiologies for that. The Button IsDefault property should be only read-only to prevent that more than one or more buttons can be set with IsDefault = true. It is very common to make mistakes in this case, if the API does not prevent it. Only the Button, Toplevel and probably Popover because they are added in the Application.Top and can have a Button as sub-view, have knowledge of the IsDefault property. When I say Toplevel I mean also his derivate classes, Window and Dialog. Doesn't make sense tying the IsDefault to the ViewBase, for a feature that only Button and Toplevel knows. For this reason I said that only Toplevel know about IsDefault property and not the button superview which may not be a Toplevel. Thus, all IsDefault code in the View class must be removed and let be handled on the Button class. Adding the IDefaultAcceptView interface is complicating even more this situation. The View doesn't know what behavior of the Key.Enter in the derived class which should be responsible to handle them. The @tznind #4182 with the analyzer solution is a great feature that will help developing Terminal.Gui a lot, but in this specific case will only hiding a bad handling of the IsDefault behavior. Please try this changes below and let me know your opinion.

In the Button.cs:

    private bool? HandleHotKeyCommand (ICommandContext commandContext)
    {
        bool cachedIsDefault = IsDefaultAcceptView; // Supports "Swap Default" in Buttons scenario where IsDefaultAcceptView changes

        if (RaiseActivating (commandContext) is true)
        {
            return true;
        }

        if (!HasFocus)
        {
            SetFocus ();
        }

        if (RaiseAccepting (commandContext) is true)
        {
            return true;
        }

        if (HasFocus || cachedIsDefault || (commandContext is CommandContext<MouseBinding> { Binding.MouseEventArgs: { } } context && context.Binding.MouseEventArgs.View == this))
        {
            return true;
        }

        return false;
    }

In the View.Command.cs remove all the IDefaultAcceptView code:

    protected bool? RaiseAccepting (ICommandContext? ctx)
    {
        Logging.Debug ($"{Title} ({ctx?.Source?.Title})");
        CommandEventArgs args = new () { Context = ctx };

        // Best practice is to invoke the virtual method first.
        // This allows derived classes to handle the event and potentially cancel it.
        //Logging.Debug ($"{Title} ({ctx?.Source?.Title}) - Calling OnAccepting...");
        args.Handled = OnAccepting (args) || args.Handled;

        if (!args.Handled && Accepting is { })
        {
            // If the event is not canceled by the virtual method, raise the event to notify any external subscribers.
            //Logging.Debug ($"{Title} ({ctx?.Source?.Title}) - Raising Accepting...");
            Accepting?.Invoke (this, args);
        }

        // 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 (SuperView is { })
            {
                Logging.Debug ($"{Title} ({ctx?.Source?.Title}) - Invoking Accept on SuperView ({SuperView.Title}/{SuperView.Id})...");

                return SuperView?.InvokeCommand (Command.Accept, ctx);
            }
        }

        return args.Handled;
    }

All unit tests in the ButtonTests.cs pass including these:

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

        Application.RaiseMouseEvent (new () { ScreenPosition = new (0, 1), Flags = MouseFlags.Button1Clicked });
        Assert.Equal (0, acceptOk);
        Assert.Equal (1, acceptCancel);
        Assert.True (btnCancel.HasFocus);

        Application.End (rs);
        Application.Top.Dispose ();
        Application.ResetState ();
    }

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

        Assert.True (Application.RaiseKeyDownEvent (Key.C));
        Assert.Equal (0, acceptOk);
        Assert.Equal (1, acceptCancel);
        Assert.True (btnCancel.HasFocus);

        Application.End (rs);
        Application.Top.Dispose ();
        Application.ResetState ();
    }

    [Fact]
    [AutoInitShutdown]
    public void HotKey_From_Non_Default_Button_Raise_Accept ()
    {
        var acceptOk = 0;
        var acceptCancel = 0;
        Button btnOk = new () { Id = "Ok", Text = "_Ok" };
        btnOk.Accepting += (s, e) => acceptOk++;
        Button btnCancel = new () { Id = "Cancel", Y = 1, Text = "_Cancel" };
        btnCancel.Accepting += (s, e) => acceptCancel++;
        Application.Top = new ();
        Application.Top.Add (btnOk, btnCancel);
        var rs = Application.Begin (Application.Top);
        Assert.True (btnOk.HasFocus);

        Assert.True (Application.RaiseKeyDownEvent (Key.Enter));
        Assert.Equal (1, acceptOk);
        Assert.Equal (0, acceptCancel);
        Assert.True (btnOk.HasFocus);

        Application.End (rs);
        Application.Top.Dispose ();
        Application.ResetState ();
    }

If you want I can submit a PR to the #4126 branch to prove that and also make the IsDefault property readonly. Also I can prove that it isn't need to set Handled = true in all situations when the obvious behavior is return true if the view handle the hotkey and the Accepting event doesn't set the Handled = true.

No, this is not correct. The "only concern" is NOT just to propagate hotkey presses. One example:

If a user double clicks on a CheckBox that is a subview of a Shortcut that is a subview of a StatusBar that is a Subview of Application.Top.

Right now, Shortcut has a bug/issue where it treats ALL clicks as Accept events. This is not right. It should be raising Activating in most cases, but that will be fixed in #4126.

If these cases then you must handle as the Button does and not let the View having this responsibility. Handle the correct behavior in the derived class by returning true or false as appropriate and not only depending of the Handling = true returned by the devs.

It is also not correct to say "the top-level responsibility" because there are many scenarios (e.g. an overlapped View) where it is an arbitrary superview's responsibility, NOT a Toplevel.

That is true. In this case where may have a Button or a view which need to handle the Accept hotkey we have to define the top most arbitrary superview as Toplevel to guarantee that the Key.Enter or mouse single, double, triple click return the appropriate value. We can't use a View base class for this. When you define a View Root property it isn't mandatory to initialize it as new View, you can initialize it as new Toplevel without any error because it's derived from View class. You already know that when I'm wrong I probable doesn't recognize it early but I always end up by beg apologies when I finally recognize that I was wrong. But I still believe that I'm right in this case.

Jul 01 '25 11:07 BDisp

If you want I can submit a PR to this branch to prove...

I prefer you don't.

I am knee deep in all of this right now and that will not help.

I believe I've proven to you previously that when we have differences like this on things I am working on I do take into account your perspective.

There are other, related problems in #4126 that I'm struggling with that will get me even more confused if you start pushing code.

So let's just leave this here for now and let me make progress. In the meantime just set e.Handled in your btnCancel code.

Jul 01 '25 12:07 tig

It's normal the View.Mouse.WhenGrabbedHandlePressed method silently set focus to a view when a view don't want this behavior? Why not call the RaiseActivating first to give a chance to the view handle it first to set Handled = true to not allow set focus? Or just to be used when WantContinuousButtonPressed is true? I know you'll say why not set the view CanFocus to false, but there is situations where we want to set focus to the view tabbing with the keyboard and not set focus if it's clicked. That method force set focus that a view may want to avoid. If it's only for highlight I think there are no need to set focus but only using scheme effect for that.

Jul 01 '25 14:07 BDisp

It's normal the View.Mouse.WhenGrabbedHandlePressed method silently set focus to a view when a view don't want this behavior? Why not call the RaiseActivating first to give a chance to the view handle it first to set Handled = true to not allow set focus? Or just to be used when WantContinuousButtonPressed is true? I know you'll say why not set the view CanFocus to false, but there is situations where we want to set focus to the view tabbing with the keyboard and not set focus if it's clicked. That method force set focus that a view may want to avoid. If it's only for highlight I think there are no need to set focus but only using scheme effect for that.

I'm already working on fixing this as part of #4126

Jul 01 '25 15:07 tig

If you want to be helpful, I'd ask you to tackle

#4183

Jul 01 '25 15:07 tig

If you want to be helpful, I'd ask you to tackle

Refactor/rewrite TabView to use modern v2 capabilities #4183

I know I only be helpful when I don't disagree with you 😃

Jul 01 '25 16:07 BDisp

@tig, please, I ask you to take a look at my branch https://github.com/BDisp/Terminal.Gui/tree/v2_4050_Activate-IsDefaultView and really give me your honest opinion. I'm not asking you to accept it, but just to review carefully if I'm processing the IsDefaultView issue poorly because it really is an exception to the rule and that's why you created a specific interface for it. I added the possibility of the Button controlling the event for Command.Quit to handle it, but this change can be removed. All unit tests pass with the exception of ShortcutTests which already fail in your branch #4126 and you're probably still trying to solve. Please, don't take this request as pressure, but just to give me your honest and realistic opinion. Whatever you decide after this request of mine, it will be decided and I won't insist on this matter again. Thank you very much.

Jul 02 '25 23:07 BDisp

IsDefaultView does not mean delegating Accepting to the SuperView, ignoring the Button's Accepting, as was happening with the FileDialogExamples.cs file, where the event execution code was in win.Accepting instead of btn.Accepting. This, quite simply, was working because the Button was explicitly being ignored, due to the lack of code, and since Key.Enter is associated with Command.Accept in the View class, the Window executed it. So I ask, why put a Button with IsDefaultView as true, if the code that will be executed is that of the Window and not that of the Button, since it would be much faster than executing the SuperView's code instead of immediately executing the Button's code. This is, at least in my opinion, debatable.

Jul 03 '25 13:07 BDisp

Even if the SuperView handles Accept, the end user expects the button that accepts the state of the dialog to visually indicate it's the Accept button.

Jul 03 '25 13:07 tig

Note the MessageBox scenario.

Note how double clicking a radiolabel causes accept (shows message box), as does pressing the button. As does hitting enter on one of the textfields.

Jul 03 '25 13:07 tig