Skip to content

[WinUI][CV2] CollectionView2 Handler Implementation for Windows#34600

Open
SuthiYuvaraj wants to merge 117 commits into
net11.0from
BugFixes_WinUI_CV2
Open

[WinUI][CV2] CollectionView2 Handler Implementation for Windows#34600
SuthiYuvaraj wants to merge 117 commits into
net11.0from
BugFixes_WinUI_CV2

Conversation

@SuthiYuvaraj

@SuthiYuvaraj SuthiYuvaraj commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Introduction

This PR implements the CollectionView2 (CV2) handler for Windows (WinUI), bringing the new ItemsRepeater-based CollectionView architecture to the Windows platform. This is part of the ongoing effort to replace the legacy ListView-based CollectionView implementation with a modern, performant, and feature-complete handler across all platforms.

The CV2 handler on Windows leverages WinUI's ItemsRepeater control as the virtualizing layout engine, providing better performance, smoother scrolling, and more consistent behavior aligned with the CV2 implementations on Android, iOS, and MacCatalyst.

Description of Changes

New Windows CV2 Handler Architecture

  • ItemsViewHandler2.Windows.cs — Core handler implementation mapping all CollectionViewproperties and commands to the WinUI ItemsRepeater-based view. Handles layout mapping (LinearItemsLayoutStackLayout, GridItemsLayoutUniformGridLayout), scroll management, selection, snapping, incremental loading, and RefreshView integration.

  • CollectionViewHandler2.Windows.cs — CollectionView-specific handler derived from the base items view handler, implementing selection modes (None, Single, Multiple), visual states, pre-selection, and pointer-based item interaction.

  • MauiItemsView.cs — Custom WinUI UserControl hosting the ItemsRepeater inside a ScrollViewer, with support for Header/Footer, EmptyView overlay, and RefreshContainer wrapping.

  • GroupableUniformGridLayout.cs — Custom layout that extends UniformGridLayout behavior to support grouped collection scenarios with proper group header/footer sizing.

Item Source & Templating Pipeline

  • ItemFactory.csIElementFactory implementation that creates WinUI elements from MAUI DataTemplate/DataTemplateSelector, manages recycling, and handles grouped vs flat item sources.

  • ObservableItemTemplateCollection2.cs — Bridges INotifyCollectionChanged source collections to ItemsRepeater-compatible observable collections, translating insert/remove/replace/move/reset operations.

  • GroupedItemTemplateCollection2.cs — Flattens grouped source data (with group headers, items, and group footers) into a single linear collection consumable by ItemsRepeater, tracking group boundary indices.

  • TemplatedItemSourceFactory2.cs — Factory that selects the appropriate templated collection type (observable, grouped, or enumerable context) based on the items source characteristics.

  • ItemTemplateContext2.cs, ItemTemplateContextEnumerable2.cs, ItemTemplateContextList2.cs — Context wrappers for item template binding.

Supporting Changes

  • ItemsViewExtensions.Windows.cs — New extension class with helper methods for converting MAUI layout types, snap point settings, and sizing strategies to their WinUI equivalents.

  • ItemsViewStyles.xaml — Updated WinUI resource dictionary with refined styles and templates for CV2 items, group headers/footers, header/footer, and empty view containers.

  • AppHostBuilderExtensions.cs — Registered the CV2 handler mappings for Windows in the MAUI host builder, gated behind the CollectionView2 runtime feature flag.

  • RuntimeFeature.cs — Added the CollectionView2 feature switch for conditional handler activation.

  • PublicAPI.Unshipped.txt — Added 43 new public API entries for the Windows CV2 surface area.

  • Build property integration — Updated .csproj files and build targets to include the UseCollectionView2 MSBuild property for opt-in activation.

Issues Fixed

The following issues are addressed by this implementation and the associated bug fixes:

Issue Description
#28895 CollectionView2 Implementation
#22320 IEnumerable source rendering not working in CollectionView
#26187 UITest failure — CollectionView items not displaying correctly
#27797 UITest failure — CollectionView interaction issues
#28212 CollectionView item layout/rendering bug
#31899 CollectionView Windows-specific rendering issue
#32543 CollectionView selection/interaction bug
#33487 CollectionView layout calculation issue

Major Functional Improvements

  • Layout: Fixed GridItemsLayout span width; fixed LayoutOptions alignment; fixed CornerRadius rendering; fixed item margin and pointer-over visuals.
  • Scrolling: Fixed PullToRefresh with ScrollViewer; fixed KeepScrollOffset/KeepItemsInView; fixed ScrollTo on unloaded view; fixed horizontal flicker.
  • Grouping: Fixed group header/footer rendering; fixed grouped grid layout; fixed ScrollTo for grouped items.
  • EmptyView: Fixed visibility toggling; removed unnecessary measure pass.
  • MeasureFirstItem: Fixed incorrect size with URL-based images; fixed size recalculation on DataTemplate change.
  • Data: Fixed DataTemplateSelector not updating on CollectionChanged.
  • Orientation: Fixed orientation switch not updating layout.

Platforms Affected

  • Windows (WinUI)

UI Difference from CV1

Selection UI
Selection appearance differs from CV1, where the selected item is now highlighted using a border that fully wraps the item, instead of the previous visual style.

Output Screenshot

CV1 Behavior CV2 Behavior
Before Fix After Fix
Before Fix - Multi Select After Fix - Multi Select

Drag and Drop Implementation

CV1 Behavior CV2 Behavior
CV1DragandDrop.mp4
AnimationFinal.mp4

Testing

  • Existing CollectionView UITests passing on Windows with CV2 handler enabled
  • Manual testing scenarios validated for: flat lists, grouped lists, grid layouts, selection modes, scrolling, snap points, pull-to-refresh, empty views, header/footer, incremental loading, and data template selectors

@vishnumenon2684

Copy link
Copy Markdown
Contributor

/azp run maui-pr-uitests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vishnumenon2684

Copy link
Copy Markdown
Contributor

/azp run maui-pr-uitests , maui-pr-devicetests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@kubaflo

kubaflo commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

AI code review refresh for net11.0 target

Head reviewed: 23594dbde96784d6f9deab6eda05b52483ce09a5
Target branch: net11.0 (open, MERGEABLE).

Verdict: 🟡 Needs discussion (new commits are clean improvements; merge still gated by red CI, but all red is on non‑Windows legs this change cannot affect)

Since round fourteen (head 45cfe077) exactly two commits landed (git compare = ahead 2 / behind 0). I inspected the delta independently from the diff before reading the narrative.

What changed since round 14 (independently inspected)

  1. 36bd1803ItemsViewHandler2.Windows.cs (+4/-1). MapIsEnabled was changed from public static new void to static new void (now private), with an added comment explaining the static new hides the inherited ViewHandler.MapIsEnabled (suppresses CS0108) and is kept private so it does not enter the public API surface. This directly resolves the latent public‑API concern from the prior rounds — I confirmed there is no MapIsEnabled entry in net-windows/PublicAPI.Unshipped.txt, so no analyzer churn is required. Behavior is byte‑for‑byte identical (still calls ViewHandler.MapIsEnabled first, then fades MauiItemsView.Opacity to 0.3 only on direct disable). Good encapsulation tidy‑up. ✅
  2. 23594dbdIssue28343_ProgressSpinnerDisabled.png. Snapshot baseline refresh (windows folder) only. Test asset, no code impact.

The round‑fourteen disabled‑state fix (#28343: app‑wide ItemContainerDisabledOpacity = 1.0 suppression + explicit whole‑list fade for the direct‑disable case) is otherwise unchanged and still reads as correct.

Prior‑review reconciliation

  • Round 14 ❌/⚠️ "MapIsEnabled adds public API surface" / encapsulationResolved this round (now private static new, confirmed absent from PublicAPI.Unshipped.txt).
  • Round 14 ⚠️ global ItemContainerDisabledOpacity override scope → unchanged; still app‑wide once CV2 styles merge. Intended for CV2; low risk (Windows‑only, ItemContainer is effectively CV2's control). Maintainer nod still appropriate.
  • Carried policy item: CV2 default‑on for all Windows CollectionView (RuntimeFeature.IsWindowsCollectionView2HandlerEnabledByDefault = true, FeatureSwitchDefinition for NET11+, Windows‑gated in AppHostBuilderExtensions #elif WINDOWS; CarouselView stays on CV1) → plumbing is clean and trimmer‑friendly, but the default‑on flip still warrants explicit maintainer sign‑off backed by a green Windows UI/device validation. Not yet demonstrated (see CI).

CI status (head 23594dbd)

  • Windows‑relevant legs are GREEN: Build Windows (Debug + Release), Pack Windows, Helix Unit Tests Windows (Debug + Release), and both UI‑test sample‑app builds — Windows (default CV2) and Windows CV1 — all succeeded. So this PR's Windows build path, including the new handler edit, compiles on both handler modes.
  • maui-pr FAILED — non‑Windows only. Build macOS (Debug) failed with error MSB4024: … Microsoft.Maui.Resizetizer.After.targets could not be loaded / Could not find file — a build‑tasks infrastructure failure (generated targets not present), not a code defect. The AOT‑macOS and RunOniOS_MauiRelease*/TrimFull* integration‑test failures cascade from that macOS build leg.
  • maui-pr-devicetests FAILED — non‑Windows only. Build DeviceTests succeeded; the Android (Mono) and MacCatalyst (Mono) run legs failed. No Windows device‑test leg ran (DeviceTestsWindows skipped), so the historical Windows‑Controls discovery‑crash concern is again not exercised.
  • maui-pr-uitests pending — failing legs so far are Android/iOS/macOS Shell; no Windows UI‑test run leg present/complete in this build (only the Windows sample‑app builds passed).

Blast radius & failure‑mode probing

This is a default‑on change to the Windows CollectionView handler, so blast radius is real. However every red leg is on iOS / Android / macOS — platforms the change cannot reach: the handler registration is behind #elif WINDOWS + RuntimeFeature, and the modified file is *.Windows.cs (Windows TFM only). The eng/cake/dotnet.cake edit only adds a gated usewindowscv1 UI‑test argument and does not alter normal builds. The macOS MSB4024 is a missing‑generated‑targets infra failure. Net: the red CI is not attributable to this PR's code, but it does mean there is no clean green Windows UI/device run in this build to back the default‑on flip.

Concise findings

  • MapIsEnabled is now correctly private — public‑API concern closed.
  • ✅ Default‑on plumbing is clean, Windows‑gated, trimmer‑aware.
  • ⚠️ Merge‑readiness still blocked by red CI; classify the failing legs (macOS build = infra MSB4024; Android/iOS/macOS device/UI = pre‑existing/flaky) and re‑run, then confirm a green Windows UI/device validation before flipping default‑on.

Confidence

  • High that the two commits equal the diff above and the MapIsEnabled change is behavior‑preserving and removes it from the public surface (verified against PublicAPI.Unshipped.txt).
  • High that the failing CI legs are all non‑Windows and therefore not caused by this Windows‑gated change; the macOS failure is clearly infra (MSB4024).
  • Medium that the Android/iOS/macOS device/UI failures are purely pre‑existing/flaky (consistent with platform isolation, but not each individually root‑caused here).

Non‑approval disclaimer: This is an automated assistant review for the net11.0 fleet refresh. It is not an approval and does not request changes — a human maintainer owns the merge decision. No code was modified.

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review Summary — PR #34600 [WinUI][CV2] CollectionView2 Handler (Windows)

Verdict: NEEDS_CHANGES (confidence: medium)

Synthesized from 3-of-4 model reviews (opus-4.8 review incomplete) — gpt-5.5 (NC), opus-4.6 (NC), gemini (ND) — clustered into 7 findings, each validated against the code at 23594dbde96784d6f9deab6eda05b52483ce09a5. One confirmed correctness error in the grouped drag-reorder path drives the NEEDS_CHANGES verdict; two confirmed edge-case warnings and four dropped findings (1 false positive, 3 intentional/already-mitigated nits).

Confirmed findings (3 inline comments)

  1. (error) Grouped same-group drag-reorder resets scroll/containersGroupedItemTemplateCollection2.HandleGroupItemsMove (line 362) fires CollectionChanged(Move) on the flat collection that backs CollectionViewSource.Source. CsWinRT maps Move -> VectorChanged(Reset), clearing all containers and scrolling to top. The flat sibling ObservableItemTemplateCollection2.MoveItem deliberately fires Remove+Add to avoid this; the grouped path must do the same. Verified: _collectionViewSource.Source is GroupedItemTemplateCollection2 in ItemsViewHandler2.Windows.cs, and the codebase's own comments document the Move -> Reset mapping.
  2. (warning) Stale IsHeaderOrFooter on recycled wrappersItemFactory.cs:143 reads wrapper.IsHeaderOrFooter, but it is only assigned on new-wrapper creation. With a pool keyed by DataTemplate, a shared header/item template (or selector) lets a recycled container keep the wrong role (non-selectable item, or selectable header).
  3. (warning) IsItemsSourceGrouped over-flattens non-grouped sourcesItemsViewHandler2.Windows.cs:362 flattens whenever the first item is IEnumerable (non-string), regardless of IsGrouped. Collection-typed items (List<int[]>, List<byte[]>, List<ObservableCollection<T>>) are silently flattened when IsGrouped == false.

Dropped findings

  • (gpt-5.5) IndexOfItem duplicate-item corruptionfalse positive. IndexOfItem is two-pass: ReferenceEquals first (correctly distinguishes value-equal but distinct reference items, e.g. duplicate records), Equals only as a fallback for value types — where any equal index is harmlessly interchangeable. The model mischaracterized it as Equals-first.
  • (gemini x2) Handler generic bounds (ReorderableItemsViewHandler2<TItemsView> : ItemsViewHandler2<ReorderableItemsView>; CollectionViewHandler2 : ReorderableItemsViewHandler2<ReorderableItemsView>) — compiles and mirrors the established CV1 handler pattern; design-consistency suggestion, not a defect.
  • (gemini) GetMethod("Move") reflection (MauiItemsView.DragDrop.cs:554) — already mitigated: fast path for ObservableCollection<object>, cached MethodInfo, IL2070 suppressed with justification; Move is a preserved public framework API used only during interactive drag/drop. Worth a second look per MAUI trim guidelines but non-blocking.

CI

Red. maui-pr (build + AOT/iOS integration tests), maui-pr-uitests (Android/iOS/macOS/MacCatalyst Shell & CollectionView legs), maui-pr-devicetests (MacCatalyst), and Build Analysis are failing. All failing legs are non-Windows, consistent with this Windows-only change (per opus-4.6); they still must be confirmed green/known-flaky via the CI investigator before merge — do not treat as LGTM.

_suppressNotifications = false;

OnCollectionChanged(new NotifyCollectionChangedEventArgs(
NotifyCollectionChangedAction.Move, movedItems, newIndex, oldIndex));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same-group drag-reorder in a grouped CollectionView2 will jump scroll to the top and recycle every container.

HandleGroupItemsMove fires OnCollectionChanged(Move) on this flat collection, which is the CollectionViewSource.Source bound to ItemsRepeater (see ItemsViewHandler2.Windows.cs where _collectionViewSource.Source is GroupedItemTemplateCollection2). CsWinRT translates CollectionChanged(Move) into VectorChanged(Reset), so ItemsRepeater clears all realized containers and resets scroll position.

This is exactly the failure mode the flat-list sibling avoids: ObservableItemTemplateCollection2.MoveItem deliberately overrides MoveItem to raise Remove + Add instead of Move (with a detailed comment), and TryMoveObservableCollection documents the same CsWinRT Move -> Reset mapping. The grouped path re-introduces the bug.

Fire Remove + Add here to match the flat path:

OnCollectionChanged(new NotifyCollectionChangedEventArgs(
    NotifyCollectionChangedAction.Remove, movedItems, oldIndex));
OnCollectionChanged(new NotifyCollectionChangedEventArgs(
    NotifyCollectionChangedAction.Add, movedItems, newIndex));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kubaflo HandleGroupItemsMove may raise a Remove + Add instead of a Move. While this behavior is not reproducible in the current implementation, a defensive safeguard has been added to align with the flat-list pattern. This is because CollectionChanged (Move) can theoretically translate to VectorChanged (Reset) via CsWinRT when the source groups are ObservableCollection.

// Must be set every time to handle recycled containers correctly.
if (wrapper is not null)
{
bool isHeaderOrFooter = wrapper.IsHeaderOrFooter;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wrapper.IsHeaderOrFooter is read here but only ever assigned in the if (wrapper is null) new-wrapper branch above. A wrapper pulled from _recyclePool keeps the role it had when first created. Because the pool is keyed by DataTemplate, this is wrong whenever a group header/footer and a regular item resolve to the same template instance (a shared DataTemplate, or a DataTemplateSelector returning the same template for both). A recycled container can then keep NonSelectableItemContainerTemplate for a real item (making it non-selectable) or swap in a selectable template for a header.

Refresh the flag from the current context before reading it, e.g. wrapper.IsHeaderOrFooter = templateContext.IsHeader || templateContext.IsFooter; for recycled wrappers too. The adjacent comment already states the template swap "Must be set every time to handle recycled containers correctly" — but it relies on a flag that is not refreshed on recycle.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kubaflo I have tried the scenario, issue is not reproduced on recycling the templated ,The fix is not applied , let me know if any further clarification or suggestion on this

var flattenedSource = itemsSource;
if (itemsSource is not null && IsItemsSourceGrouped(itemsSource))
{
flattenedSource = FlattenGroupedItemsSource(itemsSource);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This else branch flattens the source whenever IsItemsSourceGrouped(itemsSource) is true, independent of GroupableItemsView.IsGrouped. IsItemsSourceGrouped only checks whether the first item implements IEnumerable (excluding string), so a non-grouped source whose items are themselves collections — e.g. List<int[]>, List<byte[]>, List<ObservableCollection<T>> — is silently flattened into its inner elements even when IsGrouped == false. The user's intended single-item-per-collection binding is lost with no error.

Consider gating the flatten on IsGrouped, or tightening IsItemsSourceGrouped (e.g. exclude Array/byte[] or require IsGrouped).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kubaflo The approach—checking whether the first item is an IEnumerable and not a string—is applied consistently across all platforms, without special handling for arrays or other collection types.
The else branch must still perform flattening even when IsGrouped is false. This is necessary because, if a user toggles IsGrouped from true to false at runtime, the data source remains grouped. Flattening in this case ensures the correct flat representation is displayed. Removing this logic would break that toggle scenario. This was intentional to display only item and not the group header , I prefer to keep it this way

@vishnumenon2684

Copy link
Copy Markdown
Contributor

/azp run maui-pr-uitests , maui-pr-devicetests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@kubaflo

kubaflo commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Re-review PR #34600 — CollectionView2 Windows handler (new head 036d05b)

Verdict: NEEDS_DISCUSSION (confidence: medium) — the previous blocking error is fixed; two non-blocking warnings from the last review remain (in files untouched by this commit).

✅ Fixed — the scroll-jump/container-recycle bug (was the ❌ error)

GroupedItemTemplateCollection2.HandleGroupItemsMove no longer fires CollectionChanged(Move) (which CsWinRT translates to VectorChanged(Reset), discarding all realized containers and resetting the scroll offset). It now fires Remove + Add, which ItemsRepeater handles by repositioning only the affected containers while preserving the ScrollViewer offset — matching the flat-list fix in ObservableItemTemplateCollection2.MoveItem. The added comment explains the rationale clearly. This resolves the same-group drag-reorder regression I flagged. 👍

⚠️ Still open (non-blocking warnings, files unchanged this push)

  1. ItemFactory.cs:143wrapper.IsHeaderOrFooter is read but only assigned in the new-wrapper branch; a wrapper pulled from _recyclePool keeps its previous role. Worth resetting the role on reuse. (see existing inline comment)
  2. ItemsViewHandler2.Windows.cs:362 — the else branch flattens the source whenever IsItemsSourceGrouped(itemsSource) is true, independent of GroupableItemsView.IsGrouped; IsItemsSourceGrouped only inspects the first item. (see existing inline comment)

Neither is a hard blocker — flagging so they're not lost. With the error resolved, this is close; addressing (or consciously deferring) the two warnings would clear it. CI reds are the usual unrelated flakes.

Multi-model review (gpt-5.5 · opus-4.8 · opus-4.6 · gemini-3.1-pro). Comments only — not a formal approval.

@PureWeen PureWeen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adversarial multi-model review — Round 3

Methodology: 3 independent reviewers (different model families) reviewed the PR in parallel; findings were cross-validated with adversarial consensus, and single-reviewer findings went through a dispute round. Each finding below was verified against the actual source. CI run status is intentionally out of scope for this review.

Context for this round

The headline change since the last review is that CV2 is now the default Windows CollectionView handler (RuntimeFeature.IsWindowsCollectionView2HandlerEnabledByDefault = true; AppHostBuilderExtensions registers CollectionViewHandler2 unless UseWindowsCollectionView2Handler=false). Every existing Windows MAUI app now gets this handler by default, so the findings below were assessed knowing any latent bug ships to all Windows apps. (Note: the PR description still describes the handler as "opt-in / gated behind the runtime feature flag" — that text is now stale.)

All 7 findings from Round 1 (#pullrequestreview-4310879278) and Round 2 (#pullrequestreview-4403329496) are resolvedGetVisibleIndexes absolute indexing, the drag-drop duplicate-item row, SelectDataTemplate container, ItemFactory GoToState gating, the test-csproj CV2 force / dedicated CV1 lane, the ItemsViewStyles theme leak (now scoped per-instance), and the GroupableUniformGridLayout O(N) hot path (now realization-windowed). Nice progress.

Findings this round

❌ Must-fix

  1. GroupableUniformGridLayout band cache is always built from the 48/36 px defaults, not the measured sizes — grouped grids clip and the list tail is unreachable, and RebuildBandCache runs O(N) every layout pass. A new defect in the reworked band cache (distinct from the now-fixed O(N) loop). 3/3. (inline)
  2. A null element in a non-observable IList source throws on realization — scroll-into-view crash; CV1 rendered a blank cell, and this PR's other two source paths don't throw. 3/3. (inline)

⚠️ Should-fix
3. Stale IsHeaderOrFooter on recycled containers — this corroborates @kubaflo's open thread on ItemFactory.cs:143. The flag is assigned only inside the wrapper is null creation block (line 94), never on recycle. The reason it didn't reproduce is that the trigger is narrow: it needs a grouped list where the group header/footer DataTemplate is the same instance as the item DataTemplate (the recycle pool is keyed by template, so distinct templates never cross-recycle). When hit, the regular item becomes non-selectable and is arranged as a full-width band, and vice-versa. Setting the flag unconditionally on every GetElement is the minimal, strictly-more-correct fix. 2 reviewers ❌, 1 ⚠️ after dispute.
4. MeasureFirstItem size cache can be polluted by a recycled first container — narrow (non-default strategy + async-sized content + reuse-without-unload) and grows-never-clips, but a real contract violation. 2/3 after dispute. (inline)
5. Duplicate ui-tests-samples-windows_failed pipeline artifact on the default Windows failure path → the second publish errors. 3/3. (inline)

💡 Consider
6. ui-tests-samples-material3-coreclr_failed publish dropped (now asymmetric with its retained success block) — dormant today (no CoreCLR+Material3 lane exists). Noted inline on #5.

Test coverage

The changed handler paths are exercised by the existing Windows CollectionView UITest suite, and this PR adds a dedicated CV1 lane so both handlers stay covered — good. However, none of the five findings above appears to have a dedicated regression test: grouped-grid sizing (#1), null-item sources (#2), header/item shared-template recycling (#3), and MeasureFirstItem + async images (#4) are all plausible UITest/device-test additions worth adding given CV2 is now the default Windows handler.

Prior review status

Round 1 / Round 2 threads are resolved (the Round-1 test-csproj threads are moot — those files were reverted). @kubaflo's ItemFactory.cs:143 thread (finding #3 above) and GroupedItemTemplateCollection2 drag-reorder thread remain open; #3 corroborates the former with the precise trigger.

bool changed = UpdateEstimates(isVerticalOrientation, maxMeasuredRegular, maxMeasuredHeader);
if (changed)
{
InvalidateBandCache();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Logic / Performance — The band cache is always built from the default estimates (48/36 px), never the measured item sizes, so grouped grids clip and can't scroll to the end.

UpdateEstimates(...) (line 76) grows _estimated*Extent to the just-measured size, but the very next call InvalidateBandCache() (line 79; body at lines 438–448) resets those estimates back to DefaultEstimatedRegularExtent/DefaultEstimatedHeaderExtent (48/36). EnsureBandCacheRebuildBandCache then reads them via GetEstimatedPrimarySize() and builds every band at 48/36. For a grouped GridItemsLayout (the only consumer — ItemsViewHandler2.Windows.cs:773-775) whose items/headers are taller than 48/36 — i.e. essentially every real template, now that CV2 is the default Windows handler:

  • ArrangeOverride squishes each item into a 48 px band → content clipped/overlapping.
  • MeasureOverride returns _cachedTotalPrimaryExtent computed from 48 px bands → scroll extent is ~half the real height, so the tail of the list is unreachable.
  • Because measured (e.g. 100) is always > the reset estimate (48), changed is true on every measure/arrange pass → RebuildBandCache (O(ItemCount)) runs every layout pass, re-introducing the O(N)-per-pass cost the prior round eliminated.

Fix: separate "invalidate cache" from "reset estimates to default." In the grow paths (Measure 77–81, Arrange 170–173) invalidate without resetting estimates (_bandCacheValid = false;) so the rebuild uses the grown values; reset to defaults only on a genuine fresh recompute (OnItemsChangedCore and the ItemCount == 0 branch).

Secondary (surfaces once the above is fixed): element.Measure(availableSize) at line 61 measures regular items at the full viewport width, but ArrangeOverride arranges them at the per-span cell width (line 112/158), so wrapping content reports too-small a height at measure time. Measure regular items with the computed cell cross-axis size.

Flagged by: 3/3 reviewers; root cause verified against source. New defect in the reworked band cache — distinct from the now-fixed O(N) loop.

{
var item = _itemsSource[index];
if (item is null)
throw new InvalidOperationException($"Item at index {index} is null. ItemTemplateContext2 requires a non-null item.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Regression — A null element in a non-observable IList source throws and crashes the layout pass; CV1 rendered a blank cell.

This indexer throws InvalidOperationException when _itemsSource[index] is null. It fires on lazy realization (when ItemsRepeater scrolls that index into view), so it is a scroll-into-view crash in the now-default Windows handler. CV1's ItemTemplateContextList passes null straight through to a null BindingContext (blank cell). The guard is also inconsistent with this PR's sibling paths: ItemTemplateContextEnumerable2 and ObservableItemTemplateCollection2 both construct ItemTemplateContext2 with the raw item and never null-check, and ItemTemplateContext2.Item is typed object?. Downstream already tolerates null — ItemFactory.GetElement does view.BindingContext = templateContext.Item ?? _view.BindingContext;.

Trigger: a List<T>/array ItemsSource containing a null element (null placeholders, async-populated lists, holey data). Fix: drop the guard and let null flow to the existing ?? _view.BindingContext, matching CV1 and the other two source paths.

Flagged by: 3/3 reviewers (2 ❌, 1 borderline ⚠️→❌); regression confirmed against CV1 source.

}
}
// Subscribe once
content.SizeChanged += OnContentSizeChanged;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ LogicMeasureFirstItem size cache can be polluted by a recycled first container.

OnContentSizeChanged is wired here (line 386) on the first measured item's content to grow the cached size when async images load, and is unwired only on content.Unloaded (lines 376–384). RecycleElement doesn't unwire it, and the captured content FrameworkElement survives recycle (reuse takes the wrapper is not null path and skips SetContent). So if that first container is recycled and rebound to a later item without an intervening Unloaded (ItemsRepeater's measure churn — see the comment at lines 187–190 — or fast continuous scroll), a later item's image load fires OnContentSizeChanged and grows the global first-item cache to that later item's size, which then pins every item via MeasureFirstItem. It only grows (never clips), so the symptom is items larger than intended plus reflow jank.

Narrow trigger (non-default MeasureFirstItem + async-growing content + reuse-without-unload), and Unloaded-on-recycle self-heals the common case — but it is a real contract violation. Fix: gate the growth inside OnContentSizeChanged on this wrapper still being the pinned/first item before calling SetCachedFirstItemSize (most local), or unwire on recycle.

Flagged by: 2/3 reviewers after dispute; mechanics verified against source.

condition: and(ne('${{ parameters.platform }}' , 'windows'), eq('${{ parameters.runtimeVariant }}' , 'CoreCLR'), eq('${{ parameters.useMaterial3 }}', 'true'), failed())
artifact: ui-tests-samples-material3-coreclr_failed_$(System.JobAttempt)
condition: and(eq('${{ parameters.platform }}' , 'windows'), eq('${{ parameters.useWindowsCV1 }}', 'false'), failed())
artifact: ui-tests-samples-windows_failed_$(System.JobAttempt)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ Config Impact — Duplicate pipeline-artifact name on the default Windows failure path.

This new block publishes ui-tests-samples-windows_failed_$(System.JobAttempt) for useWindowsCV1=false + windows + failed. The pre-existing catch-all just below (lines 149–151, and(platform=windows, failed())) publishes the same artifact name. useWindowsCV1 defaults to false, so on any default Windows build-sample failure both conditions are true and the second publish errors (An artifact with the name … already exists), polluting failure diagnostics on an already-failed job.

Fix: remove the now-redundant catch-all at lines 149–151 — the two specific blocks (142–143 for cv1=false, 146–147 for cv1=true) already cover all Windows failure cases.

Separately (💡, dormant): the old ui-tests-samples-material3-coreclr_failed publish was repurposed into the block above, so a CoreCLR+Material3 build failure now publishes no artifact while its success counterpart (lines 112–114) remains. No live impact today — the only Material3 stage builds with runtimeVariant: Mono, not CoreCLR — but the lane is now asymmetric.

Flagged by: 3/3 reviewers.

@vishnumenon2684

Copy link
Copy Markdown
Contributor

/azp run maui-pr-uitests , maui-pr-devicetests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review Summary — PR #34600 [WinUI][CV2] CollectionView2 Handler (Windows)

Verdict: NEEDS_CHANGES (re-review at head 332324f, prior verdict NEEDS_DISCUSSION at 036d05b)

Synthesized from 4 independent model reviews (gpt-5.5 = NEEDS_CHANGES, gemini = NEEDS_CHANGES, opus-4.8 = NEEDS_DISCUSSION, opus-4.6 = LGTM dissent). Each finding was validated against the actual code at HEAD.

What changed

This head ("Fix for phase3 review") resolves the prior round's blockers, and all four models confirm them fixed against the code: GroupableUniformGridLayout now measures items at their per-cell cross-axis size and preserves grown band estimates (sets _bandCacheValid = false instead of resetting to the 48/36 px defaults) with a windowed GetRealizationIndexRange; ItemFactory now refreshes IsHeaderOrFooter on recycled wrappers (assigned unconditionally in GetElement, L154) and explicitly syncs Selected/Normal visual state on every rebind; and the non-observable list path (ItemTemplateContextList2) now tolerates null items (Count = source.Count + item!), rendering them as blank cells to match CV1.

Consensus finding

Three of four models (gpt-5.5 and gemini at error, opus-4.8 at warning) independently flag the same remaining gap at ObservableItemTemplateCollection2.cs: the observable path still skips null source items in PopulateInitialItems (L78), Add (L225), Replace (L426) and Reset (L451). Validated against HEAD: the class extends ObservableCollection<ItemTemplateContext2> and its incremental handlers index off source-derived positions (args.NewStartingIndex/OldStartingIndex), so dropping a null desyncs the wrapper from the source and a subsequent Insert(startIndex + index)/Items[itemIndex] goes out of range — a reproducible ArgumentOutOfRangeException (source {null,"A"} then Add("B")Insert(2) on a 1-element collection). Because ItemTemplateContext2.Item is object? (wrapping null is safe and renders blank per ItemFactory's own comment), ItemTemplateContextList2 retains nulls, and CV1's ObservableItemTemplateCollection wraps nulls unconditionally, the observable path is the lone outlier and contradicts the PR's own null-support design. The author fixed the list path but left the same bug in the observable path. opus-4.6 (LGTM) did not examine the observable path, hence its dissent.

Rationale & note

Verdict is NEEDS_CHANGES: a latent crash inside the PR's own declared null-support scope, confirmed by code inspection and flagged at error severity by two models. Separately, opus-4.6 asks whether IsWindowsCollectionView2HandlerEnabledByDefault = true should ship enabled. This is not repeated as an inline finding because it is already tracked in the thread — the author confirmed on May 21 it is intentional for the review phase and "will be disabled accordingly" before merge, and a prior Round-3 comment already noted the now-stale "opt-in" PR description. It remains an open pre-merge gate (the flag is still true and the description still says opt-in at this head) but needs no new comment.

Items.Clear();
foreach (var item in _itemsSource)
{
if (item is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This observable path skips null source items (here in Reset, and identically in PopulateInitialItems L78, Add L225, and Replace L426), so the wrapper's indices stop matching the source's. The incremental handlers index off source-derived positions, so a null preceding a later INCC update sends those indices out of range and throws ArgumentOutOfRangeException — e.g. source {null,"A"} then Add("B") fires NewStartingIndex=2 and calls Insert(2) on a 1-element collection. Wrap null items in ItemTemplateContext2 (its Item is object? and renders a blank cell) to keep 1:1 index parity, matching ItemTemplateContextList2, ItemFactory's documented blank-cell behavior, and CV1's ObservableItemTemplateCollection, which all retain nulls.

@SuthiYuvaraj

Copy link
Copy Markdown
Contributor Author

@PureWeen Thanks — I addressed the product-code issues from this round in the latest update.

  • GroupableUniformGridLayout now measures regular items using the computed cell cross-axis size instead of the full viewport, and when estimates grow it invalidates the band cache without resetting the measured estimates. That allows the rebuilt cache to use the measured extents instead of falling back to the 48/36 defaults.
  • ItemFactory now refreshes IsHeaderOrFooter on every bind, not just during wrapper creation, so recycled containers don’t retain stale header/footer state.
  • The first-item SizeChanged observer is now unwired during recycle, so a reused wrapper can’t continue mutating the global first-item size cache after it has been rebound.
  • ItemTemplateContextList2 no longer throws for null entries coming from a non-observable IList source, which avoids the realization-time crash called out in the review.

@vishnumenon2684

Copy link
Copy Markdown
Contributor

/azp run maui-pr-uitests , maui-pr-devicetests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@PureWeen PureWeen added the p/0 Current heighest priority issues that we are targeting for a release. label Jun 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution p/0 Current heighest priority issues that we are targeting for a release. partner/syncfusion Issues / PR's with Syncfusion collaboration platform/windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants