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

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

Draft
wants to merge 19 commits into
base: v2_develop
Choose a base branch
from

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Apr 9, 2024

Fixes

Proposed Changes/Todos

  • Add mouse clicked logic for only to raise on the same view that pressed it
  • If a mouse button was pressed and grabbed the mouse, the OnMouseEnter isn't ran. Now it run
  • Created the ProcessMouseEvent method allowing reuse the code
  • Add unit test
  • Add hot color for highlight
  • Some formatting

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not had a chance to fully review this, nor run it to see how it works. So I can't comment on whether it works.

But i do not think it actually makes tracking button presses easier, and I do not like aspects of the design. I've commented on those in my code review.

Terminal.Gui/Application.cs Outdated Show resolved Hide resolved
Terminal.Gui/Application.cs Outdated Show resolved Hide resolved
Terminal.Gui/Input/Mouse.cs Show resolved Hide resolved
@BDisp BDisp marked this pull request as draft May 2, 2024 11:02
@BDisp BDisp marked this pull request as ready for review May 2, 2024 14:20
Terminal.Gui/Application.cs Outdated Show resolved Hide resolved
Terminal.Gui/Application.cs Outdated Show resolved Hide resolved
Terminal.Gui/Application.cs Outdated Show resolved Hide resolved
@BDisp
Copy link
Collaborator Author

BDisp commented Jun 9, 2024

@tig please say if this PR is of interest or not. If it is not necessary I will no longer waste time on it and will close it. Thanks.

@tig
Copy link
Collaborator

tig commented Jun 9, 2024

@tig please say if this PR is of interest or not. If it is not necessary I will no longer waste time on it and will close it. Thanks.

I think we should fix this issue as well as the other deep design issues the current mouse support has.

I would love it if you tackled that.

However, I feel like you ignored a bunch of feedback I gave in my review.

I'm not sure what to do about that.

Would you like me to do a fresh review of this after you fix the merge conflict?

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 9, 2024

Would you like me to do a fresh review of this after you fix the merge conflict?

Yes, I'll fix the merge conflicts, again. Thanks.
But if you also have other better options to improve this, I can close this PR and you can create another one, no problem.

@tig
Copy link
Collaborator

tig commented Jun 9, 2024

I think we can use this PR as the base.

Here's a recap of my opinions on this:

We already have MouseFlags.Button1Pressed. The ENTIRE point of this Issue (#3312) is that we have confused the meaning of Pressed.

Instead of a design that reflects how users actually use the mouse, we have a mess.

"Pressed" is a past-tense of the verb "Press". The "-ed" indicates the "Press" happened in the past. Thus "Pressed" LITERALLY means "Is Currently Down".

"Released" is also past-tense. It means "Is No Longer Down".

I believe the terminology we should use (and the designers of Windows, WinForms, Mac Carbon, and most other UI libraries I've worked with) should distinguish, clearly, between

  • The button WAS not pressed, but it just became pressed. Down makes the most sense to me, but the verb Press works too.
  • The button was previously pressed, and continues to be so: Pressed
  • The button was previously pressed, but it has just become un-pressed: Released

You are proposing to make things even more confusing. What this code says is:

  • The button WAS not pressed, but it just became pressed. Pressed.
  • The button was previously pressed, and continues to be so: IsMouseDown
  • The button was previously pressed, but it has just become un-pressed: Released

This may seem sublte and insignificant to you, but I don't think it is.

And...

Let's try this... pretend we are building a new View that wants the following user behavior:

The view shows the following counters:

  • "clicked"
  • "doubleclicked"
  • "tripleclicked"
  • "down"
  • "pressed"
  • "up"

Assume the view only cares about Button1.

  • If the user does a typical "mouse click", increment the "clicked" counter by one.
  • If the user repeatedly does a "mouse click" 10 times in a row, the "clicked" counter increases by 10
  • If the user does a "mouse click" twice in a row, the "clicked" counter increases by 2 and the "doubleclicked" counter increases by one
  • If the user does a "mouse click" three times in a row, the "clicked" counter increases by 3, and the "tripleclicked" counter increases by one.
  • If the users presses the mouse down, wiggles it (inside or outside the view) and then releases it, the "down" counter increases by one, the "pressed" counter increases by some number (depends on how long), and the "up" counter increases by one.

In C#, the setup code for this View would look like this:

MouseDown += Handle_Down;
MouseUp += Handle_Down;
MouseClicked += Handle_Clicked;
MouseEvent += HandleEvent;

What would the logic in Handle_Down look like, ideally?

  • It needs to know what mouse button was involved (because I stated a requirement for this view that it only cares about button1).

Should that be if (e.HasFlag(Button1Pressed))? Or if (e.HasFlag(Button1)? Or if (e.Button1 == true)? Or something else?

I don't have time right now to ask the same level of questions about MouseUp/MouseDown etc..., but in my mind that's how we figure out what the right design is.

I think after fixing the merge conflict, you should build a new Scenario that implements the View I describe above.

We can then use that to debate/refine the design and API.

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 9, 2024

MouseUp += Handle_Down;

I think you meant what's below, right?

MouseUp += Handle_Up;

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 9, 2024

Done @tig.

@tig
Copy link
Collaborator

tig commented Jun 24, 2024

(Nevermind).

@tig
Copy link
Collaborator

tig commented Jun 26, 2024

How's your progress on this coming @BDisp? Should it be marked as a draft for now?

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 26, 2024

How's your progress on this coming @BDisp? Should it be marked as a draft for now?

I haven't worked on this for a while now. I'll mark it as a draft and take a good look at what you intend, including the scenario. But in my opinion this can only be handled in Application.OnMouseEvent but then don't complain as there could probably be even more static variables.

@BDisp BDisp marked this pull request as draft June 26, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants