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

INotifyCollectionChanged is implemented inconsistently for Move and Replace operations. #17157

Open
mpylon opened this issue Sep 28, 2024 · 0 comments · May be fixed by #17171
Open

INotifyCollectionChanged is implemented inconsistently for Move and Replace operations. #17157

mpylon opened this issue Sep 28, 2024 · 0 comments · May be fixed by #17171
Labels

Comments

@mpylon
Copy link
Contributor

mpylon commented Sep 28, 2024

Describe the bug

Various INotifyCollectionChanged.CollectionChanged event handlers in Avalonia have an off-by-one error and / or fail to correctly respond to multi-item change events. For example, the following event handler does not respond to multi-item Move or Replace operations correctly.

case NotifyCollectionChangedAction.Replace:
case NotifyCollectionChangedAction.Move:
Remove(e.OldStartingIndex, e.OldItems!.Count);
Add(e.NewStartingIndex, e.NewItems!);
break;

To correctly implement this type of behavior, we should follow the example of MAUI.

https://github.com/dotnet/maui/blob/cc683f0e99547c2ed4875296d7451332ef64ca73/src/Controls/src/Core/Internals/NotifyCollectionChangedEventArgsExtensions.cs#L47-L61

Furthermore, the event publishers are not consistent in their interpretations of INotifyCollectionChanged. For example, the AvaloniaList.Move and AvaloniaList.MoveRange methods produce different events for the same mutation (or, equivalently, the same events for different mutations). See the below unit test reflecting this incongruity.

[TestFixture]
public class Tests
{
    [Test]
    public void Move()
    {
        AvaloniaList<int> items = [1, 2, 3];

        using (new AssertionScope())
        {
            AssertEvent(items, () => items.Move(0, 1), new NotifyCollectionChangedEventArgs(
                action: NotifyCollectionChangedAction.Move,
                changedItems: new[] { 1 },
                index: 1,
                oldIndex: 0));

            items.ToArray().Should().BeEquivalentTo(new[] { 2, 1, 3 }, o => o.WithStrictOrdering());
        }
    }

    [Test]
    public void MoveRange()
    {
        AvaloniaList<int> items = [1, 2, 3];

        using (new AssertionScope())
        {
            AssertEvent(items, () => items.MoveRange(0, 1, 1), new NotifyCollectionChangedEventArgs(
                action: NotifyCollectionChangedAction.Move,
                changedItems: new[] { 1 },
                index: 1,
                oldIndex: 0));

            items.ToArray().Should().BeEquivalentTo(new[] { 2, 1, 3 }, o => o.WithStrictOrdering());
        }
    }

    private static void AssertEvent(INotifyCollectionChanged items, Action action, NotifyCollectionChangedEventArgs expectedEvent)
    {
        items.CollectionChanged += OnCollectionChanged;

        try
        {
            action();
        }
        finally
        {
            items.CollectionChanged -= OnCollectionChanged;
        }

        return;

        void OnCollectionChanged(object? sender, NotifyCollectionChangedEventArgs actualEvent)
        {
            actualEvent.Should().BeEquivalentTo(expectedEvent);
        }
    }
}

To Reproduce

To reproduce one of these behaviors, call AvaloniaList.MoveRange and verify that it successfully mutates the list, but fails to correctly update the PanelContainerGenerator.

[AvaloniaTest]
public void MoveRange()
{
    // Arrange

    AvaloniaList<int> items = [1];

    var itemsControl = new ItemsControl
    {
        ItemsPanel = new FuncTemplate<Panel?>(() => new Panel()),
        ItemsSource = items
    };

    var window = new Window
    {
        Content = itemsControl,
        Height = 768,
        Width = 1024
    };

    window.Show();

    // Act

    items.MoveRange(0, 1, 1);

    // Assert

    var actualContents = itemsControl
        .ItemsPanelRoot!
        .GetVisualChildren()
        .Cast<ContentPresenter>()
        .Select(x => x.Content);

    actualContents
        .Should()
        .BeEquivalentTo(items, o => o.WithStrictOrdering());
}

Expected behavior

  • The AvaloniaList.Move and AvaloniaList.MoveRange operations should throw exceptions before emitting events that will cause subsequent listeners to throw exceptions due to mismatched indices.
  • Move(X, Y) should have the same meaning as MoveRange(X, 1, Y).
  • INotifyCollectionChanged handlers throughout Avalonia should interpret indices in the same way.

Avalonia version

11.2.0-beta2

OS

Windows

Additional context

No response

@mpylon mpylon added the bug label Sep 28, 2024
mpylon added a commit to mpylon/Avalonia that referenced this issue Sep 30, 2024
mpylon added a commit to mpylon/Avalonia that referenced this issue Sep 30, 2024
@mpylon mpylon linked a pull request Sep 30, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant