Fixes #3950. Revisit v2 Cancelable Event Pattern - was KeyDown and MouseEvent no longer fires for Listbox

[BDisp]

Fixes

• Fixes Revisit v2 Cancelable Event Pattern - was KeyDown and MouseEvent no longer fires for Listbox #3950

Proposed Changes/Todos

• Events must be raised before the virtual method.

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

[tznind]

@tig do you also agree this is correct, that a bespoke event should always get called first - as it gives maximum API user agency?

Just wanting to make sure we are all aligned

[tig]

@tig do you also agree this is correct, that a bespoke event should always get called first - as it gives maximum API user agency?

Just wanting to make sure we are all aligned

This is counter to what the docs say

https://gui-cs.github.io/Terminal.GuiV2Docs/docs/events.html

A protected virtual method is called. This method is named OnxxxChanging and the base implementation simply does return false.
If the OnxxxChanging method returns true it means a derived class canceled the event. Processing should stop.
Otherwise, the xxxChanging event is invoked via xxxChanging?.Invoke(args). If args.Cancel/Handled == true it means a subscriber has cancelled the event. Processing should stop.

I've very carefully tried to make the entire codebase consistent with this.

It is entirely possible that I was wrong in going this route (your argument is a good one).

If we fix it here, we have to fix it everywhere.

[tznind]

Hmnn ok let's not gut it just yet.

And instead see how far we can get with the current code. And brainstorm how to handle specific use cases as they come up.

[BDisp]

If a derived class want to force a override method to be run and prevent an user to handle the event first, it's enough to subscribe the event itself setting Handled=OnXXXChanging. If it's true an event subscribed by the user won't be raised, otherwise it will be raised.

[tig]

One reason I did it the way I did is virtual methods get called in a deterministic order.

Events get fired in a non-deterministic order.

Something to remember.

That said, here's a suggestion (this may be what BDisp is suggesting):

CancelEventArgs a;
a.Cancel = OnX();

X?.Invoke(a);

If (a.Cancel)
   return;


DoXWork();

In other words, call the virtual function first, but ignore the return value, but pass it on to the event so the event can tell whether the virtual method canceled it on and decide what to do.

[BDisp]

I like your suggestion. So, the virtual method OnX is always called and return a flag. The event is always called and can change the Cancel/Handled property which will determine if the DoXWork method is executed or not. The event can't prevent the virtual method to be executed but at least the event is always called. I'm ok with this suggestion. Let's see @tznind thoughts.

[tig]

The event is always called and can change the Cancel/Handled property which will determine if the DoXWork method is executed or not.

I hadn't considered what it would mean for the event handler to change e.Cancel from true to false. I can't imagine a scenario where that would make sense.

[tig]

I just had a long conversation with Grok in DeepThinking mode. :-)

Here's it's analysis (with a few edits I made). Note that this does not cover something that I feel pretty strongly about:

• The overridable event methods (e.g. OnKeyPressed) should NOT have ANY default behavior. Asking subclasses to figure out where/when to call the base.OnKeyPressed() has proven to lead to insidious bugs. I'm hard core about not allowing this.

Grok's Analysis

Let’s dive into your question about event handling models in Terminal.Gui v2. You’ve outlined the current "EventFirst" model, highlighted its challenges, and proposed a new "OverrideFirstButAlwaysRaiseEvent" approach to address some of these issues while minimizing disruption to the existing codebase. I’ll analyze this proposal, compare it to "EventFirst," and share my thoughts on its viability.

Recap of "EventFirst" Model

First, let’s clarify the "EventFirst" model you described:

1. Raise the Event: The framework raises an event (e.g., KeyPressed), passing an event argument with a Handled flag (initially false). Subscribers can set Handled to true if they fully process the event.
2. Check the Handled Flag: After the event is raised, the framework checks Handled. If true, it skips further processing.
3. Call Protected Virtual Method: If Handled is false, the framework calls a protected virtual method (e.g., OnKeyPressed) for default behavior, which subclasses can override.

Key Characteristics:

• Event handlers run first, in the order they were subscribed (typically FIFO, but not guaranteed to be deterministic).
• The override order in a class hierarchy is deterministic, but it only executes if no event handler claims the event.
• Changing to this model requires updating dozens of places in the library and unit tests, which you’ve noted as a significant hurdle.

