Browse Source

Fix SelectionChanged not raised on collection Reset (#20942)

* fix: raise SelectionChanged event on collection Reset when items are deselected

When a collection bound to a SelectingItemsControl (e.g. ListBox) was
cleared via NotifyCollectionChangedAction.Reset, the SelectionChanged
event was not raised despite the selection being lost.

The root cause was in InternalSelectionModel.OnSourceReset: the base
SelectionModel.OnSourceReset() directly reset _selectedIndex to -1
before any Operation could capture the old selection state. The
subsequent SyncFromSelectedItems created an Operation that saw no
change (old and new both -1), so CommitOperation never fired
SelectionChanged.

The fix snapshots _writableSelectedItems before sync, diffs against
the post-sync state to find items that were actually lost (not merely
re-selected at a new index after reorder), and injects them as
DeselectedItems on the pending Operation — following the same pattern
used by OnSelectionRemoved for individual item removals.

Fixes #20897

* fix: use multiset diff to report lost duplicate selections on Reset

The Reset diff in InternalSelectionModel used a HashSet to detect
which previously-selected items were still present after sync. Selection
allows duplicates (same instance or equal items at multiple indices),
so set semantics collapsed duplicates into one entry and under-reported
deselections when only some occurrences were lost.

Track counts per item plus a null counter and decrement per match, so
RemovedItems reflects the actual number of lost selections.

Adds a duplicate-items Reset test covering the regression.

* fix: raise SelectionChanged for reset-lost selection

ListBox and other SelectingItemsControl callers did not receive SelectionChanged when a Reset cleared the selected items. The selection model reports this path through LostSelection, but the control only used that callback for AlwaysSelected recovery.

Track the last selected items at the control boundary, capture that snapshot for Reset notifications, and raise the routed SelectionChanged event when LostSelection commits during that reset. This avoids diffing reset contents while preserving the removed-items payload for clear/reset-to-empty cases.

* fix: address review feedback on SelectionChanged Reset snapshot

- Replace per-change ToArray() snapshot with persistent List<object?> to
  avoid allocations on every selection change (review: MrJul).
- Read snapshot in PreCollectionChanged instead of Selection.SelectedItems
  because the source is already empty by the time Reset fires.
- Align LostSelection event-raising with SelectionChanged path: use
  BuildEventRoute + HasHandlers guard to avoid allocating args when
  no handlers are attached (review: copilot).
- Harden existing Reset tests to Assert.Single to catch double-fire.

* fix: consolidate SelectionChanged raising and defend against stale snapshot

- Extract RaiseSelectionChanged helper so both the normal
  (SelectionChanged) and reset (LostSelection) paths share the same
  BuildEventRoute/HasHandlers guard and SelectionChangedEventArgs
  construction (review: copilot).
- Move _selectedItemsBeforeReset clear outside the conditional in
  OnSelectionModelLostSelection so the field is always nulled after
  LostSelection, preventing accidental reuse (review: copilot).
- Add comment documenting the snapshot lifecycle in
  OnItemsViewPreCollectionChanged.

* perf: defer SelectionChangedEventArgs allocations until handlers are confirmed

SelectionChangedEventArgs materialized arrays via ToArray() at the call site
before checking whether the routed event had any registered handlers. This
allocated needlessly in the common case of no external subscribers.

The event items (IReadOnlyList<object?>) already implement IList via
ReadOnlySelectionListBase. RaiseSelectionChanged now accepts
IReadOnlyList<object?> and casts to IList inside the HasHandlers gate,
falling back to ToArray() only for non-IList enumerables.
pull/21441/head
Nathan Nguyen 4 months ago
committed by GitHub
parent
commit
b0877fe88f
No known key found for this signature in database GPG Key ID: B5690EEEBB952194
  1. 55
      src/Avalonia.Controls/Primitives/SelectingItemsControl.cs
  2. 95
      tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs

55
src/Avalonia.Controls/Primitives/SelectingItemsControl.cs

@ -1,5 +1,6 @@
using System; using System;
using System.Collections; using System.Collections;
using System.Collections.Generic;
using System.Collections.Specialized; using System.Collections.Specialized;
using System.ComponentModel; using System.ComponentModel;
using System.Diagnostics.CodeAnalysis; using System.Diagnostics.CodeAnalysis;
@ -145,6 +146,8 @@ namespace Avalonia.Controls.Primitives
private int _oldSelectedIndex; private int _oldSelectedIndex;
private WeakReference _oldSelectedItem = new(null); private WeakReference _oldSelectedItem = new(null);
private WeakReference<IList?> _oldSelectedItems = new(null); private WeakReference<IList?> _oldSelectedItems = new(null);
private readonly List<object?> _selectedItemsSnapshot = new();
private object?[]? _selectedItemsBeforeReset;
private bool _ignoreContainerSelectionChanged; private bool _ignoreContainerSelectionChanged;
private UpdateState? _updateState; private UpdateState? _updateState;
private bool _hasScrolledToSelectedItem; private bool _hasScrolledToSelectedItem;
@ -153,7 +156,9 @@ namespace Avalonia.Controls.Primitives
public SelectingItemsControl() public SelectingItemsControl()
{ {
((ItemCollection)ItemsView).SourceChanged += OnItemsViewSourceChanged; var items = (ItemCollection)ItemsView;
items.SourceChanged += OnItemsViewSourceChanged;
items.PreCollectionChanged += OnItemsViewPreCollectionChanged;
} }
/// <summary> /// <summary>
@ -465,6 +470,14 @@ namespace Avalonia.Controls.Primitives
} }
} }
private void OnItemsViewPreCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e)
{
if (e.Action == NotifyCollectionChangedAction.Reset && _selectedItemsSnapshot.Count > 0)
{
_selectedItemsBeforeReset = _selectedItemsSnapshot.ToArray();
}
}
/// <inheritdoc /> /// <inheritdoc />
protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e) protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e)
{ {
@ -1013,16 +1026,11 @@ namespace Avalonia.Controls.Primitives
UpdateSelectedValueFromItem(); UpdateSelectedValueFromItem();
} }
var route = BuildEventRoute(SelectionChangedEvent); _selectedItemsSnapshot.Clear();
_selectedItemsSnapshot.AddRange(Selection.SelectedItems);
_selectedItemsBeforeReset = null;
if (route.HasHandlers) RaiseSelectionChanged(e.DeselectedItems, e.SelectedItems);
{
var ev = new SelectionChangedEventArgs(
SelectionChangedEvent,
e.DeselectedItems.ToArray(),
e.SelectedItems.ToArray());
RaiseEvent(ev);
}
} }
/// <summary> /// <summary>
@ -1033,12 +1041,37 @@ namespace Avalonia.Controls.Primitives
/// <param name="e">The event args.</param> /// <param name="e">The event args.</param>
private void OnSelectionModelLostSelection(object? sender, EventArgs e) private void OnSelectionModelLostSelection(object? sender, EventArgs e)
{ {
if (_selectedItemsBeforeReset?.Length > 0)
{
RaiseSelectionChanged(_selectedItemsBeforeReset, Array.Empty<object?>());
}
_selectedItemsBeforeReset = null;
if (AlwaysSelected && ItemsView.Count > 0) if (AlwaysSelected && ItemsView.Count > 0)
{ {
SelectedIndex = 0; SelectedIndex = 0;
} }
} }
/// <summary>
/// Raises the <see cref="SelectionChangedEvent"/> if there are registered handlers.
/// </summary>
/// <param name="removedItems">The items removed from the selection.</param>
/// <param name="addedItems">The items added to the selection.</param>
private void RaiseSelectionChanged(IReadOnlyList<object?> removedItems, IReadOnlyList<object?> addedItems)
{
var route = BuildEventRoute(SelectionChangedEvent);
if (route.HasHandlers)
{
RaiseEvent(new SelectionChangedEventArgs(
SelectionChangedEvent,
removedItems as IList ?? removedItems.ToArray(),
addedItems as IList ?? addedItems.ToArray()));
}
}
private void SelectItemWithValue(object? value) private void SelectItemWithValue(object? value)
{ {
if (ItemCount == 0 || _isSelectionChangeActive) if (ItemCount == 0 || _isSelectionChangeActive)
@ -1248,6 +1281,8 @@ namespace Avalonia.Controls.Primitives
_oldSelectedIndex = model.SelectedIndex; _oldSelectedIndex = model.SelectedIndex;
_oldSelectedItem.Target = model.SelectedItem; _oldSelectedItem.Target = model.SelectedItem;
_selectedItemsSnapshot.Clear();
_selectedItemsSnapshot.AddRange(model.SelectedItems);
if (_updateState is null && AlwaysSelected && model.Count == 0) if (_updateState is null && AlwaysSelected && model.Count == 0)
{ {

95
tests/Avalonia.Controls.UnitTests/Primitives/SelectingItemsControlTests.cs

@ -856,6 +856,101 @@ namespace Avalonia.Controls.UnitTests.Primitives
Assert.Equal(-1, target.SelectedIndex); Assert.Equal(-1, target.SelectedIndex);
} }
[Fact]
public void Resetting_Items_Collection_Should_Raise_SelectionChanged()
{
var items = new ObservableCollection<Item>
{
new Item(),
new Item(),
new Item(),
};
var target = new SelectingItemsControl
{
ItemsSource = items,
Template = Template(),
};
Prepare(target);
target.SelectedIndex = 1;
var selectedItem = items[1];
var receivedArgs = new List<SelectionChangedEventArgs>();
target.SelectionChanged += (_, args) => receivedArgs.Add(args);
items.Clear();
Assert.Null(target.SelectedItem);
Assert.Equal(-1, target.SelectedIndex);
Assert.Single(receivedArgs);
Assert.Empty(receivedArgs[0].AddedItems);
Assert.Equal(new[] { selectedItem }, receivedArgs[0].RemovedItems);
}
[Fact]
public void Resetting_Items_To_Empty_With_Multiple_Selection_Should_Raise_SelectionChanged()
{
var items = new ObservableCollection<Item>
{
new Item(),
new Item(),
new Item(),
};
var target = new TestSelector
{
ItemsSource = items,
Template = Template(),
SelectionMode = SelectionMode.Multiple,
};
Prepare(target);
target.SelectedIndex = 0;
target.Selection.Select(2);
var selected0 = items[0];
var selected2 = items[2];
var receivedArgs = new List<SelectionChangedEventArgs>();
target.SelectionChanged += (_, args) => receivedArgs.Add(args);
items.Clear();
Assert.Null(target.SelectedItem);
Assert.Equal(-1, target.SelectedIndex);
Assert.Single(receivedArgs);
Assert.Empty(receivedArgs[0].AddedItems);
Assert.Equal(2, receivedArgs[0].RemovedItems.Count);
Assert.Contains(selected0, receivedArgs[0].RemovedItems.Cast<object>());
Assert.Contains(selected2, receivedArgs[0].RemovedItems.Cast<object>());
}
[Fact]
public void Resetting_Items_With_Preserved_Selection_Should_Not_Report_Deselection()
{
var items = new ResettingCollection(3);
var target = new SelectingItemsControl
{
ItemsSource = items,
Template = Template(),
};
target.ApplyTemplate();
target.SelectedIndex = 1;
var receivedArgs = new List<SelectionChangedEventArgs>();
target.SelectionChanged += (_, args) => receivedArgs.Add(args);
items.Reset(new[] { "Item2", "Item0", "Item1" });
Assert.Equal("Item1", target.SelectedItem);
Assert.Single(receivedArgs);
Assert.Empty(receivedArgs[0].RemovedItems);
}
[Fact] [Fact]
public void Raising_IsSelectedChanged_On_Item_Should_Update_Selection() public void Raising_IsSelectedChanged_On_Item_Should_Update_Selection()
{ {

Loading…
Cancel
Save