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

refactor: Remove reflection usage in Brush #16741

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

dr1rrb
Copy link
Member

@dr1rrb dr1rrb commented May 15, 2024

Refactoring (no functional changes, no api changes)

Remove reflection usage in Brush

What is the current behavior?

We use reflection to invoke event handler

What is the new behavior?

No reflection!

PR Checklist

Please check if your PR fulfills the following requirements:

@dr1rrb dr1rrb enabled auto-merge May 15, 2024 15:53
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite scary class to touch, but I don't see leak concerns with the new impl. Just few comments.

I'd also love us to have this well-tested.

e.g (either runtime test or unit test should be fine, though my preference is always runtime test if no platform-specifics are involved):

using System;
using System.Collections.Generic;
using Uno.UI.Helpers;

namespace Uno.UI.RuntimeTests.Tests.Uno_UI;

[TestClass]
public class Given_WeakEventManager
{
	private sealed class EventPublisher
	{
		private readonly WeakEventManager _manager = new();

		internal event Action Event
		{
			add => _manager.AddEventHandler(value);
			remove => _manager.RemoveEventHandler(value);
		}
	}

	private sealed class EventSubscriber
	{
		public void M() { }
	}

	[TestMethod]
	public void When_Many_Subscriptions_Should_Not_Leak()
	{
		var publisher = new EventPublisher();
		var weakRefs = new List<WeakReference<EventSubscriber>>();
		Subscribe(publisher, weakRefs);

		for (int i = 0; i < 10; i++)
		{
			GC.Collect();
			GC.WaitForPendingFinalizers();
		}

		foreach (var x in weakRefs)
		{
			Assert.IsFalse(x.TryGetTarget(out _));
		}
	}

	private void Subscribe(EventPublisher publisher, List<WeakReference<EventSubscriber>> weakRefs)
	{
		for (int i = 0; i < 1000; i++)
		{
			var subscriber = new EventSubscriber();
			publisher.Event += subscriber.M;
			weakRefs.Add(new WeakReference<EventSubscriber>(subscriber));
		}
	}
}

Maybe also have additional tests around a handler that tries to subscribe/unsubscribe, re-entrancy, etc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI there are some tests from MAUI here. I haven't looked at them, but just in case there is any interesting test scenario there. This comment isn't necessarily to port them as-is, but just looking if there is anything interesting :)

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

Co-authored-by: Agnès ZITTE <[email protected]>
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

1 similar comment
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16741/index.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants