Formalize the Cancellable Work Pattern as v2s core eventing model

[dodexahedron]

The Cancellable Work Pattern is integral to Terminal.Gui, enabling flexible, decoupled workflows across scheme management, rendering, input, commands, and properties. Proposed helper classes streamline its implementation, particularly for property changes like Orientation. The Scheme system leverages the CWP for dynamic styling, while the Command system’s updates (Command.Activate, Handled, PropagatedCommands) enhance clarity and coordination. Ongoing documentation efforts ensure these improvements are well-communicated, supporting robust UI development in v2_develop.

This will be addressed in this PR:

• Fixes #3209 - Formalize Cancellable Work Pattern and add helpers #4092

Original Post

As mentioned in discussion #3206, event handling could use some TLC.

This issue is meant to be the main issue, though it may spawn others, depending on just how involved this ends up being.

Work items I wanted to address, many of which already have been partially completed in existing work on v2 or may not currently even exist/be problems in the current code, but I'm listing these out for completeness and as reminders for myself to at least confirm them all:

• Standardize event delegates according to design recommendations
  • Basically, exactly how the Terminal.Gui design specification already says to do, but with an additional degree of scrutiny around the type arguments to EventHandler<T>, considering potential needs for future changes/features added to TG.
• OnX wrappers for raising events are public, but most should be protected virtual and none should be public.
• Declaring the actual events inside interfaces, which are then implemented by classes that need to expose those events.
• Making sure that events are first defined at the most-derived type that is reasonable or necessary for common handling (the interfaces will help with that, too).
• Ensure that, when relevant, derived classes ask their base class to raise the event, and that there are no virtual/overridden events nor any events hidden with new (I don't think either of those last things exist in the code)
• Ensuring that standard rules and practices for inheritance and related concepts such as overrides are followed as closely as possible.
• Consider and, if prudent, implement necessary constructs for thread-safety around event raising
  • Including at least being aware that subscribers may provide an async void handler that returns before the associated background work is actually complete. Though this doesn't necessarily mean actively attempting to defend against it, since that's 100% the consumer's fault.
• Ensure/implement more formal/recommended handling of exceptions thrown in subsribers' handlers, as appropriate, via the following methods, as appropriate:
  • Stopping processing of the invocation list when an exception is caught and adding any relevant information to the stack trace before re-throwing the caught exception
  • Cleaning up or undoing any work the wrapper has done, if possible.
  • Cleaning up or undoing any work a handler has done, if it's safe to do so based on event or application state (this will probably be rare or not at all)
  • Hard-crashing the application immediately, via Environment.FailFast, if/when appropriate
  • Ensuring that raising of events (such as the wrappers themselves or the immediate call-sites of them) never throws exceptions, per design guidelines
  • Ensuring that exceptions thrown by handlers are not swallowed by the wrappers
    • If that behavior is desired and/or already exists anywhere, it will be modified to be an opt-in behavior, as that's a non-standard and counterindicated practice.

I think I'll start by getting an overall picture of all the events that exist and then work on creating appropriate interfaces that strike a balance between granularity and not needlessly creating tons of interfaces for no reason. Just as a guess based on what I know of TG, I'm betting there will probably be around 5ish interfaces, to group certain high-level categories of events common to types in specific hierarchies. But, we'll see. Could be more; Could be fewer.

Issues Potentially Impacted and/or Where Related Changes Are Needed

• Refactor MouseEvent and MouseEventEventArgs to simplify #3029
• InvokingKeyBindings event not invoked in OnInvokingKeyBindings overridden method. #3056
• BeginInit/EndInit should follow CWP #2235
• Ability to set Action on StatusItem post creation #3070
• ConfigurationManager: Applied and Updated events are clumsy #2871
• ProcessButtonClick strips any modifier keys from mouse events #2588
• TextValidateField doesn't show the cursor and doesn't have TextChanged event. #3189

[dodexahedron]

Initial post is the outline.

Any comments, suggestions, requests, etc are welcome and appreciated!

[tig]

I added a section to your first post.

[dodexahedron]

Oh yeah I had meant to mention some issues that this may affect. Thanks. I have a couple more to add, I think (once I find where I stuck the list of issue numbers).

[dodexahedron]

Found em and added to the list

[tznind]

Can you show a code example for a simple view event e.g. Button click? I think that would better articulate the proposal. Having both Action and event for the same system seems redundant but happy to learn the practical use cases.

…

On Wed, 24 Jan 2024, 06:11 dodexahedron, ***@***.***> wrote: I also wanted to talk about design of things that essentially implement their own event dispatch via the use of Action properties that take delegates. Those are events. But they're limited to single handlers and don't follow the same rules as events for inheritance and dispatch. That doesn't mean it's a bad pattern, by any means. In fact, it's similar to the Command pattern in WPF. But, WPF provides both Commands and events for most controls. So my preliminary proposals, assuming the desire to have that form of dispatch available is essentially a hard requirement, are: - Synchronize the naming and at least the high-level/public-facing API of that functionality with a standard implementation, such as Commands in WPF - Unfortunately, we can't just use those actual types, since the WPF portion of dotnet is still only provided in the windows dotnet flavors. - It's pretty likely, however, that there's a nuget package out there that already provides that infrastructure. - Ensure that any Views or any other types that have that form of dispatch available also expose or inherit events which enable at least the most common/likely scenarios, so consumers can use whichever paradigm they prefer or possibly require, if they need to be able to have multiple subscribers. - Additionally, when and where feasible and appropriate, the custom dispatch mechanism would be prudent to modify in any one or more of the following ways: - Any type that exposes that mechanism can declare an internal or private event and actually use that for dispatch, so that the code path is unified and consistent as early as possible. - The inverse of that. In other words, the actual formal event invocations from their wrapper functions could delegate themselves through calls of the custom mechanism. This one seems like a ticking time bomb of maintenance issues to me, though. - Both the events and the custom dispatchers could call the same internal, private, or protected internal method that handles invocation of anything that's been set up by the consumer (in other words, checks and calls the custom mechanism, if defined, AND checks and raises the corresponding event handler, if anything is subscribed. I kinda like this idea, since it's basically the first one, but potentially results in a small reduction of duplicated code, while still having the standard behavior consumers are likely to expect, as well as providing consistency or at least substation similarity between the internal code paths each mechanism spins up. - Nothing at all, and just let them both be completely unrelated. I'm not a big fan of this either (obviously, or I wouldn't have brought it up). - Of course, if the two mechanisms cannot be reconciled (which is fairly unlikely), then this ends up being the best choice by necessity. - More than one of the above strategies can coexist in the application, especially for different types, if desired. I think that would be an unwarranted introduction of inconsistent behavior and expectations, however, so I think it is *most* ideal to use the same general strategy (be it one, the other, or both mechanisms) project-wide. - This is another thing that defining and using interfaces, as mentioned in my earlier walls of text, can help to both simplify and standardize. — Reply to this email directly, view it on GitHub <#3209 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHO3C5HS4QUSJYUDXZWYNNDYQCQYFAVCNFSM6AAAAABCHW7NVWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBXGQZTONJWGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[BDisp]

Really good the intention to convert all the public virtual OnX to protected virtual. Oly derived classes can override them and the consumer of the object shouldn't be allowed to call the OnX method.

[dodexahedron]

Can you show a code example for a simple view event e.g. Button click? I think that would better articulate the proposal. Having both Action and event for the same system seems redundant but happy to learn the practical use cases.

I removed the comment because I realized I needed to revise a few parts of it before it's worth discussing.

But, to answer the basic question:

With an event, if there is a button and it has a Click event, the consuming code can subscribe to it for multiple subscribers, since an event is a MulticastDelegate. For example:

• The button needs to fire off some database action. Whatever ViewModel-type class is in use can subscribe to the Button.Click event to do it.
• The button also needs to cause some other View to update in some way. Maybe a last updated timestamp or whatever you can imagine. That item can subscribe to the event.
• The application is making use of detailed logging or, similarly, telemetry to collect usage data of various functionality. Those things can subscribe to the event.

When the button is clicked, ALL of these things happen, without dependency on each other.

If the only mechanism available is explicit dispatch of an Action or Func, the only ways to make that happen are to put it all in a monolithic handler (which is often not ideal, especially if some things only conditionally want to subscribe to the event), or to chain the method calls, inside the handler (which is effectively the same as a monolith anyway - just organized differently).

Windows Forms, WPF, WinUI, and asp.net all always provide events for things. When the Command style is available, there are still events as well, relevant to the given type.

If we only expose the custom dispatcher, we force consumers to have to couple types and methods that are potentially only incidentally related, and that's not cool.

[dodexahedron]

Initial findings about the current state of this:

• There's a ton of coupling, currently, and lots of places that break encapsulation, which was only possible because of the public OnX wrappers).
  • Making the affected wrappers protected internal is a quick-fix workaround that at least makes them not public, so work can continue. Actually fixing and de-coupling some of those situations will require careful thought, and will come with other incidental changes within those types/methods.
  • These situations may actually be more ideal to just take a wrecking ball to and start over with.
  • This, to me, is an acceptable interim step that can be performed immediately in this issue, and then fully fixed by iterating/refining it from there in later commits.
  • This step may actually be more prudent to perform in its own branch and PR, as the very first task, and then the rest of the work gets branched from that point. It needs to get at least contained ASAP, so it doesn't keep getting abused in new work.
• There's a lot of stuff declared way down in Responder, but the first types that ever actually use any of it are descendants of Responder or even not related.
  • Pos, for example, currently calls an event wrapper on View. That's one of the most troubling violations of how events are supposed to work that I've found so far.
• Responder declares most of the wrapper methods, yet doesn't have the corresponding events.
  • These methods need to be pushed down to more-derived types or, at minimum, made abstract in Responder. It's possible, in various situations, for a call to one of the wrapper methods to never actually invoke any events. That should never be possible.
  • The only one that has an event in Responder is the new Dispose code. The rest have no real business being there.
  • The only type that inherits from Responder is View.
    • This makes Responder an ideal candidate for simply merging it into View, making it abstract (either in part or in full), or turning it into an interface.
      • A combination of turning it into an Interface and pushing some members down to View seems most appropriate.
• Responder also has other problems that aren't really in scope of this issue, that basically amount to it not needing to exist and making things complicated because it does exist. However, the major overhaul this issue prompts is a good opportunity to deal with it all, especially since it covers the majority of the Responder type in the first place.
  • The work on burninating Responder that is not directly related to this specific issue can, if desired, be handled in a separate branch, branched from this one. It's such a small and simple type, though, that I don't really think it's worth it to separate the work, and it can just be considered incidental to the work necessary for this issue.
• Naming is inconsistent and sometimes odd, unclear, or potentially misleading. This will be addressed as the associated events/methods are touched.

[tig]

Responder

Remember that we're nuking Responder from orbit. I thought there was a master issue for it, but there's not. Related work:

• Refactor KeyEvent and KeyEventEventArgs to simplify #2926
• Refactor View to fix litany of issues in v1 #2360
• Responder.MouseEvent and View.OnMouseEvent are backwards - Refactor #2579

Some of what you're seeing there is that's a WIP. My hope was we'd actually clean this up mostly as part of

• Refactor MouseEvent and MouseEventEventArgs to simplify #3029

[dodexahedron]

Woo that's good news.

In that case, I will approach the event stuff as if Responder is already gone.

The end result would be the same as the goal, anyway, whether the class lived on or not, but it does make it easier if I get to assume it's not going to be there.

Thanks for pointing that out.

Always sucks being the new guy and having to get filled in on things that are old hat for everyone else.¹ 😅

Footnotes

1. That is, it sucks a little for the new guy and a lot for everyone else. ↩