Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Element.OnChildRemoved() - Incorrect variable used in function. #12633

Closed
Martyf101 opened this issue Jan 13, 2023 · 4 comments · Fixed by #11741
Closed

Element.OnChildRemoved() - Incorrect variable used in function. #12633

Martyf101 opened this issue Jan 13, 2023 · 4 comments · Fixed by #11741
Assignees
Labels
fixed-in-7.0.58 Look for this fix in 7.0.58! fixed-in-7.0.100 fixed-in-7.0.101 fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! t/bug Something isn't working

Comments

@Martyf101
Copy link

Wrong variable in function call prevents retrieving required information:

ChildRemoved?.Invoke(child, new ElementEventArgs(child));

The first parameter should be "this" not "child", as its already in the second parameters EventArgs and not the sender.

Here is the ChildAdded line for comparison, with the correct parameters:

ChildAdded?.Invoke(this, new ElementEventArgs(child));

Element.cs lines 344-370 :

`

	protected virtual void OnChildAdded(Element child)
	{
		child.Parent = this;

		child.ApplyBindings(skipBindingContext: false, fromBindingContextChanged: true);

		ChildAdded?.Invoke(this, new ElementEventArgs(child));

		VisualDiagnostics.OnChildAdded(this, child);

		OnDescendantAdded(child);
		foreach (Element element in child.Descendants())
			OnDescendantAdded(element);
	}

	protected virtual void OnChildRemoved(Element child, int oldLogicalIndex)
	{
		child.Parent = null;

		ChildRemoved?.Invoke(child, new ElementEventArgs(child));

		VisualDiagnostics.OnChildRemoved(this, child, oldLogicalIndex);

		OnDescendantRemoved(child);
		foreach (Element element in child.Descendants())
			OnDescendantRemoved(element);
	}`

The following call throws "Specified cast is not valid." error as the sender is incorrectly set.

childPage.ChildRemoved += (s, e) => System.Diagnostics.Debug.WriteLine(((NavigationPage)s).Height);

Element.cs Line 363 should be written as:

ChildRemoved?.Invoke(this, new ElementEventArgs(child));

@jsuarezruiz jsuarezruiz added the t/bug Something isn't working label Jan 13, 2023
@jsuarezruiz jsuarezruiz added this to the Backlog milestone Jan 13, 2023
@jsuarezruiz jsuarezruiz self-assigned this Jan 13, 2023
@ghost
Copy link

ghost commented Jan 13, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@hartez
Copy link
Contributor

hartez commented Jan 13, 2023

This was fixed in November: f9c7e4d

What branch are you seeing this on?

@Martyf101
Copy link
Author

Okay I can see this is fixed now on main branch. I would have got there though the search/reference popup on GitHub.

However it is still sending 'child' in the version I'm running, and I appear to be fully updated : 6.0.548

childPage.ChildRemoved += (s, e) => { if (s == e.Element) System.Diagnostics.Debug.WriteLine("SENDER EQUALS ELEMENT"); }

Shouldn't this code already be in the released version?

@hartez
Copy link
Contributor

hartez commented Jan 17, 2023

It's been backported to 7.0 and will be in the next 7.0 service release (which should be coming soon). It has not been backported to 6.0 and there are currently no plans to do so.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2023
@samhouts samhouts added fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! fixed-in-7.0.58 Look for this fix in 7.0.58! labels May 10, 2023
@samhouts samhouts modified the milestones: Backlog, .NET 8 May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fixed-in-7.0.58 Look for this fix in 7.0.58! fixed-in-7.0.100 fixed-in-7.0.101 fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants