Rename `StateChangedEventArgs<T>` to `CancelEventArgs<T>`

[tig]

I mis-named this. I originally thought it was just for representing the property that represented the primary state of a View, but that's way to restrictive. This class is useful for any event that is tracking the state of a property.

[tig]

Related:

I introduced some related changes I'm now not happy with around Handled vs Cancel. Dotnet includes

CancelEventArgs and HandledEventArgs base classes. I went down a path of changing some cases of "Handled" to "Cancel" and I shouldn't have. Here's what I now think is right. Debatable for sure...

• Handled (and deriving from HandledEventArgs) applies to scenarios where an event can either be handled by an event listener (or override) vs not handled. User interface events like mouse moves and key presses are examples.
• Cancel (and deriving from CancelEventArgs) appiles to scenarios where something can be cancelled. Changing the Orientation of a Slider is cancelable and thus OrientationChanging should use CancelEventArgs.

Precisely where I screwed up:

• HighlightEventArgs - Should be handled
• View.Accept - Should be handled

[BDisp]

Please don't forget to fix the issue where the Dialog are opening again after the Ok/Cancel button is clicked. I thin that is related with View.Accept. Thanks.

[tig]

Please file an Issue.

[dodexahedron]

Please don't use that name. That is the name of the built-in type for the PropertyChanged event in INotifyPropertyChanged. We don't need more name collisions.

https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.propertychangedeventargs?view=net-8.0

Which, if there's a custom version of that interface for some reason, we should be using the standard. That's a VERY commonly used piece of functionality in .net.

[BDisp]

Please file an Issue.

Done in #3566.

[BDisp]

I think @dodexahedron is right in insisting not to change the name to PropertyChangedEventArgs because it would collide with the existing one. Now, if the issue is that you don't like the current name of StateChangedEventArgs, you can use another name other than the one that already exists in the system.

[dodexahedron]

For common types, in particular.

Attribute already creates enough fun when wanting to use actual attributes, since it's in our root namespace.

[tig]

Ok, how's SomethingChangedEventArgs<T>? Lol.

[BDisp]

Ok, how's SomethingChangedEventArgs<T>? Lol.

No, blablablaChangedEventArgs<T> is much better. :-)

Leave it as is, in my opinion.

[dodexahedron]

Why not use the built-in property change notification interfaces? They come with caller info tracking, too.

And then people can use them like they're already used to everywhere else.

An example, when I eventually write it against TG v2 at a future time: The XAML-based Terminal.Gui generator I want to make, which is intended to accept the same WPF/WinUI XAML used to produce the graphical applications and spit out a text mode UI from it, without code changes.

INotifyPropertyChanged and INotifyPropertyChanging are widely used and, in the example of WPF/WinUI and such, it's required for live data binding. And it's used elsewhere throughout .net as well.

In general, if we can use standard interfaces for things, it's good to do so - interop with other components being one of the biggest reasons.

[dodexahedron]

There are also IObservable<T> and IObserver<T>, which are meant for similar purposes as those, but for the observer pattern.

[tig]

I have a long love hate relationship with INPC.

Remember I'm the guy who put Silverlight into Windows Phone.

I don't need to cover the deficiencies as they're well documented.

We should be using it more and I recently put a use in as part of DimAuto.

However, this discussion is really about the fact that the built in PCEV class is not a generic.

[BDisp]

As the event is derived from CancelEventArgs I suggest that the class name could be CancellablePropertyChangedEventArgs. This way the user will know that this class will contain the current and previous values ​​that can be cancelled.

[tig]

Regarding wanting to migrate to INPC: I love the fact TG currently supports cancel/handle for events. INPC has no provision for that (and don't suggest using INotifyPropertyChanging... it was not designed to support cancelation). We will need a way to figure that out.

[tig]

INotifyPropertyChanged and INotifyPropertyChanging are widely used and, in the example of WPF/WinUI and such, it's required for live data binding. And it's used elsewhere throughout .net as well.

Thinking more... "live data binding" is the crux of this debate, me thinks.

Tenets for Terminal.Gui Eventing (Unless you know better ones...)

• UI Interaction and Live Data Are Different Beasts - TG distinguishes between events used for human interaction and events for live data. We don't believe in a one-size-fits-all eventing model. For UI interactions we use the event Cancel/HandleEvent pattern. For data we think INotifyPropertyChanged is groovy.

(How's that sit?)

[BDisp]

They really are different situations. The existing StateEventArgs is derived from CancelEventArgs which itself is derived from EventArgs. Therefore ideal for human interaction events. For live data events, the ideal is to use a class derived from INotifyPropertyChanged, which will be used in the properties that we want to notify by invoking the PropertyChangedEventHandler, which will inform which property was changed. If we want this change to be cancelable then it will be necessary to invoke a cancelable event before the property is set. Therefore, PropertyChangedEventHandler is different from PropertyChangedEventArgs.

[tig]

Related:

I introduced some related changes I'm now not happy with around Handled vs Cancel. Dotnet includes

CancelEventArgs and HandledEventArgs base classes. I went down a path of changing some cases of "Handled" to "Cancel" and I shouldn't have. Here's what I now think is right. Debatable for sure...

• Handled (and deriving from HandledEventArgs) applies to scenarios where an event can either be handled by an event listener (or override) vs not handled. User interface events like mouse moves and key presses are examples.
• Cancel (and deriving from CancelEventArgs) appiles to scenarios where something can be cancelled. Changing the Orientation of a Slider is cancelable and thus OrientationChanging should use CancelEventArgs.

Precisely where I screwed up:

• HighlightEventArgs - Should be handled
• View.Accept - Should be handled

This is fixed in https://github.com/gui-cs/Terminal.Gui/pull/3571

[tig]

I suggest that the class name could be CancellablePropertyChangedEventArgs. This way the user will know that this class will contain the current and previous values ​​that can be cancelled.

Not bad, but pretty long.

Only CancelEventArgs (not generic) is built-in. What if we renamed StateChangedEventArgs<T> to CancelEventArgs<T>?

[dodexahedron]

I have a long love hate relationship with INPC.

Remember I'm the guy who put Silverlight into Windows Phone.

I don't need to cover the deficiencies as they're well documented.

We should be using it more and I recently put a use in as part of DimAuto.

However, this discussion is really about the fact that the built in PCEV class is not a generic.

All that's necessary to support both, while still using a custom, is having yours inherit from the standard one, or just implement both. Explicit interface implementations also work, but that always feels a big janky to me, when it's not necessary.

There have definitely been plenty of baffling decisions on some components, like that, in .net. It's been nice to at least get a glimpse into some of them, though, since it's been open sourced - especially when they post design meeting notes and stuff like that. That has more than once turned baffled anger at a design choice into frustrated empathy, upon learning the real why, or part of it. 😅

I suggest that the class name could be CancellablePropertyChangedEventArgs. This way the user will know that this class will contain the current and previous values ​​that can be cancelled.

Not bad, but pretty long.

Only CancelEventArgs (not generic) is built-in. What if we renamed StateChangedEventArgs<T> to CancelEventArgs<T>?

Hm. Yeah I think a public class CancelEventArgs<T> : CancelEventArgs where T : [appropriate filter expression] sounds perfectly reasonable.

[dodexahedron]

I have long wished they would make an IEventArgs interface, too, and switch to using that instead of the base EventArgs class everywhere. The original EventArgs class is pretty useless, being a literally empty type, with a static on it that is for representing the empty version of....an empty concrete class. It should have at least been abstract or something...

Here's the literal entirety of the source code of it:

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;

namespace System
{
    // The base class for all event classes.
    [Serializable]
    [TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
    public class EventArgs
    {
        public static readonly EventArgs Empty = new EventArgs();

        public EventArgs()
        {
        }
    }
}

[dodexahedron]

Of course you can also abuse the hell out of the mechanism by your derived eventargs type having a delegate, so you can inject the method signature you want, with the added bonus of being harder to debug! 😆

(please don't haha)

[tig]

Please elaborate on this: [appropriate filter expression] in as specific way possible.

[dodexahedron]

That's a generic type parameter constraint clause.

They are restrictions on what is allowed for the type parameter, and are specified with the same syntax as inheritance, but also have some additional keywords available for control that is relevant to generics, such as requiring a class (class), a struct (struct), a parameterless constructor (new()), soft non-null (ie can be, but will just emit a warning: notnull), etc.

Without a type parameter constraint, a type parameter is actually less selective than even object is, which can lead to some rather interesting and unexpected behaviors, especially around implicitly-defined methods such as .Equals(), because the object overloads of things will get used preferentially over an unconstrained type parameter of a generic, if the method in the body of the generic method can be figured out by Roslyn to require a specific type (but that's a rare case, and also usually means it isn't really generic, though it is written that way).

That issue will show up in unit tests, if you pay close attention to the coverage results when running cases intended to use a specific generic method. And, when record structs are passed into an unconstrained generic method that checks for equality anywhere, you can get a StackOverflowException, due to how record struct and struct equality is not the same.

Check out the review I just finished on your IDesignable goodies for a very simple example that uses notnull.

For the rest of what you can do and specifics of how it works, here's the doc on where.

[BDisp]

Not bad, but pretty long.

Only CancelEventArgs (not generic) is built-in. What if we renamed StateChangedEventArgs<T> to CancelEventArgs<T>?

I agree @tig. Good choice.

[tig]

That's a generic type parameter constraint clause.

They are restrictions on what is allowed for the type parameter, and are specified with the same syntax as inheritance, but also have some additional keywords available for control that is relevant to generics, such as requiring a class (class), a struct (struct), a parameterless constructor (new()), soft non-null (ie can be, but will just emit a warning: notnull), etc.

Without a type parameter constraint, a type parameter is actually less selective than even object is, which can lead to some rather interesting and unexpected behaviors, especially around implicitly-defined methods such as .Equals(), because the object overloads of things will get used preferentially over an unconstrained type parameter of a generic, if the method in the body of the generic method can be figured out by Roslyn to require a specific type (but that's a rare case, and also usually means it isn't really generic, though it is written that way).

That issue will show up in unit tests, if you pay close attention to the coverage results when running cases intended to use a specific generic method. And, when record structs are passed into an unconstrained generic method that checks for equality anywhere, you can get a StackOverflowException, due to how record struct and struct equality is not the same.

Check out the review I just finished on your IDesignable goodies for a very simple example that uses notnull.

For the rest of what you can do and specifics of how it works, here's the doc on where.

I understand. But I still have no idea what constraints to put in for this particular case.

[dodexahedron]

I stuck some more details in my previous comment, covering some things that can or will happen, with a fully open generic, usually for some poor sap who calls it from their code and doesn't realize the method they think is being called is really not, and their value type is getting boxed, among other fun things that sometimes work and sometimes break catastrophically.

[dodexahedron]

Good timing haha.

No prob. I'll check out your branch and take a look at it in-situ to see if there's anything relevant. There may not be, which is also cool.

[dodexahedron]

Aaaaannnnd cue VS bug. It decided it wanted to REFUSE to show me that file. GRR. Close VS, delete obj folder, re-launch of course fixed it, but GDI! 😠

Just venting haha. It's loading back up right now.

[dodexahedron]

Wait. I was looking at the IDesignable branch.

Have you pushed current work on this branch to your remote yet?

[tig]

Oops. Now pushed.

[dodexahedron]

Coolio.

Now I'll take a look! 🙂

[dodexahedron]

Hm... What's the branch called? I don't see one with 3561 in it. Is it part of tig/v2_3570-TextView-ENTER?

That one was pushed around the time of that comment, anyway.

[dodexahedron]

Yep that's the one. I see the commit mentioning this.

Taking a look once I pull it down.

[dodexahedron]

I have half an essay typed up and realized the code changes are like 1/10 that, so I'll just summarize:

Biggest thing I'd do here is make two types: one for reference types and one for value types.

Why?

Because, without more work that is very subtle, value types and reference types will have radically different behaviors with this class. Value types work as expected for the most part, but reference types are very not thread-safe.

The base class libraries seem to get around those problems a few ways most frequently:

• Simply documenting that they're not thread safe, pushing the responsibility entirely off onto the consumer.
• Not making a generic and just letting the consumer (us, in this case) figure it out for themselves. Anything that came after .net framework 2.0 that seems like it should but curiously doesn't have a generic form is probably a good bet this was the reason.
• If they did go generic, doing one of various different things different with that generic, such as:
  • Really permissive but subtly yet profoundly important type parameter constraints (like class, an interface, new(), etc), and just falling back on the "not our job" case above for whatever that excludes.
  • Making two generic types - one that takes struct and one that takes class.
    • Usually that also comes with the types themselves being a struct and a class (like ValueTuple and Tuple), but that's not a requirement of that option.
    • This works best with an interface, so you can then have the delegates accept the interface and the user can pick which actual type implementing that interface they want to use.

I think the "just copy it and make one take struct and the other take class" method is probably the simplest, here, combined with documenting the lack of thread-safety of the class version.

And then an interface that covers both, for use in your delegates.

[dodexahedron]

And yeah.... That's the shorter version lol.

[dodexahedron]

Otherwise, if the "just document that it's their problem" option is selected, I'd at least use where notnull to help the consumer with a warning if they do goofy stuff.

In any case, here's a starting point that isn't much of a change at all, but does add some formalities:

/// <summary>
///     <see cref="EventArgs"/> for events that convey changes to a property of type <typeparamref name="T"/>.
/// </summary>
/// <typeparam name="T">The type of the value that was part of the change being canceled.</typeparam>
/// <remarks>
///     Events that use this class can be cancellable. Where applicable, the <see cref="CancelEventArgs.Cancel"/> property
///     should be set to
///     <see langword="true"/> to prevent the state change from occurring.
/// </remarks>
public class CancelEventArgs<T> : CancelEventArgs where T : notnull
{
    /// <summary>Initializes a new instance of the <see cref="CancelEventArgs{T}"/> class.</summary>
    /// <param name="currentValue">The current (old) value of the property.</param>
    /// <param name="newValue">The value the property will be set to if the event is not cancelled.</param>
    /// <param name="cancel">Whether the event should be canceled or not.</param>
    /// <typeparam name="T">The type of the value for the change being canceled.</typeparam>
    public CancelEventArgs (T currentValue, T newValue, bool cancel = false) : base (cancel)
    {
        CurrentValue = currentValue;
        NewValue = newValue;
    }

    /// <summary>The value the property will be set to if the event is not cancelled.</summary>
    public T NewValue { get; set; }

    /// <summary>The current value of the property.</summary>
    public T CurrentValue { get; }
}

In particular, xmldoc for type parameters, the constraint of notnull (otherwise they may as well use the regular CancelEventArgs class) annnnd... I think that's all I did to it. 🤔 Not much.

[dodexahedron]

Oh. And the constructor adds an optional cancel parameter, passed to the base type.

[BDisp]

What happens when T is a nullable type and null is being passed, will notnull allowed it?

[dodexahedron]

Yes. You get a warning from the compiler that you're doing something suboptimal, but it is allowed.

[tig]

Yes. You get a warning from the compiler that you're doing something suboptimal, but it is allowed.

CheckBox.Toggled is bool?.

Thus I'm getting this warning. I'm not sure what the correct thing to do is. I know ignoring the warning is not the correct thing here.

[tig]

I am struggling to understand this:

I think the "just copy it and make one take struct and the other take class" method is probably the simplest, here, combined with documenting the lack of thread-safety of the class version.

And then an interface that covers both, for use in your delegates.

What is "it" in "just copy it"?

What would an "interface that covers both" look like?

I'm feeling particularly dense RN.

[BDisp]

Remove the where T : notnull from the class.

[dodexahedron]

Yes. You get a warning from the compiler that you're doing something suboptimal, but it is allowed.

CheckBox.Toggled is bool?.

Thus I'm getting this warning. I'm not sure what the correct thing to do is. I know ignoring the warning is not the correct thing here.

That's where you use the dammit operator (!). That's actually what it's for - when you know it's null, and that's not actually incorrect behavior.

The compiler is warning you because there's no indication that you're aware of the possibility, until you tell it in some way that you are or until null isn't possible.

Saying null!, for example, is your acknowledgement that you are aware and have taken responsibility, in that specific place, without disabling the analysis or simply ignoring it which, as you mentioned, is, in fact, a poor procedure.

! gets misused quite a bit, probably partially because many analyzers offer it as a code fix where it would at least quench the warning, and because it feels really similar to the null conditional operator (?) and all its compound operators like null conditional member access (?.). But, if there's a warning that null is possible, it's about 99 to 1 that an unexpected null is possible (not applicable here, of course, because you're intentionally using null), so you either accept that responsibility with !, tell it you did it on purpose with !, or fix the source of the null in some other way.

Type parameter constraints are attributes. In fact, this is what that notnull constraint results in, during code generation, before the actual compilation occurs:

[NullableContext(1)]
[Nullable(0)]
public class CancelEventArgs<T> : CancelEventArgs

You're not supposed to use the NullableContextAttribute directly, though, and will probably get a warning if you do. That's what the #nullable xxxx or the project-level configuration element is for. And the NullableAttribute will also give a warning, when nullable context is enabled, telling you to use the annotation, rather than the attribute.

[tig]

Great in theory. But in practice there's no way to use ! here, as far as I can tell.

[dodexahedron]

Also, it's important to remember that a nullable struct is not capable of being null. It always exists as a Nullable value, which simply has its HasValue set to false. They are not null, which creates syntax differences, which is another reason why generics that mix structs and classes can be annoying.

[dodexahedron]

Good timing. I just was writing exactly what you just ran into.

Crossing the streams gets annoying.

BUT!

You can just write explicit constructors for those, if you want to use the same class.

[tig]

Good timing. I just was writing exactly what you just ran into.

Crossing the streams gets annoying.

BUT!

You can just write explicit constructors for those, if you want to use the same class.

What does "those" refer to here?

[dodexahedron]

What I mean by that is this:

    public CancelEventArgs (bool? currentValue, bool? newValue, bool cancel = false) : base (cancel)
    {
        // do what you gotta do.
    }

[dodexahedron]

Factory methods are usually a better idea than constructors, there, because that results in a lot less duplication.

[dodexahedron]

But yeah... Structs and classes accepted into the same generic that actually does something with it other than sticking it in an array is fraught with peril and frustration, unless you are able to restrict it to something like an interface.