-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Removed private WeakEventListener#4450
Removed private WeakEventListener #4450puppetsw wants to merge 2 commits intoCommunityToolkit:main from
Conversation
NetworkConnectionStateTrigger.cs had a private duplicate version of WeakEventListener. This has now been removed and uses the existing WeakEventListener in the toolkit.
ghost
commented
Jan 10, 2022
Thanks puppetsw for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌
@michael-hawker
michael-hawker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this approach is still capturing the object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be NetworkInformation.NetworkStatusChanged += _weakEvent.OnEvent - otherwise we capture the event and defeat the purpose of the WeakEventListener, no?
I wonder if we need to look at the history, the original developer probably had the copy because of the strange delegate from NetworkStatusChanged... Trying to think of how we can work around this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NetworkInformation.NetworkStatusChanged += _weakEvent.OnEvent isn't possible because NetworkInformation.NetworkStatusChanged is a custom delgate, not a normal event handler.
We can probably use the sender to access fields on NetworkConnectionStateTrigger and make this method static, that might help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@puppetsw Let's do the same thing for the OnDetachAction if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Arlodotexe I'm sorry, but I'm not sure how to go about this, or I'm not understanding. If you could give a pointer I'm happy to make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dug into this a bit. Looks like OnDetachAction doesn't supply the access to the instance inside the delegate. Though it does null out the OnDetachAction after invoking it, so it should release the delegate and all references captured by it.
@michael-hawker does that sound right? Should be able to move forward if so.
Microsoft.Toolkit.Uwp.UI/Triggers/NetworkConnectionStateTrigger.cs
Outdated
Show resolved
Hide resolved
ghost
commented
Jan 19, 2022
This PR has been marked as "needs attention 👋" and awaiting a response from the team.
Arlodotexe
commented
Sep 26, 2022
Copied from the original issue:
While closing #4450, we noticed that the code is not identical to what we have in the toolkit. It removes
TEventArgsto allow us to doNetworkInformation.NetworkStatusChanged += weakEvent.OnEvent;.After discussing with @michael-hawker, we won't be removing the private event listener for now. Instead, we'll use the more flexible CommunityToolkit/dotnet#404 when it arrives.
NetworkConnectionStateTrigger.cs had a private duplicate version of WeakEventListener. This has now been removed and uses the existing WeakEventListener in the toolkit.
Fixes
Issue #4059
PR Type
What kind of change does this PR introduce?
Bugfix
What is the current behavior?
"NetworkConnectionStateTrigger is using a private copy of WeakEventListener, it's identical to the one we already have in the Toolkit, so we should update the source to use the existing code within the Toolkit."
What is the new behavior?
"Use existing code over shadow copy."
PR Checklist
Please check if your PR fulfills the following requirements:
Other information