From 2d0b49a25874fa7d3ecd153b76b55f5519e4b518 Mon Sep 17 00:00:00 2001 From: Julien Lebosquain Date: Fri, 18 Apr 2025 11:26:23 +0200 Subject: [PATCH] Keep SelectingItemsControl selection values until ItemsSource is set (#18634) * Add failing tests for SelectedItem/SelectedIndex without an ItemsSource * Keep SelectedItem/SelectedIndex until ItemsSource is set * Add failing tests for setting SelectedValue without an ItemsSource * Keep SelectedValue until ItemsSource is set --- src/Avalonia.Controls/ItemCollection.cs | 9 +- src/Avalonia.Controls/ItemsSourceView.cs | 10 +++ .../Primitives/SelectingItemsControl.cs | 41 +++++++-- .../Primitives/SelectingItemsControlTests.cs | 89 ++++++++++++++++++- ...electingItemsControlTests_SelectedValue.cs | 47 ++++++++++ 5 files changed, 177 insertions(+), 19 deletions(-) diff --git a/src/Avalonia.Controls/ItemCollection.cs b/src/Avalonia.Controls/ItemCollection.cs index 03f46551c5..03fed042be 100644 --- a/src/Avalonia.Controls/ItemCollection.cs +++ b/src/Avalonia.Controls/ItemCollection.cs @@ -11,15 +11,10 @@ namespace Avalonia.Controls /// public class ItemCollection : ItemsSourceView, IList { -// Suppress "Avoid zero-length array allocations": This is a sentinel value and must be unique. -#pragma warning disable CA1825 - private static readonly object?[] s_uninitialized = new object?[0]; -#pragma warning restore CA1825 - private Mode _mode; internal ItemCollection() - : base(s_uninitialized) + : base(UninitializedSource) { } @@ -100,7 +95,7 @@ namespace Avalonia.Controls { if (IsReadOnly) ThrowIsItemsSource(); - if (Source == s_uninitialized) + if (Source == UninitializedSource) SetSource(CreateDefaultCollection()); return Source; } diff --git a/src/Avalonia.Controls/ItemsSourceView.cs b/src/Avalonia.Controls/ItemsSourceView.cs index 977d712371..22cddddd05 100644 --- a/src/Avalonia.Controls/ItemsSourceView.cs +++ b/src/Avalonia.Controls/ItemsSourceView.cs @@ -27,6 +27,13 @@ namespace Avalonia.Controls /// public static ItemsSourceView Empty { get; } = new ItemsSourceView(Array.Empty()); + /// + /// Gets an instance representing an uninitialized source. + /// + [SuppressMessage("Performance", "CA1825:Avoid zero-length array allocations", Justification = "This is a sentinel value and must be unique.")] + [SuppressMessage("ReSharper", "UseCollectionExpression", Justification = "This is a sentinel value and must be unique.")] + internal static object?[] UninitializedSource { get; } = new object?[0]; + private IList _source; private NotifyCollectionChangedEventHandler? _collectionChanged; private NotifyCollectionChangedEventHandler? _preCollectionChanged; @@ -49,6 +56,9 @@ namespace Avalonia.Controls /// public IList Source => _source; + internal IList? TryGetInitializedSource() + => _source == UninitializedSource ? null : _source; + /// /// Retrieves the item at the specified index. /// diff --git a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs index 0d98d41f08..59289838bb 100644 --- a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs +++ b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs @@ -592,10 +592,7 @@ namespace Avalonia.Controls.Primitives { base.OnInitialized(); - if (_selection is object) - { - _selection.Source = ItemsView.Source; - } + TryInitializeSelectionSource(_selection, _updateState is null); } /// @@ -896,8 +893,8 @@ namespace Avalonia.Controls.Primitives private void OnItemsViewSourceChanged(object? sender, EventArgs e) { - if (_selection is not null && _updateState is null) - _selection.Source = ItemsView.Source; + if (_updateState is null) + TryInitializeSelectionSource(_selection, true); } /// @@ -1202,7 +1199,7 @@ namespace Avalonia.Controls.Primitives { if (_updateState is null) { - model.Source = ItemsView.Source; + TryInitializeSelectionSource(model, false); } model.PropertyChanged += OnSelectionModelPropertyChanged; @@ -1237,6 +1234,32 @@ namespace Avalonia.Controls.Primitives } } + private void TryInitializeSelectionSource(ISelectionModel? selection, bool shouldSelectItemFromSelectedValue) + { + if (selection is not null && ItemsView.TryGetInitializedSource() is { } source) + { + // InternalSelectionModel keeps the SelectedIndex and SelectedItem values before the ItemsSource is set. + // However, SelectedValue isn't part of that model, so we have to set the SelectedItem from + // SelectedValue manually now that we have a source. + // + // While this works, this is messy: we effectively have "lazy selection initialization" in 3 places: + // - UpdateState (all selection properties, for BeginInit/EndInit) + // - InternalSelectionModel (SelectedIndex/SelectedItem) + // - SelectedItemsControl (SelectedValue) + // + // There's the opportunity to have a single place responsible for this logic. + // TODO12 (or 13): refactor this. + if (shouldSelectItemFromSelectedValue && selection.SelectedIndex == -1 && selection.SelectedItem is null) + { + var item = FindItemWithValue(SelectedValue); + if (item != AvaloniaProperty.UnsetValue) + selection.SelectedItem = item; + } + + selection.Source = source; + } + } + private void DeinitializeSelectionModel(ISelectionModel? model) { if (model is object) @@ -1266,7 +1289,7 @@ namespace Avalonia.Controls.Primitives if (_selection is InternalSelectionModel s) { - s.Update(ItemsView.Source, state.SelectedItems); + s.Update(ItemsView.TryGetInitializedSource(), state.SelectedItems); } else { @@ -1275,7 +1298,7 @@ namespace Avalonia.Controls.Primitives SelectedItems = state.SelectedItems.Value; } - Selection.Source = ItemsView.Source; + TryInitializeSelectionSource(Selection, false); } if (state.SelectedValue.HasValue) diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs index 5c1ece224a..13c6ee50a5 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs @@ -441,7 +441,7 @@ namespace Avalonia.Controls.UnitTests.Primitives } [Fact] - public void Setting_SelectedIndex_Out_Of_Bounds_Should_Clear_Selection() + public void Setting_SelectedIndex_Out_Of_Bounds_With_ItemsSource_Should_Clear_Selection() { var items = new[] { @@ -462,11 +462,50 @@ namespace Avalonia.Controls.UnitTests.Primitives } [Fact] - public void Setting_SelectedItem_To_Non_Existent_Item_Should_Clear_Selection() + public void Setting_SelectedIndex_Out_Of_Bounds_Without_ItemsSource_Should_Keep_Selection_Until_ItemsSource_Is_Set() { var target = new SelectingItemsControl { - Template = Template(), + Template = Template() + }; + + target.ApplyTemplate(); + target.SelectedIndex = 2; + + Assert.Equal(2, target.SelectedIndex); + + target.ItemsSource = Array.Empty(); + + Assert.Equal(-1, target.SelectedIndex); + } + + [Fact] + public void Setting_SelectedIndex_Without_ItemsSource_Should_Keep_Selection_If_Index_Exists_When_ItemsSource_IsSet() + { + var target = new SelectingItemsControl + { + Template = Template() + }; + + target.ApplyTemplate(); + target.SelectedIndex = 2; + + Assert.Equal(2, target.SelectedIndex); + + var items = new Item[] { new(), new(), new(), new() }; + target.ItemsSource = items; + + Assert.Equal(2, target.SelectedIndex); + Assert.Same(items[2], target.SelectedItem); + } + + [Fact] + public void Setting_SelectedItem_To_Non_Existent_Item_With_ItemsSource_Should_Clear_Selection() + { + var target = new SelectingItemsControl + { + ItemsSource = Array.Empty(), + Template = Template() }; target.ApplyTemplate(); @@ -476,6 +515,50 @@ namespace Avalonia.Controls.UnitTests.Primitives Assert.Null(target.SelectedItem); } + [Fact] + public void Setting_SelectedItem_To_Non_Existent_Item_Without_ItemsSource_Should_Keep_Selection_Until_ItemsSource_Is_Set() + { + var item = new Item(); + + var target = new SelectingItemsControl + { + Template = Template() + }; + + target.ApplyTemplate(); + target.SelectedItem = item; + + Assert.Equal(-1, target.SelectedIndex); + Assert.Same(item, target.SelectedItem); + + target.ItemsSource = Array.Empty(); + + Assert.Equal(-1, target.SelectedIndex); + Assert.Null(target.SelectedItem); + } + + [Fact] + public void Setting_SelectedItem_Without_ItemsSource_Should_Keep_Selection_If_Item_Exists_When_ItemsSource_IsSet() + { + var item = new Item(); + + var target = new SelectingItemsControl + { + Template = Template() + }; + + target.ApplyTemplate(); + target.SelectedItem = item; + + Assert.Equal(-1, target.SelectedIndex); + Assert.Same(item, target.SelectedItem); + + target.ItemsSource = new[] { new(), new(), item, new() }; + + Assert.Equal(2, target.SelectedIndex); + Assert.Same(item, target.SelectedItem); + } + [Fact] public void Adding_Selected_Item_Should_Update_Selection() { diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests_SelectedValue.cs b/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests_SelectedValue.cs index b0e430cbe0..a779e8d074 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests_SelectedValue.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests_SelectedValue.cs @@ -170,6 +170,53 @@ namespace Avalonia.Controls.UnitTests.Primitives Assert.Equal(items[2].Name, sic.SelectedValue); } + [Fact] + public void Setting_SelectedValue_To_Non_Existent_Item_Without_ItemsSource_Should_Keep_Selection_Until_ItemsSource_Is_Set() + { + var target = new SelectingItemsControl + { + Template = Template(), + SelectedValueBinding = new Binding("Name") + }; + + target.ApplyTemplate(); + target.SelectedValue = "Item2"; + + Assert.Equal(-1, target.SelectedIndex); + Assert.Null(target.SelectedItem); + Assert.Same("Item2", target.SelectedValue); + + target.ItemsSource = Array.Empty(); + + Assert.Equal(-1, target.SelectedIndex); + Assert.Null(target.SelectedItem); + Assert.Null(target.SelectedValue); + } + + [Fact] + public void Setting_SelectedValue_Without_ItemsSource_Should_Keep_Selection_If_Item_Exists_When_ItemsSource_IsSet() + { + var target = new SelectingItemsControl + { + Template = Template(), + SelectedValueBinding = new Binding("Name") + }; + + target.ApplyTemplate(); + target.SelectedValue = "Item2"; + + Assert.Equal(-1, target.SelectedIndex); + Assert.Null(target.SelectedItem); + Assert.Same("Item2", target.SelectedValue); + + var items = TestClass.GetItems(); + target.ItemsSource = items; + + Assert.Equal(2, target.SelectedIndex); + Assert.Same(items[2], target.SelectedItem); + Assert.Equal("Item2", target.SelectedValue); + } + [Fact] public void Setting_SelectedValue_During_Initialize_Should_Take_Priority_Over_Previous_Value() {