Fix CollectionView record struct selection on Windows#33488
Conversation
|
@hartez WDYT? |
It took me a bit to figure out what was going on here, but yeah, this change makes sense. Because ListViewBase's public record struct TestRecordStruct(string Name);
...
var x = new TestRecordStruct("test");
var y = x;
var ox = x as Object;
var oy = y as Object;
Console.WriteLine($"{ox == oy}"); // False
Console.WriteLine($"{object.Equals(ox, oy)}"); // TrueI think it's fine, but I don't work on MAUI anymore, so we should tag in @PureWeen or @mattleibow. |
|
Ah yup. I just checked the blame and tagged you... Thanks for the review all the same. Much appreciated! |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33488Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33488" |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33488 | Use object.Equals instead of == when matching SelectedItem in Windows SelectableItemsViewHandler |
⏳ PENDING (Gate) | src/Controls/src/Core/Handlers/Items/SelectableItemsViewHandler.Windows.cs |
Original PR |
🚦 Gate — Test Verification
Gate Result: ⚠️ SKIPPED
Result: ❌ SKIPPED
Platform: android
Mode: Full Verification
- Tests exist: ❌
- Android affected/applicable: ❌
- Tests FAIL without fix: ❌
- Tests PASS with fix: ❌
Notes
- The linked bug in CollectionView selection is ineffective with record structs on Windows #33487 is Windows-only; Android is not an affected validation platform for this issue.
- PR Fix CollectionView record struct selection on Windows #33488 changes only
src/Controls/src/Core/Handlers/Items/SelectableItemsViewHandler.Windows.csand adds no UI, device, or unit tests. - Because no applicable test exists, FAIL-without-fix / PASS-with-fix verification could not be executed.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) | Centralize selection comparison with ItemEquals(...) and also fix UpdateItemContentControlSelection() |
1 file | Found an additional == comparison in visual selection state that the PR does not touch |
|
| 2 | try-fix (claude-sonnet-4.6) | Drive single selection by SelectedIndex and compute visual state from ListViewBase.SelectedItems; add a Windows device test |
2 files | More invasive but covers both logic and missing test coverage | |
| 3 | try-fix (gpt-5.3-codex) | Minimal extension of the PR: keep current UpdatePlatformSelection() change and also use object.Equals(...) in UpdateItemContentControlSelection() |
1 file | Simplest complete alternative identified | |
| 4 | try-fix (gemini-3-pro-preview) | Use EqualityComparer<object>.Default.Equals(...) at both single-selection comparison sites |
1 file | Equivalent coverage with a slightly different equality API | |
| 5 | try-fix (gemini-3-pro-preview, cross-pollination) | Explicitly select the exact native item instance via value equality and also fix visual selected-state comparison | 1 file | Functionally similar to #3 but more verbose | |
| PR | PR #33488 | Replace == with object.Equals(...) in UpdatePlatformSelection() only |
1 file | Original PR; does not address the remaining UpdateItemContentControlSelection() comparison |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | No | NO NEW IDEAS |
| claude-sonnet-4.6 | 2 | No | NO NEW IDEAS |
| gpt-5.3-codex | 2 | No | NO NEW IDEAS |
| gemini-3-pro-preview | 2 | Yes | Explicitly select the exact native object instance matching the target value |
Exhausted: Yes
Selected Fix: Candidate #3 — smallest change that appears to cover both affected single-selection comparison sites without changing handler flow.
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Windows-only issue; PR changes one Windows handler file and adds no tests |
| Gate | Android was requested but is not an affected platform; no applicable automated test exists | |
| Try-Fix | ✅ COMPLETE | 5 attempts, 0 runtime-validated due Linux host / Windows-targeting blocker |
| Report | ✅ COMPLETE |
Summary
PR #33488 correctly identifies one broken equality site in UpdatePlatformSelection(), but the review found a second single-selection comparison in UpdateItemContentControlSelection() that still uses == and appears subject to the same boxed-value/reference-equality problem. Because the PR also adds no Windows-focused coverage, the current review cannot verify that the change fully fixes #33487 or would catch regressions.
Root Cause
This Windows handler mixes reference equality (==) into single-selection logic for values that may be boxed record structs. Boxed value types compare unequal by reference even when their values are equal. The PR fixes that in native-item matching inside UpdatePlatformSelection(), but the visual selected-state path in UpdateItemContentControlSelection() still compares ItemsView.SelectedItem with FormsDataContext using ==.
Fix Quality
The current PR is directionally correct but likely incomplete. The best alternative found was a minimal extension of the PR: keep the object.Equals(...) changes in UpdatePlatformSelection() and also switch the remaining UpdateItemContentControlSelection() single-selection comparison to value equality. A Windows device test covering record-struct single selection would also be needed so Gate can validate FAIL-without-fix / PASS-with-fix behavior on an affected platform.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the AI's summary?
Compare collection items with value equality Fixes: dotnet#33487
b6ab9da to
03bf678
Compare
Technically, the AI is correct and we should change it. I don't think that it matters in practice though since reference equality happens to work OK there. However, there is another case that does matter that it missed - in ItemContentControl it needs value equality for the initial selection when the control is realized. Not sure if you require a test here, or whether you're OK with it just not breaking existing tests? |
### Description of Change Compare collection items with value equality ### Issues Fixed Fixes: #33487 <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
### Description of Change Compare collection items with value equality ### Issues Fixed Fixes: dotnet#33487 <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
Description of Change
Compare collection items with value equality
Issues Fixed
Fixes: #33487