Fixes - #2359 All events now use sender EventArgs

[tznind]

Fixes - #2359 Include a terse summary of the change or which issue is fixed.

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]

20 down, 48 to go 😓

[tznind]

Hmn so in the old version of virtual void OnAdded (View view) the view passed into this method was not passed on to Invoke. Instead this was passed to Invoke.

That means that the view reported by the Added event is the parent and not the child being added.

This is why the test is failing. Added_Removed asserts the following:

v.Added += (s,e) => {
	Assert.True (v.SuperView == e.View);
};

I propose we change this test to instead say

v.Added += (s,e) => {
	Assert.True (v.SuperView == s);
	Assert.True (v == e.View);
};

[tznind]

Actually I am confused about this test. This is what it looks like in v2_develop (see below)

The description for the Added event says:

Event fired when a subview is being added to this view.

But the test is registering the event on the child (v) and then calling add on the parent (t).

Is this to say then that Added event covers both

• When a subview is added to yourself
• When you are added as a subview to someone else?

public void Added_Removed ()
{
	var v = new View (new Rect (0, 0, 10, 24));
	var t = new View ();

	v.Added += (View e) => {
		Assert.True (v.SuperView == e);
	};

	v.Removed += (View e) => {
		Assert.True (v.SuperView == null);
	};

	t.Add (v);
	Assert.True (t.Subviews.Count == 1);

	t.Remove (v);
	Assert.True (t.Subviews.Count == 0);
}

[BDisp]

I agree. This event is normally used by the subview to get notified when the superview has add it and so your logic is perfectly right. By the way thanks for great effort.

[tznind]

• When a subview is added to yourself

• When you are added as a subview to someone else?

Ok this is not the case. I think the description and my expectation is in fact wrong. Based on the Add method, the Added event is fired on a View when it is aded to a parent. And not "when a subview is being added" [to it].

public virtual void Add (View view)
{
	if (view == null)
		return;
        [...]
	OnAdded (view);

[BDisp]

Yes, that why it's invoke view.Added?.Invoke (this);

[BDisp]

I propose this change, because if the user use the arg passed will think it's the superview and that why the test is failing.

public virtual void OnAdded (ViewEventArgs e)
{
	var view = e.View;
	view.IsAdded = true;
	view.x ??= view.frame.X;
	view.y ??= view.frame.Y;
	view.width ??= view.frame.Width;
	view.height ??= view.frame.Height;
	e.View = this;
	view.Added?.Invoke (this, e);
}

[BDisp]

The same for the OnRemoved;

[tznind]

Ok I have added a new EventArgs called SuperViewChangedEventArgs so it is clear to user what view is being talked about (since it contains both Parent and Child). Test now passes as:

Note: I thought about calling it ParentChangedEventArgs but its called SuperView in View so I thought better stick to it. Although I did go with Parent and Child for the properties.

[Fact]
public void Added_Removed ()
{
	var v = new View (new Rect (0, 0, 10, 24));
	var t = new View ();

	v.Added += (s,e) => {
		Assert.Same (v.SuperView, e.Parent);
		Assert.Same (t, e.Parent);
		Assert.Same (v, e.Child);
	};

	v.Removed += (s, e) => {
		Assert.Same (t, e.Parent);
		Assert.Same (v, e.Child);
		Assert.True (v.SuperView == null);
	};

	t.Add (v);
	Assert.True (t.Subviews.Count == 1);

	t.Remove (v);
	Assert.True (t.Subviews.Count == 0);
}

[BDisp]

Much better solution, congrats for your brilliant implementation.

[tig]

Fantastic stuff.

What about cancelable events?

[tznind]

Fantastic stuff.

Thanks. I'm keeping this in draft though as still working my way through all the event Action<x> events. There are a lot!

What about cancelable events?

Hopefully most would already use an event args with Handled (or similar) so should be ok I think?

[tznind]

Only 7 more to go 🏁 👁️ but looks like they will all need event args classes.

I've added any new EventArgs classes I've created in the EventArgs folder but using the normal namespace (Terminal.Gui). A lot of existing ones are nested within parent classes though which I think isn't ideal (makes them harder to find and take up more editor space in method signatures and generally not be friendly). But maybe can revisit that later in another PR!

[BDisp]

@tznind I submitted the PR tznind#163 to help a little on your awesome work. Thanks.

[BDisp]

@tig, @tznind should we have to create default EventArgs for empties EventHandler for the cases where eventually we need to have arguments later without breaking changes?

[BDisp]

@tig, @tznind should we have to create default EventArgs for empties EventHandler for the cases where eventually we need to have arguments later without breaking changes?

Well, I think we sure don't need to create a default EventArgs for empties EventHandler, like was done on ConfigurationManagerEventArgs, because we can invoke another EventArgs type on the argument parameter. In the subscriber method the EventArgs args are the same that was passed by the invoke method and thus there is no breaking change if we invoke another EventArgs type. I tested it and it work great. I'm wrong?

[tznind]

That's right, Now that we are using object, EventArgs we can pass any subclass of EventArgs we might create later.

[BDisp]

Now the problem is the lot of warnings due the use of #nullable enable. Some of them have quick fix but others not. What the best solution for those that doesn't have a quick fix?

[tznind]

Now the problem is the lot of warnings due the use of #nullable enable. Some of them have quick fix but others not. What the best solution for those that doesn't have a quick fix?

Hmn thats odd, must have been a bad autocomplete suggestion or a side effect of refactoring command I used to pull out to seperate file. Seems they started appearing in 2ed71e2

I didn't mean to add it so I will just get rid of them.

[tznind]

Ah actually some of them were added by @tig for example in UICatalog in c94f916

See c94f916#diff-81646b59a9d4df0e26e5e72c0dddde6f499db0ae941481edcd40326d5ec800eeR17-R18

Maybe this is a work in progress, I had better not go chop them out just yet.

[BDisp]

Ah ok we wait for @tig answer. Thanks.

[tig]

I'm quite confused about nullable. It just confuses the hell out of me.

We need a clear and concise set of guidelines for v2. I'm not smart enough (apparently) to author such a thing.

[tznind]

I'm quite confused about nullable. It just confuses the hell out of me.

We need a clear and concise set of guidelines for v2. I'm not smart enough (apparently) to author such a thing.

Before NRT

C# has value types and classes (aka reference types). Value types (int, bool, float, struct) cannot be null. Classes can be null.

A while ago the ? operator was added to allow Value types to be null (int?, bool? etc).

Classes can be null, so MyClass blah; means a variable of type Myclass. The variable will start out as null and may be assigned values or null later on etc.

You do not need the #nullable enable directive to use these features and they are not related to NRT - Nullable Reference Types.

NRT - Nullable Reference Types

You can turn on NRT for a file with the #nullable enable or for the whole project as a csproj setting.

Enabling NRT changes MyClass blah; from meaning "A variable of type 'MyClass'" to meaning "a variable of type 'MyClass' which will NEVER BE NULL!".

Obviously when you turn this on you immediately start getting compiler warnings that basically say:

"You told me this variable will never be null but then you assigned it null!"

So you have to manually add a question mark to the class i.e. MyClass? blah. Adding the question mark restores the previous interpretation before you enabled NRT (i.e. "A variable of type 'MyClass'").

You also have to do this for methods e.g. if you return null from a method it needs to have the question mark on the class name returned too e.g. public MyClass? MaybeGetInstance().

This means you end up with questionmarks all over the place to limited value. You can see this when methods like string? ToString(). That method should almost never return null but since theres some corner cases in some persons library where they do return null then the signature needs the question mark.

Worse still is that when you call ToString, because theres a question mark on the Type the compiler makes the assumption that "it could return null!" which means any variable you assign it to needs to allso allow null so you end up with:

string? name = thing.ToString();

or (my prefered option)

string name = thing.ToString() ?? throw new Exception("ToString method unexpectedly returned null");

The first case has the big down side that anyone who references or takes string? name later on needs to also be considered "might be null". The second option is you have a throw which will probably never ever get met and is just there to satisfy compiler warnings.
For more info read here: https://devblogs.microsoft.com/dotnet/embracing-nullable-reference-types/

When is NRT a good idea

NRT is a great idea when you are first starting a project (Terminal.Gui.Designer has it for example) since it is not too much work when writing code to make informed decisions about whether a class instance should be allowed to be null.

NRT does make you think a lot more about nullability and if you have the spare effort to put in (think 2-3 weeks of solid work for a library our size) then it can reduce the number of null references client libraries end up with.

[BDisp]

Well if it's for to have sure that a class never can be null and ensuring that all the consumers are instantiating a non null class, that really a good advantage of NRT. But if it's for end up putting ? around all the code and acting as non NRT, then is better remove the #nullable enable, right?

[tznind]

Well if it's for to have sure that a class never can be null and ensuring that all the consumers are instantiating a non null class, that really a good advantage of NRT. But if it's for end up putting ? around all the code and acting as non NRT, then is better remove the #nullable enable, right?

Yes that is the trade off.

NRT is great for a new codebase. But when you start getting into NRT in an existing big codebase then it starts to proliferate throughout the codebase. Despite the ability to enable it per file it still ends up spilling everywhere.

So you either spend a lot of effort doing it properly or you do a half way job (either suppressing the warnings or putting ? on everything everywhere). At least that is my experience of it.

But its a big enough task that we should probably make a branch specifically for implementing NRT if that's the way we want to go down rather than just turning it on ad-hoc as part of other work.

[tig]

What do y'all think about adopting INotifyPropertyChanged in v2?

[tznind]

What do y'all think about adopting INotifyPropertyChanged in v2?

I think this is a worthy goal but maybe do it as a seperate PR?

[tznind]

Tests are passing and the only warnings are around those nullability issues. I'm happy if you are ready to merge this?

[tig]

This is gonna be fun... Not.

[tig]

Excellent work @tznind !! This is huge.

[tznind]

Looks like I misnamed this now as MouseEventEventArgs instead of MouseEventArgs

[tznind]

Fix is in #2435

[BDisp]

@tznind I did the change on purpose to MouseEventEventArgs because it's related with the MouseEvent class, for the same reason why there is a MouseFlagsEventArgs related with MouseFlags enum.