diff --git a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs index beec0bdcb16..e20e928f8d8 100644 --- a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs +++ b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs @@ -1,6 +1,5 @@ using System; using System.Collections; -using System.Collections.Generic; using System.Collections.Specialized; using System.ComponentModel; using System.Diagnostics.CodeAnalysis; @@ -8,7 +7,6 @@ using Avalonia.Controls.Selection; using Avalonia.Data; using Avalonia.Input; -using Avalonia.Input.Platform; using Avalonia.Interactivity; using Avalonia.Metadata; using Avalonia.Threading; @@ -187,14 +185,21 @@ public bool AutoScrollToSelectedItem /// public int SelectedIndex { - get => + get + { // When a Begin/EndInit/DataContext update is in place we return the value to be // updated here, even though it's not yet active and the property changed notification // has not yet been raised. If we don't do this then the old value will be written back // to the source when two-way bound, and the update value will be lost. - _updateState?.SelectedIndex.HasValue == true ? - _updateState.SelectedIndex.Value : - Selection.SelectedIndex; + if (_updateState is not null) + { + return _updateState.SelectedIndex.HasValue ? + _updateState.SelectedIndex.Value : + TryGetExistingSelection()?.SelectedIndex ?? -1; + } + + return Selection.SelectedIndex; + } set { if (_updateState is object) @@ -213,11 +218,18 @@ public int SelectedIndex /// public object? SelectedItem { - get => - // See SelectedIndex setter for more information. - _updateState?.SelectedItem.HasValue == true ? - _updateState.SelectedItem.Value : - Selection.SelectedItem; + get + { + // See SelectedIndex getter for more information. + if (_updateState is not null) + { + return _updateState.SelectedItem.HasValue ? + _updateState.SelectedItem.Value : + TryGetExistingSelection()?.SelectedItem; + } + + return Selection.SelectedItem; + } set { if (_updateState is object) @@ -270,6 +282,7 @@ protected IList? SelectedItems { return _updateState.SelectedItems.Value; } + else if (Selection is InternalSelectionModel ism) { var result = ism.WritableSelectedItems; @@ -456,10 +469,8 @@ private protected override void OnItemsViewCollectionChanged(object? sender, Not protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e) { base.OnAttachedToVisualTree(e); - if (Selection?.AnchorIndex is int index) - { - AutoScrollToSelectedItemIfNecessary(index); - } + + AutoScrollToSelectedItemIfNecessary(GetAnchorIndex()); } /// @@ -470,10 +481,8 @@ protected override void OnApplyTemplate(TemplateAppliedEventArgs e) void ExecuteScrollWhenLayoutUpdated(object? sender, EventArgs e) { LayoutUpdated -= ExecuteScrollWhenLayoutUpdated; - if (Selection?.AnchorIndex is int index) - { - AutoScrollToSelectedItemIfNecessary(index); - } + + AutoScrollToSelectedItemIfNecessary(GetAnchorIndex()); } if (AutoScrollToSelectedItem) @@ -482,6 +491,15 @@ void ExecuteScrollWhenLayoutUpdated(object? sender, EventArgs e) } } + internal int GetAnchorIndex() + { + var selection = _updateState is not null ? TryGetExistingSelection() : Selection; + return selection?.AnchorIndex ?? -1; + } + + private ISelectionModel? TryGetExistingSelection() + => _updateState?.Selection.HasValue == true ? _updateState.Selection.Value : _selection; + protected internal override void PrepareContainerForItemOverride(Control container, object? item, int index) { // Ensure that the selection model is created at this point so that accessing it in @@ -634,10 +652,7 @@ protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs chang if (change.Property == AutoScrollToSelectedItemProperty) { - if (Selection?.AnchorIndex is int index) - { - AutoScrollToSelectedItemIfNecessary(index); - } + AutoScrollToSelectedItemIfNecessary(GetAnchorIndex()); } else if (change.Property == SelectionModeProperty && _selection is object) { @@ -671,7 +686,7 @@ protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs chang return; } - var value = change.GetNewValue(); + var value = change.GetNewValue(); if (value is null) { // Clearing SelectedValueBinding makes the SelectedValue the item itself @@ -921,11 +936,10 @@ private void OnSelectionModelPropertyChanged(object? sender, PropertyChangedEven if (e.PropertyName == nameof(ISelectionModel.AnchorIndex)) { _hasScrolledToSelectedItem = false; - if (Selection?.AnchorIndex is int index) - { - KeyboardNavigation.SetTabOnceActiveElement(this, ContainerFromIndex(index)); - AutoScrollToSelectedItemIfNecessary(index); - } + + var anchorIndex = GetAnchorIndex(); + KeyboardNavigation.SetTabOnceActiveElement(this, ContainerFromIndex(anchorIndex)); + AutoScrollToSelectedItemIfNecessary(anchorIndex); } else if (e.PropertyName == nameof(ISelectionModel.SelectedIndex) && _oldSelectedIndex != SelectedIndex) { @@ -1279,9 +1293,17 @@ private void EndUpdating() state.SelectedItem = item; } + // SelectedIndex vs SelectedItem: + // - If only one has a value, use it + // - If both have a value, prefer the one having a "non-empty" value, e.g. not -1 nor null + // - If both have a "non-empty" value, prefer the index if (state.SelectedIndex.HasValue) { - SelectedIndex = state.SelectedIndex.Value; + var selectedIndex = state.SelectedIndex.Value; + if (selectedIndex >= 0 || !state.SelectedItem.HasValue) + SelectedIndex = selectedIndex; + else + SelectedItem = state.SelectedItem.Value; } else if (state.SelectedItem.HasValue) { @@ -1338,39 +1360,12 @@ private void TextSearchTimer_Tick(object? sender, EventArgs e) // - Both the old and new SelectionModels have the incorrect Source private class UpdateState { - private Optional _selectedIndex; - private Optional _selectedItem; - private Optional _selectedValue; - public int UpdateCount { get; set; } public Optional Selection { get; set; } public Optional SelectedItems { get; set; } - - public Optional SelectedIndex - { - get => _selectedIndex; - set - { - _selectedIndex = value; - _selectedItem = default; - } - } - - public Optional SelectedItem - { - get => _selectedItem; - set - { - _selectedItem = value; - _selectedIndex = default; - } - } - - public Optional SelectedValue - { - get => _selectedValue; - set => _selectedValue = value; - } + public Optional SelectedIndex { get; set; } + public Optional SelectedItem { get; set; } + public Optional SelectedValue { get; set; } } /// diff --git a/tests/Avalonia.Controls.UnitTests/EnumerableExtensions.cs b/tests/Avalonia.Controls.UnitTests/EnumerableExtensions.cs index a2d752ab396..25bf3d3c4ed 100644 --- a/tests/Avalonia.Controls.UnitTests/EnumerableExtensions.cs +++ b/tests/Avalonia.Controls.UnitTests/EnumerableExtensions.cs @@ -1,8 +1,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Text; -using System.Threading.Tasks; namespace Avalonia.Controls.UnitTests { @@ -16,5 +14,29 @@ public static IEnumerable Do(this IEnumerable items, Action action) yield return i; } } + + public static IEnumerable Permutations(this IEnumerable source) + { + var sourceArray = source.ToArray(); + var results = new List(); + Permute(sourceArray, 0, sourceArray.Length - 1); + return results; + + void Permute(T[] elements, int depth, int maxDepth) + { + if (depth == maxDepth) + { + results.Add(elements.ToArray()); + return; + } + + for (var i = depth; i <= maxDepth; i++) + { + (elements[depth], elements[i]) = (elements[i], elements[depth]); + Permute(elements, depth + 1, maxDepth); + (elements[depth], elements[i]) = (elements[i], elements[depth]); + } + } + } } } diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs index aa2b7ca5310..8c8bf2b33fb 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs @@ -5,7 +5,6 @@ using System.Collections.Specialized; using System.ComponentModel; using System.Linq; -using System.Reactive.Disposables; using System.Threading.Tasks; using Avalonia.Collections; using Avalonia.Controls.Presenters; @@ -18,7 +17,6 @@ using Avalonia.Interactivity; using Avalonia.Layout; using Avalonia.Markup.Data; -using Avalonia.Platform; using Avalonia.Styling; using Avalonia.Threading; using Avalonia.UnitTests; @@ -2294,6 +2292,134 @@ public void Should_First_Raise_Property_Changed_Notification_Then_Fire_Selection } + [Theory] + [MemberData(nameof(GetSelectionFieldPermutationParameters))] + public void SelectedItem_And_Selection_Properties_Work_In_Any_Order_When_Initializing(SelectionField[] fields) + => TestSelectionFields(vm => vm.SelectedItem = vm.Items[2], fields); + + [Theory] + [MemberData(nameof(GetSelectionFieldPermutationParameters))] + public void SelectedIndex_And_Selection_Properties_Work_In_Any_Order_When_Initializing(SelectionField[] fields) + => TestSelectionFields(vm => vm.SelectedIndex = 2, fields); + + [Theory] + [MemberData(nameof(GetSelectionFieldPermutationParameters))] + public void SelectedValue_And_Selection_Properties_Work_In_Any_Order_When_Initializing(SelectionField[] fields) + => TestSelectionFields(vm => vm.SelectedValue = 12, fields); + + private void TestSelectionFields(Action setItem2, SelectionField[] fields) + { + using var _ = Start(); + + var vm = new FullSelectionViewModel + { + Items = + { + new ItemModel { Id = 10, Name = "Item0" }, + new ItemModel { Id = 11, Name = "Item1" }, + new ItemModel { Id = 12, Name = "Item2" }, + new ItemModel { Id = 13, Name = "Item3" } + } + }; + + setItem2(vm); + + var root = new TestRoot + { + Width = 100, + Height = 100 + }; + + // Match the Begin/EndInit sequence emitted by the XAML compiler + root.BeginInit(); + var target = new ListBox(); + target.BeginInit(); + root.Child = target; + target.DataContext = vm; + + foreach (var field in fields) + { + switch (field) + { + case SelectionField.ItemsSource: + target.Bind(ItemsControl.ItemsSourceProperty, new Binding(nameof(FullSelectionViewModel.Items))); + break; + case SelectionField.SelectedItem: + target.Bind(SelectingItemsControl.SelectedItemProperty, new Binding(nameof(FullSelectionViewModel.SelectedItem))); + break; + case SelectionField.SelectedIndex: + target.Bind(SelectingItemsControl.SelectedIndexProperty, new Binding(nameof(FullSelectionViewModel.SelectedIndex))); + break; + case SelectionField.SelectedValue: + target.Bind(SelectingItemsControl.SelectedValueProperty, new Binding(nameof(FullSelectionViewModel.SelectedValue))); + break; + case SelectionField.SelectedValueBinding: + target.SelectedValueBinding = new Binding(nameof(ItemModel.Id)); + break; + default: + throw new InvalidOperationException($"Unkown field {field}"); + } + } + + target.EndInit(); + root.EndInit(); + + Assert.Equal(vm.Items[2], target.SelectedItem); + Assert.Equal(2, target.SelectedIndex); + Assert.Equal(12, target.SelectedValue); + + Assert.Equal(vm.Items[2], vm.SelectedItem); + Assert.Equal(2, vm.SelectedIndex); + Assert.Equal(12, vm.SelectedValue); + } + + [Fact] + public void SelectedItem_Can_Access_Selection_DuringInit() + { + using var _ = Start(); + + var target = new ListBox(); + target.BeginInit(); + + var item = new ItemModel(); + + target.Selection = new SelectionModel { + SelectedItem = item + }; + + Assert.Equal(item, target.SelectedItem); + } + + [Fact] + public void SelectedIndex_Can_Access_Selection_DuringInit() + { + using var _ = Start(); + + var target = new ListBox(); + target.BeginInit(); + + target.Selection = new SelectionModel { + SelectedIndex = 42 + }; + + Assert.Equal(42, target.SelectedIndex); + } + + [Fact] + public void AnchorIndex_Can_Access_Selection_DuringInit() + { + using var _ = Start(); + + var target = new ListBox(); + target.BeginInit(); + + target.Selection = new SelectionModel { + AnchorIndex = 42 + }; + + Assert.Equal(42, target.GetAnchorIndex()); + } + private static IDisposable Start() { return UnitTestApplication.Start(TestServices.StyledWindow); @@ -2336,6 +2462,9 @@ private static FuncControlTemplate Template() }.RegisterInNameScope(scope)); } + public static IEnumerable GetSelectionFieldPermutationParameters() + => Enum.GetValues().Permutations().Select(fields => new object[] { fields }); + private class Item : Control, ISelectable { public string Value { get; set; } @@ -2467,5 +2596,61 @@ public void Reset(IEnumerable items) public event NotifyCollectionChangedEventHandler CollectionChanged; } + +#nullable enable + + private sealed class FullSelectionViewModel : NotifyingBase + { + private ItemModel? _selectedItem; + private int _selectedIndex = -1; + private int? _selectedValue; + + public ObservableCollection Items { get; } = new(); + + public ItemModel? SelectedItem + { + get => _selectedItem; + set => SetField(ref _selectedItem, value); + } + + public int SelectedIndex + { + get => _selectedIndex; + set => SetField(ref _selectedIndex, value); + } + + public int? SelectedValue + { + get => _selectedValue; + set => SetField(ref _selectedValue, value); + } + } + + private sealed class ItemModel : NotifyingBase + { + private int _id; + private string? _name; + + public int Id + { + get => _id; + set => SetField(ref _id, value); + } + + public string? Name + { + get => _name; + set => SetField(ref _name, value); + } + } + + public enum SelectionField + { + ItemsSource, + SelectedItem, + SelectedIndex, + SelectedValue, + SelectedValueBinding + } } } diff --git a/tests/Avalonia.UnitTests/NotifyingBase.cs b/tests/Avalonia.UnitTests/NotifyingBase.cs index 9542e023d5c..3989118b79d 100644 --- a/tests/Avalonia.UnitTests/NotifyingBase.cs +++ b/tests/Avalonia.UnitTests/NotifyingBase.cs @@ -1,3 +1,6 @@ +#nullable enable + +using System.Collections.Generic; using System.ComponentModel; using System.Linq; using System.Runtime.CompilerServices; @@ -6,9 +9,9 @@ namespace Avalonia.UnitTests { public class NotifyingBase : INotifyPropertyChanged { - private PropertyChangedEventHandler _propertyChanged; + private PropertyChangedEventHandler? _propertyChanged; - public event PropertyChangedEventHandler PropertyChanged + public event PropertyChangedEventHandler? PropertyChanged { add { @@ -32,9 +35,19 @@ public int PropertyChangedSubscriptionCount private set; } - public void RaisePropertyChanged([CallerMemberName] string propertyName = null) + public void RaisePropertyChanged([CallerMemberName] string? propertyName = null) { _propertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName)); } + + protected bool SetField(ref T field, T value, [CallerMemberName] string? propertyName = null) + { + if (EqualityComparer.Default.Equals(field, value)) + return false; + + field = value; + RaisePropertyChanged(propertyName); + return true; + } } }