Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mouse API makes it way too hard to track button pressed #3312

Open
tig opened this issue Mar 11, 2024 · 22 comments
Open

Mouse API makes it way too hard to track button pressed #3312

tig opened this issue Mar 11, 2024 · 22 comments
Milestone

Comments

@tig
Copy link
Collaborator

tig commented Mar 11, 2024

To illustrate what I think is wrong with the current Terminal.Gui (and v1) API in this regard, consider this class.

    public class MouseDemo : View
    {
        public MouseDemo ()
        {
            CanFocus = true;
            MouseEvent += (s, e) =>
                          {
                              if (e.MouseEvent.Flags.HasFlag (MouseFlags.Button1Pressed))
                              {
                                  ColorScheme = Colors.ColorSchemes ["Toplevel"];
                              }
                              if (e.MouseEvent.Flags.HasFlag (MouseFlags.Button1Released))
                              {
                                  ColorScheme = Colors.ColorSchemes ["Dialog"];
                              }
                          };
            MouseLeave += (s, e) => { ColorScheme = Colors.ColorSchemes ["Dialog"]; };
        }
    }

I want this view to show the "TopLevel" colorscheme while button1 is pressed; when button1 is released, or the mouse leaves the view, the color should go back to "Dialog".

Here's how it behaves now. Note that if I press button1 outside of the view and drag into the view, the color changes to "Toplevel". This is not what I want. The current API provides me no way of knowing that the Button1Pressed flag was set while the mouse was outsdie of my view.

5DhZHvn 1

Now, I can work around this like this, but this is awful.

    public class MouseDemo : View
    {
        private bool _button1PressedOnEnter = false;
        public MouseDemo ()
        {
            CanFocus = true;
            MouseEvent += (s, e) =>
                          {
                              if (e.MouseEvent.Flags.HasFlag (MouseFlags.Button1Pressed))
                              {
                                  if (!_button1PressedOnEnter)
                                  {
                                      ColorScheme = Colors.ColorSchemes ["Toplevel"];
                                  }
                              }
                              if (e.MouseEvent.Flags.HasFlag (MouseFlags.Button1Released))
                              {
                                  ColorScheme = Colors.ColorSchemes ["Dialog"];
                                  _button1PressedOnEnter = false;
                              }
                          };
            MouseLeave += (s, e) =>
                          {
                              ColorScheme = Colors.ColorSchemes ["Dialog"];
                              _button1PressedOnEnter = false;
                          };
            MouseEnter += (s, e) =>
                          {
                              _button1PressedOnEnter = e.MouseEvent.Flags.HasFlag (MouseFlags.Button1Pressed);
                          };
        }
    }

FWIW, the impact of this behavior can be seen (in v1 and v2) in theWindows & FrameViews scenario.

Press button1 while in the Bounds of 1 - Windows Loop... and drag up.

ENpcvt9 1

I think there should be a View.MouseButton1Pressed and View.MouseButton1Released event.

Originally posted by @tig in #3290 (comment)

@BDisp
Copy link
Collaborator

BDisp commented Mar 11, 2024

How about only using the MouseEnter and MouseLeave events and dealing with the buttons that are pressed when they are invoked?

@tig
Copy link
Collaborator Author

tig commented Mar 11, 2024

That's what the code above does. I think having to do that is cumbersome.

@BDisp
Copy link
Collaborator

BDisp commented Mar 12, 2024

I think there should be a View.MouseButton1Pressed and View.MouseButton1Released event.

I recommend to create 3 events as MouseDown, MouseUp and MouseMove, passing the sender and the MouseEventEventArgs which can handle any button. The MouseMove can have button pressed or not, since there are report mouse position.

@dodexahedron
Copy link
Collaborator

That's what the code above does. I think having to do that is cumbersome.

Agreed it's cumbersome and even a little odd for that function.

One thing though: For any new or modified code: Please use actual named private methods (not lambdas) when subscribing to public events in library code. Additionally, making those methods static will both perform better (both CPU and heap memory impact) and force or encourage better and more testable code for the event and whatever code caused it to be raised.

And remember that any single instance of any delegate can target 0 to n methods, so don't assume that your subscriber is the only one, the first one, nor that it will even get called at all or exactly once, if part of a public event.

For code that doesn't do those things when I get to events, it will be done at that time, so don't worry about it for existing code if you don't feel like it (though it will make those diffs easier to grok when they come). But please do for new work. 🙏

@BDisp
Copy link
Collaborator

BDisp commented Apr 8, 2024

Here's how it behaves now. Note that if I press button1 outside of the view and drag into the view, the color changes to "Toplevel". This is not what I want. The current API provides me no way of knowing that the Button1Pressed flag was set while the mouse was outsdie of my view.

@tig you want when press button 1 in the view changes color to Toplevel and when drag outside with the button pressed, changes the color to Dialog and when drag again into the view with the button still pressed, change the color to Toplevel. Right?

@tig
Copy link
Collaborator Author

tig commented Apr 8, 2024

I don't think asking the question that way makes sense.

As a View, i want to know when the user has pressed a button down.

Then I want to know the user has kept the button pressed (regardless of whether the mouse was moved or not).

The current api only gives me the later.

@BDisp
Copy link
Collaborator

BDisp commented Apr 8, 2024

I don't think asking the question that way makes sense.

As a View, i want to know when the user has pressed a button down.

Imagine the button was pressed on a view and the MouseGrabView is set to that view with WantContinousButtonPressed as true.

Then I want to know the user has kept the button pressed (regardless of whether the mouse was moved or not).

The task ProcessContinuousButtonPressedAsync or the ButtonPressed+ReportMousePosition will continue raise the MouseEvent. Then we check if the View poroperty is the same as MouseGrabView.

The current api only gives me the later.

With WantContinousButtonPressed as true you will always have mouse events raised.

@tig
Copy link
Collaborator Author

tig commented Apr 9, 2024

I don't think you're getting what I'm puttin' down.

This has nothing to do with MouseGrabView or WantContinuous.

Today if a user presses the mouse button, moves it a bit, and then releases it, Applicaiton.OnMouseEvent is called with:

MouseButton1Pressed 
MouseButton1Pressed | ReportMousePosition
MouseButton1Pressed | ReportMousePosition
...
MouseButton1Released
MouesButton1Clicked

What I want is:

MouseButton1Pressed 
MouseButton1Down | ReportMousePosition
MouseButton1Down | ReportMousePosition
...
MouseButton1Released
MouseButton1Clicked

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

I'm my opinion it isn't the mouse API that have to track those behavior but the View itself. The MouseButton1Pressed or the MouseButton1Down is the same.
When I open the Buttons scenario it still is having the MouseButton1Pressed | ReportMousePosition. I'll try to find why, but I suspect that have to do with Windows Terminal.

@tig
Copy link
Collaborator Author

tig commented Apr 9, 2024

I'm my opinion it isn't the mouse API that have to track those behavior but the View itself. The MouseButton1Pressed or the MouseButton1Down is the same.

I'd like to undertstand WHY you think this.

I disagree because:

The ideal way to design a UI library API is to have the API match how end-users think. End users, using mice, think in terms of

  • I press the mouse button down to start an operation
  • I move the mouse with the mouse button already pressed
  • I release the mouse button
  • I may continue moving the mouse after releasing the button

The current Terminal.Gui API does not follow this at all. It provides no event for the first action.

Other mouse APIs do. For example Windows provides WM_LBUTTONDOWN which is only sent when the mouse button first goes down.

WindowsForms: MouseDown is only fired when the mouse is first pressed.

I propose the View API looks like this.

// Passes all Application mouse events on; lets views have total flexiblity
protected internal virtual bool OnMouseEvent (MouseEvent mouseEvent); 
 // When the user first presses the button down over a view
protected internal virtual bool OnMouseDown (MouseEvent mouseEvent);
// When the user moves the mouse over a view, or if mouse was grabbed by the view. Flags will indicate if a button is down.
protected internal virtual bool OnMouseMove (MouseEvent mouseEvent);
// When the user lets go of the mouse button. Only received i the mouse is over the view or it was grabbed by the view.
protected internal virtual bool OnMouseUp (MouseEvent mouseEvent);
// When the user presses down and then releases the mouse over a view (they could move off in between). 
// If they press and release multiple times in quick succession this event will be called for each up action
protected internal virtual bool OnMouseClick (MouseEvent mouseEvent);
// When the user presses and releases the mouse button twice in quick succession without moving the mouse outside the view
protected internal virtual bool OnMouseDoubleClick (MouseEvent mouseEvent);
// When the user presses and releases the mouse button thrice in quick succession without moving the mouse outside the view
protected internal virtual bool OnMouseTripleClick (MouseEvent mouseEvent);

The logic for determining which of these methods to call could either be in Application.OnMouseEvent or in View.NewMouseEvent.

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

Describe the bug The clicked event should only occur if the view mouse button was pressed and released on the same view, not at same same coordinates as it happened previously. The terminal mouse API should raise the clicked event but is View responsibility to send it to the valid view. The terminal mouse API don't know about the View class and only send the clicked event because a Pressed+Release mouse button occurred.
To Reproduce Steps to reproduce the behavior:

  1. Go to 'UICatalog.'
  2. Open the 'Buttons' scenario
  3. Check if the first 'TextField' is selected and press the mouse on it
  4. Kept the mouse pressed and move the mouse to any of the buttons and release the mouse
  5. The Dialog is opened even the mouse button pressed and released wasn't in that view
  6. This doesn't happens if you press the mouse on a Button and release it on another Button, which is good

Expected behavior The clicked event should not occurred.

The cause is:

Another way to reproduce it:

  1. Run UI Catalog
  2. Ctrl-A to open about box
  3. Press and hold mouse outside of about box
  4. Drag over about box

Expected:

  • Nothing happens

Actual:

  • About box moves

XDOIDNj 1

I think in this case isn't totally a bug because none of the outer views want handle the mouse and the Toplevel also grab the mouse on Button1Pressed | ReportMousePosition.

If you move the mouse without press any button and while the mouse is moving you press the Button1, you will get a Button1Pressed | ReportMousePosition event and never a Button1Pressed alone. I don't think this is very easy to be to bypass this.

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

I'll will provide my workaround for this soon, in 1 or 2 hours.

@tig
Copy link
Collaborator Author

tig commented Apr 9, 2024

I'll will provide my workaround for this soon, in 1 or 2 hours.

Why waste time on a work around? We should fix the core issue in a well thought through way.

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

I'll will provide my workaround for this soon, in 1 or 2 hours.

Why waste time on a work around? We should fix the core issue in a well thought through way.

Can't you wait too see it, at least. If you don't like you'll say it for sure.

@tig
Copy link
Collaborator Author

tig commented Apr 9, 2024

I'll will provide my workaround for this soon, in 1 or 2 hours.

Why waste time on a work around? We should fix the core issue in a well thought through way.

Can't you wait too see it, at least. If you don't like you'll say it for sure.

If it's a holistic solution that addresses the fundamental problem with the current API then absolutely! If it's hack/workaround, then I'm not interested.

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

I'm my opinion it isn't the mouse API that have to track those behavior but the View itself. The MouseButton1Pressed or the MouseButton1Down is the same.

I'd like to undertstand WHY you think this.

I disagree because:

The ideal way to design a UI library API is to have the API match how end-users think. End users, using mice, think in terms of

  • I press the mouse button down to start an operation

While the mouse is moving and a button is pressed you never will have a unit flag. For this you have to stop the mouse and then press a button.

  • I move the mouse with the mouse button already pressed

Ok. If you in the previous step you only get one flag now you'll have two

  • I release the mouse button
  • I may continue moving the mouse after releasing the button

If the button is released while the mouse is moving sometimes the released event doesn't occurs. In CursesDriver is absolutely sure it will not be raised.

The current Terminal.Gui API does not follow this at all. It provides no event for the first action.

Ok after the fixes in the #3393 those events can be created on the ViewMouse.cs.