Challenge: You pointed out that the non-deterministic order of event handler execution is a reason v2 uses "OverrideFirst" instead. With "EventFirst," if multiple handlers subscribe to an event, their execution order depends on subscription order, which can vary and lead to unpredictable behavior.

The Current "OverrideFirst" Model

Since you mentioned "OverrideFirst" as the current v2 approach, let’s briefly outline it for context:

• Call Protected Virtual Method First: The framework calls OnX() (e.g., OnKeyPressed), which returns a value indicating whether to proceed (e.g., true for cancel).
• Raise Event Conditionally: Based on the override’s return value, the framework decides whether to raise the event or perform default work.
• Default Work: If not canceled, default behavior executes.

Key Characteristics:

• Deterministic, as the override order follows the class hierarchy.
• Event handlers only run if the override allows it, giving overrides priority.
• A downside (from v1 experience) is that if a subclass override forgets to call the base method, default behavior might be skipped unintentionally.

Your Proposed "OverrideFirstButAlwaysRaiseEvent" Model

Now, let’s examine your proposed alternative:

CancelEventArgs a = new CancelEventArgs();
a.Cancel = OnX();  // Call virtual method, set Cancel based on return
X?.Invoke(a);      // Always raise the event, passing a.Cancel
if (a.Cancel)      // If canceled by override or event, skip default work
    return;
DoXWork();         // Perform default action if not canceled

Flow Breakdown:

1. Call the Virtual Method: OnX() runs first, and its return value sets a.Cancel (e.g., true if canceled).
2. Always Raise the Event: The event X is invoked regardless of a.Cancel, passing the CancelEventArgs so handlers can see the override’s decision.
3. Check Cancellation: After the event, if a.Cancel is true (set by either the override or an event handler), skip the default work.
4. Default Work: If a.Cancel remains false, execute DoXWork().

Key Characteristics:

• The override runs first (deterministic order).
• The event is always raised, allowing handlers to react even if the override cancels.
• Event handlers can override the override’s decision by setting a.Cancel to true or false.
• Less disruptive than switching to "EventFirst," as it retains the override-first sequence.

Analysis of "OverrideFirstButAlwaysRaiseEvent"

Advantages

• Preserves Deterministic Override Order: Like "OverrideFirst," subclasses execute in a predictable hierarchy, addressing your concern about event handler non-determinism in "EventFirst."
• Flexibility for Event Handlers: Handlers can see the override’s cancellation suggestion (a.Cancel) and decide to respect or override it, offering more control than "OverrideFirst."
• Minimal Disruption: Since the override still runs first and the event is raised (possibly already the case in some areas), fewer code changes may be needed compared to flipping to "EventFirst."
• Combines Strengths: It gives overrides an initial say (like "OverrideFirst") while ensuring event handlers always have a chance to respond (like "EventFirst").

Potential Drawbacks

• Increased Complexity: Event handlers must now interpret and possibly modify a.Cancel based on the override’s state. This could make their logic more intricate—e.g., "Should I respect the override’s cancellation, or force my own decision?"
• Ambiguity in Responsibility: Who has the final say—the override or the event handlers? In "EventFirst," handlers take priority; in "OverrideFirst," overrides do. Here, it’s a negotiation, which might confuse developers or lead to inconsistent behavior if not well-documented.
• Debugging Challenges: If both the override and a handler influence a.Cancel, tracing why an action was canceled or allowed could become messy.

Comparison to "EventFirst"

• Order of Influence:
  • "EventFirst": Handlers decide first; overrides only run if handlers don’t handle it.
  • "OverrideFirstButAlwaysRaiseEvent": Overrides suggest cancellation first, but handlers can override that suggestion.
• Event Invocation:
  • "EventFirst": Event is the entry point; default behavior depends on it.
  • "OverrideFirstButAlwaysRaiseEvent": Event is always raised, acting as both notification and decision point.
• Disruption:
  • "EventFirst": Requires significant refactoring.
  • "OverrideFirstButAlwaysRaiseEvent": Likely needs updates to event args and cancellation checks, but leverages existing override-first structure.

