Rename `StateChangedEventArgs<T>` to `CancelEventArgs<T>` about terminal.gui HOT 63 CLOSED

Please file an Issue.

from terminal.gui.

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.

from terminal.gui.

For common types, in particular.

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

from terminal.gui.

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>?

from terminal.gui.

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.

from terminal.gui.

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

from terminal.gui.

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

from terminal.gui.

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.

from terminal.gui.

Please file an Issue.

Done in #3566.

from terminal.gui.

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.

from terminal.gui.

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

from terminal.gui.

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

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

Leave it as is, in my opinion.

from terminal.gui.

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.

from terminal.gui.

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

from terminal.gui.

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.

from terminal.gui.

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.

from terminal.gui.

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.

from terminal.gui.

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?)

from terminal.gui.

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.

from terminal.gui.

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 #3571

from terminal.gui.

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.

from terminal.gui.

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()
        {
        }
    }
}

from terminal.gui.

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)

from terminal.gui.

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

from terminal.gui.

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.

from terminal.gui.

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.

from terminal.gui.

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.

from terminal.gui.

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.

from terminal.gui.

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.

from terminal.gui.

Wait. I was looking at the IDesignable branch.

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

from terminal.gui.

Oops. Now pushed.

from terminal.gui.

Coolio.

Now I'll take a look! 🙂

from terminal.gui.

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.

from terminal.gui.

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

Taking a look once I pull it down.

from terminal.gui.

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.

from terminal.gui.

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

from terminal.gui.

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.

from terminal.gui.

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

from terminal.gui.

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

from terminal.gui.

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.

from terminal.gui.

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.

from terminal.gui.

Remove the where T : notnull from the class.

from terminal.gui.

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.

from terminal.gui.

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

from terminal.gui.

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.

from terminal.gui.

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.

from terminal.gui.

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?

from terminal.gui.

What I mean by that is this:

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

from terminal.gui.

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

from terminal.gui.

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.

from terminal.gui.

There is. It's just hidden. Because the code has only changed by the inclusion or exclusion of one element: [Nullable(0)] applied to the type parameter.

No execution differences exist.

The warnings you're getting are for a reason - because the execution model isn't what you actually expect and isn't consistent.

from terminal.gui.

There's no trouble if I don't add where T : notnull clause to the class.

from terminal.gui.

Weird that posted out of order...

Anyway it's a response to

There's no trouble if I don't add where T : notnull clause to the class.

Just copying it for convenience:

There is. It's just hidden. Because the code has only changed by the inclusion or exclusion of one element: [Nullable(0)] applied to the type parameter.

No execution differences exist.

The warnings you're getting are for a reason - because the execution model isn't what you actually expect and isn't consistent.

from terminal.gui.

The only reason they don't appear without it is because the analysis simply isn't being performed.

from terminal.gui.

Weird that posted out of order...

Anyway it's a response to

There's no trouble if I don't add where T : notnull clause to the class.

Just copying it for convenience:

There is. It's just hidden. Because the code has only changed by the inclusion or exclusion of one element: [Nullable(0)] applied to the type parameter.

No execution differences exist.

The warnings you're getting are for a reason - because the execution model isn't what you actually expect and isn't consistent.

But yeah, in reference to this...

Aside from the attribute, which isn't executed in any way at run time (and, in fact, won't even exist if the assembly is trimmed), you end up with identical IL with or without that type param constraint. Thus, it telling you there's something odd is legit.

Those generic method calls are resolved at compile time, even though the concrete types are created at JIT-time upon first encountering a new value type (reference types share one implementation in most cases).

To be run-time safe, anything we have that is generic that actually does anything with the data given to it with type T must exist either narrowly defined to only accept what will always work, for all types accepted, or has to have separate implementations for the cases that don't behave the same implicitly.

It's annoying and makes it understandable why the option to not use a generic is still sometimes chosen in new BCL features to this day.

from terminal.gui.

I guess I need you to provide a PR to my PR showing me exactly how to do this right because I have not been able to follow most of what you've explained. I will sleep on it and see if that helps, but I'm just not getting all the nuances. I struggle to understand some of your vague references and translate them into actual code:

• "make two types: one for reference types and one for value types."
• "just copy it and make one take struct and the other take class"
• "And then an interface that covers both, for use in your delegates."
• "Factory methods are usually a better idea than constructors, there, because that results in a lot less duplication."

from terminal.gui.

Well... The problem is it's a tough decision.

I, like you, really want and appreciate generics when possible.

But, they have some really irksome caveats like this, which require specific considerations and decisions to be made.

If you wanna do it that way, though, where you put it in as-is but minus the constraint, and the caveats can be addressed later, that's cool as far as I'm concerned.

But I'd make the events themselves accept the non-generic base class CancelEventArgs, instead, if you haven't already, both to avoid an API surface change and so consumers who don't need to pass the actual values in aren't forced to supply them.

from terminal.gui.

Or actually put it in with the constraint, so we have the warning as a reminder to fix it... That might be smarter.

But still with the event delegates accepting the immediate base class.

In any event-handlers for them, you simply do a type check to see if it's the thing you expect.

Because remember - if it got overridden and it's not what you expected, that's a run-time error.

from terminal.gui.

(Remember, I'm just whiteboarding here. This all isn't explicit answers/recommendations, and that's intentional - just options)

One of the issues that makes this annoying AF on our end is there are a ton of ways to approach it, but all have pros and cons. There's no perfect solution. There are even entire libraries out there that try to deal with this exact set of issues.

To get actually consistent behavior between struct and class, which is the bigger issue at play, here, I'd typically try to favor reference semantics OR value semantics, but not both (note I didn't say types). It's easier to support and easier to use. Reference semantics are a lot easier to implement. Value is harder both for us AND the consumer (even though C# is already pass/return by value...reference types muddy that a lot).

If you want a longer explanation with my general and preliminary opinions, I can certainly give you one.

But suffice it to say that the syntactic issues and the very legitimate warnings from analysis due to nullability (or lack thereof, for structs) are symptoms, and the disease is a clash of expectations with what the language and CLR actually do or can do, on its own, without extra work, which avoiding/reducing is part of the reason for wanting and using generics in the first place, right?. Just kinda moves where the work has to be done. 🤷‍♂️

from terminal.gui.

Or we just throw up our hands and let consumers deal with it, which is the lightest touch approach.

But then we also shouldn't do anything internally in the library that assumes one or the other without a graceful fallback, if that's the chosen path. And I'd also suggest you explicitly annotate the type parameter as nullable, if you do that (so, <T?>), to let the analyzer know null is allowed. Otherwise it is assuming it isn't.

from terminal.gui.

if you do that (so, <T?>

You can't do that. Gives a syntax error.

from terminal.gui.

BTW, after sleeping on it and writing a bunch of code, I now get what you've bee putting down. Mostly.

from terminal.gui.

Coolio. Yeah I saw the PR and it looks like you're going the right way.

Still some stuff we can shore up, but not important enough to hold it up IMO.

Maybe drop an appropriate [ComponentGuarantees] on that event, as well, for now, in case those tweaks are breaking?

from terminal.gui.