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.

Sign up for GitHub

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

Removed private WeakEventListener #4450

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Removed private WeakEventListener #4450

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0450a40
Removed private WeakEventListener
puppetsw Jan 10, 2022
bc06cdd
Updated NetworkConnectionStateTrigger OnEventAction
puppetsw Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 16 additions & 71 deletions Microsoft.Toolkit.Uwp.UI/Triggers/NetworkConnectionStateTrigger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System;
using Microsoft.Toolkit.Uwp.Helpers;
using Windows.Networking.Connectivity;
using Windows.System;
using Windows.UI.Xaml;
Expand All @@ -14,6 +15,8 @@ namespace Microsoft.Toolkit.Uwp.UI.Triggers
/// </summary>
public class NetworkConnectionStateTrigger : StateTriggerBase
{
private readonly WeakEventListener<NetworkConnectionStateTrigger, object, EventArgs> _weakEvent;

private DispatcherQueue _dispatcherQueue;

/// <summary>
Expand All @@ -22,13 +25,14 @@ public class NetworkConnectionStateTrigger : StateTriggerBase
public NetworkConnectionStateTrigger()
{
_dispatcherQueue = DispatcherQueue.GetForCurrentThread();
var weakEvent =
new WeakEventListener<NetworkConnectionStateTrigger, object>(this)
{
OnEventAction = (instance, source) => NetworkInformation_NetworkStatusChanged(source),
OnDetachAction = (weakEventListener) => NetworkInformation.NetworkStatusChanged -= weakEventListener.OnEvent
};
NetworkInformation.NetworkStatusChanged += weakEvent.OnEvent;

_weakEvent = new WeakEventListener<NetworkConnectionStateTrigger, object, EventArgs>(this)
{
OnEventAction = (instance, source, eventArgs) => { NetworkInformation_NetworkStatusChanged(source); },
puppetsw marked this conversation as resolved.
Show resolved Hide resolved
OnDetachAction = listener => { NetworkInformation.NetworkStatusChanged -= OnNetworkEvent; }
};

NetworkInformation.NetworkStatusChanged += OnNetworkEvent;
Copy link
Member

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

Copy link
Member

@Arlodotexe Arlodotexe Jan 19, 2022 •
edited
Loading

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@Arlodotexe Arlodotexe Sep 13, 2022 •
edited
Loading

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.

UpdateState();
}

Expand All @@ -37,6 +41,11 @@ private void NetworkInformation_NetworkStatusChanged(object sender)
_ = _dispatcherQueue.EnqueueAsync(UpdateState, DispatcherQueuePriority.Normal);
}

private void OnNetworkEvent(object source)
{
_weakEvent?.OnEvent(source, EventArgs.Empty);
}

private void UpdateState()
{
bool isConnected = false;
Expand Down Expand Up @@ -69,70 +78,6 @@ private static void OnConnectionStatePropertyChanged(DependencyObject d, Depende
var obj = (NetworkConnectionStateTrigger)d;
obj.UpdateState();
}

private class WeakEventListener<TInstance, TSource>
where TInstance : class
{
/// <summary>
/// WeakReference to the instance listening for the event.
/// </summary>
private WeakReference _weakInstance;

/// <summary>
/// Gets or sets the method to call when the event fires.
/// </summary>
public Action<TInstance, TSource> OnEventAction { get; set; }

/// <summary>
/// Gets or sets the method to call when detaching from the event.
/// </summary>
public Action<WeakEventListener<TInstance, TSource>> OnDetachAction { get; set; }

/// <summary>
/// Initializes a new instance of the <see cref="WeakEventListener{TInstance, TSource}"/> class.
/// </summary>
/// <param name="instance">Instance subscribing to the event.</param>
public WeakEventListener(TInstance instance)
{
if (instance == null)
{
throw new ArgumentNullException("instance");
}

_weakInstance = new WeakReference(instance);
}

/// <summary>
/// Handler for the subscribed event calls OnEventAction to handle it.
/// </summary>
/// <param name="source">Event source.</param>
public void OnEvent(TSource source)
{
TInstance target = (TInstance)_weakInstance.Target;
if (target != null)
{
// Call registered action
OnEventAction?.Invoke(target, source);
}
else
{
// Detach from event
Detach();
}
}

/// <summary>
/// Detaches from the subscribed event.
/// </summary>
public void Detach()
{
if (OnDetachAction != null)
{
OnDetachAction(this);
OnDetachAction = null;
}
}
}
}

/// <summary>
Expand Down