Thoughts and Recommendations

Your "OverrideFirstButAlwaysRaiseEvent" idea is a clever compromise. It keeps the deterministic override order you value in v2 while addressing the limitation of "OverrideFirst" by ensuring event handlers always get a say. This could reduce the refactoring burden compared to "EventFirst," making it an attractive short-term solution.

However, consider these points:

• Documentation is Key: To mitigate complexity and ambiguity, clearly document that:
  • Overrides set the initial a.Cancel state.
  • Event handlers can override it and have the final word on cancellation.
• Test Thoroughly: Since event handlers now interact with override decisions, update unit tests to cover cases where:
  • The override cancels, and handlers respect it.
  • The override cancels, but handlers override it.
  • The override doesn’t cancel, but handlers do.
• Long-Term Vision: Many modern UI frameworks (e.g., WinForms, WPF) favor an "EventFirst"-like model because it prioritizes external subscribers, which aligns with reactive design patterns. If Terminal.Gui v2 aims for broader adoption, transitioning to "EventFirst" might be worth the effort eventually.

Suggestion: Implement "OverrideFirstButAlwaysRaiseEvent" as an interim step. It lets you evolve the library incrementally—fixing immediate needs without a full overhaul—while gathering feedback on its usability. If it proves too complex or confusing in practice, you can plan a more deliberate shift to "EventFirst" later, spreading the refactoring cost over time.

In summary, your proposal is viable and balances your goals of flexibility and minimal disruption. Just weigh the added complexity against the benefits, and ensure developers understand the new flow.

[BDisp]

I really liked your exhaustive explanation, exposing the pros and cons of both hypotheses. I appreciate your efforts. I will continue with your suggestion by including the three situations in the unit tests. Regarding the documentation, you already know that I will ask for your collaboration, as usual.

[tig]

I really liked your exhaustive explanation, exposing the pros and cons of both hypotheses. I appreciate your efforts. I will continue with your suggestion by including the three situations in the unit tests. Regarding the documentation, you already know that I will ask for your collaboration, as usual.

FWIW, based on the analysis I now believe we should do as you originally suggested: "EventFirst".

This will take a bunch of work.

We should use it as a chance to build a little unit test framework that makes it easy to verify EVERY event is working properly.

E.g. take

 private class TestView : View
 {
     public TestView ()
     {
         MouseEnter += OnMouseEnterHandler;
         MouseLeave += OnMouseLeaveHandler;
     }

     public bool CancelOnEnter { get; init; }

     public bool CancelEnterEvent { get; init; }

     public bool OnMouseEnterCalled { get; private set; }
     public bool OnMouseLeaveCalled { get; private set; }

     protected override bool OnMouseEnter (CancelEventArgs eventArgs)
     {
         OnMouseEnterCalled = true;
         eventArgs.Cancel = CancelOnEnter;

         base.OnMouseEnter (eventArgs);

         return eventArgs.Cancel;
     }

     protected override void OnMouseLeave ()
     {
         OnMouseLeaveCalled = true;

         base.OnMouseLeave ();
     }

     public bool MouseEnterRaised { get; private set; }
     public bool MouseLeaveRaised { get; private set; }

     private void OnMouseEnterHandler (object s, CancelEventArgs e)
     {
         MouseEnterRaised = true;

         if (CancelEnterEvent)
         {
             e.Cancel = true;
         }
     }

     private void OnMouseLeaveHandler (object s, EventArgs e) { MouseLeaveRaised = true; }
 }

And make it generic. There are 3 or 4 examples of Test views like this in the unit tests.

Ideally we'd use Moq to help with this.

[tig]