Other mouse APIs do. For example Windows provides WM_LBUTTONDOWN which is only sent when the mouse button first goes down.

They use low level code for that. Here you have to deal with what the drivers provides.

WindowsForms: MouseDown is only fired when the mouse is first pressed.

Here if you are moving the mouse when the button is pressed you'll have ButtonPressed ! ButtonReportMousePosition.

I propose the View API looks like this.

// Passes all Application mouse events on; lets views have total flexiblity
protected internal virtual bool OnMouseEvent (MouseEvent mouseEvent); 
 // When the user first presses the button down over a view
protected internal virtual bool OnMouseDown (MouseEvent mouseEvent);
// When the user moves the mouse over a view, or if mouse was grabbed by the view. Flags will indicate if a button is down.
protected internal virtual bool OnMouseMove (MouseEvent mouseEvent);
// When the user lets go of the mouse button. Only received i the mouse is over the view or it was grabbed by the view.
protected internal virtual bool OnMouseUp (MouseEvent mouseEvent);
// When the user presses down and then releases the mouse over a view (they could move off in between). 
// If they press and release multiple times in quick succession this event will be called for each up action
protected internal virtual bool OnMouseClick (MouseEvent mouseEvent);
// When the user presses and releases the mouse button twice in quick succession without moving the mouse outside the view
protected internal virtual bool OnMouseDoubleClick (MouseEvent mouseEvent);
// When the user presses and releases the mouse button thrice in quick succession without moving the mouse outside the view
protected internal virtual bool OnMouseTripleClick (MouseEvent mouseEvent);

The logic for determining which of these methods to call could either be in Application.OnMouseEvent or in View.NewMouseEvent.

With the fixes I provided I think it's more easy to do them in the View.NewMouseEvent. The Enter and Leave events are raised from the Application.OnMouseEvent. The others you described above could be implemented in the ViewMouse.cs.

But that I'll leave with you because it's better you continue those implementations.
You could be less selfish and value the efforts of those who contribute to this magnificent library.

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

It is also possible to create a new property in the MouseEvent class for IsMouseDown to handle the first mouse press and sent by the Application.OnMouseEvent method.

In the View.NewMouseEvent we can read this property to call the OnMouseDown and OnMouseUp and invoke the MouseDown and MouseUp. events. But I think it should have a triple state by adding OnMouseNone to distinguish from the first button pressed and last button released.
In the Application.OnMouseEvent it can be handled with a bool? _isMouseDown, where true is mouse down, false mouse up and null is none. It's needed to define where _isMouseDown = null is defined, on the ButtonReleased or the ButtonClicked. In my opinion it should be in the ButtonReleased for the actions can be take immediately.

What do you think?

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

I think that with the changes I've made it's much more easy for you to implement the methods and events as you whish.

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

I also will add the above methods and events in this PR as well.

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

I think is all done.

@tig
Copy link
Collaborator Author

tig commented Apr 9, 2024

You could be less selfish and value the efforts of those who contribute to this magnificent library.

I am sorry you feel I'm acting selfish. I do not believe I am. I am doing my best to ensure this project is successful and I strive to focus on the technical issues, not personality.

I am surprised you think I don't value your efforts. I feel like I regularly take the time to thank you for your hard work and great ideas. I can't help how you feel though, but I am motivated to keep trying to do the right thing.

@BDisp
Copy link
Collaborator

BDisp commented Apr 9, 2024

I am sorry you feel I'm acting selfish. I do not believe I am. I am doing my best to ensure this project is successful and I strive to focus on the technical issues, not personality.

I am surprised you think I don't value your efforts. I feel like I regularly take the time to thank you for your hard work and great ideas. I can't help how you feel though, but I am motivated to keep trying to do the right thing.

I know you are trying to do the right thing, as everyone who contributes also tries, but you already make value judgments without letting anyone present their solutions, discouraging them from even presenting them. That's what I disagree with you about.

@tig tig added this to the V2 Beta milestone May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: 📋 Approved - Need Owner
Development

No branches or pull requests

3 participants