Repository navigation
[Android] CollectionView: Defer collection changes during native layout - #39495
jfversluis wants to merge 3 commits into
Conversation
Queue adapter snapshot mutations and notifications together while RecyclerView computes layout, preserving FIFO ordering and synchronous changes outside layout. Preserve grouped child sources and handle non-list observables, range changes, temporary EmptyView swaps, disposal, and native reordering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b474211-673f-4256-919f-ecdce2fa6611
GitHub Actions
Inactive
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 39495Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 39495" |
GitHub Actions
Inactive
This comment has been minimized.
This comment has been minimized.
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
GitHub Actions
Inactive
This comment has been minimized.
This comment has been minimized.
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
area-controls-collectionview
GitHub Actions
Inactive
github-actions
Bot
added
the
s/agent-review-in-progress
Retain incremental group source tracking while incorporating the upstream first/last empty-group notifications. Add real-layout coverage for root and child EmptyView transitions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b474211-673f-4256-919f-ecdce2fa6611
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Avoid binding the callback during handler setup before its RecyclerView capture is assigned. Populate the initially empty source after initialization and keep all explicit layout and snapshot assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4b474211-673f-4256-919f-ecdce2fa6611
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
|
CI follow-up for head
Coverage gap: the macOS Cells/CheckBox/ContextActions/CustomRenderers/DatePicker/Dispatcher/DisplayAlert/DisplayPrompt/DragAndDrop shard hit its 180-minute job limit during Install .NET, before its tests ran (setup log). This is not a passing test result. The macOS ScrollView tests completed successfully. The reporter has been invited to try the draft artifacts. This PR remains draft; CI is not entirely green. Review and reporter-feedback monitoring continues. |
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Follow-up to the automated test report above: two listed failures recovered in the same-head category retries. The requested snapshot comparison was performed during the earlier investigation: Windows CharacterSpacing differs in the displayed FocusStatus: Focused value versus the blank baseline; the requested character spacing is applied. Mac GroupedCollectionViewItems renders both groups and all four expected items, with differences on header glyph/divider/window edges. Its fixture populates before binding and does not mutate afterward; the Apple item-source implementation does not construct the modified MarshalingObservableCollection. These observations are causal evidence, not claims of exact target-branch matches. For the remaining Android password test, the actual failure hierarchy has the requested selection 3–8 applied to the focused password Entry. The keyboard is open and the lower cursor inputs are no longer visible. The test uses the conditional hide helper rather than the focus-aware helper (test/helpers); navigation and the update itself succeeded. No Entry change is proposed here. The report correctly retains limitations: no runtime-matched baseline proof for the safe-area failures, no evaluated-compile-input proof for the macOS Debug sample build failure, and no test results for the shard canceled during SDK installation. Those are not being represented as green coverage. The earlier CI follow-up records the successful device coverage and the setup-timeout gap. |
|
2 similar comments
|
|
|
1 similar comment
|
github-actions
Bot
added
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Tests Failure Analysis
🧪 CI Analysis — click to expand📊 maui-prNo failures found. 🧪 maui-pr-devicetestsNo failures found. 🧪 maui-pr-uitests
🧭 Follow-up — actions and refreshNext action: Restore omitted UI results/logs; inspect initial Android setup errors and unresolved snapshots before attributing them.
|
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
Binding a CollectionView item can synchronously raise a collection change. In the reported reproduction, TimePicker binding triggers sorting via
ObservableCollection.Movewhile RecyclerView is computing layout. Mutating the adapter snapshot and issuing its native notification at that point throwsIllegalStateException.Add an adapter-owned, layout-aware FIFO dispatcher shared by observable snapshots, grouped roots, and grouped children. Defer the snapshot mutation and corresponding notification together until native layout finishes; normal main-thread changes remain synchronous. Posting only the native notification is insufficient because RecyclerView would still read an already-mutated snapshot during layout.
Preserve unaffected grouped child sources during incremental root changes so pending child notifications survive. Retain the gate through temporary EmptyView swaps, drop disposed-source work, and prevent native drag reorder from overtaking queued changes. Snapshot non-IList observables too; events without usable indices capture an event-time reset instead of absorbing later queued mutations. Correct multi-item move ordering and affected-range counts. No public API additions.
Merged current
mainwithout rewriting history to incorporate #37954. The resolution preserves upstream grouped EmptyView first/last-empty-group transitions while retaining incremental child tracking. Both models reviewed this integration, and an additional native-layout regression test covers it.Runtime correctness: both actions crash in the unchanged reporter application with
UseWorkaround=falseon MAUI 10.0.110 and the original source baseline. After the merge, Edit (1x) and Edit 3x fast both pass, the application stays alive, and native row order is D, B, C, A, E. Tested on an Android 15/API35 ARM64 emulator; the reporter's Android 17 environment was not available. Exact Controls DLL payloads were verified inside all A/B APKs.Validation:
15 passed after merging main, covering existing marshaling behavior, multi-item moves, and event-time resets for indexless changes followed by indexed changes.
Initial filtered build passed. Subsequent exact-binary A/B APKs were repackaged using:
Merged build passed, with an existing unrelated SwipeView analyzer warning. The session-local target substitutes the preserved Controls DLL; embedded payloads were verified before execution.
19 passed, 0 failed after merging main. Before the integration test was added, the same 18-test suite against the payload-verified original source baseline produced 17 failures, chiefly native layout exceptions; normal synchronous behavior passed. Cases include binding-triggered mutation, FIFO mixed changes, grouped roots/children and bulk ranges, non-list observables, EmptyView transitions, reorder, and disposal. Explicit user 0 bypasses a local XHarness invocation problem (
Invalid userId -2) that otherwise prevents tests from running.Merged build passed: zero errors, three Gradle Java-8 obsolescence warnings. Original scoped repository formatting verification exited 0 (workspace reference warnings); conflict-resolution whitespace verification also exited 0. Task-owned changes pass
git diff origin/main --check; unrelated whitespace already on main was not modified.Runtime performance: no per-bind/per-scroll queue checks were added. Indexed normal-path notifications avoid a redundant dispatch closure; bulk grouped updates use range operations instead of repeated shifts.
Earlier short studies were inconclusive due to host noise and remain preserved. After merging main, the 400-move/1,200-scroll protocol cleared scrolling but left move timing inconclusive. A separately predefined higher-duration move protocol changes only
MovesPerPhase=200to2000, keeping scrolling at 1,200 calls. It retains all 8 counterbalanced pairs (16 launches, each with 3 warmups and 7 measured rounds), with 2,000 fixed-height rows, 4,000 moves and 1,200 native scroll calls per round. Baseline and merged variants use identical harness and frozen current-main Core/Essentials/Graphics dependencies. No rounds were excluded or optional stopping used. Analysis uses paired launch medians, not correlated rounds as independent samples.These satisfy the predefined +5% non-regression screen for this longer workload. Managed move allocations fell 392 → 232 bytes/move (-40.82%), or 1,568,000 → 928,000 bytes per 4,000-move round. Managed scroll allocations increased 0.91%, within the screen. Across both variants there were 448,000 measured moves, 134,400 measured scroll calls, and 13,888 passing rendered-order checkpoints. All launches completed without fatal/layout exceptions. The earlier 400-move study remains inconclusive rather than being replaced or presented as passing.
All session-local harnesses, raw results, host observations, hashes, paired analyses, and earlier studies are preserved on the external volume. Final post-merge reports are
perf-results-merged-long8/REPORT.mdandperf-results-merged-moves8/REPORT.mdunder/Volumes/NieuwVolume/maui-39084-runtime/; both protocols completed, with the longer-move protocol meeting the unchanged screen for all timed metrics.Limitations: Debug APKs, API35 emulator, simple nongrouped fixed-height rows. This does not establish physical-device, Release/AOT, grouped-template, native-memory, or frame-tail/jank performance. Final merged Controls SHA-256:
351cf969c5d1720fae1282e944a7b7fa39d95515e12379afad310c4aa18ae425.Independent multi-model design/review findings were addressed before final runtime verification, including after the main merge. No other open PR mentioning this issue was found when initially publishing. Kept in draft pending CI and reporter validation.
Issues Fixed
Fixes #39084