Make OnMouseXXX follow the correct pattern

[tig]

Make OnMouseXXX follow the correct pattern per #3209

Originally posted by @tig in #3281 (comment)

[dodexahedron]

Hey I'm all for it, but it's no problem if you want to not bother yet and just leave that for when I do that work. I'll be doing the same thing to a bunch of code anyway, so I already was assuming I'd have to do it.

But less work for me is still cool haha. :)

But, if you do go ahead and knock it out yourself, here's the general way I've got current stuff designed on my old branch:

Almost every event uses EventHandler<TypeUsedForAndNamedAfterThisEvent> delegates for the events, where that type is a public record type, sealed if prudent, does not inherit from EventArgs (because we have no need for that), and carries a CancellationToken with it.

I have an interface for all such types to implement, but it's likely to change anyway, so don't worry about that. The CancellationToken is definitely a part of it, though, so if you do that from the start that's awesome.

There are also (Verb)ing and (Verb)ed events, for the majority of them, and the wrappers that invoke them use them in a consistent way.

Generally, that means:

• The incoming value is first checked against current state. If no change, the events are NOT raised and the operation is canceled or returns, as appropriate in context. Otherwise:
• If a change is going to be made, the (Verb)ing event is raised.
  • Some events may also store rollback data, if feasible and sensible.
  • Subscribers can respect or ignore cooperative cancellation at their option. That's not on us to dictate.
  • Cancellation of this does mean that, once the delegate returns, we stop and return from the wrapper without doing anything else.
• After that, the change is made. If successful, the (Verb)ed event is raised.
• If, at any point in all of this, an exception is thrown that bubbles up to the wrapper, we can either allow the program to crash, attempt rollback, or eat the exception.
  • Typically, for something like this, letting it go unhandled is probably the right thing to do. We can't safely assume anything about the state of the application at this point. Environment.FailFast() may be warranted.
  • A log hook at this point is probably a good idea, no matter what we do.
  • Rollback attempts can result in stack overflows because the same exception is quite possible to happen during the rollback, too. Should probably really only be done for events that are not exposed in the public API surface.
  • Eating the exception is dangerous in too many ways, especially for a library, so I'm pretty strongly against that.

[dodexahedron]

Note that not everything has an -ing and -ed event.

A lot of mouse events are in that category. They already get it naturally, if there's a ButtonDown and ButtonUp in addition to the full Click, DoubleClick, etc events.

Oh yeah. And, while the guidelines recommend using -ing and -ed, rather than prefixes like Pre-, Before-, After-, etc., I think there is a valid argument to be made for different naming in isolated cases, if forcing it into that box makes something non-sensical.

For a contrived example, MouseButtonDowning sounds like some sort of play in a rodent NFL game. PreMouseButtonDown or BeforeMouseButtonDown would be much more natural. But again, that's already not really an event that needs to exist - just an example for illustration. But, for any such cases, one should at least make the attempt to make the modifier as generic as possible, so that the same word can be used for other events that might need to do that, rather than having 15 different naming styles.

They also recommend against other suffixes, like Started, Completed, etc. But same argument goes there, too, I think. Just don't be weird haha.

[tig]

Dupe of #3029. Closing.