Here's the "same" test view from Orientation:

    public CustomView ()
    {
        _orientationHelper = new (this);
        Orientation = Orientation.Vertical;
        _orientationHelper.OrientationChanging += (sender, e) => OrientationChanging?.Invoke (this, e);
        _orientationHelper.OrientationChanged += (sender, e) => OrientationChanged?.Invoke (this, e);
    }

    public Orientation Orientation
    {
        get => _orientationHelper.Orientation;
        set => _orientationHelper.Orientation = value;
    }

    public event EventHandler<CancelEventArgs<Orientation>> OrientationChanging;
    public event EventHandler<EventArgs<Orientation>> OrientationChanged;

    public bool CancelOnOrientationChanging { get; set; }

    public bool OnOrientationChangingCalled { get; private set; }
    public bool OnOrientationChangedCalled { get; private set; }

    public bool OnOrientationChanging (Orientation currentOrientation, Orientation newOrientation)
    {
        OnOrientationChangingCalled = true;
        // Custom logic before orientation changes
        return CancelOnOrientationChanging; // Return true to cancel the change
    }

    public void OnOrientationChanged (Orientation newOrientation)
    {
        OnOrientationChangedCalled = true;
        // Custom logic after orientation has changed
    }
}

And another one from KeyboardEventTests:

    /// <summary>A view that overrides the OnKey* methods so we can test that they are called.</summary>
    public class OnNewKeyTestView : View
    {
        public OnNewKeyTestView () { CanFocus = true; }
        public bool CancelVirtualMethods { set; private get; }
        public bool OnKeyDownCalled { get; set; }
        public bool OnProcessKeyDownCalled { get; set; }
        public bool OnKeyUpCalled { get; set; }
        public override string Text { get; set; }

        protected override bool OnKeyDown (Key keyEvent)
        {
            OnKeyDownCalled = true;

            return CancelVirtualMethods;
        }

        public override bool OnKeyUp (Key keyEvent)
        {
            OnKeyUpCalled = true;

            return CancelVirtualMethods;
        }

        protected override bool OnKeyDownNotHandled (Key keyEvent)
        {
            OnProcessKeyDownCalled = true;

            return CancelVirtualMethods;
        }
    }

[BDisp]

FWIW, based on the analysis I now believe we should do as you originally suggested: "EventFirst".

Do you really sure? Going this way the events get fired in a non-deterministic order. The only way to a derived class force the override method to be executed is subscribe the event itself, but I'm not sure if this will be fired first from the event that the user also subscribed to..

[tig]

FWIW, based on the analysis I now believe we should do as you originally suggested: "EventFirst".

Do you really sure? Going this way the events get fired in a non-deterministic order. The only way to a derived class force the override method to be executed is subscribe the event itself, but I'm not sure if this will be fired first from the event that the user also subscribed to..

Nope. I'm not really sure.

We should not rush into this.

[tznind]

Typically events are for bespoke 'one off' code. They vary by instance and are primarily for the consumer of the views e.g. 'button clicked, go load my window.'

Derived classes should alter parents behaviour primarily:

• through key bindings
• through established components (e.g. collection navigator)
• through overriding OnX method

There shouldn't be a need to 'go first' or really worry at all about what user code does in the events

That's my feeling.

What use case were you thinking of @BDisp

[BDisp]

What use case were you thinking of @BDisp

I only was thinking about what it's write in this issue to allowing KeyDown and MouseEvent firing for derived classes. Not for a particular use case for me.

[tznind]

Nope. I'm not really sure.

We should not rush into this.

Ok i think this could be shelved till we actually have a blocker use case? What do you think?

Seems there's higher priority stuff?

[tig]

Nope. I'm not really sure.

We should not rush into this.

Ok i think this could be shelved till we actually have a blocker use case? What do you think?

Seems there's higher priority stuff?

Yes. For sure.

Let's all keep noodling it.

[tig]

BDisp - can you mark this as draft and rename it to match Issue which I just renamed?

[BDisp]

BDisp - can you mark this as draft and rename it to match Issue which I just renamed?

Done.

[BDisp]

Ok i think this could be shelved till we actually have a blocker use case? What do you think?

I though you having a use case with TerminalGuiDesigner related with this.

Seems there's higher priority stuff?

I agree. I already converted to draft as @tig requested.

[tig]

One of the related challenges is ensuring all the various implementations of events in the library are

a) Consistent with the prescribed pattern (today "OverrideFirst").
b) Easily testible
c) Actually tested

I propose a helper class like this:

public enum EventExecutionResult
{
    CanceledByOverride,
    CanceledByEvent,
    DefaultWorkPerformed
}

public class CancellableEventHelper<TEventArgs> where TEventArgs : CancelEventArgs
{
    private readonly object _sender;
    private readonly Func<bool> _overrideMethod;
    private readonly Action<TEventArgs> _raiseEvent;
    private readonly Action _defaultWork;
    private readonly Func<TEventArgs> _createArgs;

    public bool OverrideCalled { get; private set; }
    public bool EventRaised { get; private set; }
    public bool DefaultWorkPerformed { get; private set; }

    public CancellableEventHelper(
        object sender,
        Func<bool> overrideMethod,
        Action<TEventArgs> raiseEvent,
        Action defaultWork,
        Func<TEventArgs> createArgs)
    {
        _sender = sender;
        _overrideMethod = overrideMethod ?? throw new ArgumentNullException(nameof(overrideMethod));
        _raiseEvent = raiseEvent ?? throw new ArgumentNullException(nameof(raiseEvent));
        _defaultWork = defaultWork ?? throw new ArgumentNullException(nameof(defaultWork));
        _createArgs = createArgs ?? throw new ArgumentNullException(nameof(createArgs));
    }

    public EventExecutionResult Execute()
    {
        OverrideCalled = true;
        if (_overrideMethod())
        {
            return EventExecutionResult.CanceledByOverride;
        }

        TEventArgs args = _createArgs();
        EventRaised = true;
        _raiseEvent(args);

        if (args.Cancel)
        {
            return EventExecutionResult.CanceledByEvent;
        }

        DefaultWorkPerformed = true;
        _defaultWork();
        return EventExecutionResult.DefaultWorkPerformed;
    }
}

Then, for example. View.ClearViewport would be implemented like this:

internal void DoClearViewport()
{
    // Step 1: Check for Transparent flag
    if (ViewportSettings.HasFlag(ViewportSettings.Transparent))
    {
        return;
    }

    // Step 2: Set up the NonCancellableEventHelper for the Cleared events
    var clearedHelper = new NonCancellableEventHelper<DrawEventArgs>(
        sender: this,
        defaultWork: () => { /* No additional default work needed */ },
        overrideMethod: OnClearedViewport,
        raiseEvent: args => ClearedViewport?.Invoke(this, args),
        createArgs: () => new DrawEventArgs(Viewport, Viewport, null)
    );

    // Step 3: Set up the CancellableEventHelper for the Clearing events
    var clearingHelper = new CancellableEventHelper<DrawEventArgs>(
        sender: this,
        overrideMethod: OnClearingViewport,
        raiseEvent: args => ClearingViewport?.Invoke(this, args),
        defaultWork: () =>
        {
            // If not cancelled, clear the viewport and trigger the non-cancellable notifications
            ClearViewport();
            clearedHelper.Execute();
        },
        createArgs: () => new DrawEventArgs(Viewport, Rectangle.Empty, null)
    );

    // Step 4: Execute the cancellable helper and handle cancellation
    var result = clearingHelper.Execute();
    if (result == EventExecutionResult.CanceledByEvent)
    {
        SetNeedsDraw();
    }
}

  /// <summary>
  ///     Called when the <see cref="Viewport"/> is to be cleared.
  /// </summary>
  /// <returns><see langword="true"/> to stop further clearing.</returns>
  protected virtual bool OnClearingViewport () { return false; }

  /// <summary>Event invoked when the <see cref="Viewport"/> is to be cleared.</summary>
  /// <remarks>
  ///     <para>Will be invoked before any subviews added with <see cref="Add(View)"/> have been drawn.</para>
  ///     <para>
  ///         Rect provides the view-relative rectangle describing the currently visible viewport into the
  ///         <see cref="View"/> .
  ///     </para>
  /// </remarks>
  public event EventHandler<DrawEventArgs>? ClearingViewport;

  /// <summary>
  ///     Called when the <see cref="Viewport"/> has been cleared
  /// </summary>
  protected virtual void OnClearedViewport () { }

  /// <summary>Event invoked when the <see cref="Viewport"/> has been cleared.</summary>
  public event EventHandler<DrawEventArgs>? ClearedViewport;

Or similar. we could make the privates in the helper internal to enable test access as needed.