`View.Mouse` raises `MouseClick` 3 times when it should just raise it once

[BDisp]

There is one situation in Terminal.Gui which make me crazy. A simple Accepting event which will allow the developer to Cancel the default behavior (but has the Handled property) will repeat the event 3 times by mouse if the developer doesn't Cancel it. UICatalog has a bunch of Button's that if they are activated by mouse will repeat the event 3 times, no matter the user press Key.Enter or Key.Esc. Despite by adding Handled = true fix this, there is no necessity of this extra code for a normal Accepting event. We can't handle mouse event as key event because only the only the view where the mouse is will be executed and doesn't not propagate to other views and with key event it can be propagate to other views that may want to handle it. I agree the leverage of keybindings, commands and context to achieve this behavior but we need to handle the mouse with much more careful.

Originally posted by @BDisp in https://github.com/gui-cs/Terminal.Gui/issues/4126#issuecomment-2976658963

I understand that the current implementation is correct to handle adornments and all the views under the mouse but the Button must be responsible to set e.Handled = true; after the MouseClickevent. to prevent further processing to itself.

[BDisp]

Also fix this.

with this code:

public class ExampleWindow : Window
{
    public static string UserName { get; set; }

    public ExampleWindow ()
    {
        Title = $"Example App ({Application.QuitKey} to quit)";

        // Create input components and labels
        var usernameLabel = new Label { Text = "Username:" };

        var userNameText = new TextField
        {
            // Position text field adjacent to the label
            X = Pos.Right (usernameLabel) + 1,

            // Fill remaining horizontal space
            Width = Dim.Fill ()
        };

        var passwordLabel = new Label
        {
            Text = "Password:", X = Pos.Left (usernameLabel), Y = Pos.Bottom (usernameLabel) + 1
        };

        var passwordText = new TextField
        {
            Secret = true,

            // align with the text box above
            X = Pos.Left (userNameText),
            Y = Pos.Top (passwordLabel),
            Width = Dim.Fill ()
        };

        // Create login button
        var btnLogin = new Button
        {
            Text = "Login",
            Y = Pos.Bottom (passwordLabel) + 1,

            // center the login button horizontally
            X = Pos.Center (),
            IsDefault = true
        };

        // When login button is clicked display a message popup
        btnLogin.Accepting += (s, e) =>
                           {
                               if (userNameText.Text == "admin" && passwordText.Text == "password")
                               {
                                   MessageBox.Query ("Logging In", "Login Successful", "Ok");
                                   UserName = userNameText.Text;
                                   Application.RequestStop ();
                               }
                               else
                               {
                                   MessageBox.ErrorQuery ("Logging In", "Incorrect username or password", "Ok");
                               }
                           };

        // Create StatusBar
        StatusBar statusBar = new ()
        {
            Visible = true,
            AlignmentModes = AlignmentModes.IgnoreFirstOrLast,
            CanFocus = false
        };

        Shortcut shortcut = new ()
        {
            Title = "Shortcut",
            CanFocus = false
        };
        statusBar.Add (shortcut);

        // Add the views to the Window
        Add (usernameLabel, userNameText, passwordLabel, passwordText, btnLogin, statusBar);
    }

[tig]

I believe the fix to the MouseClick being raised 3 times is this, in HandleMouseGrab()

-            if (MouseGrabView?.NewMouseEvent (viewRelativeMouseEvent) is true)
-            {
-                return true;
-            }
+            if (MouseGrabView?.NewMouseEvent (viewRelativeMouseEvent) is true || viewRelativeMouseEvent.IsSingleDoubleOrTripleClicked)
+            {
+               // If the view that grabbed the mouse handled the event OR it was a click we are done.
+               return true;
+            }

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.

[tznind]

(move discussion)

[tig]

This test reproduces:

 [Theory]
 [InlineData (MouseState.None)]
 [InlineData (MouseState.In)]
 [InlineData (MouseState.Pressed)]
 [InlineData (MouseState.PressedOutside)]
 public void RaiseMouseEvent_ButtonClicked_Raises_MouseClick_Once (MouseState states)
 {
     Application.Init (new FakeDriver ());

     Application.Top = new Toplevel ()
     {
         Id = "top",
         Height = 10,
         Width = 10
     };

     var view = new View
     {
         Width = 1,
         Height = 1,
         WantContinuousButtonPressed = false,
         HighlightStates = states
     };
     Application.Top.Add (view);
     Application.LayoutAndDraw ();

     var clickedCount = 0;
     view.MouseClick += (s, e) => clickedCount++;

     var me = new MouseEventArgs ();
     Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Pressed, });
     Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Released, });
     Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Clicked, });

     Application.Top.Dispose ();
     Application.ResetState (true);

     Assert.Equal (1, clickedCount);
 }

Fails for all except MouseState.None (clickedCount == 3).

[BDisp]

I think this is more correct by using MouseBindings and Accepting event.

    [Theory]
    [InlineData (MouseState.None)]
    [InlineData (MouseState.In)]
    [InlineData (MouseState.Pressed)]
    [InlineData (MouseState.PressedOutside)]
    public void RaiseMouseEvent_ButtonClicked_Raises_MouseClick_Once (MouseState states)
    {
        Application.Init (new FakeDriver ());

        Application.Top = new Toplevel ()
        {
            Id = "top",
            Height = 10,
            Width = 10
        };

        var view = new View
        {
            Width = 1,
            Height = 1,
            WantContinuousButtonPressed = false,
            HighlightStates = states
        };

        view.MouseBindings.ReplaceCommands (MouseFlags.Button1Clicked, Command.Accept);

        Application.Top.Add (view);
        Application.LayoutAndDraw ();

        var clickedCount = 0;
        view.Accepting += (s, e) => clickedCount++;

        var me = new MouseEventArgs ();
        Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Pressed, });
        Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Released, });
        Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Clicked, });

        Application.Top.Dispose ();
        Application.ResetState (true);

        Assert.Equal (1, clickedCount);
    }

[tig]

I think this is more correct by using MouseBindings and Accepting event.

Not really. The bug is in the MouseClick code. It is independent of how Accept works. Sure, your version shows there's a bug, and passes when the bug is fixed, but it is operating at a higher-level.

Note that there's actually TWO bugs:

1. HandleMouseGrab as described above

Fixing that reduces the # of times MouseClick is raised by one to 2.

2. This code in View.WhenGrabbedHandleClicked():

- // If mouse is still in bounds, generate a click
+ // If mouse is still in bounds, return false to indicate a click should be raised.
if (!WantMousePositionReports && Viewport.Contains (mouseEvent.Position))
{
-     return RaiseMouseClickEvent (mouseEvent);
+         return false;
}

By changing both of these, my new test, as well as yours pass.

There's yet another bug in here that I don't yet have a fix for, but leads to confusion:

The reason Button needs to subscribe to MouseClick instead of registering a MouseBinding is InvokeCommandsBoundToMouse is never called in the WantContinuousButtonPressed code path. So if WantContinuousButtonPressed is true (e.g. for NumericUpDown) using MouseBindings doesn't work.

[tig]

Quesiton:

Right now we generate MouseClick for any mouse button. The Button code does not distinguish. Thus this test passes:

    [InlineData (MouseFlags.Button1Pressed, MouseFlags.Button1Released, MouseFlags.Button1Clicked)]
    [InlineData (MouseFlags.Button2Pressed, MouseFlags.Button2Released, MouseFlags.Button2Clicked)]
    [InlineData (MouseFlags.Button3Pressed, MouseFlags.Button3Released, MouseFlags.Button3Clicked)]
    [InlineData (MouseFlags.Button4Pressed, MouseFlags.Button4Released, MouseFlags.Button4Clicked)]
    public void WantContinuousButtonPressed_True_ButtonClick_Accepts (MouseFlags pressed, MouseFlags released, MouseFlags clicked)
    {

Shouldn't Button ONLY respond to Button1 by default? Is there a good reason it supports all 4 buttons?

[BDisp]

Shouldn't Button ONLY respond to Button1 by default? Is there a good reason it supports all 4 buttons?

I don't see any reason to support all them.

[tig]

Another question:

View is pretty standard for built-in command support:

        KeyBindings.Add (Key.Space, Command.Activate);
        KeyBindings.Add (Key.Enter, Command.Accept);

IOW, Space is "activate something" and Enter is "accept it".

View is a mess right now:

        MouseBindings.Add (MouseFlags.Button1Clicked, Command.Activate);
        MouseBindings.Add (MouseFlags.Button1Clicked | MouseFlags.ButtonCtrl, Command.Activate);

...

// a bunch of code that SOMETIMES raises `Activate` and sometimes raises `MouseClick` including:

 protected bool RaiseMouseClickEvent (MouseEventArgs args)
 {
     // Pre-conditions
     if (!Enabled)
     {
         // QUESTION: Is this right? Should a disabled view eat mouse clicks?
         return args.Handled = false;
     }

     Logging.Debug ($"{args.Flags};{args.Position}");

     // Cancellable event
     if (OnMouseClick (args) || args.Handled)
     {
         return args.Handled;
     }

     MouseClick?.Invoke (this, args);

     if (args.Handled)
     {
         return true;
     }

     // Post-conditions

     // By default, this will raise Activating/OnActivating - Subclasses can override this via AddCommand (Command.Select ...).
     args.Handled = InvokeCommandsBoundToMouse (args) == true;

     return args.Handled;
 }

I think the OnMouseClick/MouseClick can be deleted and the default behavior be:

        MouseBindings.Add (MouseFlags.Button1Clicked, Command.Activate);
        MouseBindings.Add (MouseFlags.Button1DoubleClicked, Command.Accept);

RaiseMouseClickEvent is not needed; the code that calls it can just call RaiseActivating.

[BDisp]

RaiseMouseClickEvent is not needed; the code that calls it can just call RaiseActivating.

Probably not because it will invoke mouse binding command with the passed mouse events flags. It's necessary always to pass the right type to the methods so that's possible to check the right type for take the right action.

[tig]

RaiseMouseClickEvent is not needed; the code that calls it can just call RaiseActivating.

Probably not because it will invoke mouse binding command with the passed mouse events flags. It's necessary always to pass the right type to the methods so that's possible to check the right type for take the right action.

I don't understand.

We can pass the mouseventargs in the command context.

[BDisp]

Sorry, my bad. I meant.

Probably not needed to use RaiseMouseClickEvent because it will invoke mouse binding command with the passed mouse events flags. It's necessary always to pass the right type to the methods so that's possible to check the right type for take the right action.

[BDisp]

This test reproduces:

[Theory] [InlineData (MouseState.None)] [InlineData (MouseState.In)] [InlineData (MouseState.Pressed)] [InlineData (MouseState.PressedOutside)] public void RaiseMouseEvent_ButtonClicked_Raises_MouseClick_Once (MouseState states) { Application.Init (new FakeDriver ());

 Application.Top = new Toplevel ()
 {
     Id = "top",
     Height = 10,
     Width = 10
 };

 var view = new View
 {
     Width = 1,
     Height = 1,
     WantContinuousButtonPressed = false,
     HighlightStates = states
 };
 Application.Top.Add (view);
 Application.LayoutAndDraw ();

 var clickedCount = 0;
 view.MouseClick += (s, e) => clickedCount++;

 var me = new MouseEventArgs ();
 Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Pressed, });
 Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Released, });
 Application.RaiseMouseEvent (new MouseEventArgs () { Flags = MouseFlags.Button1Clicked, });

 Application.Top.Dispose ();
 Application.ResetState (true);

 Assert.Equal (1, clickedCount);

} Fails for all except MouseState.None (clickedCount == 3).

