search

gui-cs / Terminal.Gui

Cross Platform Terminal UI toolkit for .NET
MIT License
9.66k stars 688 forks source link

Refactor and align event handling with common/best practices #3209

Open dodexahedron opened 9 months ago

dodexahedron commented 9 months ago

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:

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

dodexahedron commented 9 months ago

Initial post is the outline.

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

tig commented 9 months ago

I added a section to your first post.

dodexahedron commented 9 months ago

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 commented 9 months ago

Found em and added to the list

tznind commented 8 months ago

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 https://github.com/gui-cs/Terminal.Gui/issues/3209#issuecomment-1907437563, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHO3C5HS4QUSJYUDXZWYNNDYQCQYFAVCNFSM6AAAAABCHW7NVWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBXGQZTONJWGM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

BDisp commented 8 months ago

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 commented 8 months ago

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:

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 commented 8 months ago

Initial findings about the current state of this:

tig commented 8 months ago

Responder

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

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

dodexahedron commented 8 months ago

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.[^1] 😅

[^1]: That is, it sucks a little for the new guy and a lot for everyone else.

dodexahedron commented 8 months ago

Just a note about my last comment:

Potential for merge conflicts exists (but easily-resolvable) since I'll be nuking the relevant events, methods, and properties from Responder early on in this work, to facilitate things.

dodexahedron commented 8 months ago

Just noting here that this work will end up removing a significant portion of the remaining code in Responder. It includes moving the events, their wrappers, and the fields and properties related to them.

If anyone goes looking for any of those things, most or all of them will be in View instead, and with the necessary tweaks to make it all nice and "proper."

dodexahedron commented 8 months ago

I've implemented two events, so far, in code in my working copy (not pushed to the branch yet). One is a re-implementation of the OnVisibleChanged event, and the other is a new corresponding OnVisibleChanging event.

They're in a new interface (which will have other events too) called ICommonViewEventsPublisher, which is declared and implemented by View.

They use standard EventHandler<T> delegates with their own EventArgs-derived eventargs classes, providing the relevant state change information about the events (or the predicted changes), a reference to the specific View that is being modified, and a cancellation flag for the Changing event, to abort a change that is about to be made, if desired, which prevents the field from changing, avoids calling any code that is irrelevant without the change being made, and of course also prevents a Changed event from being raised.

I'm not sure how many unit tests actually exist around raising the existing event, explicitly (though that's the next thing I'm going to do anyway), but the changes I've made so far have only resulted in one existing unit test failing, and I think that's actually the fault of the test itself being invalid because of not taking all relevant data into account, but I'll dig into that as I write up a proper test suite for these.

There is also a change to the behavior of the Visible property.

It no longer has an accessible setter (it is init-only), and it reflects the actual effective visibility of the View, by the following definition:

    public bool Visible {
        get => _visible && (SuperView?.Visible ?? true);
        init => _visible = value && Visible;
    }

This does two important things:

The public setter was turned into the SetDesiredVisibility method, primarily to indicate the actual intent of the operation.

I think the Visible property should be revisited, though, and here are my thoughts on how and why:

The actual changes across the project necessary to implement this aren't really extensive at all, especially since most things would just need to ask for IsVisible instead of the current Visible property.

This provides a more expressive control and reporting of the behavior, for consumers. For example, when a consumer really doesn't care about the explicit value of it, on a SubView of something, the default value of the enum conveys what's going to happen. It also enables a consumer to unset a desired visibility for a View, so they can stop caring about its value, after some operation they've performed. For example, maybe I have a view contained in another that I absolutely want shown, initially. But, after some other function/validation/whatever is done, I no longer care and just want to let its parent dictate it.

How's that different from the binary case? It enables optimizations in the library to skip certain code units, including dispatching the events for change of the Visibility property itself. If a SuperView is not visible, I might not want the VisibleChanged events on its SubViews to be raised, and I don't want to have to manually track that and subscribe/unsubscribe from them when it happens, as that is just a bunch of bug-prone boilerplate. If that SubView is not explicitly set Visible or Hidden, that gives me a way to control that atomically.

That's just the first example that popped into my head, though, and I'm sure there are plenty of others.

I think similar concepts may apply to other properties, but this is the one I'm working on right now, so I wanted to point it out and solicit thoughts.

@tig @BDisp: any thoughts/commentary/concerns/ideas about that proposal?

This is, by the way, the basic principle behind how WPF handles this sort of property and situation, if you want a real-world example.

UIElement, which is sorta analogous to our View class, has this idea implemented with the Visibility property (which is the same idea as above, just with a different purpose), and the IsVisible get-only property, which is exactly the concept described above. It's got many properties or complementary pairs/groups of properties that follow the same basic principle.

https://learn.microsoft.com/en-us/dotnet/api/system.windows.uielement.isvisible?view=windowsdesktop-8.0

The Remarks section of that is basically everything I said above:

Remarks Determination of the IsVisible value takes all factors of layout into account. In contrast, Visibility, which is a settable property, only indicates the intention to programmatically make an element visible or invisible.

Elements where IsVisible is false do not participate in input events (or commands), do not influence either the measure or arrange passes of layout, are not focusable, are not in a tab sequence, and will not be reported in hit testing. In contrast, elements where IsEnabled is false will still participate in events and commands, and hit testing, but are also not focusable.

dodexahedron commented 8 months ago

Addendum: Doing that could also optionally get rid of the separate SetDesiredVisibility method.

dodexahedron commented 8 months ago

Something else potentially worth considering is whether a change in visibility on a View should inform all of its subviews to raise the events on themselves if their SuperView's effective visibility has changed.

Whether that's just the Changed event or also include the Changing event is something that would need more thought, because of cancellation. That could be both powerful and dangerous, since something deeper in the tree could potentially cancel an event targeted at a node closer to root, depending on how it's implemented here. There are many options there.

And, of course, for any of these ideas, configurability either as a static default and/or as something that can be set in configuration files adds to flexibility, but just adds more work on our end.

I have that kind of cascading relationship in use in my SnapsInAZfs project, to handle recursive setting and updating of properties in a tree (displayed in a Terminal.Gui TreeView, in fact), where inheritance of the physical values is important. It's kinda ridiculous how simple it is to implement for the power it gives.

dodexahedron commented 8 months ago

I'm pausing most of my work on this, for now, pending the major refactors in progress elsewhere, since this touches almost everything.

What I'll do is work on the interfaces and such that will be used by it, but which don't affect anything until they're actually declared on the types.

Also, a note about some of what I'm planning/doing for the design of this:

I'm supporting cooperative cancellation, for some operations, using the standard cooperative cancellation facilities provided in .net (using CancellationToken and all that). In particular, any of the "XChanging" events, which are/will be raised before a value is actually changed typically will get that capability, to allow aborting changes using standard practices. That'll make things friendlier for use with async methods, as well.

There is something, though, that is relevant to other work and might be worth considering.

So, we want to get rid of the Responder class. That's cool, in the context of what that class was. However, a well-designed implementation of events does create a worthwhile reason to have an abstract base class that provides a standard implementation of the plumbing for various events.

Why?

Well, interfaces have the limitation that we can't enforce anything private, but there are components necessary to support a proper implementation of some of them that aren't really appropriate to be in the interface and thus public or protected.

One such concept is the cancellation. An interface exposing cancellation capabilities can only require implementers to accept the CancellationTokens and such in parameters, but otherwise leave the implementer to do whatever they want beyond that. That's potentially dangerous, and creates the situation wherein a user's derived type implements the interface, but won't work correctly with Terminal.Gui-defined types, because they neglect to do it properly. In particular, a type supporting cancellation typically should own an instance of a CancellationTokenSource which is the orchestrator/arbitrator of that functionality. It is not appropriate, though, for that object to be anything but private or, in limited situations, protected, and enforcing that in an interface isn't possible. A base class with that implementation takes care of that.

Is it strictly necessary to do it in an abstract base class? No, but it is a better design than duplicating that implementation in all of the least-derived types in inheritance hierarchies that have events. Since not everything inherits from View, that means there is not just one such place to do that. It's also how it's done in every other major framework/library that provides that kind of functionality, and the abstract base is a very simple type, rather than what Responder was/had become. It basically exists to implement the interface, with a standard implementation that can then be trusted by types in this library, even if those types are extended by a consumer's code. They can still override that, if they want to, but then it's an explicit and intentional action, rather than simply an accident or omission.

As a quick and dirty example, it would look something like this (these are not real types and are for illustration only):

public interface ITextChangingEventSource {
  event EventHandler<TextChangingEventArgs>? TextChanging;
}

public class TextChangingEventArgs : ISupportsCancellation {
  public TextChangingEventArgs(ITextChangingEventSource target, CancellationToken cancellationToken) {
    Target = target;
    if(cancellationToken = CancellationToken.None) {
      _cts = new ();
    }
    else {
      _cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
    }
  }
  private CancellationTokenSource _cts;
  public ITextChangingEventSource Target{ get; init; }
  public void Cancel() {
    // Deal with cancellation via _cts
  }
  public void Dispose() {
    // Dispose the token source
  }
}

public abstract class CancellableEventSource : ITextChangingEventSource { // and any other interfaces relevant at the base
  protected CancellationTokenSource Cts = new();
  public event EventHandler<TextChangingEventArgs>? TextChanging;
  protected void OnTextChanging(object? sender, TextChangingEventArgs e) {
    // Validate input and raise the event.
    // This can also optionally be implemented in such a way that, as soon as a cancellation occurs, it stops invoking subscribed delegates, rather than invoking them all and trusting them to respect cancellation.
  }
}

That's a rudimentary example and written in this textbox on github, so it's not great by any means, but hopefully illustrates the example I mean to illustrate.

dodexahedron commented 8 months ago

Actually.... I need to correct a pretty important statement in the previous comment, now that I'm on the PC where I'm doing this work and have the code in front of me...

Any abstract base class suggested above would NOT be the base of View or others. It would be the base for EventArgs classes used by cancelable events, and would, itself, inherit from EventArgs, as is the recommendation for all custom EventArgs types.

The abstract class would implement the ISupportsCancellation interface.

Any cancelable events would then have their EventArgs classes derived from the abstract class, and thus all have a standard/consistent implementation of the basic cancellation infrastructure, which also avoids duplication of boilerplate around that. I'll paste some sample code from what I've written in a follow-up comment, as soon as I clean it up a little bit.

dodexahedron commented 8 months ago

Here's the interface for cancelable eventargs types:

#nullable enable
namespace Terminal.Gui;

/// <summary>
///   An interface providing basic standardized support for cooperative cancellation.
/// </summary>
/// <remarks>
///   Notes to implementers:
///   <para>
///     Types implementing this interface should typically have a non-static private readonly field of type
///     <see cref="CancellationTokenSource" /> to manage cancellation.
///   </para>
///   <para>
///     This interface declares <seealso cref="IDisposable" />, which should be used to dispose of the <see cref="CancellationTokenSource" />.
///   </para>
/// </remarks>
[UsedImplicitly (ImplicitUseTargetFlags.WithMembers)]
public interface ISupportsCancellation : IDisposable {
    /// <summary>
    ///   Gets the <see cref="System.Threading.CancellationToken" /> associated with this instance.
    /// </summary>
    /// <remarks>
    ///   Should typically be provided by a <see cref="CancellationTokenSource" /> owned by the implementing type.
    /// </remarks>
    CancellationToken CancellationToken { get; }

    /// <inheritdoc cref="CancellationToken.IsCancellationRequested" />
    public bool IsCancellationRequested => CancellationToken.IsCancellationRequested;

    /// <summary>
    ///   Requests cancellation for this instance of <see cref="ISupportsCancellation" />.
    /// </summary>
    void RequestCancellation ();

    /// <summary>
    ///   Requests cancellation for this instance of <see cref="ISupportsCancellation" /> and provides the associated
    ///   <see cref="CancellationToken" /> as an output parameter.
    /// </summary>
    void RequestCancellation (out CancellationToken cancellationToken);
}

Note the default implementation for the boolean IsCancellationRequested property. That's a language feature introduced several versions of C# ago and can be overridden by implementors, if desired, but otherwise can be left out if the default behavior is all that's wanted (which will be typical, for this one).

Here's the abstract base class for them:

#nullable enable
namespace Terminal.Gui;

