Skip to content
Open
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ public partial class CollectionViewHandler2 : ReorderableItemsViewHandler2<Reord
{
bool _ignorePlatformSelectionChange;
bool _selectionDirty;
// Prevents MapSelectedItem from calling UpdatePlatformSelection when
// UpdateVirtualSingleSelection is writing SelectedItem in response to a
// platform tap — otherwise the null-item WinUI selection gets undone.
bool _ignoreVirtualSelectionChange;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Cross-Platform Behavioral Consistency - The new flag is only honoured for SelectionMode.Single. UpdateVirtualMultipleSelection does not set it, MapSelectedItems has no equivalent guard, and ExtractPlatformSelectedItems() explicitly drops nulls (if (selectedItem is not null) result.Add(...), line 427).

So in SelectionMode.Multiple a null row can now be checked in the WinUI UI (it gets a real, selectable ItemContainer from the new ItemFactory path) but it never lands in SelectedItems, and the first subsequent MapSelectedItems -> UpdatePlatformSelection -> DeselectAll() visually undoes the check. That is exactly the round-trip this PR fixes for Single mode, left in place for Multiple.

Either extend the fix to multi-select or explicitly document/assert that null items are not selectable in Multiple mode, and add a device test pinning the chosen behaviour.


// Cache for MeasureFirstItem optimization
global::Windows.Foundation.Size _firstItemMeasuredSize = global::Windows.Foundation.Size.Empty;
Expand Down Expand Up @@ -75,6 +79,13 @@ public static void MapItemsSource(CollectionViewHandler2 handler, SelectableItem

public static void MapSelectedItem(CollectionViewHandler2 handler, SelectableItemsView itemsView)
{
// When UpdateVirtualSingleSelection sets SelectedItem in response to a platform
// tap, skip the round-trip back to UpdatePlatformSelection. Without this guard,
// tapping a null item causes: WinUI selects null → SelectedItem = null →
// MapSelectedItem → UpdatePlatformSelection → DeselectAll (undoes the selection).
if (handler._ignoreVirtualSelectionChange)
Comment thread
SuthiYuvaraj marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness / Regression Prevention - This guard is too broad and breaks the most common MAUI selection idiom on Windows CV2.

Element.OnBindablePropertySet deliberately defers UpdateHandlerValue until after property.PropertyChanged fires (see Element.cs:694-703 and the comment at Element.cs:711-719). SelectableItemsView raises the public SelectionChanged event from that PropertyChanged callback. So everything the app does inside its SelectionChanged handler executes inside the _ignoreVirtualSelectionChange window opened in UpdateVirtualSingleSelection, and MapSelectedItem for both the outer and any nested write is suppressed.

Concrete failing scenario - the canonical tap-then-clear pattern:

cv.SelectionChanged += (s, e) => { Navigate(e.CurrentSelection[0]); cv.SelectedItem = null; };
  1. User taps Item 1 -> PlatformSelectionChanged -> UpdateVirtualSingleSelection sets _ignoreVirtualSelectionChange = true.
  2. ItemsView.SelectedItem = Item 1 -> SelectionChanged -> app sets SelectedItem = null -> nested MapSelectedItem suppressed.
  3. Outer MapSelectedItem suppressed too (still inside the try).
  4. UpdatePlatformSelection() never runs, so PlatformView.DeselectAll() never runs. The WinUI ItemContainer stays visually selected and PlatformView.SelectedItem stays non-null.
  5. Tapping the same row again produces no WinUI SelectionChanged, so the app handler never fires for that item again.

Before this PR both writes reached UpdatePlatformSelection and the platform was correctly deselected.

Suggested fix: make the guard value-scoped rather than time-scoped - record the value being written (e.g. _pendingVirtualSelection) and in MapSelectedItem only skip when ReferenceEquals(itemsView.SelectedItem, pending); otherwise fall through to UpdatePlatformSelection() so re-entrant app writes still propagate. Also save/restore the previous flag value in the finally on line 348 rather than hard-assigning false, so a nested UpdateVirtualSingleSelection cannot clear the outer frame's guard early.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Handler Mapper and Property Patterns — This guard is broader than the round-trip it is meant to break, and it swallows re-entrant SelectedItem writes made by application code.

SelectableItemsView.SelectedItemPropertyChanged runs the BindableProperty changed callback synchronously inside SetValue, and that callback executes SelectionChangedCommand and raises the public SelectionChanged event (src/Controls/src/Core/Items/SelectableItemsView.cs, SelectionPropertyChanged). So any user handler runs while _ignoreVirtualSelectionChange == true.

Concrete scenario (very common MAUI pattern — clearing selection so the same row can be tapped twice):

  1. SelectedItem is "A"; user taps "B".
  2. UpdateVirtualSingleSelection sets the flag and assigns SelectedItem = "B".
  3. The synchronous SelectionChanged/SelectionChangedCommand handler does cv.SelectedItem = null.
  4. That nested write''s MapSelectedItem hits this early return, and so does the outer one.
  5. finally clears the flag but performs no reconciliation.

Result: MAUI SelectedItem is null while the WinUI container for "B" stays visually selected, and nothing re-syncs. Before this change, the nested MapSelectedItemUpdatePlatformSelectionDeselectAll kept the two in sync. Consider scoping the suppression to the single assignment (e.g. compare the value the handler itself wrote and re-run UpdatePlatformSelection in the finally when ItemsView.SelectedItem no longer matches it), or fixing the root cause noted below.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Handler Mapper and Property Patterns — Suppressing MapSelectedItem for the entire assignment window drops legitimate selection changes that originate during that window. Concrete scenario: SelectedItem is two-way bound to a view model that coerces/rejects the value (a common "don't allow selecting the placeholder row" pattern). UpdateVirtualSingleSelection sets SelectedItem = null, the VM setter writes back the previous item, the resulting mapper call is swallowed by this guard, and the platform is never told — WinUI keeps the blank row highlighted while CollectionView.SelectedItem is the previous item. Prefer detecting the no-op (compare the incoming value against the current platform selection inside UpdatePlatformSelection) over an unconditional early return, so genuine re-entrant changes still propagate.

return;

handler.UpdatePlatformSelection();
}

Expand Down Expand Up @@ -277,7 +288,10 @@ void UpdateVisualStates()
if (itemContainer?.Child is ElementWrapper wrapper && wrapper.VirtualView is VisualElement visualElement)
{
var actualItem = visualElement.BindingContext;
bool isSelected = object.Equals(ItemsView.SelectedItem, actualItem) || ItemsView.SelectedItems.Contains(actualItem);
// Guard against false positives for null items: object.Equals(null, null) = true would
// mark every null-item container as Selected whenever SelectedItem is null (no selection).
bool isSelected = actualItem is not null &&
(object.Equals(ItemsView.SelectedItem, actualItem) || ItemsView.SelectedItems.Contains(actualItem));
VisualStateManager.GoToState(visualElement, isSelected ? VisualStateManager.CommonStates.Selected : VisualStateManager.CommonStates.Normal);

// When the item template defines a "Selected" visual state, MAUI
Expand Down Expand Up @@ -313,10 +327,13 @@ void UpdateVirtualSingleSelection()
? itemPair.Item
: PlatformView.SelectedItem;

// Use flag instead of detach/re-attach so that MapSelectedItem is suppressed
// while SelectedItem is set. Both fire synchronously; the flag is reset after.
_ignoreVirtualSelectionChange = true;
Comment thread
SuthiYuvaraj marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Cross-Platform Behavioral Consistency — The round-trip guard is applied only to the single-selection path. UpdateVirtualMultipleSelection still writes ItemsView.SelectedItems with no equivalent flag, and MapSelectedItems (line 90) calls UpdatePlatformSelection() unconditionally. Concrete scenario: SelectionMode="Multiple" over a source containing a null entry — tapping the blank row runs the same WinUI-selects-null → virtual-write → MapSelectedItemsUpdatePlatformSelection round trip this PR fixes for single selection, and the selection is undone. Neither device test added here covers multiple selection, so the asymmetry is untested.

ItemsView.SelectionChanged -= VirtualSelectionChanged;
ItemsView.SelectedItem = selectedItem;

ItemsView.SelectionChanged += VirtualSelectionChanged;
_ignoreVirtualSelectionChange = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[moderate] Handler Mapper and Property Patterns_ignoreVirtualSelectionChange and the temporary SelectionChanged unsubscribe are not restored in a finally block. If ItemsView.SelectedItem = selectedItem throws from a bindable-property callback, validation path, or user PropertyChanged handler, the handler stays with SelectionChanged detached and _ignoreVirtualSelectionChange == true, so subsequent virtual/platform selection changes are silently suppressed for the handler lifetime. Wrap the assignment in try/finally that always reattaches VirtualSelectionChanged and resets _ignoreVirtualSelectionChange.

Comment thread
SuthiYuvaraj marked this conversation as resolved.
Outdated
}

void UpdateVirtualMultipleSelection()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
}
}

ItemTemplateContext2 CreateItemContext(object item) =>
ItemTemplateContext2 CreateItemContext(object? item) =>
new(_itemTemplate, item, _container, mauiContext: _mauiContext);

ItemTemplateContext2? CreateHeaderContext(object group) =>
Expand Down Expand Up @@ -320,11 +320,9 @@ void HandleGroupItemsReplace(NotifyCollectionChangedEventArgs e, int flatIndex)
for (int i = 0; i < e.NewItems.Count; i++)
{
oldItems.Add(Items[replaceIndex + i]);
var item = e.NewItems[i];
if (item is null)
continue;

var newItem = CreateItemContext(e.NewItems[i]!);
// Always create a new context even for null new items — skipping the slot update
// would leave the stale old ItemTemplateContext2 in the flat list (desync).
var newItem = CreateItemContext(e.NewItems[i]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention - The Replace-with-null desync fix here is correct (previously oldItems.Count != newItems.Count in the raised NotifyCollectionChangedEventArgs and the stale ItemTemplateContext2 was left in the flat list), but nothing in this PR exercises it.

The grouped path received two behaviour-affecting changes in this PR - this one and the switch of PerformGroupedReorder to positional _draggedSourceIndex matching (MauiItemsView.DragDrop.cs:862-900) - yet every added test (2 device tests, 2 UI tests, 1 HostApp page) uses a flat, non-grouped source. Grouped + null items with group headers/footers is precisely the case the new flat-index arithmetic was written for (see the comment on line 864 about multiple null items across groups), and it is completely uncovered.

Please add at least: (a) a test that replaces a group item with null and asserts the flat collection stays in sync in Items.Count and index positions, and (b) a grouped variant of the HostApp reorder page with a null item in a group so the header/footer offset arithmetic in PerformGroupedReorder is exercised.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — This desync fix (always creating a replacement context so the flat mirror keeps a slot for a null new item) is correct and matches the initial-build path at line 140, which never skipped nulls. It has no test: neither device test in this PR uses IsGrouped, and the UI test page is ungrouped. The specific scenario to cover is a grouped ObservableCollection where an inner group's item is replaced with null (group[i] = null) — before this change the flat list kept the stale ItemTemplateContext2 and every subsequent index-based lookup in MauiItemsView.DragDrop.cs was off. Add a device test asserting the flattened collection count/contents after such a replace; the same test project also exercises the PerformGroupedReorder rewrite flagged at line 884.

newItems.Add(newItem);
Items[replaceIndex + i] = newItem;
Comment on lines +323 to 327
}
Expand Down
19 changes: 19 additions & 0 deletions src/Controls/src/Core/Handlers/Items2/Windows/ItemFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,25 @@ internal partial class ItemFactory(ItemsView view) : IElementFactory
// NOTE: 1.6: replace w/ RecyclePool
if (args.Data is ItemTemplateContext2 templateContext)
{
// Null regular-data items render as blank rows with no template content,
// matching CV1 behaviour (ItemContentControl.Realize() returns early when
// dataContext is null). Header/footer items fall through to the normal
// path so they can inherit the parent ItemsView.BindingContext.
if (templateContext.Item is null && !templateContext.IsHeader && !templateContext.IsFooter)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Performance-Critical Path — The new blank ItemContainer returned for null regular-data items bypasses _recyclePool entirely: RecycleElement keys pooling on wrapperView?.GetValue(OriginTemplateProperty), which is always null for these blank containers (no ElementWrapper/no view), so template != null is never true and the container is never added back to _recyclePool. Concrete scenario: a long scrollable CollectionView2 on Windows whose source contains many null rows — every time a null row scrolls into view, GetElement allocates a brand-new ItemContainer (line 54) instead of reusing a pooled one like every other item type, adding avoidable GC churn on the scroll hot path that this PR's own performance-sensitive code elsewhere (e.g. FindAllContainers, IndexOfItem) is careful to avoid.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Cross-Platform Behavioral Consistency — Short-circuiting here means a null data item no longer inflates the ItemTemplate at all on Windows CV2, which diverges from Android and iOS.

On Android (Items/Android) and iOS (Items2/iOS) a null item still realizes the template with a null BindingContext, so a template containing static/non-bound content (a fixed Label, an icon, a FallbackValue/TargetNullValue binding, or a converter that maps null to placeholder text) renders normally. After this change the same app shows an empty 32px strip on Windows only. The comment justifies the behaviour by matching CV1 Windows, but the Items/ handler is not the cross-platform contract — the other two platforms are.

If the goal is only to avoid the crash, the narrower fix is to keep realizing the template (and to guard DataTemplateSelector.SelectTemplate(null, ...) at line 81, which is the call that actually throws for user selectors), rather than dropping the template entirely.

{
// CV1's ItemContentControl.MeasureOverride returns a 32px height hint when
// _handler is null (virtualization hint only). The actual rendered height
// is driven by WinUI's ListViewItem default MinHeight (40px from the
// ListViewItemMinHeight theme resource). CV2 has no ListViewItem wrapper, so
// set MinHeight = 40 directly on the ItemContainer to match CV1's visual slot.
return new ItemContainer
Comment thread
SuthiYuvaraj marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Windows CollectionView layout — The null-item placeholder only sets MinHeight. In a horizontal CollectionView the blank container is measured with unconstrained width and finite height; with no child and no width hint it can report 0 width, collapsing null items and making drag/drop hit-testing skip that slot. CV1 handled the same null-data path with a width hint when width was unconstrained (ItemContentControl.MeasureOverride returns 88px for finite-height/infinite-width). Please mirror that finite-dimension behavior here instead of hard-coding only vertical height.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Memory Leak Prevention / Performance-Critical Path — Null-item ItemContainers created here are never returned to _recyclePool. In RecycleElement, wrapperView is null for these blank containers (no ElementWrapper), so template is null and the if (template != null && item is not null) pool-insertion guard is skipped. ItemsRepeater still calls RecycleElement when the container scrolls off-screen, but since it's not pooled, GetElement allocates a fresh ItemContainer on every subsequent realization. In a list with null items under repeated scrolling this causes unbounded allocation pressure. Add a null-item recycle pool keyed on a sentinel (e.g. _nullItemPool) and return / retrieve from it in RecycleElement/GetElement respectively.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Cross-Platform Behavioral Consistency / Backward Compatibility - Returning a bare ItemContainer for null items silently drops all template content for those rows, which is a visible behaviour change on Windows CV2 and a divergence from the other CV2 platforms.

Before this change a null item still realized the DataTemplate with BindingContext = null (lines 144-146), so anything in the template that is not item-bound still rendered: static labels/borders, TargetNullValue/FallbackValue bindings, placeholder graphics, and any HeightRequest. After this change none of that renders and the row collapses to a fixed 32x88 box. Note this also bypasses DataTemplateSelector.SelectTemplate entirely, so a selector that deliberately returns a placeholder template for null is never consulted.

On iOS/MacCatalyst CV2 (TemplatedCell2) and on Android the template is still realized with a null binding context, so the same app now shows a template-rendered row on iOS/Android and an empty 32px strip on Windows. The comment justifies this as matching CV1 behaviour (ItemContentControl.Realize() early-returns on a null dataContext, ItemContentControl.cs:173) - accurate for CV1/Windows, but CV1 parity was chosen over CV2 cross-platform parity without calling out the trade-off.

Also, MinHeight = 32 / MinWidth = 88 are unexplained magic numbers that ignore ItemsLayout.ItemHeight/ItemWidth, ItemSizingStrategy, and any HeightRequest in the template. At minimum extract them as named constants with a comment citing the WinUI/CV1 source of the values, and confirm the behaviour change is intentional.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Layout Measure-Arrange Correctness — Hardcoding MinHeight = 32 / MinWidth = 88 for the blank container is risky for GridItemsLayout.

CreateGridView only sets MinItemWidth/MinItemHeight when the CollectionView has an explicit cross-axis WidthRequest/HeightRequest (ApplyMinItemSizeForSpan returns early otherwise, ItemsViewHandler2.Windows.cs:813). With those left at NaN, WinUI''s UniformGridLayout derives the uniform cell size from the first realized element. A collection whose first item is null therefore sizes every cell in the grid to 88×32, clipping all real items.

Please either exclude blank containers from the layout''s size derivation, or size the blank container from the measured/cached item size (CollectionViewHandler2.GetCachedFirstItemSize) rather than from two magic numbers. At minimum, add a GridItemsLayout + leading-null test — no test in this PR covers a grid layout.

{
MinHeight = 40,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] CollectionView Windows Layout — The null-item placeholder only sets a vertical size hint. In a horizontal CollectionView (ItemsLayout horizontal) the null row has no child content and no MinWidth, so it can measure to zero width and the blank null slot collapses instead of occupying a selectable/reorderable item slot. CV1 supplied a width hint for the unbounded-width case; CV2 should set the along-axis placeholder size as well, and add horizontal coverage.

VerticalAlignment = VerticalAlignment.Stretch,
HorizontalAlignment = HorizontalAlignment.Stretch
};
Comment thread
SuthiYuvaraj marked this conversation as resolved.
Outdated
}
Comment on lines +56 to +76

DataTemplate? template = templateContext.MauiDataTemplate;
if (template is DataTemplateSelector selector)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ void ItemsRepeater_ElementPrepared(ItemsRepeater sender, ItemsRepeaterElementPre
// item's new position, hide it immediately. This is what makes the
// "empty source slot" follow the dragged item without us having to
// chase a moving _sourceContainer reference through dispatcher races.
if (_draggedItem is not null && IsContainerBoundToDraggedItem(itemContainer))
if (_draggedSourceIndex >= 0 && IsContainerBoundToDraggedItem(itemContainer))
{
itemContainer.Opacity = 0;
itemContainer.IsHitTestVisible = false;
Expand All @@ -332,7 +332,7 @@ void ItemsRepeater_ElementPrepared(ItemsRepeater sender, ItemsRepeaterElementPre
else
{
// Apply dim if a drag is in progress and this is not the source.
itemContainer.Opacity = _draggedItem is not null ? DragDimOpacity : 1;
itemContainer.Opacity = _draggedSourceIndex >= 0 ? DragDimOpacity : 1;
itemContainer.IsHitTestVisible = true;
}
}
Expand Down Expand Up @@ -409,21 +409,31 @@ void ItemContainer_DragStarting(UIElement sender, UI.Xaml.DragStartingEventArgs
{
var itemContainer = (ItemContainer)sender;

// Check whether this container is bound to a source slot at all.
// A container that has an ElementWrapper with a View IS bound — the item
// itself may be null (valid null data row), so we must not treat null as
// "no binding". We store the result so the null-item path shares the same
// cancel logic as the "container not yet set up" path.
bool hasBinding = itemContainer.Child is ElementWrapper _ew && _ew.VirtualView is View;
Comment thread
Copilot marked this conversation as resolved.
Outdated

// Use the container's currently bound item first. The Tag/index can become
// stale after a reorder because the element is reused without being recreated.
object? item = GetContainerItem(itemContainer);

// Fallback: look up by index from the source (works for IList and IEnumerable).
if (item is null && itemContainer.Tag is int index && index >= 0)
// Only run when there is no ElementWrapper binding (container not yet set up),
// NOT when item is null — a null BindingContext is a valid null data row.
if (!hasBinding && itemContainer.Tag is int index && index >= 0)
{
var sourceList = GetSourceList();
if (sourceList is not null && index < sourceList.Count)
{
item = GetItemAtIndex(index, sourceList);
hasBinding = true;
}
}

if (item is null)
if (!hasBinding)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness — Now that every downstream guard was switched from _draggedItem is not null to _draggedSourceIndex >= 0 (lines 326, 335, 672, 755, 854, 1766), DragStarting must also validate the index it captures.

_draggedSourceIndex = GetContainerIndex(itemContainer) (line 453) can return -1: the new null-item branch returns -1 when GetElementIndex fails and Tag is not an int, and the final fallback returns allContainers.IndexOf(container) which is -1 for a container not found in the visual-tree walk. In that case the drag is not cancelled — args.Cancel is left false and the deferred callback at line 470 still sets _sourceContainer.Opacity = 0 — but ScrollViewer_Drop immediately bails on _draggedSourceIndex < 0, so the user sees the row vanish, drags, drops, and nothing reorders. Add if (_draggedSourceIndex < 0) { args.Cancel = true; return; } after the index is captured.

{
args.Cancel = true;
return;
Comment on lines +446 to 449
Expand Down Expand Up @@ -539,17 +549,28 @@ void ScrollViewer_DragOver(object sender, UI.Xaml.DragEventArgs e)

bool IsContainerBoundToDraggedItem(ItemContainer container)
{
return _draggedItem is not null && IsContainerBoundToItem(container, _draggedItem);
// Use _draggedSourceIndex as the "drag active" sentinel so that a null
// _draggedItem (valid null data row) does not short-circuit the check.
return _draggedSourceIndex >= 0 && IsContainerBoundToItem(container, _draggedItem);
}

static bool IsContainerBoundToItem(ItemContainer container, object item)
static bool IsContainerBoundToItem(ItemContainer container, object? item)
{
if (container.Child is not ElementWrapper wrapper || wrapper.VirtualView is not View view)
{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[minor] Complexity Reduction — This branch is unreachable from the only caller and over-matches if it ever becomes reachable.

IsContainerBoundToDraggedItem (line 568) already returns via the container.Child is not ElementWrapper Tag comparison before delegating here, so the "blank container (no ElementWrapper)" case described in the comment can never reach this code. The condition that does reach it is Child is ElementWrapper but VirtualView is not View — a partially realized container — and returning item is null there means every such container is treated as the drag source (opacity 0, hit-testing off) during a null-item drag. Either delete the branch or restrict it to container.Child is null.

return false;
// Blank container (no ElementWrapper) represents a null data item.
// It matches when the dragged item is also null.
return item is null;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major] Logic and Correctness — Treating any blank ItemContainer as bound to null makes a null-item drag match every realized null row, not just the source row. During live reorder, ItemsRepeater_ElementPrepared calls IsContainerBoundToDraggedItem after ApplyDragAffordance, so with two null items all blank null containers get Opacity = 0 and IsHitTestVisible = false. Use the instance state available there (for example container.Tag is int t && t == _draggedSourceIndex, or ItemsRepeater.GetElementIndex) for blank/null containers instead of value matching on null.

Comment thread
SuthiYuvaraj marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness - This new branch (and the bound is null && item is null branch on lines 596-599) re-introduces exactly the ambiguity that the Tag-based discriminator on line 570 was added to eliminate: when _draggedItem is null, any container without a resolvable ElementWrapper/View reports itself as the drag source, so ItemsRepeater_ElementPrepared (line 326) can hide more than one row (Opacity = 0; IsHitTestVisible = false).

It is also unreachable for the case the comment describes. The only caller, IsContainerBoundToDraggedItem, already routes every container.Child is not ElementWrapper case to the Tag comparison on line 570, so this method is only entered when the container does have an ElementWrapper. That leaves dead code plus a live-but-ambiguous path for a wrapper whose VirtualView is not a View.

Recommend deleting these null-matching branches and keeping return false (the Tag check is the authoritative null-row discriminator), or routing this method through the same index comparison so a null dragged item can never match by value.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[minor] Complexity Reduction — This new early-return is unreachable. IsContainerBoundToItem has exactly one caller (IsContainerBoundToDraggedItem, line 573), and that caller already returns at line 568-571 whenever container.Child is not ElementWrapper. The container.Child is not ElementWrapper half of this condition can therefore never be true here, and the wrapper.VirtualView is not View half returns item is null — a semantics change that only ever produces true for a null dragged item on a container whose wrapper has a non-View virtual view. Either delete the block (keep return false;) or make it the single place blank containers are matched and drop the duplicate logic in the caller.

}

var bound = view.BindingContext;
// Allow null-bound containers to match a null dragged item.
// ReferenceEquals(null, null) == true, Equals(null, null) == true.
if (bound is null && item is null)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] CollectionView Windows (Drag/Drop) — This new null-matching branch (plus the corresponding removal of the _draggedItem is not null guard in IsContainerBoundToDraggedItem above) can misidentify a group header/footer container as the dragged container. IsContainerBoundToDraggedItem only special-cases blank (non-ElementWrapper) containers via Tag equality; header/footer containers DO have an ElementWrapper child, so they fall through to IsContainerBoundToItem(container, _draggedItem). If a null data row is being dragged (_draggedItem == null) and a header/footer's view.BindingContext also happens to be null (e.g. an ungrouped header combined with a null page BindingContext, per ItemFactory.GetElement's templateContext.Item ?? _view.BindingContext fallback), this now returns true for that unrelated header/footer container, causing it to be incorrectly dimmed/hidden as if it were the drag source. Headers/footers already have CanDrag = false so they can never legitimately be _draggedItem's container; consider excluding IsHeaderOrFooter wrappers from this null-match branch.

{
return true;
}

if (bound is null)
{
return false;
Expand Down Expand Up @@ -619,7 +640,9 @@ void ScrollViewer_DragLeave(object sender, UI.Xaml.DragEventArgs e)

void ScrollViewer_Drop(object sender, UI.Xaml.DragEventArgs e)
{
if (!_canReorderItems || _draggedItem is null || _insertionIndex < 0 || _mauiVirtualView is null)
// _draggedSourceIndex < 0 means no active drag. _draggedItem may be null for
// null data rows, so we cannot use _draggedItem is null as the guard here.
if (!_canReorderItems || _draggedSourceIndex < 0 || _insertionIndex < 0 || _mauiVirtualView is null)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Null Safety and Defensive Coding - Swapping the drag-in-progress sentinel from _draggedItem is not null to _draggedSourceIndex >= 0 is right for null rows, but _draggedSourceIndex is now assigned from a fallible lookup: _draggedSourceIndex = GetContainerIndex(itemContainer) (line 453), and GetContainerIndex can return -1 (the new null branch at line 1252 returns -1 when GetElementIndex fails and Tag is not an int; the non-null path can also fall through to allContainers.IndexOf(container) returning -1).

When that happens DragStarting has already returned without setting args.Cancel, so WinUI starts a real drag, but every downstream guard (ItemsRepeater_ElementPrepared lines 326/335, DimNonSourceContainers line 1767, this Drop guard, PerformReorder line 755, PerformGroupedReorder line 854) now treats it as no drag in progress: no dim, no source hiding, and the drop is silently discarded. Previously a non-null _draggedItem kept all of those alive.

Guard at the source - after computing the index in DragStarting, cancel the drag if it is negative:

_draggedSourceIndex = GetContainerIndex(itemContainer);
if (_draggedSourceIndex < 0) { args.Cancel = true; return; }

{
CleanupDragState();
return;
Expand Down Expand Up @@ -701,7 +724,8 @@ void ScrollViewer_Drop(object sender, UI.Xaml.DragEventArgs e)

bool PerformReorder(IList itemsList)
{
if (_draggedItem is null)
// _draggedSourceIndex < 0 means no active drag; _draggedItem may be null for null data rows.
if (_draggedSourceIndex < 0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness Verification — With _draggedItem allowed to be null, the resolution below degrades badly for multi-null sources. If the captured index fails validation (GetItemAtIndex(_draggedSourceIndex, itemsList) not equal to _draggedItem), the fallback oldIndex = IndexOfItem(_draggedItem, itemsList) with a null item returns the first null in the list. Concrete scenario: ItemsSource = [A, null, B, null, C], drag the second null after any concurrent insert/remove shifts the row — the first null is moved instead, producing a silently wrong order with no error. Because Equals(null, null) is true, the index validation itself is also non-discriminating: any index that happens to hold null is accepted. For null payloads the fallback should fail the reorder (return false) rather than guess a slot.

{
return false;
}
Expand Down Expand Up @@ -799,7 +823,8 @@ bool PerformReorder(IList itemsList)
/// </summary>
bool PerformGroupedReorder(IList groupsList)
{
if (_draggedItem is null || _mauiVirtualView is not GroupableItemsView groupableView)
// _draggedSourceIndex < 0 means no active drag; _draggedItem may be null for null data rows.
if (_draggedSourceIndex < 0 || _mauiVirtualView is not GroupableItemsView groupableView)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major] Logic and Correctness — Allowing _draggedItem == null into grouped reorder still leaves source-group discovery based on value equality (ReferenceEquals(groupItem, _draggedItem) || Equals(groupItem, _draggedItem)). In grouped data with more than one null item, this picks the first null in the first matching group rather than the row whose container started the drag, so dragging Group B[0] == null can move Group A's null item instead. For null items, map _draggedSourceIndex (the captured flat row index, accounting for group headers/footers) back to source group/item instead of searching by value.

Comment thread
SuthiYuvaraj marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] CollectionView Windows (Drag/Drop) — Changing this guard from _draggedItem is null to _draggedSourceIndex < 0 now lets null-item drags reach PerformGroupedReorder's source-row lookup below (the foreach (var groupItem in groupItems) { if (ReferenceEquals(groupItem, _draggedItem) || Equals(groupItem, _draggedItem)) ... } loop, unchanged by this PR). That loop still discovers the dragged row purely by value equality and stops at the FIRST match. Concrete scenario: a grouped ReorderableItemsView with two groups that each contain a null row (or two groups containing value-equal duplicate items) — dragging the null/duplicate row in the second group will resolve sourceGroupIndex/sourceItemIndex to the first group's matching row instead, silently reordering the wrong item. PerformReorder (the flat/non-grouped sibling, lines ~752-764 in this same file) was correctly updated in this PR to validate _draggedSourceIndex first and only fall back to value-equality search; PerformGroupedReorder should receive the equivalent fix (map _draggedSourceIndex to (sourceGroupIndex, sourceItemIndex) the same way _insertionIndex is mapped to (targetGroupIndex, targetItemIndex) a few lines below) instead of relying solely on ReferenceEquals/Equals across all groups.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] CollectionView — Shared Models — Allowing null drags into PerformGroupedReorder still leaves grouped source discovery based on ReferenceEquals/Equals below. With two null (or value-equal) rows in different groups, the scan selects the first matching item rather than the flat slot captured in _draggedSourceIndex, so dragging the second null can remove/move the first null instead. The grouped path needs to map _draggedSourceIndex to source group/item index, not search by value.

{
return false;
}
Expand Down Expand Up @@ -1179,18 +1204,23 @@ int GetContainerIndex(FrameworkElement container)
// index and avoids the ambiguity where group headers and footers share the
// same underlying Item (the group object). Validate the tag by checking that
// the item at that index still matches the container's current item.
// The containerItem is not null guard was intentionally removed: null-item
// containers (valid null data rows) need tag validation too, and
// Equals(null, null) correctly returns true for them.
if (container.Tag is int tagIndex && sourceList is not null &&
Comment on lines 1255 to 1262
tagIndex >= 0 && tagIndex < sourceList.Count)
{
var tagItem = GetItemAtIndex(tagIndex, sourceList);
if (containerItem is not null && Equals(tagItem, containerItem))
if (Equals(tagItem, containerItem))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major] Logic and Correctness — The tag validation now accepts Equals(null, null), so a stale Tag on a null-item container is considered valid whenever the old index also contains any null item. If that check does not return, the new linear-search fallback has the same ambiguity (IndexOfItem(null, sourceList) returns the first null). After a reorder or recycled-container handoff, a second drag on a later null row can therefore capture the wrong _draggedSourceIndex and move the wrong data item. Use ItemsRepeaterControl.GetElementIndex(container as ItemContainer) for null/blank containers before any value-equality fallback.

Comment thread
SuthiYuvaraj marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] CollectionView Windows (Drag/Drop) — Removing the containerItem is not null guard means a stale container.Tag is accepted as valid whenever Equals(tagItem, containerItem) is true — which for a null-item container (containerItem is always null for these blank containers) is true for ANY index in sourceList that currently also holds null, not just the container's actual slot. Concrete scenario: a source list with two null rows; after a reorder shifts positions, a container's now-stale Tag can point at the OTHER null row's index and still pass this check (both are null), so GetContainerIndex — and therefore _draggedSourceIndex (set from this call in ItemContainer_DragStarting) and targetIndex in ScrollViewer_DragOver — can silently resolve to the wrong null slot when multiple null rows exist. Consider using the ItemsRepeater's authoritative GetElementIndex for null-item containers here too (as UpdateAllContainerIndices now does below), rather than trusting an Equals(null, null) match against a stale Tag.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] CollectionView — Windows — This validation now accepts Equals(null, null) for blank null-item containers. If a realized null container has a stale Tag that points at any other null row, the tag is treated as authoritative and GetContainerIndex returns the wrong source slot; the fallback below has the same ambiguity because IndexOfItem(null, ...) returns the first null. Null containers should use ItemsRepeater.GetElementIndex (as UpdateAllContainerIndices does) or another per-slot discriminator instead of value equality.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[minor] Complexity Reduction - The comment added above this line (The containerItem is not null guard was intentionally removed: null-item containers ... need tag validation too, and Equals(null, null) correctly returns true for them) is stale and misleading, as is the matching comment on line 1273.

The new early return on line 1240 (if (containerItem is null) { ... return; }) means this code is only reachable when containerItem is not null. The removed guard therefore has no effect, and null-item containers never reach either the tag validation or the IndexOfItem fallback - they are handled entirely by the GetElementIndex branch above.

Either drop these two comments, or restore the explicit containerItem is not null guard and delete the early return if the intent really was to let null containers use tag validation.

{
Comment on lines +1259 to 1267
return tagIndex;
}
}

// Tag is stale — fall back to a linear search.
if (sourceList is not null && containerItem is not null)
// The containerItem is not null guard was removed: null-item containers
// need linear search fallback just like any other item.
if (sourceList is not null)
Comment on lines 1272 to +1275
{
var liveIndex = IndexOfItem(containerItem, sourceList);
if (liveIndex >= 0)
Expand All @@ -1209,7 +1239,7 @@ int GetContainerIndex(FrameworkElement container)
return allContainers.IndexOf(container);
}

int IndexOfItem(object item, IList itemsList)
int IndexOfItem(object? item, IList itemsList)
{
// First pass: reference equality — correctly distinguishes two items that are
// value-equal but distinct objects (e.g., duplicate records in the list).
Expand Down Expand Up @@ -1256,6 +1286,8 @@ void UpdateAllContainerIndices()
return;
}

var repeater = ItemsRepeaterControl;

// Derive each container's Tag from its item's actual position in the source.
// A positional loop (containers[i].Tag = i) is wrong when ItemsRepeater
// virtualizes: FindAllContainers skips unrealized slots, so containers[i]
Expand All @@ -1271,6 +1303,20 @@ void UpdateAllContainerIndices()
container.Tag = actualIndex;
}
}
else
{
// For null-item containers, IndexOfItem returns the first null which
// may be a different row. Use the ItemsRepeater's authoritative element
// index instead — it is always accurate after a layout pass.
if (repeater is not null && container is ItemContainer ic)
{
int repeaterIndex = repeater.GetElementIndex(ic);
if (repeaterIndex >= 0)
{
container.Tag = repeaterIndex;
}
}
}
}
}

Expand Down Expand Up @@ -1664,7 +1710,9 @@ static void RemoveDragGhostAppearance(ItemContainer container)
/// </summary>
void DimNonSourceContainers()
{
if (_draggedItem is null)
// _draggedSourceIndex < 0 means no drag is in progress.
// _draggedItem may be null for null data rows, so we cannot use that as the guard.
if (_draggedSourceIndex < 0)
{
return;
}
Expand Down
Loading