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-child styles on virtualized lists #13770

Merged
merged 5 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions src/Avalonia.Base/PropertyStore/BindingEntryBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,6 @@ private BindingEntryBase(
_uncommon = new() { _hasDataValidation = true };
}

public bool HasValue
{
get
{
Start(produceValue: false);
return _hasValue;
}
}

public bool IsSubscribed => _subscription is not null;
public AvaloniaProperty Property { get; }
AvaloniaProperty IValueEntry.Property => Property;
Expand All @@ -70,6 +61,12 @@ public void Dispose()
BindingCompleted();
}

public bool HasValue()
{
Start(produceValue: false);
return _hasValue;
}

public TValue GetValue()
{
Start(produceValue: false);
Expand Down
7 changes: 5 additions & 2 deletions src/Avalonia.Base/PropertyStore/IValueEntry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@ namespace Avalonia.PropertyStore
/// </summary>
internal interface IValueEntry
{
bool HasValue { get; }

/// <summary>
/// Gets the property that this value applies to.
/// </summary>
AvaloniaProperty Property { get; }

/// <summary>
/// Checks whether the entry has a value, starting the entry if necessary.
/// </summary>
bool HasValue();

/// <summary>
/// Gets the value associated with the entry.
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Avalonia.Base/PropertyStore/ImmediateValueEntry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ public ImmediateValueEntry(
}

public StyledProperty<T> Property { get; }
public bool HasValue => true;
AvaloniaProperty IValueEntry.Property => Property;

public void Unsubscribe() { }

public void Dispose() => _owner.OnEntryDisposed(this);

bool IValueEntry.HasValue() => true;
object? IValueEntry.GetValue() => _value;
T IValueEntry<T>.GetValue() => _value;

Expand Down
2 changes: 1 addition & 1 deletion src/Avalonia.Base/PropertyStore/ValueFrame.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ protected ValueFrame(BindingPriority priority, FrameType type)
}

public int EntryCount => _index.Count;
public bool IsActive => GetIsActive(out _);
public bool IsActive() => GetIsActive(out _);
public ValueStore? Owner => !_isShared ? _owner :
throw new AvaloniaInternalException("Cannot get owner for shared ValueFrame");
public BindingPriority Priority { get; }
Expand Down
6 changes: 3 additions & 3 deletions src/Avalonia.Base/PropertyStore/ValueStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ private void ReevaluateEffectiveValue(
// evaluated last as it can cause bindings to be subscribed.
if (foundEntry &&
HasHigherPriority(entry!, priority, current, changedValueEntry) &&
entry!.HasValue)
entry!.HasValue())
{
if (current is not null)
{
Expand Down Expand Up @@ -911,7 +911,7 @@ private void ReevaluateEffectiveValues(IValueEntry? changedValueEntry = null)
{
var frame = _frames[i];

if (!frame.IsActive)
if (!frame.IsActive())
continue;

var priority = frame.Priority;
Expand All @@ -927,7 +927,7 @@ private void ReevaluateEffectiveValues(IValueEntry? changedValueEntry = null)
if (!HasHigherPriority(entry, priority, effectiveValue, changedValueEntry))
continue;

if (!entry.HasValue)
if (!entry.HasValue())
continue;

if (effectiveValue is not null)
Expand Down
24 changes: 12 additions & 12 deletions src/Avalonia.Base/Styling/Activators/NthChildActivator.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Avalonia.LogicalTree;
using System.Collections.Generic;
using Avalonia.LogicalTree;

namespace Avalonia.Styling.Activators
{
Expand All @@ -12,7 +13,7 @@ internal sealed class NthChildActivator : StyleActivatorBase
private readonly int _step;
private readonly int _offset;
private readonly bool _reversed;
private int? _index;
private int _index = -1;

public NthChildActivator(
ILogical control,
Expand All @@ -28,7 +29,7 @@ public NthChildActivator(

protected override bool EvaluateIsActive()
{
var index = _index ?? _provider.GetChildIndex(_control);
var index = _index >= 0 ? _index : _provider.GetChildIndex(_control);
return NthChildSelector.Evaluate(index, _provider, _step, _offset, _reversed).IsMatch;
}

Expand All @@ -50,26 +51,25 @@ private void ChildIndexChanged(object? sender, ChildIndexChangedEventArgs e)
// 3. We're a reversed (nth-last-child) selector and total count has changed
switch (e.Action)
{
// We're using the _index field to pass the index of the child to EvaluateIsActive
// *only* when the active state is re-evaluated via this event handler. The docs
// for EvaluateIsActive say:
// The docs for EvaluateIsActive say:
//
// > This method should read directly from its inputs and not rely on any
// > subscriptions to fire in order to be up-to-date.
//
// Which is good advice in general, however in this case we need to break the rule
// and use the value from the event subscription instead of calling
// and use the value from the event subscription where possible instead of calling
// IChildIndexProvider.GetChildIndex. This is because this event can be fired during
// the process of realizing an element of a virtualized list; in this case calling
// GetChildIndex may not return the correct index as the element isn't yet realized.
// the process of realizing an element of a virtualized list; in this case there may
// be more than one `nth-child` style on a the list item and when the other is
// re-evaluated calling GetChildIndex may not return the correct index as the element
// isn't yet realized.
case ChildIndexChangedAction.ChildIndexChanged when e.Child == _control:
_index = e.Index;
_index = e.Index >= 0 ? e.Index : _provider.GetChildIndex(_control);
ReevaluateIsActive();
_index = null;
break;
case ChildIndexChangedAction.ChildIndexesReset:
case ChildIndexChangedAction.TotalCountChanged when _reversed:
_index = null;
_index = _provider.GetChildIndex(_control);
ReevaluateIsActive();
break;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ public PropertySetterTemplateInstance(AvaloniaProperty property, ITemplate templ
Property = property;
}

public bool HasValue => true;
public AvaloniaProperty Property { get; }

public bool HasValue() => true;
public object? GetValue() => _value ??= _template.Build();

bool IValueEntry.GetDataValidationState(out BindingValueType state, out Exception? error)
Expand Down
2 changes: 1 addition & 1 deletion src/Avalonia.Base/Styling/Setter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ public object? Value
}
}

bool IValueEntry.HasValue => true;
AvaloniaProperty IValueEntry.Property => EnsureProperty();

public override string ToString() => $"Setter: {Property} = {Value}";
Expand Down Expand Up @@ -88,6 +87,7 @@ internal override ISetterInstance Instance(IStyleInstance instance, StyledElemen
return this;
}

bool IValueEntry.HasValue() => true;
object? IValueEntry.GetValue() => Value;

bool IValueEntry.GetDataValidationState(out BindingValueType state, out Exception? error)
Expand Down
17 changes: 0 additions & 17 deletions tests/Avalonia.Base.UnitTests/Styling/TestSelectors.cs

This file was deleted.

67 changes: 64 additions & 3 deletions tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,64 @@ public void Does_Not_Realize_Items_If_Self_Outside_Viewport()
Assert.Equal(1, panel.VisualChildren.Count);
}

[Fact]
public void Alternating_Backgrounds_Should_Be_Correct_After_Scrolling()
{
// Issue #12381.
static void AssertColors(VirtualizingStackPanel target)
{
var containers = target.GetRealizedContainers()!
.Cast<ListBoxItem>()
.ToList();

for (var i = target.FirstRealizedIndex; i <= target.LastRealizedIndex; i++)
{
var container = Assert.IsType<ListBoxItem>(target.ContainerFromIndex(i));
var expectedBackground = i % 2 == 0 ? Colors.Green : Colors.Red;
var brush = Assert.IsAssignableFrom<ISolidColorBrush>(container.Background);

Assert.Equal(expectedBackground, brush.Color);
}
}

using var app = App();
var styles = new[]
{
new Style(x => x.OfType<ListBoxItem>())
{
Setters = { new Setter(ListBoxItem.BackgroundProperty, Brushes.White) },
},
new Style(x => x.OfType<ListBoxItem>().NthChild(2, 1))
{
Setters = { new Setter(ListBoxItem.BackgroundProperty, Brushes.Green) },
},
new Style(x => x.OfType<ListBoxItem>().NthChild(2, 0))
{
Setters = { new Setter(ListBoxItem.BackgroundProperty, Brushes.Red) },
},
};
var (target, scroll, itemsControl) = CreateUnrootedTarget<ListBox>();

// We need to display an odd number of items to reproduce the issue.
var root = CreateRoot(itemsControl, clientSize: new(100, 90), styles: styles);
root.LayoutManager.ExecuteInitialLayoutPass();

var containers = target.GetRealizedContainers()!
.Cast<ListBoxItem>()
.ToList();

Assert.Equal(0, target.FirstRealizedIndex);
Assert.Equal(8, target.LastRealizedIndex);
AssertColors(target);

scroll.Offset = new Vector(0, 10);
target.UpdateLayout();

Assert.Equal(1, target.FirstRealizedIndex);
Assert.Equal(9, target.LastRealizedIndex);
AssertColors(target);
}

private static IReadOnlyList<int> GetRealizedIndexes(VirtualizingStackPanel target, ItemsControl itemsControl)
{
return target.GetRealizedElements()
Expand Down Expand Up @@ -903,7 +961,7 @@ private static (VirtualizingStackPanel, ScrollViewer, T) CreateTarget<T>(
where T : ItemsControl, new()
{
var (target, scroll, itemsControl) = CreateUnrootedTarget<T>(items, itemTemplate);
var root = CreateRoot(itemsControl, styles);
var root = CreateRoot(itemsControl, styles: styles);

root.LayoutManager.ExecuteInitialLayoutPass();

Expand Down Expand Up @@ -942,10 +1000,13 @@ private static (VirtualizingStackPanel, ScrollViewer, T) CreateUnrootedTarget<T>
return (target, scroll, itemsControl);
}

private static TestRoot CreateRoot(Control? child, IEnumerable<Style>? styles = null)
private static TestRoot CreateRoot(
Control? child,
Size? clientSize = null,
IEnumerable<Style>? styles = null)
{
var root = new TestRoot(true, child);
root.ClientSize = new(100, 100);
root.ClientSize = clientSize ?? new(100, 100);

if (styles is not null)
root.Styles.AddRange(styles);
Expand Down