/// <summary>
///   Provides a default implementation of the <see cref="ISupportsCancellation" /> interface.
/// </summary>
[UsedImplicitly (ImplicitUseTargetFlags.WithMembers)]
[MustDisposeResource (false)]
public abstract class CancelableEventArgs : EventArgs, ISupportsCancellation {
    /// <summary>
    ///   The <see cref="CancellationTokenSource" /> that owns the <see cref="System.Threading.CancellationToken" /> for this instance and
    ///   arbitrates cancellation
    /// </summary>
    protected readonly CancellationTokenSource Cts;

    private bool _isDisposed;

    /// <summary>
    ///   Protected constructor for the abstract <see cref="CancelableEventArgs" /> class, which delegates to the
    ///   <see cref="CancelableEventArgs(System.Threading.CancellationToken)" /> overload, passing
    ///   <see cref="System.Threading.CancellationToken.None" />.
    /// </summary>
    [MustDisposeResource (false)]
    protected CancelableEventArgs () : this (CancellationToken.None) { }

    /// <summary>
    ///   Protected constructor for the abstract <see cref="CancelableEventArgs" /> class, using <paramref name="cancellationToken" /> to create a
    ///   linked token source.
    /// </summary>
    /// <param name="cancellationToken"></param>
    /// <remarks>
    ///   If <paramref name="cancellationToken" /> is <see cref="System.Threading.CancellationToken.None" />, creates a new, independent
    ///   <see cref="CancellationTokenSource" />.
    ///   <para />
    ///   For all other values, creates a linked <see cref="CancellationTokenSource" /> based on that token
    /// </remarks>
    [MustDisposeResource (false)]
    protected CancelableEventArgs (CancellationToken cancellationToken) { Cts = cancellationToken == CancellationToken.None ? new () : CancellationTokenSource.CreateLinkedTokenSource (cancellationToken); }

    /// <inheritdoc />
    /// <remarks>
    ///   The value returned for types derived from <see cref="CancelableEventArgs" /> is the token provided by the protected <see cref="Cts" />
    ///   instance owned by this instance.
    /// </remarks>
    public CancellationToken CancellationToken => Cts.Token;

    /// <inheritdoc />
    public void RequestCancellation () {
        ObjectDisposedException.ThrowIf (_isDisposed, this);

        Cts.Token.ThrowIfCancellationRequested ();

        Cts.Cancel ();
    }

    // Disable this warning because the analyzer doesn't understand that we are using the correct Token anyway.
#pragma warning disable PH_P007
    /// <inheritdoc />
    public void RequestCancellation (out CancellationToken cancellationToken) {
        ObjectDisposedException.ThrowIf (_isDisposed, this);

        RequestCancellation ();

        cancellationToken = Cts.Token;
    }
#pragma warning restore PH_P007

    /// <inheritdoc />
    public virtual void Dispose () {
        if (_isDisposed) {
            return;
        }

        Dispose (true);
        GC.SuppressFinalize (this);
    }

    /// <summary>
    ///   Protected implementation for disposal, called by the public <see cref="Dispose" /> method and the type finalizer, if defined.
    /// </summary>
    /// <param name="disposing">
    ///   Whether this method call is from a call of the public <see cref="Dispose()" /> method (<see langword="true" />) or by the GC, in the type
    ///   finalizer (<see langword="false" />).
    /// </param>
    /// <remarks>
    ///   When invoked with <paramref name="disposing" /> <see langword="true" />, will only execute once. Subsequent calls with
    ///   <paramref name="disposing" /> <see langword="true" /> will return immediately.
    /// </remarks>
    protected virtual void Dispose (bool disposing) {
        if (!disposing || _isDisposed) {
            return;
        }

        Cts.Dispose ();
        _isDisposed = true;
    }

    /// <inheritdoc />
    ~CancelableEventArgs () { Dispose (false); }
}

I'll provide an example implementation for an actual EventArgs type for real events shortly.

dodexahedron commented 8 months ago

Here's an example implementation of an EventArgs class for an Enabling and Disabling pair of events:

#nullable enable
namespace Terminal.Gui;

/// <summary>
///   Event arguments intended for use with the <see cref="ISupportsEnableDisable.Enabling" /> and
///   <see cref="ISupportsEnableDisable.Disabling" /> events of the <see cref="ISupportsEnableDisable" /> <see langword="interface" />.
/// </summary>
/// <remarks>
///   <para>
///     Note that all state values set are snapshots of their associated values as of the time that the event was raised and this
///     <see cref="EnablingDisablingEventArgs" /> instance was initialized.
///     <para />
///     If actual current values on <see cref="Target" /> are required, that must be handled by the subscriber's implementation.
///   </para>
/// </remarks>
[UsedImplicitly (ImplicitUseTargetFlags.WithMembers)]
[MustDisposeResource (false)]
public class EnablingDisablingEventArgs : CancelableEventArgs {
    /// <summary>
    ///   Creates a new instance of <see cref="EnablingDisablingEventArgs" /> intended for use with the
    ///   <see cref="ISupportsEnableDisable.Enabling" /> and <see cref="ISupportsEnableDisable.Disabling" /> events of the
    ///   <see cref="ISupportsEnableDisable" /> <see langword="interface" />, with the supplied values.
    /// </summary>
    /// <remarks>
    ///   This constructor overload sets all required properties. It is not necessary to set them in an initializer, when using this overload.
    /// </remarks>
    /// <param name="target">
    ///   A reference to the instance of the <see cref="ISupportsEnableDisable" /> that is the intended target of the change.
    /// </param>
    /// <param name="oldDesiredState">
    ///   The current desired <see cref="EnableState" /> value before the requested change would be executed.
    /// </param>
    /// <param name="newDesiredState">The <see cref="EnableState" /> requested by this event.</param>
    /// <param name="oldEffectiveState">
    ///   The current effective <see cref="EnableState" /> value before the requested change would be executed.
    /// </param>
    /// <param name="predictedEffectiveState">
    ///   The effective <see cref="EnableState" /> value that is predicted to result after this change would be executed.
    /// </param>
    /// <param name="cancellationToken"></param>
    [SetsRequiredMembers]
    [MustDisposeResource (false)]
    public EnablingDisablingEventArgs (ISupportsEnableDisable target, EnableState oldDesiredState, EnableState newDesiredState, EnableState oldEffectiveState, EnableState predictedEffectiveState, CancellationToken cancellationToken) : base (cancellationToken) {
        Target = target;
        OldDesiredState = oldDesiredState;
        NewDesiredState = newDesiredState;
        OldEffectiveState = oldEffectiveState;
        PredictedEffectiveState = predictedEffectiveState;
    }

    /// <summary>
    ///   Creates a new instance of <see cref="EnablingDisablingEventArgs" /> intended for use with the
    ///   <see cref="ISupportsEnableDisable.Enabling" /> and <see cref="ISupportsEnableDisable.Disabling" /> events of the
    ///   <see cref="ISupportsEnableDisable" /> <see langword="interface" />.
    /// </summary>
    /// <remarks>
    ///   All <see cref="EnableState" /> properties must be set in an initializer, when using this constructor overload.
    /// </remarks>
    /// <param name="target">
    ///   A reference to the instance of the <see cref="ISupportsEnableDisable" /> that is the intended target of the change.
    /// </param>
    [MustDisposeResource (false)]
    public EnablingDisablingEventArgs (ISupportsEnableDisable target) : this (target, CancellationToken.None) { }

    /// <summary>
    ///   Creates a new instance of <see cref="EnablingDisablingEventArgs" /> intended for use with the
    ///   <see cref="ISupportsEnableDisable.Enabling" /> and <see cref="ISupportsEnableDisable.Disabling" /> events of the
    ///   <see cref="ISupportsEnableDisable" /> <see langword="interface" />.
    /// </summary>
    /// <remarks>
    ///   All <see cref="EnableState" /> properties must be set in an initializer, when using this constructor overload.
    /// </remarks>
    /// <param name="target">
    ///   A reference to the instance of the <see cref="ISupportsEnableDisable" /> that is the intended target of the change.
    /// </param>
    /// <param name="cancellationToken">The <see cref="CancellationToken" /> to associate with this instance.</param>
    [MustDisposeResource (false)]
    public EnablingDisablingEventArgs (ISupportsEnableDisable target, CancellationToken cancellationToken) : base (cancellationToken) { Target = target; }

    /// <summary>Gets the <see cref="EnableState" /> requested by this event.</summary>
    public required EnableState NewDesiredState { get; init; }

    /// <summary>
    ///   Gets the current desired <see cref="EnableState" /> value before the requested change would be executed.
    /// </summary>
    public required EnableState OldDesiredState { get; init; }

    /// <summary>
    ///   Gets the current effective <see cref="EnableState" /> value before the requested change would be executed.
    /// </summary>
    public required EnableState OldEffectiveState { get; init; }

    /// <summary>
    ///   Gets the effective <see cref="EnableState" /> value that is predicted to result after this change would be executed.
    /// </summary>
    /// <remarks>
    ///   This value is only guaranteed to be accurate at the time the event is initially raised. Subscribers may alter state.
    /// </remarks>
    public required EnableState PredictedEffectiveState { get; init; }

    /// <summary>
    ///   Gets a reference to the instance of the <see cref="ISupportsEnableDisable" /> that is the intended target of the change.
    /// </summary>
    public required ISupportsEnableDisable Target { get; init; }

    /// <inheritdoc />
    ~EnablingDisablingEventArgs () { Dispose (false); }
}
dodexahedron commented 8 months ago

Now, why is this model better than just having a boolean flag like "Canceled" or something like that? Lots of reasons. But I'll list the first big ones that come to mind:

dodexahedron commented 8 months ago

Status update:

I tried merging the latest state of the constructor removal branch into what I've done so far, for this, and the merge conflicts are so numerous and kinda pointless to try to resolve safely that I'm going to stash non-conflicting changes such as newly-created types and whatnot, reset or delete this branch, and rebase on the constructor removal branch.

I'm not sure how much more I'll really be able to do, beyond just making new types, until that work is completed and merged.

tig commented 8 months ago

Status update:

I tried merging the latest state of the constructor removal branch into what I've done so far, for this, and the merge conflicts are so numerous and kinda pointless to try to resolve safely that I'm going to stash non-conflicting changes such as newly-created types and whatnot, reset or delete this branch, and rebase on the constructor removal branch.

I'm not sure how much more I'll really be able to do, beyond just making new types, until that work is completed and merged.

Yep. This is a massive mess.

I hope to have the time today to spend the hours and hours it will take to get @BDisp's #3181 and my #3202 merged.

dodexahedron commented 8 months ago

I'm more than happy to split some of the work if you like, since this is a pretty big work item to get over and really should block all other work til it's done so it doesn't get any crazier than it already is. 😅

At least we won't have to deal with it any more (or at least it'll be minimal) afterward!

As for how to split the work....

I could branch from the same point and resolve merge conflicts for a specified subset of the files, with the same sort of review and whatnot that would normally occur on a merge to master, and then that gets merged into this one, once it's confirmed that subset is good, just taking the incoming versions wholesale, here.

I [sym|em]pathize with you about the sheer volume and tedium of this one. 😆

tig commented 8 months ago

I'm more than happy to split some of the work if you like, since this is a pretty big work item to get over and really should block all other work til it's done so it doesn't get any crazier than it already is. 😅

At least we won't have to deal with it any more (or at least it'll be minimal) afterward!

As for how to split the work....

I could branch from the same point and resolve merge conflicts for a specified subset of the files, with the same sort of review and whatnot that would normally occur on a merge to master, and then that gets merged into this one, once it's confirmed that subset is good, just taking the incoming versions wholesale, here.

I [sym|em]pathize with you about the sheer volume and tedium of this one. 😆

What would you say to focusing on getting #3202 merged asap, and then tackling this on a class by class (or component) basis? That's what we did when we made the first pass a addressing event inconsistencies back in the early days of v2.

dodexahedron commented 8 months ago

Unfortunately, a lot of what needs to be done isn't the sort of thing that is compatible with that approach.

But, I'm ok with the fact I'm going to have to frequently merge mainline code back into my branch to avoid excessive conflicts.

However, your suggestion makes me more inclined to still do it in a piecemeal fashion, just in a different way.

Since interfaces are a key and core part of it all, I can do it one interface at a time, which will take care of subsets of events across all types involved with them. Then I won't have to give one monolithic PR with all of it at once, and everyone else can work with the updated code as I finish with each set.

Sound like a nice middle ground?

dodexahedron commented 5 months ago

Just to update status on this...

This is on hold til things stabilize more and can be handled much more cleanly at that time. Not closing, though, as it's very much still my intent to do this later.