Fixes #2579. MouseEvent/OnMouseEvent are stoopid

[tig]

Fixes

• Fixes Responder.MouseEvent and View.OnMouseEvent are backwards - Refactor #2579

Proposed Changes/Todos

• Move MouseEvent from Responder to View
• Combine View.OnMouseEvent and MouseEvent into one method (called View.OnMouseEvent).
• Clean up MouseClick event related things
  • OnMouseClick -> protected
  • Updated API docs
  • MouseClick occurs for ANY click event (any button, single or double)
• Make OnMouseXX protected internal` as a quick-fix workaround that at least makes them not public, so work can continue.

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

[dodexahedron]

Not sure I love the consolidation of events goal there.

If I want clicks, I don't want other mouse events. That's noise and I have to filter for it.

The standard across many other frameworks, in multiple languages tends to be providing quite a few events, around mouse operations, such as MouseButtonDown, MouseButtonUp, Clicked, DoubleClicked, and pre-events for each of those, as well, to provide an intercept point if something needs to hijack and/or cancel it all.

Heck, even look at what javascript has for standard web interactions on every displayable element in the DOM:

https://www.w3schools.com/jsref/obj_mouseevent.asp

[BDisp]

Is a question on how the user want to handle it. All these events already exist on the OnMouseEvent, OnMouseEnter and OnMouseLeave, with the addition of mouse wheeling. The user can check for a button pressed, released, clicked, double clicked and triple clicked, within a left, right or middle button.

[tig]

Is a question on how the user want to handle it. All these events already exist on the OnMouseEvent, OnMouseEnter and OnMouseLeave, with the addition of mouse wheeling. The user can check for a button pressed, released, clicked, double clicked and triple clicked, within a left, right or middle button.

Right. But the API for that is clumsy because it involves testing flags. See the cruft a the top of each OnMouseEvent override of "if not all these flags I don't care about".

I'm prob not going to address this here but it is ugly.

[BDisp]

Right. But the API for that is clumsy because it involves testing flags. See the cruft a the top of each OnMouseEvent override of "if not all these flags I don't care about".

Each derived view must know what mouse events flags will be needed. They don't needed to tests for all flags they don't want. Only is needed to handle what they needed returning true and return false on the end or call the base class on flags that aren't needed. The reason they check for all flags they don't needed is for set focus on the view which is bad and can be done by call a method that do that on each flag the view need.

I'm prob not going to address this here but it is ugly.

I agree, because the flags are checked twice, first to reject the unneeded flags and second to process each one that was referenced in the first. Is really ugly.

Another flags users may need is check if the Ctrl, Alt, Shift keys were pressed along the mouse event.

[BDisp]

@tig did you already notice WindowsDriver is always sending mouse button pressed + report mouse position when enter and after closing a scenario, when only the mouse is moving without pressing any button? This isn't related with Terminal.Gui because isn't happening on conhost but only on Windows Terminal.

[tig]

@tig did you already notice WindowsDriver is always sending mouse button pressed + report mouse position when enter and after closing a scenario, when only the mouse is moving without pressing any button? This isn't related with Terminal.Gui because isn't happening on conhost but only on Windows Terminal.

No, but thanks for the head's up.

[tig]

Not sure I love the consolidation of events goal there.

If I want clicks, I don't want other mouse events. That's noise and I have to filter for it.

The standard across many other frameworks, in multiple languages tends to be providing quite a few events, around mouse operations, such as MouseButtonDown, MouseButtonUp, Clicked, DoubleClicked, and pre-events for each of those, as well, to provide an intercept point if something needs to hijack and/or cancel it all.

Heck, even look at what javascript has for standard web interactions on every displayable element in the DOM:

https://www.w3schools.com/jsref/obj_mouseevent.asp

For now, I've met in the middle:

• Use MouseClick if you care about single or double clicks of any button. Check MouseEvent.Flags to determine which button was clicked.
• Use MouseEvent for all other mouse events.

We can add additional, commonly used events, like WheeledUp/Down later).

[tig]

@BDisp & @dodexahedron this is ready for review.

Future PRs will:

• Make OnMouseXXX follow the correct pattern per Refactor and align event handling with common/best practices #3209

[BDisp]

Good work. I only did some questions.

[dodexahedron]

I started writing this a couple nights ago and apparently forgot to actually submit the comment. 😅 So... Here ya go...

Is a question on how the user want to handle it. All these events already exist on the OnMouseEvent, OnMouseEnter and OnMouseLeave, with the addition of mouse wheeling. The user can check for a button pressed, released, clicked, double clicked and triple clicked, within a left, right or middle button.

Right. And there's no reason there can't also be a combined event like that (though it is kinda bad form).

Lots of words that amount to "don't abuse events that way without a REALLY good reason." Expand if you like staring at diatribes in github comments.

One of many problem is that having one event that covers multiple concepts creates a lot of unnecessary work for the publisher and all subscribers, when a subscriber only wants some or one of the events. It also leads to `EventHandler` delegates and their `T`s having to carry around dead weight. That's generation of garbage, heap allocations, and unclear behavior (and/or the need to document it heavily) in those cases where one or more fields, properties, or methods on that `T` are not relevant to a given event, yet the consumer can still interact with them. To prevent that requires throwing exceptions, but you're in an event handler and that's program crash territory, now (which is why you're not supposed to throw there).

And, as a consumer of an API, I'm pretty annoyed when the API makes me do work that costs several lines at each event subscriber method that has to now do the work that events themselves already implicily would have done, when it would have cost the creator of the library just a few lines, per event, once per event, to avoid it all.

If I, the consumer, need all of those events, I just subscribe to them all.

Otherwise, why not simply have one delegate for the entire program called "OnStuff?" While that sounds absurd, that's the point. It is absurd, especially for what are already common/standard/expected behaviors. It costs us basically no effort (sometimes less, if you end up avoiding extra types to deal with handling dispatch....instead of letting events do what they do, which is dispatch).

There's also a real performance cost to concatenating everything into one delegate/event. Each delegate is run in sequence, when you call .Invoke() on it. If I've got dozens of subscribers (not even a little bit unreasonable in even a smallish app), and if even one of them is expensive, crashes, or otherwise is a pain, ALL subscribers later in the chain just get to sit and wait while we do it all, for all listeners, whether the event that was just raised was even relevant to one out of a hundred of them.

If you have unique events, all that melts away.

There are a ton of concurrency pitfalls, many of which are so much easier to hit if you have that single universal OnDoStuffWithWhateverITellYouAbout_After_IPushAStackFrame(lolImABadValueAndYouHaveToDefendAgainstMeForAllPermutationsOfThisMonolith) method than if you just let things work as intended and publish descriptive and meaningful events that are raised under intuitively obvious circumstances, from their names.

If you just define the one, ALL calls to that event have to be synchronized, making an already hot path now extremely contentious, and difficult to debug, among other things. The UI latency can quickly grow to noticeable levels with that, especially if anything interesting changes and has to be re-calculated and re-drawn, and execution order still technically isn't guaranteed, so it's not like

Another way to put it: You're basically using enums/strings/magic values/whatever and control flow statements like if/else and switch based on them in the place of named methods, when you do that.

TFM...For R-ing

Whether you read any of that or not, the below is key reading.

Here are some excellent docs on delegates and events, and some concepts that build from there, at the bottom.
First, there's this entire series for delegates and events, all of which is like... Honestly some of the best-written stuff in the C# docs at MS Learn. Linking most of them here for convenience, but you can navigate in the tree there as well.

Anyway...

Delegates

Since events are essentially a special case of delegates, it's pretty clear why this doc series starts there first.

• Overview of Delegates
• delegate the type declaration, Delegate the class, and friends - What they mean and the basics of their use
  • The above two I think do a pretty good job of pointing this out, but it bears repeating, but I'll put it in this footnote: ¹
• This one is kind of an aside, but is the next in the tree so here's the link:
  Why it's silly to declare specific delegate signatures for most things
  • In short, use pre-made things like EventHandler<T>, Action<T,...>, Func<T....>, etc unless you just want to make a named delegate to be a rebel or the expressiveness of a named delegate type is desirable. But be sure to namespace them properly. And don't declare a delegate type inside a class, for many of the same reasons you generally shouldn't declare any other nested type.
    • There's an exception to this, however, if you want to enforce stronger compile-time type safety and don't mind writing a bit of sometimes slightly confusing generics because of the inversion of control that events and delegates often represent. I'll either find an example or write one up, for the events section below.
• Where the lines start to blur between delegates and events: Common Patterns for Delegates
  • Basically, it's similar, because events are based on delegates anyway, but with special rules and behaviors on top. So it's easy to confuse them for a lot of reasons.
  • People often erroneously think using a delegate (say, perhaps, a Func<int,int> as a method parameter as a sort of callback?) means there's only one possible target. Nooooope!
    Check out this dotnetfiddle I wrote up to illustrate a couple things, including that
  • Most of the time, if someone uses that pattern (individual delegates as fields or properties that can be assigned via some mechanism, their intent is actually completely served (and likely better served) by events, and they're basically just doing events, but worse.²

Events, or "Delegates 2: Method Dispatch Boogaloo" (I kid, but only mostly):

• Overview of Events
• "Um. Ok @dodexahedron. Sounds like delegates to me."
  • They are. But with more formality and standardization and, therefore, a bit of a social contract between creator and consumer that things will behave a certain way.
• Event Patterns, in General, but from an older dotnet Framework perspective.
  • Still perfectly valid, but we have nicer things now to make it even better, and you already know what those are, according to some of the v2 code.
• And the next document talks about the newer way!.
  • Mostly, it just talks about how we now have the nice standard generic EventHandler<T> delegate and provides basic guidance on how to write an async subscriber to an event.

Something about stronger type-safety/static analysis? Huh?

In what way?

Well, the loosened guidance on event definitions (such as not being shamed into deriving the event args from EventArgs anymore), mostly.

But be reasonable.

Take Terminal.Gui, for example. EventHandler<SomeType> is perfectly fine for most events. But, if we wanted to, we could define our own delegate with a type like View for sender, instead of just object. And then you also get nullability analysis complaining when sender is null, so you don't do that. Then, subscribers get strong type safety, both inside the library and in consuming code.

But I wouldn't really go farther than that.

Footnotes

1. A delegate kinda feels like a function pointer and people sometimes describe them that way. And really, the tangible end result essentially is the same as that, from a high-level view, with some caveats I've explained elsewhere. But, a delegate type is literally a first-class language construct that wraps a compiler-generated hidden class with a virtual method whose signature is that of the delegate declaration and for which assigned methods are basically overrides. And that's why you sometimes see things like DisplayClass_1<SomeJunk>_Whatever.YourDelegateName in stack traces involving them (I know you've seen those in LINQ and unit test failure stack traces and such plenty of times in this project alone). That's the compiler-generated class that holds your delegate (and also anything you've captured, which I explained somewhere else) ↩

2. For single delegates invoked exactly as an event would have been, anyway, I've never seen a justification that amounted to more than "I wanted to break encapsulation because I'm bad"...or just not being comfortable/experienced with events or the difference being too subtle to have made an impact (ok, those are probably about equally as common as the first one....maybe). ↩

[dodexahedron]

Oh hey, and this is relevant to the above:

https://learn.microsoft.com/en-us/dotnet/csharp/distinguish-delegates-events

[dodexahedron]

@BDisp & @dodexahedron this is ready for review.

Future PRs will:

• Make OnMouseXXX follow the correct pattern per Refactor and align event handling with common/best practices #3209

Yep yep. That's the goal. :)