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

Remove from logical children and clear item container on reset #15855

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

Meloman19
Copy link
Contributor

What does the pull request do?

On collection reset remove from logical children and clear container (similarly like OnItemsChanged.Remove)

What is the current behavior?

Currently on collection reset removing from logical children happen after checking for ItemIsOwnContainer. And the container is not cleared at all.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048706-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented May 29, 2024

  • All contributors have signed the CLA.

@Meloman19
Copy link
Contributor Author

@cla-avalonia agree

var panel = _presenter.Panel;

foreach (var c in panel.Children)
{
itemsControl.RemoveLogicalChild(c);

if (!c.IsSet(ItemIsOwnContainerProperty))
Copy link
Contributor

@jp2masa jp2masa May 30, 2024

Choose a reason for hiding this comment

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

This code was already here, but it looks a bit suspicious. It checks if the ItemIsOwnContainerProperty is not set, but it should also check if it's set to false, I think?

EDIT: I see, it's a private property and it's only set to true, so it's probably done like this for performance reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's just mistake made due to inattention. You can look at the local method Remove in OnItemsChanged at this file. When item removed from the list, it is always remove from logical children and IsOwn property used only for clearing.

@pavelovcharov
Copy link
Contributor

A simple test for this case could be added here

        [Fact]
        public void ContainerClearing_Is_Raised_When_ItemsSource_Is_Cleared()
        {
            using var app = Start();
            var itemsSource = new ObservableCollection<object> { "Foo", "Bar", "Baz" };
            var target = CreateTarget(itemsSource: itemsSource);

            var expectedContainers = itemsSource.Select(x => target.ContainerFromItem(x)).ToArray();  
            var actualContainers = new List<Control>();
            var raised = 0;
            
            target.ContainerClearing += (s, e) =>
            {
                actualContainers.Add(e.Container);
                ++raised;
            };
            
            itemsSource.Clear();
            
            Assert.Equal(3, raised);
            Assert.Equal(expectedContainers, actualContainers);
        }

@Meloman19
Copy link
Contributor Author

@pavelovcharov Thanks! I forgot about the test.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0048726-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 requested a review from grokys May 31, 2024 05:11
Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

Sorry about the delay in getting to this, looks good to me, thank you!

@grokys grokys added this pull request to the merge queue Jun 25, 2024
@maxkatz6 maxkatz6 removed this pull request from the merge queue due to a manual request Jun 25, 2024
@maxkatz6 maxkatz6 added bug backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels Jun 25, 2024
@maxkatz6 maxkatz6 merged commit 47cbad7 into AvaloniaUI:master Jun 25, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants