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

Fix nth-last-child styles on virtualizing layouts #10055

Merged
merged 11 commits into from
Feb 23, 2023

Conversation

grokys
Copy link
Member

@grokys grokys commented Jan 23, 2023

What does the pull request do?

After #9677 was merged, nth-last-child styles stopped working in ListBox due to the problem described here.

This PR modifies the IChildIndexProvider API to add the following:

  • The new index of the child is now passed in ChildIndexChangedEventArgs
  • A TotalCountChanged event was added

It also removes the hack in ItemsControl which was used to work around this problem.

Questions

I've marked this PR as draft for the moment because I'm not sure about one thing:

We now have two events on IChildIndexProvider but they will always be subscribed by the same object. Maybe we need to merge the two events into a single event with an Action property which would be an enum of:

  • ChildIndexChanged
  • TotalCountChanged

Breaking changes

The IChildIndexProvider interface has changed. Shouldn't be much of a problem as it's unlikely to be client-implemented.

Fixed issues

Fixes #9997

Will hopefully make it a bit more obvious if it breaks in future: previously it was easier to miss as both `nth-child` and `nth-last-child` changed the foreground.
And a passing test for `nth-child` selector.
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028939-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

Comment on lines 40 to 45
_provider.TotalCountChanged += TotalCountChanged;
}

protected override void Deinitialize()
{
_provider.ChildIndexChanged -= ChildIndexChanged;
Copy link
Member

Choose a reason for hiding this comment

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

  1. Do we even need to subscribe on TotalCountChanged when it's not reversed?
  2. TotalCountChanged is not unsubscribed on Deinitialize

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I think this depends on the answer to the question in the PR description
  2. This too, really..,

@grokys
Copy link
Member Author

grokys commented Feb 21, 2023

@maxkatz6 ping - still waiting for your thoughts on the question in the PR description.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0030360-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member

We now have two events on IChildIndexProvider but they will always be subscribed by the same object. Maybe we need to merge the two events into a single event with an Action property which would be an enum.

I don't have strong preference here. Having a single event makes sense in general.

If keep two different events, then total count can be added to the TotalCountChanged event args. So TryGetTotalCount/GetChildIndex calls can be basically eliminated except of initial read.
But I think it's a hardly beneficial, as it will add allocations for an additional argument for each item added.
Without that, there is not much usage of independent TotalCountChanged.

Merge it into existing `IChildIndexProvider.ChildIndexChanged` event.
@grokys grokys marked this pull request as ready for review February 22, 2023 17:26
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0030423-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 merged commit bd5865f into master Feb 23, 2023
@maxkatz6 maxkatz6 deleted the fixes/9997-nth-last-child-itemscontrol branch February 23, 2023 03:38
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0030445-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

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

Successfully merging this pull request may close these issues.

"nth-last-child" is getting broken (not applying on the elements) after you scroll with scrollbar thumb.
3 participants