This unit test does not reproduce the real problem of this question. The OP (original post) is about triggering two additional Accepting events by just using MouseFlags.Button1Clicked. I've tried every way but I'm pretty sure it's not reproducible through unit tests but only through app UI.

[tig]

@BDisp please pull down the latest #4126 and run Generic.cs.

In that PR I now have several new unit tests that I believe prove:

1. The bug could be reproduced with unit tests.
2. The bug is in a combination of the Application.Mouse and View.Mouse handling
3. I have fixed the bug
4. There are now unit tests that prove the bug is fixed.

I have not yet removed the MouseClick/OnMouseClick events but plan on doing so.

[BDisp]

Can you please tell me what the name of the unique unit test that can prove that can be reproducible this issue?

[tig]

WantContinuousButtonPressed_True_Button_Press_Repeatedly_Raises_MouseClick_Repeatedly

in MouseTests.cs

[BDisp]

WantContinuousButtonPressed_True_Button_Press_Repeatedly_Raises_MouseClick_Repeatedly

Sorry @tig. That does not reproduces this issue because my issue it's only using Button1Clicked flag and it open 3 times the MessageBox. I paused the debug and the MouseGrabView is null. Your unit test prove also a issue but not related with the issue of this OP.

[tig]

So when you run Generic.cs and click with button1 on the Button, it opens the message box 3 times?

[tig]

I think the OnMouseClick/MouseClick can be deleted and the default behavior be:

    MouseBindings.Add (MouseFlags.Button1Clicked, Command.Activate);
    MouseBindings.Add (MouseFlags.Button1DoubleClicked, Command.Accept);

RaiseMouseClickEvent is not needed; the code that calls it can just call RaiseActivating.

I've changed my mind on this. I went through the process of removing OnMouseClick/MouseClick and OnActivating/Activating CAN replace it, but in some cases it's pretty inconvenient. So I'm leaving it in but making the API docs more clear.

[BDisp]

So when you run Generic.cs and click with button1 on the Button, it opens the message box 3 times?

Yes. Also with the changed Generic scenario if you click on the shortcut without action will also open the message box. If you click in the shortcut with action will run the button first and then run the shortcut action.

[BDisp]

The reason of place a TextField at beginning of the Generic scenario was for test for default button, otherwise the button will have always the focus.

[BDisp]

So when you run Generic.cs and click with button1 on the Button, it opens the message box 3 times?

I see now that you ask this related with your PR. Button is only open once and the shortcut without action is opening the button message box.

[tig]

So when you run Generic.cs and click with button1 on the Button, it opens the message box 3 times?

I see now that you ask this related with your PR. Button is only open once

So, do you now agree that I've fixed it?

and the shortcut without action is opening the button message box.

That is issue #4170

[BDisp]

Yes 👏