Fix SelectionChanged not raised on collection Reset - #20942
Conversation
|
You can test this PR using the following package version. |
|
@NathanDrake2406 Is there something missing that you converted this PR to a draft? |
|
You can test this PR using the following package version. |
ecab00d to
f2e8388
Compare
…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 AvaloniaUI#20897
68eb1d7 to
7db50f8
Compare
|
You can test this PR using the following package version. |
|
I had a deeper look at this. From my understanding, the current behavior is expected from the However, the An alternative would be to add a flag or operation type to cc @grokys for additional thoughts. |
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.
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.
|
You can test this PR using the following package version. |
|
I agree with @MrJul that we shouldn't be trying to diff or restore selection after a reset - we explicitly don't try to do that in the selection model because it involves copying an arbitrarily large collection. We should be raising |
| UpdateSelectedValueFromItem(); | ||
| } | ||
|
|
||
| _selectedItemsSnapshot = Selection.SelectedItems.ToArray(); |
There was a problem hiding this comment.
Memory: keep a single List for the snapshot instead of creating a new array each time the selection changes.
| private object?[] _selectedItemsSnapshot = Array.Empty<object>(); | ||
| private object?[]? _selectedItemsBeforeReset; |
| _selectedItemsSnapshot = Selection.SelectedItems.ToArray(); | ||
| _selectedItemsBeforeReset = null; |
| if (_selectedItemsBeforeReset?.Length > 0) | ||
| { | ||
| RaiseEvent(new SelectionChangedEventArgs( | ||
| SelectionChangedEvent, | ||
| _selectedItemsBeforeReset, | ||
| Array.Empty<object>())); | ||
| _selectedItemsBeforeReset = null; | ||
| } |
- 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.
| var ev = new SelectionChangedEventArgs( | ||
| SelectionChangedEvent, | ||
| _selectedItemsBeforeReset, | ||
| Array.Empty<object?>()); |
| private void OnItemsViewPreCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) | ||
| { | ||
| if (e.Action == NotifyCollectionChangedAction.Reset && _selectedItemsSnapshot.Count > 0) | ||
| { | ||
| _selectedItemsBeforeReset = _selectedItemsSnapshot.ToArray(); | ||
| } | ||
| } |
…apshot - 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.
| _selectedItemsSnapshot.Clear(); | ||
| _selectedItemsSnapshot.AddRange(Selection.SelectedItems); | ||
| _selectedItemsBeforeReset = null; | ||
|
|
||
| if (route.HasHandlers) | ||
| { | ||
| var ev = new SelectionChangedEventArgs( | ||
| SelectionChangedEvent, | ||
| e.DeselectedItems.ToArray(), | ||
| e.SelectedItems.ToArray()); | ||
| RaiseEvent(ev); | ||
| } | ||
| RaiseSelectionChanged(e.DeselectedItems.ToArray(), e.SelectedItems.ToArray()); |
| private readonly List<object?> _selectedItemsSnapshot = new(); | ||
| private object?[]? _selectedItemsBeforeReset; |
…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.
|
You can test this PR using the following package version. |
* 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 AvaloniaUI#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.
What does the pull request do?
Fixes
SelectingItemsControl.SelectionChangednot being raised when an items collection sends aResetnotification that clears the current selection. This covers controls such asListBoxbound to anObservableCollectionthat is cleared.What is the current behavior?
When a selected item is removed by a
Reset,SelectionModelreports the loss throughLostSelection.SelectingItemsControlonly uses that callback to recoverAlwaysSelectedselection, so no public routedSelectionChangedevent is raised for reset-to-empty cases.What is the updated/expected behavior with this PR?
SelectionChangedis raised when a reset causes the control to lose its selected items. The event reports the previously selected items inRemovedItemsand does not report removals when a reset preserves the selection.Validated with:
dotnet run --project tests/Avalonia.Controls.UnitTests/Avalonia.Controls.UnitTests.csproj -- --filter-class "Avalonia.Controls.UnitTests.Primitives.SelectingItemsControlTests"dotnet run --project tests/Avalonia.Controls.UnitTests/Avalonia.Controls.UnitTests.csproj -- --filter-class "Avalonia.Controls.UnitTests.Selection.InternalSelectionModelTests"dotnet run --project tests/Avalonia.Controls.UnitTests/Avalonia.Controls.UnitTests.csproj -- --filter-namespace "Avalonia.Controls.UnitTests.Selection"How was the solution implemented (if it's not obvious)?
SelectingItemsControlnow keeps a snapshot of the last selected items at the control boundary. When an itemsResetstarts, the control captures that existing snapshot. IfSelectionModel.LostSelectionis committed for that reset, the control raises the routedSelectionChangedevent using the captured items asRemovedItems.This keeps
SelectionModelreset semantics intact and avoids diffing the reset collection contents.Checklist
Breaking changes
None.
Obsoletions / Deprecations
None.
Fixed issues
Fixes #20897