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

Consider reviewing IAvaloniaList.TrackItemPropertyChanged-extension #15681

Open
AliveDevil opened this issue May 10, 2024 · 0 comments
Open

Consider reviewing IAvaloniaList.TrackItemPropertyChanged-extension #15681

AliveDevil opened this issue May 10, 2024 · 0 comments

Comments

@AliveDevil
Copy link

Is your feature request related to a problem? Please describe.

Currently IAvaloniaList.TrackItemPropertyChanged-extension allocates a Tuple-object instance for each property changed event.
This can be made alloc-free by switching to ValueTuple.

Additionally the TrackItemPropertyChanged-method doesn't support resets, which should be reevaluated, as the tracking list can be used to dispose of all event registrations.

The Disposable returned by ForEachItem in TrackItemPropertyChanged is never explicitely disposed - which can result in never-collected and always firing NotifyPropertyChanged-handlers.

Describe the solution you'd like

API Proposal:

  namespace Avalonia.Collections;

  public static class AvaloniaListExtensions
  {
-     public static IDisposable TrackItemPropertyChanged<T>(
-         this IAvaloniaReadOnlyList<T> collection,
-         Action<Tuple<object?, PropertyChangedEventArgs>> callback);
+     public static IDisposable TrackItemPropertyChanged<T>(
+         this IAvaloniaReadOnlyList<T> collection,
+         Action<(object? Sender, PropertyChangedEventArgs Args)> callback);
}

Functionality change:

  public static IDisposable TrackItemPropertyChanged<T>(
              this IAvaloniaReadOnlyList<T> collection,
              Action<Tuple<object?, PropertyChangedEventArgs>> callback)
  {
      …
-     collection.ForEachItem(
+     var collectionSubscription = collection.ForEachItem(
      …
      return Disposable.Create(() =>
      {
+         collectionSubscription.Dispose();
          …
      };
}

Resetable TrackItemPropertyChanged:

- () => throw new NotSupportedException("Collection reset not supported."));
+ () =>
+ {
+     tracked.ForEach(x => x.PropertyChanged -= handler);
+     tracked.Clear();
+ }

Alternatively this could use PooledList<T> to create a copy, which is then disposed:

() =>
{
    using PooledList<INotifyPropertyChanged> copy = new(tracked);
    tracked.Clear();
    copy.ForEach(x => x.PropertyChanged -= handler);
}

Describe alternatives you've considered

I considered creating the functionality myself in an extension method, unfortunately Avalonia.Reactive.Observable and PooledList are internal.

Open for discussion is whether TrackItemPropertyChanged should be able to filter on AvaloniaPropertyChangedEventArgs and AvaloniaPropertyChangedEventArgs<T>.

Additional context

I'm open to providing a PR.

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

No branches or pull requests

1 participant